Repository navigation
fix(server): tombstone deleted sandboxes in log and event buses - #5428
Conversation
Late publishers re-created a deleted sandbox's log and platform-event buffers after delete cleanup had freed them, so every create/delete cycle leaked about 1.7 MiB per sandbox on the gateway that served the delete. Nothing freed the re-created buffers again, and gateway RSS grew until it was OOM-killed. Mark the sandbox's cursor space closed in the shared allocator during delete cleanup. Both buses drop publishes for a closed space before allocating a tail or a broadcast channel, and subscriptions return an already-closed receiver instead of re-creating the entry. The gateway tracing layer and driver platform events are covered by the same gate. An open PushSandboxLogs stream now checks that tombstone on every batch and re-reads the sandbox record at most once per second, so a replica that did not run the delete still notices the sandbox is gone. The handler closes the local bus entry and ends the stream with NotFound; a transient store failure keeps the stream open and retries on the next cadence. remove() keeps its current reset semantics for the resume-cursor tests. Closes NVIDIA#5402 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
TaylorMutch
added
area:gateway
|
Validated the RSS reproduction for #5402 against this PR ( Both runs used the Helm development environment on local ARM64 k3d (Kubernetes v1.31.5+k3s1), with one gateway per release and matching sandbox/supervisor images from the PR revision. Each run created 20 sandboxes, executed a command in each, deleted all 20, then repeated four times with 8 new sandboxes: 52 sandboxes per run, 104 total. The workload image was RSS below comes directly from the gateway process's
After the final cycle, the PR gateway later fell to 66.51 MiB RSS while idle, compared with 67.75 MiB after the warm-up cycle. That idle decrease was largely in file-backed RSS. At the matched final 50-second samples, anonymous RSS was 104.53 MiB for the parent versus 59.40 MiB for the PR. Both valid runs completed with zero gateway restarts, constant gateway container IDs throughout each run, and no sandbox pods remaining. The isolated cluster is now stopped. Methodology limits:
|
|
/ok to test e305308 |
|
/ok to test e305308 |
TaylorMutch
left a comment
TaylorMutch
left a comment
There was a problem hiding this comment.
Reviewed commit e305308. No actionable code findings.
The terminal tombstone prevents both late publications and subscriptions from recreating deleted buffers, with a consistent shared-allocator-to-bus-map lock order. Cleanup is tied to durable sandbox removal. The push-stream path preserves authorization checks before existence checks and tolerates transient store failures without treating them as deletion. Added coverage exercises closure, late publishers/subscribers, publication races, deletion integration, and stream termination.
The independent Helm/k3d RSS comparison posted above supports the targeted fix: after 52 deletions, settled RSS was 128.91 MiB for the parent versus 76.87 MiB for this PR, with the PR later returning to 66.51 MiB while idle. Cross-replica cleanup remains tracked in #5401, and the intentionally retained small tombstones are documented.
Branch Checks are still running at the time of approval. The unrelated optional Review Readiness failure is excluded from this code-review decision.
Summary
Deleted sandboxes used to leave their log and platform-event buffers behind. The buses create a sandbox's buffers for any id, and after delete cleanup ran once, late log pushes and post-
Deleteddriver events re-created them. Every create/delete cycle leaked about 1.7 MiB per sandbox until the gateway was OOM-killed.Cleanup now closes the sandbox's cursor space, which makes deletion terminal: both buses drop later publishes and subscriptions for that id.
Related Issue
Closes #5402
Changes
close()toTracingLogBus. It frees both buses' per-sandbox entries and leaves a tombstone in the shared cursor allocator, so a late publisher drops its event before allocating a tail or a broadcast channel.remove()keeps its reset semantics for the resume-cursor tests.subscribe()on both buses, so a subscription racing a close gets an already-closed receiver instead of re-creating the entry.handle_push_sandbox_logschecks the tombstone on every batch and re-reads the sandbox record at most once per second. A missing record ends the stream withNotFound; a transient store error keeps it open.close(), which also covers gateway tracing logs, OCSF audit events, and driver platform events for the deleted sandbox.Notes: the tombstone lives on the replica that ran delete cleanup. A stream on another replica ends on its next batch through the existence recheck, but freeing that replica's buffers is tracked in #5401. A stream ends on its next received batch, not on a server-side timer. Sandboxes in
Deletingare not cut off; the tombstone fires once the record is gone. A supervisor that outlives its sandbox now getsNotFoundon its next batch and reconnects; making that terminal in the supervisor is a follow-up. This touches the same functions as #5408, so whichever lands second rebases.Release Notes
Deleted sandboxes no longer leave memory behind on the gateway that served the delete. Repeated create/delete cycles keep gateway memory flat instead of growing until the gateway is OOM-killed.
Testing
cargo test -p openshell-server --features test-support: all suites pass.cargo clippy -p openshell-server --all-targets -- -D warningsandcargo fmt --all -- --checkare clean. The new tests cover publish/subscribe after close, active receivers, close-vs-publish races, repeated create/delete cycles, and a tonic-over-TCP push stream endingNotFoundafter delete. The Kubernetes RSS repro from the issue was not rerun.Checklist