fix: make js_image_layer tar output independent of the spawn strategy - #2998
Open
acozzette wants to merge 9 commits into
Open
fix: make js_image_layer tar output independent of the spawn strategy#2998acozzette wants to merge 9 commits into
acozzette wants to merge 9 commits into
Conversation
This change enables `--experimental_use_hermetic_linux_sandbox` on CI and makes a small fix to `js_image_layer` in order to do so. I think this CI coverage would be valuable to have, because without this flag it is still possible to escape the `linux-sandbox` by following symlinks. Ideally we want to make sure we are limiting ourselves to declared inputs and not inadvertently relying on files that are supposed to be outside the sandbox. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Run //... under --config=hermetic-sandbox from a dedicated job rather than a step in `test`, so it gets an output base of its own and covers only the root workspace -- the config lives in this workspace's .bazelrc and the e2e workspaces have not been vetted under it. Sandbox flags are not part of the action key, so any cache hit satisfies an action and the job would replay the ordinary legs' outputs without ever entering the hermetic sandbox. A fresh runner only empties the local action cache, so the job also needs --noremote_accept_cached for the shared remote cache and --disk_cache= for a disk cache, which that flag does not cover. --noremote_upload_local_results keeps its outputs out of the shared cache so a strategy-dependent action can never poison the other legs. Note the same caching in .bazelrc: adding the config to an existing output base re-runs nothing, which makes it easy to think the build is hermetic when no action has actually executed in the sandbox. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The job passed on a commit that deliberately reintroduced the js_image_layer bug. It never entered the hermetic sandbox at all: its summary line read "5894 processes: 2901 internal, 98 local, 2895 processwrapper-sandbox", with zero linux-sandbox spawns. ubuntu-24.04 sets kernel.apparmor_restrict_unprivileged_userns=1, which blocks the CLONE_NEWUSER linux-sandbox needs. Bazel's support probe fails and it falls back to processwrapper-sandbox silently, and since --experimental_use_hermetic_linux_sandbox only applies to linux-sandbox, the whole config became a no-op that turned the job green unconditionally. Clear the restriction, then guard the result twice, because a check that cannot fail is worse than no check: probe linux-sandbox up front so an unusable sandbox fails fast with an obvious reason, and fail the job if any spawn reports processwrapper-sandbox. A machine where linux-sandbox works reports only linux-sandbox and local, so the assertion does not false-positive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a sysctl Replaces the `sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0` step. ubuntu-22.04 predates that restriction, so linux-sandbox works there with no privileged setup, and the root workspace's `test` leg already runs on this runner group. The two runners were measured on the same commit and the same 5894 spawns: ubuntu-24.04 reported "2901 internal, 98 local, 2895 processwrapper-sandbox" while ubuntu-22.04-32core reported "2901 internal, 2895 linux-sandbox, 98 local". The linux-sandbox probe goes too: pinning the image covers the known cause, and the processwrapper-sandbox assertion still fails the job if a future image brings the silent fallback back, which is the property that actually matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit 4e8e5a0.
acozzette
marked this pull request as ready for review
September 1, 2026 16:42
--strategy=linux-sandbox now makes Bazel fail rather than fall back, so the grep over the teed log is gone and nothing reads the file any more. `set -o pipefail` only existed to stop `tee` from masking a failing `aspect test`, so both go together -- keeping the pipe without pipefail would quietly make the job unable to fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thesayyn
approved these changes
Sep 1, 2026
jbedard
reviewed
Sep 1, 2026
| // hard link, but only when the source file's st_nlink > 1. The standard linux sandbox | ||
| // symlinks inputs (nlink 1) while the hermetic one hard links them (nlink 2), so | ||
| // without this the tar bytes would differ by spawn strategy. | ||
| return `${dest} uid={{UID}} gid={{GID}} time=0 mode={{FILE_MODE}} nlink=1 type=file content=${vis( |
Member
There was a problem hiding this comment.
Where did you/robot learn about this fix? Has this been done anywhere else (in the bazel ecosystem)?
Contributor
Author
There was a problem hiding this comment.
Claude somehow figured out this fix. The issue is that we call bsdtar and give it an mtree spec to tell it the files we want to put it in the output .tar file representing an image layer. Apparently if the mtree spec defines two or more entries that both point to the same file via content=somefile and somefile has more than one link, then bsdtar will encode them as hard links, unless we explicitly say we only want one link via nlink=1.
There does seem to be a precedent for this as I see that tar.bzl does the same thing: bazel-contrib/tar.bzl#99
I suspect we should do the same thing in rules_py if we want to make it compatible with --experimental_use_hermetic_linux_sandbox, because I found that a bunch of tests under e2e/cases/oci there fail with what appears to be the same issue.
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.
This change enables
--experimental_use_hermetic_linux_sandboxon CI and makes a small fix tojs_image_layerin order to do so.I think this CI coverage would be valuable to have, because without this flag it is still possible to escape the
linux-sandboxby following symlinks. Ideally we want to make sure we are limiting ourselves to declared inputs and not inadvertently relying on files that are supposed to be outside the sandbox.Changes are visible to end-users: yes
Test plan