fix(runtime): recover SSH relays and bound startup diagnostics - #4011
Conversation
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This localized runtime bug fix is project-valid, but the initial review found one blocking relay-delivery race introduced by the reconnect change.
Action required: @drew, ensure a pending relay is sent at most once to the same supervisor session and add the concurrent registration/replay regression test described inline.
Blocking findings:
GATOR-182b64ba-01: unconditional replay can duplicate a relay already sent to the newly registered session
Carried findings:
- None
Gator metadata
- Validation: Maintainer-authored, concentrated fixes for reproduced runtime failures
- Docs: Not needed because the patch changes internal recovery and diagnostics without changing commands, configuration, or documented workflows
- Checks: Current-head branch checks are still running; review feedback blocks pipeline handoff
- E2E:
test:e2eis required for gateway/supervisor and Docker runtime behavior, but dispatch waits until blocking review feedback is resolved - Head SHA:
182b64ba13ee2a14329766bd819abfb9a7ebe441 - Base SHA:
9912d21d30978d9a4389a71e871da47e0f974feb - Merge base SHA:
252882f37f2daa63ef4078b3bbb89adbcf43b409 - Patch ID:
5f7487db3f2352fa955b25c8587fefb7e0e3f800 - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
the
gator:in-review
|
i am new to github this looks so awesome and im so impressed you can do this. |
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
|
Label |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @drew. I checked the relay-delivery fix and its new registration/replay regressions against the prior finding; GATOR-182b64ba-01 is resolved, and the follow-up review found no new blocking issue in the author delta.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Gator metadata
- Validation: Maintainer-authored, concentrated fixes for reproduced runtime failures
- Docs: Not needed because the patch changes internal recovery and diagnostics without changing commands, configuration, or documented workflows
- Checks: Current-head branch checks are running and Rust lint has reported a failure; pipeline diagnosis follows after E2E dispatch
- E2E:
test:e2eis applied; E2E Label Help requires rerunning current-head run36798528755, and dispatch is pending the approved sandbox policy reload - Head SHA:
e8dc61f9d63ccb68d619350c4973df50680f982f - Base SHA:
9912d21d30978d9a4389a71e871da47e0f974feb - Merge base SHA:
252882f37f2daa63ef4078b3bbb89adbcf43b409 - Patch ID:
f794f595866a67dad63421a6ba7a33fe95bfbdd1 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
182b64ba13ee2a14329766bd819abfb9a7ebe441 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @drew. I reviewed the two author commits after e8dc61f9, including the absolute relay-setup deadline and its virtual-time regressions. The prior relay-delivery finding remains resolved, and this follow-up found no new blocking issue in the delta.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Gator metadata
- Validation: Maintainer-authored, concentrated fixes for reproduced runtime failures
- Docs: Not needed because the delta changes internal relay timing and tests without changing commands, configuration, or documented workflows
- Checks: Current-head Branch Checks and required E2E are running
- E2E:
test:e2eis applied, and current-head run36800721792is in progress - Head SHA:
32db1b210a49294bb696a0468667bede29856f33 - Base SHA:
9912d21d30978d9a4389a71e871da47e0f974feb - Merge base SHA:
252882f37f2daa63ef4078b3bbb89adbcf43b409 - Patch ID:
1d276120e4ae453f0af2bb30538e0ff4c4e532e0 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
e8dc61f9d63ccb68d619350c4973df50680f982f - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Reviewed head 32db1b2 and compared the relay fixes with #3583, #3588, #3618, #3619, and #3766. The reconnect replay and per-session delivery fixes overlap with that earlier work; this implementation strengthens delivery by reserving queue capacity and atomically validating the session, recording delivery, and enqueueing the open. The shared setup deadline, SSH proxy shutdown/error propagation, and bounded Docker diagnostics are useful additional fixes. No blocking findings remain from this review. Required Branch Checks, E2E, GPU E2E, and Helm Lint statuses are successful. Follow-up: rebase #3766 onto this change, retain its deterministic reconnect conformance coverage and architecture documentation, and remove the overlapping server fix. The underlying Docker startup stall remains unresolved.
johntmyers
added
gator:merge-ready
and removed
gator:approval-needed
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: Gator review found no remaining blocking findings, the required Branch Checks and E2E gate passed, and maintainer approval was present before merge. I removed the active |
4 participants
Summary
Fix confirmed defects exposed by intermittent checks on #3979: pending SSH relays can be lost during reconnect, the SSH proxy can hang after its relay closes, and large Docker startup errors can become HTTP/2 protocol errors.
The underlying Docker boundary startup stall is still unresolved; this PR does not claim to fix that stall.
Failure assessment
These are mostly implementation problems exposed by tests.
PROTOCOL_ERRORThe tests also have weaknesses: the Kubernetes test lets a CLI command hang until the overall 300-second timeout, and CI retains too little useful diagnostic output. Per-command deadlines and better failure-time log retention remain follow-up work. Increasing timeouts or adding retries alone would mask the confirmed implementation defects.
Related Issue
No issue required: localized bug fixes supported by CI logs and reproductions with the #3979 CLI binary.
Investigation reference: #3979
Changes
Bound routing, session retries, and outbound queue-capacity waits with one absolute setup deadline. Local attempts immediately fall back when no session exists while retaining the routing caller's deadline for queue waits.
Replay pending relays on every accepted supervisor session, including reconnects after the previous registration was removed. Record delivery per session and atomically enqueue under the registry locks after reserving queue capacity, so a forward opened between registration and replay is delivered only once.
Read SSH proxy stdin on a dedicated OS thread so a blocked read cannot prevent Tokio runtime shutdown.
Propagate relay and output errors to the proxy caller instead of returning success.
Limit Docker log tails included in gRPC statuses to 1 KiB per container, with UTF-8-safe truncation. Preserve longer supervisor and sandbox tails in the gateway warning log when the supervisor exits before readiness.
Cover disconnected-session relay replay, actual proxy subprocess shutdown with stdin held open, and the encoded gRPC header budget for large multibyte diagnostics.
Review follow-up
Addressed
GATOR-182b64ba-01ine8dc61f9d: a relay opened after registration can no longer be duplicated by replay. Added regressions for that interleaving, repeated replay, and session replacement while an initial forward waits for outbound queue capacity. These new regressions have not been run locally; no additional local checks were started, as requested.Testing
Added virtual-time regressions for blocked outbound queues and repeated reconnects retaining the original deadline, plus immediate missing-session fallback. These new tests have not been run locally, per the request to avoid additional local checks.
Proxy subprocess regressions: clean relay close and deadline error both exit with stdin still open; error status propagates.
Docker diagnostic regression: both log tails preserve their final diagnostic and encoded status headers fit below 16 KiB.
Original gateway relay regressions passed on the initial PR commit. The new review-follow-up regressions await CI.
mise run pre-commitpassed on the initial PR commit. It was not rerun for the review follow-up, as requested.mise run testandmise run e2ewere started; final results pending at PR creation.mise run cidid not pass: Go SDK gateway-list tests see this host's/etc/openshellsystem gateway and fail their empty-directory/count expectations. The initial pre-commit attempt also hit a transient missing TypeScript buf executable while npm installation was running concurrently; a subsequent attempt was started.Pushed before the remaining checks completed, as requested. No additional checks will be started.
Checklist