Repository navigation
P1: Close eight run-lifecycle races (double fork, verify window, checkpoint vs next turn, unguarded worktree IPC) #1430
Description
Found in the T3 Orchestrator V2 deep dive (report: reports/T3 Code orchestrator deep dive.md on branch coder/i-want-you-to-research-t3-codes-orchestr-fae77a). Everything below comes from reading the code at fbba4e10. None of it was reproduced live. A scan of the live store found no sign of R1 yet.
A run-long per-thread lock is the wrong fix. It would deadlock whenever a thread calls thread_stop on itself or thread_merge onto its own tree. A lock wouldn't help startRun either: there are no awaits between the active.has check (runner.js:8229) and the preparing entry (:8644). Fix the races directly instead.
Phase A: targeted fixes, no lock (~80–120 lines)
| ID | Race | Where | Fix |
|---|---|---|---|
| R1 | An orchestrator thread's first-prompt fork hop holds no active entry while it waits on the pool. A second send forks again, Stop does nothing during the hop, and a worker that fails synchronously has its wake notice forked as if it were a task. |
runner.js:8318-8394, 7766-7799 |
Register the same preparing entry the main path uses, and delete it in finally only if it's still ours. Call abortIfCancelled between workers. Run the hop only when pendingFork && !input.fromNotice. |
| R2 | The verify gate sets the thread to working without registering it as active. A user turn, notice or quota timer can start a run mid-verify, verify then overwrites that run's status, and Stop can't cancel verify. |
runner.js:1496-1516, 1707-1711 |
Make verify an active entry {kind:"verify", runId, stopping, handle:{kill}}. Write status only while active.get(id)?.runId still matches. |
| R3 | The checkpoint on the non-verify path is fire-and-forget, so it can race the next queued turn and commit that turn's edits under the previous message id. | runner.js:1526-1532 |
Await the checkpoint before finishSuccessfulTurn. #1427 needs this too. |
| R4 | Rewind and restore check thread state, await, then reset, leaving a gap. | services.js:4140-4210, worktrees.js:5450-5510 |
Phase B lock on the IPC entry points. |
| R5 | A synchronous throw after the preparing entry is registered leaks it, and the thread reports "already active" forever. |
runner.js:8644-8704 |
try/catch that deletes the entry if it's still ours. |
| R6 | The Codex session-writer check ignores preparing entries, which reopens the #1265 failure mode. |
runner.js:1221-1232 |
codexSessionHeld counts a preparing Codex entry with the same sessionId as holding the session. This invariant spans threads, so a per-thread lock can't fix it. |
| R7 | The git:mergeWorktree and git:removeWorktree IPC handlers have no running guard. |
ipc.js:1714-1720 |
Refuse while the thread is running, reusing the requireLandableWorker wording. |
| R8 | steerRun appends its row with a runId it read before awaiting. |
runner.js:8805-8851 |
Re-read the active entry after the await. |
Phase B: a lock held only during transitions (~25 lines plus wiring)
// ponytail: one promise chain per thread; never held across a run or verify.
const tails = new Map();
function withThreadLock(id, fn) {
const prev = tails.get(id) || Promise.resolve();
const run = prev.then(fn, fn);
const tail = run.catch(() => {});
tails.set(id, tail);
tail.then(() => { if (tails.get(id) === tail) tails.delete(id); });
return run;
}- Wrap:
startRun(rename the body tostartRunInnerand rebind it; internal callers are at runner.js 1405, 1730, 2103, 3235, 3297, 3345, 7779 and 8351), the publicstopRun(the recursion instopCrewcalls the inner version), and the rewind / undo-rewind / restoreCheckpoint IPC entry points. - Never wrap: notice flush, enqueue,
drainQueued,maybeCreateCheckpoint, verify,stopAll,steerRun,thread_merge. - Stop: sets
stoppingon apreparingentry before waiting for the lock (the P1: Honor Stop while a run is preparing to launch #1228 fast path). - Ordering: never nest the same key. Cross-thread work takes locks one after another, lead before worker.
Tests
- stop-during-prepare: Stop during the fork hop; a synchronous throw leaves no entry behind.
- orchestrator-threads: two concurrent sends create one worker; a notice is never forked.
- verify-gate: a start during verify stays queued; Stop cancels verify; verify never overwrites a newer run.
- checkpoints: a queued follow-up starts only after the checkpoint commit.
- codex-writer-lock: the
preparingcase. - A unit test for
withThreadLock.
Phase A first. It should land before #1427 (which needs R3) and before any new lifecycle entry points such as lead steering or PR-watch wakes. #1428's replay harness makes R1, R2 and R3 testable without sleeps.
Activity
currentbits commentedon Oct 4, 2026
Repro results at 615b38a0, in a CI-comparable env against the real runner, store and git. The repro tests assert the correct behaviour, so they fail on current code. They're in
research_notes/T3 orchestrator ACP UX and repros/repros/on branchcoder/i-want-you-to-research-t3-codes-orchestr-fae77a.fromNoticeturns); Stop during/committeestill starting the second worker. Not reproduced: Stop during the single-worker fork hop, where the crew cascade stops the preparing worker and only the lead's transcript is misleadingdone. Stop has no effect on verify, and verify's success then starts the queued follow-upnotifyRunTerminaldrains the queue a second time (runner.js:2067-2078). Also addstatus !== "done" &&to that condition. With both parts, R3 passes 3/3 and the neighbouring suites stay green (69/69)failed. Deleting the leaked entry alone leaves it showingworkingQ1, answered: Stop already holds a queued follow-up (
queued-draintests, 6/6). The exception is Stop during verify (R2).Not driven: the
git:prMergepath, the web bridge, and eject (Stop without the crew cascade).Suggested order: land these repro files with #1428's harness, then fix Phase A against them.
currentbits commentedon Oct 9, 2026
One more race for this list, re-verified on main @24d3c36b: a send during rollback can undo it.
electron/worktrees-checkpoints.js:445checksisRunningonce, then awaitslistCheckpoints(:463),maybeCreateCheckpoint(:476) andreset --hard(:478) before clearingsessionId(:495). There is no per-thread lock across those awaits. A message sent in that window resumes the old session, and its session id can overwrite the cleared one, so the agent continues from the pre-rollback conversation. Fix: take the same per-thread run lock that sends take, for the whole rollback.currentbits commentedon Oct 12, 2026
Phase A (R1, R2, R3, R5, R6, R7, R8) is committed as 3f6c35c3 on branch
coder/p1-close-eight-run-lifecycle-races-doubl-fc27bb. It is not pushed yet.preparingentry viaholdForHop. Notices skip the fork path. A Stop during the hop stops any worker that slipped past stopCrew, and dispatch starts no further workers.activeentry{kind:"verify"}. Stop kills the command through an AbortSignal, andreleaseVerifygates every status write.finishSuccessfulTurn. The duplicate drain innotifyRunTerminalis removed.preparing.codexSessionHeldnow counts apreparingentry on a Codex thread.git:mergeWorktreeandgit:removeWorktreerefuse whilerunner.isRunning.steerRunthrows instead of filing the row when the run changed during the send.Tests: each race has one regression test, and all 10 fail on the pre-fix source. The full electron suite has 4668 passing and 2 failing. Both failures also happen on clean HEAD:
store.test.jsround-trip andautomation-create-mcpGitHub trigger.Still open: R4 and the Phase B
withThreadLock.currentbits commentedon Oct 12, 2026
Phase A is up for review as PR #1609 (rebased onto main at 3b650a5, commit c07e696). R4 and Phase B are still open.
Close run-lifecycle races R1-R3, R5-R8 (#1430 Phase A) (#1609)
currentbits commentedon Oct 12, 2026
Landed from Solenta thread "P1: Close eight run-lifecycle races (double fork, verify win". Closed on merge.
Close run-lifecycle races R1-R3, R5-R8 (#1430 Phase A)