Skip to content

fix(podman): wait for container stop completion - #2820

Merged
pimlock merged 2 commits into
mainfrom
fix-podman-stop-race/pimlock
Aug 20, 2026
Merged

fix(podman): wait for container stop completion#2820
pimlock merged 2 commits into
mainfrom
fix-podman-stop-race/pimlock

Conversation

@pimlock

@pimlockpimlock commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Wait for Podman to report a sandbox container as fully stopped before allowing the stop operation to complete. This prevents an immediate restart from being regressed to Error by the previous container run's delayed exit event.

Related Issue

No issue required: this is a localized fix for a confirmed race in the recently added Podman stop/start lifecycle.

Changes

  • Poll Podman inspect after a stop request until the container reaches exited or stopped.
  • Apply the same wait when retrying a stop while Podman already reports stopping.
  • Bound the wait by the configured stop timeout plus five seconds of completion headroom.
  • Add regression coverage for both the initial stop and idempotent retry paths.
  • Document the lifecycle completion invariant in the compute-runtime architecture and Podman driver README.

Race

A representative failure followed this sequence:

  1. StopSandbox asked rootless Podman to stop the container.
  2. The supervisor did not exit before the 15-second grace period, so Podman escalated from SIGTERM to SIGKILL.
  3. The stop API returned while the container still reported stopping.
  4. The test immediately called StartSandbox. Since the prior exit had not completed, the existing event fence had no FinishedAt timestamp to record.
  5. A delayed die/stop event from the previous run arrived after the new run entered Starting. The watcher reported exit code 137 and the gateway changed the sandbox from Starting to Error.

StartSandbox restarts the same logical sandbox and retained Podman container; it does not create a new sandbox. In compact form, the race is:

sandbox cc95, run 1: stop -> delayed die event
sandbox cc95, run 2: start -> Starting
delayed run-1 event -> update sandbox cc95 -> Error

Waiting for a terminal Podman state closes the gap between steps 3 and 4. The existing timestamp fence can then identify any delayed event from the completed prior run.

Failing jobs

Introduction context

The evidence points to the race being introduced with stop/start itself, rather than by a later regression:

  • Stop/start was developed in PR #2653 on August 12 and merged on August 13 as 0f8fad23.
  • The sandbox_stop_start_preserves_workspace E2E test was added in that same PR, so there is no test history from before the feature existed.
  • The first feature-branch E2E run failed with this exact test and Starting -> Error / exit-code-137 signature at 16:39 UTC on August 12, shortly after the initial implementation.
  • An event-fencing mitigation was added to the feature branch later that day, but post-merge runs continued to reproduce the race because the stop API could return before FinishedAt was available.
  • The lifecycle test and default Podman stop configuration have not materially changed since the feature merged.

The CI history establishes that the race existed in the initial implementation: the new test failed before merge and continued to fail afterward. It does not establish that the failure rate recently increased or that concurrent Branch E2E activity changed its frequency. The observed SIGTERM-to-SIGKILL path explains the race window, but the available evidence does not support a broader claim about why failures clustered over time.

Testing

  • mise run pre-commit passes
  • cargo fmt --all -- --check
  • cargo clippy -p openshell-driver-podman --all-targets -- -D warnings
  • cargo test -p openshell-driver-podman (176 passed)
  • Unit tests added/updated
  • E2E tests added/updated (existing sandbox_stop_start_preserves_workspace covers this path; rootless Podman execution is delegated to CI)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock

Copy link
Copy Markdown
CollaboratorAuthor

An alternative would be to add an explicit lifecycle epoch (or execution generation) to each sandbox run and have the gateway discard events from older epochs. Conceptually:

sandbox cc95, epoch 7: stop -> delayed die event tagged 7
sandbox cc95, epoch 8: start -> Starting
gateway receives epoch 7 while current epoch is 8 -> discard

That would be useful defense in depth, but it is a substantially larger change than this fix. A driver-local counter is not enough: Podman events contain a container ID and timestamps, but no native execution generation. If the driver increments its counter before a delayed epoch-7 event arrives, it could incorrectly tag that old event as epoch 8.

Making epochs reliable would require:

  • persisting the current generation in the gateway;
  • extending the compute-driver protocol so lifecycle requests, snapshots, and watch events carry it;
  • teaching the Podman driver to associate raw events with a specific execution, probably by correlating Podman event timestamps with StartedAt and FinishedAt;
  • defining behavior for driver reconnects, gateway/driver restarts, initial reconciliation, and ambiguous events;
  • updating every built-in driver plus external-driver compatibility/capability negotiation;
  • regenerating affected SDK/protobuf bindings and adding cross-driver reconciliation tests and documentation.

The sandbox ID already prevents events from unrelated sandboxes from crossing this boundary. The missing distinction is between two executions of the same retained sandbox/container.

For this PR, waiting until Podman reports the previous execution as terminal establishes the required causal boundary and gives the existing FinishedAt fence enough information to reject a delayed exit event. An explicit epoch could be pursued separately as a broader protocol-level hardening effort, but it should complement rather than replace correct stop completion semantics.

@pimlockpimlock added the test:e2e Requires end-to-end coverage label Aug 20, 2026
@pimlockpimlock self-assigned this Aug 20, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for f13b0a1. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@pimlock

Copy link
Copy Markdown
CollaboratorAuthor

@sjenning FYI. I think this makes sense, but I'd love if you could double check.

I will merge this, since the fix is quite contained and this helps with the CI, but feel free to propose something different (I explored one alternative and added the comment, but that would be much more involved).

@pimlock
pimlock enabled auto-merge August 20, 2026 00:59
@pimlock
pimlock added this pull request to the merge queueAug 20, 2026
Merged via the queue into main with commit 701382dAug 20, 2026
78 checks passed
@pimlock
pimlock deleted the fix-podman-stop-race/pimlock branch August 20, 2026 01:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2eRequires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pimlock@johntmyers