feat(server): keep pool sandbox sessions on their supervisor's gateway by FrostGod · Pull Request #5052 · NVIDIA/OpenShell

FrostGod · 2026-10-29T23:57:48Z

Summary

Each pool sandbox session dials the gateway replica that serves its supervisor, so a sandbox and its parent supervisor always share a gateway.

Closes #5051

Changes

HostAccepted carries the accepting replica's peer endpoint.
The supervisor host publishes it; its sandbox instances dial their sessions there (redirected=true, so the replica serves them locally). They wait up to 5 s for it after a host reconnect.
A stopping gateway refuses new host streams and ends existing ones, so hosts reattach to a live replica and their sessions follow.
After a host stream ends, for example on supervisor token expiry, the host redials the replica that accepted it before falling back to the Service, so its sessions stay put.
An established session reconnects only if its host does land on another replica.
e2e:kubernetes:shared-supervisor-ha now runs two gateway replicas on PostgreSQL and checks colocation through gateway and supervisor failures.

flowchart LR
  G1[gateway A] -- host stream --> S1[supervisor 1]
  S1 -. sessions dial A .-> G1
  G2[gateway B] -- host stream --> S2[supervisor 2]
  S2 -. sessions dial B .-> G2

Testing

Unit tests: openshell-supervisor-control, openshell-supervisor-process, openshell-supervisor, openshell-server
The colocation test on k3d (2 gateways, PostgreSQL, 2 supervisors, 4 sandboxes): sessions stay on their supervisor's gateway and exec works after start, a gateway crash, a gateway rolling restart, and a supervisor crash, with 60 s supervisor tokens so hosts also move between gateways on their own

copy-pr-bot · 2026-10-29T23:57:59Z

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.

…lica A shared supervisor pool sandbox's session now goes to the gateway replica serving its owning member's stream, instead of the replica the ring picks by sandbox ID. Each pool reconciliation pass publishes the sandbox-to-replica map; sessions fall back to the ring when the parent is unknown, not a live peer, or this replica is shutting down. Signed-off-by: divesh <dgude@nvidia.com>

A pool sandbox session served by a replica other than the one serving its owning member is redirected there on its next heartbeat, reusing the shutdown handoff so the sandbox stays Ready. This re-homes sessions left on the wrong replica by a rolling restart. Signed-off-by: divesh <dgude@nvidia.com>

Removes the per-sandbox parent map and the heartbeat relocation; pool sessions will dial their supervisor's gateway directly instead. Signed-off-by: divesh <dgude@nvidia.com>

…plica HostAccepted now carries the accepting replica's peer endpoint. A shared host's sandbox sessions dial it directly, so each session runs on the replica serving its supervisor. A stopping replica sheds its host streams, and redirected sessions follow the host to its next replica. Closes #5051. Signed-off-by: divesh <dgude@nvidia.com>

…ailover Add the shared-supervisor-tree lane: two gateway replicas on PostgreSQL with a two-member supervisor pool. After start, a gateway crash, a gateway rolling restart, and a supervisor crash, every pool sandbox's session runs on the replica serving its supervisor and exec reaches every sandbox. Signed-off-by: divesh <dgude@nvidia.com>

github-actions · 2026-10-30T22:14:18Z

🌿 Preview your docs: https://nvidia-preview-pr-5052.docs.buildwithfern.com/openshell

drew · 2026-10-30T23:02:43Z

Established sandbox sessions do not follow a host that reconnects elsewhere. Before this PR, session placement followed the hash ring independently of the host. This PR colocates sessions initially, but reads gateway_home only between connection attempts. Once run_single_session(...).await establishes a healthy session, later home changes are never observed.

This can happen during normal supervisor-token expiry: the gateway closes the host's assignment stream, and the host reconnects through the Service, potentially landing on another replica. Existing sandbox sessions remain on the previous gateway indefinitely, breaking the promised tree topology.

Please watch for home changes while a session is active and reconnect when a different nonempty endpoint arrives. Add coverage that moves only the host stream while keeping sandbox sessions healthy; the new gateway crash/restart tests close both and miss this case.

Validation: reviewed the full diff and surrounding lifecycle code; did not run tests locally.

drew · 2026-10-30T23:07:53Z

Can we fold this coverage into e2e:kubernetes:shared-supervisor-ha instead of adding a new lane? We could run that lane with two gateways and PostgreSQL, add the colocation assertions and gateway failure/restart steps, and keep its existing checks that workloads survive without restarting.

