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

[rhuss]

Minor: The new SSH_CLEANUP_TIMEOUT_MS bound has no test. This test's revokeSshSession returns right away, so nothing checks that close() still resolves when the revoke stalls.

Why this matters: teardown() awaits connectionTasks, and each task now awaits the revoke, so handle.close() and handle.closed depend on that timeout firing. If a later refactor dropped the timeout signal, a gateway that accepts the RPC but never answers would make close() hang, and every forward test would still pass. The router transport only races the handler against the request signal, so a handler that never resolves hangs as long as the client signal allows.

Suggested fix: Make the cleanup timeout injectable (a module-level override or a test hook on ForwardOptions), then add a test with revokeSshSession: () => new Promise(() => {}) that asserts await handle.close() resolves within the bound. Optionally, also assert that the handler sees a ctx.signal that starts out non-aborted and aborts later.

Source: test-quality agent

[rhuss]

Minor: close() and closed can now wait up to about 5s on token revokes. Teardown awaits connectionTasks, and each task's finally now awaits a revoke that is no longer pre-aborted. The revokes run in parallel, so the worst case is roughly one timeout, not one per connection.

Why this matters: Before this change the revoke failed instantly on the aborted signal, and close() returned as soon as the listener and sockets were down. Now shutdown paths (exit handlers, test teardown, a "stop forwarding" action) can stall for up to 5s when the gateway is slow or unreachable, which is a common reason to tear a forward down, and aborting opts.signal no longer shortens that. The ForwardHandle comment (client.ts:486-488) only mentions listener teardown. This is a reasonable trade-off, not a correctness or security problem.

Suggested fix: Either document the bound on ForwardHandle (for example: "close() may wait up to 5s for best-effort revokes of open connections' tokens"), or fire the revoke without blocking teardown:

const revoke = this.grpc
  .revokeSshSession({ token, allowMissing: true }, { signal: AbortSignal.timeout(SSH_CLEANUP_TIMEOUT_MS) })
  .catch(() => {});
if (!signal.aborted) await revoke;

The second option means a process that exits right after close() may not finish the revoke, so documenting the bound is probably the simpler choice.

Source: gator agent

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