[xyuzh]

Description

Each image lookup on a create scans the whole cache for eviction (sizing every cached image and live sandbox marker), then takes the image's exclusive lock, so creates of one image queue. Each bundle also runs runsc spec, a Go process, for the same default spec.

This PR:

• Has a failed pull raise its own error instead of a wrapped one. (The backend already does the one lookup that pins the image, since [core][sandbox] Stop a sandbox with its owner, and reclaim what it leaves #67022 landed.)
• Registers a cache hit under a shared lock. A hit skips the eviction scan unless the disk is below [core][sandbox] Keep a free-space floor under the image cache #67025's free-space floor, which it checks with two statvfs calls; a pull still scans under the size cap and the floor, before it starts and again once its own image counts.
• Caches the default spec per runsc binary.
• Logs the warning about falling back to a writable root overlay once per process.

Measured on 2026-10-14 against base 00a3adb, before #67025 and the floor check (16 cores, medians of 5 alternating runs; the other images were stand-in cache entries):

	master	this PR
Cache hit, 100 other images, 96 live sandboxes	3.32 ms	0.088 ms
48 creates at once, idle node	86/s	94/s
48 creates at once, 100 other images, 96 live	24/s	90/s

Related issues

Split out of #66821.

Additional information

• Not a duplicate: no open PR makes hits skip the scan or caches the spec. [core][sandbox] Enforce CPU and memory limits with host cgroups #66932 also drops the duplicate lookup and [core][sandbox] Boot sandboxes from an overlayfs rootfs if required #66470 the warning; [core][sandbox] Run runsc CLI calls with GOMAXPROCS=2 #67075 and [core][sandbox] Keep sandboxes booted with SandboxRuntime warm pools #67077 edit the same functions. Each is a small merge.
• Tests: six new tests cover the spec cache, no scan on a hit above the floor, eviction on a hit below it, a hit during another's shared lock, one lookup, and a failed pull. Four fail on master; the floor and failed-pull tests fail without this PR's floor check and its re-raise of the pull's error.
• Tested with master 541bd56 merged in, in a privileged container (aarch64, runsc release-20261026.0, erofs-utils 1.9.4): TEST_SANDBOX=1 pytest tests/test_{gvisor_backend,checkpoint_restore,image_manager,image_utils,sandbox_runtime,overlayfs,sandbox_config}.py: 368 passed, 15 skipped (master: 361 passed, 15 skipped). pre-commit passes.
• I used Claude Code to help write and test this change.

[gemini-code-assist]

Code Review

This pull request optimizes sandbox creation and image caching performance. Key changes include caching the default OCI specification per runsc binary to avoid redundant process spawning, introducing a fast path with shared locks for image cache hits, and delegating image pulling and pinning entirely to the backend to prevent redundant lookups and clean up failed creations. Feedback on these changes suggests optimizing the _short_of_free_space helper to reduce statvfs system calls to at most one, and opening lock files in append mode ('a') instead of write mode ('w') to avoid unnecessary file truncation and metadata contention under high concurrency.

[gemini-code-assist]

The _short_of_free_space function currently calls image_cache_min_free_bytes and _free_bytes, both of which may invoke shutil.disk_usage (which performs a statvfs system call). This results in up to two statvfs calls on every single cache hit. We can optimize this to at most one statvfs call by directly calling shutil.disk_usage once and using its returned total and free fields.

def _short_of_free_space(images_dir: str) -> int:
    """The free-space floor if the cache's filesystem is below it, else 0.

    At most one statvfs call, cheap enough for every cache hit.
    """
    raw = os.environ.get(_IMAGE_CACHE_MIN_FREE_BYTES_ENV)
    if raw is not None and raw.strip():
        try:
            min_free = max(int(raw), 0)
        except ValueError:
            min_free = None
    else:
        min_free = None

    try:
        if min_free is not None:
            if min_free == 0:
                return 0
            free = shutil.disk_usage(images_dir).free
            return min_free if free < min_free else 0
        else:
            usage = shutil.disk_usage(images_dir)
            min_free = usage.total // 10
            return min_free if usage.free < min_free else 0
    except OSError:
        return 0

[xyuzh]

Not changing this: two statvfs calls cost a few microseconds against a 0.09 ms cache hit, and folding them into one would duplicate how image_cache_min_free_bytes reads RAY_SANDBOX_IMAGE_CACHE_MIN_FREE_BYTES.

[gemini-code-assist]

Opening the lock file with mode "w" truncates the file. Under high concurrency (e.g., many sandboxes starting at once and hitting the cache), multiple processes concurrently truncating the same lock file causes unnecessary write I/O and metadata contention on the filesystem. Since the lock file is only used for flock and its content is irrelevant, opening it with mode "a" (append mode) is a safer and more efficient alternative that avoids truncation while still creating the file if it does not exist.

Suggested change

	with open(lock_path, "w", encoding="utf-8") as f_lock:
	with open(lock_path, "a", encoding="utf-8") as f_lock:

[xyuzh]

Done in 12ec976: both opens in pull_and_extract_container_image use "a" now. flock doesn't care about the mode, and a cache hit no longer truncates the file.

[gemini-code-assist]

Similarly, opening the lock file with mode "w" here truncates the file. Using mode "a" (append mode) is preferred to avoid unnecessary write I/O and metadata updates on the filesystem.

Suggested change

	with open(lock_path, "w", encoding="utf-8") as f_lock:
	with open(lock_path, "a", encoding="utf-8") as f_lock:

[xyuzh]

Same commit, 12ec976.

[xyuzh]

@pcmoritz Rebased onto master, with one change after your approval. This PR used to pull the image before making the sandbox's directory, but #67022 now has the create take the bundle's owner file first, so that a reclaim can release the image pin of a worker that dies mid-create. The pull now stays after the owner file, and a SandboxCreationError from it is raised as is instead of wrapped, which keeps the failed-pull test's behavior. On a CoreWeave pod it's 174 passed and 1 skipped against master's 168 and 1, plus 21 netns tests passed.

[xyuzh]

@pcmoritz I merged master in ea07691. Two things changed after your approval, both where master moved code this PR touches:

• [core][sandbox] Move an image aside before deleting it #66469 now finishes interrupted image deletions at the top of every pull. I kept that call on every pull, cache hits included. It's one listdir, and it means a node that only ever hits its cache still gets that space back. Eviction stays where this PR put it: on a pull, or on a hit below the free-space floor.
• [core][sandbox] Boot sandboxes from an overlayfs rootfs if required #66470 now boots a readonly sandbox with a workdir from a kernel overlay, so the warning this PR rate-limited no longer exists. The once-per-process rule moved to the warning that remains, for workers that can't mount an overlay and fall back to a writable root. test_writable_root_fallback_warns_once_per_process covers it.

Real gVisor in a privileged container (aarch64, runsc release-20261026.0): test_{gvisor_backend,checkpoint_restore,image_manager,image_utils,sandbox_runtime,overlayfs,sandbox_config}.py 368 passed, 15 skipped (master: 361 passed, 15 skipped).

[xyuzh]

Merged master in 5df6c08 and adapted one test in 67226c2. Master's #67874 added test_layer_cache_reclaimed_before_images, whose size case used a cache hit to start the eviction scan. With this PR a hit scans only when the disk is below the free-space floor, and the size cap waits for the next pull, which is what the docs on master say ("Before each pull"). That case now pulls a second image to start the scan, and the free-space case still uses a hit. Nothing else changed.

macOS: 468 passed, and the same 31 environment failures as master. Real gVisor (runsc release-20261026.0): test_image_utils.py, test_image_manager.py and test_sandbox_runtime.py, 138 passed.

@pcmoritz, this is the only change since your approval.

[sunnylovestiramisu]

Checking RAY_SANDBOX_IMAGE_CACHE_MAX_BYTES only before a pull (while the new image contributes 0 bytes) and skipping it after build_erofs_image and on cache hits leaves the cache above its byte cap indefinitely once a pull or later unpin crosses it. Have you considered recording each EROFS image's size in .extracted and re-checking max_cache after build_erofs_image?

[xyuzh]

Fixed for pulls in a7d3c36: a pull scans for the size cap again once its own image is in the cache, so a pull that crosses the cap evicts right away. Test: test_a_pull_that_takes_the_cache_over_its_size_cap_evicts_at_once. An image that was pinned when the cap was crossed still waits for the next pull after it's released, since a hit doesn't grow the cache. Recording sizes in .extracted would make a check on every hit affordable; I left that out of this PR.

[sunnylovestiramisu]

Running _remove_stale_deletions unlocked on every cache hit races active evictions that moved a directory to .deleting. and are running shutil.rmtree outside the image lock, making concurrent cache hits block on synchronous rmtree.

[xyuzh]

Fixed in a7d3c36: on a cache hit the sweep skips a .deleting.* directory that changed in the last minute, which is one an eviction is still deleting, and takes the rest. A pull still sweeps everything after it releases the lock. Test: test_a_cache_hit_leaves_a_deletion_in_progress_to_its_owner.

[xyuzh]

Update: 25e7de9 replaces the one-minute rule with a lock. A deletion holds a lock on the directory it removes, and a cache hit skips a directory whose lock is held, so it's exact for a deletion of any length.

[sunnylovestiramisu]

Have you considered passing the already-pinned image_dir from _start_sandbox into prepare_oci_bundle / create_oci_spec so each create doesn't call pull_image twice on a cache hit?

[xyuzh]

Not changing this: with this PR a cache hit costs about 32 µs (5,000 hits on a cached busybox image in the test container), so the second lookup isn't worth threading the pinned directory through prepare_oci_bundle and create_oci_spec, which are part of BaseImageManager's interface.

[sunnylovestiramisu]

_default_spec_lock isn't held across subprocess.run(['runsc', 'spec']), so a cold multi-thread burst spawns concurrent runsc spec processes; also consider passing runsc from shutil.which('runsc') rather than re-searching PATH.

[xyuzh]

Fixed in a7d3c36: the lock is held while runsc spec runs, so a burst of first creates runs it once, and it runs the binary that shutil.which found and the cache is keyed on. Test: test_a_burst_of_first_creates_runs_runsc_spec_once.

[sunnylovestiramisu]

PR description nit: item 1 already landed on master (this diff only adds a comment here), and item 3's warning in image_manager.py now covers the writable-root overlayfs fallback rather than readonly workdir.

[xyuzh]

Fixed in the description: the first bullet now says only what this diff does there, and the warning is described as the writable-root fallback.

[xyuzh]

Thanks for the review. a7d3c36 addresses the size cap, the stale-deletion sweep and the runsc spec lock, and the description is corrected.

@pcmoritz, the change since your approval is that commit: a pull checks the size cap a second time after its image is built, a cache hit only sweeps up moved-aside directories that have sat for a minute, and runsc spec runs under the cache's lock.

[xyuzh]

Fixed for pulls in a7d3c36: a pull scans for the size cap again once its own image is in the cache, so a pull that crosses the cap evicts right away. Test: test_a_pull_that_takes_the_cache_over_its_size_cap_evicts_at_once. An image that was pinned when the cap was crossed still waits for the next pull after it's released, since a hit doesn't grow the cache. Recording sizes in .extracted would make a check on every hit affordable; I left that out of this PR.

[xyuzh]

Fixed in a7d3c36: on a cache hit the sweep skips a .deleting.* directory that changed in the last minute, which is one an eviction is still deleting, and takes the rest. A pull still sweeps everything after it releases the lock. Test: test_a_cache_hit_leaves_a_deletion_in_progress_to_its_owner.

[xyuzh]

Not changing this: with this PR a cache hit costs about 32 µs (5,000 hits on a cached busybox image in the test container), so the second lookup isn't worth threading the pinned directory through prepare_oci_bundle and create_oci_spec, which are part of BaseImageManager's interface.

[xyuzh]

Fixed in a7d3c36: the lock is held while runsc spec runs, so a burst of first creates runs it once, and it runs the binary that shutil.which found and the cache is keyed on. Test: test_a_burst_of_first_creates_runs_runsc_spec_once.

[xyuzh]

Fixed in the description: the first bullet now says only what this diff does there, and the warning is described as the writable-root fallback.

[cursor]

Stale Bugbot comment from a previous run.

[xyuzh]

@pcmoritz, one more commit since your approval: 25e7de9 replaces the one-minute rule from a7d3c36 with a lock on each directory that's being deleted.

[xyuzh]

Update: 25e7de9 replaces the one-minute rule with a lock. A deletion holds a lock on the directory it removes, and a cache hit skips a directory whose lock is held, so it's exact for a deletion of any length.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 25e7de9. Configure here.}

[xyuzh]

@pcmoritz, one more commit since your approval: aa07e86 closes a window Bugbot found in 25e7de9, where a cache hit could delete an image that another pull had just moved aside.