[myleshorton]

Why

The reviews of the Rust port (getlantern/soar-rs#1), by Codex, Copilot, CodeRabbit and an automated security review, turned up bugs that the Go client and server share. Each was fixed and tested in Rust first. This PR ports the fixes.

Client

• Close lost the request. Conn.Close sent RESET as soon as the send buffer was empty. But an empty buffer means the data was acknowledged, not the FIN, so the server aborted the stream before dialing and the request was lost. RESET now waits for the FIN's ack.
• Discovery stayed stopped after a blocked resolver. It counted every path ever verified toward Want. The client never suspends its last usable path, so a blocked resolver kept counting. Now only healthy paths count: verified, usable, and not on a run of losses.
• Discovery paced at about one candidate a second. It adds one candidate per pass, but the run loop only woke on its one-second default. It now wakes when the pace allows the next one.
• The liveness clock stopped too early. An answer stopped it even with other queries still out, so if every path then died, the session was never presumed lost. It now restarts instead.
• Hello retries reached only 16 resolvers. They always revisited the first 16; they now go to the least recently tried first.
• The tail copy of an upload could exceed MaxInflight. It now checks the cap.
• newTxnLocked could spin forever. It retried random IDs with no exit, so if every DNS ID was taken it looped holding the client lock. MaxInflight is now capped at 1,024, and nothing is sent once 60,000 queries are pending. Every send path checks that ceiling: hellos, probes of unverified paths, fill and the tail copy.

Session and server

• A full control queue silently dropped frames. Callers had already counted them as sent, so a dropped MAX_STREAM_DATA stalls a stream for good. Overflow now ends the session (only a misbehaving peer can fill the queue), and exact duplicates are skipped. In Rust, this exact change caused a double release of the cross-session stream slot, which the security review caught, so the Go version includes that guard: maybeRemove gives back a slot only if the stream is still there, and peerStream returns its reserved slot if advertising ended the session.
• MaxStreams had no ceiling. A stream can queue up to three control frames, so a large MaxStreams could let legitimate traffic fill the control queue, which now ends the session. It's capped at 256 (a quarter of the queue) on both client and server.
• A malformed destination left waitOpened hanging. It ended in a graceful close, which a client waiting for OPENED can't tell from success. It's now refused with RESET_STREAM code 1.
• PublicOnly accepted three reserved ranges: 192.88.99.0/24 (6to4 relay anycast), 2001:20::/28 (ORCHIDv2) and 3fff::/20 (documentation).

Testing

• go test -race ./... passes.
• Each fix has a test, and each test fails with its fix disabled. I checked that by applying each mutation and running the test.
• rawSession now also returns the downstream cipher, so TestMalformedDestinationIsRefused can read the server's frames.
• One full-suite run hit a 60 s handshake timeout in TestIdleSessionGoesQuiet (e2e_test.go:1150). It didn't reproduce: 15 isolated runs and 3 full runs each passed on this branch and on main. I'm treating it as a rare flake, not caused by this PR, but noting it in case it recurs.

Pre-PR review (local Codex gate)

• Rounds: 2. Final: CODEX GATE: verdict=approve critical=0 important=0 minor=0 reviewed=12
• Exit: approve. Both rounds read every changed file.
• Fixed:
  • client_gaps_test.go:40: the discovery tests raced the client's run loop, which could add a candidate before the test took the lock. The tests now build the client with discovery off and enable it under the lock.
• Dismissed: none

🤖 Generated with Claude Code

https://claude.ai/code/session_01QQFpfCXs3JqrfyDuiGabF5

Summary by CodeRabbit

• Reliability
  • Improved connection discovery and retry behavior when network paths fail, with timing that follows configured pacing.
  • Capped pending queries and stream counts at supported limits.
  • Improved session handling when control queues fill, including stream cleanup and error reporting.
  • Fixed stream closing so acknowledged data can be followed by a FIN without prematurely resetting the connection.
  • Invalid or unusable stream destinations now receive a reset instead of a graceful close.
• Security
  • Expanded the IP address ranges excluded from public destinations.

[coderabbitai]

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 42 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 65b83711-fcd9-46a5-84dd-dc6990566a36

📥 Commits

Reviewing files that changed from the base of the PR and between 3d9275d and 398e761.

📒 Files selected for processing: 1

• client_gaps_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: edea53fb-102a-402c-9949-a0c0b23838e0

📥 Commits

Reviewing files that changed from the base of the PR and between 1cc714d and 3d9275d.

📒 Files selected for processing: 2

• client.go
• client_gaps_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The client updates discovery scheduling, resolver retries, query limits, and liveness tracking. Session handling updates control-queue limits and stream accounting. Invalid-destination and connection-close handling change. The server excludes additional address ranges.

Changes: Client and Session Reliability

Layer / File(s)	Summary
Discovery path management discover.go, client.go, client_gaps_test.go	Discovery targets healthy verified paths and schedules candidate checks using Discovery.Pace. Hello retries rotate through paths in least-recently-tried order. Tests cover discovery resumption and retry scheduling.
Query limits and liveness client.go, client_gaps_test.go	The client caps MaxInflight at 1,024 and pending queries at 60,000. Query sends observe these limits, and authenticated answers preserve liveness tracking when other live queries remain. Tests cover query limits and liveness.
Stream control and lifecycle client.go, server.go, session.go, client_gaps_test.go, e2e_test.go	Client and server stream caps are set to 256. Control frames are deduplicated, and queue overflow closes the session. Stream creation and removal update stream-slot accounting.
Stream protocol handling server.go, stream.go, e2e_test.go	Invalid destinations trigger a RESET. Conn.Close waits for FIN acknowledgment before sending RESET. End-to-end tests check destination refusal and FIN delivery.
Public address filtering server.go, e2e_test.go	The server excludes 192.88.99.0/24, 2001:20::/28, and 3fff::/20. Tests add addresses from these ranges to the rejected cases.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 3d927

No identified issue blocks merging; queued data remains available for retry when capacity returns.

Security architecture risk: 🔵 Low · up to 3d927

The changes strengthen resource bounds and destination filtering while correcting stream cleanup and delivery behavior. No introduced security bypass was established. Risk remains low because the changes affect shared session state and failure handling, and deployment-specific guarantees were not fully assessed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — A peer able to establish its own protocol session can exercise stream admission and control-queue limits. Overflow directly aborts that session’s streams and releases their server-wide accounting; the new failure path is session-scoped, although admission uses a shared server-wide stream budget.

Trust Boundaries and Controls

• observed — The default server dialer restricts destination ports and applies PublicOnly after resolution. Custom Dial implementations bypass that default wrapper and must preserve equivalent policy themselves. This extension point predates the PR; the inspected changes strengthen its shared address-filter helper rather than introduce the exception.
• observed — The revised client liveness clock is updated only after a matching pending question and successful packet authentication. Unauthenticated error answers remain insufficient to complete the query or alter path health immediately.

Resilience and Maintainability Implications

• inferred — The 60,000-entry pending ceiling leaves transaction IDs available and prevents current send paths from constructing untrackable packets when saturated. Pending transactions retain an owner through authenticated completion or expiration cleanup; duplicate data remains eligible for later transmission when capacity returns.

Pre-merge checks: 5

Passed checks: 5 passed

Check name	Status	Explanation
Docstring Coverage		Docstring coverage is 85.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 7 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the pull request as fixing review-identified gaps across the Go client and server. It is concise and specific.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

🟡 Changes recommended

The fixed queue limit can terminate valid high-concurrency sessions, and critical recovery branches lack regression coverage.

3 open findings

• Control queue overflow can terminate healthy sessions · New
• Add regression test for liveness restart after authenticated answer · New
• Add coverage for reserved-slot recovery on advertise closure · New

What changed in this PR

Ports reliability and security fixes discovered during the Rust implementation back into the Go client and server.

Changes:

• Corrects stream closure, control-queue, and malformed-destination handling.
• Improves discovery, liveness, retry rotation, and query limits.
• Expands reserved-address filtering and regression coverage.

File	Description
stream.go	Waits for FIN acknowledgement before reset.
session.go	Handles control-queue overflow and slot accounting.
server.go	Refuses malformed destinations and reserved ranges.
e2e_test.go	Adds server and stream regressions.
discover.go	Counts only healthy discovery paths.
client.go	Improves scheduling, retries, liveness, and limits.
client_gaps_test.go	Adds client regression tests.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The fixes are coherent and regression-covered, with only non-blocking documentation clarifications remaining.

2 open findings

• Document the 1,024 maximum for Config.MaxInflight · New
• Update Want documentation to clarify healthy-path counting · New

3 resolved since last review

• Control queue overflow can terminate healthy sessions
• Add coverage for reserved-slot recovery on advertise closure
• Add regression test for liveness restart after authenticated answer

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

The new pending-query ceiling can still be exceeded by batched send paths.

1 open finding

• Enforce maxPending limit across all query send paths · New

5 resolved since last review

• Control queue overflow can terminate healthy sessions
• Update Want documentation to clarify healthy-path counting
• Document the 1,024 maximum for Config.MaxInflight
• Add coverage for reserved-slot recovery on advertise closure
• Add regression test for liveness restart after authenticated answer

🧠 Review effort: Balanced

[Copilot]

🟢 Approval recommended

The fixes are consistently bounded and covered by targeted regression tests with no unresolved defects found.

0 open findings

1 resolved since last review

• Enforce maxPending limit across all query send paths

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

The cross-cutting protocol, lifecycle, and concurrency changes warrant final human validation despite strong regression coverage.

0 open findings

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

The cross-cutting transport and session-state changes warrant final human validation despite comprehensive regression coverage.

0 open findings

🧠 Review effort: Balanced