…es gateway A host whose stream ends, for example when its supervisor token expires, can reattach to another replica. Its sandbox sessions now watch for that and reconnect to the host's new replica instead of staying on the old one. Signed-off-by: divesh <dgude@nvidia.com>

…-ha lane Run the shared-supervisor-ha lane with two gateway replicas on PostgreSQL and move the gateway crash, rolling restart, and supervisor crash colocation test into it, replacing the separate shared-supervisor-tree lane. Signed-off-by: divesh <dgude@nvidia.com>

…ream ends A host's stream ends routinely, for example when its supervisor token expires. Redialing the replica that accepted it, before falling back to the Service, keeps the host and its sandbox sessions on that replica instead of moving them all on every reconnect. Signed-off-by: divesh <dgude@nvidia.com>

FrostGod · 2026-11-01T19:14:42Z

Established sandbox sessions do not follow a host that reconnects elsewhere. Before this PR, session placement followed the hash ring independently of the host. This PR colocates sessions initially, but reads gateway_home only between connection attempts. Once run_single_session(...).await establishes a healthy session, later home changes are never observed.

This can happen during normal supervisor-token expiry: the gateway closes the host's assignment stream, and the host reconnects through the Service, potentially landing on another replica. Existing sandbox sessions remain on the previous gateway indefinitely, breaking the promised tree topology.

Please watch for home changes while a session is active and reconnect when a different nonempty endpoint arrives. Add coverage that moves only the host stream while keeping sandbox sessions healthy; the new gateway crash/restart tests close both and miss this case.

Validation: reviewed the full diff and surrounding lifecycle code; did not run tests locally.

Good find, this is actually much bigger than I thought. I didn't know the host stream gets closed on every token expiry, so hosts were hopping gateways and leaving their sessions behind. Fixed it two ways: the host now redials the same gateway first, so nothing moves in the normal case, and sessions follow the host if it does end up somewhere else. Tested on k3d with short tokens, hosts stayed put across expiries.

FrostGod · 2026-11-01T19:15:11Z

Can we fold this coverage into e2e:kubernetes:shared-supervisor-ha instead of adding a new lane? We could run that lane with two gateways and PostgreSQL, add the colocation assertions and gateway failure/restart steps, and keep its existing checks that workloads survive without restarting.

@drew done.

EmilienM

Another iteration needed: 2 major, 7 minor (inline, 1 below)

A host move that lands while a sandbox session is connecting is swallowed by mark_unchanged(), so the session can stay off its supervisor's gateway. The reconnect loop's Service fallback is also undone by the home re-pin on the next attempt.

Outside the diff:

Minor: docs/kubernetes/high-availability.mdx:194, Established streams do not move between gateway replicas is no longer true for pool sandbox sessions, which now follow their host; the page needs a sentence on the new behavior.

How this was reviewed

Commit 403902950, round 1
Deep review, because the change reroutes gateway and supervisor sessions (crates/openshell-server/src/grpc/supervisor_host.rs, crates/openshell-supervisor-process/src/supervisor_session.rs) and reworks the HA e2e lane
Passes: architecture, performance, security, scalability, rust, kubernetes, then correctness
Independent skeptics checked each candidate finding (3 for the blocking and major code-lens candidate, traced rather than reproduced; 2 for architecture and scalability candidates; 1 for minor ones); 8 of 9 held up
Repo rules read from origin/main: AGENTS.md, CLAUDE.md, CONTRIBUTING.md, docs/CONTRIBUTING.mdx, .github/PULL_REQUEST_TEMPLATE.md
Not covered: nothing skipped

TaylorMutch

Local review found one race in the gateway-home watcher, detailed inline. Validation: 131 openshell-supervisor-process tests, 12 openshell-supervisor-control tests, and 17 shared-host tests passed. A focused reproduction using the new watch-channel helpers confirmed the missed update. The Kubernetes HA lane was not run.

An established session marked its host's current gateway as seen before watching it, so a move that landed while the session was connecting was never followed. Compare the current gateway first, then wait for changes. Signed-off-by: divesh <dgude@nvidia.com>

…replica has no peer endpoint Validate the advertised peer endpoint scheme with the shared supervisor, and document that pool sessions follow their supervisor's gateway. Signed-off-by: divesh <dgude@nvidia.com>

…imit Signed-off-by: divesh <dgude@nvidia.com>

FrostGod · 2026-11-03T23:01:51Z

