Skip to content

feat: restart native ICE without reopening application streams - #488

Closed
0xcadams wants to merge 3 commits into
0xcadams/native-direct-upgradefrom
0xcadams/native-ice-restart
Closed

0xcadams wants to merge 3 commits into
0xcadams/native-direct-upgradefrom
0xcadams/native-ice-restart

Conversation

@0xcadams

Copy link
Copy Markdown
Member

What

Recover direct UDP after a failed ICE setup or lost path while keeping the existing native connection and its application streams on relay delivery.

Stacked on #487. The connector starts each attempt; the publisher replaces only its connectivity agent. The prepared pair, relay-attested source, serving deadline, application admission, and target socket remain owned by the existing session.

Why

One successful ICE negotiation is not enough for a long-lived native connection. An unreachable first candidate set or a changed UDP address must not leave direct delivery permanently unavailable, and retrying must not create a second application session or add serving time.

Changes

  • Add monotonically increasing, connection-local attempt numbers to the bounded connectivity message. Reject reused attempts and mismatched pair/delivery bindings.
  • Retain one signaling reader and at most one queued message per endpoint. Bound partial stream reads, join each old agent before replacement, and limit publisher agent creation even if offers arrive in a burst.
  • Retry failed setup, lost consent, and a direct path that stops passing packet probes after a five-second pause. Gather current candidates and fresh credentials on each attempt; late answers cannot install a carrier or restart the retry timer.
  • Exercise retry after an unreachable first answer, loss of an active ICE agent, and a newly signaled peer UDP address with the same target counter and inner connection.
  • Qualify retry through real control and the default publisher dialer: withhold the first publisher ICE checks until that attempt fails, continue application exchanges over relays, then require a second successful attempt and direct traffic while both relay UDP paths are dropped.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-10-28T04:03:08.831233Z bbb4464 New commits
🔒 Security Review ✅ Completed 2026-10-28T04:06:51.469786Z bbb4464 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@0xcadams

Copy link
Copy Markdown
Member Author

Local qualification on signed commit c70927e1: full Go race suite, public Go consumer, vet/staticcheck, formatting, and generation comparison passed. Real ICE race cases cover failed setup, active-agent loss, changed peer UDP address, attempt replay, invalid binding/candidates, partial signaling, and shared-socket lifetime. The real-control default-dialer retry proof passed under race in 30.264s: twenty exchanges retained one target counter, including relay exchanges while the first ICE attempt was withheld, a required second attempt, and direct delivery with both relay UDP paths dropped. The default QUIC and TLS/TCP direct consumer cases also passed. Base #487 has a separate CI failure in the installed Drizzle public case; its subprocess diagnostics and Linux reproduction are being investigated. This receipt does not mark the coordinated cutover complete.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

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: ba747cb3c9

ℹ️ 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 on lines 277 to +278
if packets.ActivePath() == Direct {
next = ConnectivityDirect
next, lastDirect = ConnectivityDirect, now

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Base the restart timeout on direct-path probe health

When both relays are unavailable and direct packet probes stop succeeding while ICE consent still succeeds, Endpoint.ActivePath() can remain Direct: Endpoint.chooseLocked() only updates active when it finds a live path and leaves the previous selection unchanged otherwise. This branch therefore continually refreshes lastDirect despite the failed probes, preventing the new restart timeout from firing. The retained application connection can then reach its QUIC idle timeout instead of attempting recovery. Check the direct path's current probe health rather than treating its selected index as proof of connectivity.

Useful? React with 👍 / 👎.

@0xcadams

Copy link
Copy Markdown
Member Author

Follow-up bbb4464c replaces the timing-sensitive warm-control budget with a held-control proof. The fixture first observes an idle run-plus-visitor-token refresh, then holds control requests and drains in-flight forwarding before warm IPC and real HTTP/PostgreSQL traffic. Both complete while the control counters remain fixed. The pinned Node 24 artifact target passed on macOS in 81.797s; its evidence records a blocked background /run request and zero forwarded warm-control work. Full Go race, vet/staticcheck, type/lint checks in the artifact target, and formatting passed. The commit also retains side/code/cause diagnostics for the earlier ICE remapping cleanup failure. Ten isolated runs, CPU scheduling variants, the peer-package suite, and the broader race run passed, but they do not explain that original failure.

@0xcadams

Copy link
Copy Markdown
Member Author

superseded by the replacement cutover tracked in #499 (replacement stack #491). closing this original PR at the author's request so only the replacement review remains active. the original branch, commits, reviews and failure evidence are preserved; no branch is deleted. outstanding integration and validation remain tracked in the replacement, not waived by this closure.

@0xcadams 0xcadams closed this Oct 29, 2026
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