fix(maestro): three defects the review flagged and left open #50

2026-08-22T16:32:14+01:00

naps62-yolo commented

2026-08-22 16:32:14 +01:00

The four headline bugs from the review series are already merged (#41, #42, #45) along with the reviewer's double-click finding (#49). These are the three defects that were flagged and deliberately left open, now closed.

The SPA destroyed a pane on any dropped frame

There was no reconnect at all — createAttachConnection built one socket, and ws.onclose went straight to onClosed → closePane. The TUI reconnects forever on the same endpoint; the browser gave up on the first blip.

Compounding it, session death reached the UI on two uncoordinated channels: the row from the events socket, the pane from ws.onclose, with nothing reconciling them. Either could happen without the other.

The catalog is now the authority for both. shouldRetryClose decides from session status:

situation	retry?	why
`Running`, code 1006	yes	the link dropped, the session is alive
`Dead`	no	`PtyBridge::seal()` closed the stream; nothing more is coming
absent from catalog	no	it is gone
code ≥ 4000	no	a relayed daemon rejection — retrying loops on it
code 1000	no	deliberate close

Backoff is 500ms doubling to 10s, reset on open, cancelled on dispose. The shell also closes any pane whose session has left the catalog, which is the reconciliation that was missing.

The policy lives in reconnect.ts rather than inside attach.ts, because attach.ts needs a DOM and an xterm instance — this way the decision table above is what the tests actually exercise, and main.ts calls the same function.

`kill_session` had a TOCTOU that produced the wrong status

It checked has() before kill(). kill already reports a miss as SessionError::NotFound → 404, so the pre-check added nothing except a window for the liveness watcher to reap the session between the two awaits — turning a 404 into a 502. Removed.

I left the equivalent pre-check on attach_ws: there it is load-bearing, since the 404 must be decided before the upgrade, and attach::serve already compensates for the race with a clean close.

`with_spawn_history` failed silently

It logged and carried on when Arc::get_mut returned None, producing a daemon whose recent-dirs picker is permanently empty with nothing to show for it. It panics now — a construction-order mistake, at startup, in code that runs exactly once.

Verification

cargo clippy --all-targets -- -D warnings    0
cargo test --all                             322 passed, 0 failed
pnpm -C web test                             43 passed (was 36)
pnpm -C web typecheck                        clean
pnpm -C web build                            ok

The four headline bugs from the review series are already merged (#41, #42, #45) along with the reviewer's double-click finding (#49). These are the three defects that were **flagged and deliberately left open**, now closed. ### The SPA destroyed a pane on any dropped frame There was no reconnect at all — `createAttachConnection` built one socket, and `ws.onclose` went straight to `onClosed` → `closePane`. The TUI reconnects forever on the same endpoint; the browser gave up on the first blip. Compounding it, **session death reached the UI on two uncoordinated channels**: the row from the events socket, the pane from `ws.onclose`, with nothing reconciling them. Either could happen without the other. The catalog is now the authority for both. `shouldRetryClose` decides from session status: | situation | retry? | why | |---|---|---| | `Running`, code 1006 | yes | the link dropped, the session is alive | | `Dead` | no | `PtyBridge::seal()` closed the stream; nothing more is coming | | absent from catalog | no | it is gone | | code ≥ 4000 | no | a relayed daemon rejection — retrying loops on it | | code 1000 | no | deliberate close | Backoff is 500ms doubling to 10s, reset on open, cancelled on dispose. The shell also closes any pane whose session has left the catalog, which is the reconciliation that was missing. The policy lives in `reconnect.ts` rather than inside `attach.ts`, because `attach.ts` needs a DOM and an xterm instance — this way the decision table above is what the tests actually exercise, and `main.ts` calls the same function. ### `kill_session` had a TOCTOU that produced the wrong status It checked `has()` before `kill()`. `kill` already reports a miss as `SessionError::NotFound` → 404, so the pre-check added nothing except a window for the liveness watcher to reap the session between the two awaits — turning a 404 into a 502. Removed. I left the equivalent pre-check on `attach_ws`: there it is load-bearing, since the 404 must be decided *before* the upgrade, and `attach::serve` already compensates for the race with a clean close. ### `with_spawn_history` failed silently It logged and carried on when `Arc::get_mut` returned `None`, producing a daemon whose recent-dirs picker is permanently empty with nothing to show for it. It panics now — a construction-order mistake, at startup, in code that runs exactly once. <details> <summary>Verification</summary> ``` cargo clippy --all-targets -- -D warnings 0 cargo test --all 322 passed, 0 failed pnpm -C web test 43 passed (was 36) pnpm -C web typecheck clean pnpm -C web build ok ``` </details>

naps62-yolo added 1 commit 2026-08-22 16:32:14 +01:00

fix(maestro): three defects the review flagged and left open

ci / web (pull_request) Successful in 28s

Details

ci / rust (pull_request) Successful in 1m58s

Details

ci / web (push) Successful in 34s

Details

ci / rust (push) Successful in 2m44s

Details

f31377c34f

SPA attach had no reconnect at all. Any dropped frame closed the socket,
`onClosed` fired, and the pane was destroyed permanently — the TUI
reconnects forever on the same endpoint. Session death also reached the UI
on two uncoordinated channels: the row from the events socket, the pane
from `ws.onclose`, with nothing reconciling them, so either could happen
without the other.

The catalog is now the authority. `shouldRetryClose` decides from session
status: Running means the link dropped, so retry with 500ms-to-10s
backoff; Dead means the PTY stream was sealed and nothing more is coming;
absent means it is gone. Application closes (4000+, a relayed daemon
rejection) and clean closes are never retried. The shell also closes any
pane whose session has left the catalog. The policy lives in reconnect.ts
so it is tested without a DOM.

kill_session checked `has()` before `kill()`, which only opened a window
for the liveness watcher to reap in between — turning a 404 into a 502.
`kill` reports the miss itself.

with_spawn_history logged and carried on when `Arc::get_mut` failed,
producing a daemon whose recent-dirs picker is permanently empty with
nothing to show for it. It panics now: that is a construction-order
mistake, at startup, in code that runs once.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

naps62-yolo merged commit f31377c34f into main

2026-08-22 16:34:30 +01:00

naps62-yolo deleted branch lithuanians

2026-08-22 16:34:30 +01:00

naps62-yolo referenced this pull request

2026-08-22 17:02:33 +01:00

fix(maestro): ask the kernel which process owns the terminal #51

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: naps62/maestro#50

fix(maestro): three defects the review flagged and left open #50

The SPA destroyed a pane on any dropped frame

kill_session had a TOCTOU that produced the wrong status

with_spawn_history failed silently

`kill_session` had a TOCTOU that produced the wrong status

`with_spawn_history` failed silently