boxd in the app: follow-ups from #356 (PR #372) #371

Activity

1. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code, from the Step 11 adversarial review of feat/boxd-in-app (#356). These are four more P3s for this follow-up. The one P2 found in that review (a machine get reply without status read as awake) was fixed on the branch in 7555c9d.

   5. Let-go commit and disconnect race

   RemoteWorkrooms.swift, RemoteHosts.observed: the last locked check runs, and then the code awaits an actor hop before HostConnectionManager.shared.disconnect(lease). A click (wake: true) or a pane attach that lands in that gap sees the host still up and clears letGoAt. The disconnect then closes the connection anyway. A Keep awake click fails with unavailable, or a pane loses its badge and prompts until the next read reconnects it. It heals itself, and it is rare.
   Fix: after the disconnect, check letGoAt and the selection again and reconnect if the host was taken back. Or have the manager disconnect only if nothing has claimed the host since.

   6. A deleted host's WakefulnessModel can come back, and its poll leaks

   RemoteWorkrooms.swift, in connect: WakefulnessModel.model(forHost: id).pollWhileConnected() creates a model when none exists. If a delete calls forgetHost while a connect is still finishing, a fresh model starts a 10 s poll that nothing cancels. Also, RemoteHosts never prunes letGoAt, asleepAt, connectedAt, busyReports, idleWindows or keys when a host is deleted.
   Fix: use a lookup that does not create the model in connect, and clear the per-host state on delete.

   7. A cancelled create may not record a paid machine it left behind (investigate)

   RemoteWorkrooms.create: the left-behind record is written through the CLI inside the caller's task. If that task is cancelled, the write can fail, and the only trace is the "could not record … left live" log line. Check whether a create can be cancelled in practice. If it can, write the record in a Task {} that does not inherit cancellation, as BoxdHostDriver.undo already does.

   8. An old agent's box is held for a ceiling after every connect (investigate)

   AgentWakefulness.swift: any status failure that is not routing-related counts as having reached the agent. That includes VCSError.backendVersion from an agent with no wakefulness service. So the untrusted hold starts and the box is reported busy for up to one ceiling. Decide whether an old agent should be held, or let go at once.

   Related, not taken here: presence treats only standby and hibernated as asleep. A transitional status such as suspending would read as awake. BoxdHostDriverTests.testAsleepIsBoxdsStandbyOrHibernated pins stopped as awake on purpose. Before narrowing "awake" to running only, check boxd's real status list and what an ssh login does to a stopped machine.
2. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code. A correction to item 3.

   Item 3 overstated the gap. RemoteWorkroomRoutingTests.testSelectingABoxdWorkroomKeepsItsBoxFromBeingLetGoOf and testAnotherWindowsSelectionKeepsThisWindowsBoxAndClosingItLetsGo already drive AppStore.selectedTargetID and deinit. The reviewer that raised it missed them, and I didn't check before filing.

   What remains untested is the restored-selection path only. That is the select in AppStore's reload, for a selection restored before its projects loaded (around AppStore.swift:3123). A test there would drive a reload of an AppStore whose selectedTargetID was set before projects, then assert the box isn't let go.
3. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code, from the #356 /ship review (run 6). Three more for this follow-up, all low confidence.

   9. Concurrent presence reads aren't shared

   RemoteHosts.boxdPresence has no in-flight sharing. Several background readers of the same unconnected host can be waiting at once: the status sweep, the file tree, history, the port-forward watch. Each one starts its own boxd machine get before the first answer lands. This goes with item 1, the unanswered read that isn't cached.
   Fix: keep one in-flight Task per host and have every reader wait on it, as connecting does.

   10. Product question: a sleeping or let-go box hides its last-known dirty state

   For .asleep and .idle, WorkroomStatus.aggregateWeight is 0 by design. RemoteWorkroomRoutingTests pins "an asleep box must not mark its project". mergeLocalStatus also replaces dirty with nil. So a workroom left with uncommitted work on a box the app let go shows nothing at the project level. The row's moon is its only sign.
   Decide: should the row keep its last-known dirty state, shown as stale, while the box sleeps?

   11. The format of boxd's idle timers is measured only once

   BoxdHostDriver.Seconds.parse reads a bare number or a single unit (120, 120s, 2m, 1h). The one real reading recorded is in docs/designs/remote-workrooms.md: "auto_suspend": "120s". A compound value such as 4h0m0s would read as no timer. The short-window warning would then never fire, and revisitAfter would fall back to 3600 s.
   Fix: record one real machine get --json with both timers set, including the default auto-hibernate, and pin it in a test. Log a timer that is present but fails to parse.
4. joelmoss

   added

   bugSomething isn't workingSomething isn't working

   remoteRemote workrooms: hosts, providers, the far-side agent and brokerRemote workrooms: hosts, providers, the far-side agent and broker

   needs-triage

   on Oct 7, 2026
5. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code, from the last #356 /ship review pass. These are two comment fixes, deferred so the review could converge.

   12. Stale doc comments

   • macapp/WorkroomApp/Core/AgentWakefulness.swift, the WakefulnessModel class doc, reads "polled while it is on screen". A connected boxd host is now also polled when nothing on screen shows it (pollWhileConnected). The comments on pollInterval and poll() were updated in 31031eb, but the class doc wasn't.
   • macapp/WorkroomApp/Core/RemoteWorkrooms.swift, the ensureConnected doc, reads "wake for a click (closing a pane, Keep awake)". A third caller is missing: PersistentSessionService.attachCommand calls ensureConnected(host, wake: true) when a pane attaches to a host the app let go of, which isn't a click.

   Low priority, also from that pass: AgentWakefulness.minimumIdleWindow = 90 follows from the Rust heartbeat's INTERVAL_S = 60.0 (vcs/crates/wr-agent/src/wakefulness/heartbeat.rs), but only prose links the two. Naming the constant in the Swift comment would let a change to one find the other.
6. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code, from the final adversarial review of #356 (/ship). That review found no P0 or P1, and recommended merging.

   13. A let-go disconnect can cut a request already in flight (P2, investigate)

   RemoteHosts.observed checks selection, port forwards and attached panes, then calls HostConnectionManager.shared.disconnect(lease). disconnect fails every pending operation on that connection with connectionLost. The only guard is letGoGrace, 10 s after connect. A box that isn't selected reads IDLE between commands, so a long git read on it could be cut off at the next 10 s poll.
   Check first: does the app issue long or mutating calls against workrooms that aren't selected? If so, have observed refuse to let go while the lease has pending operations.

   14. A boxd record with no account isn't checked at delete (P3)

   HostDescriptor.init(from:) decodes account with try?, so a missing or corrupt value gives DriverKey.boxd(org:, account: nil). checkOrg then skips the account check. With another account signed in, a delete would read "not found" and treat the box as gone, leaving a paid box running. New code always writes account, so only hand-edited or corrupt config hits this.
   Fix: have deletionKey throw invalidConfiguration for a boxd descriptor without account.

   15. Create-progress steps can apply out of order (P3, cosmetic)

   In AppStore.swift, the stepped closure starts an unstructured Task { @MainActor } for each step, so the progress ring can briefly go backwards. A late step from a finished create can also land on the next create of the same project.
   Fix: carry a per-create token, or a step index, and drop any step older than the last one applied.

   (The reviewer's timer-format point is item 11.)
7. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code, from Codex's review of PR #372.

   16. A boxd create needs gh only to name the repository

   AppStore.createRemoteWorkroom names the GitHub repository with WorkroomStatusResolver().resolveRepository (gh repo view) before it provisions anything. It throws "Couldn't find its GitHub repository with gh" when gh isn't signed in. Every remote workroom does this, Docker included, so it isn't new. For boxd, though, clone credentials come from Codaset alone, so a user signed in to Codaset and boxd but not gh sees boxd enabled in the menu and then fails at create.
   Fix: for boxd, parse owner/name from the local origin (github.com only) and skip gh. Otherwise, gate and explain the gh prerequisite in the menu.

   Also from that review: item 6 (the model re-created after a delete) is fixed in PR #372 for the idle-window read. Pruning the per-host dictionaries is still open.
8. joelmoss commented on Oct 7, 2026

   

   joelmoss

   on Oct 7, 2026

   OwnerAuthor

   More actions

   🤖 Left by Claude Code, from Codex's review of PR #372.

   17. A stale IDLE report can let a busy box go (low likelihood)

   WakefulnessModel.report(busy:) (AgentWakefulness.swift) starts its own Task for each applied reading, and those tasks have no ordering. If two readings land together with opposite verdicts (IDLE, then BUSY) and the IDLE task starts second, observed snapshots the already-incremented busyReports and can let a busy box go.

   • Likelihood: very low. Readings are normally 10 s apart, and it needs overlapping refreshes right as the verdict flips.
   • Cost: the box stays awake, because its heartbeat runs on the box. But the app drops its connection, so a ceiling prompt isn't shown until the next revisit (the idle window plus 30 s).
   • Fix: pass a status generation with each report, and have observed reject one older than the newest it has seen. Alternatively, send reports through one serial stream per host.
9. joelmoss

   changed the title [-]boxd in the app: four small follow-ups from #356's last review pass[/-] [+]boxd in the app: follow-ups from #356 (PR #372)[/+] on Oct 7, 2026
10. joelmoss

    closed this as completedin #381on Oct 9, 2026
11. joelmoss commented on Oct 9, 2026

    

    joelmoss

    on Oct 9, 2026

    OwnerAuthor

    More actions

    Items 6 (rest), 8, 9, 13, 17 and 18 are fixed in #381 (merged 2026-10-09), and ticked off above.

    #381 removed the logic these items were edge cases of: the app's let-go, its status poll, the awake ceiling and its prompt, and the idle-window warnings. Before merge, the owner also had the agent-side let-go removed, so nothing closes an idle connection now. A box an open app holds awake is the user's to free, by deleting its workroom. Item 18 is resolved by that decision rather than by making the box sleep.

    Still open here: 3, 7, 10, 11, 12, 14 and 15.

    🤖 Left by Claude Code