Repository navigation
engine: faults accepted into a per-FB inbox and executed only by the FB's owner - #185
Conversation
…ross-FB reports never block (failing)
…B's owner; no wait on another FB's lock
EvidenceNew tests on
Rewritten existing test: Reviewer probes rebuilt against this branch (the delayed probe's pause moved from the removed post function to the new acceptance point via the host test hook; sequence and output unchanged)
Adversarial probes (normal and ThreadSanitizer builds; 0 TSan reports)
Verification
Notes for the reviewer
|
… and refused reports during the recheck (failing) Two reproductions fail against the single recheck in exec_exit: - a fault_fn that reports on its own FB once per invocation: one lock-free report executes two links of a six-link chain and returns; - a report refused (trylock) during the holder's recheck pass stays in the inbox after the holder's report_fault returns. A storm of lock-free and lock-holding reporters on an FB without a dispatch thread checks that every report is executed or counted as overflow and that the inbox is empty afterwards.
…e boot closing window, count clamp, failed-boot residual and R-sub-33 exec_exit rechecked the inbox once after releasing an FB's execution lock. A reason pushed during that recheck pass by a thread whose trylock was refused had no executor until the FB's next report: a fault_fn that reports on its own FB lost every link after the second, and a report made from another FB's handler could stay latched indefinitely. exec_exit now repeats drain pass, release, check until a check that follows a release finds the inbox empty. A push after that check finds the lock free and executes itself, or is refused only because a new holder exists, which runs the same loop. Contracts now state what the code keeps: - a report accepted in the boot closing window (after the post-init drain, before the boot thread releases the lock) runs immediately after RUNNING is published; the state never returns to RUNNING; - PENDING_OVERFLOW and BOOT_ABORTED counts saturate at 65535, and one PENDING_OVERFLOW event is emitted per drain pass that observed drops; - a fault_fn executing when boot fails runs to completion; later reports are latched until the next boot failure, a retried boot, or reset; - R-sub-33: inside fault_fn, publish only with embediq_bus_post_async(). The execution-lock argument excludes lock cycles, not bus-capacity waits.
|
Review of cfb5530: the release/recheck loop and the prior before-init/delayed-shutdown/cross-fault reproductions pass, but one ownership gap remains. Please hold merge for this P1. [P1] Revoke stale NONE-owner execution authority when boot aborts
Deterministic reproduction using the PR's existing report-delay hook and unchanged libraries:
The same reproduction runs under TSan with zero reports: this is a protocol race. No fault handler was executing when boot failed, so the documented allowance for an already-running handler does not cover it. Reason 22 should stay latched until a valid boot retry (or be counted/discarded according to the failed-boot policy), not invoke FB code in a failed system. Please coordinate execution permission with current owner/boot generation after acquiring the execution lock, and prevent ordinary reporter/drainer paths from consuming BOOT-owned inboxes. Retain the new no-stranding loop, and add this delayed-NONE-reporter → boot-abort → post-abort-report regression. Independent checks: 11 relevant ordinary host tests pass; local TSan engine 35/35 and OSAL 23/23 pass with zero reports; five relevant checkers and standalone FB header pass. Linux CI run 41515493654 is green at this exact head, 33/33 in both jobs, zero TSan findings/aborts. Those tests do not cover this ownership transition. |
… across a dispatch_boot flip (failing) A reporter reads the FB's owner when it accepts a reason and acquires the execution lock afterwards. Two reproductions park a reporter between the two steps with the report delay hook: - boot fails meanwhile; a reason reported after the failure is latched under BOOT, yet the resumed reporter executes it; - dispatch_boot makes the dispatch thread the owner meanwhile; a thread that took the lock under the old owner executes the reason instead of the dispatch thread.
…contract states what it executes; saturating boot count; cost bound only in uninstrumented builds report_fault reads an FB's owner inside the critical section but takes the execution lock outside it. A reporter that read owner NONE and was descheduled could take the lock after a failed boot and execute a reason latched under BOOT, or execute after dispatch_boot made the dispatch thread the owner. A drain pass now runs in a role (dispatch thread, or any other thread) and checks the owner against it inside the critical section, with its budget and again before every reason, and before taking the overflow count. A thread whose owner read went stale can win the lock but drains nothing; the release-then-check loop wakes the dispatch thread, or leaves a BOOT-owned reason for the boot thread to execute or count. The loop retakes the lock only in the reporter role under NONE. Also: - dispatch_shutdown's contract states what it executes: the faults of each FB whose execution lock it can take; a fault in progress on a reporting thread completes there and that thread's loop carries the rest; shutdown never waits for an execution lock; - the BOOT_ABORTED count adds the inbox and overflow counts with saturation; - the dispatch-cost test asserts its wall-clock bound only in builds without a sanitizer; every correctness assertion is unchanged.
|
Reply to the review of cfb5530: fixed in bce11d1. The owner is now re-checked under the critical section before every reason: each drain pass runs in a role (the FB's dispatch thread, or any other thread) and executes only while the current owner allows that role, checked together with its budget, before each pop and before taking the overflow count. A reporter whose NONE read went stale can still win the lock but drains nothing. BOOT-owned reports stay latched until the boot thread executes them after |
|
Verified the follow-up at bce11d1. The P1 in my previous review is resolved; no new blocking findings in this delta. PR #185 is acceptable to merge into dev at this head. The original failed-boot reproduction, rebuilt against the new normal and TSan libraries, now gives: The role check covers taking the initial budget, popping each reason, and taking the overflow count. BOOT permits neither drain role; the boot thread drains only after handing ownership to NONE. This is consistent with the intended protocol, and the added dispatch-owner regression verifies thread affinity across takeover. The overflow-count check appropriately leaves accounting to the valid owner. Retaining only two roles is justified by the actual call sites. Also independently reran the earlier pre-init, delayed-shutdown, and full-HIGH mutual-fault reproductions: before-init callbacks 0; shutdown 0 ms with the fault executed; both cross-fault reporters complete. Local relevant ordinary suite: 11/11. Local TSan engine: 37/37; OSAL: 23/23; both zero reports. Five relevant checkers and the standalone FB header pass. The sanitizer-specific performance-test adjustment retains correctness assertions and the uninstrumented timing bound. GitHub run 41526709828 tests this exact head and is successful: 33/33 in both Linux jobs; zero TSan findings and runtime aborts. I did not independently repeat the full local ASan suite or thermostat-capture benchmark. This closes the engine ownership finding, not the separately tracked TLS callback and persistence work. Nothing was merged by the reviewer. |
|
Final documentation recheck at cd53066: no new blocking findings; the recommendation to merge #185 into dev stands. Reviewed both cd53066 and the intervening 80d35a9 since the previously verified bce11d1. The cumulative changes affect comments/documentation only, with no executable-code changes. The taken-reason rule matches the critical-section dequeue linearization point, and shutdown wording now correctly describes the release/check loop without promising a fixed total bound under continuing reports. Both build and tsan checks are successful for cd53066 itself (run 41533052844). No tests were repeated locally for this documentation-only delta; nothing was merged by the reviewer. |
The bugs (post-merge review of #182 and #184)
A fault reported on a Functional Block before its
init_fnran executedfault_fnon uninitialised resources. Areporter descheduled between the shutdown checks and its enqueue could post after the final drain, orphaning the fault
until the next
dispatch_boot; the 100 ms in-flight wait diagnosed that stall without preserving delivery. Two FBs withdispatch threads and full HIGH queues reporting on each other from their fault handlers deadlocked on each other's
execution locks.
What breaks, and when
A fault handler dereferencing resources that do not exist yet; a fault silently deferred across a shutdown; two FBs —
and then shutdown — hung for ever under a fault storm.
What changed and why it fixes it
Accepting a fault and executing
fault_fnare now separate steps. Every FB has a framework-owned fault inbox, writtenand read only inside the OSAL critical section together with the FB's owner state (boot, none, dispatch). A report is
accepted in constant time and never waits for queue capacity or for another FB's lock. Execution belongs to the owner:
the boot thread after
init_fnreturns, the dispatch thread when one runs, otherwise the reporting thread — whichacquires the execution lock blocking only if it holds no execution lock itself, and by trylock otherwise, leaving the
fault for the current holder to execute on exit. Owner transitions happen inside the critical section and each executor
drains after the transition that made it the executor, so no report is missed; no thread that holds an execution lock
ever waits for another, so no cycle exists. After releasing an execution lock the holder repeats drain pass, release,
check until a check that follows a release finds the inbox empty, so a reason whose trylock was refused during a pass is
never stranded. A thread acts on an owner it read before taking the lock, so every drain pass re-checks the owner under
the critical section, with its budget and before every reason, and executes only in a role that owner allows: a reporter
whose owner read went stale (boot failed, or
dispatch_boothanded the FB to its dispatch thread) can win the lock butexecutes nothing. The timed in-flight wait and the bus post path for faults are removed.
Also: a failed boot executes no latched fault (one
EMBEDIQ_FAULT_BOOT_ABORTEDevent per FB counts them) anddispatch_bootdoes nothing after a failed boot; an FB whose dispatch thread cannot be created keeps having its faultsexecuted by reporters; the OSAL gains
embediq_osal_mutex_try(), whose refusal is an expected outcome and is notobserved as a fault.
Evidence
Seven new engine tests and one OSAL test failing on dev and passing after; the reviewer's three probes rebuilt
(before-init callbacks 1 → 0; delayed reporter executed before
report_faultreturns, shutdown 100 ms → 0 ms;cross-faults completed 0 → 2); boot/shutdown cycling under eight reporters with executed == reported − counted
overflow; a six-link self-reporting chain and a report refused during the holder's second pass, both executed before
the reporter returns (before the loop: 2 of 6 and 2 of 3); full suite under ThreadSanitizer: 0 findings.
Limits stated
Beyond EMBEDIQ_FB_FAULT_INBOX_DEPTH pending reasons on one FB, further reports are counted and reported as one
PENDING_OVERFLOW event per drain pass (its 16-bit count clamps at 65535); the FB enters FAULT when its first pending
reason executes. A
fault_fnthat blocks on the bus (publishing HIGH into a full queue) blocks the thread executing it —the dispatch thread, or the reporter's thread for an FB without one; inside
fault_fn, publish only withembediq_bus_post_async(), which never waits for capacity (R-sub-33) — the execution-lock argument excludes lock cycles,not bus-capacity waits. A fault accepted before or during
init_fnis executed before the FB is marked RUNNING. A reportaccepted in the closing window — after that drain and before the boot thread releases the lock — is executed immediately
after; the trace may then show RUNNING followed by FAULT for that FB. The state never returns to RUNNING.
🤖 Generated with Claude Code