diff --git a/docs/architecture/crates/core/README.md b/docs/architecture/crates/core/README.md index 5c186ac..28ca49f 100644 --- a/docs/architecture/crates/core/README.md +++ b/docs/architecture/crates/core/README.md @@ -34,7 +34,7 @@ Core library for ALPN-based protocol dispatch. Every handler crate depends on al | [031](../../decisions/031-credentialstore-repo-trait.md) | CredentialStore Repo Trait | Second repo trait in core; `InMemoryCredentialStore` default adapter | | [033](../../decisions/033-storage-boundary-and-repo-adapter-pattern.md) | Storage Boundary and Repo/Adapter Pattern | Core defines traits + in-memory defaults; persistence adapters are separate crates | | [065](../../decisions/065-connection-from-stream-generic-single-stream.md) | `Connection::from_stream` — Generic Single-Stream Connections | `from_stream`/`from_bidi` accept any `AsyncRead + AsyncWrite`; yield-once `accept_bi` contract; unblocks TCP+TLS, SSH channels, WebTransport, wasm | -| [070](../../decisions/070-bidistreamsource-trait.md) | BidiStreamSource Trait — Open Connection for Extension | `Connection` holds `Box`; QUIC/iroh/stream wrap crate-private impls; downstream crates implement the trait to add connection shapes (channels, future transports) without editing core | +| [070](../../decisions/070-bidistreamsource-trait.md) | BidiStreamSource Trait — Open Connection for Extension | `Connection` holds `Box`; QUIC/iroh/stream wrap crate-private impls; `from_source` is the public constructor for downstream crates that implement the trait (channels, future transports) | ## Relevant Open Questions diff --git a/docs/architecture/crates/core/core-types.md b/docs/architecture/crates/core/core-types.md index 6055e8c..c051060 100644 --- a/docs/architecture/crates/core/core-types.md +++ b/docs/architecture/crates/core/core-types.md @@ -94,6 +94,11 @@ impl Connection { remote_addr: Option, ) -> Self; + /// Construct from a caller-supplied `BidiStreamSource` impl. The + /// extension point for downstream crates — implement the trait and + /// construct a `Connection` from it without editing core. See ADR-070. + pub fn from_source(source: impl BidiStreamSource, alpn: Vec) -> Self; + pub async fn accept_bi(&self) -> Result<(SendStream, RecvStream), StreamError>; pub async fn open_bi(&self) -> Result<(SendStream, RecvStream), StreamError>; pub fn remote_alpn(&self) -> &[u8]; @@ -186,11 +191,15 @@ indirection and are not the transport's concern. | `QuinnBidiStreamSource` | `Connection::from_quinn` / `from_quinn_with_alpn` (feature `quinn`) | many streams | | `IrohBidiStreamSource` | `Connection::from_iroh` (feature `iroh`) | many streams | | `StreamBidiStreamSource` | `Connection::from_stream` / `from_bidi` (no feature gate) | yield-once, then `ConnectionClosed`; `open_bi` returns `StreamClosed` | +| *(caller-supplied)* | `Connection::from_source` (no feature gate) | per the caller's `BidiStreamSource` impl | Downstream crates do not wrap `from_stream` to implement -`BidiStreamSource` — they implement the trait directly. `from_stream` is -the compatibility path for callers that want a `Connection` over a single -pre-split stream (the ADR-065 use case). +`BidiStreamSource` — they implement the trait directly and construct the +`Connection` via `from_source`. `from_stream` is the compatibility path for +callers that want a `Connection` over a single pre-split stream (the +ADR-065 use case); `from_source` is the extension path for callers that +implement `BidiStreamSource` themselves (the channels crate's +`ChannelBidiStreamSource`, a future transport, a test double). ## BiStream @@ -366,7 +375,7 @@ registration bundle. | ProtocolHandler receives Connection, not BiStream | [ADR-007](../../decisions/007-bistream-type-definition.md) | Handlers that need multiple streams (SSH, call) have direct access to the Connection | | BiStream is a trait | [ADR-007](../../decisions/007-bistream-type-definition.md) | WASM door preserved, test mocks possible | | `Connection::from_stream` — generic single-stream connections | [ADR-065](../../decisions/065-connection-from-stream-generic-single-stream.md) | `from_stream`/`from_bidi` accept any `AsyncRead + AsyncWrite`; yield-once `accept_bi` contract; unblocks TCP+TLS, SSH channels, WebTransport, wasm; QUIC variants feature-gated, `Stream` variant always available; `MockConnection`/`ConnectionKind::Mock` removed (tests use `from_stream` with `sink`/`empty`) | -| `BidiStreamSource` — open `Connection` for extension | [ADR-070](../../decisions/070-bidistreamsource-trait.md) | `Connection` holds `Box`; QUIC/iroh/stream wrap crate-private impls; downstream crates implement the trait to add connection shapes (channels, future transports) without editing core; public `Connection` API preserved; `close(code, reason)` kept on the trait (non-QUIC impls ignore the args — fixes the ADR-065 leftover clippy warning under `--no-default-features`) | +| `BidiStreamSource` — open `Connection` for extension | [ADR-070](../../decisions/070-bidistreamsource-trait.md) | `Connection` holds `Box`; QUIC/iroh/stream wrap crate-private impls; `from_source` is the public constructor for downstream crates that implement the trait (channels, future transports); `from_quinn`/`from_iroh`/`from_stream`/`from_bidi` preserved; `close(code, reason)` kept on the trait (non-QUIC impls ignore the args — fixes the ADR-065 leftover clippy warning under `--no-default-features`) | | HandlerError is non-fatal | [ADR-010](../../decisions/010-alpn-router-and-endpoint.md) | Handler errors close the connection, not the endpoint | | SendStream/RecvStream wrap quinn + iroh + generic streams | [ADR-010](../../decisions/010-alpn-router-and-endpoint.md), [ADR-065](../../decisions/065-connection-from-stream-generic-single-stream.md) | Internal enum dispatch for QUIC sources and the generic `Stream` variant | | Connection stores handler-resolved identity | OQ-11 (resolved) | `set_identity` via `OnceLock` — write-once-read-many; read by handler-side logging, not by the endpoint (C13 resolved) | diff --git a/docs/architecture/decisions/070-bidistreamsource-trait.md b/docs/architecture/decisions/070-bidistreamsource-trait.md index 336a4c8..a6f5b4b 100644 --- a/docs/architecture/decisions/070-bidistreamsource-trait.md +++ b/docs/architecture/decisions/070-bidistreamsource-trait.md @@ -140,11 +140,22 @@ delegates to `self.source`. `remote_alpn` reads `self.alpn` (unchanged). | `from_quinn` / `from_quinn_with_alpn` (feature `quinn`) | `QuinnBidiStreamSource` (crate-private) | | `from_iroh` (feature `iroh`) | `IrohBidiStreamSource` (crate-private) | | `from_stream` / `from_bidi` (no feature gate) | `StreamBidiStreamSource` (crate-private, yield-once) | +| `from_source` (no feature gate) | caller-supplied `impl BidiStreamSource` — the extension point for downstream crates | + +`from_source(source: impl BidiStreamSource, alpn: Vec) -> Self` is the +constructor that makes the trait the extension point. A downstream crate +implements `BidiStreamSource` (e.g. the channels crate's +`ChannelBidiStreamSource`) and constructs a `Connection` from it via +`from_source` — no core edit. The built-in impls (`QuinnBidiStreamSource`, +`IrohBidiStreamSource`, `StreamBidiStreamSource`) are crate-private; +`from_source` is the only path a downstream crate uses to wrap its own +impl. The `from_quinn` / `from_iroh` / `from_stream` / `from_bidi` +constructors are convenience wrappers for the three built-in impls. The `Stream`-backend implementations are crate-private; downstream crates do not implement `BidiStreamSource` by wrapping `from_stream`. They -implement the trait directly (channels: `ChannelBidiStreamSource`), or they -use a public constructor that already wraps an impl. +implement the trait directly (channels: `ChannelBidiStreamSource`) and +construct the `Connection` via `from_source`. ### `from_stream`-backed default impl is the compatibility path @@ -233,10 +244,10 @@ the args are optional for their transport. - `Connection` is open for extension. The channels crate implements `ChannelBidiStreamSource` in its own crate and constructs `Connection` - from it — no core edit. A future transport, test double, or relay - connection follows the same path. This is the structural payoff: the - connection type is no longer a closed enum that every new connection - shape must edit. + from it via `from_source` — no core edit. A future transport, test + double, or relay connection follows the same path. This is the + structural payoff: the connection type is no longer a closed enum that + every new connection shape must edit. - A channels connection is a first-class peer of QUIC: one `ChannelConnection` that yields N streams, rather than a bag of yield- once `Connection`s. The channels layer's API matches its actual shape. diff --git a/tasks/core/connection-from-source-constructor.md b/tasks/core/connection-from-source-constructor.md new file mode 100644 index 0000000..c4ff652 --- /dev/null +++ b/tasks/core/connection-from-source-constructor.md @@ -0,0 +1,153 @@ +--- +id: core/connection-from-source-constructor +name: "Add Connection::from_source constructor (ADR-070 gap — the public extension point)" +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +--- + +## Description + +ADR-070 made `Connection` hold `Box` so downstream +crates can add connection shapes without editing core. The trait exists, +the three built-in impls exist, but the **public constructor that lets a +downstream crate construct a `Connection` from its own `BidiStreamSource` +impl is missing.** The `source` field is private; there is no way for +the channels crate (or any future crate) to build a `Connection` from a +`ChannelBidiStreamSource` — the trait is currently unusable from outside +core. + +This is a gap in the ADR-070 implementation, not a new architectural +decision. ADR-070 says "the channels crate implements +`ChannelBidiStreamSource` in its own crate and constructs `Connection` +from it" (§Consequences), but the constructor for that path was never +added. The alknet-channels POC update surfaced this when it tried to use +the trait directly and found no `from_source`. + +### Current state (`crates/alknet-core/src/types.rs`) + +```rust +pub struct Connection { + source: Box, // private + alpn: Vec, + identity: OnceLock, +} + +impl Connection { + // from_quinn, from_quinn_with_alpn, from_iroh, from_stream, from_bidi + // — all wrap crate-private impls. No constructor takes a + // caller-supplied BidiStreamSource. +} +``` + +### Target state + +```rust +impl Connection { + /// Construct from a caller-supplied `BidiStreamSource` impl. The + /// extension point for downstream crates — implement the trait and + /// construct a `Connection` from it without editing core. See ADR-070. + pub fn from_source(source: impl BidiStreamSource, alpn: Vec) -> Self { + Self { + source: Box::new(source), + alpn, + identity: OnceLock::new(), + } + } +} +``` + +The signature takes `impl BidiStreamSource` (not `Box`) to match the ergonomic style of `from_stream` / +`from_bidi` (which take `impl AsyncWrite + Send + Unpin + 'static`). The +trait already requires `Send + Sync + 'static`, so no extra bounds are +needed. The `Box::new(source)` is an internal implementation detail. + +### Why not just use `from_stream` + +`from_stream` wraps a `StreamBidiStreamSource` (yield-once). A downstream +crate that implements `BidiStreamSource` for a multi-stream source (e.g. +channels' `ChannelBidiStreamSource` that yields one stream per channel) +cannot use `from_stream` — it needs `from_source` to wrap its own impl. +Using `from_stream` for a multi-stream source would give yield-once +behavior, which is wrong. This is exactly the gap the channels POC hit. + +### Test + +Add a test that constructs a `Connection` from a minimal custom +`BidiStreamSource` impl (a mock that yields a fixed `SendStream` / +`RecvStream` pair) and verifies `accept_bi` / `open_bi` / `remote_addr` / +`close` delegate correctly. The test impl does not need to be realistic — +it just needs to prove the public constructor works end-to-end with a +non-built-in `BidiStreamSource`. + +```rust +#[cfg(test)] +mod from_source_tests { + use super::*; + + struct MockSource { + stream: Option<(SendStream, RecvStream)>, + addr: Option, + } + + #[async_trait] + impl BidiStreamSource for MockSource { + async fn accept_bi(&self) -> Result<(SendStream, RecvStream), StreamError> { + // yield-once mock + } + async fn open_bi(&self) -> Result<(SendStream, RecvStream), StreamError> { + Err(StreamError::StreamClosed) + } + fn remote_addr(&self) -> Option { self.addr } + fn close(&self, _code: u32, _reason: &str) {} + } + + #[tokio::test] + async fn from_source_delegates_to_custom_impl() { + let conn = Connection::from_source( + MockSource { stream: None, addr: None }, + b"alknet/test".to_vec(), + ); + assert_eq!(conn.remote_alpn(), b"alknet/test"); + assert_eq!(conn.remote_addr(), None); + // accept_bi delegates to MockSource::accept_bi + } +} +``` + +(The test shape is illustrative — the implementer should make the mock +yield a real `SendStream`/`RecvStream` pair via `tokio::io::duplex` so +`accept_bi` returns a usable stream, and verify the stream round-trips.) + +## Acceptance Criteria + +- [ ] `Connection::from_source(source: impl BidiStreamSource, alpn: Vec) -> Self` added to `types.rs` +- [ ] `from_source` is `pub` and has no feature gate (always available, like `from_stream`) +- [ ] `from_source` boxes the source internally: `source: Box::new(source)` +- [ ] `from_source` initializes `alpn` and `identity: OnceLock::new()` (same as the other constructors) +- [ ] Unit test: construct a `Connection` from a custom `BidiStreamSource` impl via `from_source`, verify `remote_alpn` / `remote_addr` / `accept_bi` / `open_bi` / `close` all delegate to the custom impl +- [ ] Existing tests pass unchanged (`from_quinn` / `from_iroh` / `from_stream` paths are not affected) +- [ ] `cargo test -p alknet-core` succeeds (all feature combos) +- [ ] `cargo clippy -p alknet-core` succeeds with no warnings (all feature combos) +- [ ] `cargo fmt --check -p alknet-core` passes + +## References + +- docs/architecture/decisions/070-bidistreamsource-trait.md — ADR-070 (§Constructors, §Consequences — the "constructs `Connection` from it" clause that requires this constructor) +- docs/architecture/crates/core/core-types.md — `Connection` impl block (updated to include `from_source`), BidiStreamSource section, built-in implementations table + +## Notes + +> This is a gap fix, not a new feature. The ADR-070 design always intended +> the trait to be the extension point for downstream crates; the +> constructor is how a downstream crate *uses* that extension point. Without +> it, the trait is unusable from outside core — the channels crate cannot +> build a `Connection` from `ChannelBidiStreamSource`, which is the entire +> reason ADR-070 exists. The fix is one constructor (~6 lines) + one test. +> The risk is low: `from_source` is additive, the existing constructors +> are untouched, and the constructor's body is the same field-init pattern +> the other four constructors already use. \ No newline at end of file