stack/agents/darkwing/work/queue-50-review/review-r1.md

Row 50, cohort scope release after a force stop, round 1 review (Darkwing)

Issue #1536, request comment 27051, packet comment 27052, queue revs 285 and 286 (a400f521). Brief: docs/plans/2026-10-10_cohort-scope-release.md, section "Cohort scope release after a force stop". Base 16a8d038; I ran it on a400f521, which adds only the two queue files. Candidate manifest sha256 375594fc64b3e42be17a2b5f8983c0925eea37c928b1e8903640f4723c19bab9, 8 files: cohort.mjs, controller.mjs, four test files, the README and Dewey's evidence.md. shim.mjs is unchanged.

Verdict: approve, with one point Sage has to rule on. The release follows a proof in every path I could build: the force stop calls it only after the claim records stopped, close only for a binding that #stopped() accepts, and releaseCohort refuses a cohort that isn't a scope or a shim that answers for another invocation. The shim itself refuses release unless engine reads populated 0, so a release can't drop live members. 13 mutants: 9 killed, 4 survive, and none of the survivors is a gap I'd hold the row for. The one open point is the normal engine exit. The brief says to fix it in this row if it leaks. It leaks, and Dewey didn't fix it. I agree it can't be fixed here, for the reason below, but that's Sage's call, not mine.

Method

• Detached worktree at a400f521, the seven candidate files copied from a snapshot of the canonical tree, evidence.md alongside, then sha256sum -c: 8 OK. A second worktree for the mutants; its seven files check OK after all runs (r1/mut/manifest-after.txt).
• I read the diff in full, shim.mjs (release at lines 160-172), #forceStop, #serveOrphan, #onEnd/#transport/#uncertain, #recover and close() in controller.mjs, and R1-R6.
• 13 mutants (r1/mut/mutate.py, run.sh), the whole conversation suite each, own TMPDIR each. After each run the runner lists any shim left under that TMPDIR, kills its mosaic-chat-* scope and the PID, and restores the files. No mutant left a shim.

Node v26.8.1, TMPDIR=~/darkwing-scratch/r50a/tmp, gate.sh with DOCKER_HOST=unix:///nonexistent.sock.

Suites

Suite	Result
conversation	171/0
webui	22/0
test-auth	15/0
test-conductor	17/0
test-config	24/0
test-discord	66/0
test-extension-package	18/0
test-foundation	44/0
test-queue	27/0
test-release	4/0 without Docker, 14/0 with it (test-release-docker.txt)
test-task	26/2

The two test-task failures are "user recall run succeeds" and "recalled user name", the live worker check that needs Docker and a model call, as in my row 41 and row 47 gates. Dewey's 98/0 ran them.

The shim and unit counts in r1/out/summary.txt aren't clean numbers. Filbert's row 50 runs were live on the same user manager (the three units after the conversation suite were in ~/filbert-scratch), and my mutants started under tmp/mut-* while the gate was still running, inside the gate's tmp prefix. Checked directly at the end: no shim under my gate's tmp/chat03-*, and no mosaic-chat-* unit whose command line names darkwing-scratch.

The change

releaseCohort (cohort.mjs) checks the kind, then hello, then the invocation ID, then sends release and polls the unit for up to 3 s. The order is right: every refusal comes before release, so an unavailable result never means a half-done release.

#releaseScope (controller.mjs) acts only on a claim record with state stopped, a cohortProof and a shim, and memoizes one promise per claim ID. The force stop takes proven = this.claim after #claimFinish, so it releases the claim it proved, even if the claim changes while the scope-release barrier holds. The force stop's reply is the force-stop-fenced outcome with the stop as its after, so the release doesn't delay the client's reply. Every fail() in #forceStop returns before the release, which R3 covers for both an unavailable outcome and a rejected proof.

Close calls it when this.b.state === "stopped" and #stopped() accepts the stop. After recover and launch the binding is no longer stopped, so close releases only the stopped binding's own claim.

A normal engine exit (for Sage)

The brief: "First check whether a normal engine exit (no force stop) also leaves the shim. If it does, fix that path in the same row; if it doesn't, say why in the packet."

