Skip to content

feat(exec-server): add ACK-based Noise relay retries - #31457

Open
apanasenko-oai wants to merge 2 commits into
mainfrom
codex/reliable-messages-01-ack-retry
Open

feat(exec-server): add ACK-based Noise relay retries#31457
apanasenko-oai wants to merge 2 commits into
mainfrom
codex/reliable-messages-01-ack-retry

Conversation

@apanasenko-oai

@apanasenko-oai apanasenko-oai commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Why

Noise relay data currently assumes the physical rendezvous WebSocket will deliver every encrypted record. If rendezvous drops a record, the receiver's ordered Noise nonce frontier stalls and the JSON-RPC stream cannot recover. Retrying by encrypting again would consume a different Noise nonce, so retries must reuse the original ciphertext.

This adds the first V1 endpoint-owned reliability layer for a trusted but flaky rendezvous.

What Changed

  • Add per-stream reliable sender state with sequence numbers starting at 1, cumulative ack handling, ack = 0 as the empty sentinel, and u32 ack_bits selective acknowledgements for the 32-record receive window.
  • Retain up to 32 unacknowledged encrypted records / 2 MiB and retry one due record at a time after 500 ms using the exact cached ciphertext.
  • Teach both harness and executor Noise relay paths to piggyback cumulative/selective ACK state on data, emit standalone ACK frames, process peer ACKs, and defer ACK writes safely when the physical queue is full.
  • Preserve WebSocket keepalive responsiveness by draining queued Pong traffic before fragments, retries, or deferred ACK writes.
  • Start new logical streams with the Noise handshake directly, document stream_id as single-use per physical relay connection, and reject recently retired IDs so delayed cached ciphertext cannot reach a replacement stream.
  • Update receive ordering to reserve sequence 0, use a 32-record receive window, reject sequence 0, and expose cumulative/selective receive state.

Verification

  • just test -p codex-exec-server noise_relay remote_environment_retries_dropped_encrypted_frames passed with 26 tests, including selective ACK release, receive-bit shifting, and cumulative send-window coverage.
  • remote_environment_retries_dropped_encrypted_frames drops the first encrypted data frame in both directions and verifies that the same sequence and ciphertext payload are retransmitted without re-encryption.

Related issue: N/A (internal reliability design work).

@apanasenko-oai
apanasenko-oai marked this pull request as ready for review July 7, 2026 19:06

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e99be8cf65

ℹ️ 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".

Comment thread codex-rs/exec-server/src/noise_relay/harness.rs
Comment on lines +44 to +47
if seq == 0 {
return Err(ExecServerError::Protocol(
"Noise reliable data sequence zero is reserved".to_string(),
));

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.

P1 Badge Bump relay protocol before rejecting seq 0

In mixed-version or third-party v1 relay peers, the first post-handshake data frame is still sent as seq = 0 (that was the documented v1 sequence start and the old sender behavior), so this receiver now treats a valid old-v1 stream as a protocol error and resets before any JSON-RPC can complete. Because the frame version remains 1, either keep accepting the old initial sequence during a compatibility window or negotiate/bump the relay protocol before reserving zero.

AGENTS.md reference: AGENTS.md:L102-L110

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct for mixed old/new peers. This PR intentionally assumes the harness and executor roll together; reserving seq = 0 is the contract change that lets ack = 0 unambiguously mean “nothing received.” A compatibility window or protocol negotiation/version bump is out of scope for this V1 reliability change.

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