fix(serve): bound the whole request head, not just each read - #2684
Conversation
`readRequest` applied `requestReadTimeoutMilliseconds` per `recv` with no overall limit, so a client trickling one byte just inside that window never timed out and held its connection — and the cooperative-executor thread serving it — for as long as it kept sending. The Host allowlist and bearer-token check both run after the head is read, and over-cap connections are closed rather than queued, so a few such clients denied service to well-behaved ones entirely pre-auth. Track a monotonic start time and cap each wait at the remaining budget, failing the request once the overall deadline passes. A client that merely goes silent was already handled by the per-read timeout; this covers the one that keeps sending. Verified red→green on Linux: before, a legitimate client never got a slot within a 25s budget; after, it is served at 10.26s, matching the deadline.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fa5390003
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #if canImport(Glibc) | ||
| import Glibc | ||
| #elseif canImport(Musl) | ||
| import Musl |
There was a problem hiding this comment.
When macOS Swift tests run for this non-doc change, CodexBarLinuxTests is still declared unconditionally in Package.swift, so this new source file is compiled on macOS too. This import block has no Darwin branch, and the code below uses the Linux-only SOCK_STREAM.rawValue form; on the macOS compile path that fails before any tests can run. Please wrap the file in #if os(Linux) like the other Linux syscall tests or add a proper Darwin socket branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks — that's a real break, not a flake, and I should have caught it.
TestsLinux is declared unconditionally in Package.swift, so the file compiled on macOS where SOCK_STREAM.rawValue and the Glibc imports don't exist — both shards failed before any test ran. My local pipeline is Linux-in-Docker, so it couldn't see this.
Fixed in 009dbbf: wrapped the file in #if canImport(Glibc) || canImport(Musl), matching AntigravityProcessLauncherLinuxTests and the other syscall suites in that directory rather than inventing a new pattern.
Re-verified after the change — Linux suite still passes (10.26s, matching the 10s deadline), swiftformat --lint clean, swiftlint --strict 0 violations / 1814 files.
`TestsLinux` is declared unconditionally in Package.swift, so this file also compiles on macOS, where the raw socket calls (`SOCK_STREAM.rawValue`, Glibc-only imports) do not build and broke both macOS test shards. Wrap the file in `#if canImport(Glibc) || canImport(Musl)`, matching AntigravityProcessLauncherLinuxTests and the other syscall suites here.
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs changes before merge. Reviewed August 5, 2026, 9:56 PM ET / August 6, 2026, 01:56 UTC. ClawSweeper reviewWhat this changesThe PR adds a total deadline for one Merge readinessKeep open: the unchanged PR head still starts the total deadline only when its client task receives an executor worker, after the connection slot is acquired. Capture the deadline at acceptance and add a saturated-queue regression test before merge. Likely related people: steipete — initial local HTTP server author (high confidence). Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s local HTTP server accepts CLI health and control requests, limits concurrent connections, then parses and validates request headers before dispatching the handler. This change bounds pre-auth header reading so incomplete requests release a connection slot. flowchart LR
A[Client socket] --> B[Connection slot gate]
B --> C[Client task]
C --> D[Request-head reader]
D --> E{Complete before deadline?}
E -->|Yes| F[Host and token validation]
E -->|No| G[Close socket and release slot]
F --> H[HTTP handler response]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Make the request-head deadline run from socket acceptance through header completion, and prove that queued accepted clients release their slots within that single budget. Do we have a high-confidence way to reproduce the issue? Yes—current-main source shows slots are acquired before task scheduling, and the supplied real-server Linux transcript establishes the slow-trickle failure mode; this review did not execute tests under the read-only contract. Is this the best way to solve the issue? No—the total-read deadline is the right narrow fix, but it must start at socket acceptance rather than when a delayed client task begins reading. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4cdb349cbc57. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
The deadline test held executor threads for the full 10s production budget, which starved unrelated suites: CLIHooksWatchSleepLinuxTests asserts a stop signal is honored within 2s and instead measured 10.31s on CI. `.serialized` only orders cases within a suite, so it does not prevent that. Make the budget injectable on CLILocalHTTPServer, defaulting to the production value, and have the test pass 1500ms. Full suite drops from ~10.5s to ~2.0s. Verified the test still fails when the deadline logic is disabled (12.37s starvation), so the shorter budget did not weaken it, and ran the full suite 8x with no flake.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Landed. Verified before merge: bound math checked (monotonic clock, wraparound-safe elapsed, per-poll wait = min(5s, remaining), 16 KB + 10 s total regardless of trickle rate); Host allowlist and bearer auth confirmed to run only after the head read, so this closes the pre-auth slowloris hold. Status mapping stays within the server’s existing 400/403 vocabulary — right call for a loopback daemon. The Linux trickle test is genuinely adversarial and correctly platform-gated. Local: 89 macOS CLIServe tests pass on the branch; PR CI fully green incl. both Linux builds. Thanks @OfficialAbhinavSingh! |
2 participants
Closes #2683.
Problem
readRequestbounds eachrecvwithrequestReadTimeoutMilliseconds(5s) but nothing bounds the request head as a whole:A client that keeps trickling bytes just inside that window never trips the timeout, so it holds its connection — and the cooperative-executor thread serving it — for as long as it keeps sending.
This is pre-auth: the Host allowlist and the bearer-token check both run only after the head has been read. And over-cap connections are not queued —
connectionGate.tryAcquire()fails and the socket is closed immediately — so legitimate clients get dropped outright rather than waiting their turn.Worth noting: a client that simply goes silent is already handled correctly by the per-read timeout. The gap is specifically the client that keeps sending.
Fix
Track a monotonic start time, cap each wait at the remaining budget, and fail the request once the overall deadline passes. 10s ceiling, generous for a legitimate head but bounded.
Proof (Linux,
swift:6.3.3)The test boots the real
CLILocalHTTPServeron an ephemeral port, opensmaximumConnectionssockets that each send one header byte per second and never terminate the header block, then retries a well-behavedGET /healthagainst a 25s budget.Before:
After:
The 10.26s matches the deadline exactly. I reverted the fix and re-ran to confirm the test genuinely fails without it, rather than trusting a green run.
Full Linux suite 356/356,
swiftformat --lintclean,swiftlint --strict0 violations / 1814 files, andScripts/regenerate-codex-parser-hash.sh checkis current.Scope
Default bind is loopback, so in practice this is a local user wedging the daemon; it becomes network-reachable only with
--host 0.0.0.0, which already demands--allow-plain-httpand a token. I treated it as robustness hardening rather than a privilege-boundary issue — happy to be corrected on that framing.Deliberately out of scope
sendResponseis likewise a blockingsendloop with noSO_SNDTIMEO, so a client that never drains its receive window can stall a writer. Same class, separate fix; left out to keep this diff small. Happy to follow up if you want it.