Hold a Bot's live screen per socket, so stopping a computer stops it - #288
Merged
davidmckayv merged 9 commits intoAug 28, 2026
Merged
Conversation
beardthelion
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 28, 2026 13:40
A Bot has one live screen and a second connection replaces the first, but the replaced socket is not closed by that: it stays open and closes whenever its client gets round to it, which on an ordinary reconnect is after the replacement is already casting. So anything that stops a viewer has to establish that it owns the one it is stopping, and today only the close handler asks. The input handler, the two computer handlers and the follow loop's own attach all reach for whatever the session holds. This adds the slot that makes ownership something a caller cannot skip. A claim is taken for the socket before the browser is asked for a page, which is what lets a close landing during a cold launch be honoured at all: there is something to release before there is a cast, and the cast the launch produces afterwards is refused. A refused install stops the cast it was given and a refused follow loop is cancelled, because a caller that forgets leaks a screencast or a 1Hz timer against a browser nobody is watching. Three answers rather than two when asked what a socket may do, because a claim with no cast yet is a screen still opening and not a screen that ended, and telling somebody the wrong one of those is the whole reason the notification exists. Occupancy counts a teardown that is still running, so the session sweep cannot drop a Bot mid-handover. The predicate stays for now; index.ts moves onto the slot next, and doing that in one change rather than two keeps every commit building. No Playwright: Screencast arrives as a type, which is erased, so this and its tests run without Chrome. Each rule is covered both ways, and the suite was checked against seventeen deliberate breakages of the module to confirm none of it passes vacuously.
The handlers reached for whatever the session held. Only the close handler asked whether the socket in front of it owned the screen, and the input handler, the sweep and the follow loop's own attach did not, which is three call sites of a question that was only ever answered at one. They go through the slot now, so the question is not one a caller can skip. The claim is taken before the browser is asked for a page: a close landing during a cold launch releases it, and the cast that launch produces afterwards is refused and stopped rather than installed for a socket that has gone. That was leaving a screencast and a 1Hz interval behind, and the interval called currentPage every second, which is a launch path, so a computer somebody had stopped came back up on its own. Input is answered rather than dropped. A superseded socket's typing used to dispatch through the session's cast and land in the page the current viewer was watching, and its sender heard nothing because the old check returned before reaching anything that could report. A socket that is still starting and a socket that is gone are told different things, because telling somebody whose screen is opening that it ended is a worse answer than none. The wheel is untouched and still refuses: it stands after the ownership question rather than instead of it, since the two refuse different things and owning the screen was never permission to drive it. The real-process tests are asked for by name, bun run test:live-screen, the way tests/smoke does. index.ts imports Playwright at module scope and CI installs the root workspaces and the two Bots but not this directory, so an ungated file would throw on import there and redden the build through the test-count floor. Reading the flag before the dynamic import keeps the default suite honest where Playwright was never installed, which was confirmed by running it with the package removed. Both failures were watched failing against the old handlers first: the stopped computer restarting, and the key from a replaced window arriving in the live one. The wheel case and the superseded close cannot fail that way, since both are existing behaviour, so each was confirmed to bite by breaking the check it guards and watching it go red.
Stopping a computer released the wheel and left the viewer running, so the follow loop asked for a page a second later, which starts a browser, and the computer somebody had just stopped was up again. Reset had the same hole. Both are fixed by not putting the teardown in either handler. A browser closes from four places, and only two of them are requests: the cap closes one after somebody else's launch and the idle sweep closes one on a timer, neither with any request involved. A viewer surviving those kept the same loop asking for a page, so a Bot with somebody watching was immune to the idle timeout and came straight back after a cap eviction, which is the same failure arriving by a route no handler is on. So the close announces itself and the screen comes down with it. stop now goes through the same eviction path as the cap and the sweep, because a request is one more reason a browser closes rather than a different kind of closing. profiles still knows nothing about viewers: it says a browser went away and the module that owns the screen decides what that means. The announcement is awaited before the context closes, so a follow tick cannot fit a relaunch into the gap. The two request paths are covered against the real process. The cap and the sweep are reachable from no endpoint, and deliberately so, so they are driven through createProfiles rather than by adding a way in that only a test would use. Each observation was made both ways: the stopped computer restarting itself was watched failing first, and removing the announcement afterwards was confirmed to redden all four paths, while making stop close a browser its own way reddened the two request cases and left the other two green, which is what says the request path really does share the close rather than repeating it.
It had one caller and the slot took that over, so it is dead now. Kept through the previous change only so index.ts moved across in one commit rather than leaving the tree unbuildable between two. Also says why one browser close stays silent. profiles announces a close so the screen watching it comes down, but the half-dead path closes a context whose browser has already gone and launches a replacement on the next line, and the follow loop re-attaches to it within the second. Telling the viewer there would end a screen that is about to be fine, which is the opposite of what the announcement is for. That is the only close of the four that does not fire it, and a reader checking coverage should not have to work out whether it was forgotten.
open's catch was the one path in this change with no coverage against the real process. A file where the profile directory would go makes Chromium refuse to start, so currentPage throws with the claim already held, which is the arrangement the catch exists for and the one the cold-launch timing makes reachable. What it pins is that the socket is told and closed rather than left waiting on a browser that will never arrive, and that the Bot is still usable afterwards. The claim being released with it stays covered by viewer.test.ts, because a leaked claim changes nothing observable from outside this process until the session map has grown, and reaching that state through the port would prove less than the unit test already does.
Review found the request path falling through the mechanism the rest of this branch added. Everything hangs off closing a browser that is running, and a launch is not one of those until it finishes, so a stop or reset arriving inside a cold start answered "nothing was running" and left the browser up a moment later with the live screen still on it, its follow loop keeping it marked recently used so the idle sweep would not reclaim it either. Reset was worse: it deleted the profile directory that the finishing launch recreated, leaving the Bot signed into what it was told to forget. A request now waits for the launch it is racing. Deliberately not inside the eviction itself, because the cap evicts from inside a launch and would then be able to wait on the launch it is running under. The waits that were unbounded are bounded now. Telling a viewer its browser went away sits on another Bot's launch path, and stopping a cast decides whether a session can ever be swept, so a screencast that never answers used to pin a launch, keep a session occupied for the life of the process, and on the way out hold every profile's flush until the container was killed. Smaller things the same pass turned up: a refused cast was stopped outside the teardown accounting, so occupancy could report nothing casting while Chrome was still encoding; the close handler created a session for a Bot that had none, which the comment three lines above it argues against; the announcement callback was optional at the one call site the fix depends on; and a stale doc block described a body that had moved. Four tests were weaker than they read. The cold-close case never established that a browser had started, so it would have passed on a machine where the launch failed. "It did not come back" was one check after a fixed sleep, which cannot tell a relaunch that never happened from one still in flight, so it is held across a whole cold start now. The superseded message was asserted as any non-empty string. And the branch that tells somebody their screen is still opening, the whole reason there are three standings rather than two, had no test at all. The port was fixed at Tailscale's default, so on a host running it this file could never have started; it asks the operating system for a free one now. The announcement tests leaked their environment knobs and passed on the alphabetical order of the files, since Bun shares one module registry across a run and ignores the order they are named in. docs/development.md documents the lane beside test:smoke, including that it needs this directory's own install.
The checklist in docs/development.md asks for this when a deployment behaves differently afterwards, and it does: a computer that was stopped stays stopped, a watched Bot stops being immune to the idle timeout and the cap, and a viewer whose screen ends is told why. It also says plainly that the app does not show that message yet, so nobody reads this and expects to see it on screen.
beardthelion
force-pushed
the
fix/live-screen-per-socket
branch
from
August 28, 2026 13:49
f7a4df4 to
0e73d8b
Compare
# Conflicts: # CHANGELOG.md # agent-computer/src/index.ts # agent-computer/src/profiles.ts
9 tasks
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.
Closes #190.
What this changes
agent-computertracked the live screen per Bot rather than per socket.BotSession.viewerheld onerecord, and
open,close, the input handler and the 1Hz follow loop all reached for it by session,so only the close handler ever asked whether the socket in front of it owned the thing it was about
to stop. Four failures came out of that one decision and they share a fix rather than four fixes.
A computer somebody stopped came back up on its own.
/computers/stopreleased the wheel andleft the viewer running. The follow loop asks for the Bot's current page every second, asking for a
page is what starts a browser, so the browser was up again a second later.
/computers/resethad thesame hole. The same loop also kept the browser marked recently used, so a watched Bot was immune to
the idle timeout and came straight back after being closed to stay under the cap. Neither of those
last two involves a request at all, which is why the fix cannot live in the stop handler.
A close during a cold launch stranded the cast.
openawaits a page, which launches Chromiumwhen nothing is running, and a socket closing inside that window found no viewer installed and so did
nothing. The launch then installed a cast and an interval for a socket already gone, and no second
close ever arrived.
A superseded socket's typing went into the current viewer's page. The input handler dispatched
through whatever the session held without asking who sent the message, and the sender was told
nothing, because the missing-viewer check returned before reaching anything that could report.
Overlapping opens could orphan a follow interval. Read from the code, not observed: nothing here
can drive two overlapping opens. It is removed by the same change and no test claims to prove it.
viewer.tsgrows from a single predicate into the module that owns viewer identity and teardown. Aclaim is taken for the socket before the browser is asked for a page, which is what makes the
cold-launch case fixable at all: there is something to release before there is a cast, and everything
the launch produces afterwards goes through that claim and is refused once revoked. A refused
installstops the cast it was handed and a refused follow loop is cancelled, because a caller thatforgets leaks a screencast or a timer against a browser nobody is watching.
Teardown hangs off the browser closing rather than off the two request handlers, so the cap and the
idle sweep are covered by construction. A request that lands while the browser is still starting now
waits for the launch it is racing; without that a stop answered "nothing was running" and the browser
appeared a moment later, and reset deleted the profile directory that the finishing launch recreated.
A socket gets three answers rather than two: casting, still starting, or gone. Collapsing the middle
one tells somebody whose screen is seconds from live that their session ended.
Where it runs
sessionsmapin the
agent-computerprocess. It is in-process on purpose and cannot be anywhere else. Thething it tracks is a CDP screencast attached to a Chromium instance that exists only in that
process, so a claim in Postgres would describe a cast no other process could stop. It sits
beside the state already kept there for the same reason, the control handover and the snapshot
generation, which
index.tskeeps in-process because a takeover the browser does not knowabout is not a takeover. This PR moves nothing from shared storage into memory; it replaces an
in-memory record with an in-memory object that owns its own teardown.
agent-computerfor the same Bot is not adeployment this supports, before or after this change. With
COMPUTER_SUPERVISOR_URLeach Botgets its own container; without it every Bot shares one. Either way exactly one process holds a
given Bot's browser, and
/streamis a socket to that process. Two of them for one Bot wouldalready have split the wheel and the snapshot counter. Nothing here is newly replica-sensitive,
and the server-side gateway, policy and audit are untouched.
await, so within the single-threaded process no interleaving can put two owners in the slot:a second connection revokes the first as one uninterrupted step, and every later use of a
revoked claim is refused rather than checked-then-written. There is no cross-process race to
serialise, because there is no second process holding this browser.
messages a viewer receives are sent on the connection terminating here, on the process that
owns the browser they are about. Nothing needs to reach a socket held elsewhere.
makes it stoppable rather than adding it, and it is now cancelled on every path that ends a
viewer instead of only on a clean close. The new test allocates an OS-assigned port, in tests
only.
Boundary and audit
here adds an acting call or moves one. This is below the gateway, in the process that holds the
browser, and the server-side path is untouched.
this process has no policy engine or audit trail of its own by design. The refusals added here
are messages to the socket that asked. What does change in the log is that
computer-browser-closednow also fires for stop and reset, withit was stoppedandit was resetas reasons, because those go through the same eviction path as the cap and the sweep.ownership is decided by socket identity held in this process and compared by reference, never
by anything the client sends, and the input handler now refuses a socket that does not own the
cast instead of dispatching through whatever the session happened to hold.
Changelog
CHANGELOG.mdunderUnreleased. It says what an operator sees change: a stoppedcomputer stays stopped, a watched Bot stops being immune to the idle timeout and the cap, and a
viewer whose screen ends is told why. It also says plainly that the app does not display that
message yet.
Proof
Test-first throughout, and the three reproducible failures were each watched failing against the old
handlers before the fix and passing after: the stopped computer restarting itself, the key from a
replaced window arriving in the live one, and the browser closed by the idle sweep coming back.
Every changed rule was then checked by breaking it on purpose and confirming something goes red. That
pass found four of my own tests asserting less than they read, and all four are fixed: one never
established that a browser had started, so it would have passed where the launch failed; "it did not
come back" was a single check after a fixed sleep, which cannot tell a relaunch that never happened
from one still in flight, and is now held across a whole cold start; the superseded message was
asserted as any non-empty string; and the still-starting branch had no test at all.
bun test agent-computer/with theplaywrightpackage physically removed, which is what CI sees:201 pass, 12 skip, 0 fail.
The real-process tests are asked for by name,
bun run test:live-screen, the waytest:smokeis,and documented beside it in
docs/development.md.index.tsimports Playwright at module scope,playwrightis declared only inagent-computer/package.json, and CI installs the root workspacesplus the two Bots and never this directory, so an ungated file would throw on import there and redden
the build through the test-count floor. The cost is that CI does not run these. Happy to add a job
that installs this directory and Chromium if you would rather have it, but that seemed like your call
rather than something to slip into this PR.
No screenshots: nothing here has a surface. The one visible half is the message a person would read,
and it is not visible yet, which is the next point.
Two things worth knowing
This does not fix what a person sees. The server tells the socket its screen ended; the app stores
that message and renders it only in the branch it is not in while the live screen is mounted, so the
canvas keeps its last frame. Filed separately as #287. Everything here is server-side and correct on
its own, but I did not want the gap to read as closed.
It collides with two open PRs, mechanically rather than in substance. #265 renames
sessionForand moves the session registry, on the same handlers this rewrites. #286 adds a field to
BotSessionand edits
createProfilesat the points this changes. Whoever merges second gets a small manualresolution; happy to rebase onto whichever lands first.