docs: ADR-049 channel-open establishment phase; verify review 006
Verify review 006's findings against source at 88e3f5e (E-01..E-04
all confirmed; E-02 cost corrected — OperationSpec has no description
field, four touchpoints) and file three additional findings from the
same sweep (N-1 client error-type gap, N-2 write-only early-arrival
counter, N-3 pump-panic posture).
ADR-049 resolves E-01 + N-1: split-hook OpenEstablisher awaited
bounded by the open-op wrapper (restoring ADR-047 §3's "channel
plan" shape), teardown + typed channel:open_failed reply on
establishment failure, ChannelClient::open_channel typed error.
Review 006 gains the post-verification remediation plan and verdict
appendix.
Verification: cargo test (597 passed), cargo doc --no-deps clean.
This commit is contained in:
1 parent
88e3f5e9c3
commit
48ceeba55c
2 files changed
+465
-5
No files matched your search
@@ -2,12 +2,27 @@
|
||||
|
||||
## Status
|
||||
|
||||
Open — findings filed from the alktunnels Phase 0 research pass
|
||||
**Resolved-by-ADR (E-01, N-1) / Verified + planned (E-02, E-03, E-04,
|
||||
N-2).** Findings filed from the alktunnels Phase 0 research pass
|
||||
(2026-09-06). This is a design review, not a code-defect review: the
|
||||
establishment gap (E-01) is real, POC-observable, and load-bearing for
|
||||
the next downstream crate; the remaining findings are smaller
|
||||
mechanism/coverage gaps noticed in the same sweep.
|
||||
|
||||
**2026-09-06 verification + remediation pass.** All four findings were
|
||||
independently re-verified against source at tree `88e3f5e` (0.4.1 +
|
||||
this review's own commit) — verdicts CONFIRMED for E-01..E-04, with
|
||||
one correction to E-02's cost estimate (see the verification appendix
|
||||
at the bottom of this file). Three additional findings filed from the
|
||||
same sweep: **N-1** (client error-type gap — blocking for E-01's
|
||||
consumer visibility), **N-2** (dead counter), **N-3** (panic posture,
|
||||
pinned). **E-01 + N-1 are resolved by ADR-049**
|
||||
(`docs/architecture/decisions/049-channel-open-establishment-phase.md`
|
||||
— split-hook `OpenEstablisher`, bounded await, `channel:open_failed`
|
||||
typed error). The original remediation sketch below is superseded by
|
||||
the "Remediation plan (post-verification)" section; the original sketch
|
||||
is retained for the record.
|
||||
|
||||
Findings continue the review numbering with prefix `E` (001–005 used
|
||||
P/C/R/A/B/C/D/F/G — each review numbers independently).
|
||||
|
||||
@@ -22,10 +37,13 @@ forwarding prior art recorded in
|
||||
|
||||
Everything below was verified directly in source at tree `a22b2b8`
|
||||
(0.4.1 + the early-arrival park fix). No code changes were made in
|
||||
this repo by this review.
|
||||
this repo by this review. The 2026-09-06 re-verification pass
|
||||
(verification appendix) re-traced all findings at tree `88e3f5e` —
|
||||
no source changes touched the reviewed paths between the two trees
|
||||
(the only delta is this review's own commit).
|
||||
|
||||
```
|
||||
Verified against: alkcall a22b2b8 (0.4.1)
|
||||
Verified against: alkcall a22b2b8 (0.4.1); re-verified at 88e3f5e
|
||||
Reading list: src/channels/operations.rs (run_open_wrapper, OpenHandler,
|
||||
make_open_handler_once/stream/sink), src/channels/manager.rs
|
||||
(open_channel, teardown_channel, route_payload, early-arrival park),
|
||||
@@ -315,7 +333,7 @@ consumer.
|
||||
|
||||
---
|
||||
|
||||
# Remediation sketch (for the ADR discussion, not landed)
|
||||
# Remediation sketch (original, superseded — see "Remediation plan (post-verification)")
|
||||
|
||||
**Unit 1 — E-01 (the establishment phase).** Decided-shape ADR first
|
||||
(this is ADR-047 §3 contract territory — the `OpenHandler` type shape
|
||||
@@ -337,6 +355,63 @@ small ADR amendment: recommend (3) — subscribe for live resource sets
|
||||
|
||||
**Unit 3 — E-03/E-04.** E-03: log-or-comment; E-04: doc note. Trivial.
|
||||
|
||||
# Remediation plan (post-verification)
|
||||
|
||||
Firm ordering, decided 2026-09-06. E-01's ADR is **ADR-049**
|
||||
(`docs/architecture/decisions/049-channel-open-establishment-phase.md`)
|
||||
— decided shape: the **split hook** (`OpenEstablisher` awaited bounded
|
||||
by the wrapper, `OpenHandler` spawned unchanged after success), chosen
|
||||
over await-and-inspect because it preserves the `OpenHandler` type
|
||||
(backward compatible by construction), keeps establishment and pump
|
||||
lifecycles separate (SSH semantics: the dial is synchronous with the
|
||||
open reply), and restores ADR-047 §3's original "channel plan" wrapper
|
||||
shape. N-1 rides the same unit — without the client error-type fix,
|
||||
`channel:open_failed`'s typed reason is unreachable through the
|
||||
primary client path.
|
||||
|
||||
**Unit 1 — E-01 + N-1 (alkcall 0.5.0).** `OpenEstablisher` +
|
||||
`register_openable_with_establisher` (no-establisher = always-OK,
|
||||
existing registrations compile unchanged); wrapper flow: `check_open`
|
||||
→ `open_channel` → await establisher bounded (dispatch deadline when
|
||||
`Some`, else `ESTABLISHMENT_TIMEOUT` = 10s) → on failure:
|
||||
`teardown_channel` + ledger `take` + `policy.on_close` + reply
|
||||
`channel:open_failed` with `details: {reason, message}` (reason ∈
|
||||
`dial_failed` / `unknown_resource` / `resource_shortage` /
|
||||
`handler_error` / `timeout`); on success: spawn pumps,
|
||||
`set_handler_task`, reply `{channel_id}`. `ChannelClient::open_channel`
|
||||
returns a typed error carrying the `CallError`. Concurrency is safe:
|
||||
the dispatcher spawns Once invocations as independent tasks
|
||||
(`dispatch.rs` `spawn_once_dispatch`), so an awaited establisher does
|
||||
not head-of-line-block channel 0. Version 0.5.0 (the `open_channel`
|
||||
error-type change is semver-relevant).
|
||||
|
||||
**Unit 2 — E-02 (ride the 0.5.0 release).** Additive
|
||||
`description: Option<String>` on `OperationSpec` — note this is four
|
||||
touchpoints, not one (see the verification appendix's E-02
|
||||
correction): struct field + `spec_to_json_pub` emit +
|
||||
`rebuild_spec_for` parse + the `services/list` output-schema doc.
|
||||
`services/list` emits it when set. OQ-40 stays deferred but gains a
|
||||
"load-bearing for alktunnels discovery UI" note; the
|
||||
`channel/resources/subscribe` half stays deferred (alktunnels v1 uses
|
||||
config-known op names).
|
||||
|
||||
**Unit 3 — E-03/E-04/N-2 (trivial batch, same PR series as Unit 1).**
|
||||
E-03: debug log (or pinning comment) on the discarded `UnknownChannel`
|
||||
at the wrapper's teardown discard. E-04: doc note on `EARLY_ARRIVAL_CAP`
|
||||
(the 64-parked-chunks observable bound) for tunnel-crate-facing
|
||||
consumers. N-2: expose an accessor for `early_arrival_count` or remove
|
||||
the write-only counter.
|
||||
|
||||
**Sequencing:** ADR-049 (landed) → alkcall 0.5.0 (Units 1+2+3) →
|
||||
alktty migration (channels-path semantic failures move into an
|
||||
establisher; the direct-ALPN in-band error frame is retained — two
|
||||
transports, two contracts) → alkhttp mechanical pass
|
||||
(`OpenableAlpn.establisher: Option<...>`, default `None`). alktunnels
|
||||
Phase 1 blocks only on the ADR decision (landed), not the
|
||||
implementation: its OQ-TN-09 direction ("self-contained control frame")
|
||||
shrinks to *post-establishment* control only once `channel:open_failed`
|
||||
exists.
|
||||
|
||||
## Verification gates for the E-01 remediation
|
||||
|
||||
- A channels end-to-end test: producer handler whose establisher fails
|
||||
@@ -352,6 +427,71 @@ small ADR amendment: recommend (3) — subscribe for live resource sets
|
||||
`open_via_channels_surfaces_negotiation_rejected` scenario resolves
|
||||
via call error (or retained error-frame — per the ADR's chosen
|
||||
compat shape) unchanged in behavior.
|
||||
- (Added post-verification) A compat gate: a no-establisher
|
||||
registration behaves exactly as today — `{channel_id}` reply,
|
||||
handler spawned, teardown on handler exit.
|
||||
|
||||
## Verification appendix (2026-09-06 re-verification pass)
|
||||
|
||||
All findings re-verified by independent source trace at tree `88e3f5e`.
|
||||
Verdicts, corrections, and the additional findings:
|
||||
|
||||
- **E-01 CONFIRMED — and strengthened.** The wrapper's
|
||||
spawn-then-reply flow is as described
|
||||
(`src/channels/operations.rs` `run_open_wrapper`: handler spawned,
|
||||
reply written before the handler performs any work). New supporting
|
||||
evidence the original pass missed: **ADR-047 §3's decision text**
|
||||
describes the ALPN open handler as "validate params, consult
|
||||
ownership, prepare the backend, return a 'channel plan'" — an
|
||||
awaited preparation the wrapper consults before replying. The
|
||||
implemented `OpenHandler` (`JoinHandle<()>`) collapsed that phase
|
||||
into a fire-and-forget spawn. E-01 is therefore **drift from
|
||||
ADR-047 §3's own wrapper shape**, not merely a missing convenience —
|
||||
the remediation restores the decided design, which lowers the ADR's
|
||||
contention cost. Also verified: the serving loop spawns Once
|
||||
invocations as independent tasks, so an awaited establishment phase
|
||||
does not head-of-line-block other calls on channel 0 (a fix-feasibility
|
||||
question the original pass did not address).
|
||||
- **E-02 CONFIRMED — one correction.** `services_list_handler` emits
|
||||
`{name, namespace, op_type}` only, as described. But the "cheap,
|
||||
additive `description` field" framing understates the work:
|
||||
`OperationSpec` has **no `description` field at all** (the
|
||||
`description` in `spec.rs` is on `ErrorDefinition`). The change is
|
||||
four touchpoints: struct field, `spec_to_json_pub` emit,
|
||||
`rebuild_spec_for` parse, and the `services/list` output-schema doc.
|
||||
Still small; still [minor].
|
||||
- **E-03 CONFIRMED.** Both teardown paths gate on the atomic
|
||||
`opener_ledger().take(id)`; the race loser gets `UnknownChannel` /
|
||||
`take → None` — no double-decrement. The `let _ =` discard on the
|
||||
wrapper's `teardown_channel` result is as described. Also traced:
|
||||
the `set_handler_task` failure path still runs the wrapper task to
|
||||
completion (self-teardown), so no leak in that arm either. The
|
||||
log-or-comment remedy stands.
|
||||
- **E-04 CONFIRMED.** `EARLY_ARRIVAL_CAP = 64`, per-channel, FIFO,
|
||||
drop-past-cap with debug log + `dropped_unknown_chunks` counter.
|
||||
Doc-note-only remedy stands.
|
||||
- **N-1 [minor today, blocking for E-01's consumer visibility] —
|
||||
`ChannelClient::open_channel` erases the typed error.**
|
||||
(`src/channels/client.rs`) It flattens the `CallError` into a
|
||||
`String` via `format!("open op failed: {e:?}")`. Even once the
|
||||
wrapper replies `channel:open_failed` with reason details, a
|
||||
consumer cannot branch on the reason — the error type destroys it.
|
||||
E-01's verification gate ("consumer's `open_channel` resolves `Err`
|
||||
with `channel:open_failed` + reason details") is unreachable without
|
||||
changing this error type, so the fix is in scope for the E-01 unit
|
||||
(ADR-049 §4), not deferred.
|
||||
- **N-2 [trivial] — `early_arrival_count` is write-only.** Incremented
|
||||
in `park_early_arrival`, never read anywhere (no accessor; only
|
||||
`dropped_unknown_chunks` is exposed). Either expose an accessor
|
||||
(observability for the E-04 bound) or remove the counter.
|
||||
- **N-3 [observation, pinned by ADR-049 §6] — panicked pump handlers
|
||||
are EOF-shaped by design.** The wrapper's teardown task swallows the
|
||||
pump handler's `JoinError` (`let _ = raw_task.await`). A panicked
|
||||
handler = phantom channel + instant EOF, indistinguishable from a
|
||||
clean short-lived channel. With the establisher split, the pump-phase
|
||||
panic stays in this category (correct — there is no mid-stream error
|
||||
channel by design; establishment errors are the only kind that
|
||||
belong in the open reply). ADR-049 §6 pins this posture; no change.
|
||||
|
||||
## References
|
||||
|
||||
@@ -374,4 +514,8 @@ small ADR amendment: recommend (3) — subscribe for live resource sets
|
||||
findings-ledger.md`) — findings here follow the same spirit
|
||||
(downstream-discovered, filed for upstream action); E-01..E-04 are
|
||||
alktunnels-discovered but numbered in alkcall's review series since
|
||||
they are alkcall findings.
|
||||
they are alkcall findings.
|
||||
- **ADR-049** (`docs/architecture/decisions/049-channel-open-
|
||||
establishment-phase.md`) — the E-01/N-1 resolution (split-hook
|
||||
`OpenEstablisher`, bounded establishment, `channel:open_failed`
|
||||
typed error, client error-type fix).
|
||||
Reference in new issue
Block a user