VC-799: Hold a Session's kickoff through a failed first attach and deliver it on Retry

[hussainph]

Problem

session_start and session_delegate sent their kickoff only after a READY first attach and stored it nowhere else. When the first attach failed, the Session existed with no task. The tool said "a person can retry it from the app", but a Retry attached an empty Session: no turn ran and no notice came back. session_send refused it (no live executor), and boot recovery skipped the child as not-started forever.

The most common trigger was the 30 s admission bound on a first attach's workspace preparation. #attach forced a follow-up: prefix onto the prepare() wait, so every first attach was bounded like a queued follow-up. The bound does not cancel prepare(), which goes on to finish, but the attach was still recorded as failed with adapter_unrecoverable.

A second, smaller issue: on the ready path, the facade's kickoff port ignored the runtime's receipt, so a kickoff the runtime refused was never reported.

Fix

• The kickoff is held. On needs-recovery, startSessionOperation and Delegations.delegate submit the kickoff with delivery: "queue" under the existing durable kickoff ids (<op>:kickoff / <op>:kickoff-message). It goes into the Session's own durable follow-up queue. The attach-failure Attention keeps that queue from draining. The next successful attach (the app's Retry) clears the Attention and the queue delivers the message as the first turn, the same way automations/run.ts owes its first message (VC-220/227). A person also sees it in the queue meanwhile.
• The caller hears back.
  • session_start: on a held kickoff, the caller's turn watch is armed now, so the Retry's run reports back the way a ready start does.
  • session_delegate: no watch is armed while the child is held. When the held task's turn starts, the existing rearm wake (agents.ts) watches it. The delegation is still unanswered, so the one notice lands under <op>:answer.
• Boot recovery leaves a held child alone. Delegations.recover treats an attachment.failed that comes first as no outcome when no turn has started since and the child's queue still holds <op>:kickoff-message. It counts the child as skipped, so the Retry's notice is the only one. Once the held task has run, recovery reads that run's own outcome rather than the failed attach before it.
• A first attach has its own ceiling on workspace preparation (session-runtime.ts). This applies to a non-queue attach on a Session with no opened attachment.
  • It waits up to 10 minutes (firstAttachPreparationTimeoutMs) instead of the 30 s admission bound. Past that, it fails as location_unavailable / configuration_invalid ("Workspace preparation did not finish within 600 s."), the same Attention as any preparation that fails.
  • A preparation that throws fails the attach exactly as before.
  • Later attaches and queue-drain attaches keep the admission bound.
