[ben-vargas]

test/retention.test.js › resume vs delete: a resume launched INSIDE the commit window still wins failed once on CI: run 37995281494, job 114039619367 (root suite · node 18.17.1), on PR #7 at 79f04fd. That PR touches no src/, test/ or scripts/ files, and the rerun passed. The failure is a timing assumption in the test, not a production regression.

Cause (test)

error: 'the resumed engine took ownership of the run'   (retention.test.js:656)
duration_ms: 8134   ← reapResumer's 8 s budget, exhausted

All earlier assertions passed: the delete was refused (live), the run was restored, the trash was empty, and the audit read ok → rolled-back/resume_raced. Only the last step failed.

Inside beforeCommit, the real launcher (cli run --resume --detach) installs .resuming and spawns a detached engine. removeRun then finishes a synchronous tail: audit, rename, marker diff, restore, lock.release(). The test assumed that tail always finishes before a freshly spawned node process boots and reaches the run. When the test process is preempted for longer than a node boot, the child arrives first and hits one of two cases:

• the run is in the trash, so it fails with "no journal" / "does not exist", or
• the delete's own run.lock is still there, held by the live test pid, so it fails with run is already being executed by pid <test pid>.

The engine's acquireRunLock does not retry, so the child exits. No foreign pid ever appears in run.lock, and reapResumer times out.

Repro. I busy-stalled the test process for 1.5 s right after the launch:

	macOS · node 18.17.1	Linux (node:18.17.1-slim)
before	0/3	0/5
after	5/5	5/5

The child's run.log in the failing runs says run is already being executed by pid <test pid>.

Fix (test only)

The spawned engine is held at process start until removeRun has returned:

• test/fixtures/spawn-gate.cjs is preloaded through NODE_OPTIONS=--require. --require is used because Node 18.17.1 has no --import. It parks the child before any flowition code runs and waits until a gate file exists. If the gate is never opened, it exits after 60 s without having touched anything.
• launchHeld() sets NODE_OPTIONS and FLOWITION_TEST_GATE only around the launch. detachRun passes process.env at spawn, so only that one spawn inherits them, and both are restored before it returns. The gate also deletes its variable, so the engine's own children run unheld.

Nothing changes in the launcher, the handoff, or the engine. The launch is still accepted inside the window, and the same engine has to take ownership. The only difference is that its start is pinned to after the delete lost, which is the interleaving the test is about. That replaces relying on a node boot to be slower than a sync tail.

Not weakened. Every original assertion is kept. Two things that were only implied before are now asserted directly, while the engine is held and can't interfere:

• run.lock is gone, so the delete released its lock.
• .resuming came back with the restored run.

Mutation checks:

Mutation in src/retention.js	Result
Rollback never releases the lock	fails at the delete released its lock
Post-rename marker diff removed (the delete commits)	fails at Missing expected rejection

Verification

• Target test, 50× each on node 18.17.1 and node 24 (macOS) under heavy CPU contention (36 busy-loop processes on 16 cores): 50/50 and 50/50. Also 30/30 on Linux, node 18.17.1, --cpus=1 with 4 hogs. The unfixed test also passed 30/30 under that natural load, because the race needs a stall longer than a node boot. That is why the stall repro above is the evidence.

• test/retention.test.js: 31/31 on node 18.17.1 and node 24.

• Full root suite on node 24: 550/551. Under node 18.17.1, two tests in other files failed locally:

  • viewer-perf P2 (a 120 ms perf budget) failed while the machine's load average was about 100.
  • viewer-e2e "viewer spine" (control socket disappeared after listen) fails the same way on untouched main on this Mac with node 18.

  Neither file is touched here, and both pass in CI.

Note: possible production follow-up (not changed here)

The same ordering exists outside tests. Suppose a resume is accepted during a delete's under-lock derivation, and its engine reaches acquireRunLock before the delete's rollback releases the lock. That engine refuses with "already being executed" and exits. Safety still holds: the run is intact and the delete refused, so you never get "accepted and deleted". But the accepted resume quietly fails, and its marker ages out after 30 s. Fixing that would need a design decision, such as the engine retrying briefly while it holds a fresh handoff marker, or retention marking its lock so the engine can tell it apart from another engine. That is out of scope for a test de-flake.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.