RFC: Wasmi extensions and native channels - #167
Conversation
|
Coverage
|
f2553b1 to
3a73eff
Compare
f611f4b to
1ee6559
Compare
|
🔗 Preview: https://blit-ga7yaa8xz-indent.vercel.app |
|
Reviewing this against #94 (units — declarative process supervision), which I'd been designing as server-side code. Having read this, most of that unit layer should be an extension instead, and I'm yielding the allocations I'd claimed: feature bit 11, the I need one thing added, and it's a sentence of specification rather than a design change. Please state that an extension endpoint receives unsolicited S2C messagesPacket parity is stated in one direction only. "An extension may form any valid C2S packet… If an operation is available to an ordinary client, it is available to an extension" tells me what a guest can send. What a guest receives unprompted is only ever implied — by the initial-burst inheritance, and by the snapshot-to-live reconciliation machinery, which would be pointless if global notifications didn't reach the extension endpoint. Anything supervisory built on this depends on the receive direction, and specifically on Concretely: a line in Packet parity saying the guest receives every unsolicited S2C message an ordinary client would, subject to the same subscription gating, with Two observations, no action needed
The at-least-once restart contract is the right call and worth keeping loud. "Blit does not checkpoint Wasm memory" plus "must make side effects idempotent or store their own progress transactionally" is precisely the constraint a supervisor has to design around, and it pushed me toward keeping unit state in KV rather than in guest memory. Happy to review the receive-direction wording once you've picked it. |
## Summary Move the existing FS/Git/KV/LSP/NET implementations and client diagnostics onto the common status registry proposed by #167. - classify malformed Git and LSP inputs as `INVALID` instead of `OTHER` - make Rust and TypeScript status renderers distinguish declared backend `OTHER` from unknown numeric status values - update existing-family documentation and comments to point at the canonical registry - add Rust and TypeScript regression tests This deliberately excludes the native process RFC, extension runtime work, Git fetch NUL hardening, and Git diff fixture stabilization. ## Stack This is based on #167 because that PR introduces `docs/protocol.md#common-status-registry`. The code cleanup can be reviewed separately and merged after the base lands. ## Verification - `cargo fmt --check` - changed Git status cases: `open_reports_repo`, `tree_blob_and_base`, and `log_follow_directory_and_unknown_flags` - LSP malformed-path regression - four remote status-text regressions - TypeScript status-text test - `tsc --noEmit` - `git diff --check`
|
Addressed in c923d46. Packet parity now explicitly says an extension endpoint receives the same initial state, correlated replies, and unsolicited S2C messages as an ordinary client under the same subscription, ownership, fan-out, and ordering rules. It names S2C_EXITED plus S2C_TITLE, S2C_USED_ROWS, and S2C_TERM_CWD_EVENT. |
## Summary Move the existing FS/Git/KV/LSP/NET implementations and client diagnostics onto the common status registry proposed by #167. - classify malformed Git and LSP inputs as `INVALID` instead of `OTHER` - make Rust and TypeScript status renderers distinguish declared backend `OTHER` from unknown numeric status values - update existing-family documentation and comments to point at the canonical registry - add Rust and TypeScript regression tests This deliberately excludes the native process RFC, extension runtime work, Git fetch NUL hardening, and Git diff fixture stabilization. ## Stack This is based on #167 because that PR introduces `docs/protocol.md#common-status-registry`. The code cleanup can be reviewed separately and merged after the base lands. ## Verification - `cargo fmt --check` - changed Git status cases: `open_reports_repo`, `tree_blob_and_base`, and `log_follow_directory_and_unknown_flags` - LSP malformed-path regression - four remote status-text regressions - TypeScript status-text test - `tsc --noEmit` - `git diff --check`
c923d46 to
6dd71f5
Compare
…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.
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.
#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.
Summary
send, blocking packetrecv, direct realtime/monotonicclock, and OS-backedrandomblit run --on ... FILE ARGS...sends module bytes only on a cache miss@nameCLI command trees, with invocation I/O carried over channelsLifecycle and resource model
extension_ididentifies one supervised installation;(extension_id, attempt)identifies one Wasmi instancebuilderandbuilder-canaryon-failure; update, cancellation, shutdown, and persistence have explicit supervisor transitionsCLI and command extensibility
blit ext run --on ... FILE ARGS...installs/runs an extension;blit runis an alias and positional tokens afterFILEare guest argumentsblit ext update NAME FILE ARGS...atomically switches a persistent definition after ID/revision checksblit.cli.v1descriptor and channel listener throughEXT_COMMANDblit ext commands,@name --help, and completion use immutable directory snapshots; invocation is never retried across attemptsKey decisions
wasm32-unknown-unknown, and one host import module,blit_v1Protocol allocation
EXTENSION, direction-local0x90through0x94CHANNEL, bidirectional0x95;0x96remains freeCREATE2(WANT_STATUS)failuresSplit follow-ups
Both are stacked on this branch so #167 remains focused and reviewable. They can merge separately after the base.
Scope and verification
This PR is a design RFC plus protocol documentation corrections. It does not implement the runtime, SDK, CLI, packet families, or non-PTY process protocol.
git diff --checkpassesdocs/design/extensions.md, selecteddocs/protocol.mdchanges, and the stale Git opcode comment correction