Skip to content

Review follow-up for #15: don't strand a raw terminal, and gate passive at the constructor - #7

Open
iksteen wants to merge 1 commit into
mrexodia:multiglassfrom
iksteen:pr15-review-fixes
Open

iksteen wants to merge 1 commit into
mrexodia:multiglassfrom
iksteen:pr15-review-fixes

Conversation

@iksteen

@iksteen iksteen commented Aug 7, 2026

Copy link
Copy Markdown

Review follow-up for iksteen#15 — one commit on top of your two, so you can take it, take part of it, or ignore it. Nothing is merged on my side.

1. A permanent hub failure could leave the terminal in raw mode

client::run bails out on a 403 (rejected key) and a 426 (wire skew). That was safe only because the source could not exist yet — the gate you're bypassing is what guaranteed it. With eager_start the command is already running in raw mode when those fire, and nothing restores it: there is no Drop for RawMode, and the only raw.restore() is the child-exit path, which never runs here. A caller that exits on the error — the CLI does — leaves a terminal with no echo and no line editing.

The fix announces through the notifier's existing outage pause before propagating, so the terminal comes back cooked with the reason on screen. A passive source is deliberately left alone, same contract as an ordinary outage.

Worth noting this was reachable before your PR too: a mid-session reconnect that gets 403'd after the hub's --allow changes hits the same bail! with a live backend. eager_start just moves it from obscure to likely.

I did not make 403/426 retryable — RECONNECT_BACKOFF is 500ms and the hub logs rejections for fail2ban, so a client retrying a bad key twice a second is exactly what that log is for.

2. passive can stop at the constructor

Notifier is the only producer of Msg::HubDown/HubUp, and the two arms you guard are the only consumers. Not installing it makes them unreachable by construction instead of by two guards that have to stay symmetric, and NoopSinkStatus already exists for this. screen_thread goes back to 10 arguments.

The flags themselves stay exactly where you put them — eager_start on the transport, passive on pty::start. They're orthogonal: an external_source() producer already gets NoopSinkStatus and wants only the first, and a PTY embedder may well want passive with the hub-gated start.

3. Docs

docs/library-api.md still said the factory runs only after the upgrade succeeds. Documented eager_start and its pairing with passive, and noted on HubState::live why it's public.


fmt, clippy -D warnings, cargo test --workspace (191 passed), and the five subset feature builds are all green. One test added for the new path.

🤖 Generated with Claude Code

Three fixes on top of the eager-start/passive change:

- A permanent hub failure (403 rejected key, 426 wire skew) returned
  straight out of `client::run` with a `bail!`. That was safe only because
  the source could not exist yet — with `eager_start` the command is
  already running in raw mode, and nothing in the tree restores it: there
  is no `Drop for RawMode` and the sole `raw.restore()` is the child-exit
  path. A caller that exits on the error (the CLI does) left the terminal
  with no echo and no line editing. Announce through the notifier's
  existing outage pause before propagating, so the terminal comes back
  cooked with the reason on screen. Also repairs the same hazard on a
  mid-session reconnect that gets 403'd after the hub's --allow changed.

- `passive` was spent in the screen loop — two guards, an 11th argument to
  `screen_thread`, and messages built and dropped every outage. The
  notifier is the only producer of `Msg::HubDown`/`HubUp`, so not
  installing it makes those arms unreachable by construction instead of by
  two guards that have to stay symmetric. `NoopSinkStatus` already exists
  for exactly this.

- docs/library-api.md still claimed the source factory runs only after the
  upgrade succeeds; document `eager_start` and its pairing with `passive`.
  Note on `HubState::live` why it is public.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant