Make the bridge's on-disk stores safe to publish and safe to watch - #45
Merged
Conversation
paired-phones.json, mobile-access-policy.json and trusted-peers.json were each written with a plain writeFileSync, which truncates before it writes. A reader arriving in that window sees an empty or half-written document, and for the remote-access policy that is the sole machine-level authorization gate reading as unset. All three now go through atomicWriteFile. The scratch file is scoped to the pid rather than shared. A single `<path>.tmp` is itself a contended file: the `antgrid` CLI writes these same stores while the host runs, both truncate into one scratch path, and the rename then publishes the blend through the very step meant to prevent one. Per-pid keeps it self-reclaiming, so a write interrupted by a kill leaves at most one stale file per target. Windows refuses a rename whose target another process holds open — a plain reader is enough — as a MoveFileEx sharing violation surfaced as EPERM/EBUSY/EACCES. Without a retry the fix would have traded silent corruption for a thrown write. It retries with no delay first and sleeps only in the tail, short-circuits on a target that will refuse however long we wait (a read-only file and a directory in the way report the same codes), and is empty on POSIX, where those codes only ever mean a permanent fault. Three consequences of publishing by rename that the old truncating write did not have. The target is briefly unlinked, so remote-access-policy re-checks before believing the file absent — absence is the one verdict that WRITES, routing to a v1 migration that derives false from grants shed on the first upgrade and persists it, permanently revoking a machine the user had enabled. That migration now also refuses to persist a derived false unless both v1 stores actually answered. And setEnabled persists before flipping memory, so a failed write cannot leave the gate reading off while disk says on. flush() can now throw where it could not before, so paired-phones guards its unref'd last-seen timer and close() (an EPERM there reached the event loop as an uncaughtException and took the whole bridge down over a timestamp), clears touchWriteRaw before the write and arms it only after one lands, and drops pendingTouches only once the write succeeded.
A rename is atomic only within a filesystem, so the scratch file has to sit in the target's own directory — and those directories are the user's, not ours. `antgrid.yaml` is written in their git working tree, where a leaked `antgrid.yaml.<pid>.tmp` shows up in `git status` and can be committed (nothing ignores it); `host.json` leaks a full copy of the control-plane token into <abDir>, which removeHostFile never reaps; the hook writers leak into ~/.claude and friends. Nor is this a crash path. Force-kill is the ROUTINE teardown on Windows — the app assigns the host to a job object with KILL_ON_JOB_CLOSE precisely so the kernel sweeps the tree — so the catch-block cleanup only ever covers a write that threw, never one that was killed. The sweep runs once per target per process, since the readdir is work the write itself does not need and a live writer rewrites anything it loses. It matches on the whole basename, so two stores sharing a directory cannot reap each other, and it skips any pid still running: pid reuse can then only make us keep a stale file, never delete one being written. The pre-<pid> shared `<path>.tmp` parses as 0 and is skipped — a bridge old enough to still write that name could be writing it now.
ConfigController.watch armed fsWatch on the FILE path, while write() publishes by rename through atomicWriteFile. An inotify watch is keyed to the inode it was armed on, so the first save orphans it and every later one is invisible for the life of the process. Measured, not inferred. Under Docker on node:22-alpine the callback counts on one watcher were 1 / 4 / 4 / 4 across write, rename-publish, write-again, rename-again — dead after the rename, identical on overlayfs and tmpfs. Bun 1.3.14 on Linux goes deaf too (1 / 2 / 2 / 2), and Bun's fs.watch is its own Zig implementation rather than libuv, so this is two independent implementations landing on the same inode semantics. The same probe on Windows gives 2 / 4 / 6 / 8: ReadDirectoryChangesW subscribes to the parent with a name filter, which is exactly what masked this. config:write applies nothing itself — it only replies config:write-result — so the watcher firing is the sole thing that restarts agents and starts or stops services per computeDiff. On Linux, saving config in the app reported success and silently did nothing until the bridge restarted. The fix is one unconditional directory watch with a basename filter, which is what the same function's own fallback arm and paired-phones.ts already did. The filter over-accepts on purpose, because a spurious hit costs one debounced re-read of a fixed path while a dropped one is the deafness itself: case-insensitively, since antgrid.yaml is user-authored and every other access resolves it case-insensitively on Windows and macOS; the .tmp scratch, since Windows names ONLY the scratch when the rename target does not yet exist, which would make a config's first creation unobservable; and a nameless event, which carries nothing to filter on.
sessions.json had two writers — an async one that awaited between its write and its rename, and a 200ms debounced sync one — sharing a single scratch filename. They interleave with no second process involved: measured, reverting the fix gives an ENOENT rejection when the sync writer renames the shared scratch out from under the async one, a resurrected row from two overlapping deletes each republishing what it read, and an EPERM from both colliding on that one name. session-manager.ts's own comment states the cost of the corrupt case: a truncated sessions.json is read as empty and silently drops every session. Both now go through one synchronous publish built on atomicWriteFile, so there is a single scratch-name and rename-retry policy for the file, and the publish occupies one turn with nothing able to interleave. withSessionsFile (createKeyedLock, the mechanism CheckoutStore already uses for its sibling file) covers the writers that SUSPEND: deletePersisted's read-modify-write, and the debounced flush now queued behind it rather than firing into the middle of one. flushNow deliberately stays OFF that lock — teardownServices does not await it, so its write has to land on the tick or the last activity bump is lost on every clean shutdown. That is safe only because the publish never suspends, and the doc comments say so rather than claiming a mutual exclusion the code does not hold: making the claim true by locking inside the primitive would deadlock every isolated-session create and every cold delete, since createKeyedLock is not reentrant and both awaited callers already hold the key. A fired debounce whose write is still queued on the lock now sets flushQueued, because flushNow read a null flushTimer as clean and would have skipped the final write on exactly the shutdown it exists to cover. The delete-persisted suite's leaked-scratch assertion named the old fixed sessions.json.tmp, which is now a filename nothing writes — it passed even with the rename deleted outright. It matches by prefix instead.
CI caught this on Linux: two paired-phones watcher tests timed out at ~3s waiting for an event that never came. Converting that store to publish by rename left its watcher filtering on the target name alone. Measured, one rename-publish into a watched directory: Node on Linux delivers rename:<scratch>, change:<scratch>, rename:<scratch>, rename:<target> — but Bun 1.3.14 delivers exactly ONE event, rename:<scratch>, and never names the target at all. Windows reports only the scratch too when the target does not yet exist. So on the runtime the bridge actually ships, an exact-name filter is permanently silent: no reload after `antgrid phones remove`, no re-advertise to any connected phone. isWatchEventFor moves to discovery.ts, the module that owns the <path>.<pid>.tmp convention, so the predicate lives with the thing it recognizes instead of being hand-rolled per watcher. config-controller's copy folds into it. The two are the only filename-filtered watchers in the tree. The regression test asserts the predicate rather than driving a live watcher: this event shape is Bun-on-Linux only, so a behavioural test passes on a Windows or macOS dev machine whether or not the bug is present — which is exactly how it reached CI. Verified in a Linux container on oven/bun:1.3.14, which reproduces CI's two failures precisely: 22 pass / 2 fail with the old filter, 24 pass / 0 fail with this one. The live-reload test goes from a 3047ms timeout to passing in 55ms.
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.
Four commits hardening how the bridge persists state. Each one started as a defect found by measurement, and the measurement is in the commit message.
1. Every machine-level store publishes by rename, never by truncation
paired-phones.json,mobile-access-policy.jsonandtrusted-peers.jsonwere written with a plainwriteFileSync, which truncates before it writes. A reader arriving in that window sees an empty or half-written document — and for the remote-access policy that is the sole machine-level authorization gate reading as unset. All three now go throughatomicWriteFile.The scratch file is scoped to the pid rather than shared. A single
<path>.tmpis itself a contended file: theantgridCLI writes these same stores while the host runs, both truncate into one scratch path, and the rename then publishes the blend through the very step meant to prevent one.Windows refuses a rename whose target another process holds open — a plain reader is enough — as a MoveFileEx sharing violation surfaced as EPERM/EBUSY/EACCES. Without a retry the fix would have traded silent corruption for a thrown write. It retries with no delay first, short-circuits on a target that will refuse however long we wait (a read-only file and a directory in the way report the same codes), and is empty on POSIX, where those codes only ever mean a permanent fault.
Three consequences of publishing by rename that the old truncating write did not have, all handled here:
remote-access-policyre-checks before believing the file absent. Absence is the one verdict that writes, routing to a v1 migration that derivesfalsefrom grants shed on the first upgrade and persists it — permanently revoking a machine the user had enabled, since a v2 file never re-migrates.falseunless both v1 stores actually answered.setEnabledpersists before flipping memory, so a failed write cannot leave the gate reading "off" in memory while disk still says "on".2. Reap the scratch file a killed writer left in the user's directories
A rename is atomic only within a filesystem, so the scratch file has to sit in the target's own directory — and those directories are the user's.
antgrid.yamlis written in their git working tree, where a leakedantgrid.yaml.<pid>.tmpshows up ingit statusand can be committed (verified: nothing ignores it).host.jsonleaks a full copy of the control-plane token into<abDir>, whichremoveHostFilenever reaps.Nor is this a crash path — force-kill is the routine teardown on Windows, since the app assigns the host to a job object with
KILL_ON_JOB_CLOSEprecisely so the kernel sweeps the tree.The sweep runs once per target per process, matches on the whole basename so two stores sharing a directory cannot reap each other, and skips any pid still running — so pid reuse can only make us keep a stale file, never delete one being written.
3. Watch the directory: a renamed-over config file goes deaf on Linux
ConfigController.watcharmedfsWatchon the file path, whilewrite()publishes by rename. An inotify watch is keyed to the inode it was armed on, so the first save orphans it and every later one is invisible for the life of the process.Measured under Docker. Cumulative callback counts on one watcher across write → rename-publish → write-again → rename-again:
Bun's
fs.watchis its own Zig implementation rather than libuv, so this is two independent implementations landing on the same inode semantics — the bridge runs on Bun, so runtime choice does not save it. Identical on overlayfs and tmpfs, so it is inotify semantics and not a storage-driver artifact. The Windows control run reproducing the earlier measurement is what shows the probe measures the real thing. Scope: measured on a WSL2 kernel, not bare metal.Why it matters.
config:writeapplies nothing itself — it only repliesconfig:write-result— so the watcher firing is the sole thing that restarts agents and starts or stops services percomputeDiff. On Linux, saving config in the app reported success and silently did nothing until the bridge restarted.The fix is one unconditional directory watch with a basename filter, which is what the same function's own fallback arm and
paired-phones.tsalready did. The filter over-accepts on purpose, because a spurious hit costs one debounced re-read of a fixed path while a dropped one is the deafness itself:antgrid.yamlis user-authored, so a repo may carryAntgrid.yaml, and every other access resolves it case-insensitively on Windows and macOS. An exact compare would find the file, seed from it, serve reads off it — then ignore every save to it..tmpscratch. Windows names only the scratch when the rename target does not yet exist, so dropping it makes a config file's first creation unobservable.4. Serialize the two sessions.json writers on one publish primitive
sessions.jsonhad two writers — an async one that awaited between its write and its rename, and a 200ms debounced sync one — sharing a single scratch filename. They interleave with no second process involved. Reverting the fix reproduces three distinct failures: an ENOENT rejection when the sync writer renames the shared scratch out from under the async one, a resurrected row from two overlapping deletes each republishing what it read, and an EPERM from both colliding on that one name.session-manager.ts's own comment states the cost of the corrupt case — a truncatedsessions.jsonis read as empty and silently drops every session.Both now go through one synchronous publish built on
atomicWriteFile, so the publish occupies a single turn with nothing able to interleave.withSessionsFile(createKeyedLock, the mechanismCheckoutStorealready uses for its sibling file) covers the writers that suspend:deletePersisted's read-modify-write, and the debounced flush now queued behind it.flushNowdeliberately stays off that lock —teardownServicesdoes not await it, so its write has to land on the tick or the last activity bump is lost on every clean shutdown. The doc comments say exactly that rather than claiming a mutual exclusion the code does not hold: making such a claim true by locking inside the primitive would deadlock every isolated-session create and every cold delete, sincecreateKeyedLockis not reentrant and both awaited callers already hold the key.A fired debounce whose write is still queued on the lock now sets a flag, because
flushNowread a nullflushTimeras clean and would have skipped the final write on exactly the shutdown it exists to cover.Testing
bun run --filter antgrid-bridge typecheck→ exit 0bun run --filter antgrid-bridge test→ 3002 pass, 6 skip, 0 fail across 244 files (baseline before this branch: 2984)Every new assertion was regression-checked by reverting the fix under it and confirming the exact failure line. Two are called out as not self-verifying on a dev machine:
config-controller.test.tscannot fail on Windows — libuv implements a file-path watch there as a parent-directory subscription, so inode deafness is structurally unreproducible. Its real verification is the first CI run on ubuntu-latest. Its comment says so, so it is not later deleted as redundant.sessions.json.tmp, which is now a filename nothing writes — it passed even with the rename deleted outright. It matches by prefix instead.Known trade, worth a second opinion
atomicWriteFile's Windows retry ladder can block the event loop up to ~93ms, anddeletePersistedruns on the WS control-plane loop where the original code chose async specifically to avoid that. The debounced flush already did an unretried sync write to the same file several times a second, and the alternative on Windows is a silently lost write rather than a delayed one — but it is a trade, not a free win.Deliberately not included
cursor-agent/hooks.ts— a retrying loser overwrites the merged document it should re-read, then returnstrue, so the session whose hooks were just dropped suppresses its OSC fallback. The fix is re-read-and-re-merge on contention, which is a design call rather than a cleanup.flushSeenProjects(host-server.ts) still writes the project catalog with a plain truncatingwriteFileSync— the catalog that is the only per-project bound on whichprojectIda phone may name.writeConfigYaml(bootstrap.ts), the interactive CLI first-run path, is a plain truncating write ofantgrid.yaml.