release - #52
Merged
Merged
Conversation
Isolated sessions were becoming undeletable, permanently. A coding agent's helpers - sandbox command runners, analysis servers - outlive the PTY that started them and become ORPHANS holding a checkout subdirectory as their cwd. Windows refuses to delete a directory that is any live process's current directory, so 'git worktree remove' dies of a sharing violation. The host log shows it landing on five checkouts across three roots, and the failure LATCHES: git deletes .git early in its sweep, the next prune reaps the registration, and the retry then skips 'git worktree remove' entirely. killProcessTree cannot reach those processes by construction - 'taskkill /F /T' walks the LIVE parent-child table from a pid, and an orphan's parent is already gone. Nor is this a rare path: 36 of 45 completed bridge lifetimes in that log were force-killed, so killAllGracefully never ran at all. So every PTY is now assigned to its own JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE job (bridge/src/win32-process.ts), the same primitive the app already applies to the host above it. Membership is inherited at CreateProcess and survives the parent's death, which is exactly why a job reaches what a parent-link walk cannot. Three invariants the comments spell out: the pid is assigned the instant it exists, the handle is closed on NATURAL exit and not only on kill, and closing the handle IS the reap - so the kernel closing the bridge's handles as it dies sweeps every PTY tree with no shutdown handler involved, which covers the force-killed majority. Where a delete still fails, it now says why. The holder processes are enumerated and named in the thrown message, capped and spelled RELATIVE to the checkout so no host path crosses the session wire; full paths go to the local log, only the count joins the analytics events. That case throws WORKTREE_DELETE_HELD rather than WORKTREE_DELETE_FAILED, because the app has a copy arm for the latter that REPLACES the bridge's message and would eat the clause; a code with no arm falls through to error.message verbatim. The reconcile sweep does the same reclaim and gets its own worktree_reclaim_failed event - a survivor moves none of ReconcileCounts, so without it the sweep reported nothing to do while failing on the same stranded directories at every create. It keeps a single rm rather than the retry budget: it runs inside session:create, which the app abandons at 15s. Coverage is not total, measured: a child created through ShellExecute is spawned by another process and joins THAT process's job. POSIX needs none of this - the process-group kill stays the mechanism there, and an orphan blocks no delete because POSIX unlinks regardless.
Every guard against re-naming a session lived in memory: SessionNamer's rank ordering and the attempt budget are both dropped when the PTY exits, and the persisted row recorded only manuallyRenamed, which a generated name deliberately leaves false. So a stop/start reached the naming path with nothing standing in its way and renamed the session twice — once from the native first-message read, then again from a fresh model spawn. Not back to the same title either: buildTitleContext returns the LAST few messages, so the new name described wherever the conversation had drifted to. Keying that on the agent's session id does not work, which is the part worth writing down. Claude Code's --resume copies the transcript into a NEW file and appends under a fresh id (measured: three ids in one lineage), so the same thread comes back wearing a different name and every restart would look like a new conversation. The winning signal is therefore persisted beside the name as autoTitleRank, and a launch records at the time it happens whether it continues the previous conversation — the first identity report spends that claim, and any rotation after it is a real /clear. applyAutoName enforces the same precedence SessionNamer does, so a first-message or OSC title can no longer displace a generated one after a reload. Legacy rows are not backfilled: a file written before the field cannot say which signal named it, and guessing would freeze those sessions against ever being named properly.
* Every machine-level store publishes by rename, never by truncation 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. * 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, 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. * Watch the directory: a renamed-over config file goes deaf on Linux 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. * Serialize the two sessions.json writers on one publish primitive 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. * A watcher over a published file must accept the scratch name 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.
The TypeScript job failed on development three times in the last week, always on the same two tests in agent-core-checkout-routing.test.ts. Neither is slow: measured, the body of "a managed checkout's services wait for worktree.setup to finish" runs in ~400ms and its teardown in ~101ms. Bun caps a hook at 5000ms, and `test(..., 20000)` does not raise it - a hook budget is separate from the test's. It also reports that failure as 'timed out after 20000ms', naming the test's timeout rather than the one it enforced, which is why the log reads like a body overrun. Worse, it does not CANCEL an overrunning hook, it just stops waiting: the body resumes inside the next test, where the module-level root/core/previousAbDir have already been reassigned. The late rmSync then deletes the running test's checkout - which is the ENOENT on setup-stub.sh that CI reported as the second failure, and why the file-watcher error and the ENOENT name the same temp dir 1ms apart. So the hook now binds everything it touches BEFORE the await and deletes in a finally. Injecting a 6s shutdown into the real file went from 0 pass / 11 fail - including a checkoutId from one test leaking into another's assertion - to 11 pass. Where a teardown still overruns, it now fails only the test it belongs to. The budget goes to 30s to match what shutdown() legitimately composes, and the one test that awaits a full shutdown() in its BODY gets the same, since the 5s default is killAllGracefully's own budget with nothing left for the drain behind it. That drain was also unbounded. teardownCheckoutRuntime's copy must stay that way - it gates a `git worktree remove` that an abandoned `git status` aborts on Windows - but shutdown removes nothing, so waiting there only buys us not orphaning a child. Against a status stalled 30s, shutdown took 28810ms; bounded, 3004ms, and it says which. owner-watchdog and the pre-update drain both give the host seconds. The third flake is unrelated and was reproduced locally: the plugin test's literal ports sit inside the Linux ephemeral range (32768-60999), so another socket in the suite is handed one first and Bun.serve dies EADDRINUSE. Port 0, read back.
* An agent is asked to leave before it is killed, on every platform Every teardown path terminated a coding agent's PTY hard. On POSIX the per-session paths (stop, archive, delete, setMode) went straight to SIGKILL on the process group; on Windows nothing was ever graceful, because killAllGracefully skips its graceful phase there and is only reached on host shutdown anyway. PR #43 measured the cost of the latter: 36 of 45 completed bridge lifetimes in the host log were force-killed, so killAllGracefully never ran at all. An agent killed that way runs no exit path - unflushed state, unwritten transcripts, and orphaned helpers. Claude Code is the legible case: it arms a boot canary before its first render and withdraws it in a process exit hook, so a hard kill leaves a stale entry and the next launch drops to the classic renderer with a user-visible message. The comment justifying the missing Windows phase gave two reasons. The second - that asking nicely COSTS the sweep, because killProcessTree walks live parent links and a leader that exits re-parents its children out of reach - stopped being true when #43 enclosed every Windows PTY in a kill-on-close job whose membership survives the parent's death. Only the first still stands: Bun maps every signal to TerminateProcess, so there is no soft signal to send. So TerminalSession.close(graceMs) is the ladder: ask, wait a bounded time, then sweep UNCONDITIONALLY - never branching on whether the agent left, which is what makes a Windows default safe. close() is deliberately not async, because stopAndAwait reads treeKilled before it awaits the exit and the assignment has to land before the first suspension or the tree wait goes vacuous. Only agent PTYs are asked: a service PTY runs a build tool in cooked mode, which cannot see the keystroke on Windows and has nothing to flush on POSIX, so it keeps today's path exactly. AgentSpec.gracefulExit refines the ask rather than gating it - escalation is unconditional, so a wrong guess costs latency, never correctness. POSIX now signals the GROUP; the old bare-leader ask was narrower than its own SIGKILL escalation, so an agent under sh -c never saw it. The reach a grace costs is bought back with a snapshot taken before the ask, while the tree is whole, and swept afterwards through an identity check on pid, parent pid and image name. A process table that cannot be read is unknowable rather than empty: the waiter keeps waiting and the sweeper kills nothing, because a snapshot pid Windows has since reissued names a stranger's whole tree. Every refusal and every ignored ask is logged - a silently skipped grace looks exactly like an agent that ignores one, and those are different bugs. The codex chat backend is not a PTY and got the same treatment: stdin EOF as the ask, a bounded wait, then killChildTree - which needed processGroupSpawn to have any POSIX reach at all, since kill on a negative pid names no group unless the child leads one. Coverage is not symmetric and should not be read as such. The Windows arm, including the grandchild case that justifies retiring the second reason above, is measured on Windows 11. The POSIX arm is exercised in a Linux container, not on macOS: the whole bridge suite runs there green, the group-ask cases included. * A same-id respawn pays the exit it will never receive A restart inside the grace leaves the replaced run's exit-driven cleanup unrun. The exit lands on a slot the replacement already owns, where TerminalManager's same-id gate drops it - and with it namer.forget, forgetTitleAttempts, the handler's per-terminal guard and noteExited. The restarted session then inherits the dead run's buffered title, its autoTitleRank and its arming, which is the exact leak agent-core's onTerminalExited comment was written to prevent. Nothing about the gate changed; the grace only made the window reachable. It used to be the couple of hundred milliseconds between kill and the exit landing, which no human hit. It is now up to AGENT_GRACE_MS, and the row reports not-running from the moment the ask goes out - so Stop, then Start, lands inside it as a matter of course. Dispatching the callback from the exit handler anyway would be worse than the bug: that cleanup is keyed by terminal id, so by the time the dead run's exit arrives it would reclaim the LIVE run's state. It runs at the replacement instead, in spawn()'s duplicate branch, while the id still means the old run. Only the callback fires there - never the terminal:exited frame, which is the half the gate exists to protect, since a frame for a live slot tells the app a running terminal is dead and nothing later corrects it. The manager test asserts with nothing awaited between the kill and the respawn, because 'eventually' is precisely the failure mode. The session-manager test pins the reachability rather than the fix - it passes either way - and covers the other end too: the replaced run's tree settling behind the restart must clear nothing, or a late settle puts the row back to stopped with a live agent under it.
…47) Returning to a session left parts of the terminal blank or showing stale content. Two independent causes, both measured with probes rather than inferred. The bridge drops terminal:output while connState.suppressed - a socket drop, a backgrounded app, or a remote-access flip - and keeps bumping the seq regardless. The only recovery, terminal:snapshot:request, fired solely for tabs the app had never seen, and TerminalService's tier-3 hydrator DISCARDED its seq cutoffs instead of refreshing them. Measured after a reconnect: four hydrator pulls, none of them a terminal one. The replay could not have worked either. _applySnapshot erased the grid and fed back a 10,000-char ScrollbackBuffer tail - a suffix of a DIFF stream, since Ink-style TUIs paint their chrome once and rewrite only the rows that changed. Measured: 4 of 50 rows addressable. Enlarging the buffer cannot fix a stream that was never a screen. So the bridge keeps a headless @xterm/headless VT per PTY (terminal-screen.ts), fed BEFORE the suppression drop, and serializes the visible grid on attach - 9.4 KB for a 200x50 screen, smaller than the broken tail and structurally complete. It composes the whole attach sequence and advertises it with a new optional 'composed' flag; the app applies such a blob verbatim. The preamble resets DECSTBM and defaults the modes @xterm/addon-serialize emits set-only, and deliberately never issues 3J: the app's engine holds far more history than the bridge does, and an erase reaching past the screen would destroy the user's own with nothing able to put it back. getAttachSnapshot registers a tail sink across the serialize barrier, re-checks screen identity after it, and reads the seq afterwards. The barrier is a real suspension point - a terminal can exit and a same-id respawn take the slot across it - and a dead screen stamped with the live PTY's seq arms a cutoff above everything that PTY will ever emit: a blank pane in front of a running process. The hydrator now re-pulls a snapshot for every live tab, on transport re-establishment and on a new narrow focusResumed edge. The backgrounded-app trigger reconnects nothing, so it had no other recovery. Cell metrics are measured synchronously up front (terminal_cell_metrics.dart), so a non-driver remount costs zero engine resizes instead of two. ghostty_vte_flutter does not reflow, so every resize leaked stale fragments out of an Ink-style TUI. Known gap, flagged rather than taken: FileService and PreviewService have the same suppression defect with no recovery. The honest fix is redriveHydrators() on AgentTransport, which lives in the Apache-2.0 packages/ tree - the licence boundary is one-way.
* Do not reserve space for session row kebab menu when unhovered Anchors the session row content height to 24px via the leading slot without shifting the 12px optical status dot centering, allowing trailing to be omitted when unhovered without vertical height jitter. Session titles now have ~20% more horizontal space before truncating in the projects drawer panel. * Remove unused import in session_row_hover_test.dart * Keep kebab menu mounted while session actions popup is open When pointer moves into the popup menu, the row unhovering would previously unmount the kebab and invalidate anchor.mounted, causing selected menu actions to abort without executing. Tracks _menuOpen and keeps trailing mounted until the menu and any spawned dialog dismisses.
Browser inspect element and draw to add feedback
The previous change flagged FileService and PreviewService as carrying the terminal suppression defect with no recovery, and named redriveHydrators() on AgentTransport as the fix it could not take: that class is Apache-2.0 and the licence boundary is one-way. The app-side edge already exists, so neither service needs it. A hydrator covers transport re-ESTABLISHMENT; a focus resume re-establishes nothing, and it is the other window the agent suppresses in. While the app is backgrounded the agent DROPS every tree:update and keeps bumping its file seq, so what resumes is a delta stream whose base is missing every add and remove from that window - a file the agent created stays invisible, a directory it deleted stays listed, for the life of the connection. Nothing self-corrects it: _snapshotSeq only ever advances on a full snapshot, which only a pull asks for. Preview is the same shape - a preview:url dropped there is remembered as undelivered agent-side, but the only thing that drains that flag is the port re-emit behind the snapshot request. Both now subscribe to focusResumed and pull their own state. A non-driver terminal viewer sized its grid to its own viewport, while the PTY's geometry belongs to whichever client drove it last - so a passenger rendered a grid the agent was never writing to, wrong on both axes. The grid is now laid out at the authoritative extent (gridExtentFor, carrying the epsilon the view's own floor() division needs) and letterboxed to fit. One consequence worth naming rather than discovering: zoom on a passenger view is a no-op past the fit boundary, because the scale cancels it. A cold attach now asks for scrollback, and the bridge answers with a blob that leads with 3J. That erase cannot simply be broadcast: a reply is published on the project bus and fans out to every client on the terminal, so one device's first attach would take another device's scrollback with it - precisely the loss the warm preamble refuses to risk. The reply therefore carries the history flag too, and a client whose engine is already painted drops the frame. The requesting client records a claim that the next snapshot to APPLY spends, so live output landing mid-round-trip cannot make it refuse the answer it asked for, and an older agent that strips the request key still retires the claim. Attach composition hardened alongside it: SGR reset now precedes the erase in both preambles, since ED fills with the latched background and a coloured cell would otherwise paint the whole screen; the screen re-settles across rounds and reports its own unparsed tail, which only xterm can know, instead of a bookkeeping map beside it; and resyncState contains a failure to the terminal that caused it. The git-status-freshness fixture was flaky by construction. It broke git status itself, which is the very read the test asserts on - the refresh behind the failed discard came back empty-handed and the bus deduped the byte-identical frame away, leaving a 250ms debounce racing a git config spawn as the only thing that could ever pass. It now breaks the discard's restore instead, by dropping the blob the file would be restored from, and asserts on content rather than on a count.
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
2 participants
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.
No description provided.