Skip to content

Commit 1291185

Browse files
st0o0st0o0
committed
TASK-038-001: Fix RedirectBidiStage — Transaction Guard + _inFlightCount Decrement
1 parent 54f02df commit 1291185

11 files changed

Lines changed: 1589 additions & 436 deletions

File tree

‎.maggus/COMMIT.md‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,17 @@
1-
# TASK-023-009: Verification Gate – Integration Test Depth Feature 023
1+
# TASK-038-001: Fix RedirectBidiStage — Transaction Guard + _inFlightCount Decrement
22

3-
Fixed `Error-H10-003` test and aligned H10 decoder behavior with RFC 1945 §7.2.2:
4-
Content-Length mismatch on abrupt close now throws instead of returning truncated body.
3+
Added `_redirectTransactionActive` guard to `RedirectBidiStage` and corrected `_inFlightCount`
4+
decrement ordering across redirect hops so that `TryCompleteIfDone` Case 2 fires reliably.
55

6-
- Updated `Http10Decoder`: added `_pendingContentLength` tracking and `IsWaitingForContentLength`
7-
property; `TryDecodeEof` throws `HttpDecoderException` on Content-Length mismatch
8-
(only when `body.Length > 0`, preserving HEAD response semantics)
9-
- Updated `Http10DecoderStage`: abrupt close (`TlsCloseKind.AbruptClose`) with
10-
`IsWaitingForContentLength` → `FailStage`; `onUpstreamFinish` catches decoder exceptions
11-
- Updated `10_DecoderStateTests.cs`: ST-001/ST-004 use HTTP/0.9 (body-until-EOF) pattern;
12-
added ST-014 for Content-Length mismatch throw behavior
13-
- Updated `ErrorHandlingIntegrationTests.cs`: Error-H10-003 updated to expect exception
14-
instead of truncated body (new correct behavior)
6+
- Added `_redirectTransactionActive` bool field to `Logic` (mirrors `RetryBidiStage` pattern)
7+
- Redirect path in `onPush(InResponse)` now uses atomic transaction:
8+
enqueue → TryEmitRedirect → `_inFlightCount--` → TryPullResponse → guard off → TryCompleteIfDone
9+
- `TryCompleteIfDone()` returns early when `_redirectTransactionActive == true`, preventing
10+
premature Out1 completion mid-redirect transaction
11+
- Catch blocks for `ProtocolDowngrade` and `MaxRedirects` do NOT set the guard (non-redirect paths)
12+
- Added `BidiFlowFeedbackRaceTests.cs` with 4 regression tests (BidiLoop-001 through BidiLoop-004)
1513

1614
Verified:
1715
- Build: 0 errors, 0 warnings
18-
- Unit tests: 3652/3652 pass
19-
- Stream tests: 810/810 pass
20-
- H10 ErrorHandling: 17/17 pass
21-
- H10 Resilience: 8/8 pass
16+
- Stream tests: 835/835 pass (BidiLoop-001..004 all green)
17+
- Roslyn get_diagnostics on RedirectBidiStage.cs: 0 errors

‎.maggus/features/feature_023_completed.md‎

Lines changed: 0 additions & 399 deletions
This file was deleted.