• The kickoff receipt is read. kickoffSubmitter (the facade's submitKickoffMessage) rejects with a KickoffRefusedError, carrying the refusal's code and detail, when the receipt is rejected. An unknown receipt is not a refusal.
  • On the ready path, the rejection is logged (kickoff was not delivered).
  • On the delegate path, it is reported, and the child's watcher settles it at the earliest durable outcome in the child's ledger.
• Tool text: "its kickoff is held as its first message … the retry sends the kickoff" for start, and the matching wording for delegate. The unsent branch keeps the old wording. The VC-659 attach-failure reason (VC-659: adopt stacked ticket branches; name why a start's attach failed #879) is passed through unchanged.

Decisions

• A 10-minute ceiling of its own on a first attach's preparation, rather than the admission bound plus an automatic re-attach.
  • The admission bound never stopped prepare(), so it only failed the attach. A re-attach would add a second attach path.
  • prepare() waits on things the git timeouts do not bound: slot waits, the per-repo worktree turn, copyIncludedFiles and materializeBlobs. So the first attach gets a generous ceiling rather than none.
  • Known gap: the kickoff is queued only once the attach settles. If the app quits while a first attach is still preparing, that kickoff is lost, as it was before.
• The kickoff reuses the follow-up queue under the existing ids, with no new ids and no new table.
  • No migration is needed.
  • A replayed start (same operation id) queues the same command id, and the queue answers from its record: one Session, one kickoff. A replay after the Retry delivered it sends nothing again.
• No watch on a held subagent at delegate time. A held child is not work in progress, and a parent's watchers would wait on it. The existing rearm on turn.started covers it under the delegation's own :answer ids.
• The in-ticket fan-out claim is kept on needs-recovery. The Session holds real pending work that a Retry runs, so it should keep counting toward the 3 slots.
• Auto-title refinement still runs only on a ready start. A held Session keeps its heuristic title.

Review

From the review of a36fdffe. Changed in af409116:

• MUST 1, recovery double notice: recover() skips a child whose first outcome is attachment.failed, with no turn.started after it and <op>:kickoff-message still in its queue. A failed queue read is reported and falls back to notifying, so the parent is still told rather than never told.
  • Extended slightly beyond the review's rule: when a turn.started does follow the failed attach, the held task ran. Recovery then takes the first outcome after that turn as the delegation's outcome, matching what the live rearm watch would have reported. Otherwise a relaunch during the held task's run would notify "failed".
  • Tests:
    • The integration test now calls delegations.recover(store.listUnansweredSubagents()) before the Retry: skipped, with no notice. After the Retry there is exactly one notice under <op>:answer, and nothing is left unanswered. With the fix disabled, this test fails (reported: 1).
    • Unit tests cover the held-but-not-yet-run, not-held, unreadable-queue and ran-after-Retry cases.
• SHOULD 2, unbounded preparation: there is now a 10-minute ceiling, raised as configuration_invalid (engine test added). The Decisions entry now states the remaining limit, the quit-during-preparation gap, instead of "git's own timeouts".
• SHOULD 3, refusal racing the watcher: KickoffRefusedError is only reported on the delegate ready path, and any other error still settles failed. A unit test covers it: a refused kickoff followed by a stop is reported as stopped.
• SHOULD 4, fixed sleeps: both tests now await a reached promise that prepare() resolves, then sleep past the bound.
• NIT 7: comments, test prose and this body describe behaviour, not history.
• NIT 8: session_start computes watchOpenedSession(...) once, for ready or held.
• Also: the not-held session_start door test now silences the error line it expects.
• bdda85a4: CI on af409116 failed stacked-branch-attach.integration.test.ts, which asserted no kickoff on a refused attach. It now asserts the kickoff is held (delivery: "queue", kickoff: "held").

Declined:

• Optional 5 (relaunch caveat in the tool text): the receipt line comes from watchOpenedSession, which every opening and steering tool shares. Every in-memory watch is lost on a relaunch, ready starts included. Saying so for this one door only would be inconsistent, and it can't be done cleanly from here: when nothing is watched, the shared line already says nothing reports back. If wanted, it belongs in watchOpenedSession as its own change.
• Optional 6 (replayed start reports held after the kickoff ran or was removed): cosmetic. The queue answers the replayed command from its record, and telling the cases apart would need a second queue read on every replay for a field nobody acts on.

Tests

Regression tests fail on origin/main source and pass with this change. I checked by restoring main's five source files with the new tests left in place: 3/3 integration cases and the first engine case failed. For the MUST-1 case, disabling the recovery skip fails the delegate integration test.

• packages/host-core/src/session-runtime/kickoff-hold.integration.test.ts (new) uses the real Sessions facade, Session runtime, SQLite ledger and follow-up queue, start and delegate operations, delegation store and wake bus. Only the executor is scripted.
  • A preparation that outlasts the admission bound: the start ends ready and the custom kickoff runs once, with no Attention.
  • The executor refuses the first attach (fails mid-attach): the kickoff is held under <op>:kickoff-message. A replayed start holds no second copy. The Retry delivers it exactly once, and a later replay plus a recovery sweep send nothing more.
  • The worktree preparation itself fails (never attached): the kickoff is held and the Retry delivers it.
  • session_delegate on a refused first attach:
    • the task is held, and a replay holds no second copy;
    • boot recovery before the Retry skips the child, with no notice;
    • the child's Retry runs the task, and the parent gets exactly one notice under <op>:answer, with nothing reported.
• packages/session-engine/src/session-follow-ups.test.ts:
  • a first attach waits out a slow preparation (accepted, no Attention);
  • a first attach past its own ceiling fails configuration_invalid;
  • a re-attach after an opened attachment is still bounded.
• start-session.test.ts, delegate-session.test.ts, agent-tool-door.test.ts cover:
  • the held kickoff's ids and delivery: "queue";
  • the held / unsent outcomes, and the watch on a held start;
  • the delegate's held and unheld wording, and recovery of held children;
  • KickoffRefusedError from kickoffSubmitter, and the watcher settling a refused kickoff.

Run locally at af409116 (plus stacked-branch-attach.integration.test.ts: 2 passed at bdda85a4), one file at a time with --maxWorkers=1 through the machine's serial runner:

• session-follow-ups.test.ts: 101 passed
• kickoff-hold.integration.test.ts: 4 passed
• delegate-session.test.ts: 52 passed
• start-session.test.ts: 31 passed
• agent-tool-door.test.ts: 59 passed
• vp check on the 10 touched files: clean

CI is the full proof. The af409116 run also hit a 5 s timeout in worktree/watch.test.ts ("500 Done tickets … 10 gh calls a cycle", 5.4 s). That file and the code it covers are untouched here, and the timeout is not on the known-flake list. The bdda85a4 run is a fresh run for the new commit, not a rerun.

Overlap: git merge-tree against origin/main is clean, and so are #909, #919, #920, #921 and #922, none of which touch these files. #879 (VC-659) is already on main. Targets main directly.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: bb8e086c-1e73-4a31-b8f7-8f29b732b8a6

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}