From 671a779ce32f6c78da142e4b2511395208bb7a18 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Thu, 29 Oct 2026 17:59:54 +0000 Subject: [PATCH] =?UTF-8?q?docs(review):=20close=20the=20wave-4=20review?= =?UTF-8?q?=20gate=20=E2=80=94=20verdict=20green=20(review-wave-4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Gate reading recorded line by line: client.md (dial surface, op rows, acceptor, lifecycle primitives), ADR-003 symmetry, ADR-005/ OQ-05/OQ-07, bounded resources, tracing, smokes, docs honesty — all confirmed - Wave-3 gate booked items 1-4 verified discharged; item 5 verified still booked with its owners (nothing absorbed) - Inline fix: the accepted-open handoff counter now releases on completion or unwind (HandoffCapGuard panic-safety; a panicking assembly acceptor can no longer strand a held slot) + unit row - Inline fix: lib.rs crate-level wave-state doc sentence (stale over w4-lifecycle) - w4-reverse-listen / w4-lifecycle Summary placeholder lines filled Verification: cargo test (default / --all-features / --features tunnel) all green; clippy x2 -D warnings clean; cargo fmt --check clean; cargo doc --no-deps 0 warnings; taskgraph validate (35 tasks) --- src/client_accept.rs | 58 +++++-- src/client_handler.rs | 9 +- src/lib.rs | 6 +- tasks/implementation/review-wave-4.md | 200 +++++++++++++++++++++- tasks/implementation/w4-lifecycle.md | 12 +- tasks/implementation/w4-reverse-listen.md | 13 +- 6 files changed, 272 insertions(+), 26 deletions(-) diff --git a/src/client_accept.rs b/src/client_accept.rs index e26b8e1..0bdd9af 100644 --- a/src/client_accept.rs +++ b/src/client_accept.rs @@ -76,8 +76,10 @@ //! set the registry would have to serve). The per-connection //! **accepted-open handoff count** is the second dedicated counter //! against the same ceiling (admitted in the dial handler's open -//! hook, accepted-with-the-count-held, released when the acceptor's -//! `accept` call completes — no accepted-open ledger anywhere +//! hook, accepted-with-the-count-held, released when the handoff +//! task ends — the acceptor's `accept` completing *or the task +//! unwinding* ([`HandoffCapGuard`]: a panicking assembly acceptor +//! can never strand a held slot) — no accepted-open ledger anywhere //! else: the pump is the acceptor's from the handoff on, the //! channel's transport bounds are russh's windows, and the slot read //! is the arrival admission). @@ -217,9 +219,11 @@ impl std::fmt::Debug for ForwardedStreamlocal { /// The `forwarded-tcpip` acceptor — the registry's per-type slot /// trait. `accept` runs on the spawned handoff task (never the /// packet loop): the assembly's pump/home of the open lives inside -/// it; the crate releases its handoff counter when the call returns, -/// so the family shape (hand off to a queue, return) keeps the -/// counter's meaning "open accepted, handoff in flight". +/// it; the crate releases its handoff counter when the handoff task +/// ends — the call returning *or the task unwinding* +/// ([`HandoffCapGuard`]) — so the family shape (hand off to a queue, +/// return) keeps the counter's meaning "open accepted, handoff in +/// flight". #[async_trait] pub trait TcpipForwardedAcceptor: Send + Sync { /// Serves one accepted open. Never invoked for an unregistered @@ -341,13 +345,28 @@ impl std::fmt::Debug for AcceptorRegistry { // The server-initiated opens' handoff (the dial handler's routed paths) // --------------------------------------------------------------------------- +/// The handoff counter's panic-safe release — the accepted open's count +/// releases exactly at the handoff task's end, on *completion or +/// unwind*: a panicking assembly acceptor (or any panic in the handoff +/// body) can never strand a held slot past the task. Saturating under +/// the [`ChannelCap`] (never manufactures capacity). +struct HandoffCapGuard(Arc); + +impl Drop for HandoffCapGuard { + fn drop(&mut self) { + self.0.release(); + } +} + /// The accepted `forwarded-tcpip` open's handoff task — the /// spawn-and-return rule's client mirror (ADR-006 decision 4; the /// hook runs in the packet loop and returns fast): accepts the open on /// the wire, converts the channel to the family duplex, delivers the /// payload to the assembly's acceptor, and releases the handoff -/// counter when the acceptor's call completes (the recorded release -/// site — the count's meaning is "accepted, handoff in flight"). +/// counter when the handoff task ends — the acceptor's `accept` +/// completing *or the task unwinding* ([`HandoffCapGuard`]; the +/// recorded release site — the count's meaning is "accepted, handoff +/// in flight"). #[cfg_attr(feature = "tunnel", allow(clippy::too_many_arguments))] pub(crate) async fn handoff_forwarded_tcpip( acceptor: Arc, @@ -371,8 +390,9 @@ pub(crate) async fn handoff_forwarded_tcpip( stream: channel.into_stream(), }, }; + let cap_guard = HandoffCapGuard(open_cap); acceptor.accept(open).await; - open_cap.release(); + drop(cap_guard); observe::serving_path_ended("forwarded-tcpip", channel_number, None); } @@ -394,8 +414,9 @@ pub(crate) async fn handoff_forwarded_streamlocal( stream: channel.into_stream(), }, }; + let cap_guard = HandoffCapGuard(open_cap); acceptor.accept(open).await; - open_cap.release(); + drop(cap_guard); observe::serving_path_ended("forwarded-streamlocal", channel_number, None); } @@ -666,7 +687,7 @@ impl SshConnection { #[cfg(test)] mod tests { - use super::AcceptorHeld; + use super::{AcceptorHeld, HandoffCapGuard}; use crate::client_accept::{ AcceptorRegistry, ForwardedStreamlocal, ForwardedTcpip, StreamlocalForwardedAcceptor, TcpipForwardedAcceptor, @@ -766,4 +787,21 @@ mod tests { "an acceptor is already registered for this channel type" ); } + + /// - *Contract stamp:* the handoff counter's panic-safe release + /// (the bounded-bookkeeping cap relation) — the + /// [`HandoffCapGuard`] releases on unwind, so a panicking + /// assembly acceptor can never strand a held slot past the + /// handoff task. + #[test] + fn the_handoff_guard_releases_on_unwind() { + let cap = Arc::new(crate::channel_cap::ChannelCap::default()); + assert!(cap.admit(1)); + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _guard = HandoffCapGuard(Arc::clone(&cap)); + panic!("the assembly acceptor panics"); + })); + assert!(outcome.is_err(), "the panic rode through"); + assert_eq!(cap.active(), 0, "the held slot released on unwind"); + } } diff --git a/src/client_handler.rs b/src/client_handler.rs index 29314e8..cd29252 100644 --- a/src/client_handler.rs +++ b/src/client_handler.rs @@ -47,8 +47,10 @@ //! [`crate::client_accept`]): a dedicated per-connection counter — //! sharing the dial's `Limits::max_concurrent_channels` ceiling the //! server direction's per-session cap uses — admitted in the hook -//! before the accept, released when the acceptor's call completes in -//! the spawned task; a full counter declines the arrival cleanly +//! before the accept, released when the handoff task ends (the +//! acceptor's call completing *or the task unwinding* — +//! [`crate::client_accept`]'s `HandoffCapGuard` panic-safety); a full +//! counter declines the arrival cleanly //! (`cap-exceeded`). #[cfg(feature = "tunnel")] @@ -76,7 +78,8 @@ use crate::{DialError, fingerprint}; /// `w4-reverse-listen` surface, under `tunnel`): the acceptor registry /// the two forwarded hooks route through, plus the accepted-open /// handoff counter and its ceiling (the dial's -/// `Limits::max_concurrent_channels`). +/// `Limits::max_concurrent_channels`; released when the handoff task +/// ends — completion or unwind, `HandoffCapGuard`'s panic-safety). #[cfg(feature = "tunnel")] pub(crate) struct TunnelSurface { acceptors: Arc, diff --git a/src/lib.rs b/src/lib.rs index 0b58003..ba07091 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,9 +20,9 @@ //! //! Architecture: `docs/architecture/` (phase 1 — ADRs 001–006 accepted; //! the foundations, the server stack, the server-direction mappings, and -//! the client dial + op rows + the `-R` acceptor leg are landed through -//! wave 4; the lifecycle primitives, the validation suite, and the -//! release are later implementation waves). See also `docs/research/`, +//! the client direction — dial, op rows, the `-R` acceptor leg, and the +//! lifecycle primitives — are landed through wave 4; the validation +//! suite and the release are wave 5). See also `docs/research/`, //! `docs/plans/implementation.md`, and family ADR-003. mod adapter; diff --git a/tasks/implementation/review-wave-4.md b/tasks/implementation/review-wave-4.md index ce13558..0ab82b0 100644 --- a/tasks/implementation/review-wave-4.md +++ b/tasks/implementation/review-wave-4.md @@ -1,7 +1,7 @@ --- id: review-wave-4 name: Wave 4 gate — the client-direction review -status: pending +status: completed depends_on: [w4-dial, w4-op-rows, w4-reverse-listen, w4-lifecycle, w4-git-exit-close, w4-channel-not-held-rows, w4-tty-coverage, w4-router-collision] scope: narrow risk: low @@ -64,16 +64,16 @@ booked (remediation before wave 5 decomposes). ## Acceptance Criteria -- [ ] Every client.md/ADR surface above read and confirmed (or +- [x] Every client.md/ADR surface above read and confirmed (or corrected with the correction landed) -- [ ] The client-side per-build matrices verified through the server +- [x] The client-side per-build matrices verified through the server double; the acceptor contract verified (registered/unregistered/ unfeatured/late-install); the wave-3 booked items 1–4 verified discharged -- [ ] Full verification set green (`cargo test`, `--all-features`, +- [x] Full verification set green (`cargo test`, `--all-features`, clippy ×2 `-D warnings`, fmt, `cargo doc --no-deps`) -- [ ] `taskgraph validate` clean -- [ ] Verdict recorded; wave 5 decomposition unblocked or findings +- [x] `taskgraph validate` clean +- [x] Verdict recorded; wave 5 decomposition unblocked or findings booked ## References @@ -89,8 +89,192 @@ booked (remediation before wave 5 decomposes). ## Notes -> To be filled by implementation agent +### Gate reading (line by line, 2026-10-29; commits 9cf5bfb…dec644e) + +Commits verified against the decomposition: all eight tasks' hashes +match `docs/plans/implementation.md`'s decomposition record; every +task file carries its completion records and checked acceptance +criteria. + +- **client.md line-by-line** ✓ — read against the landed code: + §The dial surface (`client.rs` — the four-step fail-closed + sequence: decrypt-at-dial key load, the bounded russh `Config` + with the keepalive 15 s / 3 posture and the always-set wall clock, + the policy at `check_server_key`, the single publickey-only auth; + the sanctioned `username` `DialConfig` amendment recorded honestly + — a required parameter, no crate default, never identity, never + logged); §Op rows (`client_ops.rs` — every table row present: the + exec/pty/shell rows under `session`, the direct rows under + `tunnel`, the git exec row under `git` over the shared grammar's + construction arm, the sftp placeholder in every build; POC-3 + §D.3 mechanics pinned in code + rows — `exec()` not + `request_exec`, the `wait()`-drain Success/Failure, split-halves + wait-pumps for the exit-consuming rows and straight + `into_stream` for the exit-less, the fire-and-forget controls + sent from the live pump, no queue consumption before any + conversion); §The acceptor (`client_accept.rs` + the + `client_handler.rs` hooks — per-type fixed slots, refuse-never- + replace, arrival-time reads, decline-not-buffer, late-install + over the live connection, listen rows gated pre-wire, the two + dedicated counters against the one ceiling); §Lifecycle + primitives (`client_accept.rs` handles + `client.rs` §Lifecycle + posture — explicit cancels, `Disconnect::ByApplication` the only + clean path, drop-is-never-the-clean-path row-pinned both ways, + the no-reconnect/restore-policy boundary stated in module docs; + the crate owns no policy). client.md's landing blocks (acceptor/ + lifecycle/op-row shapes recorded at landing) are in the doc. +- **ADR-003 symmetry** ✓ — the features gate both directions + (`Cargo.toml` unchanged: `session`/`git`/`tunnel`/`sftp`, + `default = []`); the client exports ride the same gates + (`RemoteSession` under `session`, `TunnelChannel` + the acceptor + surface + `ReverseListener` under `tunnel`, `GitPackSession` + under `git`, the sftp row feature-free); the per-build matrices + are green (the default build's 16 binaries + per-feature/ + `--all-features`; the unfeatured builds' fail-closed wire rows + ride `tests/client_dial.rs`'s decline-all row — the + `client_handler.rs` hooks are cfg-declined in every build); + honest advertisement unchanged; x11/agent never-features + untouched on both directions. +- **ADR-005 / OQ-05 / OQ-07** ✓ — decrypt-at-dial with every + `KeySourceError` posture a no-dial outcome (`DialError::KeySource` + preserved per-variant; `error.rs` taxonomy rows); the policy seam + consumed at the hook (`client_handler.rs::check_server_key` — the + fingerprint rides the crate's single path, `PresentedHostKey` + carries host/port, the verdict passes through; `RejectAll` + connects to nothing, no accept-all built-in); the wave-1 + `Zeroizing` wrap posture intact (`key_source.rs`, untouched). +- **The wave-3 gate's booked items 1–4** ✓ — all discharged: + item 1 (`w4-git-exit-close`) — the split-halves seam duplex + (`ChannelHalveReader` over the owned close-free read half; no + drop path ever messages the wire) with the by-construction + property wire-pinned and the row falsifying the old shape + deterministic-red-under-revert; item 2 + (`w4-channel-not-held-rows`) — the guard's wire rows on all + three served rows per build, vocabulary-asserting; item 3 + (`w4-tty-coverage`) — the `channel_close`-fired cancel path + wire-witnessed, the Closed/Break no-exit-chunk shape row-pinned + (Break's frame classification unit-rowed, wire-unreachable, + recorded), the `write_all` no-deadline limit recorded in module + docs; item 4 (`w4-router-collision`) — the register-refuses + pre-check with the error-surface decision recorded honestly (the + wire-time decline disposition, not a taxonomy variant that would + kill the session). **Item 5 verified still booked with its + owners, not absorbed**: no `set_identity`/`take_call_connection` + anywhere in `src/` code (doc references only, recording the + no-detach posture + the upstream ask), and mapping.md's + correlation-miss event-vocabulary nuance records stand. +- **Bounded resources** ✓ (convention 9 read over every new + client serving/pumping path) — the acceptor registry bounded by + construction (two slots); the reverse-listen count and the + accepted-open handoff count are two dedicated `ChannelCap` + counters against the same `Limits::max_concurrent_channels` + ceiling (admit pre-wire/arrival, release at confirmed cancels / + refused paths / the handoff task's end); the pumps bounded + (64 KiB duplexes, the 16 KiB git pump buffer, the 1024-message + launch-drain cap, russh windows verbatim, the always-on wall + clock); the git row's command/repo gates answer pre-wire. One + gap found and fixed inline (below). +- **Tracing** ✓ — the client-side decline paths ride the uniform + vocabulary (`decline_open` with `no-acceptor` / `cap-exceeded` / + `never-feature`; the skeleton's decline rows event-pinned); the + safety posture held — structurally no username/fingerprint/secret + parameter exists on any event helper (the `&'static str` + vocabulary); the client op rows' typed surfaces carry outcomes + without events (the recorded asymmetry decision). +- **The smokes** ✓ — three recorded honestly: the alkssh-client ↔ + alkssh-server dial smoke (`tests/client_dial.rs`), the client-exec + ↔ server-session-rows smoke (`tests/client_ops.rs`, check-gated + on `sh`), the full-circle reverse-listen smoke + (`tests/tunnel_mapping.rs`); real-`sshd` e2e named wave-5 + territory explicitly in each task's records — nothing silently + green, nothing dropped. +- **Docs honesty** ✓ — module docs name each new surface and its + remaining wave-5 material (the fake-`sshd` posture, the + DialConfig amendment, the russh client hook shapes at this pin, + the acceptor-registry shape decision, the recorded wire facts — + the port-0-only mint, the drop/is-closed semantics); one stale + crate-level sentence fixed inline (below). + +### Minor issues found and fixed inline (this session) + +- **The accepted-open handoff counter's panic-unsafe release** (the + gate's one code fix): the counter released only after + `acceptor.accept(...)` returned — an assembly acceptor panicking + inside `accept` stranded one held slot for the connection's + lifetime (a bounded-resource leak path on the new cap). Fixed + with a panic-safe guard (`HandoffCapGuard` in + `src/client_accept.rs`): the release fires at the handoff task's + end, on completion *or unwind*; the recorded release-site wording + updated in the module docs, the trait docs, and + `client_handler.rs`'s two references; a unit row + (`the_handoff_guard_releases_on_unwind`) pins the unwind release. + No surface change, no behavior change on any happy path. +- **`lib.rs` crate-level wave-state doc** still said the lifecycle + primitives were "later implementation waves" (stale over + `w4-lifecycle`; the same staleness class the wave-3 gate fixed + inline). Corrected to the landed state. + +### Interpretations (flagged at decomposition) — all confirmed + +1. **The fail-closed client Handler skeleton in `w4-dial`, the + registry in `w4-reverse-listen`** — confirmed: the skeleton's + decline-all default is row-pinned in every build, and the + registry grows exactly the two forwarded hooks; the split kept + both surfaces reviewable. +2. **The fake-`sshd` client-direction vehicle in `tests/common`** — + confirmed: vanilla-parity, real loopback TCP, one dial per + instance, and the harness's `ForwardedAcceptor` vocabulary was + grown (not forked) into `client_accept.rs`'s real surface. +3. **No `DialConfig` course-correction anticipated** — confirmed, + with the one sanctioned exception recorded in-task (the required + `username` — the USERAUTH_REQUEST's structural requirement; the + amendment follows the `w3-seam-config` pattern and is honestly + documented). +4. **The hardening tasks ordered by single-writer needs** — + confirmed: `tests/git_mapping.rs` then `tests/tty_mapping.rs` + sequencing held, no cross-task file conflicts in the landed + history. + +### Verification (all green, 2026-10-29) + +`cargo test` (default build) · `cargo test --all-features` (all 16 +binaries green) · `cargo test --features tunnel` (the gate's +per-feature spot re-run, covering the edited acceptor surface + the +new guard row) · `cargo clippy --all-targets -- -D warnings` ✓ · +`cargo clippy --all-features --all-targets -- -D warnings` ✓ · +`cargo fmt --check` ✓ · `cargo doc --no-deps` 0 warnings · +`taskgraph validate` ✓ (35 tasks). ## Summary -> To be filled on completion \ No newline at end of file +**Verdict: green.** The client direction reads clean against +client.md/ADR-003/ADR-005 line by line: the four-step fail-closed +dial over the bounded config (the sanctioned `username` amendment +recorded), the op rows with the POC-3 mechanics pinned in code +(split-halves wait-pumps vs straight conversions; the typed-open +decline mirror family-typed; the sftp placeholder in every build), +the acceptor registry as the refuse-never-replace per-type-slot +gate with arrival-time declines and bounded two-counter bookkeeping, +and the lifecycle primitives with the drop/clean-close boundary +row-pinned both ways and the no-policy posture stated. All three +interpretations confirmed except as recorded (all favorable); the +wave-3 booked items 1–4 verified discharged and item 5 verified +still booked upstream — nothing absorbed. The full verification set +is green. Two minors fixed inline (the handoff counter's +panic-unsafe release — the one code fix, guard + unit row; a stale +crate-level wave-state doc sentence). No structural findings; wave +5 decomposition is unblocked and should absorb nothing beyond its +own plan (the decline matrix, real-tool e2e, fuzzing, docs, the +publish). + +## Summary + +**Verdict: green** (2026-10-29) — see Notes for the full gate +reading. All eight wave-4 tasks verified against the client.md/ADR +contract surfaces line by line; the wave-3 gate's booked items 1–4 +discharged, item 5 verified still booked upstream; the full +verification set + `taskgraph validate` green. Two inline fixes +landed with the gate: the accepted-open handoff counter's +panic-safe release (`HandoffCapGuard` — the gate's one code fix) +and the stale crate-level wave-state doc sentence in `lib.rs`. +Wave 5 decomposition is unblocked. \ No newline at end of file diff --git a/tasks/implementation/w4-lifecycle.md b/tasks/implementation/w4-lifecycle.md index 20b14cc..2d000d9 100644 --- a/tasks/implementation/w4-lifecycle.md +++ b/tasks/implementation/w4-lifecycle.md @@ -165,7 +165,17 @@ remain under `--all-features`, unchanged). ## Summary -> To be filled on completion +The OQ-12 primitives landed: the `ReverseListener` handles carrying +the actually-bound endpoint with the explicit wire-honest `cancel()` +(releases the listener count exactly once per confirmed cancel; the +stateless cancel forms compose into the same shared paths), the +`SshConnection` internals restructured behind the arc so handles +cancel against a live stack independently, the public surface +unchanged, the abrupt-drop/clean-close boundary row-pinned both ways +(drop is never the clean path; `close()` is), and the +primitives-not-policy posture stated in the module docs and +client.md's landing block. No reconnect/restore policy in the crate. +Details in the `Landed` block below. Landed (`feat(client)`: w4-lifecycle): diff --git a/tasks/implementation/w4-reverse-listen.md b/tasks/implementation/w4-reverse-listen.md index 4d984a6..7c4bec7 100644 --- a/tasks/implementation/w4-reverse-listen.md +++ b/tasks/implementation/w4-reverse-listen.md @@ -158,7 +158,18 @@ aborts explicitly now). ## Summary -> To be filled on completion +The `-R` leg landed complete: the forward-request rows +(`tcpip_forward`/`streamlocal_forward` + the cancel forms, the +runtime-Unix gate on the listen side, the port-0-only mint fact +surfaced), the acceptor registry as the real crate surface +(per-type fixed slots, refuse-never-replace, arrival-time reads, +decline-not-buffer, late-install), the forwarded hooks routed +through it with bounded two-counter bookkeeping, the data leg +handed to the assembly acceptor on spawned handoff tasks, and the +per-build contract rows through the fake-`sshd`'s real-socket `-R` +server half plus the full-circle smoke through the server +direction's fabric. Real-`sshd` `-R` stays wave-5 e2e, recorded. +Details in the `Landed` block below. Landed (`feat(client)`: w4-reverse-listen):