[ericcurtin]

Summary

forward().close() aborted each connection's signal, then used that signal to revoke its SSH token, so the revoke never went out. Revoke with a fresh 5 s timeout instead, like the Go SDK.

Related Issue

Closes #4516

Changes

• Revoke the token with its own bounded signal.
• Add a test that closes a forward with an open connection.

Release Notes

Testing

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

Lint, typecheck, tests and build in sdk/typescript pass. The new test fails without the fix.

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]

@krishicks @matthewgrossman could you please triage #4516 so the issue check can pass? PTAL when you get a chance. Thank you!

[rhuss]

cc-review Summary

This is an agent-assisted review: I ran cc-review with several reviewer agents. The timing scenarios below were derived from reading the code and were not executed.

Nice, focused fix. Both findings are non-blocking Minor suggestions, so take or leave them as you see fit. With those considered, this looks ready for a maintainer.

What Went Well

• forwardConnection (sdk/typescript/src/client.ts:1900-1904) now revokes with its own AbortSignal.timeout(SSH_CLEANUP_TIMEOUT_MS) instead of the connection signal that teardown() aborts first, so tokens of connections still open at close() really get revoked. Value and pattern match the Go SDK's revokeSessionForCleanup (sdk/go/openshell/v1/ssh_client.go:139, sshCleanupTimeout = 5 * time.Second).
• The bound is a named constant (client.ts:726-727), and the inline comment says why the connection signal can't be used, which guards against a refactor bringing the bug back.
• Revokes run concurrently inside connectionTasks and teardown awaits them with Promise.allSettled, so close latency is bounded by one timeout rather than one per connection.
• The new test (client.test.ts:2239-2274) is a real regression test: the mock only records revokes whose ctx.signal is not aborted, so it fails against the old { signal } code. It waits for an echoed frame before close(), asserts the exact list with toEqual(['open-tok']), and needs no sleep.

Findings

Severity	File	Description	Source
Minor	sdk/typescript/src/client.test.ts:2239	No test covers the new SSH_CLEANUP_TIMEOUT_MS bound when revokeSshSession stalls during teardown	test-quality
Minor	sdk/typescript/src/client.ts:1903	close() and closed can now wait up to about 5s on best-effort token revokes	gator

Both findings are posted as inline comments; none are summary-only.

Review Details

• Findings posted: 2
• Findings reviewed and not posted: 0
• Gate outcome: PASS
• Participating reviewers: test-quality, gator (architecture, correctness, goal-alignment, production and security ran with no findings; the external tools were skipped for this checkout)

[ericcurtin]

Documented the 5s close() bound on ForwardHandle and added a test with a never-resolving revoke (cleanup timeout overridable via a small test hook). sdk:ts:ci passes.

[ericcurtin]

@FrostGod @TaylorMutch addressed the review comments, could you please triage #4516 so the issue check can pass? PTAL, and /ok to test 09b24e9d8d159664f0b8e3b41bc0f151d46c559a if it looks good. Thank you!

[TaylorMutch]

/ok to test 09b24e9

[ericcurtin]

@TaylorMutch @rhuss PTAL, threads are resolved and CI is green. Could #4516 be marked state:accepted so the issue gate passes? Thank you!