Skip to content

Preserve forwarder bytes across socket write timeouts - #988

Merged
AnthonyRonning merged 1 commit into
masterfrom
codex-forwarder-audit-maple
Sep 24, 2026
Merged

AnthonyRonning merged 1 commit into
masterfrom
codex-forwarder-audit-maple

Conversation

@AnthonyRonning

Copy link
Copy Markdown
Contributor

A socket write timeout could make the enclave traffic forwarder discard unsent bytes and read the next chunk. Track partial send progress and retain the remaining bytes across timeouts before receiving more data. Configure both socket polling timeouts before either forwarding worker starts.

Adds six offline regression tests for short writes, repeated timeouts, receive ordering, shutdown, zero-byte sends, real socket backpressure, and timeout initialization. The existing backend Nix CI job runs the suite through a new traffic-forwarder check.

Validation:

  • All six tests pass in the pinned backend environment.
  • Both byte-preservation tests fail against unchanged master by byte comparison; the real socket fixture receives 64,512 of 65,536 bytes before the fix and all bytes after it.
  • All five current-host Nix checks pass on aarch64-darwin.
  • Independent source and test review found no introduced blockers.

Linux VSOCK and deployed enclave behavior remain untested. The local tests establish the forwarding defect and its correction, not the cause of any particular production streaming failure. Separate EOF propagation, paired-worker cancellation, and logging-policy changes are outside this PR.

@AnthonyRonning

Copy link
Copy Markdown
Contributor Author

Review: no blockers, ready to merge.

Read-only review of head 65fd463 from a local checkout. Trial merge with current master (4e1408d) is clean; nothing merged since the base touches the forwarder, flake.nix, or entrypoint.sh.

Defect and fix

  • forward() called sendall() on a socket with a 1 s timeout. CPython treats that timeout as a total deadline for the whole call and reports no partial progress, and the loop caught socket.timeout and returned to recv(), so the unsent suffix was dropped under backpressure with no error logged.
  • The new send() loop over a memoryview is correct: a timeout from send() means nothing was written, so retrying the same view is safe; partial writes slice the view; a zero return raises and exits through the existing error path; the shutdown flag exits both loops.
  • Setting both timeouts in handle_connection() before either worker starts closes a real gap: the accepted TCP socket was blocking and the VSOCK socket still carried its 30 s connect timeout until the peer thread ran.

Verified

  • Head: 6/6 on host Python 3.13; 6/6 via the README recipe in the pinned dev shell (Python 3.13.14, worktree left clean); 6/6 on the aarch64-darwin check with nix build --rebuild.
  • Baseline: the same test file against unchanged master fails 3 exactly as described (ab vs abcdefNEXT; 64,512 of 65,536 bytes; timeout ordering).
  • CI run 35638757738 ran checks.x86_64-linux.traffic-forwarder green. opensecret_change_detection.py maps any nitro-toolkit/ edit to the nix and eif jobs, so the check gates future forwarder edits.
  • The script is copied verbatim into the EIF and started by entrypoint.sh for every outbound forwarder, so the fix lands with the next EIF/PCR.

Non-blocking

  • Shutdown exits the inner loop with bytes pending and logs nothing; a debug line with the dropped count would help later forensics.
  • A peer that stays open but never reads now pins the worker in the send loop instead of corrupting the stream. That is correct relay behaviour, but the client-side EOF is not observed while blocked; the deferred paired-worker cancellation is what would bound it.
  • Pre-existing, untouched: in finally, if source.shutdown(SHUT_RD) raises ENOTCONN the destination.shutdown(SHUT_WR) in the same try is skipped, so EOF reaches the peer only at close after the join (the deferred EOF-propagation item).
  • ScriptedSocket.sendall exists only so the suite can run against the old code and is dead once merged. Author's call whether to keep it.

Agree with the scope note: this establishes and fixes a real forwarding defect, not the cause of the production streaming drops.

Independent review by Claude (Fable 5.1), posted from Anthony's account.

@AnthonyRonning
AnthonyRonning merged commit 312f4f6 into master Sep 24, 2026
20 checks passed
@AnthonyRonning
AnthonyRonning deleted the codex-forwarder-audit-maple branch October 3, 2026 02:27
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.

1 participant