docs(arch): alknet-tty spec sanity-check fixes — TtyControlHandle newtype (C1), backend cleanup contract (W1, ADR-056), clarifications (S1-S5)

C1: Box<dyn TtyControl + Clone> does not compile (Clone is not object-safe).
Replace with a TtyControlHandle newtype: #[derive(Clone)] wrapping
Arc<dyn TtyControl + Send + Sync>. The trait stays object-safe; the
newtype carries the Clone-ability. Updated across tty-backend.md,
ADR-053, OQ-43, and the tty README.

W1: 'Drop is the cleanup, threads exit on channel close' was false for
the local PTY waiter thread (blocked in Child::wait(), does not observe
channel close) — a child that ignores stdin EOF would be orphaned on
session cancel. New ADR-056 commits a backend cleanup contract: dropping
the exit_code future MUST kill the session target. The local backend
implements it via a ChildKiller held in the exit_code future's Drop
guard (disarmed on resolve). Corrected the lifecycle section in
tty-adapter.md, added the mechanism + constraint to tty-local.md, and
referenced ADR-056 from tty-backend.md, overview.md, both READMEs.

S1: error-frame < 16 MiB stated as a wire-format invariant (not an
assumption) so the 0x00-first-byte disambiguation trick is sound by
construction.
S2: carriage field validation (MUST be "raw" in v1, else
malformed_negotiation) specified.
S3: NegotiateRequest + TerminalParamsWire Rust structs added with
serde defaults and validation rules.
S4: modes field annotated 'backends MUST ignore content in v1' in both
tty-backend.md and ADR-053.
S5: AsyncWrite/Stream qualified with crate origins (tokio::io,
futures_core) in the TtyHandle struct definitions.
This commit is contained in:
glm-5.2 committed 2026-07-07 10:38:49 +00:00
1 parent b90d717ac2
commit 25f0f55592
11 files changed
+594 -92

No files matched your search

