From 5fa2fa9d730ba86ab24738af3ef6c747095265fd Mon Sep 17 00:00:00 2001 From: Loretta Date: Tue, 28 Apr 2026 14:18:27 +0200 Subject: [PATCH] [LOADED_DOCS: 2 files, no new loads] Add ADR-0003: AcBinary streaming receive unification - Add cross-cutting ADR-0003 for AsyncPipeReaderInput and transport-agnostic streaming helpers (NamedPipe, FileStream) - Update BINARY and SIGNALR_BINARY_PROTOCOL docs to reference ADR-0003 - Add migration TODOs for each ADR-0003 step with acceptance criteria - Add project ADR-0001 (binary projection serialization) and index - Clarify buffer sizing and ADR refs in SegmentBufferReader docs - Migrate JWT key/token log issues from LOGGING to new AUTH topic per ADR - Update ADR template and improve doc formatting throughout --- AyCode.Core.sln | 1 + .../Binaries/SegmentBufferReader.cs | 4 +- .../Binaries/SegmentBufferReaderInput.cs | 10 + AyCode.Core/docs/BINARY/BINARY_TODO.md | 63 ++++ AyCode.Core/docs/BINARY/README.md | 4 + AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md | 2 + AyCode.Core/docs/adr/0000-TEMPLATE.md | 5 + .../0001-binary-projection-serialization.md | 91 +++++ AyCode.Core/docs/adr/README.md | 19 ++ .../docs/SIGNALR_BINARY_PROTOCOL/README.md | 1 + .../SIGNALR_BINARY_PROTOCOL_TODO.md | 13 + ...acbinary-decorator-feature-stack-design.md | 1 + docs/AUTH/AUTH_ISSUES.md | 78 ++++- ...acbinary-streaming-receive-architecture.md | 319 ++++++++++++++++++ docs/adr/README.md | 7 +- 15 files changed, 612 insertions(+), 6 deletions(-) create mode 100644 AyCode.Core/docs/adr/0000-TEMPLATE.md create mode 100644 AyCode.Core/docs/adr/0001-binary-projection-serialization.md create mode 100644 AyCode.Core/docs/adr/README.md create mode 100644 docs/adr/0003-acbinary-streaming-receive-architecture.md diff --git a/AyCode.Core.sln b/AyCode.Core.sln index a31383b..8c89ca2 100644 --- a/AyCode.Core.sln +++ b/AyCode.Core.sln @@ -68,6 +68,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "adr", "adr", "{8D1B72D8-714 ProjectSection(SolutionItems) = preProject docs\adr\0000-TEMPLATE.md = docs\adr\0000-TEMPLATE.md docs\adr\0001-user-bearer-token-flow.md = docs\adr\0001-user-bearer-token-flow.md + docs\adr\0003-acbinary-streaming-receive-architecture.md = docs\adr\0003-acbinary-streaming-receive-architecture.md docs\adr\README.md = docs\adr\README.md EndProjectSection EndProject diff --git a/AyCode.Core/Serializers/Binaries/SegmentBufferReader.cs b/AyCode.Core/Serializers/Binaries/SegmentBufferReader.cs index d78f3b7..3963d76 100644 --- a/AyCode.Core/Serializers/Binaries/SegmentBufferReader.cs +++ b/AyCode.Core/Serializers/Binaries/SegmentBufferReader.cs @@ -48,7 +48,9 @@ public sealed class SegmentBufferReader : IDisposable /// /// Creates a new SegmentBufferReader with the specified initial capacity. - /// Typical value: chunkSize * 2 (e.g. 8192 for 4096-byte chunks). + /// Recommended: options.BufferWriterChunkSize * 2 (e.g. 8 KB for the SignalR-context 4 KB chunk size, + /// 128 KB for the standalone 64 KB default). See class remarks + /// for the full sizing rationale (two-chunks-worth headroom + reset-to-0 cycling). /// /// Initial buffer size. Rounded up by ArrayPool. /// Optional logger for diagnostic output (Debug level). Only emits in DEBUG builds. diff --git a/AyCode.Core/Serializers/Binaries/SegmentBufferReaderInput.cs b/AyCode.Core/Serializers/Binaries/SegmentBufferReaderInput.cs index b42fe7b..1763ed8 100644 --- a/AyCode.Core/Serializers/Binaries/SegmentBufferReaderInput.cs +++ b/AyCode.Core/Serializers/Binaries/SegmentBufferReaderInput.cs @@ -20,6 +20,16 @@ namespace AyCode.Core.Serializers.Binaries; /// /// Position reset: when the producer detects readPos == writePos (all consumed), /// it resets both to 0. After waking from Wait, this input re-reads the adjusted positions. +/// +/// +/// Recommended initialCapacity: +/// options.BufferWriterChunkSize * 2 (typically 8 KB for the SignalR-context 4 KB chunk size, +/// 128 KB for the standalone 64 KB default). Sized to fit two chunks worth of in-flight bytes — +/// enough headroom for the producer to write the next chunk while the consumer is reading the +/// previous, with reset-to-0 cycling reusing the same buffer for the message's lifetime regardless +/// of total payload size. Larger values waste memory; smaller values trigger occasional grows +/// under burst-write conditions. +/// /// public struct SegmentBufferReaderInput : IBinaryInputBase { diff --git a/AyCode.Core/docs/BINARY/BINARY_TODO.md b/AyCode.Core/docs/BINARY/BINARY_TODO.md index c0eb34d..8d63b79 100644 --- a/AyCode.Core/docs/BINARY/BINARY_TODO.md +++ b/AyCode.Core/docs/BINARY/BINARY_TODO.md @@ -96,3 +96,66 @@ Define a serializer-native ignore attribute (working name `[BinaryIgnore]`; fina - Both native attribute and `[JsonIgnore]` recognized during a transitional period; native attribute takes precedence on conflict. - `[JsonIgnore]` recognition flagged for removal in a future major version (track in a follow-up cleanup TODO once consumer projects have migrated). - No new code dependency on Newtonsoft.Json for property-exclusion logic. + +## ACCORE-BIN-T-Y6R2: Implement projection serialization phase 1 (runtime path) +**Priority:** P1 · **Type:** Feature · **Related:** `../adr/0001-binary-projection-serialization.md` (canonical) + +Implement the phase 1 runtime path of source→target projection serialization per ADR 0001. See the ADR for full context, decision rationale, alternatives, consequences, and acceptance criteria. + +**Sibling rebrand-prep TODOs:** `ACCORE-BIN-T-Z3K8` (IId migration), `ACCORE-BIN-T-N7V1` (JsonIgnore replacement). + +## ACCORE-BIN-T-D6H4: Create `AsyncPipeReaderInput` class (Step 1 of ADR-0003) +**Priority:** P1 · **Type:** Refactor · **Related ADR:** [`docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) Step 1 + +Add new `sealed class AsyncPipeReaderInput : IBinaryInputBase, IDisposable` in `AyCode.Core/Serializers/Binaries/AsyncPipeReaderInput.cs`. Self-contained sliding-window buffer (`byte[]` + `_writePos` + `_readPos` + `_completed` + `ManualResetEventSlim`) with reset-to-0 cycling preserved verbatim from today's `SegmentBufferReader`. Producer API: `Feed(ReadOnlySpan)`, `Complete()`. Consumer API (IBinaryInputBase): `Initialize` / `TryAdvanceSegment` / `Release`. + +Existing `SegmentBufferReader.cs` and `SegmentBufferReaderInput.cs` remain unchanged in this step — they keep serving the SignalR `AcBinaryHubProtocol.TryParseChunkData` path. Migration to the new class is in Step 6 (`ACCORE-SBP-T-G7T2`). + +**Naming rationale:** `AsyncPipeReaderInput` mirrors the existing send-side `AsyncPipeWriterOutput`. The `Async` prefix follows .NET BCL convention for type-level naming (`AsyncEnumerable`, `IAsyncDisposable`, `AsyncLocal`). + +**Acceptance:** +- New class compiles; isolated unit tests cover `Feed` / `TryAdvanceSegment` / `Complete` / `Dispose` contracts (incl. producer-consumer concurrency, missed-signal double-check, grow-buffer handoff race). +- Existing SignalR tests continue to pass on the unchanged `SegmentBufferReader` path (no behavioral regression). + +## ACCORE-BIN-T-M2K1: Add `AsyncPipeReaderInput.DrainFromAsync` extension (Step 2 of ADR-0003) +**Priority:** P1 · **Type:** Feature · **Related ADR:** [`docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) Step 2 · **Depends on:** `ACCORE-BIN-T-D6H4` + +Add `public static async Task DrainFromAsync(this AsyncPipeReaderInput input, PipeReader reader, CancellationToken ct)` in `AyCode.Core/Serializers/Binaries/AsyncPipeReaderInputExtensions.cs` (NEW file). Pulls from a `System.IO.Pipelines.PipeReader` and feeds the input via repeated `Feed(span)` calls; calls `Complete()` at end-of-stream. + +Separate file (not a method on the class) so the core `AsyncPipeReaderInput.cs` does not import `System.IO.Pipelines` in its primary contract — the pull-mode is opt-in at use-sites. + +**Acceptance:** +- Extension drains an in-memory `Pipe` end-to-end in a unit test (write some bytes → DrainFromAsync → assert AsyncPipeReaderInput state). +- `Complete()` correctly invoked at end-of-stream (`result.IsCompleted`). + +## ACCORE-BIN-T-V7C9: Replace misleading parallel test with real parallel pipeline test (Step 3 of ADR-0003) +**Priority:** P1 · **Type:** Test · **Related ADR:** [`docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) Step 3 · **Depends on:** `ACCORE-BIN-T-M2K1` + +The current `AcBinarySerializerPipeParallelTests.cs` is misleading — it does not actually exercise serializer↔deserializer parallelism (single-threaded in practice). Rewrite to drive a producer thread (serializer) and a consumer thread (deserializer) through an in-memory `Pipe`, with `AsyncPipeReaderInput.DrainFromAsync` on the receive side. Measure ser+deser overlap and verify the ADR-0003 claimed ~1 µs / MB perf delta vs today's struct-based path. + +**Acceptance:** +- Test passes consistently on Windows + Linux CI. +- Measured perf delta documented in test output / commit message. +- Test serves as regression guard for future receive-side changes (no silent perf-cliff regression goes undetected). + +## ACCORE-BIN-T-A3T8: Add NamedPipe helpers — `SerializeToNamedPipeAsync` / `DeserializeFromNamedPipeAsync` (Step 4 of ADR-0003) +**Priority:** P1 · **Type:** Feature · **Related ADR:** [`docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) Step 4 · **Depends on:** `ACCORE-BIN-T-V7C9` + +Add static extension methods on `AcBinarySerializerOptions` for full NamedPipe IPC lifecycle (one-shot send / receive). New file `AyCode.Core/Serializers/Binaries/AcBinarySerializerNamedPipeExtensions.cs`. Send: `NamedPipeServerStream` → `PipeWriter.Create(stream)` → `AsyncPipeWriterOutput`. Receive: `NamedPipeClientStream` → `PipeReader.Create(stream)` → `AsyncPipeReaderInput.DrainFromAsync`. + +Cross-platform: Windows + Linux (Unix-domain-socket via NamedPipe BCL API). WASM throws `PlatformNotSupportedException` per BCL contract. + +**Acceptance:** +- Cross-platform integration test: roundtrip a complex object graph through a NamedPipe; assert structural equality. +- WASM build does not link these helpers (or throws clear PNS at runtime if invoked). + +## ACCORE-BIN-T-B5Y6: Add FileStream helpers — `SerializeToFileStreamAsync` / `DeserializeFromFileStreamAsync` (Step 5 of ADR-0003) +**Priority:** P1 · **Type:** Feature · **Related ADR:** [`docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) Step 5 · **Depends on:** `ACCORE-BIN-T-A3T8` + +Add static extension methods on `AcBinarySerializerOptions` for streaming file I/O. New file `AyCode.Core/Serializers/Binaries/AcBinarySerializerFileStreamExtensions.cs`. Send: `FileStream.Create(path)` → `PipeWriter.Create(fileStream)` → `AsyncPipeWriterOutput`. Receive: `FileStream.OpenRead(path)` → `PipeReader.Create(fileStream)` → `AsyncPipeReaderInput.DrainFromAsync`. + +**Critical streaming-doctrine invariant:** peak buffer memory bounded by `BufferWriterChunkSize × 2` (~8 KB at default), regardless of file size. **NOT file-size-aware** — do not pre-allocate to file size (would defeat streaming and break zerocopy / zeroalloc). + +**Acceptance:** +- Large-file roundtrip test (≥ 100 MB) passes with memory profiler showing peak buffer ≤ 16 KB throughout. +- Full structural equality of round-tripped object. diff --git a/AyCode.Core/docs/BINARY/README.md b/AyCode.Core/docs/BINARY/README.md index 63775fa..a22180e 100644 --- a/AyCode.Core/docs/BINARY/README.md +++ b/AyCode.Core/docs/BINARY/README.md @@ -22,3 +22,7 @@ For a first read-through, start with [`BINARY_FEATURES.md`](BINARY_FEATURES.md) - **Serialization overview** (Toon vs AcBinary vs AcJson, shared infrastructure): `../../Serializers/README.md` - **SignalR binary transport** (uses this serializer): `../../AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md` - **Glossary terms**: `../../../docs/GLOSSARY.md` + +## Related ADRs + +- [`AyCode.Core/docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) — *AcBinary streaming receive — AsyncPipeReaderInput unified primitive and transport-agnostic helpers* (Status: Proposed (2026-04-27)). Repo-level cross-cutting ADR establishing the receive-side streaming architecture and transport-agnostic helpers (NamedPipe + FileStream) for this serializer. `AsyncPipeReaderInput` (sealed class) consolidates today's `SegmentBufferReader` + `SegmentBufferReaderInput` pair into a single self-contained primitive; the `Async`-prefixed naming mirrors the existing `AsyncPipeWriterOutput` send-side primitive. Implementation tracked across `ACCORE-BIN-T-D6H4` / `M2K1` / `V7C9` / `A3T8` / `B5Y6` (Steps 1–5) in `BINARY_TODO.md` and `ACCORE-SBP-T-G7T2` (Step 6) in `AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md`. diff --git a/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md b/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md index 60b9cfa..1bc7d59 100644 --- a/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md +++ b/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md @@ -205,6 +205,7 @@ Remove the `GlobalLogger.Detail($"Key: ...")` line entirely. The signing key mus **Caveat:** DEBUG builds still write the key to log. CI/CD must build with `-c Release` for production deployments. The inline `// SECURITY:` comment in the code warns future maintainers. ### Related +- **Superseded by:** [`ACCORE-AUTH-I-B3X5`](../../../docs/AUTH/AUTH_ISSUES.md#accore-auth-i-b3x5) — entry relocated to AUTH topic per ADR-0001 "Status migration on AUTH topic creation" follow-up. AUTH is now the canonical home for current discussion; this entry preserved per append-only governance (`TOPIC_CODES.md` Status conventions). - **Reference:** ADR [`0001-user-bearer-token-flow.md`](../../../docs/adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences. - `ACCORE-LOG-I-K1Z7` (sibling — access token also leaked, same file, same code path) - `LOGGING/README.md` may need a new "Never log secrets" guideline section (separate `_TODO.md` candidate) @@ -241,6 +242,7 @@ Remove the `GlobalLogger.Detail($"AccesToken: ...")` line entirely. If issuance **Caveat:** Same DEBUG-build leak constraint as `ACCORE-LOG-I-P5W3`. CI/CD must build with `-c Release` for production. ### Related +- **Superseded by:** [`ACCORE-AUTH-I-T8N2`](../../../docs/AUTH/AUTH_ISSUES.md#accore-auth-i-t8n2) — entry relocated to AUTH topic per ADR-0001 "Status migration on AUTH topic creation" follow-up. AUTH is now the canonical home for current discussion; this entry preserved per append-only governance (`TOPIC_CODES.md` Status conventions). - **Reference:** ADR [`0001-user-bearer-token-flow.md`](../../../docs/adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences. - `ACCORE-LOG-I-P5W3` (sibling — JWT signing key also leaked, same file) - Same `LOGGING/README.md` "Never log secrets" guideline gap as ACCORE-LOG-I-P5W3 diff --git a/AyCode.Core/docs/adr/0000-TEMPLATE.md b/AyCode.Core/docs/adr/0000-TEMPLATE.md new file mode 100644 index 0000000..bb99a4c --- /dev/null +++ b/AyCode.Core/docs/adr/0000-TEMPLATE.md @@ -0,0 +1,5 @@ +# ADR Template + +The canonical template lives at [`AyCode.Core/.github/skills/adr-author/references/ADR_TEMPLATE.md`](../../../.github/skills/adr-author/references/ADR_TEMPLATE.md) — that's what the `adr-author` skill copies when authoring a new ADR. + +This file exists only as a placeholder so `docs/adr/` numbering starts visibly at `0001`. Don't edit this file; edit the canonical template if the format needs to change. diff --git a/AyCode.Core/docs/adr/0001-binary-projection-serialization.md b/AyCode.Core/docs/adr/0001-binary-projection-serialization.md new file mode 100644 index 0000000..20901b9 --- /dev/null +++ b/AyCode.Core/docs/adr/0001-binary-projection-serialization.md @@ -0,0 +1,91 @@ +# ADR 0001: AcBinary projection serialization (TSource → TTarget wire schema) + +> ⚠️ **DRAFT — not yet finalized.** This ADR captures architectural intent. Status remains `Proposed` until phase 1 implementation produces benchmarks validating the performance hypothesis. + +## Status + +Proposed (2026-04-28) + +## Context + +AcBinary today serializes a runtime instance using **its own runtime type** as the wire-schema authority. To produce DTO-shaped wire output (e.g., for SignalR API responses), the consuming code must currently: + +1. Map `Entity → DTO` via a hand-written mapper (this codebase does NOT use AutoMapper, Mapster, or any external mapping library) +2. Serialize the resulting `DTO` instance via AcBinary + +Friction observed: + +- **Boilerplate**: every `Entity ↔ DTO` pair requires a hand-written mapper method, scattered across the codebase +- **Allocation pressure**: each call materializes the intermediate DTO instance + nested DTO graph (often dozens of objects per request in high-throughput SignalR scenarios) +- **Two property-iterations per call**: mapper reads Entity → writes DTO; serializer reads DTO → writes wire — duplicate work over the same data + +The HybridPack rebrand timeline makes API-stability decisions urgent before the public NuGet release: shipping a non-projection API and adding projection later is a breaking change. + +**In scope:** +- Phase 1: runtime projection path in the binary serializer +- Phase 2/3: SGen-projection per type-pair (architectural framing only — this ADR does not commit to phase 2/3 implementation timing) +- Property name-match cases (the 80% common case) +- API surface design (`ToBinaryAs` or equivalent) + +**Out of scope:** +- Toon projection serialization (separate future ADR if and when needed) +- Structural divergence handling (e.g., `Order.Address: Address` → `OrderDto.AddressLine: string` flatten) — phase 1 covers only name-match; divergent cases need explicit config addressed in a follow-up +- Deserializer-side projection (TSource read into TTarget without mapper) — separate concern, not part of this ADR +- Cross-language wire-format negotiation + +## Decision + +Adopt **direct source→target projection serialization** in AcBinary, exposed as `entity.ToBinaryAs()` (and equivalents on the explicit `AcBinarySerializer` API surface). + +Implementation strategy is **phased**: + +- **Phase 1** — Runtime projection path. New `BinarySerializationProjectionContext` drives the wire write loop using `TTarget`'s metadata as the schema authority and `TSource`'s compiled-expression property accessors as the data source. Hash-based property matching reuses the existing `UseMetadata` infrastructure. +- **Phase 2** — Source-generated per-pair projection writers, opt-in for hot type-pairs. Distinct registry from the existing per-type SGen writers. +- **Phase 3** — Generalization (broader API ergonomics, divergence-config support, possible Toon equivalent) only if phases 1-2 demonstrate measurable production benefit. + +Phase 1 internally forces `UseGeneratedCode=false` regardless of caller options, because the existing per-type SGen writers (`Unsafe.As` cast) would corrupt the wire output if reused for projection. + +## Consequences + +**Positive:** + +- Eliminates manual mapper boilerplate for name-match cases (estimated 80%+ of Entity↔DTO pairs in this codebase) +- Removes intermediate DTO allocation per call — measurable GC pressure reduction in SignalR high-throughput scenarios +- One property-iteration instead of two — net CPU savings even before Phase 2 SGen optimization +- Builds on AcBinary runtime path which benchmarks already faster than MessagePack contract-based SGen — projection runtime path expected to preserve this advantage end-to-end. AcBinary SGen path benchmarks faster than MemoryPack +- Genuine USP for the HybridPack brand: no mainstream .NET serializer (System.Text.Json, MessagePack, MemoryPack, Newtonsoft.Json, Mapster) offers direct cross-type wire-schema serialization without intermediate materialization +- Reuses existing `UseMetadata` hash-matching infrastructure — minimal new surface area in phase 1 + +**Negative:** + +- New context type (`BinarySerializationProjectionContext`) adds API surface, even if internal implementation +- Phase 2 SGen path implies N×M generator pairs (one per `(TSource, TTarget)` combination registered) — opt-in per-pair to bound the explosion +- Structural-divergence cases (renamed properties, flattening) need explicit config — phase 1 only handles name-match, leaving 20%-ish of real-world cases requiring follow-up work +- `UseGeneratedCode=false` enforced internally in projection mode — documented behavioural surprise for users who set it globally to `true` +- Phase 1 projection-context overhead vs non-projection path is small but non-zero (extra hash-matching at type-pair init) — irrelevant for end-to-end workflow but visible in microbenchmarks + +**Follow-ups required:** + +- `BINARY_TODO.md` — phase 1 implementation entry (`ACCORE-BIN-T-Y6R2`) +- `BINARY_TODO.md` — phase 2 SGen-projection path entry (deferred, opens only after phase 1 production validation) +- Benchmark suite addition — projection runtime vs `mapper+MessagePack-SGen` baseline (phase 1 acceptance criterion) +- API-shape finalization decision (`ToBinaryAs()` vs `SerializeAs()` vs builder-style — pending phase 1 prototyping) +- `BINARY_FEATURES.md` — new "DTO projection" section once phase 1 ships +- Documentation cross-link from `BINARY/README.md` → ADR + +## Alternatives considered + +- **Status quo: manual mapper + AcBinary serialize** (rejected): preserves boilerplate, allocation overhead, and 2-iteration cost. Current source of the friction this ADR addresses. + +- **External mapping library (Mapster / AutoMapper)** (rejected): would require adopting a new dependency and learning its conventions. The codebase has not used external mappers historically; bringing one in to solve this specific problem is disproportionate. Mapster's source-gen path would still materialize the intermediate DTO instance — does not address the allocation-pressure and 2-iteration concerns. + - *Cost:* medium — adoption + config + new dependency + - *Future flexibility:* locks team into the external library's mapping conventions + +- **LINQ-style projection lambda** (`Serializer.Serialize(order, src => new OrderDto { Id = src.Id, ... })`) (rejected): cleaner syntax than scattered mappers, but the lambda still materializes the DTO inline — same allocation and 2-iteration cost as the status quo. Solves the boilerplate dispersal but not the performance concerns. + +## Related + +- Related TODOs: `ACCORE-BIN-T-Y6R2` (phase 1 implementation, this ADR's primary follow-up), `ACCORE-BIN-T-Z3K8` (`IId` migration — sibling refactor, both required before NuGet release), `ACCORE-BIN-T-N7V1` (`[JsonIgnore]` replacement — sibling brand-decoupling work) +- Related ADRs: none yet (first ADR in `AyCode.Core/docs/adr/`) +- Related discussions: HybridPack rebrand discussion (this design session) established the brand context for projection serialization as a flagship feature +- External references: none (no public benchmarks linked yet — will be added as part of phase 1 acceptance) diff --git a/AyCode.Core/docs/adr/README.md b/AyCode.Core/docs/adr/README.md new file mode 100644 index 0000000..92b0eaf --- /dev/null +++ b/AyCode.Core/docs/adr/README.md @@ -0,0 +1,19 @@ +# AyCode.Core — Architecture Decision Records + +Project-scoped ADRs for the AyCode.Core framework library. Decisions internal to AyCode.Core's architecture (binary serializer, source generators, type metadata, etc.). Cross-cutting decisions (affecting multiple sub-projects) live in the [repo-root `docs/adr/`](../../../docs/adr/README.md) instead. + +See [`.github/skills/adr-author/SKILL.md`](../../../.github/skills/adr-author/SKILL.md) for the full authoring procedure. + +## Convention + +- **Format:** Nygard-style ADR; one file per decision +- **Filename:** `NNNN-.md` +- **Numbering:** sequential, append-only, derive next from `max(existing) + 1` +- **Template:** see [`0000-TEMPLATE.md`](0000-TEMPLATE.md) +- **Status:** `Proposed (YYYY-MM-DD)` → `Accepted (YYYY-MM-DD)` / `Rejected (YYYY-MM-DD)` / `Superseded by ADR-XXXX (YYYY-MM-DD)` + +## Index + +| ID | Title | Status | +|---|---|---| +| [0001](0001-binary-projection-serialization.md) | AcBinary projection serialization (TSource → TTarget wire schema) | Proposed (2026-04-28) | diff --git a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md index ff25e66..126d2bd 100644 --- a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md +++ b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md @@ -274,5 +274,6 @@ services.AddSingleton(); // derived from AcSignalRClientBase ## Related ADRs - [`adr/0001-acbinary-decorator-feature-stack-design.md`](../adr/0001-acbinary-decorator-feature-stack-design.md) — *AcBinaryHubProtocol optional feature stack — decorator-based composition design* (Status: Proposed). Umbrella ADR for the optional decorator-based feature stack (encryption, compression with `MinSize`, OpenTelemetry tracing, HMAC signing). Each NuGet-competitiveness TODO entry (`ACCORE-SBP-T-H7M5` / `N9F3` / `J5W8` / `B3K6` in `SIGNALR_BINARY_PROTOCOL_TODO.md`) resolves under this umbrella's architectural framework. Forthcoming leaf ADRs (0002-0005) will provide per-feature design + threat model. +- [`AyCode.Core/docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) — *AcBinary streaming receive — AsyncPipeReaderInput unified primitive and transport-agnostic helpers* (Status: Proposed (2026-04-27)). Repo-level cross-cutting ADR establishing the receive-side architecture. Consolidates `SegmentBufferReader` + `SegmentBufferReaderInput` into a single `AsyncPipeReaderInput` class shared across SignalR (`TryParseChunkData` delegation in Step 6 / `ACCORE-SBP-T-G7T2`), NamedPipe IPC, and FileStream helpers. The unified AsyncSegment chunked wire format (`[INT32 length][200 CHUNK_START][201][UINT16 size][data][202 CHUNK_END]`) documented in this README's "Wire Format" / "BinaryProtocolMode" sections is preserved verbatim and is the transport-agnostic invariant the new ADR generalizes to NamedPipe + FileStream. **Source:** `AyCode.Services/SignalRs/AcBinaryHubProtocol.cs` (base), `AyCode.Services/SignalRs/AyCodeBinaryHubProtocol.cs` (consumer logic), `AyCode.Services/SignalRs/BinaryProtocolMode.cs` (enum), `AyCode.Services/SignalRs/AcBinaryHubProtocolOptions.cs` (options), `AyCode.Services/SignalRs/AcSignalRProtocolExtensions.cs` (DI extensions) diff --git a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md index fb40543..87b6e5c 100644 --- a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md +++ b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md @@ -45,6 +45,19 @@ Alternative to wire-detection: use SignalR handshake message's `extensions` JSON ``` Zero first-message overhead, fully explicit. Both sides advertise their send-modes; pick intersection. Specification to be drafted; compatibility with non-AC clients (pure JSON etc.) must remain. +## ACCORE-SBP-T-G7T2: Migrate `AcBinaryHubProtocol.TryParseChunkData` to `AsyncPipeReaderInput`; delete `SegmentBufferReader` + `SegmentBufferReaderInput` (Step 6 of ADR-0003) +**Priority:** P1 · **Type:** Refactor · **Related ADR:** [`docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) Step 6 · **Depends on:** `ACCORE-BIN-T-B5Y6` (and all earlier BIN steps) + +Switch `AcBinaryHubProtocol.TryParseChunkData` from `SegmentBufferReader.Write(span)` to `AsyncPipeReaderInput.Feed(span)`. Update `AsyncChunkState` field type from `SegmentBufferReader Buffer` to `AsyncPipeReaderInput Input`. Lazy `Task.Run` deser-task start (after first chunk), `CHUNK_END` lifecycle (`Complete()` + `Dispose()` + `_chunkStates.Remove`), and the WASM synchronous-deser path all preserved. Wire format unchanged. + +After cutover: delete `SegmentBufferReader.cs` and `SegmentBufferReaderInput.cs` from `AyCode.Core/Serializers/Binaries/`. Final irreversible step of the ADR-0003 migration. + +**Acceptance:** +- All existing SignalR integration tests pass (no behavioral regression). +- Wire format unchanged (binary diff of CHUNK_START / CHUNK_DATA / CHUNK_END frames identical to pre-cutover capture). +- Code-search finds 0 references to `SegmentBufferReader` or `SegmentBufferReaderInput` after deletion. +- ADR-0003 Status promoted from `Proposed` to `Accepted (YYYY-MM-DD)` in this commit or an immediate follow-up. + --- # 🟡 NuGet competitiveness ideas — NOT current priority diff --git a/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md b/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md index b349f2d..1ae50e8 100644 --- a/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md +++ b/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md @@ -212,4 +212,5 @@ A v1 client (no decorators registered) connecting to a v2 server (decorators in - Related TODOs: `../SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md` ACCORE-SBP-T-H7M5 (encryption), ACCORE-SBP-T-N9F3 (compression+MinSize), ACCORE-SBP-T-J5W8 (tracing), ACCORE-SBP-T-B3K6 (signing). The `AcHubProtocolDecoratorBase` impl + handshake-mechanism TODO is **not yet created** — see Follow-up #1 for the conditions under which it will be added. - Forthcoming ADRs: 0002 (payload encryption), 0003 (message compression with MinSize), 0004 (OpenTelemetry tracing), 0005 (HMAC signing-only) — all will declare `Depends on: ADR-0001`. +- Repo-level ADR (cross-cutting): [`AyCode.Core/docs/adr/0003-acbinary-streaming-receive-architecture.md`](../../../docs/adr/0003-acbinary-streaming-receive-architecture.md) — receive-side `AsyncPipeReaderInput` primitive (replaces `SegmentBufferReader` + `SegmentBufferReaderInput`) that this decorator stack operates on top of in AsyncSegment mode (per-chunk decoration constraint, see Decision §"AsyncSegment per-chunk decoration constraint" above). The repo-level ADR's number `0003` is unrelated to this folder's project-scoped numbering — separate sequences per ADR folder. - Topic-folder cross-ref: `../SIGNALR_BINARY_PROTOCOL/README.md` to add `## Related ADRs` section in Step 8. diff --git a/docs/AUTH/AUTH_ISSUES.md b/docs/AUTH/AUTH_ISSUES.md index 504f3bf..75de147 100644 --- a/docs/AUTH/AUTH_ISSUES.md +++ b/docs/AUTH/AUTH_ISSUES.md @@ -7,11 +7,85 @@ For the architectural decision that scoped this topic, see [`../adr/0001-user-be ## Active entries -*(No `AUTH-I-N` entries yet — topic just created. Currently-relevant security entries live as `ACCORE-LOG-I-P5W3` and `ACCORE-LOG-I-K1Z7` in [`LOGGING_ISSUES.md`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md), pre-migration. ADR 0001's "Status migration on AUTH topic creation" follow-up is the planned home transfer — separate user-approved task.)* +## ACCORE-AUTH-I-B3X5: JWT signing key written to log (CRITICAL security) + +**Severity:** 🛑 **Critical (security)** · **Status:** Closed (2026-04-25) · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192` + +### Description +`GenerateAccessToken` writes the JWT signing key to the logger via `GlobalLogger.Detail`: + +```csharp +GlobalLogger.Detail($"Key: {configuration["JWT:Key"]}"); +``` + +The signing key is the cryptographic secret used to sign all JWT access tokens. Anyone with read access to the logs (file system, log aggregators, retention archives, operator dashboards, screenshots during incident response) can: + +1. Forge arbitrary tokens for any user +2. Impersonate any role / claim +3. Bypass authentication entirely + +Log storage is typically at a lower trust level than dedicated secret stores. This exposure path is a complete authentication bypass. + +### Fix direction +Remove the `GlobalLogger.Detail($"Key: ...")` line entirely. The signing key must NEVER appear in any log output. If presence verification is needed for diagnostics, log only a hash prefix (e.g. `Key configured: SHA256[0..8] = abc12345`). + +### Resolution + +**What:** Wrapped the `GlobalLogger.Detail($"Key: ...")` call in `#if DEBUG` / `#endif` with a `// SECURITY:` comment block explaining the risk. Line preserved (not deleted) — user opted for DEBUG-gating over removal so local auth-debug retains the diagnostic. + +**Where:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192` (line numbers shifted slightly due to the inserted comment). + +**Why:** User-directed deviation from the originally proposed "remove entirely" fix direction (bearer-token ADR planning session, 2026-04-25). DEBUG-gating ensures production (`-c Release`) builds strip the line at compile time, while preserving dev-time diagnostics. + +**Caveat:** DEBUG builds still write the key to log. CI/CD must build with `-c Release` for production deployments. The inline `// SECURITY:` comment in the code warns future maintainers. + +### Related +- **Supersedes:** [`ACCORE-LOG-I-P5W3`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md#accore-log-i-p5w3) — original LOG-topic home; relocated here per ADR-0001 "Status migration on AUTH topic creation" follow-up. The LOGGING entry retains its full body (immutable per `TOPIC_CODES.md` Status conventions) and now points back here as the canonical home. +- **Reference:** ADR [`0001-user-bearer-token-flow.md`](../adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences. +- `ACCORE-AUTH-I-T8N2` (sibling — access token also leaked, same file, same code path) +- `LOGGING/README.md` may need a new "Never log secrets" guideline section (forthcoming TODO entry in `LOGGING_TODO.md`, no ID assigned yet). +- **Discovery context**: this issue and `ACCORE-AUTH-I-T8N2` emerged during the user-bearer-token ADR context-gathering phase (2026-04-25, while reading `AcLoginServiceServer.cs`). Originally tracked under LOG topic (`ACCORE-LOG-I-P5W3`); relocated here when AUTH topic was created (`LLMP-DEC-49`). + +## ACCORE-AUTH-I-T8N2: JWT access token written to log (CRITICAL security) + +**Severity:** 🛑 **Critical (security)** · **Status:** Closed (2026-04-25) · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212` + +### Description +After `GenerateAccessToken` produces a token, the token text itself is written to the logger: + +```csharp +GlobalLogger.Detail($"AccesToken: {writtenToken}"); +``` + +(Side note: `AccesToken` is a typo for `AccessToken` — fix while at the line.) + +Access tokens are bearer credentials — anyone holding the token can authenticate as the user until token expiry (currently 6h per `GenerateAccessToken` config). Logged tokens leak via the same channels as `ACCORE-AUTH-I-B3X5` (live streams, retention archives, screenshots, shared logs during ops). + +### Fix direction +Remove the `GlobalLogger.Detail($"AccesToken: ...")` line entirely. If issuance verification is needed, log only **metadata** — user ID, expiry timestamp, issuer — never the token itself. + +### Resolution + +**What:** Two changes: +1. Wrapped the `GlobalLogger.Detail($"AccessToken: ...")` call in `#if DEBUG` / `#endif` with a `// SECURITY:` comment block. +2. Fixed the `AccesToken` → `AccessToken` typo (flagged in original Description as a side-note) in the log-message string literal. + +**Where:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212` (line numbers shifted slightly due to the inserted comment). + +**Why:** Same as `ACCORE-AUTH-I-B3X5` — user opted for DEBUG-gating over deletion (2026-04-25, bearer-token ADR session). Typo fix is opportunistic since the line was being touched. + +**Caveat:** Same DEBUG-build leak constraint as `ACCORE-AUTH-I-B3X5`. CI/CD must build with `-c Release` for production. + +### Related +- **Supersedes:** [`ACCORE-LOG-I-K1Z7`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md#accore-log-i-k1z7) — original LOG-topic home; relocated here per ADR-0001 "Status migration on AUTH topic creation" follow-up. +- **Reference:** ADR [`0001-user-bearer-token-flow.md`](../adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences. +- `ACCORE-AUTH-I-B3X5` (sibling — JWT signing key also leaked, same file) +- Same `LOGGING/README.md` "Never log secrets" guideline gap as `ACCORE-AUTH-I-B3X5`. +- **Discovery context**: emerged alongside `ACCORE-AUTH-I-B3X5` during the bearer-token ADR session. Originally tracked under LOG topic (`ACCORE-LOG-I-K1Z7`); relocated here when AUTH topic was created (`LLMP-DEC-49`). ## Entry format -When adding the first entry, follow the standard `_ISSUES.md` shape used across topics (LOGGING, BINARY, SIGNALR), with `AUTH-I-N` ID format per [`TOPIC_CODES.md`](../../.github/skills/docs-check/references/TOPIC_CODES.md): +When adding new entries, follow the standard `_ISSUES.md` shape used across topics (LOGGING, BINARY, SIGNALR), with `ACCORE-AUTH-I-` ID format per [`TOPIC_CODES.md`](../../.github/skills/docs-check/references/TOPIC_CODES.md) and [`REPO_PREFIXES.md`](../../.github/REPO_PREFIXES.md): - ID line with `**Severity:** ... · **Status:** ... · **Area:** ...` - `### Description` — concrete problem with quotable evidence diff --git a/docs/adr/0003-acbinary-streaming-receive-architecture.md b/docs/adr/0003-acbinary-streaming-receive-architecture.md new file mode 100644 index 0000000..9209f30 --- /dev/null +++ b/docs/adr/0003-acbinary-streaming-receive-architecture.md @@ -0,0 +1,319 @@ +# ADR 0003: AcBinary streaming receive — AsyncPipeReaderInput unified primitive and transport-agnostic helpers + +## Status + +Proposed (2026-04-27) + +## Context + +The `AcBinaryHubProtocol` (`AyCode.Services/SignalRs/AcBinaryHubProtocol.cs`) AsyncSegment chunked streaming receive-side currently uses two coupled types in `AyCode.Core/Serializers/Binaries/`: + +- **`SegmentBufferReader`** (sealed class) — thread-safe single-producer/single-consumer byte buffer with sliding-window semantics (write/read positions reset to 0 when consumer catches up). Backed by `ArrayPool`. Replaces an earlier `System.IO.Pipelines.Pipe`-based implementation. +- **`SegmentBufferReaderInput`** (struct) — `IBinaryInputBase` adapter that reads from a `SegmentBufferReader`. Holds only a class-reference; state lives in the buffer class. + +This receive-side architecture has three structural limitations that block the framework's NuGet positioning and ergonomic standalone use: + +### 1. Type identity coupled to a SignalR-mode name + +The "Segment" prefix in both type names refers to AsyncSegment (the SignalR `BinaryProtocolMode`). For a NuGet consumer using `AcBinarySerializer` standalone (NamedPipe IPC, file-stream deserialization), the type names suggest "this is a SignalR thing — won't apply to me", even though the underlying primitive (sliding-window buffer with push/pull feed sources) is fully transport-agnostic. + +### 2. Send-/receive-side asymmetry without payoff + +The send side has a clean, single-class primitive: **`AsyncPipeWriterOutput`** (perf-critical hot path, polished). The receive side has a 2-class adapter pair (`SegmentBufferReader` + `SegmentBufferReaderInput`) where the struct is just a thin proxy over the class. The struct/class split was originally motivated by JIT specialization for `DeserializeSequence` — but profiling shows the per-call indirection cost is on the order of 1 µs / MB, which is dominated by serialization work itself. + +### 3. No first-class transport helpers + +NamedPipe IPC and large-file streaming are common deserialization scenarios for a binary protocol library. Today, consumers must wire up their own `Pipe` + `Task.Run` + manual lifecycle. A self-contained NuGet package should ship turnkey helpers — `SerializeToNamedPipeAsync` / `DeserializeFromNamedPipeAsync` / `SerializeToFileStreamAsync` / `DeserializeFromFileStreamAsync` — sharing the same chunked AsyncSegment wire format. + +### Wire format (recap) + +The AsyncSegment chunked wire format is the streaming-deserialization format — a transport-agnostic invariant: + +``` +[INT32 LE length: 4 byte][message body] + message body: + [200] CHUNK_START + [201][UINT16 size][data] ← repeats + [201][UINT16 size][data] + ... + [202] CHUNK_END +``` + +This format is unchanged by this ADR. The receive-side architecture is the subject; the wire format is the invariant. + +### Streaming doctrine: peak memory bounded by chunk size + +The current `SegmentBufferReader.Write` resets `_writePos` and `_readPos` to 0 whenever `_readPos == _writePos > 0` (consumer caught up). This is the central streaming invariant: peak buffer memory is bounded by **two-chunks-worth-of-bytes** (typically ~8 KB), regardless of total message size. A 1-GB file and a 4-KB SignalR message both run with the same ~8-KB peak buffer. This must be preserved across the redesign. + +### Cross-project scope + +The change touches both projects in this repo: + +- **AyCode.Core** (Layer 0, framework): `AsyncPipeReaderInput`, `DrainFromAsync` extension, NamedPipe + FileStream helpers, deletion of `SegmentBufferReader` + `SegmentBufferReaderInput`. +- **AyCode.Services** (Layer 1): `AcBinaryHubProtocol.TryParseChunkData` migration to delegate to the new primitive; `AsyncChunkState` field type change. + +ADR placed at the repo-root `docs/adr/` per the multi-project routing rule in [`.github/skills/adr-author/SKILL.md`](../../.github/skills/adr-author/SKILL.md) Step 1 ("cross-cutting decision → highest common ancestor"). The receive-side primitive lives in `AyCode.Core` but is consumed by transport helpers in the same project AND by `AcBinaryHubProtocol` in `AyCode.Services`. The project-scoped sibling [`AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md`](../../AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md) (decorator feature-stack umbrella) depends on this primitive at the AsyncSegment per-chunk decoration boundary; the cross-reference is one-way (Services-ADR depends on Core-ADR). + +The number `0002` in this folder is intentionally skipped — reserved by [`0001-user-bearer-token-flow.md`](0001-user-bearer-token-flow.md)'s Follow-ups + Related sections for the forthcoming refresh-token flow ADR; preserving that reservation avoids amending an already-Accepted ADR's immutable body. + +## Decision + +Consolidate the receive-side architecture around a single transport-agnostic primitive — **`AsyncPipeReaderInput`** — and ship NamedPipe + FileStream helpers using the same chunked AsyncSegment wire format as the SignalR protocol. + +### 1. `AsyncPipeReaderInput` — single sealed class replaces SegmentBufferReader + SegmentBufferReaderInput + +A new `sealed class` in `AyCode.Core/Serializers/Binaries/`: + +```csharp +public sealed class AsyncPipeReaderInput : IBinaryInputBase, IDisposable +{ + // Buffer state (sliding window — _readPos/_writePos reset to 0 on consumer catch-up) + private byte[] _buffer; + private int _writePos; + private int _readPos; + private bool _completed; + + // Synchronization + private readonly ManualResetEventSlim _dataAvailable; + + // Grow tracking (rare path — old buffers held until Dispose to avoid use-after-free + // while the consumer thread holds a local reference) + private byte[][]? _oldBuffers; + private int _oldBufferCount; + + private readonly ILogger? _logger; + + // Producer API (push) — used by SignalR's TryParseChunkData and DrainFromAsync alike + public void Feed(ReadOnlySpan data); + public void Complete(); + + // IBinaryInputBase + public void Initialize(out byte[] buffer, out int position, out int bufferLength); + public bool TryAdvanceSegment(ref byte[] buffer, ref int position, ref int bufferLength, int needed); + public void Release(); + + public void Dispose(); +} +``` + +**Naming convention rationale**: `AsyncPipeReaderInput` is symmetric with the existing `AsyncPipeWriterOutput`. The `Async` **prefix** follows .NET BCL precedent for async-oriented types (`AsyncEnumerable`, `IAsyncDisposable`, `AsyncLocal`, `AsyncCallback`, `IAsyncResult`); the BCL convention puts `Async` as **suffix** on methods (returning `Task` / `ValueTask`) but as **prefix** on types. The `Async` prefix here also differentiates from the BCL `System.IO.Pipelines.PipeReader` type at first glance. + +`sealed` permits JIT inlining + dynamic PGO devirtualization at `IBinaryInputBase` interface call sites in shared generic code. + +**Class vs struct rationale**: the struct/class split in today's design exists to keep `DeserializeSequence` JIT-specialized over a small struct (the `SegmentBufferReaderInput` proxy). Empirically, the inner deserialization hot path reads from local stack variables (`buffer`, `position`, `bufferLength`) — not through `input.X` — so the struct specialization win is concentrated at the rare-path `TryAdvanceSegment` call (~1 call per chunk worth of bytes). The class-based design pays one extra vtable dispatch on that rare call (~5 ns) in exchange for removing one indirection through the inner buffer reference (~1 ns saved); net delta is sub-microsecond per MB. A pure-struct alternative (with internal buffer state) was rejected — see Alternatives. + +### 2. Feed-API for push-mode (SignalR pattern) + +`Feed(ReadOnlySpan data)` is the producer-thread API for transports that **push** chunk data inline (the SignalR receive loop). The protocol calls `Feed` from `TryParseChunkData` whenever a `[201]` CHUNK_DATA frame's payload arrives. Internal logic: + +1. If consumer has caught up (`_readPos == _writePos > 0`), reset both to 0 (sliding-window cycling). +2. If `_writePos + data.Length > _buffer.Length`, grow (last-resort path — under typical chunk-aligned write patterns this never fires). +3. Copy bytes to the buffer, advance `_writePos` (volatile write), signal `_dataAvailable`. + +Synchronization model preserved verbatim from today's `SegmentBufferReader`: `_writePos` is volatile-written by producer / volatile-read by consumer; `_readPos` is volatile-written by consumer / volatile-read by producer; reset-to-0 happens only when both equal (consumer not actively reading). + +### 3. `DrainFromAsync` extension for pull-mode (NamedPipe / FileStream pattern) + +Transports backed by `System.IO.Pipelines.PipeReader` (NamedPipe and FileStream-via-PipeReader) **pull** data through `PipeReader.ReadAsync()`. An extension method handles this: + +```csharp +public static class AsyncPipeReaderInputExtensions +{ + public static async Task DrainFromAsync( + this AsyncPipeReaderInput input, + PipeReader reader, + CancellationToken cancellationToken = default) + { + while (true) + { + var result = await reader.ReadAsync(cancellationToken).ConfigureAwait(false); + foreach (var segment in result.Buffer) + input.Feed(segment.Span); + reader.AdvanceTo(result.Buffer.End); + if (result.IsCompleted) break; + } + input.Complete(); + } +} +``` + +The extension lives in a separate file (`AsyncPipeReaderInputExtensions.cs`). This keeps the core class free of `System.IO.Pipelines` import surface in its primary contract and makes the optional pull-mode visible at use-sites. + +### 4. Initial buffer capacity — `BufferWriterChunkSize × 2` heuristic + +Constructor parameter `initialCapacity` recommended value: `options.BufferWriterChunkSize × 2`. + +| Context | Chunk size | Initial capacity | +|---------|-----------|------------------| +| SignalR (Kestrel slab-aligned) | 4 KB | 8 KB | +| Standalone (default) | 64 KB | 128 KB | + +Two-chunks-worth of headroom lets the producer write the next chunk while the consumer is still reading the previous one — without growth. Reset-to-0 cycling reuses the same buffer for the message's lifetime regardless of total payload size. + +**Explicitly NOT file-size-aware**: an earlier draft considered pre-allocating to file size for `DeserializeFromFileStreamAsync`. Rejected — would defeat the streaming purpose (peak memory ∝ file size, no zerocopy/zeroalloc). The fixed ~8-KB peak (or ~128-KB at the standalone default) is the streaming-doctrine invariant — see Alternatives. + +### 5. Reset-to-0 sliding-window cycling preserved + +The peak-memory-bounded streaming invariant from today's `SegmentBufferReader.Write` is preserved verbatim: when `_readPos > 0 && _readPos == _writePos`, both fields reset to 0. Consumer's local `position` re-reads from `_readPos` inside `TryAdvanceSegment` after wake-from-Wait. This is the architectural cornerstone — without it, peak memory grows linearly with message size and the transport-agnostic FileStream helper becomes useless on multi-GB files. + +### 6. `AsyncPipeWriterOutput` unchanged + +The send side stays as-is. It's perf-critical and well-tested. This ADR addresses only the receive side. Renaming `AsyncPipeWriterOutput` for full naming uniformity was considered and rejected (breaking change for early NuGet consumers — see Alternatives). + +### 7. NamedPipe helpers — full lifecycle, one-shot + +```csharp +public static class AcBinarySerializerNamedPipeExtensions +{ + public static async Task SerializeToNamedPipeAsync( + this AcBinarySerializerOptions options, + T value, + string pipeName, + CancellationToken cancellationToken = default); + + public static async Task DeserializeFromNamedPipeAsync( + this AcBinarySerializerOptions options, + string pipeName, + CancellationToken cancellationToken = default); +} +``` + +Send-side flow: `NamedPipeServerStream` → `PipeWriter.Create(stream)` → `AsyncPipeWriterOutput` → AcBinary chunked write → flush → close. + +Receive-side flow: `NamedPipeClientStream` → `PipeReader.Create(stream)` → `AsyncPipeReaderInput.DrainFromAsync(pipeReader)` (background task) → `AcBinaryDeserializer.DeserializeAsync(input)` → result. + +Cross-platform: `NamedPipeServerStream` / `NamedPipeClientStream` work on Windows + Linux (Unix domain sockets under the hood). WASM throws `PlatformNotSupportedException` per BCL contract. + +### 8. FileStream helpers — streaming, fixed peak memory + +```csharp +public static class AcBinarySerializerFileStreamExtensions +{ + public static async Task SerializeToFileStreamAsync( + this AcBinarySerializerOptions options, + T value, + string filePath, + CancellationToken cancellationToken = default); + + public static async Task DeserializeFromFileStreamAsync( + this AcBinarySerializerOptions options, + string filePath, + CancellationToken cancellationToken = default); +} +``` + +Send-side flow: `FileStream.Create(path)` → `PipeWriter.Create(fileStream)` → `AsyncPipeWriterOutput` → AcBinary chunked write → flush → close. + +Receive-side flow: `FileStream.OpenRead(path)` → `PipeReader.Create(fileStream)` → same `DrainFromAsync` pattern as NamedPipe. + +A 1-GB file deserializes with ~8-KB peak buffer (per the streaming-doctrine invariant — Decision #5). Random-access file reads are not used; the file is a one-pass forward stream from the deserializer's perspective. + +### 9. Unified AsyncSegment chunked wire format across all transports + +NamedPipe, FileStream, and SignalR all use the same chunked wire format (`[INT32 length][200 CHUNK_START]+[201][UINT16 size][data]+[202 CHUNK_END]`). This is the cornerstone of the unification — one `AsyncPipeReaderInput` implementation handles all three because the wire bytes are identical. + +Implication: a file written by `SerializeToFileStreamAsync` can be drained by a NamedPipe consumer (with appropriate transport adapter) and produce the same deserialized object. The wire format is the contract; transports are interchangeable. + +### 10. `AcBinaryHubProtocol.TryParseChunkData` delegates to `AsyncPipeReaderInput.Feed` + +The existing chunked path in `AcBinaryHubProtocol.TryParseChunkData` (currently uses `state.Buffer.Write(span)` on a `SegmentBufferReader`) migrates to `state.Input.Feed(span)` on an `AsyncPipeReaderInput`. Behavioral equivalence — only the type changes. + +State storage in `AsyncChunkState`: + +```csharp +// Before: +public SegmentBufferReader Buffer = null!; + +// After: +public AsyncPipeReaderInput Input = null!; +``` + +The cutover preserves the existing protocol mode dispatch, lazy `Task.Run` deser-task start (after first chunk), and `CHUNK_END` lifecycle (`Complete()` + `Dispose()` + `_chunkStates.Remove`). Wire format is unchanged. + +### 11. UINT16 65535-byte chunk-size invariant — transport-agnostic + +The `[201][UINT16 size][data]` chunk frame caps wire-chunks at 65535 bytes. This is a **transport-agnostic** invariant — applies to NamedPipe and FileStream chunks, not just SignalR. Consumers configuring `options.BufferWriterChunkSize > 65535` will fail at write-time validation (the existing chunking logic enforces this via the wire format). + +This ADR codifies the limit as a wire-protocol invariant, not a SignalR-/Kestrel-derived constraint. (See [`AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md`](../../AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md) "UINT16 size prefix design intent" for the original rationale; this ADR generalizes the scope.) + +### 12. Standalone NuGet positioning — `AcBinarySerializer` self-contained + +The `AcBinarySerializer` NuGet package (`AyCode.Core` assembly) ships with: + +- `AsyncPipeReaderInput` — transport-agnostic streaming primitive +- `DrainFromAsync` extension — `System.IO.Pipelines` consumer +- `SerializeToNamedPipeAsync` / `DeserializeFromNamedPipeAsync` — NamedPipe IPC +- `SerializeToFileStreamAsync` / `DeserializeFromFileStreamAsync` — file streaming + +No transitive `Microsoft.AspNetCore.SignalR` dependency. The SignalR integration (`AcBinaryHubProtocol`) lives in the separate `AyCode.Services` assembly and references `AyCode.Core` for the streaming primitive — not the other way around. + +## Consequences + +### Positive + +- **Single primitive across transports**: SignalR / NamedPipe / FileStream use the same `AsyncPipeReaderInput` — one mental model, one set of bug fixes propagates everywhere. +- **NuGet-clean self-contained package**: `AcBinarySerializer` ships turnkey transport helpers without forcing consumers to wire `Pipe` + `Task.Run` + lifecycle by hand. +- **Streaming-doctrine preserved**: ~8-KB peak buffer regardless of message / file size — same as today. +- **Symmetric send/receive naming**: `AsyncPipeWriterOutput` ↔ `AsyncPipeReaderInput` reads as a pair in the API; the `Async` prefix follows .NET BCL convention for type-level naming. +- **Type identity decoupled from SignalR**: "Segment" prefix removed; "AsyncPipeReader" describes the class's actual capability (async pipe-style reader), not a SignalR mode. +- **Easier to test in isolation**: `AsyncPipeReaderInput` can be unit-tested without SignalR scaffolding (which is what the new "real parallel pipeline test" exercises in Step 3 of the migration plan). +- **Send-side untouched**: `AsyncPipeWriterOutput` (perf-critical, well-tested) stays as-is — risk concentrated on the receive side only. + +### Negative + +- **~1 µs / MB perf delta vs current struct+class architecture**: virtual call dispatch on the `IBinaryInputBase` interface in shared generic code path (vs JIT-specialized struct dispatch today). Negligible at typical message sizes (sub-millisecond cost on a 1-GB stream); accepted for the architectural simplicity gain. Verified in Step 3's parallel pipeline test as part of the migration acceptance criteria. +- **Allocation parity, not improvement**: same 1 class allocation per deserialize (today: `SegmentBufferReader`; after: `AsyncPipeReaderInput`). No regression, no gain. +- **Migration touches both `AyCode.Core` and `AyCode.Services`**: `AsyncPipeReaderInput` lands in Core; `AcBinaryHubProtocol` migration lands in Services. Two-assembly rollout sequenced via the 6-step migration plan below. +- **Brief temporary duplication during transition**: between Step 1 (new class lands) and Step 6 (old types deleted), both `SegmentBufferReader` + `SegmentBufferReaderInput` AND `AsyncPipeReaderInput` exist. Bounded by the migration window; mechanical delete at the end. + +### Migration plan (6 steps, each commit-reviewable) + +| Step | Topic | Files | Review checkpoint | +|------|-------|-------|-------------------| +| 1 | BIN | `AsyncPipeReaderInput.cs` (NEW); existing `SegmentBufferReader.cs` + `SegmentBufferReaderInput.cs` unchanged | New class compiles, unit-tested in isolation; SignalR path still on old types | +| 2 | BIN | `AsyncPipeReaderInputExtensions.cs` (NEW — `DrainFromAsync`) | Extension drains an in-memory `Pipe` end-to-end in a unit test | +| 3 | BIN | `AcBinarySerializerPipeParallelTests.cs` (REWRITE — replaces today's misleading test with a real parallel pipeline test) | Real ser+deser overlap measured on a thread pair; ~1 µs / MB delta confirmed | +| 4 | BIN | `AcBinarySerializerNamedPipeExtensions.cs` (NEW) | Cross-platform NamedPipe IPC roundtrip test passes | +| 5 | BIN | `AcBinarySerializerFileStreamExtensions.cs` (NEW) | Large-file (≥ 100 MB) roundtrip test passes with fixed ~8-KB peak buffer | +| 6 | SBP | `AcBinaryHubProtocol.cs` migrated to `AsyncPipeReaderInput`; `SegmentBufferReader.cs` + `SegmentBufferReaderInput.cs` DELETED | Existing SignalR tests pass; wire format unchanged; code-search finds 0 references to deleted types | + +Steps 1–5 are tracked under the `BIN` topic ([`AyCode.Core/docs/BINARY/BINARY_TODO.md`](../../AyCode.Core/docs/BINARY/BINARY_TODO.md)). Step 6 is tracked under the `SBP` topic ([`AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md`](../../AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md)). + +The migration is **abortable** between Step 5 and Step 6: if Step 3's parallel pipeline test or Step 4/5's roundtrip tests reveal an unfixable perf regression, the migration is paused; SignalR stays on the old types; the new helpers ship as-is on `AsyncPipeReaderInput` for NamedPipe / FileStream consumers; old types live on indefinitely. Step 6 (delete old types) is the final irreversible step. + +### Follow-ups required + +1. **TODO entries** — 6 freshly-generated `ACCORE-BIN-T-` and `ACCORE-SBP-T-` IDs for the 6 migration steps, added to `AyCode.Core/docs/BINARY/BINARY_TODO.md` (Steps 1–5) and `AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md` (Step 6) per the `docs-check` skill Step 5 procedure. Tracked as a separate `mehet` round. +2. **`AyCode.Core/docs/adr/README.md` index update** — append an `0003` row to the index table (Status: Proposed (2026-04-27)). Tracked as a separate `mehet` round. +3. **ADR-0001 (Services) cross-reference** — [`AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md`](../../AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md) "Related" section can optionally cross-reference ADR-0003 (the per-chunk decoration constraint in AsyncSegment mode benefits from the unified `AsyncPipeReaderInput` primitive). Tracked as a separate `mehet` round. +4. **Topic-folder cross-references** — [`AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md`](../../AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md) `## Related ADRs` section gets a row for ADR-0003. [`AyCode.Core/docs/BINARY/README.md`](../../AyCode.Core/docs/BINARY/README.md) gets a cross-ref to ADR-0003 for the streaming primitive. Tracked as a separate `mehet` round. +5. **Promotion to Accepted** — once all 6 migration steps land, Status changes from `Proposed (2026-04-27)` to `Accepted (YYYY-MM-DD)`. Old types deleted, NuGet package self-containment verified by a no-SignalR-reference build check. + +## Alternatives considered + +- **Rename existing types in place, preserve 2-class split** (rejected): keep `SegmentBufferReader` + `SegmentBufferReaderInput` as 2 types, just rename to `AsyncPipeReaderBuffer` + `AsyncPipeReaderInput`. Solves the naming-decoupling goal but doesn't solve the proxy split. The struct-as-thin-proxy-over-class indirection survives; users see two types where one would do. + +- **Struct merger with internal buffer state** (rejected): `struct AsyncPipeReaderInput` carrying `byte[]`, `_writePos`, `_readPos` directly. JIT struct specialization in `DeserializeSequence` preserved. Rejected — struct copy semantics break producer/consumer state sharing: the producer (SignalR receive thread) writes to a struct copy, the consumer (deserialization thread) reads from a different copy. Would require all interface methods to take `ref this` or all call sites to pass the struct by `ref` — sprawling API ergonomics issue, sinks into ref-everywhere boilerplate. The class-based design takes the ~1 µs / MB perf hit in exchange for clean copy-by-reference semantics. + +- **`PipelineReaderInput` naming (no `Async` prefix)** (rejected): cleaner standalone, no BCL `PipeReader` overlap concern, namespace-neutral. Rejected — breaks symmetry with the existing `AsyncPipeWriterOutput` send-side primitive, where the `Async` prefix is established project convention. The asymmetric naming pair would obscure the send/receive symmetry to API readers. + +- **Rename `AsyncPipeWriterOutput` to match a non-Async receive-side name** (rejected): full uniformity at the cost of a public-API breaking change. Rejected — `AsyncPipeWriterOutput` is part of the existing public surface; renaming would break all early consumers and predates the NuGet positioning effort. The new receive-side name should adopt the established convention, not invert it. + +- **`Async` as suffix instead of prefix** (rejected): `PipeReaderInputAsync` / `PipeWriterOutputAsync` matching the .NET method-naming convention (`ReadAsync`, `WriteAsync`). Rejected — the `Async` suffix convention applies to **methods** returning `Task` / `ValueTask`, not to types. .NET BCL types use `Async` as **prefix** (`AsyncEnumerable`, `IAsyncDisposable`, `AsyncLocal`, `AsyncCallback`, `AsyncTaskMethodBuilder`, `IAsyncResult`); no BCL type uses an `Async` suffix. A type-suffix `Async` would read as a method name to .NET developers. + +- **File-size-aware `initialCapacity` for FileStream helpers** (rejected): `DeserializeFromFileStreamAsync` could `Stat` the file and pre-allocate the buffer to file size, achieving "zero growth, single allocation" for the receive buffer. Rejected — defeats the streaming purpose: peak memory becomes O(file size) instead of O(chunk size). A 1-GB file would allocate 1 GB on the heap, eliminating the streaming win. The `BufferWriterChunkSize × 2` heuristic preserves O(chunk size) regardless of file size; the rare grow-path handles the unusual case where chunk-size is misconfigured. + +- **Composition: `AsyncPipeReaderInput` wrapping a kept `SegmentBufferReader`** (rejected): preserve `SegmentBufferReader` (sealed class) as the buffer primitive, build `AsyncPipeReaderInput` as a thin wrapper that adds Feed/DrainFromAsync API + `IBinaryInputBase`. Rejected — only marginally simpler than today's struct+class split; doesn't realize the "single primitive" goal; users see two types where one suffices. The pure merger is conceptually cleaner. + +- **`PipeReader`-inheriting type** (rejected): make `AsyncPipeReaderInput` derive from `System.IO.Pipelines.PipeReader`. Rejected — `PipeReader` is an abstract async-buffer reader contract; implementing it commits to all of `ReadAsync` / `AdvanceTo` / `CancelPendingRead` / `Complete` semantics, only ~30% of which apply to the AcBinary streaming use-case. The `IBinaryInputBase` contract is purpose-built for AcBinary deserialization (`Initialize` / `TryAdvanceSegment` / `Release`); inheriting `PipeReader` would force impedance-mismatched API surface for marginal pattern-match value. + +- **Place ADR at `AyCode.Services/docs/adr/`** (rejected): an earlier draft placed this ADR in the project-scoped folder as ADR-0002 (sibling of the decorator-feature-stack ADR). Rejected — the change touches both `AyCode.Core` (primary, primitive + helpers) and `AyCode.Services` (protocol delegation). Per the multi-project routing rule, cross-cutting ADRs live at the highest common ancestor — repo-root `docs/adr/`. The project-scoped folder remains for project-internal decisions. + +## Related + +- [`AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md`](../../AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md) — project-scoped umbrella for `AcBinaryHubProtocol` optional feature stack. ADR-0003's `AsyncPipeReaderInput` is the receive-side base primitive that the decorator ADR's per-chunk decorators (compression / encryption / signing) operate on top of in AsyncSegment mode. +- Send-side counterpart: `AsyncPipeWriterOutput` in `AyCode.Core/Serializers/Binaries/`. Unchanged by this ADR. +- Wire format: [`AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md`](../../AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md) documents the AsyncSegment chunked wire format (the invariant this ADR generalizes to NamedPipe + FileStream transports). +- Topic homes: [`AyCode.Core/docs/BINARY/README.md`](../../AyCode.Core/docs/BINARY/README.md) (Core-side primitive + helpers), [`AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md`](../../AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md) (Services-side protocol delegation). +- Number `0002` in this folder skipped: reserved by [`0001-user-bearer-token-flow.md`](0001-user-bearer-token-flow.md) Follow-ups for the forthcoming refresh-token flow ADR. +- Upcoming TODO entries: 6 in BIN + SBP topics — see Follow-up #1. diff --git a/docs/adr/README.md b/docs/adr/README.md index cba07d5..8c6053e 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -14,9 +14,10 @@ See [`.github/skills/adr-author/SKILL.md`](../../.github/skills/adr-author/SKILL ## Index -| ID | Title | Status | -|-------------------------------------------------|------------------------------------------------------------------------|-----------------------| -| [0001](0001-user-bearer-token-flow.md) | User bearer-token authentication flow (HTTP + SignalR + tag dispatch) | Accepted (2026-04-25) | +| ID | Title | Status | +|---------------------------------------------------------|----------------------------------------------------------------------------------------------------|-----------------------| +| [0001](0001-user-bearer-token-flow.md) | User bearer-token authentication flow (HTTP + SignalR + tag dispatch) | Accepted (2026-04-25) | +| [0003](0003-acbinary-streaming-receive-architecture.md) | AcBinary streaming receive — AsyncPipeReaderInput unified primitive and transport-agnostic helpers | Proposed (2026-04-27) | ## Related