server: make the connection and pty caps reachable; correct drifted docs - #188
Merged
Conversation
`Config::max_connections` and `Config::max_ptys` are enforced in blit-server, but the CLI — the only thing that ever builds a `Config` — hardcoded both to 0. No flag, no env var, no way to set them. The enforcement has been dead code, and `active_connections` was maintained to feed a check that could not fire. Read them from `BLIT_MAX_CONNECTIONS` and `BLIT_MAX_PTYS`, matching how every other server knob is configured, and document them. Deleting the fields instead would have been the other way to resolve this, but `Config` is public API on a published crate with no `Default` impl, so removing a field breaks every embedder's struct literal. These are an operator sanity bound against runaway automation, not a security control — a client that can open one PTY can already spend the machine from inside it — so 0/unlimited stays the default. A refused CREATE gets no reply: the protocol has no "create refused" message and all four call sites drop the request, so the client sees a timeout. Log the refusal server-side so a cap does not look like a hang, and say so in the docs. A real refusal message needs a protocol addition. Verified with BLIT_MAX_PTYS=2: two terminals start, the third is refused and logged. Co-Authored-By: Claude <noreply@anthropic.com>
Found while inventorying the tree's hardcoded limits. Each of these describes something the code does not do. `server.md` said the forked PTY child "drops privileges". It does not — there is no setuid, setgid, chroot, seccomp or landlock anywhere in crates/. What the fork does is close inherited descriptors, which keeps one terminal from reaching another's PTY master, but that is hygiene between sibling terminals, not a boundary between a client and the machine. Say what actually holds: a connection is an interactive login shell as the server's user, and confinement belongs outside the server. This is the one that could get someone hurt. `fs-grep.md` said `FS_GREP_MAX_FILE` is 4 MiB in prose while its own budget table two hundred lines down said 64 MiB. The code says 64 MiB. `fs-search.md` said the client file index refreshes when older than 15 s. `fileIndex.ts` uses 60 s. `fs-watch.md` listed `BLIT_FS_RETAIN_MAX` in the knobs table with a footnote admitting it does not exist. A row in an env-var column reads as configurable however it is footnoted; move it to prose. Its security section also claimed oversized paths are rejected — no length check exists in `validate_root` or `resolve_wire_path`. Over-length paths are clipped on encode, which stops them corrupting the surrounding message but is not rejection. `BLANKET_FRAME_INTERVAL_SURFACE`'s comment claimed 8 ms (~120 Hz) and an idle 33 ms (30 Hz). The constants are 62.5 ms (16 Hz) and 250 ms (4 Hz). Also note what the value is for, since 16 Hz reads alarmingly like a frame rate cap and is nothing of the sort. Co-Authored-By: Claude <noreply@anthropic.com>
|
Coverage
|
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
`BLIT_MAX_PTYS` became reachable in #188, but it could not be used safely: all four create arms refuse with a bare `continue` and no reply. `allocate_pty_id` says so itself — "the protocol has no 'create refused' message" — and settles for an `eprintln` so the cap at least leaves a trace in the server log. The client still sees nothing. `blit terminal start` hits its 10s socket timeout and reports "timeout waiting for server response"; the browser client's promise simply never settles. Turning the cap on traded an unbounded terminal count for a client that hangs. Give the protocol the missing message. `CREATE2(WANT_STATUS)` gets exactly one outcome — `CREATED_N` or `CREATE_FAILED [nonce:2][status:1][detail:N]` — negotiated through a new HELLO bit, with statuses from a common registry this also introduces. `BUDGET` names which exhaustion was hit, since raising a cap and waiting for ids to free up are different operator actions. Deliberately opt-in: `CREATE`, `CREATE_AT`, `CREATE_N`, and unflagged `CREATE2` keep their success-only contract, so a legacy client cannot mistake a refusal for PTY zero. That leaves `CREATE_N` unable to learn why it was refused — it has no feature byte to carry the request — and the `eprintln` stays for those arms. Both shipped clients use `CREATE2`. Every refusal added here replaces a path that was already dropping the request, except one: a tag or command over `u16::MAX` now returns `TOO_LARGE` instead of truncating through `pty_list_msg`'s `as u16` casts and desynchronizing `S2C_LIST` for every client. `--max-ptys` is added alongside the env var, for symmetry with the other server knobs. The default stays 0 — #188 argued unlimited is right, since a client that can open a terminal can already spend the machine from inside it, and nothing here changes that. Verified end to end against a server run with `--max-ptys 1`: the second create is refused in milliseconds with "budget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)" and exit 1, and the terminal list still holds exactly one.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
`BLIT_MAX_PTYS` became reachable in #188, but it could not be used safely: all four create arms refuse with a bare `continue` and no reply. `allocate_pty_id` says so itself — "the protocol has no 'create refused' message" — and settles for an `eprintln` so the cap at least leaves a trace in the server log. The client still sees nothing. `blit terminal start` hits its 10s socket timeout and reports "timeout waiting for server response"; the browser client's promise simply never settles. Turning the cap on traded an unbounded terminal count for a client that hangs. Give the protocol the missing message. `CREATE2(WANT_STATUS)` gets exactly one outcome — `CREATED_N` or `CREATE_FAILED [nonce:2][status:1][detail:N]` — negotiated through a new HELLO bit, with statuses from a common registry this also introduces. `BUDGET` names which exhaustion was hit, since raising a cap and waiting for ids to free up are different operator actions. Deliberately opt-in: `CREATE`, `CREATE_AT`, `CREATE_N`, and unflagged `CREATE2` keep their success-only contract, so a legacy client cannot mistake a refusal for PTY zero. That leaves `CREATE_N` unable to learn why it was refused — it has no feature byte to carry the request — and the `eprintln` stays for those arms. Both shipped clients use `CREATE2`. Every refusal added here replaces a path that was already dropping the request, except one: a tag or command over `u16::MAX` now returns `TOO_LARGE` instead of truncating through `pty_list_msg`'s `as u16` casts and desynchronizing `S2C_LIST` for every client. `--max-ptys` is added alongside the env var, for symmetry with the other server knobs. The default stays 0 — #188 argued unlimited is right, since a client that can open a terminal can already spend the machine from inside it, and nothing here changes that. Verified end to end against a server run with `--max-ptys 1`: the second create is refused in milliseconds with "budget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)" and exit 1, and the terminal list still holds exactly one.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
`BLIT_MAX_PTYS` became reachable in #188, but it could not be used safely: all four create arms refuse with a bare `continue` and no reply. `allocate_pty_id` says so itself — "the protocol has no 'create refused' message" — and settles for an `eprintln` so the cap at least leaves a trace in the server log. The client still sees nothing. `blit terminal start` hits its 10s socket timeout and reports "timeout waiting for server response"; the browser client's promise simply never settles. Turning the cap on traded an unbounded terminal count for a client that hangs. Give the protocol the missing message. `CREATE2(WANT_STATUS)` gets exactly one outcome — `CREATED_N` or `CREATE_FAILED [nonce:2][status:1][detail:N]` — negotiated through a new HELLO bit, with statuses from a common registry this also introduces. `BUDGET` names which exhaustion was hit, since raising a cap and waiting for ids to free up are different operator actions. Deliberately opt-in: `CREATE`, `CREATE_AT`, `CREATE_N`, and unflagged `CREATE2` keep their success-only contract, so a legacy client cannot mistake a refusal for PTY zero. That leaves `CREATE_N` unable to learn why it was refused — it has no feature byte to carry the request — and the `eprintln` stays for those arms. Both shipped clients use `CREATE2`. Every refusal added here replaces a path that was already dropping the request, except one: a tag or command over `u16::MAX` now returns `TOO_LARGE` instead of truncating through `pty_list_msg`'s `as u16` casts and desynchronizing `S2C_LIST` for every client. `--max-ptys` is added alongside the env var, for symmetry with the other server knobs. The default stays 0 — #188 argued unlimited is right, since a client that can open a terminal can already spend the machine from inside it, and nothing here changes that. Verified end to end against a server run with `--max-ptys 1`: the second create is refused in milliseconds with "budget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)" and exit 1, and the terminal list still holds exactly one.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
…dlines, retention (#204) Closes #181. Implements all three gaps in #181. They share one cause — nothing in the server owned a PTY's lifetime — and one implementation: a supervisor loop that runs when the delivery tick does not. Five commits, each building standalone and separately verified: | Commit | #181 item | What it fixes | | --- | --- | --- | | `answer a refused create instead of dropping it` | 3 (half) | all four create arms refuse with a bare `continue`, so a nonce-bearing client waits forever | | `kill a terminal's process group, not just its leader` | 2 | `kill(pid)` / `kill(pid, SIGHUP)` reached the session leader alone — kill a shell, keep its children | | `detect a terminal's exit from the child, not from EOF` | 1, 2 | exit detection was EOF-on-master, which means "the slave closed", not "the child exited" | | `enforce opt-in terminal deadlines, and say when one fired` | 1 | every timeout was client-side, so none survived the client that set it | | `bound retained terminals, and count the cap against live ones` | 3 (rest) | nothing but an explicit `CLOSE` ever removed an exited entry | ## Relationship to #188 #188 made `BLIT_MAX_PTYS` reachable and argued — correctly — that unlimited is the right default. It also documented the gap this PR closes, in `allocate_pty_id`: *"the protocol has no 'create refused' message"*, settling for an `eprintln` so the cap at least leaves a trace in the server log. So the cap could be set but not safely used: turning it on traded an unbounded terminal count for a client that hangs. This adds the missing message and **leaves the default at 0**. `--max-ptys` is added alongside the env var for symmetry with the other server knobs; the `eprintln` stays, because the older create opcodes still drop the request silently by design. The last commit does change the cap's *counting* to live terminals only, so a client running short commands under `--max-ptys N` is not refused after N of them with nothing running. Exited terminals get their own bound instead. ## Verification Each commit message records its own check. The load-bearing ones, all re-run after the rebase onto main: - **Refusal**: server with `--max-ptys 1` refuses the second create in milliseconds with `budget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)` and exit 1. Previously: a 10s hang, then a generic socket timeout. - **Exit detection**: A/B'd against a pre-change server with the same command, `bash -c '(trap "" HUP; sleep N) & exit 7'` — a grandchild that ignores the hangup and keeps the slave open. Before: the terminal sits at `running` indefinitely. After: `exited(7)`. - **Deadlines**: a terminal created with `--deadline 5` and abandoned dies at ~5s with no client attached. `blit terminal wait` prints `signal(15) — killed by deadline` where a hand-rolled `kill 9` prints a bare `signal(9)`. Refreshed every 2s against a 4s deadline it survived 12s, then died 8s after the refreshes stopped. - **Retention**: with `--max-ptys 2 BLIT_MAX_EXITED=3`, six consecutive short commands all succeed and the list settles at the newest three. - **Group kill**: two tests pin both halves — one asserts a child survives a leader-only kill, the other that a group kill reaches it. Mutation-checked by flipping the second to leader-only and confirming it fails. Workspace clippy clean, `cargo fmt` clean, 556 Rust tests and 812 JS tests passing, JS typecheck clean. ## Review notes **Two things not verified here.** The Windows job-object half has no toolchain in this checkout (Nix, no rustup) and rests on CI's windows build. And the third commit removes `reap_zombies`' global `waitpid(-1)` drain — a strict improvement, since it was reaping other subsystems' children and discarding their statuses out from under the audio pipeline's own `try_wait`, but it is a change outside the PTY family. **Group kill's limit, stated rather than papered over.** It reaches the leader's process group and, via `TIOCGPGRP`, the terminal's foreground group. A job backgrounded by an interactive shell is in neither and survives. Bounding that needs a cgroup, not a signal. **Feature bits 11–13 are left unallocated** for the extension, channel, and process families under review in #167 and #173. This takes 14 (`CREATE_STATUS`), 15 (`KILL_MODE`), and 16 (`PTY_DEADLINE`), matching the allocation #167's `protocol.md` already proposes for 14. The common status registry this introduces is #167's design; landing it here means #167 can drop that section rather than restate it. **Five pre-existing test failures in `crates/git`** are unrelated — identical 55-passed/5-failed on a tree with none of these changes (a local git config makes `git tag v1` demand a message). ## Follow-ups, deliberately not in here - `docs/design/units.md` (#94) needs reconciling before it merges: it allocates `S2C_LEASE = 0x10`, which this PR now uses for `CREATE_FAILED`; it gives `CREATE_FAILED` a different opcode *and* payload; and its "the `C2S_KILL` flags arm is `data.len() >= 7`" is off by one, since 7 is the existing message length. - The timed `C2S_CLOSE` escalation from units.md needs `CLOSE` to hold the entry in a "closing" state, which tangles with the retention path, and is not part of what #181 asks for. - Bounding the *aggregate* `S2C_LIST` size needs a logical-message ceiling that does not exist yet. The per-field `TOO_LARGE` check is in.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
The primitives section proposed wire that has since landed, and in two
places landed differently. Left as written it now contradicts the
protocol.
- `S2C_LEASE` moves to `0x11`. `0x10` was free when this was written;
#204 shipped `S2C_CREATE_FAILED` there.
- `S2C_CREATE_FAILED` is `[0x10][nonce:2][status:1][detail:N]`, not
`[0x11][nonce:2][reason:1]` — the common status registry rather than
a message-local reason byte, matching what #167's protocol.md had
already allocated. It is also opt-in per request via
`CREATE2_WANT_STATUS`, so a legacy client cannot read a refusal as
PTY zero.
- `max_ptys` kept its `0` default rather than gaining a real one.
#188 landed the env var in the meantime and argued unlimited is
right, and that argument holds: a client that can open a terminal
can already spend the machine from inside it.
- `FEATURE_UNITS` moves to bit 17. 11-13 are reserved for the
extension, channel, and process families; 14-16 shipped with #204.
- The `C2S_KILL` flags arm is `data.len() >= 8`, not `>= 7`. Seven is
the existing message length, so arming there reads a byte that is
not there.
Delivery marks 1-3 shipped and narrows 2 to what is actually left: the
lease family, and the timed `C2S_CLOSE` escalation, which needs `CLOSE`
to hold a "closing" state and tangles with the retention path.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
#204 landed the RFC's primitives (delivery items 1-3, tracked as #181). Two of them landed differently from what the RFC proposed, and the RFC now contradicts the shipped protocol in ways that would mislead whoever implements the unit layer on top. | RFC said | Shipped | Why | | --- | --- | --- | | `S2C_LEASE [0x10]` | must move to `0x11` | `0x10` was free when written; #204 put `S2C_CREATE_FAILED` there | | `S2C_CREATE_FAILED [0x11][nonce:2][reason:1]` | `[0x10][nonce:2][status:1][detail:N]` | common status registry rather than a message-local byte, matching #167's `protocol.md` | | `max_ptys` gets a real default | kept `0` | #188 landed the env var meanwhile and argued unlimited is right | | `FEATURE_UNITS` bit 11 | bit 17 | 11-13 reserved for extension/channel/process, 14-16 shipped with #204 | | `C2S_KILL` flags arm at `len >= 7` | `>= 8` | 7 is the existing message length — arming there reads a byte that isn't present | Also worth knowing for the layer above: the refusal is **opt-in per request**. A client sets `CREATE2_WANT_STATUS` (bit 3) after seeing `FEATURE_CREATE_STATUS` (HELLO bit 14), so `CREATE`, `CREATE_AT`, `CREATE_N` and unflagged `CREATE2` keep their success-only contract and a legacy client can't read a refusal as PTY zero. Delivery now marks 1-3 shipped and narrows item 2 to what's actually left: the lease family, and the timed `C2S_CLOSE` escalation. That second one needs `CLOSE` to hold the entry in a "closing" state, which tangles with the retention path #204 added — it was deliberately out of scope for #181 and is still open. No changes to the unit layer itself; how the primitives landed doesn't affect it.
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
1 participant
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.
Dead limit knobs
Config::max_connectionsandConfig::max_ptysare enforced inblit-server, but the CLI — the only thing that ever builds aConfig— hardcoded both to0. No flag, no env var, no way to set them. The enforcement has been dead code, andactive_connectionswas maintained to feed a check that could not fire.Reads them from
BLIT_MAX_CONNECTIONS/BLIT_MAX_PTYS, matching how every other server knob is configured, and documents them.Deleting the fields was the other option, but
Configis public API on a published crate with noDefaultimpl, so removing a field breaks every embedder's struct literal.These are an operator sanity bound against runaway automation, not a security control — a client that can open one PTY can already spend the machine from inside it — so 0/unlimited stays the default.
Caveat, documented: a refused
CREATEgets no reply. The protocol has no "create refused" message and all four call sites drop the request, so the client sees a timeout. This adds a server-side log line so a cap doesn't look like a hang. A real refusal needs a protocol addition.Verified with
BLIT_MAX_PTYS=2: two terminals start, the third is refused and logged.Documentation that described things the code doesn't do
server.mdsaid the forked PTY child "drops privileges". It does not — there is nosetuid,setgid,chroot,seccomporlandlockanywhere incrates/. What the fork does is close inherited descriptors, which keeps one terminal from reaching another's PTY master — hygiene between sibling terminals, not a boundary between a client and the machine. This is the one that could get someone hurt.fs-grep.mdsaidFS_GREP_MAX_FILEis 4 MiB in prose while its own budget table said 64 MiB. Code says 64 MiB.fs-search.mdsaid the client file index refreshes at 15 s.fileIndex.tsuses 60 s.fs-watch.mdlistedBLIT_FS_RETAIN_MAXin the knobs table with a footnote admitting it doesn't exist — a row in an env-var column reads as configurable however it's footnoted. Its security section also claimed oversized paths are rejected; no length check exists invalidate_rootorresolve_wire_path.BLANKET_FRAME_INTERVAL_SURFACE's comment claimed 8 ms (~120 Hz) and idle 33 ms (30 Hz). The constants are 62.5 ms (16 Hz) and 250 ms (4 Hz).