[core][sandbox] Preserve archived mtimes when extracting image layers - #65737
Merged
Conversation
extract_tar_layer wrote files with copyfileobj and never called os.utime, so every file in the sandbox rootfs carried the extraction time as its mtime. That breaks tools that trust timestamps — most visibly apt, which revalidates its package lists with If-Modified-Since derived from the file mtime: with a reset-to-now mtime, mirrors answer 304 and apt keeps stale image-baked lists whose Release files have long expired, failing every 'apt-get install' with 404s. Found running Terminal-Bench 2.1 under Harbor, where every verifier bootstraps with apt-get and ~30% of tasks failed on this. Directory mtimes are applied after the extraction loop (extracting children would bump them) and are best-effort. Signed-off-by: xyuzh <xinyzng@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the container image extraction logic to preserve archived modification times (mtimes) for files and directories, ensuring tools like apt inside the sandbox can correctly validate package lists. A corresponding unit test was also added to verify this behavior. The review feedback suggests two robust improvements: wrapping the file mtime update in a try-except block to handle potential OS errors gracefully, and checking if a directory path is a symlink before applying mtime updates to avoid corrupting target directories in UsrMerge environments.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 9657d20. Configure here.
This was referenced Aug 26, 2026
2 tasks
andrewsykim
reviewed
Aug 27, 2026
Review feedback: wrap the per-file utime in try/except OSError like the directory pass (preserving mtimes is best-effort), and skip preserved symlinks in the directory pass — under UsrMerge images (/bin -> usr/bin) utime would follow the link and stamp the target with the wrong time. Signed-off-by: xyuzh <xinyzng@gmail.com>
Fix another small nit pointed out by Andrew Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
pcmoritz
approved these changes
Aug 28, 2026
pcmoritz
pushed a commit
that referenced
this pull request
Aug 31, 2026
## Why are these changes needed? Two filesystem-behavior divergences from Docker, both found running Terminal-Bench 2.1 oracle evaluations in Ray sandboxes (via the Harbor integration): 1. **`localhost` does not resolve.** Container engines inject `/etc/hosts` at run time, so images do not ship a usable one — and the sandbox never provided it either. glibc consults hosts files before DNS, and public resolvers will not answer for `localhost`, so every task that starts a local server and connects to it fails with `Name or service not known` (6 of 89 TB tasks). Fix: generate a per-sandbox hosts file next to the generated `resolv.conf` (seeded from the node's file under host networking) and bind-mount it **read-write**, matching engine behavior — the source is always a per-sandbox copy, never the node's file. 2. **`/tmp` sits on a different device than the rootfs.** runsc mounts a private tmpfs over an *empty* `/tmp`, so the common tempfile-then-`rename(2)` pattern fails with `EXDEV` where it works under Docker (plain rootfs `/tmp`). Fix: seed the extracted rootfs `/tmp` with a placeholder so runsc keeps it on the rootfs; **readonly** sandboxes get an explicit tmpfs mount instead, preserving the always-writable `/tmp` they have today. ## Related issue number Follow-up to #65570, sibling of #65737. ## Checks - [x] Signed off (DCO); pre-commit hooks pass on the changed files. - [ ] Unit tests included (`test_oci_spec_docker_parity_hosts_and_tmp`, `test_prepare_oci_bundle_writes_hosts_file`); both run without gVisor. --------- Signed-off-by: xyuzh <xinyzng@gmail.com>
pcmoritz
added a commit
that referenced
this pull request
Sep 1, 2026
## Why are these changes needed? Sandbox image pulls are anonymous Docker Hub pulls. A cluster of nodes pulling distinct multi-GB benchmark images concurrently runs straight into Docker Hub's anonymous rate limits and pays WAN latency on every node — during a Terminal-Bench 2.1 evaluation under Harbor, big-image tasks failed in the concurrent sweep but passed in isolation. `RAY_SANDBOX_REGISTRY_MIRROR` names a registry that mirrors Docker Hub, as `host[:port][/repo-prefix]`: - an **ECR pull-through cache** (`<acct>.dkr.ecr.<region>.amazonaws.com/dockerhub`), - an **Artifact Registry remote repository**, or - an in-cluster `registry:2` proxy. Docker Hub pulls are rewritten to the mirror (the prefix prepended to the repository, as ECR requires); other registries pass through untouched. This mirrors Docker's own `registry-mirrors` semantics, minus the fallback: when set, the mirror is authoritative, and it uses the same anonymous token flow as any registry. ## Related issue number Follow-up to #65570; sibling of #65737 and #65744. ## Checks - [x] Signed off (DCO); pre-commit hooks pass on the changed files. - [ ] Unit test included (`test_registry_mirror_rewrites_docker_hub_only`); runs without gVisor or network. --------- Signed-off-by: xyuzh <xinyzng@gmail.com> Signed-off-by: Philipp Moritz <pcmoritz@gmail.com> Co-authored-by: Philipp Moritz <pcmoritz@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
extract_tar_layerwrites regular files withcopyfileobjand never callsos.utime, so every file in an extracted sandbox rootfs carries the extraction time as its mtime instead of the archived one.That breaks in-sandbox tools that trust timestamps. The most visible victim is apt: it revalidates
/var/lib/apt/listswithIf-Modified-Sincederived from the file mtime, so with a reset-to-now mtime the mirror answers304 Not Modifiedfor image-baked lists whose Release files expired long ago — and everyapt-get installthen 404s on package versions that no longer exist. Docker-extracted images don't have this problem because layer extraction preserves mtimes.Found while running Terminal-Bench 2.1 oracle evaluations under Harbor against a Ray Sandbox deployment: every TB verifier bootstraps with
apt-get install curl, and ~30% of the suite failed with this signature (verified in a live sandbox:rm -rf /var/lib/apt/lists/* && apt-get updateimmediately fixes it).Fix: apply
os.utimefrom the tar member after writing each regular file; directory mtimes are applied after the extraction loop (extracting children would bump them), best-effort.Related issue number
Follow-up to #65570.
Checks
test_extract_tar_layer_preserves_mtimes(pure, no runsc needed).