Skip to content

feat(supervise): acknowledged worker cancellation via the run layout - #854

Merged
drewstone merged 1 commit into
mainfrom
feat/acknowledged-worker-cancellation
Aug 15, 2026
Merged

feat(supervise): acknowledged worker cancellation via the run layout#854
drewstone merged 1 commit into
mainfrom
feat/acknowledged-worker-cancellation

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Closes #758. Implements the audited design in #758 (comment).

What ships

1. Acknowledgement record in the run layout (src/runtime/supervise/run-layout.ts)

  • WorkerCancelRequest — one durable NDJSON line in cancellations/requests.ndjson, keyed by a caller-minted operationId.
  • WorkerCancellation — the durable acknowledgement at cancellations/<opId>.json, written ONLY by the runtime acknowledger. effect reuses RetainedRunEffect (cancel_requested | cancelled | not_live | unknown) — no second four-state vocabulary.
  • Idempotency is a lookup: an existing record for an operationId is returned as-is; a repeated operation never applies twice. Filename sanitization collisions between distinct operation ids fail loud on read.

2. The acknowledger in the existing turn loop (src/runtime/supervise/coordination-driver.ts)

  • DriverAgentOptions.controlDir mounts the acknowledger inside hooks.beforeTurn — one cancellation-inbox read per turn, plus one final pass after the post-loop drain. No new process, no poller, no socket, no new lifetime.
  • Apply goes through the worker's existing per-child abort chain: CoordinationTools.abortWorker keeps the Handle from the spawn that minted it and calls handle.abort(), which cascades to that child's subtree and no sibling (the scope's own AbortController chain). No parallel mechanism.

3. Two-phase effect, honestly reported

  • cancel_requested when the abort is issued.
  • cancelled only when the worker's settlement is DELIVERED on the settle path with a terminal down.
  • not_live when the worker already settled, or when it settles done despite the abort — its result stands and the operation terminated nothing.
  • A reference matching nothing stays unanswered and reads unknown. A missing process never reads as success.

4. One export mirroring writeWorkerSteer

  • cancelWorker(eventDir, worker, operationId, { reason?, source? }) returns the operation's current durable state; readWorkerCancellation(eventDir, operationId) is the pure reader a reconnecting client uses. Both exported from src/runtime/index.ts beside the steer exports.

5. Descendant naming

  • Cancelling a lead cascades to its subtree by design (the scope signal chain). The record's terminated set names the requested worker plus every subtree node id with a terminal down/cancelled journal record at or after requestedAt, read from the durable spawn journal beside the layout. The nested trees journal their terminal records before the lead settles (closeNestedScope drains them), so the evidence is durable by reconcile time.

6. The TUI bypass

  • Worker-focused c now goes through cancelWorker — one writer, one acknowledger, an honest notice showing the acknowledged effect.
  • The supervisor-focused c is genuinely ROOT-scoped: it cancels the whole run, and a non-retained supervisor tree exposes no root handle to acknowledge with. Per the audit, that is a separate contract gap, so the root-level path stays a cancel.request.json write for the host process to honor, and its notice now says so (host-honored; not runtime-acknowledged) instead of implying acknowledgement. It is not folded into this worker-scoped contract.

Wiring: supervise({ runDir }) passes the run directory as controlDir to the root manager (supervise.ts, SupervisorAgentDeps.controlDir), so every durable run acknowledges cancels with no extra configuration. The acknowledger resolves references against its OWN workers; a request naming a deeper descendant stays unanswered — cancel its lead instead (documented on controlDir).

Acceptance tests (tests/kernel/worker-cancellation.test.ts, all real runtime runs over a file-backed journal)

Acceptance line Test
Repeating one operationId applies once; second call returns the SAME record repeating one operationId applies cancellation once and returns the same record (abort listener fired once; one request line; deep-equal record)
Cancelling one child leaves siblings running cancelling one child leaves siblings running until they settle normally (sibling live through the abort, settles done, wins the run)
A lead's record names every terminated node; a sibling subtree survives cancelling a lead names every terminated descendant while a sibling subtree survives (terminated = [lead, lead:s0, lead:s1]; peer subtree delivers)
A reconnecting client reads the acknowledged result a reconnecting client reads the acknowledged result from the layout alone
A missing process returns unknown/rejected, never success a worker that is already gone acknowledges not_live, and an unknown one stays unknown — never success
Runtime tests prove the worker reaches a terminal state the cancelled worker reaches a terminal down state visible on the settle path (bus settled event + durable journal record)

Docs

  • docs/canonical-api.md decision table gains the cancel row beside the steer row.
  • docs/api/* regenerated with pnpm run docs:api; docs:freshness reports OK (it reported 21 drift issues before this branch).

Verification

  • pnpm typecheck clean.
  • tests/kernel/worker-cancellation.test.ts: 6/6 pass.
  • Full pnpm test run on this branch (results below in first comment / CI).

Closes #758.

The run layout gains a worker-scoped cancellation contract:
- cancelWorker appends a durable request keyed by a caller-minted
  operationId and returns the operation's current durable state.
  Idempotency is a lookup: an existing acknowledgement returns as-is,
  so a repeated operation can never apply twice.
- The acknowledgement record reuses RetainedRunEffect. The coordination
  driver's turn loop is the acknowledger: one cancellation-inbox read
  per turn, applied through the worker's existing per-child abort chain.
  It records cancel_requested when the abort is issued, cancelled only
  when the worker reaches a terminal down state on the settle path, and
  not_live when the worker is already gone. A missing worker never
  reads as success.
- A cancelled lead cascades to its subtree by design; the record's
  terminated set names every node id proven terminated, filled from the
  durable spawn journal as descendants settle.
- supervise() wires runDir as the control surface; the TUI's
  worker-focused cancel now goes through cancelWorker. The run-level
  cancel stays a host-honored file because a non-retained tree has no
  root handle to acknowledge with (a separate issue).

Runtime tests cover the acceptance list: idempotent repeat, sibling
isolation, descendant naming with a surviving sibling subtree,
reconnect read from the layout alone, not_live/unknown for a gone or
unknown worker, and the terminal state on the settle path.
@drewstone

Copy link
Copy Markdown
Contributor Author

Full local pnpm test on this branch: 2697 passed / 6 skipped, 2 failed (candidate-execution-claim, improvement-driver). Both failures are load flakes, not this change: the failing set differed across two full runs (a first run failed 7 unrelated process-group/worktree tests), and both files pass in isolation on this branch (43/43, 29s). pnpm typecheck clean. node scripts/check-docs-freshness.mjs reports OK after the docs:api regeneration (it reported 21 drift issues before this branch).

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 42a6bda4

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-15T03:27:34Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 42a6bda4

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-15T03:29:14Z

@drewstone
drewstone merged commit 91ad653 into main Aug 15, 2026
4 checks passed

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Value Audit did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.

Status audit-incomplete (could not run)
Why value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)
Lenses answered 0 of 2
What to do re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=3/48 — no model was started

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=4/48 — no model was started

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/kernel/worker-cancellation.test.ts

+const perWorker: Budget = { maxIterations: 4, maxTokens: 1000 }


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260815T033325Z

@drewstone

Copy link
Copy Markdown
Contributor Author

Adversarial review (post-merge — the value audit that published 'no verdict')

Read against 91ad653c with intent to refute. Method: full read of the five runtime files + TUI + tests, the shipped test suite re-run green (6/6), and one new adversarial repro executed on the real driver path.

Confirmed defect — fixed in #859

The TUI offered a worker cancel that can never be acknowledged. top-model lists EVERY spawned journal node as a worker; requestCancel wrote a cancelWorker request for any running one. The acknowledger resolves the root manager's DIRECT children only (scope.view.nodes = direct children, scope.ts:2145; nested drivers get no controlDir). Repro (now a committed boundary test): cancel a live nested descendant by node id → the request crosses multiple acknowledger passes plus the post-drain pass, is never acknowledged, never applied, and still reads unknown after the run — while the notice implied a cancel was in progress. Writing a request nothing will read is the anti-pattern #758 banned. Fix: #859 (routing helper + refusal notice naming the lead + boundary test + doc clause).

Deferred with evidence

Claims that survived refutation attempts

  • Idempotency race: two concurrent cancelWorker calls with one operationId can both miss the pending lookup and double-append — but the single acknowledger dedups by operationId (in-memory map + durable-record lookup per pass), so the abort applies once. Holds under the one-acknowledger-per-runDir invariant; two supervise() processes sharing a runDir is already documented as a collision.
  • Cancel dead during a hung await: refuted — await_event is bounded (returns {pending}), so the turn loop keeps running and beforeTurn passes fire. Latency is one await ceiling, not unbounded.
  • 'not_live' for a wedged-but-live worker: refuted — liveness comes from the view status, not activity; a wedged live worker resolves and gets the abort.
  • Tests are a hand-wired simulation: refuted — all six go through driverAgent + createSupervisor().run with the real file layout and journal; the acknowledger is exercised only via the real turn loop.
  • Atomic ack write: tmp+rename in one dir; readers verify the stored operationId and fail loud on sanitize collisions. Holds.

Checks: pnpm typecheck clean; vitest worker-cancellation 6/6 at merge, 7/7 with the boundary test; biome check clean on touched files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(supervisor): export acknowledged worker cancellation

2 participants