Skip to content

fix(firecracker): read API responses by framing, not EOF - #25

Merged
drewstone merged 1 commit into
mainfrom
fix/api-response-framed-read
Jul 3, 2026
Merged

drewstone merged 1 commit into
mainfrom
fix/api-response-framed-read

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

firecracker_request reads the API response with stream.read_to_end(...), i.e. it waits for the server to close the connection. Firecracker's API server never does: it holds the connection open even when the request sends Connection: close (measured on v1.6.0 and v1.12.0 — the response carries Connection: keep-alive and no half-close ever arrives; verified with nc -U against a live socket, EOF still absent after 8s).

Consequence: every API call blocks until the socket read timeout and then errors. wait_for_socket_ready burns its entire window on a single probe, so create_vm fails with firecracker api socket not ready within 5s against a perfectly healthy VMM — every real-Firecracker path in this crate is currently unusable (an strace -f shows FC binding its API socket ~5ms after spawn and answering GET / fine, while the client read never returns).

Fix

Read the response by its HTTP/1.1 framing instead of EOF:

  • headers up to the blank line (growth capped at 64 KiB, fail-loud on overflow),
  • then exactly Content-Length body bytes (absent header = empty body, which is how Firecracker's micro_http answers bodyless 204-style action replies),
  • Transfer-Encoding rejected loudly (micro_http never emits it).

No behavior change for callers: firecracker_request still returns the same parsed status/JSON.

Tests

  • read_http_response_returns_without_server_close — the named bug: a keep-alive peer that never closes; with the old read_to_end implementation this test hangs and fails on its 2s guard timeout, with the fix it returns immediately.
  • read_http_response_handles_missing_content_length — headers-only responses.

Verified end-to-end downstream (ai-agent-sandbox-blueprint warm-pool e2e, real FC v1.12 + /dev/kvm) with this branch as a [patch.crates-io]: cold create→running median 111.3ms, warm snapshot-restore claim median 32.8ms, n=5 — the same e2e fails at the first create_vm on published 0.4.0-alpha.2.

Suggest cutting 0.4.0-alpha.3 with this fix so downstream consumers can drop the patch.

Firecracker's API server holds connections open regardless of the
request's Connection: close (measured on v1.6.0 and v1.12.0: responses
carry Connection: keep-alive and the server never half-closes), so
read_to_end blocks until the socket read timeout on every request.
wait_for_socket_ready then burns its whole window on one probe and
create_vm fails with 'api socket not ready' against a healthy VMM —
every real-Firecracker code path is unusable.

Read the response by its HTTP/1.1 framing instead: headers to the blank
line (capped at 64 KiB), then exactly Content-Length body bytes (absent
means empty body; transfer-encoded responses are rejected loudly).

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 9b44868d

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-03T21:16:50Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 85.1s (2 bridge agents)
Total 85.1s

💰 Value — sound

Replaces the broken EOF-based response read with HTTP/1.1 framing (headers up to blank line, then Content-Length bytes); fixes every real-Firecracker path in the crate, no new deps, fits the existing hand-rolled HTTP pattern.

  • What it does: Adds read_http_response() that reads one HTTP/1.1 reply from the Firecracker API Unix socket by its framing: loop-read until the \r\n\r\n header terminator (growth-capped at 64 KiB, fail-loud on overflow), reject Transfer-Encoding, then read exactly Content-Length body bytes (0 when the header is absent). Replaces stream.read_to_end(&mut response) at firecracker.rs:1130, which waited for an EO
  • Goals it achieves: Make every Firecracker API call actually return instead of blocking until the read timeout. As written, firecracker_request (firecracker.rs:1067) blocks on read_to_end; wait_for_socket_ready (firecracker.rs:783) then burns its entire 5s window on a single probe and create_vm fails with 'api socket not ready' against a healthy VMM — so the whole real-FC path (create, pause/resume, snapshot cr
  • Assessment: Good change, in the grain of the codebase. The framing logic is the textbook-correct way to read an HTTP/1.1 response: header terminator first, then exactly Content-Length body bytes, with explicit rejection of chunked transfer-encoding (which FC's micro_http never emits) and safe handling of partial reads / overflow / mid-body EOF. It is narrowly scoped (one file, +134/-2), changes no caller's co
  • Better / existing approach: none — this is the right approach. I searched Cargo.toml and grepped src/ for httparse|hyper|ureq|reqwest|minreq|attohttpc (0 hits in deps; the only match was an unrelated comment in in_memory.rs). The crate is explicitly a 'pure-Rust primitive' whose Cargo.toml goes to lengths to justify even sha2 and base64; pulling in any HTTP crate for one read site would fight that grain. The request side is
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Load-bearing fix: replaces an EOF-terminated read that blocked until timeout on every Firecracker API call with a correct HTTP/1.1 framing reader, unblocking all 13 real-FC call sites; fits the codebase's existing hand-rolled HTTP pattern with no competing equivalent.

  • Integration: read_http_response is reached by exactly one caller — firecracker_request at src/adapters/firecracker.rs:1130 — which is the single chokepoint for every Firecracker API interaction in the crate (13 sites: machine-config, boot-source, rootfs, drives, vsock, network-interfaces, snapshot/load, snapshot/create, actions start/pause/resume, and the readiness probe at :788). The bug claim is verified
  • Fit with existing patterns: Consistent with the established pattern. The write side already hand-rolls HTTP/1.1 request strings at lines 1100-1110 and parses responses by string-splitting at lines 1136-1159; Cargo.toml pulls no HTTP library (no hyper/reqwest/ureq) and no memchr. The fix makes the read half as deliberate as the write half already is, in the same idiom. It does not compete with or duplicate any existing capa
  • Real-world viability: Holds up past the happy path. Each call opens a fresh UnixStream (line 1074), so no shared-state/concurrency concerns. The reader handles: header-terminator scan with a 64 KiB growth cap that fails loud (lines 91-99), EOF-before-headers (lines 100-105), Transfer-Encoding rejection (lines 116-122), Content-Length parse error propagation (lines 124-138), overflow via checked_add (lines 140-1
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Missing Content-Length silently truncates to empty body [robustness] ``

Lines 139 unwrap_or(0) means any future Firecracker response carrying a body without a Content-Length header would be silently truncated to empty and then return Ok(None) at line 1152 (empty-body branch). This is the documented micro_http behavior today and is correct for current FC, but a comment noting 'a non-empty body without Content-Length will be dropped' at the unwrap_or would make the assumption explicit for whoever reads this if Firecracker ever swaps HTTP servers. Does not gate shippin


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260703T212033Z

@drewstone
drewstone merged commit 37b9b74 into main Jul 3, 2026
4 checks passed
@drewstone
drewstone deleted the fix/api-response-framed-read branch July 3, 2026 21:21
drewstone added a commit that referenced this pull request Jul 3, 2026
Releases #25 (framed HTTP reads for the FC API — the server never closes
connections, so read_to_end hung every real-FC call).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants