fix: upgrade, availability and state-honesty defects found after #27 - #28
Open
levi770 wants to merge 11 commits into
Open
fix: upgrade, availability and state-honesty defects found after #27#28levi770 wants to merge 11 commits into
levi770 wants to merge 11 commits into
Conversation
Two defects from the independent review of the merged branch. CH3 — the engine dispatches work nobody awaits from a timer and from several async callbacks: the pump tick, drains kicked off after a state change, and post-delivery verification. None of the seven sites had a catch. Node terminates the process on an unhandled rejection, so a rejecting transport or a full disk inside any of them takes the whole server down from a tick no caller can catch. A probe with a throwing transport produced eleven unhandled rejections in a few hundred milliseconds; in production the first one is fatal. Route every background dispatch through one guard that logs and continues. NEW-channels-1 — when the pane could not be read even once during the verify window there is no evidence either way, and the engine re-enqueued the item anyway, up to five times. That was safe while the drain held an unobservable session: the re-enqueued item waited until the pane could be read again. The hold is gone, so the re-enqueue now delivers immediately and blind, pasting the same onboarding prompt or nudge into the agent's context repeatedly. The contract comment on reenqueueStuck still asserted the vanished guard. Leave the durable .stuck-unobservable for the operator instead: force-deliver is a deliberate act with a human weighing the risk. The retry counter and its cap become unreachable and are removed rather than left as a dead knob. The regression test wires the same durability renames production wires; without them no .stuck file exists, the retry finds nothing, and an earlier version of this test passed against the unfixed engine while proving nothing. Restoring the re-enqueue now fails it.
…id-ACK W4 — startup awaited the session-identity migration before the first render, and that migration fetches from the server. A restarting server, a proxy blip, or an offline tab therefore left the operator with a text line instead of the app, recoverable only by a reload that fails the same way. A failed migration just means local storage keeps its old keys until the next boot; a failed render means there is no product. Move the boot order into its own module so the guarantee is testable, and render unconditionally. W1 — when a SUBSCRIBE ACK arrived for a surface the operator had already hidden, the client released the channel server-side and returned without clearing awaitingAck. The reveal path refuses to re-subscribe while that flag is set, so the cell came back with no channel: it swallowed every keystroke and offered no Reconnect. The ACK resolved that subscribe, so clear the flag on the way out. Both regressions fail without their fix: restoring the awaited migration leaves render uncalled, and dropping the flag reset leaves the revealed surface on one SUBSCRIBE instead of two.
NEW-agent-state-1 — the schema caps a health reason and REJECTS anything longer, while the producer trimmed to the looser detail cap. An OpenCode session error between the two limits therefore produced an envelope the authority refused, dropping the degradation it was reporting and leaving the session looking healthy. The limit was written twice as a bare literal; export it once and bound the producer to the number the schema enforces. W3 — a failed pulse kept the last successful views, so every indicator went on claiming "working" or "needs approval" for as long as the bridge was down. An unreadable authority is not evidence about the agent, and the model already says so: a session with no snapshot renders unknown on purpose. Drop the agent evidence on failure and keep lifecycle, since an unreadable authority is not a death certificate either and blanking it would flip every row to starting. CH1 — the review read the "nothing is dropped" guarantee as false. In context it describes lifecycle holds, and there it is accurate. The real gap is that the send-timeout path was never documented at all: after thirty seconds the engine stops waiting and does not retry, because the paste may still land and a retry would duplicate it. That is a defensible trade, but it has to be written down — the session shows send-failed, the durable record survives a restart, and the operator can revert by hand. Document it rather than trade the delay for a duplicate. Each regression fails without its fix: restoring the detail-cap trim emits a 180-char reason, and the view helper is asserted directly on tone, liveness, and the untouched non-agent row.
… the edge CH2 — queue restore removed the source file and only then rewrote the queue snapshot, so a crash between the two left the item in neither. Defer the removal until persistQueue has made the new snapshot durable; a .consumed file surviving a crash is re-read by the next restore, so the deferral cannot lose it either. The window was narrow — no I/O between the two steps — which is also why this carries no regression test: a faithful one would have to crash the process between two synchronous statements. The ordering is the fix, and the existing restore suite pins the behaviour around it. P3 — resume reached the manifest from the HTTP seam with no grammar check while profileId, three lines below, was validated with a comment explaining why. Validate it the same way, using the resume grammar the capture seam already owns rather than a second copy. Scoped deliberately to sessions Desk builds the command for. The first attempt validated unconditionally and broke a real case the suite already covered: a custom command receives resume as its own argument, so an operator wrapping their agent may use any token their wrapper understands and Desk has no standing to reject it. That case is now asserted alongside the rejections. This was never the traversal the review described — every consumer quotes the value and no path is built from it — so this is defence in depth: a malformed id now fails at the request that introduced it instead of at session launch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the defects an independent review found in #27 after it merged, plus two the review did not name. Nothing here changes what the product does on the happy path; every item is behaviour under failure, under load, or a contract that no longer matched the code.
Startup and upgrade
An upgrade from 0.3.1 could not start. The identity migration built its rename map only from sessions that spelled
tmuxSessionout in the manifest, but that name used to be derived at runtime, so a typical manifest carried no such field. Any queue item or paused entry keyed by a derived name was therefore unmapped, the migration refused to seed a partial journal, anddesk servethrew on every boot — with no option to acknowledge the drop, because the option named in the error did not exist. Identity is now reconstructed the way 0.3.1 derived it.The browser had the mirror-image problem:
main.tsxawaited a network-backed storage migration before the first render, so a restarting server or an offline tab left the operator looking at a line of text with no way back. Boot order moved into its own module and renders unconditionally.Losing and duplicating work
The terminal master exited on
EAGAIN. Client sockets are non-blocking, so a consumer that fell behind during a burst of output killed the master and the agent session under it — a plausible cause of "sessions die under load". Writes now queue per client with a bounded ordered buffer and a writability drain; a slow client is dropped, the master and its child survive.The generation ledger skipped a torn final line on replay but never truncated it, so the next append landed on the fragment and a later restart could reissue a generation already in use — collapsing the fence the whole state model rests on. The tail is now truncated and fsynced, with interior corruption still failing closed.
Channel delivery re-pasted a prompt it could not verify. When the pane could not be read even once, there was no evidence either way, and the engine re-enqueued anyway; that was safe while an unobservable session held its queue, and stopped being safe when observability stopped gating delivery. It now leaves a durable stuck record for the operator, because force-deliver is a deliberate act with a human weighing the risk.
Crashing and lying
Seven background dispatches — the pump tick, four drains, submit verification — ran with no rejection handler. Node terminates on an unhandled rejection, so a throwing transport or a full disk took the server down from a tick no caller could catch. One guard now covers all of them.
Three surfaces reported state they no longer had evidence for: a failed pulse kept showing the last known activity (now dropped to unknown, while liveness is preserved), a surface hidden mid-ACK stayed marked as awaiting one and could never resubscribe (a dead cell that swallowed input), and an OpenCode error longer than the schema's reason cap was rejected outright, silently losing the degraded fact it carried (the cap lived in two places and now lives in one).
OpenCode push facts also carried no provider session scope, so a subagent's
session.idlecould clear a parent's leases mid-turn. Facts are now accepted only on an exact endpoint, provider-session and producer-instance match.Contracts
The engine's own file header and the protocol page still described a ready-pane gate on standalone prompts that no longer exists, and the send-timeout path — thirty seconds, then no retry, deliberately, because a late paste plus a retry duplicates — was documented nowhere. Both now say what the code does.
Verification
Node 22.23.1 / npm 10.9.8 / Bun 1.3.14: typecheck clean; 305 files and 3179 tests pass, 6 skipped (live provider probes needing real credentials);
build:distributionsucceeds and the bundled atch answers--version; all six native targets build under-Werror; ASan and UBSan clean over the title scanner and event sink.Each fix was checked by removing it and confirming a test fails — a first attempt at the resume validation was rejected that way by the existing suite, for refusing a token a custom command legitimately owns.