fix(lifecycle): durably deliver worker-idle completions to the orchestrator

[ikeshavvarshney]

Wake the orchestrator when a worker finishes — durably, exactly-once, without interrupting it

Closes #2820

Supersedes my earlier PR (#2823 ).

Problem

When the orchestrator spawns a worker, it was never told when that worker finished. Nothing reacted to a worker's active → idle transition, so a completed worker sat silent until the human manually asked the orchestrator to poll (#2820).

Approach

active → idle is the completion signal, and waking the project orchestrator is preferable to polling every worker. The key design point from review: recording that a worker finished and delivering that fact to the orchestrator are two separate responsibilities. They are now decoupled behind a durable, at-least-once outbox.

Invariant: once AO accepts a worker-completion signal, it stays pending until AO successfully attempts delivery to a safe orchestrator. Duplicates after a crash are acceptable; silent loss is not.

What changed

• worker_idle_events outbox (migration 0024): project, worker, transition time, delivery state. A partial unique index on worker_id WHERE pending coalesces repeated completions to one pending row.
• Atomic persistence (RecordWorkerIdle): the worker's active → idle transition and its outbox event are written in one transaction, so a crash cannot save the worker idle while losing the delivery work.
• Dispatcher (DispatchProject / DispatchAllPending): resolves the current orchestrator at delivery time (never bound to one that may be replaced), routes through sessionguard.Nudge, and marks an event delivered only when the guard reports a pane write was attempted — suppressed outcomes stay pending.
• Grounded nudge: the message tells the orchestrator to run ao session get <worker>, embedding no status snapshot that could be stale by delivery time.

Explicit delivery contract

This reconciles the two review threads (one asked for "idle only"; the other correctly noted that active delivery is safe for harnesses like Codex where Enter steers the current turn, and that "idle only without retention" would itself create a lost-notification path):

Orchestrator state	Action
Idle	Deliver now
Active and harness steers active turns	Deliver now
Active without safe steering	Defer
Blocked / waiting-input	Defer
Missing / terminated	Defer until an orchestrator is available

Active-turn steering is an explicit harness capability (HarnessSteersActiveTurn), defaulting conservatively to false for any unknown harness.

Addressing Reviewer @Vaibhaav-Tiwari (active-orchestrator behavior)

The contract above makes the description and implementation agree: an active orchestrator is only nudged when its harness can safely steer; otherwise the completion is retained, not dropped.

Added a test: a worker goes active → idle while the orchestrator is active (non-steering) and no nudge is sent — the event is retained and delivered later.

Addressing Reviewer @illegalcall (durable at-least-once delivery)

• Typed, project-scoped worker_idle event — ✅
• Durable internal outbox (not human-facing dashboard notifications) with project/worker/kind/transition/delivery state — ✅
• Activity transition + outbox event persisted atomically — ✅
• Coalesce repeated completions per worker — ✅ (partial unique index)
• Delivery resolves the current orchestrator at delivery time — ✅
• Route through sessionguard.Nudge; mark delivered only on an attempted pane write; suppressed stays pending — ✅
• Retry on: event creation, orchestrator entering a safe state, orchestrator spawn/restore, daemon start, and a low-frequency recovery sweep — ✅
• Active-turn steering as an explicit harness capability, conservative default — ✅
• Grounded nudge (ao session get <worker>, no stale snapshot) — ✅
• Test matrix — ✅ (below)

Tests

• Lifecycle: idle orchestrator delivers; steerable-active delivers; unsafe-active / blocked / waiting / missing orchestrator retain; deferred delivery fires when the orchestrator becomes safe; repeated completions coalesce to a single delivery; destination is the orchestrator session id (not merely "a message was sent"); DispatchAllPending delivers then does not re-deliver.
• Store: atomic record, coalesce, mark-delivered, and pending persistence.
• Session manager: an orchestrator spawn triggers dispatch; a worker spawn does not.

Verification

go build, go vet, gofmt, and all touched-package tests pass locally.

I could not run golangci-lint or go test -race in my environment (toolchain limitation), so CI is the first full lint/race pass — happy to fix anything it surfaces.

---

cc @Vaibhaav-Tiwari @illegalcall

[somewherelostt]

Thanks for contributing to Agent Orchestrator.

This PR is being picked up by the current external contributor on-call pair:

• @neversettle17-101
• @somewherelostt

If someone is already working on this, please continue as usual.
The on-call pair is added for visibility, tracking, and support, not to take over the work.
If you need help with review, direction, reproduction, or next steps, please tag @neversettle17-101 and @somewherelostt here.

For faster context or live questions, you can also join the AO Discord.

Join the session here:
https://discord.gg/H6ZDcUXmq

Come by if you want to see what is being built, ask questions, or just hang around with the community.

[illegalcall]

This is a strong improvement over #2823. The durable outbox, atomic worker-state/event persistence, coalescing, delivery to the current orchestrator, retained suppressed outcomes, and retry triggers are the right overall architecture. The focused race tests and all current CI checks pass.

I am requesting changes because two correctness gaps can still violate the delivery contract under normal operation.

Blocking correctness issues

1. The safe-delivery policy is not enforced at the final write boundary.

   DispatchProject checks safeToDeliver from a ListSessions snapshot, but sessionguard.Nudge rereads the session and only suppresses needs-input. If a non-steering orchestrator changes from idle to active between those operations, the event is still written into an active turn. The same can happen when one delivered event activates the orchestrator while additional pending events remain in the loop.

   I reproduced this with a temporary test: the dispatcher observed an idle snapshot, the guard observed active Claude Code, and one nudge was still sent.

   Please make the last-mile guard capability-aware and re-evaluate the current session state immediately before sending. For harnesses that cannot safely steer an active turn, active must remain suppressed at that boundary.

2. Concurrent dispatch can deliver the same pending event more than once.

   There is no per-project serialization or atomic claim/lease around list-pending -> send -> mark-delivered. Startup, sweep, worker transition, orchestrator transition, spawn, and restore paths can overlap. Two dispatchers can read the same row and both send it before either marks it delivered.

   I reproduced this with two concurrent DispatchProject calls: the same pending event was sent twice.

   Please add either per-project dispatch serialization or an atomic outbox claim/lease protocol, plus a deterministic concurrency test. Crash-window redelivery may remain an accepted at-least-once tradeoff, but duplicate delivery during ordinary concurrent execution should not be.

Other required follow-ups

• Retry promptly on every transition into a deliverable state, not only active -> idle. For example, a Codex orchestrator moving from blocked/waiting to active is safe to steer but currently may wait for the periodic sweep.
• Reconsider worker_idle_events.worker_id ... ON DELETE CASCADE. Deleting/cleaning a worker currently deletes its undelivered completion, which conflicts with the durable-delivery invariant unless source deletion explicitly makes the event obsolete. Document and test whichever semantic is intended.
• Add the missing boundary tests:
  • waiting-input and terminated orchestrator suppression;
  • a real persistence/reopen restart test;
  • restore-triggered dispatch;
  • forced event insert/coalesce failure proving the worker state update rolls back atomically;
  • the safe-state TOCTOU case and concurrent-dispatch duplicate case above.
• Describe the guarantee as at-least-once, not exactly-once, unless the design also closes the send-before-ack crash window.

Architecture and repository fit

• HarnessSteersActiveTurn hardcodes Codex terminal behavior in the shared domain package. That capability belongs behind the agent adapter/port so lifecycle policy consumes an adapter-provided capability rather than knowing harness-specific semantics.
• WithActiveSteering appears to be an unused one-off injection point. Please either exercise it through a real boundary/test or remove it.
• The kind column is currently redundant in a table dedicated to worker-idle events and is not used by the domain/query path. Unless a concrete near-term consumer requires it, omitting it would keep this migration surgical.
• A name such as DispatchPendingWorkerIdleEvents would make the narrower behavior clearer than the generic DispatchProject.

Once the final-boundary safety check and normal-concurrency duplicate path are addressed, the core outbox approach looks sound.

[ikeshavvarshney]

Thanks @illegalcall — both blocking issues were real, and the reproductions made them easy to pin down. Fixed in 817a62da.

1. Safe-delivery policy at the final write boundary

You were right that the policy lived in the wrong place: the dispatcher checked a ListSessions snapshot while sessionguard.Nudge only knew about needs-input.

Delivery now goes through a new sessionguard.NudgeCoordination, which re-reads the session and its harness at the moment of writing and refuses an active turn unless the harness declares it steerable (new SuppressedBusy outcome).

The snapshot check is now only an optimization.

The dispatch loop also returns on the first non-Sent outcome, so a delivery that wakes the orchestrator can't push the remaining events in behind it. That was the second half of your reproduction.

---

2. Concurrent duplicate delivery

Dispatch is now serialized per project around the list → send → mark sequence, with a deterministic test that gates one dispatcher inside the critical window and drives a second.

I chose serialization over a database claim/lease because the daemon is a single process. This is an explicit design choice: it does not protect two daemon processes sharing the same database.

Happy to switch to an atomic claim if you'd rather have that guarantee.

---

Other follow-ups

• Retry now keys on the policy rather than idle state, so a steerable orchestrator leaving blocked or waiting receives delivery immediately instead of waiting for the next sweep.
• worker_id ON DELETE CASCADE was kept, but is now intentional and documented. Since the event's only instruction is ao session get <worker>, deleting the worker makes the event obsolete rather than retryable. A test was added.
• The delivery guarantee is now documented as at-least-once, with the send-before-ack crash window explicitly described.
• Added all requested boundary tests:
  • waiting-input suppression
  • blocked suppression
  • terminated suppression
  • real reopen persistence
  • restore-triggered dispatch
  • forced insert failure
  • coalesce path (forces the session write to fail so the existing pending row remains untouched)
  • TOCTOU case
  • concurrent delivery case

---

Architecture

• HarnessSteersActiveTurn has been removed from the domain layer.
• It is now ports.ActiveTurnSteerer, implemented by the Codex adapter alongside its existing EmitsSubmitActivity and EmitsBlockedActivity capabilities, and resolved by the daemon.
• The lifecycle layer no longer knows any harness-specific names.
• The default behavior is that nothing is steerable.
• WithActiveSteering is now the actual wiring boundary, with tests verifying:
  • Codex resolves to true
  • Claude Code resolves to false
  • Unknown harnesses resolve to false
  • nil resolves to false
• The kind column has been removed.
• DispatchProject has been renamed to DispatchPendingWorkerIdleEvents.

[illegalcall]

Findings

• dispatch: still has over-delivery edge cases and can send multiple nudges in the same cycle.
• dispatch can attempt delivery before runtime safety is fully settled at startup/restore.
• Please address these before merging.

[ikeshavvarshney]

Thanks @illegalcall — both were real. Fixed in 4370ca4, and I validated the behavior against a running daemon (details below).

Over-delivery. Root cause was timing: pasting a nudge only moves the orchestrator out of idle asynchronously (the activity hook lands later), so the per-send guard re-read still saw stale idle and the loop could dump the whole backlog into one turn. Dispatch now delivers at most one event per cycle; the orchestrator's next entry into idle re-triggers it, draining one nudge per turn. The trigger is refined so entering idle drains the backlog and a steerable harness resuming from a pause (blocked/waiting→active) delivers immediately, while ordinary idle→active does not pull the backlog into a fresh turn.

Delivery before runtime settled. safeToDeliver now also requires a non-zero FirstSignalAt — an authentic activity signal proving the runtime is up — and delivery fires on that first signal. I removed the premature spawn/restore dispatch from the session manager in favour of this first-signal trigger, so nothing writes into an unready pane.

getRecord in restore. RestoreAll now treats a session reaped between listing and relaunch (ErrNotFound) as a non-fatal skip.

loadProject fail-fast — I did not apply this. Making the shared helper error on a missing project broke ~20 valid spawn/restore paths, and the motivation (orchestrator-dispatch correctness) is moot now that dispatch lives entirely in lifecycle (which resolves the project/orchestrator itself) rather than the session manager. Happy to add a narrow fail-fast if you still want it, but it seemed out of scope for the fix.

Validation. Beyond unit/integration tests, I ran the actual daemon and drove the real POST …/activity path (the same endpoint the agent hook posts): a worker completion stayed pending while the orchestrator was unsettled and delivered once it got its first signal; and with two pending events, one idle cycle delivered exactly one (pending 2→1→0). -race/golangci-lint are on CI as before.

[illegalcall]

Reviewed the outbox path end to end. Atomic persistence, fail-closed guard, per-project delivery serialization, and the grounded nudge all hold up — FK ON DELETE CASCADE is real (foreign_keys(ON) is set in db.go), and the new SuppressedBusy outcome is enum-complete (every consumer checks == Sent). One merge blocker on the migration version, plus two minor notes inline.

[ikeshavvarshney]

Thanks @illegalcall for the thorough end-to-end review — glad the outbox path, fail-closed guard, delivery serialization, and cascade all held up.

Addressed in 6b961d25:

• 🚨 Migration collision — Renamed 0024_worker_idle_outbox.sql → 0025_worker_idle_outbox.sql. The content is unchanged (Goose orders migrations by the numeric prefix). TestMigrationVersionsAreUnique now passes, and I'll rebase onto the latest main so it sits cleanly after 0024_session_rename_cdc.

• Boot-path dispatch — DispatchAllPendingWorkerIdleEvents now runs in a goroutine during startup, keeping it off the critical boot path. The recovery sweep still backstops delivery if it doesn't complete before shutdown.

• Per-project mutex map — Left as-is per your "flagging only" note. Eviction would require refcounting or cleanup-on-delete, which introduces additional race surface for negligible benefit at the current project scale. Happy to revisit if it ever becomes worthwhile.

Rebasing onto main now and force-pushing. Thanks again!

[illegalcall]

CI lint (golangci-lint / errcheck) fails on the one spot below — .golangci.yml sets errcheck.check-type-assertions: true, so the single-value assertion lock.(*sync.Mutex) is flagged (it discards ok and would panic on a bad assertion). Two-value form fixes it. Only lint failed; build-test/race/format all pass.

[ikeshavvarshney]

@illegalcall ,

I've pushed a follow-up commit (7c543855) addressing the lint issue by switching the type assertion to the two-value form. The behavior is unchanged, as the map only stores *sync.Mutex.

The latest CI run is currently waiting for maintainer approval because this is my first contribution from a fork. Could you approve the workflow so the lint check can run on the updated commit?