fix(op): use a BTreeSet for override_deps so rootfs assembly is deterministic - #1178
Conversation
📝 WalkthroughWalkthroughPatched builds and rootfs assembly now preserve dependency order. Sandbox mappings are deterministic. Local backend operations use GCS caches and ChangesDependency and execution updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
twitchyliquid64
left a comment
There was a problem hiding this comment.
Just change HashSet to BTreeSet, deterministic iteration order makes sense but i dont think we want to make it possible for someone to choose 'winning' packages because its an issue if files overlap and that should be fixed downstream.
a6027eb to
5eda5c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/op/src/patched.rs (1)
41-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve dependency precedence in
PatchedBuild.
transitive_runtime_depsis aHashMap, so sort its entries with the same declared-before-inherited and spec-hash ordering used bySpecBuild::rootfs_mapped. Then deduplicate cache paths while retaining the first occurrence.BTreeSetsorts paths and can select the wrong provider for conflicting files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/op/src/patched.rs` around lines 41 - 62, Update PatchedBuild’s dependency collection around Transitives::transitive_runtime_deps to use the same declared-before-inherited and spec-hash ordering as SpecBuild::rootfs_mapped, rather than relying on HashMap iteration order. Resolve dependencies in that order, deduplicate cache paths while retaining the first occurrence, and replace the BTreeSet-based path collection so conflicting providers preserve the intended precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/op/src/specs.rs`:
- Around line 318-344: Replace the `with_disable_networking` call in the sandbox
configuration with `with_network_mode`, selecting `NetworkMode::NoNet` when both
`needs_dns` and `needs_internet` are false, and `NetworkMode::HostNet`
otherwise. Preserve the existing build configuration and
`Sandbox::run_with_cancel` flow.
In `@crates/orchestrator/src/local_backend.rs`:
- Line 135: Update the `LocalBackend::remote_cache` field to use
`RemoteCache<AnyBackend>` instead of `RemoteCache<GcsStorage>`, matching the
type returned by `Context::remote_cache` and accepted by
`LocalBackend::new_orchestrator`; preserve support for HTTPS mirrors that do not
have a `GcsStorage` client.
- Line 186: Update LocalBackend::new_orchestrator and the remote-cache plumbing
to accept and preserve RemoteCache<AnyBackend> from Context::remote_cache(),
including HTTPS mirror support. Replace all three spawn_blocking(async move || {
... }) usages with synchronous blocking closures containing only blocking work,
or explicitly drive the async future with the runtime Handle, ensuring no future
executes on the async runtime after the second await.
In `@crates/sandbox2/src/lib.rs`:
- Around line 122-138: Preserve ordered rootfs precedence by changing
Config::rootfs and related construction in with_rootfs and with_add_rootfs to
use Vec<SandboxMapped>, deduplicating inserts while retaining the first
occurrence; remove the source-path sort in Sandbox::new and update the stale
rootfs_mapped comment. In crates/sandbox2/src/lib.rs lines 122-138, apply these
changes; in crates/op/src/specs.rs lines 31-41, make no direct change because
its documentation becomes correct once sandbox2 preserves order.
---
Outside diff comments:
In `@crates/op/src/patched.rs`:
- Around line 41-62: Update PatchedBuild’s dependency collection around
Transitives::transitive_runtime_deps to use the same declared-before-inherited
and spec-hash ordering as SpecBuild::rootfs_mapped, rather than relying on
HashMap iteration order. Resolve dependencies in that order, deduplicate cache
paths while retaining the first occurrence, and replace the BTreeSet-based path
collection so conflicting providers preserve the intended precedence.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f5ec6c9-659b-449e-ae9b-507fed2778a7
📒 Files selected for processing (4)
crates/op/src/patched.rscrates/op/src/specs.rscrates/orchestrator/src/local_backend.rscrates/sandbox2/src/lib.rs
| .with_disable_networking(!needs_dns && !needs_internet); | ||
| if let Some(a) = &build.build_args { | ||
| config = config.with_build_args(a.iter()); | ||
| } | ||
| if let Some(w) = self.cpu_weight { | ||
| config = config.with_cpu_weight(w); | ||
| } | ||
| if let Some(id) = &opts.daemon_id { | ||
| config = config.with_daemon_id(id.clone()); | ||
| } | ||
| let mut sandbox = config.build(&opts.exec_base, channel).await?; | ||
| sandbox.keep_dir(true); | ||
|
|
||
| info!("Building package: {}", build.name); | ||
| let start = Instant::now(); | ||
| self.execute_in_sandbox(&mut sandbox, build).await?; | ||
| sandbox | ||
| .run_with_cancel( | ||
| self.invocations(build)? | ||
| .into_iter() | ||
| .map(|(program, args)| sandbox2::config::Invocation { | ||
| executable: program, | ||
| args, | ||
| envs: Default::default(), | ||
| }) | ||
| .collect(), | ||
| self.stdout_writer.take(), | ||
| self.stderr_writer.take(), | ||
| self.cancel.clone(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C3 'fn (with_disable_networking|run_with_cancel)\b' crates/sandbox2
rg -nP -C2 '\b(with_disable_networking|run_with_cancel)\s*\(' --type=rustRepository: gominimal/minimal
Length of output: 693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sandbox2 config methods ---'
rg -n -C3 'with_(disable_networking|dns|network_mode|network)\b' crates/sandbox2/src crates/op/src
printf '%s\n' '--- run_with_cancel definition ---'
sed -n '810,875p' crates/sandbox2/src/lib.rs
printf '%s\n' '--- config builder definition and build path ---'
rg -n -C4 'struct Config|impl Config|pub fn build|disable_network|network_mode|network' crates/sandbox2/src
printf '%s\n' '--- call-site context ---'
sed -n '290,355p' crates/op/src/specs.rsRepository: gominimal/minimal
Length of output: 39866
Replace with_disable_networking with with_network_mode. Sandbox::run_with_cancel accepts the provided arguments, but with_disable_networking does not exist. Use NetworkMode::NoNet when both network requirements are false, and NetworkMode::HostNet otherwise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/op/src/specs.rs` around lines 318 - 344, Replace the
`with_disable_networking` call in the sandbox configuration with
`with_network_mode`, selecting `NetworkMode::NoNet` when both `needs_dns` and
`needs_internet` are false, and `NetworkMode::HostNet` otherwise. Preserve the
existing build configuration and `Sandbox::run_with_cancel` flow.
| pub(crate) sf: SF, | ||
| pub(crate) output_base: PathBuf, | ||
| pub(crate) remote_cache: Option<RemoteCache<AnyBackend>>, | ||
| pub(crate) remote_cache: Option<RemoteCache<GcsStorage>>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C5 'new_orchestrator\s*\(' --type=rust
rg -nP -C3 'RemoteCache<' --type=rustRepository: gominimal/minimal
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files 'crates/orchestrator/src/local_backend.rs' 'crates/mctx/src/lib.rs' '*orchestrator*' '*remote_cache*'
printf '%s\n' '--- RemoteCache and LocalBackend references ---'
rg -n -C4 'RemoteCache|LocalBackend|new_orchestrator|remote_cache' crates --glob '*.rs' || true
printf '%s\n' '--- local_backend outline and relevant sections ---'
ast-grep outline crates/orchestrator/src/local_backend.rs --view expanded || true
sed -n '100,160p' crates/orchestrator/src/local_backend.rs
sed -n '380,425p' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- mctx remote cache implementation ---'
rg -n -C12 'pub.*remote_cache|remote_cache' crates/mctx/src/lib.rs || true
sed -n '500,615p' crates/mctx/src/lib.rsRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- local backend imports, constructor, and fetch path ---'
sed -n '1,35p' crates/orchestrator/src/local_backend.rs
sed -n '286,320p' crates/orchestrator/src/local_backend.rs
sed -n '403,435p' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- mctx cache construction and orchestrator call ---'
sed -n '510,585p' crates/mctx/src/lib.rs
sed -n '710,775p' crates/mctx/src/lib.rs
printf '%s\n' '--- rcache backend types and constructors ---'
sed -n '1,180p' crates/rcache/src/remote.rs
rg -n -C4 'type AnyBackend|enum AnyBackend|new_any|new_any_configured|AnyUrl|FetchBackend' crates --glob '*.rs' | head -n 180
printf '%s\n' '--- read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
local = Path("crates/orchestrator/src/local_backend.rs").read_text()
mctx = Path("crates/mctx/src/lib.rs").read_text()
remote = Path("crates/rcache/src/remote.rs").read_text()
checks = {
"LocalBackend stores GcsStorage": "Option<RemoteCache<GcsStorage>>" in local,
"constructor accepts GcsStorage cache": "remote_cache: Option<RemoteCache<GcsStorage>>" in local,
"mctx returns AnyBackend cache": "Result<RemoteCache<AnyBackend>" in mctx,
"mctx constructs HTTPS without GCS storage": "if matches!(url, AnyUrl::Gcs(_))" in mctx and "gcs_storage = if" in mctx,
"mctx passes cache to LocalBackend": "LocalBackend::new_orchestrator" in mctx and "rc.clone()" in mctx,
"rcache has HTTPS constructor": "impl RemoteCache<Client>" in remote and "new_over_https" in remote,
"rcache has GCS constructor": "impl RemoteCache<Storage>" in remote and "new(" in remote,
}
for name, ok in checks.items():
print(f"{name}: {'YES' if ok else 'NO'}")
raise SystemExit(0 if all(checks.values()) else 1)
PYRepository: gominimal/minimal
Length of output: 28802
Use RemoteCache<AnyBackend> in LocalBackend. Context::remote_cache returns this type and passes it to LocalBackend::new_orchestrator. HTTPS mirrors use an AnyBackend without a GcsStorage client, so RemoteCache<GcsStorage> prevents compilation and removes HTTPS-mirror support at both fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/orchestrator/src/local_backend.rs` at line 135, Update the
`LocalBackend::remote_cache` field to use `RemoteCache<AnyBackend>` instead of
`RemoteCache<GcsStorage>`, matching the type returned by `Context::remote_cache`
and accepted by `LocalBackend::new_orchestrator`; preserve support for HTTPS
mirrors that do not have a `GcsStorage` client.
|
|
||
| let shared_hnd2 = shared_hnd.clone(); | ||
| let artifact = async move { | ||
| let artifact = spawn_blocking(async move || { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C2 'spawn_blocking\(\s*async' --type=rust
rg -nP 'rust-version|edition' Cargo.toml crates/orchestrator/Cargo.toml 2>/dev/null || trueRepository: gominimal/minimal
Length of output: 155
🌐 Web query:
tokio spawn_blocking async closure returns future not executed on blocking thread
💡 Result:
tokio::task::spawn_blocking is designed specifically for synchronous, blocking code, not asynchronous code [1][2][3]. If you pass an async closure to spawn_blocking, it will not execute the returned Future [4]. Instead, it will simply treat the Future as the return value of the closure and return it as the output of the task, leaving the Future un-polled (unexecuted) [4]. Because spawn_blocking expects a closure that returns a value directly (not a Future), attempting to provide an async block will often result in a compilation error (e.g., "expected FnOnce<()> closure, found impl Future") [4]. If you need to run asynchronous code from within a blocking thread pool, you must manually drive the Future to completion using a runtime handle, typically via tokio::runtime::Handle::block_on [4][5]: rust tokio::task::spawn_blocking(move || { // Get the current runtime handle let rt = tokio::runtime::Handle::current(); // Block on the async future to execute it rt.block_on(async move { // Your async code here }) }); Key takeaways: - spawn_blocking runs synchronous code on a dedicated thread pool to avoid blocking the main async executor threads [1][6][7]. - It does not automatically poll or execute async blocks [4]. - Using block_on inside spawn_blocking is a common pattern when bridge-crossing from blocking code back into an async context, but it should be used judiciously to avoid thread pool exhaustion [4][8][5].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html
- 2: https://docs.rs/tokio/latest/tokio/task/
- 3: https://rust-lang.github.io/async-book/part-guide/io.html
- 4: FR: async spawn_blocking tokio-rs/tokio#2866
- 5: https://stackoverflow.com/questions/76965631/how-do-i-spawn-possibly-blocking-async-tasks-in-tokio
- 6: https://users.rust-lang.org/t/tokio-calling-sync-operation-from-async-and-awaiting-still-blocks-the-thread/85990
- 7: https://users.rust-lang.org/t/spawn-vs-spawn-blocking-tokio/128174
- 8: https://stackoverflow.com/questions/71001701/tokio-spawn-blocking-when-passing-reference-requires-a-static-lifetime
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate file map ---'
ast-grep outline crates/orchestrator/src/local_backend.rs --lang rust 2>/dev/null || true
printf '%s\n' '--- relevant source ---'
sed -n '120,225p' crates/orchestrator/src/local_backend.rs
sed -n '270,305p' crates/orchestrator/src/local_backend.rs
sed -n '345,375p' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- exact spawn_blocking call sites ---'
rg -n -C3 'spawn_blocking|async[[:space:]]+move' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- cache type declarations and uses ---'
rg -n -C3 'RemoteCache|GcsStorage|new_orchestrator|remote_cache' crates/orchestrator/src crates/mctx/srcRepository: gominimal/minimal
Length of output: 34818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete build call ---'
sed -n '180,270p' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- complete hydrate call ---'
sed -n '285,355p' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- complete subset call ---'
sed -n '360,425p' crates/orchestrator/src/local_backend.rs
printf '%s\n' '--- remote cache API definitions ---'
rg -n -C5 'pub struct RemoteCache|type AnyBackend|enum AnyBackend|new_any_configured' crates
printf '%s\n' '--- orchestrator construction call ---'
sed -n '515,585p' crates/mctx/src/lib.rs
sed -n '715,740p' crates/mctx/src/lib.rs
printf '%s\n' '--- manifest toolchain constraints ---'
rg -n 'rust-version|edition|tokio' Cargo.toml crates/orchestrator/Cargo.toml crates/mctx/Cargo.tomlRepository: gominimal/minimal
Length of output: 21855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- configured cache constructor return type ---'
sed -n '235,285p' crates/rcache/src/remote.rs
printf '%s\n' '--- backend trait and concrete implementations ---'
sed -n '380,435p' crates/common/src/fetchers.rs
printf '%s\n' '--- read-only source verifier ---'
python3 - <<'PY'
from pathlib import Path
local = Path("crates/orchestrator/src/local_backend.rs").read_text()
mctx = Path("crates/mctx/src/lib.rs").read_text()
remote = Path("crates/rcache/src/remote.rs").read_text()
assert local.count("spawn_blocking(async move ||") == 3
assert local.count(".await\n .unwrap()\n .await") == 3
assert "remote_cache: Option<RemoteCache<GcsStorage>>" in local
assert "remote_cache: Option<RemoteCache<AnyBackend>>" in mctx
assert "Result<RemoteCache<AnyBackend>" in mctx
assert "rc.clone()," in mctx
assert "pub async fn new_any_configured(" in remote
print("three async closures are passed to spawn_blocking and each returned future is awaited")
print("orchestrator requires RemoteCache<GcsStorage>")
print("context remote_cache returns RemoteCache<AnyBackend>")
print("new_any_configured constructs the dynamic backend used by HTTPS and GCS URLs")
PYRepository: gominimal/minimal
Length of output: 4345
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
local = Path("crates/orchestrator/src/local_backend.rs").read_text()
mctx = Path("crates/mctx/src/lib.rs").read_text()
remote = Path("crates/rcache/src/remote.rs").read_text()
checks = {
"three async spawn_blocking closures": local.count("spawn_blocking(async move ||") == 3,
"three nested awaits": local.count(".await\n .unwrap()\n .await") == 3,
"orchestrator GCS cache field": "remote_cache: Option<RemoteCache<GcsStorage>>" in local,
"context AnyBackend return": "Result<RemoteCache<AnyBackend>" in mctx,
"direct rc clone argument": "rc.clone()," in mctx,
"dynamic constructor": "pub async fn new_any_configured(" in remote,
}
for name, value in checks.items():
print(f"{name}: {value}")
print("nested await occurrences:", local.count(".await\n .unwrap()\n .await"))
print("context return signatures:")
for line in mctx.splitlines():
if "Result<RemoteCache" in line:
print(line.strip())
PYRepository: gominimal/minimal
Length of output: 547
Fix the remote-cache type and the spawn_blocking calls.
Context::remote_cache()returnsRemoteCache<AnyBackend>, butLocalBackend::new_orchestratorrequiresRemoteCache<GcsStorage>. This prevents compilation and rejects HTTPS mirror support. PreserveAnyBackend.- Replace all three
spawn_blocking(async move || { ... })calls. The future runs on the async runtime after the second.await. Move synchronous blocking work intospawn_blocking, or drive the future explicitly withHandle::block_on.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/orchestrator/src/local_backend.rs` at line 186, Update
LocalBackend::new_orchestrator and the remote-cache plumbing to accept and
preserve RemoteCache<AnyBackend> from Context::remote_cache(), including HTTPS
mirror support. Replace all three spawn_blocking(async move || { ... }) usages
with synchronous blocking closures containing only blocking work, or explicitly
drive the async future with the runtime Handle, ensuring no future executes on
the async runtime after the second await.
Source: Learnings
| // DETERMINISTIC assembly order. config.rootfs arrives from a HashSet (op/specs.rs | ||
| // rootfs_mapped), so when two inputs provide the SAME path — e.g. a bootstrap anchor's | ||
| // usr/include/stdio.h vs a build's own libc headers — the winner of the first-writer-wins | ||
| // hardlink below was decided by hash-iteration order: fixed within one sandbox, random | ||
| // across sandboxes. On minimalmertic's stage0 rungs that race masqueraded as a | ||
| // nondeterministic mes-libc codegen SIGSEGV for two months (2026-08-04 core dump: tcc | ||
| // erroring inside glibc's bits/*.h after glibc's stdio.h won the slot, with the error | ||
| // reporter then crashing and eating the diagnostic). Sorting by source path makes every | ||
| // sandbox assemble identically: same inputs, same rootfs, same outcome — and the | ||
| // "Not linking …, already exists" warnings mean the same thing on every run. | ||
| let mut rootfs_inputs: Vec<&config::SandboxMapped> = config.rootfs.iter().collect(); | ||
| rootfs_inputs.sort_by_key(|m| match m { | ||
| config::SandboxMapped::Dir(p) | ||
| | config::SandboxMapped::File(p) | ||
| | config::SandboxMapped::FileCopy(p) => p.clone(), | ||
| config::SandboxMapped::TempDir(td) => td.path().to_path_buf(), | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
The ordered override_deps contract cannot hold while Config::rootfs is unordered and re-sorted by path. The upstream layers now treat list position as precedence, but sandbox2 discards that position twice: Config::rootfs is a HashSet<SandboxMapped> that loses insertion order, and Sandbox::new then sorts inputs by source path. A cache directory path derives from a spec hash, so path order selects an arbitrary provider for a contended path such as /usr/include/stdio.h.
crates/sandbox2/src/lib.rs#L122-L138: changeConfig::rootfstoVec<SandboxMapped>, dedup on insert inwith_rootfsandwith_add_rootfsby keeping the first occurrence, remove thissort_by_key, and update the stale comment that describesrootfs_mappedas returning aHashSet.crates/op/src/specs.rs#L31-L41: after the sandbox layer preserves order, keep this doc comment; until then it describes behaviour the code does not provide.
📍 Affects 2 files
crates/sandbox2/src/lib.rs#L122-L138(this comment)crates/op/src/specs.rs#L31-L41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sandbox2/src/lib.rs` around lines 122 - 138, Preserve ordered rootfs
precedence by changing Config::rootfs and related construction in with_rootfs
and with_add_rootfs to use Vec<SandboxMapped>, deduplicating inserts while
retaining the first occurrence; remove the source-path sort in Sandbox::new and
update the stale rootfs_mapped comment. In crates/sandbox2/src/lib.rs lines
122-138, apply these changes; in crates/op/src/specs.rs lines 31-41, make no
direct change because its documentation becomes correct once sandbox2 preserves
order.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| /// undiagnosed for two months. A `BTreeSet` made it deterministic but sorted by path, | ||
| /// which picks arbitrarily. A Vec preserves the caller's order, so precedence is a | ||
| /// decision the caller makes rather than an accident of hashing or alphabetisation. | ||
| pub override_deps: Option<Vec<PathBuf>>, |
There was a problem hiding this comment.
This should be a BTreeSet, also trim commentary which is now mostly incorrect
| match input { | ||
| BuildDep::Local { full_path, .. } => { | ||
| build_deps.push(SandboxMapped::FileCopy(full_path.to_path_buf())) | ||
| build_deps.push(SandboxMapped::File(full_path.to_path_buf())) |
There was a problem hiding this comment.
This will break builds where cwd is on a different filesystem to the minimal / cache dir
…ministic `SpecBuild.override_deps` was a `HashSet<PathBuf>`. `rootfs_mapped` passes it to the sandbox, which hardlinks the entries first-writer-wins, so when two dependencies install the same path the winner was decided by iteration order. `HashSet` iteration follows a per-process `RandomState` seed, so that choice varied between runs of the same build with the same inputs. `BTreeSet` gives a stable iteration order, so a given set of dependencies now assembles the same rootfs every time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5eda5c6 to
16a846a
Compare
|
Done — rebuilt the branch on current Rebuilding on On the wider point, agreed and taking it downstream. For context on why I'd reached for One related spot I left alone: |
|
Feel free to fix rootfs_mapped and add |
| pub override_deps: Option<HashSet<PathBuf>>, | ||
| /// | ||
| /// Ordered, because `rootfs_mapped` passes these to the sandbox, which hardlinks them | ||
| /// first-writer-wins: with a `HashSet` the winner of a path two deps both install was |
There was a problem hiding this comment.
Now that HashSet is gone theres no need to mention it, that doesnt help a future reader
There was a problem hiding this comment.
Trimmed in the follow-up commit — it now states just the property (ordered because assembly is first-writer-wins), no history.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
twitchyliquid64
left a comment
There was a problem hiding this comment.
Honestly theres a lot of cruft in here, maybe rebase and just
Summary
SpecBuild.override_depswas anOption<HashSet<PathBuf>>. That collection feeds rootfs assembly, which hardlinks entries first-writer-wins, so its iteration order decides which package provides a path that several packages install — andHashSetiteration follows a per-processRandomStateseed, so the winner changed from one run of the binary to the next.Change
Option<BTreeSet<PathBuf>>: iteration is deterministic across processes, first write still wins. Packages installing overlapping paths remains a packaging bug to fix in the packages themselves — this only keeps the symptom deterministic (per review: no caller-chosen precedence).Companion: #1180 makes the downstream
sandbox2::Config::rootfscollection ordered too, which covers every rootfs producer.