Repository navigation
fix(server): prevent unrelated sandbox deletes from blocking deletion events #2326
Description
Agent Diagnostic
- Loaded the repo-local
create-spikeworkflow and used a principal-engineer review of the CLI, gateway compute runtime, Podman driver/watcher, supervisor process path, E2E harness, and architecture docs. - Inspected the linked failure and searched GitHub Actions logs for the exact
sandbox_create_with_no_keep_cleans_up_after_tty_commandassertion. Found at least seven exact failures between July 6 and July 16, 2026, across unrelated PRs and merge-queue candidates. - Confirmed the linked PR's AWS STS changes do not intersect the sandbox delete, Podman event, supervisor shutdown, or E2E lifecycle paths.
- Confirmed the same SHA passed on rerun, including the linked PR's attempts 2 and 3.
- Searched open and closed repository issues for the test name, assertion,
--no-keep, Podman deletion races, andDeleteSandbox; no duplicate was found. - Latest release testing is not applicable because this is a current Branch E2E failure against unreleased PR and merge-queue SHAs.
Problem Statement
The rootless Podman E2E test sandbox_create_with_no_keep_cleans_up_after_tty_command flakes because sandbox deletion can take more than 20 seconds to become absent from ListSandboxes. A slow delete holds a process-wide gateway mutex across the external Podman call, so an unrelated sandbox delete blocks watch-driven cleanup for the first sandbox. Host-native rootless Podman amplifies the race because the process-enabled supervisor runs as container PID 1 without SIGTERM handling, causing each stop to consume the full 10-second timeout before SIGKILL.
The test's 10-second observation window is equal to one Podman stop timeout. It also sleeps after its final poll without checking once more, so it can fail even when deletion completes during that final sleep.
Reproduction Steps
- Run the Rust E2E suite against rootless Podman on Ubuntu 24.04.
- Let the two tests in
e2e/rust/tests/sandbox_lifecycle.rsexecute concurrently. - Both tests create a sandbox and eventually delete it. The default-keep test performs explicit cleanup while the no-keep CLI path performs automatic cleanup.
- Observe Podman wait 10 seconds for SIGTERM before using SIGKILL, while each gateway delete holds the shared
ComputeRuntime::sync_lock. - Observe the no-keep sandbox remain visible as
Deletingwhile its watcher events wait behind the sibling sandbox deletion. The fixed 20 x 500 ms poll can then expire.
CI Occurrences
The following jobs contain the exact should have been deleted automatically assertion:
| Date (UTC) | Context | Job |
|---|---|---|
| 2026-07-16 | PR #1782, AWS STS | 87501178906 |
| 2026-07-15 | Merge queue for PR #2288 | 87491941822 |
| 2026-07-15 | PR #1782, earlier SHA | 87251810998 |
| 2026-07-15 | Merge queue for PR #2302 | 87486355113 |
| 2026-07-15 | PR #2027, supervisor middleware | 87245713093 |
| 2026-07-10 | PR #2216, Helm supervisor image | 86439898310 |
| 2026-07-06 | PR #2153, compute readiness | 85442993990 |
PR #2216's run passed on attempt 2, which shows that searching only final workflow conclusions misses occurrences hidden by reruns.
Technical Context
Observed timeline from the linked job
For no-keep sandbox meek-rabbit:
- Podman deletion starts at
00:07:35.549. - Podman cannot stop PID 1 with SIGTERM, waits 10 seconds, then uses SIGKILL.
DeleteSandboxreturns success at00:07:45.769after 10.225 seconds.- The sibling test starts deleting
resilient-manakinand holds the same gateway synchronization mutex for another Podman stop timeout. meek-rabbitremains in the store while its watch events wait for the mutex.- At
00:07:56.437, after the sibling delete releases the lock, a queued event changesmeek-rabbitfromDeletingtoErrorwithContainerExitedand exit code 137. - The test fails at
00:07:56.930.
Other occurrences show the same pattern: a 10-second Podman stop timeout, successful DeleteSandbox, and either a late Deleting to Error / Provisioning transition or delayed deleted-event cleanup.
Architecture and data flow
sandbox create --no-keepruns the TTY command andfinalize_sandbox_create_sessionsynchronously callssandbox_delete.ComputeRuntime::delete_sandboxtakes one process-widesync_lock, writes phaseDeleting, and keeps the mutex while awaiting the compute driver'sDeleteSandboxRPC.- The Podman driver sends SIGTERM with
stop_timeout_secs = 10, force-removes the container, and returns whether the container existed. - When the driver returns
true, the gateway does not synchronously remove the sandbox record. It relies on the Podman watcherremoveevent andapply_deletedto remove the record. - Podman
stop/dieevents produce status snapshots.removeproduces the deleted event. Applying either event requires the same process-widesync_lock. ListSandboxesreturns all persisted sandbox rows, includingDeleting, so delayed event application remains user-visible after the delete RPC reports success.- Podman starts
/opt/openshell/bin/openshell-sandboxdirectly as container PID 1. The process-enabled supervisor awaits the entrypoint child but does not select on SIGINT / SIGTERM. The existing shutdown-signal helper is used only for network-only mode, so Linux PID 1 does not take the normal default SIGTERM action.
Affected Components
| Component | Key files | Role |
|---|---|---|
| CLI lifecycle | crates/openshell-cli/src/run.rs |
Runs the session and synchronously requests no-keep deletion. |
| Gateway compute runtime | crates/openshell-server/src/compute/mod.rs |
Serializes lifecycle/store mutations, invokes drivers, and applies watch events. |
| Podman driver and watcher | crates/openshell-driver-podman/src/driver.rs, crates/openshell-driver-podman/src/watcher.rs, crates/openshell-driver-podman/src/container.rs |
Stops/removes containers and translates Podman events into snapshots/deletions. |
| E2E coverage | e2e/rust/tests/sandbox_lifecycle.rs, .github/workflows/e2e-test.yml |
Executes concurrent lifecycle tests on host-native rootless Podman. |
Code References
| Location | Description |
|---|---|
crates/openshell-cli/src/run.rs:1744 |
finalize_sandbox_create_session awaits automatic deletion when the sandbox should not persist. |
crates/openshell-cli/src/run.rs:2345 |
TTY command completion enters the no-keep finalizer. |
crates/openshell-server/src/compute/mod.rs:566 |
delete_sandbox holds the process-wide sync_lock across the driver RPC. |
crates/openshell-server/src/compute/mod.rs:598 |
Store deletion happens synchronously only when the driver reports deleted = false. |
crates/openshell-server/src/compute/mod.rs:1083 |
Watch snapshots and deleted events feed the same runtime state machine. |
crates/openshell-server/src/compute/mod.rs:1112 |
Applying a watch snapshot waits for the same sync_lock. |
crates/openshell-server/src/compute/mod.rs:1347 |
Applying a deleted event also waits for the same sync_lock. |
crates/openshell-driver-podman/src/driver.rs:655 |
Podman delete waits for the configured stop timeout before removal. |
crates/openshell-driver-podman/src/watcher.rs:184 |
remove maps to deletion while stop / die map to status snapshots. |
crates/openshell-driver-podman/src/container.rs:895 |
The supervisor binary is installed directly as container entrypoint / PID 1. |
crates/openshell-sandbox/src/lib.rs:622 |
Process-enabled mode only awaits run_process. |
crates/openshell-sandbox/src/lib.rs:738 |
SIGINT / SIGTERM handling is explicitly limited to network-only mode. |
e2e/rust/tests/sandbox_lifecycle.rs:104 |
The no-keep E2E test uses a fixed 20 x 500 ms loop and sleeps after the final poll. |
.github/workflows/e2e-test.yml:95 |
Rootless Podman runs directly on Ubuntu hosts, where this timing is visible. |
Current Behavior
- Deleting sandbox A can block lifecycle events for unrelated sandbox B because external driver latency occurs inside one process-wide critical section.
DeleteSandboxcan report success while the sandbox remains visible inListSandboxesuntil a watcher event acquires that mutex.- Queued
stop/diesnapshots can regress a record fromDeletingtoErrororProvisioningbefore the laterremoveevent cleans it up. - Process-enabled Podman sandboxes routinely require SIGKILL after 10 seconds because the PID 1 supervisor does not handle SIGTERM.
- The E2E test's observation budget is exactly one stop timeout and can miss deletion during its final sleep.
Proposed Approach
Treat this as a gateway lifecycle synchronization fix, not a test-only retry:
- Do not hold the process-wide sandbox synchronization mutex across the external driver delete await. Preserve same-sandbox serialization or idempotency with an ID-scoped in-flight gate or equivalent mechanism while allowing watcher events for unrelated sandboxes to proceed.
- Keep the durable record in
Deletingwhile the driver request is in flight, then explicitly reconcile or recover it if the driver returns an error. Preserve event-driven deletion semantics for Kubernetes, where successful delete may mean accepted rather than physically complete. - Make delete reconciliation monotonic. Stale
stop/diesnapshots must not incorrectly revive a completed deletion, while genuine driver delete failures must still be allowed to recover fromDeleting. - Keep the E2E tests parallel as the regression scenario, but replace the fixed loop with a named deadline helper that performs a final check and has a budget greater than the configured stop timeout. Do not serialize the tests as the only fix, because that hides cross-sandbox head-of-line blocking.
Process-enabled PID 1 signal forwarding is deliberately out of scope. It explains the consistent 10-second Podman delay and deserves a separate product issue, but slow driver operations can still occur and must not block unrelated sandbox lifecycle events.
Alternative Approaches Considered
- Only increase retries: reduces CI noise but preserves slow PID 1 shutdown, global lock contention, and non-monotonic lifecycle state.
- Only serialize the two tests: avoids the observed overlap but hides the production behavior when users delete different sandboxes concurrently.
- Fix PID 1 signal forwarding in this issue: would reduce the common Podman delay but bundles a separate process-lifecycle change and would not protect unrelated sandboxes from other slow driver operations.
- Release the global lock without same-sandbox coordination: addresses head-of-line blocking but permits duplicate deletes and unsafe watcher interleavings for one sandbox.
Patterns to Follow
- Continue using CAS/resource versions for persisted phase transitions. The gateway architecture documents
update_message_casas the concurrency boundary for internal compute phase updates. - Preserve the existing
Deleting/Errorterminal-state protections used by supervisor-session updates. - Keep Podman watcher ordering explicit:
stop/dieare status observations, whileremoveis resource deletion. - Use deterministic fake-driver synchronization in unit tests rather than wall-clock sleeps to prove that one blocked delete cannot delay another sandbox's event.
Scope Assessment
- Complexity: Medium
- Confidence: High on the failure mechanism; medium on the final synchronization design
- Estimated files to change: 3 to 5
- Issue type:
fix - Documentation impact: Update
architecture/compute-runtimes.mdif delete completion or driver/watch ownership changes. No gateway TOML field or published config reference change is expected. - LSM impact: No direct SELinux/AppArmor policy change is required.
Risks and Open Questions
- What is the public contract of successful
DeleteSandbox: driver resource accepted for deletion, driver resource removed, or gateway record absent? Kubernetes deletion and finalizers make synchronous physical removal an unsafe assumption, while the CLI currently printsDeleted sandbox, so delayed list visibility is surprising. - Releasing the process-wide mutex around the driver RPC introduces same-sandbox races unless deletion ownership is explicit and stale watch snapshots are rejected.
- The current test
apply_sandbox_update_allows_delete_failures_to_recoverpermitsDeletingto becomeReady. The fix must distinguish a failed delete that should recover from an in-flight or completed delete that must remain monotonic. - HA gateways cannot rely only on an in-memory tombstone. Any deletion generation/ownership used for correctness must fit the persisted CAS model or be clearly limited to eliminating local queued events after the backend has been removed.
Test Considerations
- Add a gateway unit test with two sandbox IDs and a fake driver that blocks deletion A. Prove deletion/watch cleanup for B is not blocked by A.
- Add state-machine tests for stale
stop/diesnapshots arriving before and after theremoveevent. A completed delete must not regress or resurrect the row. - Keep the rootless Podman E2E test concurrent and make its eventual assertion deadline-based with a final check.
- Run the focused server unit/integration suites, the rootless Podman E2E path on Ubuntu 24.04 and 26.04, and the broader sandbox E2E path because this changes compute lifecycle behavior.
Created by spike investigation. Use build-from-issue to plan and implement.
Activity
pimlock commentedon Jul 16, 2026
Implementation Plan
Issue type:
fixComplexity: Medium
Confidence: High — clear path
Summary
Remove slow delete and recovery driver calls from the process-wide gateway critical section. Add deletion-specific per-sandbox gates to serialize duplicate request-side deletes and their recovery, while deliberately keeping live watcher events off those gates so the existing sequential
watch_loopnever waits behind a slow delete.Make watcher application safe through store-authoritative, idempotent state rules: ordinary snapshots are persisted-state no-ops while a row is
Deleting, deleted events may remove the row while the driver call is still in flight, and snapshots for absent rows are ignored. Preserve exact-version CAS as the HA correctness boundary and audit deletion/reconciliation lookups so no external driver await occurs under the global store/invariant guard.No keyed concurrent watch dispatcher is required for this issue. PID 1 signal forwarding and general driver-initiated sandbox adoption remain separate designs.
Scope
crates/openshell-server/src/compute/mod.rsComputeRuntime, initialized in every production and test constructor. Name it to reflect its narrow ownership—such asdelete_gatesorin_flight_deletes—rather than implying that all sandbox lifecycle events use it. Use weakly held entries or equivalent cleanup so historical sandbox IDs do not leak memory.sync_lockand re-fetch/revalidate the row before mutation. Never wait for a delete gate while holdingsync_lock; the only combined order isdelete gate → global guard.ComputeRuntime::delete_sandboxsosync_lockprotects only short store, CAS, index, notification, and owned-record cleanup operations. Release it beforedriver.delete_sandbox(...).await, while retaining the delete gate across the driver result and any recovery.Deletingmust not trigger a second driver delete; return the existing accepted/in-progress outcome consistently with the current API contract.watch_loop. Do not acquire a delete gate fromapply_watch_event,apply_sandbox_update, orapply_deleted; each event must finish after only short store/CAS work so an A status event cannot stop a later B deleted event behind A's slow driver call.Deletingrow a complete persisted-state no-op: do not change phase or status, increment the resource version, update the index, or notify the sandbox bus. This preserves the exactDeletingversion for failure recovery. A deleted event remains allowed to remove the row idempotently.spec, is persisted before driver creation, and an immediate watcher event updates that record rather than replacing it with a minimal snapshot.Deletingtransition; query the driver while holding only the delete gate; reacquire the global guard; then reconcile the observed backend state or CAS-restore only if the exactDeletingversion is still current. Never recreate a row already removed by a deleted event or overwrite another concurrent CAS update.reconcile_snapshot_sandboxandprune_missing_sandbox, which currently callget_driver_sandbox(...).awaitwhile holdingsync_lock. Split each into capture/validation, external lookup without the global guard, and re-fetch/exact-version reconciliation. These paths do not need the deletion gate; CAS and monotonic deletion rules resolve races.deleted = trueleaves final removal to a deleted event, which may arrive before or after the driver response;deleted = falseremoves the stale row synchronously. Delete completion and cleanup must tolerate the watcher already having removed the row.#[cfg(test)]module.e2e/rust/tests/sandbox_lifecycle.rsarchitecture/compute-runtimes.mddelete gate → global guardorder.Deletingno-op snapshots, idempotent deleted events, and store-authoritative handling of unknown/late snapshots.ffeaf0dd, not a supported driver-initiated adoption mechanism. Future adoption requires a separate design spanning live watch, reconciliation, complete specs, ownership, policy/identity, tombstones, and HA/CAS behavior.No changes are planned for
crates/openshell-cli/src/run.rs, the Podman driver/watcher,openshell-sandbox, or.github/workflows/e2e-test.yml. General create/delete serialization, future stop/restart lifecycle gates, and a concurrent keyed watch dispatcher are outside this issue.Implementation Steps
ComputeRuntimeconstructors.Deletingwith CAS.true,false, and error outcomes under a briefly reacquired global guard, tolerating rows already removed by the watcher.Deleting.GetSandboxlookup outside the global guard and exact-version CAS under the reacquired guard.Deletingrows to complete no-ops; ignore snapshots for absent rows; preserve complete API-created records; keep deleted events idempotent.sync_lock, followed by re-fetch/version validation before applying or pruning.Test Plan
Unit tests: In
crates/openshell-server/src/compute/mod.rs:DeleteSandboxfor sandbox A, emit an A status event followed by a B deleted event through the actual driver watch stream, and prove B is removed before A is unblocked. Callingapply_deleted(B)directly is insufficient.Deletingis handled idempotently.Deletingchanges neither the stored payload nor resource version and emits no index/bus update.specwhen a watcher event arrives immediately.deleted = falseremoves the row synchronously, and a previously queued snapshot cannot recreate it.deleted = trueleaves removal to the deleted event.GetSandboxcall and prove an unrelated watcher event can still acquire the global guard and apply.ReadyorProvisioningsandbox receivingContainerExitedstill becomesError.Integration tests: N/A as a separate test file. The in-process fake driver and real watch loop exercise the runtime/driver/watch boundary deterministically; the full user-visible workflow is covered by E2E.
E2E tests: Update
e2e/rust/tests/sandbox_lifecycle.rs, keep lifecycle tests parallel, and verify--no-keepeventually disappears with a deadline exceeding the 10-second stop timeout. Runmise run e2e:podman; the existing rootless workflow supplies Ubuntu 24.04/Podman 4 and Ubuntu 26.04/Podman 5 coverage. Also runmise run pre-commit.Risks & Open Questions
Deletingsnapshots being complete no-ops, deleted events being idempotent, absent snapshots being ignored, and recovery using exact-version CAS.Deletingresource version; otherwise it could spuriously prevent delete-failure recovery from matching the transition it owns.sync_lock.deleted = truemust remain watcher-owned.Documentation Impact
architecture/compute-runtimes.mdwith delete-gate scope/order, external-await boundaries, intentional watcher interleaving, store authority, monotonic deletion, and delete/watch ownership.docs/reference/gateway-config.mdxordocs/reference/sandbox-compute-drivers.mdx.LSM Compatibility
/proc, process-identity, or binary-execution behavior changes./proc/<pid>/exefailures.Revision 3 — narrowed to deletion-specific gates and CAS-driven sequential watcher progress; deferred keyed concurrent dispatch
Revision 2 — added bounded keyed watch dispatch, store-authoritative history, lock-order/external-await audit, and expanded state/concurrency regressions
Revision 1 — initial plan
pimlock commentedon Jul 17, 2026
Follow-up investigation: missing gateway rows in the live watch path
While reviewing the proposed store-authoritative handling of late driver snapshots, we traced why
apply_sandbox_update_lockedcurrently creates a minimalSandbox { spec: None }when a driver snapshot has no corresponding gateway row.Git history
d5d3c71e(2026-02-03, initial Kubernetes sandbox implementation) introduced the Kubernetes watch handler and its missing-row fallback.ba78e278(2026-03-07) documented the reason: the create API created the Kubernetes resource before persisting the gateway row, so the watcher could win that race and needed to create a placeholder.ffeaf0dd(2026-03-08, fix(sandbox): fix create ordering race, dual-registry credentials, and policy identity clearing #176) fixed the race by reversing the order: persist the complete gateway record first, then create the Kubernetes resource. The missing-row fallback remained.60035c6a(2026-04-14, refactor(server): extract kubernetes compute driver #817) moved the behavior into the compute-driver abstraction.7f16d60e(2026-05-18, feat(db) resource version cas #1292) converted the fallback write to CAS/MustCreate, but retained the behavior.This history suggests the live watch creation branch is a legacy workaround for an API creation-order race that no longer exists, rather than a deliberately designed driver-initiated sandbox registration feature.
Current behavior
The live watcher is driver-wide, not started per known sandbox. Kubernetes watches all supported
AgentSandboxresources in the configured namespace, and Podman watches all containers carrying the OpenShell managed labels. A snapshot for an unknown ID can therefore reachapply_sandbox_update_locked, which currently creates a minimal gateway record.The periodic reconciliation loop behaves differently.
reconcile_snapshot_sandboxlooks up the gateway row and returns immediately when it is absent. It does not create sandboxes that exist on the driver side but not on the gateway side. In current production code, the only top-level sandbox creation paths are:spec: None.Recommendation for #2326
Treating the gateway store as authoritative in the ordinary live watch path—ignoring a status snapshot when its row is absent—is consistent with both the current API creation ordering and the reconciliation loop.
If OpenShell wants to support driver-initiated sandboxes in addition to API-initiated sandboxes, that capability should be designed holistically rather than preserved through this incidental fallback. The design should include at least:
This may warrant a separate issue/design proposal. For #2326, removing the live watch's ability to create a missing row appears consistent with the rest of the current control-plane model.
pimlock commentedon Jul 17, 2026
Implementation Complete
Draft PR: #2340
What was built
Sandbox deletion is serialized per stable sandbox ID, while driver and reconciliation waits no longer hold the gateway-wide state guard. Deletion transitions use version-bound recovery and stable-ID row removal; existing best-effort name-based settings cleanup remains unchanged.
Tests
test:e2eCI requested at commit8283eecff739992796381daf88841b2fe2f09714mise run pre-commitpassedDocs updated
architecture/compute-runtimes.md: concise deletion invariants and restart behaviorThe issue will auto-close when the PR is merged.
lunarwhite commentedon Jul 21, 2026
Appreciate your work! Just appending a new occurrence: #2177 (comment)