NVIDIA / NVIDIA/OpenShell

refactor: centralize persisted sandbox lifecycle traversal

Open
#2,827 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

state:stale
Dominant language
Rust
Stars
8.7k
Forks
1.3k
Avg merge
2d 11h
Merged PRs (30d)
253

Description

Description

Evaluate a small refactor that centralizes the common traversal mechanics used by persisted sandbox lifecycle sweeps without hiding their distinct behavior.

start_persisted_sandboxes, stop_persisted_sandboxes_on_shutdown, and persisted lifecycle recovery repeat the same general pattern:

  1. Capture persisted sandbox IDs.
  2. Acquire the per-sandbox lifecycle gate.
  3. Re-read the sandbox record while holding the gate.
  4. Handle records that disappeared or could not be read.
  5. Inspect the current phase.
  6. Perform an asynchronous, phase-specific action while retaining the gate.

Consider extracting only these common mechanics into a small traversal or map helper. For example, the caller could select the concurrency policy and provide an asynchronous callback. Start-, stop-, and recovery-specific RPCs, outcome handling, state mutations, aggregation, and logging should remain explicit at their call sites.

The refactor must preserve:

  • the shutdown stop sweep's bounded concurrency;
  • startup's current ordering and error semantics;
  • the lifecycle gate's lifetime across each operation; and
  • the correctness property that the sandbox record is re-read only after acquiring its gate.

This is a readability and maintainability follow-up to #2743. It should not block #2743.

This refactor does not solve the race in which a new sandbox can be created after the shutdown sweep captures its inventory. It does not replace #2826, which tracks quiescing and draining in-flight lifecycle requests before shutdown cleanup.

Context

PR #2743 expanded persisted sandbox start and shutdown-stop reconciliation. Review of its lifecycle loops found that their shared concurrency and correctness mechanics are difficult to compare, increasing the risk that future changes cause the implementations to drift.

The abstraction should remain deliberately narrow. A single generic lifecycle sweep that obscures the meaningful differences between start, stop, and recovery would make the code harder to review.

Related:

  • #2743
  • #2826

Definition of Done

  • Common traversal mechanics are centralized only if the resulting code is easier to read and review.
  • Start-, stop-, and recovery-specific RPCs, outcome handling, state mutations, aggregation, and logging remain visible at their call sites.
  • Stop preserves bounded concurrency, and start preserves its current ordering and error semantics.
  • The lifecycle gate remains held for the full per-sandbox operation, with the persisted record re-read after gate acquisition.
  • Tests cover gate/re-read behavior, disappeared-record skipping, read failures, and the caller-selected concurrency choices.
  • Existing lifecycle behavior has no regression.
  • The change does not claim to resolve or replace the shutdown-draining work tracked by #2826.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating start_persisted_sandboxes, stop_persisted_sandboxes_on_shutdown, and persisted lifecycle recovery, then compare their traversal mechanics and existing lifecycle tests. Preserve the gate lifetime, post-gate record reread, disappeared-record and read-failure handling, bounded stop concurrency, and startup ordering while keeping caller-specific behavior visible. Done means the shared mechanics are centralized only if the code is easier to review and all listed lifecycle behaviors remain covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.