fix(review-008 audit): adopted-entry drop guard; explicit-ALPN guards; review 009
Post-landing audit of the 0.7.1 -> 0.8.0 remediation diff: two hardening guards, one rejection-posture fix, two log/message corrections, and the deferred coverage debt filed as review 009. - RelayPlan owns the producer-leg ChannelManager and reclaims the adopted spoke channel_id via a Drop guard (replaces the pump handler's post-pump_bidi explicit reclaim). Closes the leak windows the pump's normal path cannot reach: the wrapper's establishment bound expiring after the adopt, and the pump handler's early-return arms (plan absent, downcast failure, try_unwrap failure, accept_bi failure). The send-half drop still EOFs the spoke leg via the mux pump's implicit-EOF sentinel, so the spoke-side cascade is unchanged. ADR-051 §6 documents the closed post-adopt window (the pre-adopt §6 window and the inside-adopt_channel cancellation point stay as documented). - rebuild_spec_for trims and rejects empty/whitespace channel_open_alpn strings — an empty explicit string previously overrode a sane name-derived ALPN. - op_name_is_standard_channel_open_shape applies the same empty-segment guard as the derivation: channels//sub no longer serializes boolean-only and then reconstructs unmarked (silent stub for a marked op); the explicit string rides instead. - reserved_reply_key_call_error interpolates RESERVED_REPLY_KEY; the establisher-bug log fires at warn! (programming error). - Regression tests: the plan drop guard, the empty-ALPN fallback, the empty-segment shape check (672 tests, 3 new). - CHANGELOG [Unreleased] entry for the audit fixes. - docs/reviews/009 — the audit's deferred test-coverage gaps (template failure arms, filtered/only, batch reserved key, wire failure path, golden pins, derivation edge shapes, builder overwrite semantics), each with the test to add and gates. Verification: cargo test 672 passed; clippy --all-targets -D warnings clean; fmt --check clean; doc --no-deps clean; wasm32 check clean.
This commit is contained in:
1 parent
50182d7298
commit
54c2a3f941
8 files changed
+416
-28
No files matched your search
@@ -214,6 +214,23 @@ relays. The end consumer never authenticates to the spoke directly.
|
||||
the window deterministically by severing the spoke leg and asserts
|
||||
the spoke-side reclaim + ledger decrement. A channel never outlives
|
||||
its leg lifetimes; never a leak past the connection's lifetime.
|
||||
- **Adopted-entry teardown on every plan path** — the hub's
|
||||
producer-leg entry (the `adopt_channel` state) is owned by the
|
||||
relay's `RelayPlan` via a `Drop` guard, so it reclaims whenever the
|
||||
plan dies: the normal pump completion, the pump handler's
|
||||
early-return arms (plan absent, downcast failure, `try_unwrap`
|
||||
failure, `accept_bi` failure), and the wrapper's establishment
|
||||
bound expiring after the adopt (the plan dropped before the pump
|
||||
ever spawns). Dropping the plan's send half also EOFs the spoke
|
||||
side through the mux pump's implicit-EOF sentinel (REQ-CH-01), so
|
||||
the spoke handler reclaims through the same cascade the normal
|
||||
path uses. This closes the adopt-then-timeout window the landing
|
||||
review surfaced (the §6 window above is the *pre-adopt* window;
|
||||
this is the *post-adopt* one, now closed by construction). The
|
||||
one remaining cancellation point *inside* `adopt_channel`'s own
|
||||
await is the same pre-existing client-side window
|
||||
(`ChannelClient::open_channel`'s adopt has it too) — bounded by
|
||||
the leg's transport-EOF `clear_all`, not by the plan.
|
||||
- **Multi-hop relays** — a chain of relays composes (each hop is a
|
||||
hub leg pair), with the compounding-bound note above. No special
|
||||
machinery.
|
||||
|
||||
@@ -0,0 +1,208 @@
|
||||
# Review 009 — Post-landing audit of the review-008 remediation (0.7.1 → 0.8.0)
|
||||
|
||||
## Status
|
||||
|
||||
Open — filed 2026-09-18 from the post-landing audit of the six commits
|
||||
`82ddddf..50182d7` (review 008's three units). Scope: correctness
|
||||
review of the full diff, test-coverage mapping, and the classic
|
||||
implementation-issue sweep. Verified against HEAD at filing (0.8.0 +
|
||||
the audit's inline hardening commit): 672 tests pass, clippy/fmt/doc
|
||||
clean, wasm check clean.
|
||||
|
||||
The audit's code findings were fixed inline at filing time (see the
|
||||
CHANGELOG [Unreleased] entry): the adopted-entry leak windows
|
||||
(audit F-1/F-2 — `RelayPlan` drop guard), the empty explicit
|
||||
`channel_open_alpn` (audit F-3), the `channels//sub` silent-stub
|
||||
shape (audit N-4), and the reserved-key message/log-level nits
|
||||
(audit N-1/N-2). ADR-051 §6 documents the closed post-adopt window.
|
||||
What remains from the audit is the **test-coverage debt** below —
|
||||
deliberately deferred to this review so the fix commit stayed small.
|
||||
|
||||
Findings continue the review numbering with prefix `C` (coverage).
|
||||
|
||||
## Scope
|
||||
|
||||
Test-coverage gaps in the 0.8.0 surface: the Unit 3 modules
|
||||
(`src/channels/relay.rs`, `src/channels/hub_leg.rs`) and the Unit 1/2
|
||||
diff surfaces (`src/channels/operations.rs`, `src/channels/client.rs`,
|
||||
`src/client/from_call.rs`, `src/registry/discovery.rs`). No production
|
||||
code changes are requested by this review unless a gap's write-up
|
||||
says otherwise; each finding names the test(s) to add and what they
|
||||
pin. None block a consumer — the e2e gates the review-008 plan pinned
|
||||
are all landed and passing; these gaps are the paths the gates never
|
||||
walk.
|
||||
|
||||
## C-1: `HubLegTemplate::install_hook` failure arms are loud-only-in-code
|
||||
|
||||
**Finding.** `src/channels/hub_leg.rs:232-275` — four install arms end
|
||||
the leg with only a `tracing::warn!`: generic channel ops registration
|
||||
failure, plain-bundle registration failure, relay-openable
|
||||
registration failure, bootstrap-discovery install failure. ADR-051 §6
|
||||
pins the posture as "loud, never a silent stub" and only the
|
||||
relay-openable arm is test-pinned
|
||||
(`hub_leg_tests.rs` `template_pub_typed_marked_spec_is_loud_at_install`).
|
||||
The other three are near-unreachable (generic ops and bootstrap
|
||||
discovery cannot realistically fail; plain bundles were already
|
||||
registered on the producing side) — but the ADR's posture claim rests
|
||||
on code that no test walks.
|
||||
|
||||
**Requested change.** Unit-test the three unpinned arms. The plain
|
||||
routes: (a) a plain bundle that fails registration — e.g. a duplicate
|
||||
name between a plain bundle and the generic channel ops — asserts the
|
||||
install task ends before `run_loop_single_stream`; (b) the same shape
|
||||
for a relay-openable failure is already pinned; (c) the
|
||||
generic-ops/bootstrap-discovery arms need an injectable failure seam
|
||||
if they are to be tested honestly — if that is disproportionate,
|
||||
instead demote the claim: document in ADR-051 §5 that only the
|
||||
relay-openable arm is test-pinned and the others are
|
||||
best-effort-loud, so the ADR and the code say the same thing.
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. The template's install task provably ends (channel 0 never
|
||||
dispatches) on each exercised failure arm.
|
||||
2. The ADR-051 §5/§6 text matches what is actually pinned.
|
||||
|
||||
## C-2: `HubLegImports::filtered` / `only` have no test
|
||||
|
||||
**Finding.** `src/channels/hub_leg.rs:91-111` — the per-consumer
|
||||
op-subset filter, the mechanism behind ADR-051 §4's "per-consumer ACL
|
||||
differentiation is a composition consequence" note, has no unit test.
|
||||
Trivial code, but it is exported pub API (`HubLegImports` is a crate
|
||||
export) and the filter story is load-bearing for the hub/spoke family.
|
||||
|
||||
**Requested change.** Unit tests: `from_bundles` splits by marker;
|
||||
`filtered`/`only` partition both halves; a `#[must_use]` misuse
|
||||
compiles with a warning (already enforced by the attribute — just
|
||||
exercise the two builders).
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. `only(&["channels/tunnel/direct"])` keeps the marked direct spec,
|
||||
drops everything else, plain bundle untouched by the marked list.
|
||||
2. An empty `only` stash installs a leg that serves only the generic
|
||||
ops + discovery (no re-exposed ops in `services/list`).
|
||||
|
||||
## C-3: Batch-form reserved reply key
|
||||
|
||||
**Finding.** `src/channels/operations.rs` — the reserved-key teardown
|
||||
test (`with_reply_field("channel_id", …)`, `operations.rs:~2606`)
|
||||
covers only the builder form. The wrapper's check inspects the merged
|
||||
map, so `with_reply_fields(map)` smuggling `channel_id` is covered by
|
||||
construction — but the batch path is the shape a hub's
|
||||
`Establishment::with_reply_fields(relay_map)` will actually use, and
|
||||
it is unasserted.
|
||||
|
||||
**Requested change.** One-line extension of the existing test (or a
|
||||
sibling): the batch form fails with reason `handler_error`, channel
|
||||
torn down, ledger decremented.
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. `Establishment::default().with_reply_fields(json_map_containing_channel_id)`
|
||||
→ `channel:open_failed` / `handler_error`; `count_for == 0` after.
|
||||
|
||||
## C-4: `open_channel_with_reply` wire failure path
|
||||
|
||||
**Finding.** `src/channels/client.rs:1654` — the new client API's e2e
|
||||
test covers the success path only. The failure path (a
|
||||
`channel:open_failed` resolving through `open_channel_with_reply`) is
|
||||
covered only indirectly (wrapper-level test + the generic
|
||||
`establisher_failure_resolves_typed_open_failed_on_the_client`).
|
||||
Low risk — the parse path is shared with `open_channel` — but the new
|
||||
pub API's error path has no direct pin.
|
||||
|
||||
**Requested change.** One e2e test against the existing
|
||||
establisher-always-fails harness: `open_channel_with_reply` resolves
|
||||
`Err(ChannelOpenError::CallFailed)` with the `channel:open_failed`
|
||||
code and details intact.
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. The full reply shape (`reason`, `message` in `details`) is
|
||||
assertable by the caller through the new API.
|
||||
|
||||
## C-5: Byte-identical claims are shape-pinned, not golden-pinned
|
||||
|
||||
**Finding.** Two "byte-identical" claims are pinned structurally
|
||||
(key-set equality), not by literal:
|
||||
|
||||
- `run_open_wrapper_without_reply_fields_is_byte_identical_to_pre_amendment`
|
||||
(`operations.rs:2553`) asserts `v == json!({"channel_id":
|
||||
v["channel_id"]})` — pins the exact key set but builds the expected
|
||||
from the actual (a wrong-typed `channel_id` value would pass).
|
||||
- The Unit-2 standard-shape wire payload
|
||||
(`spec_standard_shape_channel_open_stays_boolean_only`) pins the
|
||||
key *absence* of `channel_open_alpn` (the real pin, solid) but not
|
||||
the payload's full key set.
|
||||
|
||||
**Requested change.** Golden-pin both: assert serialized bytes or a
|
||||
literal `json!` against the actual for the no-fields reply, and a
|
||||
full-object comparison for the standard-shape payload. Cheap, and it
|
||||
converts "structurally equal" into "these exact bytes" for the two
|
||||
claims the ADRs advertise as wire-stable.
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. The no-fields open reply equals `json!({"channel_id": <exact
|
||||
literal>})` — not a self-referential compare.
|
||||
2. The standard-shape `services/schema` payload compares equal to a
|
||||
literal object including key order (serde_json default map).
|
||||
|
||||
## C-6: `derive_alpn_from_op_name` edge shapes unpinned
|
||||
|
||||
**Finding.** `src/client/from_call.rs:326-337` — the empty-segment
|
||||
guard (`segment.is_empty()`) and the bare-no-slash name have no unit
|
||||
test, and the 4-segment name behavior *changed* (pre-amendment:
|
||||
`None`; now: `Some("x/sub")`) without any test noticing. The audit
|
||||
closed the `channels//sub` serialization half (N-4); the derivation
|
||||
function's own edge-shape unit tests remain unlanded.
|
||||
|
||||
**Requested change.** Unit tests over the derivation directly:
|
||||
`"channels//sub"` → `None`; `"channels//direct"` → `None`;
|
||||
`"channels"` → `None`; `"channels/x/sub/extra"` → `Some("alk/x/sub")`
|
||||
(the deliberate strict-superset behavior, worth pinning as intended);
|
||||
`"channels/alk/tty/sub"` → `Some("alk/tty")` (the verbatim case).
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. All five shapes assert as above; the 4-segment case is annotated
|
||||
as a behavior change vs the pre-amendment derivation.
|
||||
|
||||
## C-7: Builder overwrite semantics unpinned
|
||||
|
||||
**Finding.** `src/channels/operations.rs:420-434` — two
|
||||
`with_reply_field("same-key", …)` calls silently last-win; the batch
|
||||
form extends (also last-win). Establisher-own concern, nit-level, but
|
||||
it is pub-API behavior a consumer will rely on or be confused by.
|
||||
|
||||
**Requested change.** Either pin last-win with a test (and one doc
|
||||
sentence on `with_reply_field`), or reject duplicates loudly at
|
||||
build time. Prefer pinning: rejection adds a failure mode with no
|
||||
consumer ask behind it.
|
||||
|
||||
**Verification gates:**
|
||||
|
||||
1. `.with_reply_field("k", v1).with_reply_field("k", v2)` →
|
||||
`reply_fields()["k"] == v2`.
|
||||
|
||||
## Non-goals (recorded to bound the review)
|
||||
|
||||
- **No wire-format or API changes** — everything above is tests plus
|
||||
at most doc text. The audit's code findings are already landed;
|
||||
they are not re-opened here.
|
||||
- **No new ADRs** — C-1's documentation route (if chosen) edits
|
||||
ADR-051 §5/§6 text only.
|
||||
- **alktunnels-side work** (the bind-first establisher, the listen-op
|
||||
spec) sequences after this review as planned; nothing here blocks
|
||||
it.
|
||||
|
||||
## References
|
||||
|
||||
- ADR-051 §6 (the post-adopt teardown bullet the drop guard landed;
|
||||
C-1 pins its assembly-arm siblings)
|
||||
- ADR-047 amendment 3 (C-6's derivation shapes), ADR-049 amendment 3
|
||||
(C-3/C-7's reply projection), ADR-047 (C-5's wire-stability claims)
|
||||
- CHANGELOG [Unreleased] — the audit's inline fixes this review
|
||||
complements
|
||||
- Review 008 — the remediation whose surface this review audits
|
||||
Reference in new issue
Block a user