Tell a Mailroom sidecar apart from a damaged graph (#315)

[scgopi]

Closes #315. Regression from #308 (11e33a3), shipped in 0.1.64-beta3 today.

What was broken

$ graphcode reap --dry-run
graphcode: a graph or terminal layout failed to decode — refusing to guess which sessions it owns

OrphanedSessionReaper.liveSessionIDs treated every .json under a workspace's projects/ directory as a LoopGraph and returned nil — refusing to reap — if any one failed to decode. True until #307 moved the Mailroom into <project>.mailroom.json beside the graph. The room is a JSON array, so it can never decode as a graph, and every reap on every workspace bailed.

It fails closed, so nothing was destroyed. That is also why it would not have been reported: reap is the PTY-exhaustion recovery tool (#197), it is broken exactly when someone needs it, and the message reads as caution rather than breakage.

The rule I chose, and why not a filename blocklist

Refusing to guess stays right for a graph that is genuinely damaged — it still owns sessions nobody can enumerate, and reaping them would be a sweep rather than a reap. The defect is that a sidecar is not a damaged graph.

So the question liveSessionIDs is really asking is "which of these files are graphs?", and the type that can answer it is the one that mints both names. ProjectPersistence now owns the answer, beside mailroomURL:

static let roomFileSuffix = ".mailroom.json"
static let sidecarFileSuffixes = [roomFileSuffix]
public static func isSidecarFileName(_ name: String) -> Bool

Both readers of projects/ ask it — the reaper, and Workspace.contents, which carried the same assumption and escaped notice only because it continues instead of bailing.

Three things about the shape:

• The suffix is used by the writer too. mailroomURL builds the name from roomFileSuffix, so the rule and the file it describes cannot drift apart.
• Answered from the name, not the contents. A corrupt room is still a room and never owned a session; deciding by decode would let a damaged room disable reap for precisely the reason being fixed here.
• Anything unlisted still fails closed. That is the safe direction, but it is also a silently broken reap, so everyFileWrittenBesideAGraphIsRecognisedAsASidecar saves a graph with a room through the real ProjectPersistence and asserts every file that lands beside it is claimed by a suffix. Add a sidecar without registering it and that test fails, instead of reap quietly dying months later.

The stronger positive rule — sidecars in their own subdirectory, so "every .json in projects/ is a graph" is true by construction — needs a migration for rooms already written by beta3–beta5 and a downgrade story. Not worth carrying on a same-day regression fix; noted as the follow-up.

Two more findings from the same file

Both are state that outlives a delete.

• A queued save could resurrect a deleted graph. deleteGraph removed the file but left the writer's pending entry, so a drain landing afterwards would rewrite it, and GraphWriter.load kept answering from the queue for a project that no longer existed. GraphWriter.forget(path:) drops it and the delete calls it before removing the file. Structural — I could not reproduce it in 30 attempts against a live daemon, the window is microseconds — but aDeletedProjectIsNotHandedBackFromTheQueue fails deterministically without the fix.
• The room digest cache was keyed by project path. One path is the same project in every workspace but a different file in each, so two workspaces shared an entry and a room could be judged unchanged against a digest taken from someone else's file and never written. Keyed by the room file now, and cleared on delete. This is what made the sidecar test fail on its first run.

Two tests that were passing for the wrong reason

routing refuses a path with no folder at it. deletingAProjectsLoopsEndsEverySessionFirst and deletingAClosedProjectsLoopsStillEndsTheirSessions used /tmp/project-f and /tmp/project-g, which exist on the machine that wrote them (dated Sep 2) and would not on a clean checkout or in CI. Without them no loop is created, and the closed-project test's three expectations all hold for having found nothing. Both now create the folder, and that test asserts it created something before asserting what was killed.

Fail without / pass with

Test	Change reverted	Result
aRoomFileBesideItsGraphDoesNotStopAReap	OrphanedSessionReaper.swift to origin/main	FAILS — liveSessionIDs(...) → nil
aCorruptGraphStillStopsAReap	same	passes — fail-closed is genuinely preserved, not just claimed
aDeletedProjectIsNotHandedBackFromTheQueue	GraphWriter.forget removed	FAILS — load returns the deleted graph

End to end on this machine, both binaries against the same four live workspaces:

$ graphcode reap --dry-run                 # shipped 0.1.64-beta5
graphcode: a graph or terminal layout failed to decode — refusing to guess which sessions it owns

$ ./.build/debug/graphcode reap --dry-run  # this branch
orphan	graphcode-16C200BF-...
orphan	graphcode-5929A5A9-...
[…]

Gate

Gated on 6bf3a69, which equals the pushed head of fix/reap-mailroom-sidecar. Private DerivedData at <worktree>/.derived.

Step	Exit	Result
xcodebuild -scheme graphcode test	0	** TEST SUCCEEDED ** — 1668 tests / 175 suites / 0 failures (+5 vs main)
xcodebuild -scheme graphcode-cli build	0
xcodebuild -scheme graphcoded build	0
swiftlint lint	—	0 errors
swift format lint --strict	0	clean
scripts/cli-smoke.sh	0	5 verbs against a throwaway daemon, after rm .build symlink + swift build

Linux to be confirmed from gh pr checks rather than inferred.

Not merging on my own gate — this needs a reader.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Gvj8H68LzmfgKg4cjEz1nx

[scgopi]

Independent review — merged with two amendments (pushed as 19028d20)

I merged this after amending. The core fix is right: ProjectPersistence owns the rule, the suffix is shared with the writer so rule and file cannot drift, and the fail-closed property survives a genuinely corrupt graph. The answer-from-the-name reasoning I accept: a corrupt room is still a room and never owned a session, and deciding by decode alone would re-create today's bug in miniature for a damaged room. That reasoning survives in the amended code — with one place where the name can lie, handled below.

Verified myself

Attack	Result
e2e reap --dry-run, shipped beta5 vs this branch	shipped aborts (bug live on this machine); branch lists orphans — both claims reproduced
Unregistered sidecar beside a graph	probe: an unregistered *.cache.json written by saveGraph → everyFileWrittenBesideAGraphIsRecognisedAsASidecar fails loudly
Unregistered sidecar vs the reaper	fails closed — liveSessionIDs → nil, a loud abort, not a mis-reap
Corrupt graph	aCorruptGraphStillStopsAReap passes; fail-closed preserved
Drive-by #1 — queued save resurrects a deleted graph	aDeletedProjectIsNotHandedBackFromTheQueue fails deterministically with forget neutralised
Drive-by #2 — digest keyed by project path	probe with the old keying: workspace A's room judged unchanged against B's digest, never written — fix matters, but had no test of its own; one added
Drive-by #3 — the two /tmp/project-* tests	correction below

Correction to the body: the suite init() that mints /tmp/project-a…i predates this PR (it is on the merge base 8dbf548), so the folders are not machine-dependent leftovers — init() creates them on every run, including CI, and deletingAProjectsLoopsEndsEverySessionFirst was not touched by this PR (still /tmp/project-f; it was already non-vacuous via #expect(everyLoop.count == 3)). Only the closed-project test was made hermetic. Harmless, but "Both now create the folder" and "would not [exist] on a clean checkout or in CI" overstate.

Amendment 1 — the name-only rule was fail-OPEN for one project shape

A project path ending in .mailroom mints a graph file that carries the sidecar suffix (_tmp_x.mailroom.json). Skipping by name dropped its live sessions from the reap's set — a real reap would kill them, the opposite direction of the safety property being traded on. Probe on the name-only version: liveSessionIDs → [] for a project with one live loop.

Fix (pushed): both readers decode first and let the name decide only the failure path. A sidecar — corrupt or not — never decodes as a graph, so the corrupt-room property is untouched; a sidecar-named file that decodes as a graph is a graph. Workspace.contents drops its pre-decode filter entirely: a room never decodes as a graph, so the decode already skipped it, and the filter was the only thing that could hide a sidecar-named project from the listing. Tests added at both readers, plus aCorruptRoomFileStillDoesNotStopAReap pinning the corrupt-room claim, which had no test.

Amendment 2 — forget narrowed the resurrection window, it did not close it

A drain that had already popped a save still had the write ahead of it — made outside the lock — and that write could land after deleteGraph removed the file and put the graph back. Observed, not hypothetical: aDeletedProjectIsNotHandedBackFromTheQueue failed with the lock-only version under full-suite load (gate log on my machine, once in two full runs). Fix: forget syncs against the drain queue, so the in-flight write completes before the delete's removeItem; the queue is the one place that ordering lives. Deterministic across repeated runs since.

Also filed #318 for aTickThatChangesNothingTellsNobody (PresencePollingTests, flaky ~1 in 5, unrelated to this PR) — it cost two full-suite gates while this review was in flight.

On deferring the sidecar-directory rule

Agreed. Sidecars in their own directory would make "every .json in projects/ is a graph" true by construction, but it needs a migration for rooms already on disk from beta3–beta5 and a downgrade story — exactly the scope that turns a same-day blocker into a week. Suffix list + save-through-persistence tripwire is a sound interim, now that the name cannot misfire.

Gate

Gate on 19028d20 (= pushed head, private DerivedData): xcodebuild test exit 0 — 1672 tests / 175 suites / 0 failures (+4 vs the branch's 1668); graphcode-cli build exit 0; graphcoded build exit 0; swiftlint 0 errors; swift-format strict clean; scripts/cli-smoke.sh exit 0 (5 verbs, throwaway daemon). Linux: pass on the same head (run 34079611052).