Added a short note in the HA docs: sandbox sessions stay on their supervisor's gateway and follow it when it moves.

@EmilienM

FrostGod · 2026-11-03T23:02:36Z

/ok to test 3e2dfab

github-actions · 2026-11-03T23:26:14Z

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. rust-sdk — No explicit risk acceptance found

openshell_sdk::raw::proto::supervisor_host::v1::HostAccepted gains a third public field, peer_endpoint, invalidating existing two-field struct literals.

Impact: SDK mock/server implementations constructing HostAccepted { session_id, heartbeat_interval } fail to compile with missing-field error E0063, while client callers merely receiving the message remain unaffected.
Action: Preserve the existing two-field SDK type on v0.1.7 by separating the internal wire representation, or defer the SDK-visible change to a minor release with notice and peer_endpoint initializer migration guidance.
PR: feat(server): keep pool sandbox sessions on their supervisor's gateway #5052

Review details

Execution: complete
Candidate: PR #5052 (eeda9254ba2ad468c478170e7366a34571ce2124)
Baseline: d7b616895d8778320f994ddf14f4e89ab777f29c (d7b616895d8778320f994ddf14f4e89ab777f29c)
Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

TaylorMutch

Re-reviewed 3e2dfab locally. The previously reported gateway-home watcher race is fixed: the session compares the current home before waiting for changes, and the new session-setup regression test passes. No further actionable code concerns found.

Local validation: 133 openshell-supervisor-process tests, 12 openshell-supervisor-control tests, and 17 shared-host tests passed. Branch Checks and Helm Lint passed for this exact revision. E2E/GPU E2E gates passed without running the optional suites because their labels were not applied; the Windows workflow completed successfully with its build/test jobs skipped. The Kubernetes HA lane was not run locally.

FrostGod requested a review from drew October 30, 2026 00:05

FrostGod added 5 commits October 30, 2026 14:38

refactor(server): drop gateway-side pool session placement

465aa71

Removes the per-sandbox parent map and the heartbeat relocation; pool sessions will dial their supervisor's gateway directly instead. Signed-off-by: divesh <dgude@nvidia.com>

FrostGod force-pushed the feat/5051-supervisor-tree-routing/FrostGod branch from 68a5a70 to efda3de Compare October 30, 2026 22:13

FrostGod added 3 commits October 31, 2026 18:12

FrostGod marked this pull request as ready for review November 2, 2026 16:44

EmilienM reviewed Nov 2, 2026

View reviewed changes

TaylorMutch reviewed Nov 2, 2026

View reviewed changes

Comment thread crates/openshell-supervisor-process/src/supervisor_session.rs Resolved

FrostGod added 3 commits November 2, 2026 14:20

fix(supervisor): dial the configured gateway at once when the host's …

f7593db

…replica has no peer endpoint Validate the advertised peer endpoint scheme with the shared supervisor, and document that pool sessions follow their supervisor's gateway. Signed-off-by: divesh <dgude@nvidia.com>

test(e2e): harden the supervisor tree check's port-forward and time l…

3e2dfab

…imit Signed-off-by: divesh <dgude@nvidia.com>

TaylorMutch approved these changes Nov 3, 2026

View reviewed changes

FrostGod enabled auto-merge November 4, 2026 02:16

FrostGod added this pull request to the merge queue Nov 4, 2026

Merged via the queue into main with commit 8758cbf Nov 4, 2026
84 of 92 checks passed

FrostGod deleted the feat/5051-supervisor-tree-routing/FrostGod branch November 4, 2026 02:33

This was referenced Nov 4, 2026

feat(server): export supervisor session placement metrics #4692

Open

decide: ring-redirect a pool session whose host is between replicas #5267

Open

Conversation

FrostGod commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Uh oh!

copy-pr-bot Bot commented Oct 29, 2026

Uh oh!

github-actions Bot commented Oct 30, 2026

Uh oh!

drew commented Oct 30, 2026

Uh oh!

drew commented Oct 30, 2026

Uh oh!

FrostGod commented Nov 1, 2026

Uh oh!

FrostGod commented Nov 1, 2026

Uh oh!

EmilienM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TaylorMutch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrostGod commented Nov 3, 2026

Uh oh!

FrostGod commented Nov 3, 2026

Uh oh!

github-actions Bot commented Nov 3, 2026

Compatibility review (advisory)

1. rust-sdk — No explicit risk acceptance found

Uh oh!

TaylorMutch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FrostGod commented Oct 29, 2026 •

edited

Loading