Skip to content

Commit a307917

Browse files
test: harden fetch fixture synchronization
* docs: design deterministic fetch fixture test * docs: plan fetch fixture test hardening * test: harden fetch fixture synchronization
1 parent 9967c5c commit a307917

4 files changed

Lines changed: 211 additions & 16 deletions

File tree

crates/cli/tests/cli.rs

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::fmt::Write as _;
22
use std::fs;
3-
use std::io::{Read, Write};
3+
use std::io::{Cursor, Read, Write};
44
use std::net::{TcpListener, TcpStream};
55
use std::path::{Path, PathBuf};
66
use std::process::Command;
@@ -955,10 +955,14 @@ const MAX_REQUEST_HEADERS_SIZE: usize = 32 * 1024;
955955

956956
fn read_request(stream: &mut TcpStream) -> String {
957957
stream.set_read_timeout(Some(Duration::from_secs(2))).unwrap();
958+
read_request_from(stream)
959+
}
960+
961+
fn read_request_from<R: Read>(reader: &mut R) -> String {
958962
let mut request = Vec::with_capacity(512);
959963
while !request.ends_with(b"\r\n\r\n") {
960964
let mut byte = [0_u8; 1];
961-
stream.read_exact(&mut byte).unwrap();
965+
reader.read_exact(&mut byte).unwrap();
962966
request.push(byte[0]);
963967
assert!(
964968
request.len() <= MAX_REQUEST_HEADERS_SIZE,
@@ -971,29 +975,51 @@ fn read_request(stream: &mut TcpStream) -> String {
971975
request_line.split_whitespace().nth(1).unwrap_or_default().to_string()
972976
}
973977

978+
struct StagedRequest {
979+
request_line: Cursor<Vec<u8>>,
980+
headers: Cursor<Vec<u8>>,
981+
waiting_sender: Option<mpsc::Sender<()>>,
982+
release_receiver: Receiver<()>,
983+
}
984+
985+
impl Read for StagedRequest {
986+
fn read(&mut self, buffer: &mut [u8]) -> std::io::Result<usize> {
987+
let read = self.request_line.read(buffer)?;
988+
if read > 0 {
989+
return Ok(read);
990+
}
991+
992+
if let Some(sender) = self.waiting_sender.take() {
993+
sender.send(()).unwrap();
994+
self.release_receiver.recv().unwrap();
995+
}
996+
997+
self.headers.read(buffer)
998+
}
999+
}
1000+
9741001
#[test]
9751002
fn read_request_waits_for_complete_headers() {
976-
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
977-
let address = listener.local_addr().unwrap();
978-
let (accepted_sender, accepted_receiver) = mpsc::channel();
1003+
let (waiting_sender, waiting_receiver) = mpsc::channel();
1004+
let (release_sender, release_receiver) = mpsc::channel();
9791005
let (path_sender, path_receiver) = mpsc::channel();
1006+
let mut request = StagedRequest {
1007+
request_line: Cursor::new(b"GET /complete HTTP/1.1\r\n".to_vec()),
1008+
headers: Cursor::new(b"Host: localhost\r\nConnection: close\r\n\r\n".to_vec()),
1009+
waiting_sender: Some(waiting_sender),
1010+
release_receiver,
1011+
};
9801012
let server = thread::spawn(move || {
981-
let (mut stream, _) = listener.accept().unwrap();
982-
accepted_sender.send(()).unwrap();
983-
path_sender.send(read_request(&mut stream)).unwrap();
1013+
path_sender.send(read_request_from(&mut request)).unwrap();
9841014
});
9851015

986-
let mut client = TcpStream::connect(address).unwrap();
987-
accepted_receiver.recv_timeout(Duration::from_secs(2)).unwrap();
988-
client.write_all(b"GET /complete HTTP/1.1\r\n").unwrap();
989-
client.flush().unwrap();
990-
1016+
waiting_receiver.recv_timeout(Duration::from_secs(2)).unwrap();
9911017
assert!(
992-
path_receiver.recv_timeout(Duration::from_millis(100)).is_err(),
1018+
matches!(path_receiver.try_recv(), Err(mpsc::TryRecvError::Empty)),
9931019
"request completed before the header terminator"
9941020
);
9951021

996-
client.write_all(b"Host: localhost\r\nConnection: close\r\n\r\n").unwrap();
1022+
release_sender.send(()).unwrap();
9971023
assert_eq!(path_receiver.recv_timeout(Duration::from_secs(2)).unwrap(), "/complete");
9981024
server.join().unwrap();
9991025
}
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
# Fetch Fixture Test Hardening Implementation Plan
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Replace the scheduler-sensitive HTTP fixture regression test with deterministic staged input and correct the prior plan's expected file scope.
6+
7+
**Architecture:** Keep `read_request` responsible for the `TcpStream` timeout and delegate header consumption to a helper generic over `Read`. Exercise that helper with a staged reader that signals and blocks exactly when more bytes are requested after the request line.
8+
9+
**Tech Stack:** Rust 2024, standard library I/O and channels, Cargo integration tests, pnpm repository scripts.
10+
11+
## Global Constraints
12+
13+
- Preserve the existing 32 KiB request-header bound and two-second socket timeout.
14+
- Add no dependencies.
15+
- Keep production HTTP fixture behavior unchanged.
16+
- Use signed conventional commits without attribution.
17+
18+
---
19+
20+
### Task 1: Deterministic complete-header regression test
21+
22+
**Files:**
23+
- Modify: `crates/cli/tests/cli.rs:1-10,955-999`
24+
25+
**Interfaces:**
26+
- Consumes: `std::io::Read`, `std::sync::mpsc`, and the existing `MAX_REQUEST_HEADERS_SIZE` constant.
27+
- Produces: `read_request_from<R: Read>(reader: &mut R) -> String` and a staged-reader regression test.
28+
29+
- [ ] **Step 1: Extract the reader-generic helper without changing behavior**
30+
31+
Keep socket configuration in `read_request` and move the existing complete-header loop and path extraction into:
32+
33+
```rust
34+
fn read_request(stream: &mut TcpStream) -> String {
35+
stream.set_read_timeout(Some(Duration::from_secs(2))).unwrap();
36+
read_request_from(stream)
37+
}
38+
39+
fn read_request_from<R: Read>(reader: &mut R) -> String {
40+
let mut request = Vec::with_capacity(512);
41+
while !request.ends_with(b"\r\n\r\n") {
42+
let mut byte = [0_u8; 1];
43+
reader.read_exact(&mut byte).unwrap();
44+
request.push(byte[0]);
45+
assert!(
46+
request.len() <= MAX_REQUEST_HEADERS_SIZE,
47+
"HTTP request headers are unexpectedly large"
48+
);
49+
}
50+
51+
let request_line = request.split(|byte| *byte == b'\n').next().unwrap_or_default();
52+
let request_line = String::from_utf8_lossy(request_line);
53+
request_line.split_whitespace().nth(1).unwrap_or_default().to_string()
54+
}
55+
```
56+
57+
- [ ] **Step 2: Add a staged `Read` implementation and replace the timeout-based test**
58+
59+
Use `Cursor<Vec<u8>>` for the two stages. On the first read after the request line is exhausted, signal the test and wait for explicit release before exposing the remaining headers:
60+
61+
```rust
62+
struct StagedRequest {
63+
request_line: Cursor<Vec<u8>>,
64+
headers: Cursor<Vec<u8>>,
65+
waiting_sender: Option<mpsc::Sender<()>>,
66+
release_receiver: Receiver<()>,
67+
}
68+
69+
impl Read for StagedRequest {
70+
fn read(&mut self, buffer: &mut [u8]) -> std::io::Result<usize> {
71+
let read = self.request_line.read(buffer)?;
72+
if read > 0 {
73+
return Ok(read);
74+
}
75+
76+
if let Some(sender) = self.waiting_sender.take() {
77+
sender.send(()).map_err(std::io::Error::other)?;
78+
self.release_receiver.recv().map_err(std::io::Error::other)?;
79+
}
80+
81+
self.headers.read(buffer)
82+
}
83+
}
84+
```
85+
86+
Spawn `read_request_from` on a worker thread. Wait for the staged reader's signal, assert `path_receiver.try_recv()` returns `mpsc::TryRecvError::Empty`, release the remaining headers, and assert the parsed path is `/complete`.
87+
88+
- [ ] **Step 3: Prove the regression test detects request-line-only behavior**
89+
90+
Temporarily change the helper loop terminator to stop after the first newline. Run:
91+
92+
```bash
93+
cargo test -p srcmap-cli --test cli read_request_waits_for_complete_headers -- --exact
94+
```
95+
96+
Expected: FAIL because the worker returns before the staged reader requests the remaining headers. Restore complete-header reading immediately afterward.
97+
98+
- [ ] **Step 4: Verify the focused behavior**
99+
100+
Run:
101+
102+
```bash
103+
cargo test -p srcmap-cli --test cli read_request_waits_for_complete_headers -- --exact
104+
cargo test -p srcmap-cli --test cli fetch
105+
```
106+
107+
Expected: both commands exit successfully.
108+
109+
### Task 2: Documentation scope and repository verification
110+
111+
**Files:**
112+
- Modify: `docs/superpowers/plans/2026-07-14-js-dependencies-fetch-fixture.md:217`
113+
114+
**Interfaces:**
115+
- Consumes: the merged file list from PR #75.
116+
- Produces: an accurate expected-scope statement naming the intentional worktree ignore entry.
117+
118+
- [ ] **Step 1: Correct the expected final scope**
119+
120+
Replace the stale sentence with:
121+
122+
```markdown
123+
Expected: only the design, plan, fixture test helper, dependency manifests, lockfiles, and the intentional `.worktrees/` ignore entry are changed; commits have valid signatures.
124+
```
125+
126+
- [ ] **Step 2: Run full verification with bounded logs**
127+
128+
Run:
129+
130+
```bash
131+
pnpm check > /tmp/srcmap-fetch-hardening-check.log 2>&1
132+
pnpm test > /tmp/srcmap-fetch-hardening-test.log 2>&1
133+
git diff --check origin/main...HEAD
134+
```
135+
136+
Expected: all commands exit successfully.
137+
138+
- [ ] **Step 3: Review and commit the intended scope**
139+
140+
Review `git status --short`, `git diff`, and staged files. Commit the test and documentation changes with:
141+
142+
```bash
143+
git commit -S -m "test: harden fetch fixture synchronization"
144+
```
145+
146+
- [ ] **Step 4: Push and create the pull request**
147+
148+
Push `codex/harden-fetch-fixture-test`, create a ready pull request targeting `main`, require all checks, and verify the exact merge commit after merging.

docs/superpowers/plans/2026-07-14-js-dependencies-fetch-fixture.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ git status --short --branch
214214
git log --show-signature --format=fuller origin/main..HEAD
215215
```
216216

217-
Expected: only the design, plan, fixture test helper, dependency manifest, and lockfile are changed; commits have valid signatures.
217+
Expected: only the design, plan, fixture test helper, dependency manifests, lockfiles, and the intentional `.worktrees/` ignore entry are changed; commits have valid signatures.
218218

219219
- [ ] **Step 3: Push and create a ready pull request**
220220

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# Fetch Fixture Test Hardening Design
2+
3+
## Goal
4+
5+
Make the regression test for complete HTTP request header consumption deterministic, and correct the implementation plan so its expected diff matches the work that was merged.
6+
7+
## Design
8+
9+
Keep the production `TcpStream` wrapper responsible for configuring its read timeout. Extract the bounded request-header reading and path parsing into a helper generic over `Read`.
10+
11+
Test the helper with a staged reader. The reader provides the request line, signals when the helper requests more input, blocks until the test releases the remaining headers, and then provides the header terminator. The test can use channel state instead of elapsed time to prove that parsing does not complete after the request line alone.
12+
13+
The existing real TCP fetch tests continue to exercise the wrapper and network behavior. The new unit-level regression test isolates only the synchronization-sensitive header consumption contract.
14+
15+
## Documentation
16+
17+
Update the existing implementation plan's expected final scope to include the intentional `.gitignore` change for `.worktrees/`.
18+
19+
## Verification
20+
21+
Prove the regression test by temporarily restoring request-line-only behavior and observing the focused test fail, then restore complete-header reading and observe it pass. Run the focused CLI fetch tests, the full repository quality gate, and the full test suite before publishing the change.

0 commit comments

Comments
 (0)