[ericcurtin]

Summary

Open each Listen tunnel in its own goroutine so one slow setup no longer blocks other clients.

Related Issue

Part of #4512. A setup deadline is a follow-up.

Changes

• Move tunnel setup out of the accept loop into the per-connection goroutine.
• Share the SSH and ForwardTcp setup in one openTunnel helper.
• Add tests for parallel setup and for a hung setup not blocking others or Close.

Testing

• Checks appropriate to the affected code and behavior pass
• Unit tests added/updated (if applicable)
• E2E tests added/updated (if applicable)

Ran go test -race ./... and golangci-lint run in sdk/go. The new tests fail on the old code.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable)

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[ericcurtin]

@TaylorMutch @SDAChess could you please triage #4512 so the issue check can pass? PTAL, and /ok to test 001749eb507a0c449c48c8922da0153a84021d63 if it looks good. Thank you!

[rhuss]

Heads-up: the Go SDK moves to ai-openshell/sdk-go on 2026-11-03 (details in #5031). For this PR that means:

• merged in core by the end of that day: it moves with the history, nothing to do
• still open: it is ported to the new repository with your commits and authorship kept (git format-patch and git am), and a comment here links the replacement before this PR is closed

Nothing to do on your side either way.

cc @TaylorMutch, a review by Monday 2026-11-02 would let this land in core.

[rhuss]

The Go SDK freeze is tomorrow, Tuesday 2026-11-03, at the end of the day (#5031). This fix is small and SDK-only, so it would be good to land it in core before then. Two things block it:

• bug(sdk/go): TCP().Listen sets up tunnels serially in the accept loop #4512 needs state:accepted for the issue check
• CI hasn't run yet (/ok to test), and there's no review

@TaylorMutch @pimlock, could one of you accept the issue and review? If it doesn't make it, it gets ported to ai-openshell/sdk-go with the original commits, as announced above.

[purp]

/ok to test 001749e

[rhuss]

cc-review Summary

This is an agent-assisted review, prepared with cc-review's review agents. The failure scenarios in the comments were derived from reading the code and were not executed.

Thanks for picking this up! Moving setup into a per-connection goroutine fixes the serialization from #4512, and the Close/WaitGroup handshake stays sound. All four findings are non-blocking suggestions (three Minor, one Notable). The main open point is the setup deadline from the #4512 triage note: either add it here or record it as a follow-up. Once that is settled, this looks ready for a maintainer.

What Went Well

• acceptLoop (tcp_client.go lines 175-183): wg.Add(1) happens under tl.mu only after checking tl.closing, and Close sets closing under the same mutex before wg.Wait, so no Add can race the Wait.
• serve (lines 205-218): the WaitGroup decrement moved from bridge into serve via defer, so every goroutine started by acceptLoop releases exactly one count on every path.
• serve (lines 212-216): a tunnel that finishes setup after Close cancelled tl.ctx is closed together with the local conn instead of being bridged, so no stream outlives the listener.
• openTunnel (lines 187-201): SSH vs ForwardTcp selection lives in one helper, so the accept loop no longer mixes accepting with transport selection, and setup on tl.ctx lets Close abort hung setups.
• TestTCPListen_ConcurrentTunnelSetup: the rendezvous barrier only completes when all four setups run at once, so it fails deterministically against the old serial loop instead of depending on timing.
• TestTCPListen_HungSetupDoesNotBlockOthers: covers both acceptance criteria (a second client is served while the first setup hangs, and Close returns within a bound), and runs Close in a goroutine with a 5s select so a regression fails rather than hangs.
• Test helpers: setupTCPTestWithResolver avoids duplicating the bufconn/gRPC bootstrap, gatedResolver embeds stubSandboxResolver instead of copying the panic stubs, and echoOnce uses a deadline and checks the exact echoed payload per connection.
• Listen still binds to 127.0.0.1 by default, so the now-parallel setup is reachable only by local clients unless the caller opts in.

During the agent run, go test -race -count=3 -run TestTCP ./openshell/v1/ passed at this head.

Findings

Severity	File	Description	Source
Minor	sdk/go/openshell/v1/tcp_client.go:207	Unbounded setup goroutines with no deadline pile up, with their sockets, on a stalled gateway	production
Minor	sdk/go/openshell/v1/tcp_client.go:208	No test asserts the EOF a client sees when setup fails or is abandoned	test-quality
Minor	sdk/go/openshell/v1/tcp_client_test.go:1217	The "tunnel setup was serialized" gate error never reaches the test output	test-quality
Notable	sdk/go/openshell/v1/tcp_client.go:207	Closes #4512 without the setup deadline from the triage note, and the deferral is not recorded	goal-alignment

All findings are posted as inline comments.

Review Details

• Findings posted: 4
• Findings reviewed and not posted: 0
• Gate outcome: PASS
• Participating reviewers: goal-alignment, production, test-quality (architecture, correctness, security and gator also ran and reported no findings)

[purp]

gator-agent

PR Review Status

The independent review found no blocking code issues in the parallel tunnel setup or its regression tests. @ericcurtin, I checked your request about issue acceptance and testing: #4512 now has state:accepted, and the Go SDK and Branch Checks passed after @purp authorized testing.

Action required: A maintainer must rerun PR Issue Acceptance now that #4512 is accepted, and approve the pending fork workflows, including Trivy Changes. Workflow Security Reports and Dependency Review also show action_required. Maintainer code approval is still needed afterward.

Blocking findings: None.

Carried findings: None.

Non-blocking suggestions: None.

Gator metadata

• Validation: Focused Go SDK bug fix implementing accepted issue #4512; both changed files reviewed.
• Docs: No update needed for this internal scheduling fix restoring concurrent service; no API, configuration, or documented workflow changes.
• Checks: Go SDK, Branch Checks, Helm Lint, DCO, and vouch gate passed. PR Issue Acceptance retains its pre-acceptance failure; its annotation explicitly requests a rerun after acceptance. Trivy Changes has not run and reports action_required, so required checks are not all green.
• E2E: No additional test labels needed for SDK-only tunnel-setup scheduling; no sandbox lifecycle, gateway, policy, driver, GPU, or Windows implementation changes. Existing E2E gates report success with test labels absent.
• Head SHA: 001749eb507a0c449c48c8922da0153a84021d63
• Base SHA: 44ef984b58f9badf79f2ff5b9982054017b01ca8
• Merge base SHA: 44ef984b58f9badf79f2ff5b9982054017b01ca8
• Patch ID: 4f9a913bb505d67a665cdf21e4dc31f0fb0cd2ee
• Gator payload: 13
• Review mode: initial
• Previous reviewed SHA: none
• Review budget exhausted: no
• Maintainer decision required: no — no unresolved code findings or scope growth
• Next state: gator:blocked
• Blocked reason: test_dispatch_required

[ericcurtin]

Added EOF and gate error test coverage; setup deadline left as follow-up, PR now says Part of #4512.

[ericcurtin]

@shailendra-nv @krishicks PTAL when you get a chance, and /ok to test 3ca79f0bdf4e37e1294655bad2bc9522d68ba150 if it looks good. The issue check wants a closing keyword but this covers part of #4512, so please say if you would rather I switch it. Thank you!

[purp]

/ok to test 3ca79f0

[rhuss]

Follow-up on my review: I checked the current head (3ca79f0) against my four threads, and a fresh review of the full change found nothing new.

• Fixed: the EOF checks for the local client (requireEOF, TestTCPListen_FailedSetupClosesConn) and the gate errors, which now reach the test output.
• Deferred to a follow-up under bug(sdk/go): TCP().Listen sets up tunnels serially in the accept loop #4512, nonblocking: a per-setup deadline. Until then, a gateway that accepts the call and never answers leaves one goroutine per reconnect until the listener closes.

The issue check fails only because "Part of #4512" isn't a closing keyword; a maintainer can decide between "Closes #4512" and a waiver. Ready for a maintainer review.

[TaylorMutch]

Reviewed the tunnel setup concurrency and shutdown paths. No actionable code defects found. All TCP Listen tests passed three times with the race detector, and the required CI checks are passing.