[myleshorton]

Summary

This fixes a stream leak that permanently breaks Soar on a long-running client. NewRoundTripper confirms its connection before returning, and that connection holds one of the session's streams until a request takes it. kindling drops round trippers that connect after another transport has already won, and nothing closed their parked connection.

What happened in the field: one Windows client on 10.0.3-beta-10 failed every Soar attempt about 1,800 times over two days with soar: too many streams. It had won 75 times before that.

• Unbounded's signaling polls to freddie.iantem.io run every ~14 s. Soar often connects after another transport has already answered, so each of those polls can leak one stream.
• An open stream keeps the session from looking idle, so the session is never retired.
• After 64 leaks (the default MaxStreams), every dial fails instantly until the app restarts.

sequenceDiagram
    autonumber
    participant K as kindling<br/>race_transport.go
    participant R as Soar transport<br/>roundtripper.go
    participant S as Soar session<br/>session.go

    K->>R: race_transport.go:627<br/>NewRoundTripper at 8 s
    R->>S: roundtripper.go:24<br/>DialContext opens a stream
    Note over R: roundtripper.go:32<br/>confirmed conn parked in preDialed ⚠️
    R-->>K: round tripper, holding the stream
    Note over K: another transport already won
    rect rgba(255, 200, 200, 0.3)
        Note over K: race_transport.go:632, drainReady :558<br/>round tripper dropped, never closed 🐛
        Note over S: stream stays open, session never idle
    end
    K->>R: next races, about every 14 s
    R->>S: openStream
    S-->>R: session.go:147<br/>soar: too many streams, after 64 leaks

Loading

kindling line numbers are from current main. A companion kindling PR closes the round trippers it drops. This PR makes Soar safe on its own as well, so neither side depends on the other.

Changes

• roundtripper.go:
  • The returned transport is now a *roundTripper that embeds *http.Transport and implements io.Closer. Close closes the parked connection if no request took it, and closes idle connections. Requests already in flight are unaffected.
  • The returned value is still an http.RoundTripper, so the public API doesn't change.
  • An untaken connection now closes itself after preDialedTTL (10 s), for callers that never call Close. A request that arrives after that just dials a fresh stream through the same transport.
  • The timer is set and stopped under preDialed.mu.
• e2e_test.go: two tests, each with MaxStreams: 2.
  • TestClosedRoundTrippersGiveBackTheirStreams: closing unused round trippers frees their streams.
  • TestUnusedRoundTripperGivesBackItsStream: unused round trippers free their streams after the TTL, even without Close.
  • The Close test pins preDialedTTL to an hour, so expiry can't stand in for a broken Close. It fails with soar: too many streams when Close stops releasing the connection, and the expiry test fails the same way when expiry is disabled.

soar-rs needs no change. It has no round tripper that parks a connection, and its Conn closes its stream on Drop.

Verification

• gofmt, go vet ./... and go test -race -count=1 ./... all pass, in the root module and in bench.
• With preDialed.close made a no-op, both new tests fail with soar: too many streams. With only roundTripper.Close broken, the Close test fails on its own.

Pre-PR review (local Codex gate)

• Rounds: 1. Final: CODEX GATE: verdict=approve critical=0 important=0 minor=0 reviewed=7
• Exit: approve. Besides the diff, the reviewer read client.go, stream.go and session.go.
• Fixed: none
• Dismissed: none

🤖 Generated with Claude Code

https://claude.ai/code/session_01QQFpfCXs3JqrfyDuiGabF5

[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 49 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

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 818b1053-ef0e-4571-9b19-32edaf59aff3

📥 Commits

Reviewing files that changed from the base of the PR and between 8efcdc4 and c5d026f.

📒 Files selected for processing: 1

• e2e_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9763ac4f-74d9-4c22-9e25-cf7b7602e37a

📥 Commits

Reviewing files that changed from the base of the PR and between a8af533 and 8efcdc4.

📒 Files selected for processing: 2

• e2e_test.go
• roundtripper.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

NewRoundTripper now returns a wrapper with a Close method. Untaken pre-dialed connections close when the wrapper is closed or when preDialedTTL expires. Added end-to-end tests check that these cleanup paths allow another connection to be established.

Changes: RoundTripper connection lifecycle

Layer / File(s)	Summary
Pre-dialed connection lifecycle roundtripper.go, e2e_test.go	NewRoundTripper returns a wrapper that closes untaken pre-dialed connections and idle transport connections. A timer closes an unused pre-dialed connection after preDialedTTL, and taking the connection stops the timer. Tests check wrapper closure and timer expiration.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant NewRoundTripper
  participant roundTripper
  participant preDialed
  Caller->>NewRoundTripper: Create transport with confirmed pre-dial
  NewRoundTripper->>roundTripper: Return wrapper and start preDialedTTL timer
  alt Caller takes the connection
    roundTripper->>preDialed: Take connection and stop timer
  else Connection remains unused
    Caller->>roundTripper: Close wrapper
    roundTripper->>preDialed: Close untaken connection
    preDialed->>preDialed: Close connection when timer expires
  end

Loading

Merge Risk: ⚪ Minimal · up to 8efcd

No actionable stream-cleanup defect is established, so the change appears mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: releasing streams held by unused round trippers.
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.

✨ Finishing Touches: 💡 1

📝 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 explicit-close regression test can pass through automatic expiry, masking a broken Close cleanup path.

1 open issue

• Prevent connection expiry from masking explicit-close regression · New

What changed in this PR

This PR prevents unused Soar HTTP transports from indefinitely holding session streams, avoiding stream exhaustion in long-running clients.

Changes:

• Wraps http.Transport with explicit cleanup for parked and idle connections.
• Automatically releases unclaimed connections after 10 seconds.
• Adds regression tests for explicit cleanup and automatic expiry.

File	Description
roundtripper.go	Adds synchronized cleanup through Close and an expiry timer.
e2e_test.go	Tests stream recovery after closing or abandoning unused transports.

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

Connection ownership and cleanup are synchronized, both release paths have focused regression coverage, and no unresolved blocking issues remain.

No open issues 🎉

1 resolved since last review

• Prevent connection expiry from masking explicit-close regression

_{Review effort: Balanced}