fix: abort dispatch after critical delivery failure#97
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughCritical delivery failures now trigger run-fenced, durable abort recovery. The flow persists abort intent, releases affected agents, retries incomplete cleanup, publishes provider-visible notices, clears lifecycle status, and restores retry eligibility. ChangesCritical delivery abort recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatch
participant StateStore
participant Fleet
participant Provider
Dispatch->>StateStore: Persist exact-run abort intent
StateStore-->>Dispatch: Return aborting lifecycle
Dispatch->>Fleet: Release and terminate affected agents
Fleet-->>Dispatch: Report cleanup result
Dispatch->>Provider: Publish correlated failure and clear status
Provider-->>Dispatch: Confirm writeback cleanup
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a fail-closed mechanism for critical briefing delivery failures, ensuring that team dispatches are cleanly aborted and returned to a ready state if initial delivery or reinjection fails. The review feedback identifies several critical issues in the abort flow: potential memory leaks of the issue key in the #criticalDeliveryAborting set if the abort process rejects or returns early due to lease loss, a potential live-lock of the queue if the optional clearStatus writeback method is missing, and a case-sensitivity bug when matching retrieved GitHub labels.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agentworkforce/features/critical-paths.md:
- Line 242: Update the durable abort recovery documentation in
.agentworkforce/features/critical-paths.md at lines 242-242 to include
src/ports/state.ts and the durable state-store implementation alongside the
existing files. Also update .agentworkforce/features/manifest.yaml at lines
559-559 to include src/ports/state.ts and src/orchestrator/batch-tracker.ts.
In @.agentworkforce/features/verify/procedures.md:
- Line 244: Update the abort-state notice requirements in the “exhausted initial
delivery and exhausted reinjection” procedure so idempotency is scoped to each
correlated abort marker: allow one cleanup-pending marker and one final aborted
marker, while preventing duplicate emission of either marker during retries or
partial cleanup. Replace the single overall provider-visible failure constraint
without changing owner fencing, cleanup acknowledgement, or restart behavior.
In `@src/orchestrator/factory.test.ts`:
- Around line 7612-7635: The test around the idempotent GitHub abort notice must
exercise provider-backed reconciliation across a restart rather than relying on
the same instance’s in-memory notice set. Persist the abort state, create a new
Factory instance with a fake GitHub writeback implementing hasCommentMarker,
retry the failed status publication, and verify cleanup completes without
reposting the factory-critical-delivery-abort comment.
- Around line 7566-7588: The test around factory.dispatch must verify cleanup
ordering, not only eventual calls. Update RecordingGithubWriteback or the test
setup so clearStatus records/asserts that both expected releases in
fleet.releases have already completed when invoked, then retain the existing
release and writeback assertions.
- Around line 7827-7908: Seed the batch state in the late terminal failure test
before dispatch, using an occupied slot or queued record, then snapshot the
batch contents and capacity state. After the late event and duplicate event,
assert the seeded occupancy and queue remain unchanged and that no queued work
is promoted. Update the test around stateStore.getBatch and the late delivery
failure assertions while preserving the existing lifecycle and ignored-event
checks.
In `@src/orchestrator/factory.ts`:
- Around line 667-668: Update stop() to await all in-flight promises tracked by
`#criticalDeliveryAborts` after cancelling and clearing
`#criticalDeliveryAbortRetryTimers`, before calling fleet.dispose(). In the
follow-up dispatch logic around the critical-delivery abort completion handler,
guard the dispatch of the next queued issue with `#stopping` so no new work starts
during shutdown.
- Around line 3994-4004: Reorder the abort handling so
`#recordDispatchFailure`(record.issue) completes before the terminal abandoned
lifecycle is saved by `#saveDispatchLifecycle`. Preserve the existing watcher
cleanup and terminal waiter resolution, ensuring the dispatch-attempt fence is
released before any crash point after lifecycle terminalization.
- Around line 3889-3896: Update the critical-delivery abort flow around
`#doAbortCriticalDelivery` so unexpected rejections also schedule recovery before
propagating the failure. Preserve the existing cleanup of
`#criticalDeliveryAborts`, ensure the corresponding `#criticalDeliveryAborting`
entry is eventually cleared or retried through the established recovery
mechanism, and keep explicitly handled branches unchanged.
In `@src/ports/writeback.ts`:
- Around line 26-27: Make clearStatus in the writeback interface a required
method by removing its optional marker, and update any implementing writebacks
or mocks to provide the Promise<void> implementation so FactoryLoop can always
perform abort recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7dc95137-bf7c-4720-89d2-fa53548e2528
📒 Files selected for processing (13)
.agentworkforce/features/critical-paths.md.agentworkforce/features/manifest.yaml.agentworkforce/features/verify/procedures.mdsrc/orchestrator/batch-tracker.tssrc/orchestrator/factory.test.tssrc/orchestrator/factory.tssrc/ports/state.tssrc/ports/writeback.tssrc/state/file-state-store.test.tssrc/state/file-state-store.tssrc/state/in-memory-state-store.tssrc/writeback/github.tssrc/writeback/writeback.test.ts
khaliqgant
left a comment
khaliqgant
left a comment
There was a problem hiding this comment.
Live-tested this branch (ec8a958) against a real dispatch. The abort does not fire on the failure it's meant to catch.
Setup
Local build of this branch, factory dispatch on AgentWorkforce/relayed#50 (an issue deliberately unanswerable from the repo), config with babysitter.enabled: true + slack.channel. Internal backend.
What happened
The #95 gate fix worked — no false trip:
[factory] Slack sync soft-degraded but webhook delivery is healthy; continuing Slack writeback
Then the exact failure this PR targets:
[factory] error {"message":"Critical delivery failed to ar-50-impl-relayed: max delivery retries exceeded","issue":"50"}
And then nothing. Sampled every 20s for ~8 minutes after the failure:
[14:28:08Z] agents=2 abort_lines=0 file_written=no
...
[14:33:10Z] agents=2 abort_lines=0 file_written=no
Both agents still alive, no abort, no release, dispatch still running. The un-briefed implementer keeps going — the #96 behaviour, unchanged.
Likely mechanism (inferred, not proven)
#handleDeliveryFailed logs unconditionally via this.#error(...) — that's the line we see — but the abort is gated:
const critical = await this.#state.consumeCritical(this.#workspaceId, info.msgId ?? '')
...
if (critical && this.#fleet.waitForInjected) { ... #abortCriticalDelivery ... }waitForInjected is implemented on InternalFleetClient (:332), so the suspect is critical. Criticals are recorded under ack.eventId (factory.ts:5397 et al) and looked up by info.msgId. In internal-fleet-client.ts, the message_delivery_failed branch (:527) has an explicit if (event.event_id) guard before using it — implying that kind can arrive without an event_id. If so, msgId is undefined → consumeCritical(workspaceId, '') → undefined → the if (critical && ...) guard is false → no abort, and the error is only logged.
max delivery retries exceeded reads like message_delivery_failed.lastError rather than delivery_failed.reason, which fits.
I haven't instrumented it to confirm which branch fired or whether event_id was actually absent — flagging the hypothesis rather than asserting it. Worth a log of info.msgId and whether consumeCritical resolved, since that would settle it in one run.
Why this matters for the fix
The PR's validation is unit-level (838 tests + a 17-case matrix), and the tests presumably construct a critical record so the guard passes. The observed production path doesn't get that far — so the suite can be green while the abort never fires in a real dispatch. That's the same shape as the --workspace-key lying-test problem in relay#1287.
Suggest either:
- Fail closed on a critical delivery failure regardless of whether
consumeCriticalresolves — the identity of the message matters less than the fact that a briefing for a known agent (info.tomaps to a record) failed; or - Make
msgIdreliably present on themessage_delivery_failedpath, and add a regression test that drives the failure through#emitDeliveryFailedwith noevent_id.
Context
The reproducible delivery failure also means #96's fabrication case is not a one-off: it has now failed to brief the implementer on 3/3 dispatches (relayed#48, #49, #50) in this environment. On #49 the un-briefed agent invented an on-call policy. Agents register a name (lastSeen current) but sit at status: "unknown" with no pid/cli — registered, never live/deliverable — while codex idles at ~0% CPU. Whether that non-registration is the root cause of the delivery failure is a separate thread worth pulling.
|
Recommend closing this as the wrong fix (not as "not a problem" — #96 is real and should stay open). Two independent reasons: 1. It doesn't fire in productionLive-tested against a real dispatch (branch Sampled every 20s for ~8 minutes after 2. It guards a mechanism the correct fix removes#97 aborts when the critical task injection fails. But that injection is itself the bug (#98): factory spawns codex with a task, codex starts working immediately, and the briefing injected on top races its interactive hold so So even a working version of #97 would be guarding a code path that shouldn't exist. What should happen
If for some reason the spawn-task fix is not landing soon and an interim guard is wanted, the useful shape is fail-closed at dispatch when the agent cannot be briefed — but that's better achieved by removing the failure mode than by reacting to it, and it would need the trigger above to actually fire first. Refs: #98 (root cause + the spawn-task fix), #99 (escalation redesign), #96 (the real problem, keep open), #79 (release/resume — the reply half). |
|
Closing per the rationale above: this doesn't fire in production (verified live) and guards a critical-injection mechanism that the spawn-task fix (#98) and escalation redesign (#99) remove. #96 stays open as the real problem — the fix there is reliable briefing delivery, not abort-on-injection-failure. |
2 participants
Summary
Validation
npm run buildnpx tsc -p tsconfig.build.json --noEmitnpm test(838 passed)node bin/factory.mjs --helpnpm pack --dry-run --jsongit diff --checkFixes #96