fix(authority): close controller boundary races - #93
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
LoopStore.fireOrExpire’s expected-identity fence compares optional fireCount with strict equality, which can permanently ignore legacy entries where fireCount is unset.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens controller authority boundaries and eliminates several session/lock/notification race conditions by adding identity fencing, stricter controller-domain validation, and more robust stale-lock handling across file-backed stores and runtimes.
Changes:
- Reworks
ReducerBackedStorelocking to use directory-based ownership with identity-conditional stale cleanup, plus recovery for legacy lock formats. - Adds workflow/loop identity fencing around
fireOrExpireand post-fire bookkeeping to prevent stale activations and state “laundering.” - Makes notification supersession monotonic (same-key ordering) and tightens task-backlog cleanup/evaluation to respect controller domains and session rebinding.
File summaries
| File | Description |
|---|---|
| test/workflow-task-integration.test.ts | Adds coverage for maxFires budgeting when startImmediately combines with bounded retries. |
| test/task-store.test.ts | Verifies pending counts refresh when a peer TaskStore instance writes to disk. |
| test/task-backlog-runtime.test.ts | Adds tests for controller-domain classification, batch-retirement callback ordering, and stale-session fencing. |
| test/store.test.ts | Adds metadata validation tests to prevent mixing workflow authority with standalone task/backlog behavior. |
| test/stale-lock-replacement.test.ts | New regression test for stale-lock cleanup races that must not delete a newly published live owner. |
| test/reducer-backed-store.test.ts | Extends lock tests for unique owner naming, legacy lock recovery, and stale directory cleanup. |
| test/orchestration-store.test.ts | Ensures orchestration controllers can’t be projected through standalone-task flags. |
| test/notification-runtime.test.ts | Tests same-key wake supersession during in-flight delivery while idle. |
| test/injection.test.ts | Tests same-key wake ordering is assigned before async task admission completes. |
| test/index.test.ts | Adds integration coverage for pre/post-fire workflow fencing, final delivery snapshot behavior, and pause suppression. |
| src/tools/workflow-tools.ts | Updates workflow default fire budgeting to account for startImmediately per bounded retry attempt. |
| src/task-store.ts | Makes pendingCount() reflect peer writes by iterating over list() (signature-gated reload). |
| src/store.ts | Centralizes controller-domain validation and adds expected-identity fencing for fireOrExpire / updateDynamic. |
| src/runtime/task-backlog-runtime.ts | Prevents workflow/orchestration controllers from being treated as backlog workers; adds rebinding fences and callback ordering. |
| src/runtime/notification-runtime.ts | Adds in-flight supersession check to prevent older same-key wakes from delivering after newer ones queue. |
| src/reducer-backed-store.ts | Implements directory-based lock acquisition/release with identity-conditional stale cleanup and legacy lock migration. |
| src/notification-reducer.ts | Ignores out-of-order NOTIFICATION_QUEUED events using queueSequence monotonicity. |
| src/index.ts | Passes expected identity into fireOrExpire, fences post-fire bookkeeping, and removes early autoTask admission gating from the event handler. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
faeaf50 to
7db3ec5
Compare
7db3ec5 to
eece1bd
Compare
Publish initialized UUID claims via hard link and require sole-claim admission. Reclaim dead contenders individually; retain legacy fences. Retry link-specific EINVAL from concurrent APFS scaffold removal.
|
Windows lock-publication repair is now at fbb512c (implementation b598c7c). Ownership is published as an initialized UUID hard-link claim and admitted only when it is the sole directory entry; publication no longer uses replacing directory rename. Live legacy owners remain fenced, dead competing claims recover individually, and link-specific APFS EINVAL retries remain bounded. Added 10 deterministic collision/recovery/error regressions; corrected baseline fixtures produced eight failures before the repair. Local validation: 969 tests, 16 property tests, build/package/audit, live orchestration, and the intact coverage push hook passed (coverage used one worker after a parallel run hit timeouts and a steering assertion). Independent bounded protocol review found no blocker. Real Windows/Ubuntu CI is pending in run 33943601869; no Windows success is claimed yet. CI now includes stacked PR bases. PR remains unmerged. |
|
Validation complete: the repair at fbb512c passed native Windows and Ubuntu CI (run 33943601869). The dependent four-layer stack is now open as #95 → #96 → #97 → #98, with exact predecessor bases. All four have successful Windows and Ubuntu checks; #96/#97 needed one bounded rerun for an existing file-backed property-test timeout. No merges performed. |
Summary
Validation
Stack
6 of 6. Base:
fix/controller-lifecycle-authority.