It does leave the shim. R4 shows it: after the fake engine exits 0 the binding is uncertain, the unit is active and the shim reports engineExit {code: 0}. Dewey didn't fix it, and gives reasons in the packet. I checked them against the code:

• A release with no recorded stop would leave the claim uncertain with its scope collected. The only way out of uncertain is a confirmed force stop (#serveOrphan's comment, line 388), and that stop reads the cohort through the shim. With the shim gone it ends unavailable, and the seat stays uncertain for good. That's worse than the leak.
• The fix that keeps the invariant is to prove the empty cohort on EOF and record stopped, which is what R4's force stop does. Doing that without a client's confirmation changes the force-stop and confirmation protocol (K6, confirmations bind target, operation and stop). The brief puts the force-stop phases and the claim protocol out of scope.

So the brief asks for two things that can't both hold. I agree with Dewey's reading, and the row still strictly reduces leaks. But the remaining leak is likely the larger one: every engine that exits on its own leaves a mosaic-chat-* scope and an idle node shim until someone runs a confirmed force stop or the user manager restarts. My recommendation: accept the row as is and open a follow-up for an engine exit, either a controller-run proof on EOF (no confirmation needed when the cohort is already empty) or a stated decision that the operator's force stop is the cleanup.

Mutants

Mutant	Change	Conversation	Result
norel	neither the force stop nor close releases	168/3	killed: R1, R2, R4
norelstop	the force stop doesn't release	169/2	killed: R1, R4
norelclose	close doesn't release	170/1	killed: R2
nowait	releaseCohort reports absent without looking	170/1	killed: R2 (unit still active right after close)
wait0	releaseCohort doesn't wait for systemd	169/2	killed: R2, R6
noevid	no entry in evidence.releases	168/3	killed: R1, R2, R4
nohello	no hello and no invocation check before release	170/1	killed: R6
early	the force stop releases before the claim records stopped	169/2	killed: R1, R4 (the guard refuses, nothing is released)
n2	killShims signals any unit	170/1	killed: R5
noguard	#releaseScope checks only the shim; callers unchanged	171/0	survives: both callers already gate on a proven stop
closenoproof	close drops the #stopped() check	171/0	survives: the inner guard still needs stopped and a cohortProof
invnull	releaseCohort drops the null-ID check	171/0	survives: a real shim always reports an ID, so null !== id refuses anyway
nopush	the release entry isn't pushed to observers	171/0	survives: no test reads the pushed evidence

noguard and closenoproof are two guards each covering for the other. Dewey's relfail and closeany weaken both at once and are killed by R3, so the pair is tested. invnull matters only for a shim that answers with no invocation ID, which shim.mjs can't produce today. nopush is the one real hole, and a small one: the board and terminal would get no release entry, and nothing would fail.

early is worth a line. With the release moved before #claimFinish, the guard sees stopping and refuses, so the stop never releases. That's the guard working; R1 and R4 catch the missing release.

Notes (not blocking)

1. nopush survives. One assertion that an observer sees { kind: "evidence", evidence: { kind: "release", ... } } in R1 would close it.
2. releaseCohort polls systemdUnits.lookup, a spawnSync of systemctl show, every 20 ms for up to 3 s on the controller's event loop. One lookup takes about 4.5 ms here, so a unit that lingers costs about a fifth of the loop for 3 s. It's the codebase's existing call and the common case returns in one or two polls. An async lookup or a longer interval would be enough if it ever shows up.
3. I agree with Dewey's two bounded follow-ups: the crash window between recording stopped and the release (a restart classifies the pair stopped and never releases), and close({ killEngine: true }) sending SIGKILL to a recorded PID after a proven stop. A release that came back unavailable or still listed also has no retry once the binding moves on after recover. That fits in the same follow-up as the crash window.

Files

• r1/candidate-manifest.sha256: copy of Dewey's.
• r1/gate.sh, r1/out/: suite runs and summary.txt.
• r1/mut/: mutant definitions, runner, per-mutant output and shims-left lists, summary.txt, and the manifest check after the runs.