Skip to content

Serve the Mailroom from a bounded mailbox request, not every broadcast (#288) - #293

Merged
scgopi merged 2 commits into
mainfrom
fix/288-mailbox-request
Sep 6, 2026
Merged

scgopi merged 2 commits into
mainfrom
fix/288-mailbox-request

Conversation

@scgopi

@scgopi scgopi commented Sep 6, 2026

Copy link
Copy Markdown
Owner

The defect

Every .graphChanged shipped the whole Mailroom to every client — 133 KB of a 176 KB frame on the live graph when the root-cause report for #288 measured it, and 166 posts of 193 KB by the time this was built. It went out on every change, including the presence tick, to clients that already had the posts and to one-shot CLI clients that never read mail. PR #291 stopped that frame from wedging the daemon; this makes it stop being enormous.

The change

The room leaves the daemon only when asked for, and a snapshot describes it instead of carrying it — #288's own proposal.

  • LoopGraph.mailroomDigest (count, latestID, a stable fingerprint) rides every .graphChanged in the posts' place. GraphStore broadcasts graph.wireSnapshot(); the graph it owns and persists is untouched, so nothing on disk changes shape. A graph that still carries posts — the daemon's own, or a snapshot from an older daemon — describes itself off them (boardDigest), so every reader works on both sides of the upgrade.
  • DaemonCommand.mailbox(projectPath:query:)DaemonEvent.mailbox(projectPath:mailbox:), answered on the asking connection alone. A MailboxQuery selects the whole room, one loop's unread slice, or one post; filters; and says whether bodies come whole, as headlines, or as the room decides by the existing triage rule. The arithmetic is one pure function in MailroomKit, Mailroom.serve, so the daemon, the tests and the shim's parity fixture all agree.
  • Bounded by construction. A headlined post keeps 80 characters of body — exactly enough for the CLI's 80-character triage line to render what the whole body would have — so a mail inbox on a full backlog is a ~36 KB answer instead of a 193 KB snapshot plus a 193 KB broadcast. highestDeliveredID is on the answer, ready for the cursor to advance to (next PR).
  • Every reader ported: the Swift CLI's mail inbox/read/list/post/status, the remote Python shim (RemoteGraphAccess.cliShimSource, parity-tested byte-for-byte against the Swift renderers, status and --mark now included), and the app — ProjectFeature carries its copy of the room across broadcasts and asks for a fresh one only when the digest says its copy is stale; AppFeature asks on a project's first snapshot and feeds an open workspace's rail.

Two visible changes in status output: the board line reads mailroom: 166 posts, unread mail for you / nothing unread for you rather than an exact unread count (the count is not derivable from a digest, and mail inbox is one command away), and mail post spells the topic itself rather than reading it off a post that is no longer on the wire.

Verification

Measured against a daemon built from this branch and the installed one, each isolated (GRAPHCODE_SUPPORT_DIR) and seeded with this project's live graph — 31 loops, 166 posts — over the raw socket:

installed daemon this branch
graphChanged frame 193,188 B 50,730 B (−74%)
posts on the wire per broadcast 166 0
mail inbox answer (room decides) 36,256 B, 166 headlines
mail inbox --full / mail list 142,725 B, on request only
mail read <id> 1,256 B

Gate: full Xcode test suite (1605 tests in 165 suites, 0 failures), swiftlint 0 errors, swift-format clean, graphcoded and graphcode-cli schemes build, SwiftPM swift build and .build/debug/graphcode (what the Linux job runs) pass locally.

New tests: MailboxTests (the snapshot, the fingerprint, Mailroom.serve's selections/search/triage bounds, the wire shape the shim types, and end-to-end over a socket that broadcasts carry no posts while the mailbox does and the registry routes/refuses like a command), MailroomFetchTests (the app asks only when the digest changes, carries its copy over, feeds an open workspace), and the renderer/parity tests rewritten around Mailbox.

Not in this PR

From the root-cause report, in their own PRs: encoding once per broadcast (after #291 lands, since it touches the same lines); advancing the cursor to highestDeliveredID instead of latest — the accepted race in graphcode-cli/main.swift is reworded here to point at that field and is closed there; coalescing presence-poll broadcasts; and #289's diagnostics.

Refs #288

🤖 Generated with Claude Code

https://claude.ai/code/session_01DeGL2CxuGmq16RSZpJYm2N

scgopi added a commit that referenced this pull request Sep 6, 2026
Never block the daemon actor on a client write (#288)

A graphChanged frame is 176 KB against an 8 KB socket send buffer, and it was
written with a blocking write(2) from inside the GraphStore actor. One client
that stopped reading for a moment — which is what the CLI does while it renders
— parked that actor and every other command for the project queued behind it:
a 6 ms round trip became a hard timeout past 12 s. Frames now go to a
per-connection OutboundChannel with its own writer thread, written non-blocking
so a wedged peer can neither stall the actor nor hang the close path.

Partially addresses #288 — the stall, not the amplification. #293 carries the
bounded mailbox response; #292 carries the registry re-keying.

Reviewed twice, independently; the review's own probe suite is included.
Gate: 1601 tests / 165 suites / 0 failures, twice. Linux CI green.
@scgopi
scgopi force-pushed the fix/288-mailbox-request branch from 52f096b to ac19002 Compare September 6, 2026 17:22
@scgopi

scgopi commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Independent review of ac19002 — I would not merge this yet

Reviewed as a wire-protocol change, in my own worktree off ac19002 (the force-pushed, rebased head), with everything below re-run and re-measured rather than read off the PR body.

The design is right and the execution is unusually careful — the digest, Mailroom.serve as one pure function, the headline budget chosen so the CLI's 80-character triage line renders identically either way, the shim parity. I verified all of it and it holds. One defect blocks: on version skew in one direction this silently destroys a loop's unread cursor. It is cheap to close, and the follow-up PR that closes it already exists.


What I verified myself

Claim Result
Full Xcode suite 1615 tests / 167 suites, 0 failures (the PR body's 1605/165 predates the rebase pulling in #291's two suites)
graphcode-cli + graphcoded schemes build ✅ both, 0 errors — the schemes the graphcode scheme does not cover
swiftlint / swift-format ✅ 0 errors, format clean
graphChanged frame, 182 posts / 32 nodes, isolated daemons 211,133 B → 54,308 B (−74.3%), matches the claimed −74%
posts on the wire per broadcast ✅ 182 → 0, including the broadcast a post itself causes
mail inbox (room decides) ✅ 39,944 B, 182 headlines
mail list / --full ✅ 157,078 B, on request only
Anything else shipping bodies in a broadcast ✅ nothing — the watcher nudge and the author's own log both go to NodeMemory, off-graph
Shim parity on a 182-post room ✅ byte-identical to the Swift CLI for mail list, list --json, list --search, read, inbox --headlines, inbox --json, inbox --mark, post (topic spelling), and the new status line. status's missing edges block is the skill's documented standing divergence.
MailroomDigest.fingerprint is UInt64 > Int64.max on the wire ✅ round-trips exactly through JSONEncoder/JSONDecoder; the shim never reads it. Not a problem.

The rebase is clean, and I checked it rather than trusting git. git diff origin/main..ac19002 is byte-identical to git diff a5f4e1e..52f096b apart from hunk headers — so #293's own patch is unchanged and #291 survived whole. Explicitly confirmed present at ac19002: (1) supersedingKey is "graphChanged:\(changed.id)", per graph, not a constant; (2) OutboundChannel still writes MSG_DONTWAIT + poll for writability; (3) the valve still measures pendingBytes - data.count > backlogBudget; (4) OutboundChannels.send still does guard let channel else { return false } rather than minting a channel on the fd. .mailbox correctly passes supersedingKey: nil, so an answer can never be superseded away.


🔴 Blocking — a 0.1.63 client against this daemon loses mail permanently, silently

Old CLI (~/.graphcode/bin/graphcode, 0.1.63) → daemon built from ac19002, isolated GRAPHCODE_SUPPORT_DIR, room seeded with 182 real posts:

$ graphcode mail list /tmp/g-seed
the room is empty — post one: graphcode mail post <project-path> <notice…>

$ graphcode mail read /tmp/g-seed 190
graphcode: no post #190 on this board — `graphcode mail list …` shows the ids that exist

$ graphcode status /tmp/g-seed | grep mailroom
(no mailroom line at all)

$ ZMX_SESSION=graphcode-<node> graphcode mail inbox /tmp/g-seed
no unread posts

Four confident false statements, no error, no warning. That alone is bad. This is what makes it blocking:

cursor lastMailroomRead now: 198        # was unset before that `mail inbox`
digest latestID: 198
unread posts the NEW daemon would now serve this loop: 0

The old CLI reads posts out of graph.mailroom (now empty), prints "no unread posts", and then still sends .mailroomInbox, which this daemon happily applies — the cursor jumps to the newest post. Those 182 posts are now unreachable via mail inbox for that loop forever, including after it upgrades. The display going blank is recoverable; the cursor advance is not.

The other direction is fine, and it is fine because someone already thought about this: a new client against an old daemon gets errorOccurred("unrecognized command — graphcoded may be older than the client that sent it") and fetchMailbox fails loudly. Verified over the raw socket. That is the correct shape — this direction just doesn't have it.

And the skew is reachable, not hypothetical. DaemonBootstrap.helpers is ["graphcoded", "zmx", "graphcode"] — the daemon is installed first, the CLI last. Any throw in between (the zmx copy, a lost staging rename between two concurrent sibling refreshers, a full disk) leaves new graphcoded + old graphcode on disk; installIfNeeded catches it, writes bootstrap.err.log and never writes the stamp — but the new daemon binary is already there, so the running daemon's staleness timer exits within 60 s and KeepAlive respawns the new one. Until the next app launch retries the install, every loop's graphcode mail inbox reports "no unread posts" and burns its cursor. refreshClosedSiblingWorkspaces runs the same ordering for workspaces with no window open.

Cheapest fix, and you already have it in flight: land fix/288-cursor-delivered with this, making .mailroomInbox carry the id to advance to. An old client sends no id and its cursor simply does not move — nothing is destroyed, and the blank board becomes a display bug that the next upgrade fixes. A one-line guard in mailroomInbox (refuse the advance from a connection that has never sent a .mailbox) would do it too. Reversing helpers to install graphcode before graphcoded is worth doing regardless, but it is a narrowing, not a fix.


🟠 Mailbox.highestDeliveredID names a cursor that skips unread mail, under search

Its doc says it is "what a cursor may honestly advance to, since it is the highest post the reader was actually handed", and the stated next PR advances the cursor to it. With a search it is the highest post that matched — every unread post below it that did not match was filtered out and never handed over. Against the seeded room:

searched unread ("release"): 78 posts, ids [6, 7, 8] .. [169, 170, 171]
highestDeliveredID = 171
posts NOT handed over that are below it: 104   (all unread for this reader)

Not reachable from today's CLI (inbox has no --search), but it is reachable over the protocol, the API invites it, and it is a landmine planted directly under the PR that is about to consume the field. Fails on ac19002:

@Test
func aSearchedInboxNeverPromisesACursorPastMailItFilteredOut() {
  var graph = LoopGraph(project: ProjectRef(path: "/tmp/x", name: "x"))
  graph.mailroom = [
    post(1, "nothing to do with it"),
    post(2, "also unrelated"),
    post(3, "the release is cut"),
  ]
  let reader = LoopNode(title: "Reader", loopType: .turnBased)
  graph.nodes.append(reader)

  let mailbox = Mailroom.serve(
    MailboxQuery(selection: .unread(reader: reader.id), search: "release"),
    from: graph.mailroom
  ) { graph.nodes[id: $0]?.lastMailroomRead }

  #expect(mailbox.posts.map(\.id) == [3])
  // #1 and #2 are unread and were never delivered, so no cursor may pass them.
  #expect(mailbox.highestDeliveredID == nil)
}
✘ Expectation failed: (mailbox.highestDeliveredID → 3) == nil

Fix: highestDeliveredID = nil whenever a search filtered the selection (or when .board), and say so on the field.


🟡 The app records the new digest before the posts arrive, so a lost answer is never retried

ProjectFeature.carryingRoom stamps the broadcast's digest onto state.graph and fires fetchBoard, which is try? await client.send(…) — errors swallowed, and a daemon errorOccurred reply just sets connectionError. If that answer never lands, the project holds the new digest with the old posts, and every later broadcast carrying that same digest is judged fresh. Nothing asks again until the room changes for some other reason. Fails on ac19002:

@Test @MainActor
func aProjectWhoseFetchNeverLandsAsksAgain() async {
  // … ProjectFeature TestStore, orchestratorClient.send records commands …
  await store.send(.daemonEvent(.graphChanged(room.wireSnapshot())))
  await store.finish()
  #expect(sent.value.count == 1)

  // The answer never comes. The next broadcast carries the same digest.
  await store.send(.daemonEvent(.graphChanged(room.wireSnapshot())))
  await store.finish()
  // The copy is still empty while the digest says it is current, so nothing asks again.
  #expect(sent.value.count == 2)
  #expect(store.state.graph.mailroom.isEmpty)
}
✘ Expectation failed: (sent.value.count → 1) == 2

Self-healing on the next post, so not blocking. The Mailbox already carries digest and the app ignores it — keeping a separate "digest my posts actually came from" and comparing that would close it exactly.


Nits

  • DaemonCommand.mailbox's doc says "Requires the project to be open on this connection". It doesn't: routing consults knownProjectPaths() and the guard is stores[canonicalPath], both process-wide, so any connection can read a resident project's room. Same as .graphCommand, which can mutate it, so this is not a new exposure — but the comment says something the code does not do.
  • MailroomDigest(of:) takes latestID from posts.last?.id while Mailroom.nextID takes it from map(\.id).max(). Equivalent today (append + order-preserving prune), but two different answers to the same question sitting three files apart.
  • Mailroom.serve(.board, fullBodies: false) on a one-post room returns bodiesTrimmed: true, which renderMailroom turns into "headlines only, that is a lot to read at once" for a single post. Unreachable from the CLI (list always asks fullBodies: true), but the flag means "I cut something" in serve and "there is a lot here" in the renderer.

What I tried to break and could not

The headline bound (byline + 80 always re-truncates identically, so no headline loses its ellipsis); the fingerprint against an author deletion, and its UInt64 JSON round-trip; unread across prune; --json escaping and dates through the shim; renderPosted's topic spelling against what the daemon actually stored (" MiXeD Case "posted #199 (mixed case), matching the stored post on both CLI and shim); reordering of a .mailbox answer against a superseded graphChanged; and the .graphCommand-shaped routing/refusal.

Verdict: merge once the cursor advance is skew-safe — either by landing fix/288-cursor-delivered alongside it or by guarding mailroomInbox. The highestDeliveredID-under-search fix belongs here too, since the PR that consumes the field is next. Everything else is a nit or a follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PfQ4GCBiAJV7wcY3KCdxp7

scgopi and others added 2 commits September 6, 2026 12:00
#288)

A .graphChanged carried the whole room — three quarters of a 176 KB frame
on the live graph, to every client on every change. Snapshots now carry
LoopGraph.mailroomDigest in the posts' place (wireSnapshot()), and posts
leave the daemon only through DaemonCommand.mailbox, answered on the asking
connection with a Mailbox: the whole room, one loop's unread slice, or one
post, searched and triaged to headlines by Mailroom.serve. The Swift CLI,
the remote Python shim and the app's rail all read through it; the app
carries its copy across broadcasts and asks again only when the digest
says it is stale.

Measured on an isolated daemon seeded with this project's live graph
(31 loops, 166 posts): graphChanged 193,188 B -> 50,730 B (-74%); a
default 'mail inbox' answer is a 36 KB headline mailbox.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeGL2CxuGmq16RSZpJYm2N
A project that adopted a broadcast's digest before the mailbox answer
landed judged every later broadcast fresh, so an answer that never came
was never asked for again. The digest a project holds is now the one its
posts came from (set by the .mailbox answer), and a first snapshot is held
without one. MailroomDigest reads latestID the way Mailroom.nextID does,
and DaemonCommand.mailbox's doc says what the routing actually requires.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeGL2CxuGmq16RSZpJYm2N
@scgopi
scgopi force-pushed the fix/288-mailbox-request branch from ac19002 to f47b4f3 Compare September 6, 2026 19:00
scgopi added a commit that referenced this pull request Sep 6, 2026
mail inbox read the room in one step and moved the cursor in another, and
the daemon moved it to the room's latest post — so a post landing between
the two was marked read without ever being printed, a race the CLI called
accepted. MailboxQuery.advanceCursor now moves the reader's cursor to
Mailbox.highestDeliveredID inside GraphStore.mailbox, in the same actor
turn the answer is drawn; persisted, never broadcast, refused to the asker
alone. An unread answer is a page (Mailroom.inboxPageSize) that says what
it left, so the rule is a bound: the cursor stops at the page and the same
command again is the next one.

From the #293 review: highestDeliveredID is the highest id below which
every unread post was handed over — nil for an empty page or a filtered
first post — and the daemon refuses advanceCursor with search. The legacy
GraphCommand.mailroomInbox moves no cursor and answers with an error naming
the version skew, so an older CLI that received nothing advances nothing.
--mark walks the unread mail through the mailbox page by page. The Swift
CLI and the remote shim send one request, and the accepted-race comment is
gone from both.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeGL2CxuGmq16RSZpJYm2N
@scgopi

scgopi commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Thanks — every finding reproduced on my side too. What changed, and where:

Blocker (0.1.63 CLI burns its cursor) — fixed in #295, which is meant to land together with this. GraphCommand.mailroomInbox now moves no cursor: the daemon answers it with an error naming the skew ("this graphcode CLI predates the daemon's mailbox — nothing was marked read. Upgrade graphcode…"), so a client that received nothing advances nothing and the failure is loud and recoverable. The new CLI never sends it; --mark walks the unread mail through the mailbox page by page. Tests: the legacy command leaves the cursor untouched and announces the skew.

highestDeliveredID under search — fixed in #295 with both halves: it is now the highest id below which every unread post was handed over (nil for an empty page, nil when the first unread post was filtered out, the matched prefix otherwise, a page's edge only when nothing below it was skipped), and the daemon refuses advanceCursor together with search (MailboxRefusal, to the asker). Your test is in verbatim, plus one per answer shape and one for the refusal.

Digest recorded before the posts arrive — fixed here in f47b4f3: the digest a project holds is the one its posts came from (set by the .mailbox answer, which already carried it), a first snapshot is held without one, so the next broadcast after a lost answer asks again. Your aProjectWhoseFetchNeverLandsAsksAgain is in, plus the app-level twin.

NitsDaemonCommand.mailbox's doc now says "resident in the daemon"; MailroomDigest reads latestID as max like nextID. The bodiesTrimmed-on-one-post wording is unreachable from the CLI (list always asks for whole bodies; an explicit --headlines suppresses the header) — left as is.

Also rebased onto main 2b6b600. Gate on the rebased head is in flight; the pre-rebase head passed (1615/167, both schemes).

@scgopi
scgopi merged commit c7f3476 into main Sep 6, 2026
1 check passed
scgopi added a commit that referenced this pull request Sep 6, 2026
Advance the mail cursor to the highest post handed over (#288)

mail inbox read the room in one step and moved the cursor in another, to the
room's latest post — so a post landing between the two was marked read without
ever being printed. The CLI called that race accepted. The cursor now moves to
Mailbox.highestDeliveredID inside the same actor turn the answer is drawn, so
nothing can land in between: a cursor moves through mail that was delivered,
never past mail that was not.

This also closes the blocker #293 carried. An older CLI still sending the legacy
mailroomInbox read an empty board off a snapshot that no longer holds posts and
advanced its cursor anyway, losing that mail permanently, upgrade or not. The
legacy command now moves no cursor and is refused loudly, to the asking
connection alone.

Two defects the review found in the fix itself, both closed: a searched inbox
could advance the cursor past unread posts it had filtered out, now refused in
the daemon rather than by convention; and a page smaller than retention opened a
window where page two could be pruned before the reader asked for it, so the page
is now the room's own cap and pruning between requests is reported as
prunedUnread rather than passing in silence.

Reviewed independently; every fix above was verified in the pushed source by the
reviewer and again by me. Linux CI green; the mail suites pass at exit 0 on this
head and #296's full gate at the sibling head is 1621/168/0.

Lifts the release hold on main. Partially addresses #288.
scgopi added a commit that referenced this pull request Sep 7, 2026
Every graph change re-serialised the whole graph — 323 KB on the graph
that filed the issue, 271 KB of it mail bodies that had not changed — and
wrote it to disk synchronously inside GraphStore.broadcast(), holding the
actor for as long as the disk took: a memo measured at 0.03–2.13 s
against a 0.003 s socket round trip. #288's shape, one layer over, and
the payload #293 removed from the wire was still in the file.

Two fixes, each sufficient on its own. The room lives in its own file
(<name>.mailroom.json) written only when its digest changed; the graph
file, rewritten on every change, carries no post. A graph saved before
the split still loads its inline room. And saves are handed to a
GraphWriter — one serial queue, latest snapshot per project wins, so a
burst of memos is one write — and the actor returns at once; the daemon
flushes it on both exit paths, and tests that read the file straight
after a command ask for synchronous persistence.

Closes #307.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeGL2CxuGmq16RSZpJYm2N
scgopi added a commit that referenced this pull request Sep 7, 2026
Persist the room beside the graph, and write off the actor (#307)

`broadcast()` called `onGraphChanged` synchronously on the `GraphStore` actor, and
that wrote the whole graph to disk — 323 KB, of which 271 KB was the Mailroom.
#293 took the room out of the snapshot clients receive and left it in the file
rewritten on every mutation, so the broadcast got 77% smaller and the disk write
did not move at all. Structurally this was #288 again, one layer over: a slow I/O
call holding the actor. Measured at 0.03-2.13s for `node memo` against a 0.003s
socket read, on a path every loop uses to leave anything behind.

The room now lives in its own file and is written only when its digest changes, so
a node edit no longer rewrites every post. `GraphWriter` takes the snapshot and
writes off the actor, keeping the latest per project and flushing on both exit
paths.

Moving the write off the actor introduced a staleness the review's gate caught
only once merged: anything re-reading the persisted graph could see an older one,
and deleting a *closed* project loads it from disk to find the sessions it must
end — so a delete could silently end fewer sessions and leave loops running.
`GraphWriter.load` now returns the queued snapshot ahead of the file and every
registry read goes through it, which fixes the orphan and the tests with one
mechanism rather than flushes sprinkled at call sites.

Found as an aside by an end-to-end loop that had ruled out #288 by testing with
and without a wedged client. Every measurement in the #288 series was taken at the
socket; the disk half was never instrumented.

Gated on the merged result at its rebased head: 1652 tests / 173 suites / 0
failures, no restarts; the built CLI answers status, mail list, mail post, mail
read and projects at exit 0; Linux green.

Closes #307.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant