Never block the daemon actor on a client write (#288)

[scgopi]

The defect

graphcode mail inbox timed out against a daemon that was never busy, only blocked.

A graphChanged frame carries the whole graph — 175,937 bytes on the live graph, of which 132,661 (75%) is the mailroom — while an AF_UNIX socket's SO_SNDBUF is 8,192 bytes. A frame 21× the send buffer cannot be handed to the kernel and forgotten: the blocking write only completes as the peer drains it, repeatedly, mid-frame.

Both callers doing that writing are actors. So one client that stopped reading for a moment — which is exactly what the graphcode CLI does while it renders its output, holding its connection open under defer { client.closeConnection() } — parked a GraphStore thread inside write(2), and every other command for that project queued behind it until that client happened to exit.

Measured against the live daemon over the raw socket, before the fix:

BASELINE openProject round trip: 0.006s (frame 175937 bytes)
bytes pushed into one deaf client before blocking: 24576
STALLED openProject: TIMED OUT after 12.013s

The CLI's own receive budget is 10 s. It freed the instant the wedging client exited, which is why it read as intermittent.

The change

Frames go to a per-connection OutboundChannel with a writer thread of its own, so the actor hands a frame over and returns.

• Non-blocking writes. Every write is MSG_DONTWAIT with a bounded poll for writability. This is not an optimisation: a thread already parked inside a blocking write(2) on a unix socket is not reliably woken by another thread's shutdown, so a wedged peer could hang the disconnecting caller for ever — the same bug, moved from the actor to the connection loop. Per call rather than O_NONBLOCK on the descriptor, because that flag is shared with the daemon's reader.
• Supersession, per graph. An undelivered graphChanged is replaced by a newer one of the same graph — the event carries the whole graph and never a diff. Keyed per graph and not on the event name, because one connection joins many projects over one socket.
• Backlog budget. Past 4 MB the client is dropped and told so on stderr. The newest frame is excluded from the measurement, so a single large snapshot is never mistaken for a backlog.
• Descriptor ownership. close stops the writer and waits before closing. open refuses to inherit a dead channel, send refuses an unregistered descriptor, and close refuses a descriptor it has no channel for — descriptor numbers are recycled aggressively.

Verification

End-to-end against a freshly built daemon in an isolated support dir, seeded to a 200,461-byte frame:

	before	after
baseline round trip	0.006 s	0.002 s
round trip with a deaf client attached	timeout, 12.0 s	0.006 s

Independently reproduced by a reviewer with its own script against its own daemon: 12.0 s → 0.018 s.

Gate: 1601 tests / 165 suites / 0 failures, run twice with no restarts; swiftlint 0 errors, swift-format clean; graphcoded and graphcode-cli schemes build; Linux CI green.

Review history — worth reading before approving

This PR was wrong three times, and each time something other than the author's judgment caught it. The seams below are where a reviewer's attention is best spent.

Found by	Defect
Linux CI	Glibc imports SHUT_RDWR as Int; the PR was called green on the macOS gate alone while CI was already red
Both reviews	Cross-project supersession — a constant "graphChanged" key let project B's snapshot delete project A's undelivered one, leaving a client permanently stale on A
Review	Lazy channel creation minted a channel on a recycled descriptor, handing a departed connection's frame to its new owner
Review	The backlog valve counted the frame it had just appended, so one oversized snapshot tripped it alone
Repeated full-suite runs	A blocked write(2) is not woken by shutdown — closeAndWait could hang for ever, in the seam an earlier commit claimed to have made safe
Full suite	send(2) only works on sockets; tests attach /dev/null

graphcode/Tests/OutboundChannelReviewTests.swift is the reviewer's own probe suite, taken wholesale — it drives the real GraphStore path and forces genuine descriptor reuse with dup2, and its 150-round close/recycle stress test is what any future change in this seam should have to pass.

Scope

Partially addresses #288 — it removes the stall, not the amplification. The bounded mailbox response, the cursor advancing only through delivered posts, and avoiding a full-graph broadcast for a cursor update are tracked separately and already in progress (measured at −74% on the frame). Deliberately not closing #288.

#292 files the remaining structural half of the descriptor-recycling story: ProjectRegistry.removeConnection is reentrant and the connection registry is keyed by descriptor rather than connection UUID. Pre-existing, and too large to fold in here.

Also for triage: #290 is a byte-identical duplicate of #289 and can be closed.

Related to #288, #289.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BP43ags4cn8fq2ZZdv85J9

[scgopi]

Review — independent verification of #291 (head 5adb642)

Verdict: request changes, then merge. The core change is right and fixes a real, daemon-wide stall that I reproduced myself. But as it stands it introduces one user-visible regression (a client joined to several projects can be left permanently stale on one of them) and keeps one wrong-connection write the new registry was in a position to remove. Both are one-to-three-line fixes, verified below with the full gate green. I would not merge 5adb642; I would merge it with those folded in. Everything I checked is in the tables — nothing here is taken from the PR body on trust.

What I verified independently

Claim	Result	How
Gate 1597 tests / 164 suites / 0 failures	✅ confirmed on 5adb642	full xcodebuild test from my own worktree
swiftlint 0 errors, swift-format clean	✅	ran both
graphcoded + graphcode-cli schemes build	✅	built both
Stall reproduction (deaf client wedges every other command)	✅ 12.0 s timeout before → 0.018 s / 0.105 s after (two runs)	my own raw-socket script against a fresh isolated daemon seeded to a 200,979-byte frame; "before" = installed 0.1.63
Linux CI	❌ red — OutboundChannel.swift:182:30: cannot convert value of type 'Int' to expected argument type 'Int32' (Glibc's SHUT_RDWR is Int)	gh pr checks 291

Findings

#	Severity	Finding	Evidence
1	🔴 regression, blocks merge	Cross-project supersession. GraphStore.send keys every snapshot with the constant "graphChanged", but one connection is joined to many stores (ProjectRegistry.joinSidebars adds every sidebar to every open project on the same fd; the app also openProjects each restored project over its one connection). While the writer is parked on a frame, project B's snapshot deletes project A's undelivered snapshot. A's client stays stale until A changes again — which may be never. Pre-PR the blocking write delivered every snapshot, so this is new.	Failing test snapshotsFromDifferentProjectsMustNotSupersedeEachOther: delivered → ["/tmp/review/b"]. Also reproduced end-to-end on the PR daemon: a client joined to A and B, C creates node NEEDED in A then mutates B → the client received A's earlier snapshot and B's, and NEEDED never arrived.
2	🟠 fix before merge	OutboundChannels.send opens a channel on an unregistered descriptor. After close(fd) the kernel recycles the number; a store still holding it (it only forgets a connection on its next refused send, and see the pre-existing race below) sends → a channel is created on whatever socket owns that number now → open() from the new connection then keeps it as alive. The new client receives a frame meant for a connection that no longer exists. The registry has exactly the information needed to refuse this and doesn't use it.	Failing test aStaleSendMustNotBindAChannelToTheNextOwnerOfTheDescriptor: accepted → true, and the new peer's first frame is the stale one. With lazy creation removed the full suite is still green (1600/165/0) — so open() at the two registration sites already covers the "silent frame loss" that motivated it.
3	🟡 sharp edge	The valve counts the frame it just appended, so one frame > 4 MB trips it on a queue of one; every client would be dropped on the broadcast it joined for. Not reachable today (50 nodes, 1 KB post bodies, 200+200 posts ≈ well under 1 MB), but the failure mode is total and silent to clients.	Failing test oneFrameLargerThanTheBudgetIsNotABacklog: .connectionClosed.
4	🟡 pre-existing, not made worse — but the PR's ownership claim doesn't cover it	ProjectRegistry.removeConnection is reentrant. It iterates a copy of connectionProjectPaths[id] with an await per store. While suspended, another connection's openProject of a new path runs joinSidebars, which inserts the departing sidebar into the new store (connectionProjectPaths[X].insert(B) + storeB.addConnection(X, fdX)). The loop never sees B, close(fdX) runs, and storeB holds X → fdX forever: every later B broadcast goes to whichever connection is handed that number — including a one-shot CLI, which prints the first graphChanged it sees as its own project. Needs the app disconnecting at the same instant a CLI opens a new folder, so rare.	Reasoning from ProjectRegistry.swift:128-137 and :390-397; I did not manage a deterministic failing test for the interleaving. Real fix: key the registry by connection UUID (every caller has it) rather than by fd, and re-read the path set after each await in removeConnection.
5	⚪ nit	OutboundChannels.close calls posixClose(fd) even when it had no channel for that number — a second call for a number would close someone else's descriptor. Only one caller today; worth a guard or a comment. Also: BoardsOffRegressionTests hands stores raw /dev/null fds and closes them itself, leaving live channels in the global registry keyed by numbers later tests reuse — harmless today because channels are number-bound, but it is the same mechanism as #2.	reading

What I could not break

Seam	Result
Close path (closeAndWait → shutdown → wait → close(2))	No ordering defect found. shutdown(SHUT_RDWR) is issued before the wait in every path that sets isClosing; a detached channel is only ever one whose shutdown already went out, so no channel shuts a number it no longer owns; lock order is registry → channel only. 150 rounds of 4-lane concurrent sends racing close with the freed number recycled straight into the next pair: no hang, no crash, 1.4 s (concurrentSendsAndClosesWithRecycledDescriptorsNeverHang, passes on 5adb642 too).
errorOccurred being dropped	Cannot happen by supersession (unkeyed frames are never removed); only the valve or close drops them, both of which end the connection. Ordering across keys is preserved.
4 MB valve	Defensible once #3 is fixed and the key is per project: a sidebar's backlog is then bounded by open projects × snapshot (~23 at today's 176 KB; ~90 once the mailroom leaves graphChanged). Reaching it genuinely means a stopped reader.

Branch with the tests and candidate fixes

review/291-probes — two commits on top of 5adb642, so the tests can be cherry-picked alone:

1. 9d0f2ca Add review probes for the outbound channel — graphcode/Tests/OutboundChannelReviewTests.swift (4 tests; the first three fail on 5adb642).
2. 909acec Candidate fixes for the review probes — per-project key in GraphStore.send, send returns false for an unregistered fd, valve measures pendingBytes - data.count. Full gate with both commits: 1600 tests / 165 suites / 0 failures.

Still needed from the author: the Glibc SHUT_RDWR cast for Linux, and I'd drop Closes #288 to a reference — the mailroom-out-of-graphChanged half is real and separate.

[scgopi]

All five findings addressed, plus one your probes did not reach. Thank you for the branch — I took OutboundChannelReviewTests.swift wholesale rather than paraphrase it, because your probes drive the real GraphStore path and force actual descriptor reuse with dup2, where mine only asserted on raw superseding keys.

#	Finding	Done
1 🔴	Cross-project supersession	Keyed graphChanged:<graph.id>
2 🟠	Channel minted on an unregistered descriptor	Lazy creation removed; send refuses an unknown fd, which is also the signal GraphStore uses to forget a connection
3 🟡	Valve counted the frame it just appended	Measures pendingBytes - data.count, so a single frame always passes
4 🟡	removeConnection reentrancy / fd-keyed registry	Filed as #292 with your suggested fix — a registry-wide re-keying is too large to fold in here
5 ⚪	close posix-closing a descriptor it had no channel for	Guarded
—	Glibc SHUT_RDWR	Int32(SHUT_RDWR); Linux CI green
—	Closes #288	Dropped to a reference; the mailroom half is separately in progress and already measuring −74%

One correction to your review

Close path — no ordering defect found. 150 rounds ... no hang, no crash.

The stress test passes, but the seam is not sound. The full suite hung twice on sendingToAClientThatNeverReadsDoesNotBlockTheCaller, while passing in isolation every time. Cause: a thread already parked inside a blocking write(2) on a unix socket is not reliably woken by another thread's shutdown, so closeAndWait could wait for ever. That is the original bug moved from the actor to the connection loop — in the exact seam I had claimed my test ruled out.

Writes are now non-blocking per call (MSG_DONTWAIT + bounded poll), so the writer notices isClosing itself instead of depending on being interrupted. MSG_DONTWAIT per call rather than O_NONBLOCK on the descriptor, since that flag lives on the open file description the daemon's reader shares.

Your 150 rounds did not catch it because every round's peer is drained or closed promptly; the hang needs a peer that stops reading and stays stopped while a close races it. Worth knowing that the stress test is not sufficient on its own.

That also caught a follow-on: send(2) only works on sockets, and several suites attach /dev/null, so the channel now picks its write call from what the descriptor actually is.

Gate

1601 tests / 165 suites / 0 failures, run twice with no restarts (the hang reproduced within one run before the fix). swiftlint 0 errors, swift-format clean, both daemon schemes build, Linux CI green.

Ready for another look at 518856c.

[scgopi]

Re-review of 518856c — approve, merge it.

Your correction on the close path is accepted: my 150-round stress drained or closed every peer promptly, so it never held a writer parked while a close raced it. The non-blocking writer removes the dependency on shutdown waking a parked write(2) at all, which is the right shape regardless of how reliable that wakeup is on Darwin.

Verified independently on 518856c

Check	Result
Full gate	✅ 1604 tests / 165 suites / 0 failures (your 1601 + three new probes of mine, below)
Stall reproduction, my script, deaf client attached	✅ baseline 0.001 s → 0.006 s with the deaf client → 0.006 s after it leaves
Cross-project supersession, end-to-end	✅ client joined to A and B now receives ['NEEDED', 'Poke'] for A, then B
Both channel suites, -run-tests-until-failure -test-iterations 40	✅ 520 runs, 0 failures, no hang, 46 s — the scenario that hung you twice did not reproduce once
Linux CI	✅ green

Attacking the new write path

Seam	Probe	Result
Partial writes mid-frame	13 frames of 1 B … 300 KB through a 2 KB SO_RCVBUF/SO_SNDBUF, peer draining in gulps with delays	✅ every frame intact, in order, none duplicated — pointer advances by exactly written, and EINTR/EAGAIN both leave it in place
Close landing mid-frame	200 KB frame parked, second queued, close after 20 ms	✅ the peer sees at most complete frames and then EOF — a truncated frame is always followed by shutdown, never by more bytes it could read as a header. That holds for every path that sets isClosing (closeAndWait, the valve, a failed write); detach only ever runs on a channel whose shutdown already went out
Peer vanishes while the writer is parked in poll	4 × 200 KB parked, then close(client) with nobody closing the channel	✅ noticed within the 50 ms slice via POLLHUP/EPIPE; the next send returns false
EINTR	by reading: send → continue with the same pointer; poll → loop	✅ correct; I did not find a way to inject it deterministically
written == 0	falls through the EAGAIN guard → false	✅ no spin
Anything else still writing to a connection fd around the channel (the old per-fd writeLocks is bypassed now)	grep	✅ only client-side writers remain; the channel is the sole daemon-side writer

Probes are on review/291-probes-2 (one commit on top of 518856c, appended to the file you took) if you want them.

Two notes, neither blocking

• Key by graph.id vs the client routing by project.path. The app keys state.projects[id: path]; the id is decoded from the persistence file and minted fresh on import/export, so it is 1:1 with a path today. It only stops being so if someone hand-copies a projects/<slug>.json — then two paths share an id and the cross-project loss quietly returns for that pair. Keying by scope/path would make the daemon and the app agree on identity by construction. Your call.
• A dropped-but-alive-and-silent client leaks its connection Task. The valve shuts the socket down, but the connection loop's reader only returns when shutdown wakes it or the peer closes. If shutdown waking a parked write is unreliable, a parked read deserves the same suspicion; the cost is one leaked fd + Task per hung client, not a stall, so it belongs with ProjectRegistry.removeConnection is reentrant, and the connection registry is keyed by descriptor #292 rather than here.