@@ -194,17 +194,18 @@ closes after it.
allocation (raw chunks) and a failed allocation (JSON error frame) begin
with bytes the client must read before knowing which framing applies.
The disambiguation is by the first byte: a JSON error frame's 4-byte
big-endian length prefix always starts with `0x00` (error frames are
small — under 16 MiB, so the high byte is zero), while a raw chunk's
first byte is a `stream_type` in `{0, 1, 2, 3}`. A stream_type of `0`
(stdin from server) is invalid — the server never sends stdin chunks —
so the client distinguishes: read the first byte; if it is `0x00`,
interpret the next 4 bytes as a big-endian length prefix and read that
many bytes as a JSON error frame; otherwise interpret it as a
`stream_type` byte and continue reading the raw chunk header. This is a
one-way-door wire-format invariant: error frames use the negotiation
framing (length prefix), success uses the raw chunk framing
(stream_type byte first); the `0x00`-as-length-prefix vs
big-endian length prefix always starts with `0x00` (error frames MUST
be under 16 MiB — `MAX_CHUNK_LEN` — so the high byte is zero; this is a
wire-format invariant, not an assumption), while a raw chunk's first
byte is a `stream_type` in `{0, 1, 2, 3}`. A stream_type of `0` (stdin
from server) is invalid — the server never sends stdin chunks — so the
client distinguishes: read the first byte; if it is `0x00`, interpret
the next 4 bytes as a big-endian length prefix and read that many bytes
as a JSON error frame; otherwise interpret it as a `stream_type` byte
and continue reading the raw chunk header. This is a one-way-door
wire-format invariant (ADR-052): error frames use the negotiation
framing (length prefix) and MUST be under 16 MiB; success uses the raw
chunk framing (stream_type byte first); the `0x00`-as-length-prefix vs
`0x00`-as-invalid-stream_type disambiguation is what makes the two
distinguishable on the wire.
@@ -59,11 +59,13 @@ oneshot::Receiver<i32>`. The trait shape below generalizes this.
### What the local-PTY POC did not resolve
The POC used a separate cloneable `PtyControl` struct for resize/signal,
not a `Box<dyn TtyControl + Send + Unpin>` trait object. The research
noted this worked cleanly because the control-chunk dispatcher needs to be
`Clone` to hand off to the spawned pump task. Phase 1 confirms the `control`
field as a separate `Clone` trait object — see OQ-43 for the confirmation
and the `Clone` constraint rationale.
not a trait object. The research noted this worked cleanly because the
control-chunk dispatcher needs to be `Clone` to hand off to the spawned
pump task. Phase 1 confirms the `control` field as a separate
`TtyControlHandle` newtype — a concrete `#[derive(Clone)]` struct
wrapping `Arc<dyn TtyControl + Send + Sync>` (the trait is NOT `Clone`;
`Clone` is not object-safe — see OQ-43). The newtype carries the
`Clone`-ability; the trait stays object-safe.
## Decision
@@ -122,7 +124,7 @@ pub struct TerminalParams {
pub rows: u16,
pub pixel_width: u16,
pub pixel_height: u16,
pub modes: serde_json::Value, // reserved — see OQ-44
pub modes: serde_json::Value, // reserved — OQ-44; backends MUST ignore content in v1
}
```
@@ -209,14 +211,19 @@ below.
```rust
pub struct TtyHandle {
/// Stdin writer — bytes the adapter pumps from client stdin chunks.
pub stdin: Box<dyn AsyncWrite + Send + Unpin>,
/// `tokio::io::AsyncWrite` (the tokio flavor, not the `futures::io`
/// one — they are incompatible traits; the tokio stack is the
/// adapter's runtime).
pub stdin: Box<dyn tokio::io::AsyncWrite + Send + Unpin>,
/// Stdout stream — bytes the adapter pumps to client stdout chunks.
/// Ends when the backend's stdout reaches EOF (process exited,
/// container output stream ended, SSH channel closed).
pub stdout: Pin<Box<dyn Stream<Item = Bytes> + Send>>,
/// `futures_core::Stream<Item = bytes::Bytes>` (re-exported by
/// `tokio_stream::StreamExt` for extension methods).
pub stdout: Pin<Box<dyn futures_core::Stream<Item = bytes::Bytes> + Send>>,
/// Stderr stream — `None` for PTY backends (stdout/stderr merged
/// into `stdout`). `Some` for pipe backends (separate streams).
pub stderr: Option<Pin<Box<dyn Stream<Item = Bytes> + Send>>>,
pub stderr: Option<Pin<Box<dyn futures_core::Stream<Item = bytes::Bytes> + Send>>>,
/// Exit code — a `Future` the adapter awaits. Resolves when the
/// process/container/SSH exec exits. The adapter sends the result
/// as the `{"type":"exit","code":N}` control chunk (ADR-055) and
@@ -228,12 +235,12 @@ pub struct TtyHandle {
/// hand it to the spawned control-chunk dispatcher. `None` only
/// when the backend genuinely has no control path (e.g., a pipe
/// backend with no PTY — signal still works via `kill(pid, sig)`,
/// but resize is a no-op). See OQ-43 for the trait-object-vs-methods
/// confirmation.
pub control: Option<Box<dyn TtyControl + Send + Unpin + Clone>>,
/// but resize is a no-op). See OQ-43 for the `TtyControlHandle`
/// newtype rationale.
pub control: Option<TtyControlHandle>,
}
pub trait TtyControl: Send {
pub trait TtyControl: Send + Sync {
/// Resize the terminal. Maps to SSH `window-change`, docker exec
/// resize, or `ioctl(TIOCSWINSZ)` on a local PTY. No-op for pipe
/// backends without a PTY (the adapter still calls it; the backend
@@ -245,6 +252,15 @@ pub trait TtyControl: Send {
/// back to the backend's default kill.
fn signal(&self, name: &str);
}
/// The `Clone`-able handle to a backend's control path. The `TtyControl`
/// trait is NOT `Clone` (`Clone` is not object-safe — `fn clone(&self) ->
/// Self` returns `Self`, which forbids `dyn` dispatch); the `Clone`-ability
/// lives on this concrete newtype, which holds the trait object behind an
/// `Arc`. The adapter clones the `Arc` to hand a handle to the spawned
/// control-chunk dispatcher. See OQ-43.
#[derive(Clone)]
pub struct TtyControlHandle(Arc<dyn TtyControl + Send + Sync>);
```
### 4. Backends are not required to be natively async (REQ-TTY-01)
@@ -320,13 +336,16 @@ layer chooses what's available.
full rationale and why the typed-enum alternative was rejected (Rust
enums are closed; the earlier `SshChannelRef` variant was an output
modeled as an input and created a dependency contradiction).
- `Box<dyn TtyControl + Send + Unpin + Clone>` is an unusual trait-object
bound (`Clone` on a trait object requires a workaround — typically a
custom clone-via-`Arc` or a `Box<dyn TtyControl>` wrapped in a small
`Clone` newtype). This is the cost of the POC-discovered constraint
that the control-chunk dispatcher needs to be `Clone` to hand off to
the spawned pump task. See OQ-43 for the confirmation and the concrete
`Clone` newtype approach.
- **`TtyControl` is not `Clone`; the `TtyControlHandle` newtype is.**
`Clone` is not object-safe (`fn clone(&self) -> Self` returns `Self`,
which forbids `dyn` dispatch), so `Box<dyn TtyControl + Clone>` does
not compile. The design splits the concerns: the `TtyControl` trait
stays object-safe (`Send + Sync`, no `Clone`); the `TtyControlHandle`
newtype (a concrete struct holding `Arc<dyn TtyControl + Send +
Sync>`) implements `Clone` by cloning the `Arc`. This is the cost of
the POC-discovered constraint that the control-chunk dispatcher needs
to be `Clone` to hand off to the spawned pump task. See OQ-43 for the
confirmation and the concrete newtype approach.
- A backend that produces neither a PTY nor a process (a hypothetical
"recorded session replay" backend) would have a no-op `TtyControl` and
a synthetic `exit_code`. The trait accommodates it but the `TtyParams`
@@ -350,12 +369,15 @@ impl doesn't break existing implementors); its return type
## Assumptions
1. **The `TtyControl` trait object can be made `Clone` via a small
newtype.** The POC used a concrete `PtyControl` struct (inherently
`Clone`). The trait-object form needs a `Clone` newtype wrapping the
`Box` (e.g., a struct holding `Arc<dyn TtyControl>` so `Clone` is an
`Arc` clone). This is a known Rust pattern; OQ-43 confirms the
approach.
1. **The `TtyControl` trait is kept object-safe by NOT putting `Clone`
on it; the `TtyControlHandle` newtype holds the trait object behind an
`Arc` and implements `Clone` by cloning the `Arc`.** The POC used a
concrete `PtyControl` struct (inherently `Clone` — it held
`Arc<Mutex<...>>` fields). The newtype generalizes the POC's shape so
a backend produces its own control type via
`TtyControlHandle::new(Arc::new(MyControl))` without the adapter
knowing the concrete shape. `Clone` cannot live on the trait itself
(it is not object-safe); the newtype is the seam. OQ-43 confirms.
2. **Backends produce a single session per `allocate()` call.** The
adapter calls `allocate()` once per accepted bidi stream (one session
@@ -0,0 +1,279 @@
# ADR-056: Backend Cleanup on Session Cancel (Drop of `exit_code` Kills)
## Status
Accepted
## Context
A `alknet/tty` session can be cancelled before the child process exits
naturally. Three cancel paths exist (see `tty-adapter.md` §"Connection and
Stream Lifecycle"):
1. **Connection drop** — the QUIC connection closes; all in-flight sessions
on it are cancelled.
2. **Stream reset** — the client (or transport) resets the bidi stream
mid-session.
3. **Task panic / adapter shutdown** — the session pump task exits without
completing the normal exit-chunk sequence.
In all three, the adapter's per-session pump tasks are dropped (Rust
`Drop`). The pump tasks hold the `TtyHandle`, whose fields — `stdin`,
`stdout`, `stderr`, `exit_code`, `control` — are dropped too.
The problem the local-PTY POC surfaced: **`Drop` alone is not sufficient
cleanup for a backend whose child may outlive the session.** The local
backend's three std threads illustrate the gap:
- **Reader thread** — blocking `MasterPty::try_clone_reader()` reads. On
`mpsc::Sender::drop` (the stdout channel's sender side drops when the
handle drops), the reader's next `blocking_send` returns a `SendError`;
the thread exits. ✓ `Drop` works.
- **Writer thread** — drains an `mpsc::Receiver<StdinCmd>`. On
`Receiver::drop`, the writer's `recv()` returns `None`; the thread
drops the master writer (EOF to the slave's stdin) and exits. ✓ `Drop`
works.
- **Waiter thread** — blocking `Child::wait()`. This is a syscall that
returns **only when the child is reaped**. It does not observe channel
close. If the child ignores stdin EOF (a daemon, a long-lived process
with no stdin reader, a process in an uninterruptible state), the
waiter thread stays blocked indefinitely and the child is **orphaned**.
✗ `Drop` does not work here.
The same concern applies to any backend whose session target can outlive
the bidi stream: a docker container with `tty: true` whose process ignores
the channel close; an SSH exec whose remote process doesn't exit on
channel close. The unifying property is **a backend-allocated session
target that may outlive the client's interest in it.**
The earlier spec text (`tty-adapter.md` §"Connection and Stream
Lifecycle" pre-this-ADR) asserted:
> No explicit cleanup is needed — `Drop` is the cleanup.
That is wrong for the waiter thread and any backend in the same shape.
This ADR corrects the claim and commits a cleanup contract that closes
the gap.
### Why this is architectural, not implementation
The cleanup contract is part of the `TtyBackend` trait's behavioral
contract (ADR-053), not a backend-internal detail, for two reasons:
1. **The adapter depends on the property.** The adapter's session
driver holds the `TtyHandle` and, on cancel, drops it. The adapter
cannot itself call a backend-specific kill — it doesn't know the
child's pid (the local backend owns it; the adapter never sees it).
The kill must be wired into the backend's handle shape, specifically
into the `exit_code` future the adapter drops on cancel. The
contract is what makes "the adapter drops the handle" sufficient.
2. **A missing contract leaks processes.** An implementer writing a
backend from the trait sketch, without this contract, would ship a
backend that orphans processes on cancel. The bug is silent (the
client got what it wanted; the orphaned process is a server-side
leak), surfaces only under cancel-heavy workloads or long-lived
sessions, and is expensive to attribute after the fact. The contract
makes the property load-bearing at the seam, not a property each
backend rediscovers.
## Decision
### 1. The `TtyBackend` cleanup contract: cancelling the `exit_code` future kills the session target
When the adapter cancels a session (drops the pump tasks, which drops the
`TtyHandle`), the backend's `exit_code` future — the
`BoxFuture<'static, Result<i32, TtyError>>` field of `TtyHandle`
(ADR-053) — is dropped *without being driven to completion*. The cleanup
contract:
> **Dropping the `exit_code` future MUST kill the session target (the
> child process, the docker exec, the SSH channel's process).** The kill
> is best-effort (the target may already be exiting; the kill is a
> no-op then), but it MUST be attempted. The kill MUST be delivered
> even when the session target is blocked in a state that ignores stdin
> EOF (a daemon, a process in uninterruptible sleep, a container whose
> process ignores channel close).
`exit_code`'s `Drop` is the cancel path. The adapter drives the future to
completion (the happy path — the child exits, the future resolves, the
adapter sends the exit chunk); on cancel, the adapter drops the future
(their `Drop`), which runs the kill.
This is a behavioral contract on the `TtyBackend` trait, not a new method.
The trait's `allocate()` returns a `TtyHandle` whose `exit_code` field is
a `Future` with a `Drop`-on-cancel that kills. The mechanism is
backend-specific (see §3 for the local backend); the contract is
backend-agnostic.
### 2. `exit_code`'s `Drop`-on-cancel MUST be safe to run after the future resolves
If the future resolved normally (the adapter awaited it, got the exit
code, sent the exit chunk), the `Drop` runs on an already-resolved
future. The kill MUST be a no-op in that case — the child is already
reaped, the kill is delivered to a nonexistent pid, etc. This is the
"best-effort" qualifier: a kill on an already-exited child is not an
error. Backends implement this with a guard that distinguishes
"resolved" from "cancelled" (a flag, an `Option` taken on resolve, an
`Arc`-shared state).
### 3. Local backend mechanism: `ChildKiller` held in the `exit_code` future's `Drop` guard
The local backend (`alknet-tty-local`, ADR-054) implements the contract
using `portable_pty::ChildKiller` — the kill handle `portable_pty`
exposes alongside `Child::wait()`. The pattern:
- `allocate()` spawns the child via `portable_pty`, obtaining a `Child`
(with `wait()`) and a `ChildKiller` (with `kill()`). It moves the
`Child` into the waiter thread (which blocks on `wait()`). It wraps
the `ChildKiller` and the `oneshot::Receiver<i32>` (from the waiter
thread) into a `Future` that becomes `TtyHandle.exit_code`.
- The `exit_code` future's `poll` delegates to the inner
`oneshot::Receiver::poll` (resolves when the waiter thread sends the
exit code).
- The `exit_code` future's `Drop` (runs on cancel only — on resolve, the
`Drop` is a no-op via the guard) calls `ChildKiller::kill(SIGHUP)`
(or the backend's configured cancel signal). The kill causes the
child to exit; the waiter thread's `wait()` returns; the waiter
thread's `oneshot::send` fails silently (the receiver was dropped
with the future). The waiter thread then exits. The child is reaped
by the waiter thread's `wait()`; no zombie.
For pipe mode (`terminal: None`), the same pattern applies with
`tokio::process::Child::start_kill()` (or `Child::kill()`) instead of
`ChildKiller`. The `exit_code` future's `Drop` guard calls
`start_kill()`; the `Child` is reaped by the future's `wait()` (or by
the waiter task).
### 4. Future backends (docker, SSH) follow the same contract
- **Docker (`DockerTtyBackend`)** — `bollard`'s exec stream is cancelled
by dropping the `AttachContainer` / `start_exec` stream and calling
`bollard::container::kill_container` (or `exec::kill_exec` if
available). The `exit_code` future's `Drop` holds the container/exec
id and the `bollard::Docker` client; on cancel, it issues the kill.
- **SSH (`SshTtyBackend`)** — russh's `Channel::close()` and/or
`Channel::signal(SIGHUP)` terminate the remote process. The
`exit_code` future's `Drop` holds the russh channel handle; on
cancel, it closes the channel.
The docker and SSH backends are future work (out of scope for this spec
set); the contract is what they implement. A future backend that does
NOT fit the contract (e.g., a "recorded session replay" backend with no
live process) implements a no-op `Drop`-on-cancel — the contract is
"kill if there is a killable target; no-op if not."
### 5. The adapter does not call a backend kill method
The adapter has no `TtyBackend::cancel()` or `TtyHandle::kill()` method
to call — the cleanup is wired into the `exit_code` future's `Drop`,
which the adapter triggers by dropping the future. This keeps the trait
surface unchanged (no new method) and the cleanup in the backend (where
the kill handle lives). The adapter's only responsibility is to drop the
`TtyHandle` (and therefore the `exit_code` future) when the session is
cancelled — which it already does by virtue of dropping the pump tasks.
The `TtyControl::signal("HUP")` path (ADR-053) is the *client-initiated*
signal forwarding path — a client sends a `{"type":"signal","name":"HUP"}`
control chunk. It is NOT the cancel-cleanup path. The cancel-cleanup
path is server-internal (the adapter drops the handle) and does not
involve the wire format. These are two different signal paths; both end
in the child receiving SIGHUP (or the backend's configured cancel
signal), but they are triggered by different actors (client vs. server
cancel).
## Consequences
**Positive:**
- A backend that conforms to the contract cannot orphan a process on
cancel. The local-PTY POC's waiter-thread gap is closed at the
contract level, not left to each backend to rediscover.
- The cleanup is idiomatic Rust — `Drop`-on-cancel of a `Future` is the
standard pattern for resource cleanup in async Rust (the same pattern
`tokio::process::Child` uses; the same pattern `tokio::io` AsyncRead
guards use). No new trait method; no adapter-side kill call.
- The contract is backend-agnostic — the mechanism (`ChildKiller` for
local, `kill_container` for docker, `channel::close` for SSH) lives
in the backend; the contract ("drop the future, the target dies")
lives at the seam.
- The happy path (the child exits, the adapter drives `exit_code` to
completion, sends the exit chunk, then drops the resolved future) is
unaffected — the `Drop`-on-resolve is a no-op via the guard.
**Negative:**
- The `exit_code` future is no longer a trivial `oneshot::Receiver<i32>`
wrapper; it carries a kill guard. This is a small implementation
complexity increase (a struct with a `Drop` impl and a resolved-flag),
but it is the cost of the contract. The POC's `LocalPty::exit_code`
was a bare `oneshot::Receiver<i32>`; the spec'd `TtyHandle.exit_code`
is a struct wrapping it. An implementer who copies the POC's bare
shape without the kill guard violates the contract.
- The contract is behavioral, not type-enforced. Rust cannot require
"the `Drop` of the future returned by `allocate()` kills the child"
in the type system. The contract is documented in the `TtyBackend`
trait's doc comment and in this ADR; conformance is the
implementer's responsibility. A test (a "cancel mid-session" test
that asserts the child is reaped after the session is dropped)
should be part of each backend's integration suite.
- A backend whose session target genuinely cannot be killed (a
backend that wraps an immutable shared resource, e.g., a "view a
log stream" backend) implements the contract as a no-op. The
contract is "best-effort kill if there is a killable target"; a
no-op `Drop`-on-cancel is conformant for a non-killable target.
## Door type
**One-way.** The cleanup contract is part of the `TtyBackend` behavioral
contract. Clients (the adapter) depend on "drop the handle, the session
is cleaned up." Changing the contract after backends exist — e.g.,
adding a separate `TtyBackend::cancel()` method and migrating the
cleanup out of `exit_code`'s `Drop` — would require every backend to
change. The `exit_code`-future-`Drop`-on-cancel mechanism is the seam.
## Assumptions
1. **The `exit_code` future's `Drop` is the only cancel path.** The
adapter does not call a separate kill method; it drops the handle.
This means the cleanup runs in the same place the cancel happens (the
pump task's `Drop`), not in a separate cancel call. This is the
idiomatic Rust async cancel pattern and the one the trait commits.
2. **The kill signal is the backend's configured cancel signal
(SIGHUP for the local backend, the docker/SSH equivalent).** This is
a server-internal signal path, distinct from the
client-initiated `TtyControl::signal()` path (ADR-053). The cancel
signal is not configurable from the wire format in v1; a backend
that needs a different cancel signal configures it internally.
3. **The waiter thread (local backend) reaps the killed child.** After
the `Drop`-on-cancel calls `ChildKiller::kill(SIGHUP)`, the child
exits; the waiter thread's `wait()` returns and reaps it (no
zombie). The waiter thread then exits. The `oneshot::send` from the
waiter thread fails silently (the receiver was dropped with the
future) — this is expected and not an error.
## References
- `tty-adapter.md` §"Connection and Stream Lifecycle" — the cancel paths
(connection drop, stream reset) that trigger the contract
- `tty-local.md` §"PTY Mode" — the local backend's three-thread bridge
and the waiter-thread gap this ADR closes
- [ADR-052](052-alknet-tty-wire-format-and-two-carriage.md) — the wire
format the cancel does not involve (the cleanup is server-internal)
- [ADR-053](053-ttybackend-trait-and-ttyhandle.md) — the `TtyBackend`
trait this contract is part of; the `exit_code` field the cleanup
wires into; the `TtyControl::signal()` path the cancel-cleanup path
is distinct from
- [ADR-054](054-local-tty-backend-sibling-crate.md) — the local backend
this ADR's reference mechanism (`ChildKiller`) is for
- [ADR-055](055-exit-code-on-control-chunk.md) — the happy-path
exit-chunk sequence (the cancel path bypasses it; no exit chunk is
sent on cancel — see `tty-adapter.md` §"Stream reset")
- `/workspace/alknet-tty-poc/src/local_pty.rs` — the POC's `LocalPty`
shape (the reference the contract generalizes; the POC's bare
`oneshot::Receiver<i32>` `exit_code` is the shape an implementer
must NOT copy without adding the kill guard)
- `portable-pty` 0.9 `ChildKiller` — the kill handle the local
backend's cancel-cleanup uses
- Spec: [crates/tty/tty-backend.md](../crates/tty/tty-backend.md),
[crates/tty/tty-adapter.md](../crates/tty/tty-adapter.md),
[crates/tty/tty-local.md](../crates/tty/tty-local.md)