‎.maggus/features/feature_038.md‎

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
<!-- maggus-id: c8e2153f-8a3d-4426-9fe3-e1772768458a -->
2+
# Feature 038: Fix BidiFlow Feedback Race + Http1XCorrelation Back-Pressure
3+
4+
## Introduction
5+
6+
Two correctness bugs in the Streams layer that both cause request-ordering violations under certain conditions.
7+
8+
**Bug A — `RedirectBidiStage` `_inFlightCount` leak:** Every redirect hop inflates `_inFlightCount` by 1 because the redirect path in `onPush(InResponse)` does not decrement the counter for the consumed original request, while `TryEmitRedirect` still increments it for the new redirect request. The result is that `TryCompleteIfDone` Case 2 (all in-flight resolved) never fires — the stream can only complete via Case 1 (InResponse upstream closed). In production, where the response inlet is long-lived, this risks the feature BidiFlow chain not completing cleanly after a redirect chain finishes. No `_retryTransactionActive`-style guard exists to protect the intermediate state.
9+
10+
**Bug C — `Http1XCorrelationStage` back-pressure violation:** `_pipelineUnlocked` is permanently set to `true` after the first request-response pair, allowing unlimited requests to queue in `_pending` without waiting for responses. The `InReset` inlet intended to reset this flag is wired to `Source.Empty<NotUsed>()` in both engines and never fires. This violates RFC 9112 §9 (strictly ordered request-response pairs on a single HTTP/1.x connection).
11+
12+
### Architecture Context
13+
14+
- **Vision alignment:** Correctness is a prerequisite for performance and reliability.
15+
- **Components involved:** `Streams/Stages/Features/RedirectBidiStage.cs`, `Streams/Stages/Routing/Http1XCorrelationStage.cs`, `Streams/Http10Engine.cs`, `Streams/Http11Engine.cs`, `StreamTests/Concurrency/BidiFlowFeedbackRaceTests.cs` (already written, untracked).
16+
- **No new components introduced** — pure correctness fixes within existing stages.
17+
- **Architecture boundary respected:** No changes to Protocol, Transport, or Client layers.
18+
19+
---
20+
21+
## Goals
22+
23+
- `RedirectBidiStage._inFlightCount` stays stable across redirect hops — Case 2 completion works correctly.
24+
- `RetryBidiStage` and `RedirectBidiStage` share the same transaction-guard pattern for in-flight tracking.
25+
- `Http1XCorrelationStage` enforces strict one-request-in-flight semantics per RFC 9112 §9.
26+
- `InReset` inlet and all associated dead code removed from `Http1XCorrelationStage`, `Http10Engine`, `Http11Engine`.
27+
- `BidiFlowFeedbackRaceTests.cs` (already written) passes and is committed as regression coverage for Bug A.
28+
- Four new back-pressure tests for Bug C pass deterministically.
29+
30+
---
31+
32+
## Tasks
33+
34+
### TASK-038-001: Fix RedirectBidiStage — Transaction Guard + _inFlightCount Decrement
35+
36+
**Description:** As the HTTP runtime, I want `RedirectBidiStage` to manage `_inFlightCount` correctly across redirect hops so that `TryCompleteIfDone` Case 2 fires reliably and the stage matches the proven `RetryBidiStage` pattern.
37+
38+
**Token Estimate:** ~40k tokens
39+
**Predecessors:** none
40+
**Successors:** TASK-038-005
41+
**Parallel:** yes — can run alongside TASK-038-002
42+
43+
**Acceptance Criteria:**
44+
- [x] New field `_redirectTransactionActive` (bool) added to `Logic`.
45+
- [x] Redirect path in `onPush(InResponse)` updated to:
46+
```csharp
47+
_redirectTransactionActive = true;
48+
_readyRedirects.Enqueue(newRequest);
49+
TryEmitRedirect();
50+
_inFlightCount--;
51+
TryPullResponse();
52+
_redirectTransactionActive = false;
53+
TryCompleteIfDone();
54+
```
55+
- [x] `TryCompleteIfDone()` has `if (_redirectTransactionActive) return;` as its first statement.
56+
- [x] Same guard applied in the `ProtocolDowngrade` and `MaxRedirects` catch blocks (both already call `TryCompleteIfDone` via the non-redirect path — verify no guard needed there, i.e., `_redirectTransactionActive` is only ever `true` in the redirect sub-path).
57+
- [x] `BidiFlowFeedbackRaceTests.cs` (untracked file at `src/TurboHttp.StreamTests/Concurrency/BidiFlowFeedbackRaceTests.cs`) is included in the `TurboHttp.StreamTests` project and all 4 tests pass:
58+
- `BidiLoop-001` — 200 concurrent redirect instances all complete
59+
- `BidiLoop-002` — retry storm bounded by MaxRetries
60+
- `BidiLoop-003` — In1 closed during retry cycle
61+
- `BidiLoop-004` — redirect + slow downstream liveness
62+
- [x] `Roslyn MCP get_diagnostics` on `RedirectBidiStage.cs` returns zero errors.
63+
- [x] No changes to `RetryBidiStage.cs`, `CacheBidiStage.cs`, or any other stage.
64+
65+
---
66+
67+
### TASK-038-002: Rewrite Http1XCorrelationStage — Strict One-Request-In-Flight
68+
69+
**Description:** As the HTTP runtime, I want `Http1XCorrelationStage` to enforce strict HTTP/1.x back-pressure so that only one request is in-flight at a time on a connection, matching RFC 9112 §9.
70+
71+
**Token Estimate:** ~80k tokens
72+
**Predecessors:** none
73+
**Successors:** TASK-038-003, TASK-038-004
74+
**Parallel:** yes — can run alongside TASK-038-001
75+
76+
**Acceptance Criteria:**
77+
- [ ] `Http1XCorrelationShape` loses the `InReset` inlet: shape becomes 2 inlets (`InRequest`, `InResponse`) + 2 outlets (`OutResponse`, `OutControl`).
78+
- [ ] `_inReset` field removed from `Http1XCorrelationStage`.
79+
- [ ] Internal `Logic` state simplified to:
80+
- `_inFlightRequest`: `HttpRequestMessage?` — the one pending request, or `null`.
81+
- `_requestUpstreamFinished`, `_responseUpstreamFinished`: completion booleans (unchanged if present).
82+
- [ ] `_pending` queue, `_waiting` queue, and `_pipelineUnlocked` flag are all removed.
83+
- [ ] Back-pressure contract enforced:
84+
- A new request is pulled from `InRequest` only when `_inFlightRequest == null`.
85+
- When a request arrives on `InRequest`, it is stored in `_inFlightRequest` and `StreamAcquireItem` is emitted on `OutControl`. The stage does NOT pull another request.
86+
- When a response arrives on `InResponse`, it is matched to `_inFlightRequest`, pushed on `OutResponse`, and `_inFlightRequest` is set to `null`. Only then is the next request pulled.
87+
- [ ] `PreStart()` removed (no longer needs to pull `_inReset`).
88+
- [ ] `onPull(OutResponse)` pulls `InResponse` and pulls `InRequest` only if `_inFlightRequest == null`.
89+
- [ ] Completion logic: stage completes only when both upstreams finish and `_inFlightRequest == null`.
90+
- [ ] `Roslyn MCP get_diagnostics` on `Http1XCorrelationStage.cs` returns zero errors.
91+
- [ ] Existing stream tests that cover correlation still pass (run `dotnet test --project TurboHttp.StreamTests/TurboHttp.StreamTests.csproj`).
92+
93+
---
94+
95+
### TASK-038-003: Remove Dead InReset Wiring from Http10Engine and Http11Engine
96+
97+
**Description:** As a developer, I want the dead `Source.Empty<NotUsed>()` InReset wiring removed from both engine graphs so that no misleading dead code remains.
98+
99+
**Token Estimate:** ~20k tokens
100+
**Predecessors:** TASK-038-002
101+
**Successors:** TASK-038-004
102+
**Parallel:** no
103+
**Model:** haiku
104+
105+
**Acceptance Criteria:**
106+
- [ ] `Http10Engine.cs`: `var resetSrc = b.Add(Source.Empty<NotUsed>());` and `b.From(resetSrc).To(correlation.InReset);` removed.
107+
- [ ] `Http11Engine.cs`: same two lines removed.
108+
- [ ] `Grep` for `InReset`, `_inReset` across the solution returns zero results.
109+
- [ ] `dotnet build --configuration Release ./src/TurboHttp.sln` succeeds with zero errors.
110+
111+
---
112+
113+
### TASK-038-004: Add Http1XCorrelation Back-Pressure Stream Tests
114+
115+
**Description:** As a developer, I want deterministic stream tests that verify the one-request-at-a-time back-pressure contract so that regressions are caught in CI.
116+
117+
**Token Estimate:** ~70k tokens
118+
**Predecessors:** TASK-038-002, TASK-038-003
119+
**Successors:** TASK-038-005
120+
**Parallel:** no
121+
122+
**Acceptance Criteria:**
123+
- [ ] Test file: `TurboHttp.StreamTests/RFC9112/08_Http1XCorrelationBackPressureTests.cs`.
124+
- [ ] **Test `bp-001` — Serial ordering:** Send 3 requests sequentially. Verify each response is delivered in FIFO order and no second request is pushed to the encoder before the first response arrives.
125+
- [ ] **Test `bp-002` — Back-pressure gate:** Provide 2 requests simultaneously at `InRequest`. Verify only the first request flows to `OutControl` (`StreamAcquireItem` emitted once). The second request is NOT pulled until the first response is delivered on `InResponse`.
126+
- [ ] **Test `bp-003` — Upstream completion mid-flight:** Send 1 request, complete `InRequest` upstream before response arrives. Verify stage does not complete prematurely; stage completes only after the response is forwarded.
127+
- [ ] **Test `bp-004` — Response without in-flight request:** Regression guard — verify defined behavior (error or ignore) when a response arrives with `_inFlightRequest == null`.
128+
- [ ] Each test uses `DisplayName("RFC9112-correlation-bp-NNN: ...")` and `[Fact(Timeout = 5000)]`.
129+
- [ ] No `Thread.Sleep`. Akka.Streams test probes used for all synchronization.
130+
- [ ] All 4 tests green on a clean run.
131+
132+
---
133+
134+
### TASK-038-005: Verification Gate
135+
136+
**Description:** As the build system, I want a full clean build and test run to confirm feature 038 leaves the codebase green.
137+
138+
**Token Estimate:** ~15k tokens
139+
**Predecessors:** TASK-038-001, TASK-038-003, TASK-038-004
140+
**Successors:** none
141+
**Parallel:** no
142+
**Model:** haiku
143+
144+
**Acceptance Criteria:**
145+
- [ ] `dotnet build --configuration Release ./src/TurboHttp.sln` — zero errors, zero new warnings.
146+
- [ ] `dotnet test --project TurboHttp.StreamTests/TurboHttp.StreamTests.csproj` — all tests pass.
147+
- [ ] `dotnet test --project TurboHttp.Tests/TurboHttp.Tests.csproj` — all tests pass.
148+
- [ ] `dotnet test --project TurboHttp.IntegrationTests/TurboHttp.IntegrationTests.csproj` — green (pre-existing failures only, documented).
149+
- [ ] `Grep` for `_pipelineUnlocked`, `_inReset`, `InReset`, `_pending` in `Http1XCorrelationStage.cs` returns zero results.
150+
- [ ] `Grep` for `Source.Empty<NotUsed>()` in `Http10Engine.cs` and `Http11Engine.cs` returns zero results (or only unrelated usages).
151+
- [ ] `BidiLoop-001` through `BidiLoop-004` pass in `BidiFlowFeedbackRaceTests`.
152+
- [ ] `bp-001` through `bp-004` pass in `Http1XCorrelationBackPressureTests`.
153+
154+
---
155+
156+
## Task Dependency Graph
157+
158+
```
159+
TASK-038-001 ──────────────────────────────────────────────┐
160+
▼
161+
TASK-038-002 ──→ TASK-038-003 ──→ TASK-038-004 ──→ TASK-038-005
162+
```
163+
164+
| Task | Estimate | Predecessors | Parallel | Model |
165+
|------|----------|--------------|----------|-------|
166+
| TASK-038-001 | ~40k | none | yes (with 002) | — |
167+
| TASK-038-002 | ~80k | none | yes (with 001) | — |
168+
| TASK-038-003 | ~20k | 002 | no | haiku |
169+
| TASK-038-004 | ~70k | 002, 003 | no | — |
170+
| TASK-038-005 | ~15k | 001, 003, 004 | no | haiku |
171+
172+
**Total estimated tokens:** ~225k
173+
174+
---
175+
176+
## Functional Requirements
177+
178+
- **FR-1:** `RedirectBidiStage._inFlightCount` MUST be decremented exactly once per consumed response (including redirect responses). A redirect response consumed internally must decrement the counter before `TryEmitRedirect` increments it for the outgoing redirect request.
179+
- **FR-2:** `RedirectBidiStage.TryCompleteIfDone()` MUST return early when `_redirectTransactionActive == true`, matching the `RetryBidiStage._retryTransactionActive` pattern.
180+
- **FR-3:** `Http1XCorrelationStage` MUST NOT pull a second request from `InRequest` until the first response has been pushed on `OutResponse`.
181+
- **FR-4:** `Http1XCorrelationStage` shape MUST have exactly 2 inlets and 2 outlets after the fix. `InReset` is removed.
182+
- **FR-5:** `Http1XCorrelationStage` MUST emit exactly one `StreamAcquireItem` on `OutControl` per in-flight request.
183+
- **FR-6:** Stage completion: `Http1XCorrelationStage` MAY complete only when both `InRequest` and `InResponse` are finished AND `_inFlightRequest == null`.
184+
- **FR-7:** No mutable queue collections (`Queue<T>`) remain in `Http1XCorrelationStage.Logic`.
185+
186+
---
187+
188+
## Non-Goals
189+
190+
- No HTTP/1.1 pipelining support — strict serial order is the correct default.
191+
- No changes to `ConnectionReuseStage`, `ExtractOptionsStage`, or `ConnectionStage`.
192+
- No changes to `RetryBidiStage`, `CacheBidiStage`, or any other feature stage.
193+
- No Protocol or Transport layer changes.
194+
- No performance benchmarks — these are correctness fixes.
195+
- No changes to HTTP/2 or HTTP/3 correlation stages.
196+
197+
---
198+
199+
## Technical Considerations
200+
201+
- **Akka.Streams single-threaded-per-stage guarantee:** No synchronization primitives needed inside stage logic — `OnPush`/`OnPull` handlers run sequentially.
202+
- **`_redirectTransactionActive` scope:** The guard only needs to be active during the redirect sub-path of `onPush(InResponse)`. The `ProtocolDowngrade` and `MaxRedirects` catch blocks follow the non-redirect (pass-through) path and do not need the guard.
203+
- **`StreamAcquireItem` emission:** With the new model, every request arrival emits one `StreamAcquireItem` (one in-flight at a time). Verify this does not break `ConnectionStage` handling — the stage already handles one `StreamAcquireItem` per request, so this is unchanged behavior.
204+
- **`BidiFlowFeedbackRaceTests.cs`:** Already fully written and untracked at `src/TurboHttp.StreamTests/Concurrency/BidiFlowFeedbackRaceTests.cs`. TASK-038-001 must include it in the project (add to `.csproj` if needed or verify it is auto-included via glob).
205+
206+
**Critical files:**
207+
- `src/TurboHttp/Streams/Stages/Features/RedirectBidiStage.cs` — Bug A fix
208+
- `src/TurboHttp/Streams/Stages/Routing/Http1XCorrelationStage.cs` — Bug C rewrite
209+
- `src/TurboHttp/Streams/Http10Engine.cs` — remove 2 lines
210+
- `src/TurboHttp/Streams/Http11Engine.cs` — remove 2 lines
211+
- `src/TurboHttp.StreamTests/Concurrency/BidiFlowFeedbackRaceTests.cs` — commit (already written)
212+
- `src/TurboHttp.StreamTests/RFC9112/08_Http1XCorrelationBackPressureTests.cs` — new file
213+
214+
---
215+
216+
## Success Metrics
217+
218+
- Zero `_redirectTransactionActive`-unguarded `TryCompleteIfDone` calls in the redirect sub-path.
219+
- Zero `_pipelineUnlocked` / `InReset` / queue references in `Http1XCorrelationStage.cs` after the fix.
220+
- All 4 `BidiLoop-*` and all 4 `bp-*` tests pass deterministically on 10 consecutive runs.
221+
- Full build green with zero new compiler warnings.
222+
223+
---
224+
225+
## Open Questions
226+
227+
*(none — all resolved during brainstorming)*

0 commit comments

Comments
 (0)