fix(toolchain): preserve host rustup context - #1
Conversation
📝 WalkthroughWalkthroughSoldr adds daemon-owned cache maintenance, versioned flush and shutdown reporting, safer cache ownership and cleanup, resolved toolchain-home propagation, scoped PEP 517 cache roots, Darwin artifact handling, and related CLI, test, and documentation updates. ChangesEmbedded daemon and cache lifecycle
Resolved toolchain and platform execution
Validation and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Cargo
participant SoldrCLI
participant SoldrDaemon
participant EmbeddedZccache
participant CacheStore
Cargo->>SoldrCLI: start cache-enabled build
SoldrCLI->>SoldrDaemon: start or connect to daemon
SoldrCLI->>EmbeddedZccache: prepare build session
EmbeddedZccache->>CacheStore: read and write cache state
SoldrCLI->>SoldrDaemon: request flush or shutdown
SoldrDaemon->>EmbeddedZccache: checkpoint and report status
SoldrDaemon-->>SoldrCLI: return generation-aware acknowledgement
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae7d94e64b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/soldr-cli/src/wrapper.rs`:
- Around line 259-262: Resolve compile_args[1] through the existing
resolve_toolchain_binary flow before passing it to
apply_resolved_toolchain_homes in the test-override spawn path. Reuse that
resolved path for home selection so managed rustc binaries are classified
correctly, while preserving the existing direct-execution behavior.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 1c17f924-bc1b-4abd-8239-80f95f8cf049
📒 Files selected for processing (13)
crates/soldr-cli/src/binaries.rscrates/soldr-cli/src/cargo_front_door/mod.rscrates/soldr-cli/src/cargo_front_door/no_cache_detach.rscrates/soldr-cli/src/fetch_overlap.rscrates/soldr-cli/src/rust_plan.rscrates/soldr-cli/src/toolchain.rscrates/soldr-cli/src/toolchain_doctor.rscrates/soldr-cli/src/toolchain_ensure.rscrates/soldr-cli/src/wrapper.rscrates/soldr-cli/tests/cli_cargo_wrappers.rscrates/soldr-cli/tests/cli_toolchain_home_boundary.rscrates/soldr-cli/tests/common/mod.rsdocs/API.md
💤 Files with no reviewable changes (1)
- crates/soldr-cli/tests/cli_cargo_wrappers.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8789a860c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Zach Vorhies <zach.vorhies@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/soldr-cli/src/wrapper.rs`:
- Around line 339-343: Preserve relative compiler paths with path components in
the wrapper’s tool resolution branch: resolve them against the working
directory, and call resolve_toolchain_binary only for bare tool names. In
crates/soldr-cli/src/wrapper.rs lines 339-343, update the path classification
accordingly; in crates/soldr-cli/tests/cli_toolchain_home_boundary.rs lines
189-203, invoke cargo/bin/rustc or ./cargo/bin/rustc and remove the resolver
override to cover the regression.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: b28fab0d-1c48-4d34-822f-98bfa1954458
📒 Files selected for processing (6)
crates/soldr-cli/src/binaries.rscrates/soldr-cli/src/toolchain.rscrates/soldr-cli/src/wrapper.rscrates/soldr-cli/tests/cli_toolchain.rscrates/soldr-cli/tests/cli_toolchain_home_boundary.rscrates/soldr-cli/tests/common/mod.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22278b59dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Closes zackees#1769 Co-authored-by: Zach Vorhies <zach.vorhies@gmail.com>
Co-authored-by: Zach Vorhies <zach.vorhies@gmail.com>
Formatting gate is green.
Validated locally; formatting gate restored in zackees#1777.
Local test and clippy validation passed.
* ci: allow manual full-matrix validation * ci: provision dsymutil for Apple cross-builds * ci: use managed LLVM dsymutil for Apple builds * fix: discover llvm-dsymutil for Darwin builds * ci: locate installed Darwin debug tool dynamically * fix: provision managed llvm-dsymutil for Darwin * fix: update LLVM tools catalogue slug * fix: define dsymutil candidates for rustup lookup * fix: discover native dsymutil on PATH * test: provide hermetic dsymutil fixtures * style: format dsymutil PATH lookup --------- Co-authored-by: Zach Vorhies <zach.vorhies@gmail.com>
* test: add executable-only Darwin symbol probe * test: discover soldr-managed LLVM tools * test: verify Darwin symbols with managed LLVM * test: keep probe symbol addressable --------- Co-authored-by: Zach Vorhies <zach.vorhies@gmail.com>
* fix(cache): add daemon-owned bounded maintenance * test(cache): canonicalize Windows root assertions
5184adc to
30328e3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30328e330f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 788e92aa1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(err) => { | ||
| return Err(SoldrError::Other(format!( | ||
| "embedded zccache checkpoint unavailable: {err:?}" | ||
| ))); |
There was a problem hiding this comment.
When SOLDR_CACHE_LIFECYCLE=command is set for a cargo invocation that never runs rustc (for example soldr cargo metadata or soldr cargo --version), the cache plan still contains a zccache session because caching is enabled, but no wrapper starts soldr-daemon. This new branch turns the resulting ClientError::NotRunning from flush_caches into a post-success error, so an otherwise successful non-compiling cargo command exits nonzero; treat NotRunning as no-op here or only flush after daemon activity was observed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/soldr-cli/src/daemon_entry.rs (1)
14-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the documented
0timeout behavior with execution.Line 14 says
0disables idle shutdown, but Line 25 now selects the daemon default timeout instead. Either preserve the disable behavior or document0as “use the daemon default”; this also affects the omitted-flag default on Line 15.🤖 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/soldr-cli/src/daemon_entry.rs` around lines 14 - 25, Align the idle timeout configuration between the `Cli` field `idle_timeout_secs` documentation/default and the `ServerOptions` construction in `run()`. Ensure an omitted or explicitly zero timeout consistently implements the documented behavior: either pass a disabled timeout through, or update the documentation to state that zero uses the daemon default.
🤖 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 @.github/workflows/ci.yml:
- Line 32: Add a top-level concurrency configuration to the workflow containing
a group derived from github.workflow and github.ref, with cancel-in-progress
enabled, so overlapping manual, push, or pull-request runs are cancelled.
In `@crates/soldr-cli/src/binaries.rs`:
- Around line 227-265: Update apply_resolved_toolchain_homes to pass its
existing paths value into the managed-home application branch instead of calling
apply_managed_toolchain_homes_if_available, and adjust that helper to accept
&SoldrPaths and reuse it when deriving managed homes. Remove the redundant
SoldrPaths::new() construction while preserving the existing matching and
home-application behavior.
- Around line 420-454: Remove the std::env::set_var mutation from
ensure_daemon_executable_handoff and return the resolved daemon path directly.
Update the downstream daemon-spawning handoff to consume this returned path
explicitly, preserving the existing configured-path, sibling, and fallback
resolution behavior without mutating SOLDR_INTERNAL_DAEMON_EXE.
In `@crates/soldr-cli/src/blessed_build.rs`:
- Around line 227-239: Update the Darwin preparation flow around
ensure_dsymutil_on_path to avoid returning an error when the active Cargo
profile uses split-debuginfo "off" or "unpacked", since those modes do not
invoke dsymutil. Probe the effective debuginfo setting for the target/profile,
or follow the SDK path’s warning-and-fall-through behavior, while still
requiring or provisioning dsymutil for packed debuginfo builds.
- Around line 419-458: Update ensure_dsymutil_on_path to derive the rustup
--toolchain value from the same resolved toolchain identity used by
find_dsymutil_in_rustup, honoring RUSTUP_TOOLCHAIN when present and otherwise
using the manifest channel. Ensure provisioning and the subsequent dsymutil
lookup target the same toolchain, while preserving the existing behavior when no
explicit toolchain can be resolved.
In `@crates/soldr-cli/src/cache/session.rs`:
- Around line 723-745: The Err arm handling flush_caches in the cache flush flow
should treat ClientError::NotRunning as a successful no-op, without adding a
failure note or returning an error. Preserve the existing failure-message
behavior for all other errors, and keep successful or incomplete report handling
unchanged.
In `@crates/soldr-cli/src/cargo_front_door/darwin_embed.rs`:
- Around line 198-208: The test finds_both_dsym_naming_conventions currently
uses an extensionless artifact, causing both candidate paths to be identical.
Add a separate extensioned-artifact case such as libfoo.dylib with its
libfoo.dSYM bundle, and assert find_dsym_bundle resolves it, while preserving
the existing extensionless case.
- Around line 72-165: Update embed_one so its temporary staging directory is
created within the artifact’s parent directory rather than the system temp
directory, ensuring the later rename from staged to artifact remains on the same
filesystem and avoids CrossesDevices failures. Preserve the existing section
extraction, backup rename, promotion rollback, and cleanup behavior.
In `@crates/soldr-cli/src/pyo3_detect.rs`:
- Around line 822-854: Update EnvVarGuard::set and its Drop implementation to
handle Rust 2024’s unsafe std::env::set_var and remove_var calls by wrapping
each mutation in an explicit unsafe block, preserving the existing ENV_LOCK and
restoration behavior.
In `@crates/soldr-cli/tests/cli_cache.rs`:
- Around line 616-617: Replace the bare test declaration for
cache_flush_fails_when_embedded_daemon_is_unavailable with the re-exported
timed_test! macro, using the default duration unless needed otherwise. Apply the
same declaration change to cache_shutdown_reports_already_absent_truthfully at
crates/soldr-cli/tests/cli_cache.rs lines 651-652.
In `@crates/soldr-cli/tests/cli_toolchain.rs`:
- Around line 304-305: Update the test declaration for
toolchain_prepare_host_cargo_keeps_managed_install_home_without_managed_rustup_home
to use the imported timed_test! macro instead of bare #[test], matching the
file’s required timed-test convention.
In `@crates/soldr-daemon/src/daemon/maintenance.rs`:
- Around line 40-55: Update ShutdownSignal::wait to create and register the
Notify::notified future before checking requested, using Notified::enable or the
equivalent registration API. Recheck is_requested after registration and only
await the notification when shutdown has not already been requested, preventing
missed notify_waiters signals while preserving the existing shutdown behavior.
In `@docs/DEBUG_SIDECARS.md`:
- Around line 62-66: Update the documentation text around the embedded zccache
architecture to use lowercase “soldr” consistently for the binary/tool,
replacing the newly introduced “Soldr” references while preserving the existing
technical guidance.
In `@src/soldr/__init__.py`:
- Around line 238-251: Update _hold_build_lease() around the helper startup
readline to use a bounded wait so lock contention cannot hang delegated PEP 517
invocations indefinitely. On startup failure, replace the unbounded
process.wait() with the existing timeout/kill/wait teardown pattern, preserving
the RuntimeError and captured stderr details.
---
Outside diff comments:
In `@crates/soldr-cli/src/daemon_entry.rs`:
- Around line 14-25: Align the idle timeout configuration between the `Cli`
field `idle_timeout_secs` documentation/default and the `ServerOptions`
construction in `run()`. Ensure an omitted or explicitly zero timeout
consistently implements the documented behavior: either pass a disabled timeout
through, or update the documentation to state that zero uses the daemon default.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 12fcbbd2-2bcd-4786-aa0d-0243ee5b0fdb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (94)
.github/workflows/ci.ymlCLAUDE.mdDESIGN.mdREADME.md_vender/zccachebench/test_darwin_symbols.pycrates/soldr-cache/Cargo.tomlcrates/soldr-cache/src/cache_lib/build_active.rscrates/soldr-cache/src/cache_lib/cargo_lock.rscrates/soldr-cache/src/cache_lib/cook_gc.rscrates/soldr-cache/src/cache_lib/gc.rscrates/soldr-cache/src/cache_lib/mod.rscrates/soldr-cache/src/cache_lib/path_safety.rscrates/soldr-cache/src/cache_lib/pep517_gc.rscrates/soldr-cache/src/cache_lib/target_registry.rscrates/soldr-cache/src/cache_lib/trash_gc.rscrates/soldr-cli/Cargo.tomlcrates/soldr-cli/src/binaries.rscrates/soldr-cli/src/blessed_build.rscrates/soldr-cli/src/build_from_source_cmd.rscrates/soldr-cli/src/cache/mod.rscrates/soldr-cli/src/cache/release_worktree.rscrates/soldr-cli/src/cache/report.rscrates/soldr-cli/src/cache/session.rscrates/soldr-cli/src/cargo_front_door/cache_plan.rscrates/soldr-cli/src/cargo_front_door/darwin_embed.rscrates/soldr-cli/src/cargo_front_door/mod.rscrates/soldr-cli/src/cargo_front_door/no_cache_detach.rscrates/soldr-cli/src/cargo_front_door/tests.rscrates/soldr-cli/src/cargo_front_door/zthreads_fallback.rscrates/soldr-cli/src/cli_args.rscrates/soldr-cli/src/compile_dispatch.rscrates/soldr-cli/src/cook.rscrates/soldr-cli/src/daemon_entry.rscrates/soldr-cli/src/doctor.rscrates/soldr-cli/src/fetch_overlap.rscrates/soldr-cli/src/gc/auto.rscrates/soldr-cli/src/lib.rscrates/soldr-cli/src/lint_cmd.rscrates/soldr-cli/src/logs_cmd.rscrates/soldr-cli/src/main_tests.rscrates/soldr-cli/src/prepare_cmd.rscrates/soldr-cli/src/pyo3_detect.rscrates/soldr-cli/src/rust_plan.rscrates/soldr-cli/src/save_load.rscrates/soldr-cli/src/soldr_main.rscrates/soldr-cli/src/toolchain.rscrates/soldr-cli/src/toolchain_doctor.rscrates/soldr-cli/src/toolchain_ensure.rscrates/soldr-cli/src/wrapper.rscrates/soldr-cli/src/zccache.rscrates/soldr-cli/tests/README.mdcrates/soldr-cli/tests/cli_cache.rscrates/soldr-cli/tests/cli_cargo_basic.rscrates/soldr-cli/tests/cli_cargo_native_cc.rscrates/soldr-cli/tests/cli_cargo_wrappers.rscrates/soldr-cli/tests/cli_daemon_displacement.rscrates/soldr-cli/tests/cli_daemon_flush_caches.rscrates/soldr-cli/tests/cli_daemon_lifecycle.rscrates/soldr-cli/tests/cli_dispatch.rscrates/soldr-cli/tests/cli_toolchain.rscrates/soldr-cli/tests/cli_toolchain_home_boundary.rscrates/soldr-cli/tests/common/mod.rscrates/soldr-cli/tests/cook_auto_gc.rscrates/soldr-cli/tests/daemon_cache_maintenance.rscrates/soldr-core/src/core/mod.rscrates/soldr-core/src/core/paths.rscrates/soldr-core/src/core/wire.protocrates/soldr-core/src/core/wire_proto.rscrates/soldr-core/src/fuzzy_match.rscrates/soldr-daemon/Cargo.tomlcrates/soldr-daemon/src/daemon/client.rscrates/soldr-daemon/src/daemon/db.rscrates/soldr-daemon/src/daemon/event_batcher.rscrates/soldr-daemon/src/daemon/history_gc.rscrates/soldr-daemon/src/daemon/ipc.rscrates/soldr-daemon/src/daemon/lifecycle.rscrates/soldr-daemon/src/daemon/maintenance.rscrates/soldr-daemon/src/daemon/mod.rscrates/soldr-daemon/src/daemon/protocol.rscrates/soldr-daemon/src/daemon/server.rscrates/soldr-daemon/src/daemon/wire.rscrates/soldr-daemon/src/daemon/wire_tests.rscrates/soldr-daemon/src/zccache_embedded.rscrates/soldr-fetch/src/fetch/apple_sdk.rscrates/soldr-fetch/src/fetch/llvm_tools_bundle.rsdocs/API.mddocs/CI_CACHE.mddocs/CROSS_COMPILE.mddocs/DEBUG_SIDECARS.mddocs/WINDOWS_PREREQS.mddocs/ZCCACHE_INTEGRATION_GUARDRAILS.mdsrc/soldr/__init__.pytests/test_pep517_pyo3_policy.py
| paths-ignore: | ||
| - '*.md' | ||
| - '**/*.md' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a concurrency group now that manual dispatch is enabled.
Adding workflow_dispatch makes overlapping runs (manual + push/PR) more likely. Without a concurrency block, redundant in-flight runs won't be cancelled, wasting CI minutes. zizmor also flags the missing concurrency setting.
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true🧰 Tools
🪛 zizmor (1.26.1)
[warning] 3-32: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 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 @.github/workflows/ci.yml at line 32, Add a top-level concurrency
configuration to the workflow containing a group derived from github.workflow
and github.ref, with cancel-in-progress enabled, so overlapping manual, push, or
pull-request runs are cancelled.
Source: Linters/SAST tools
| /// Apply homes that match a toolchain binary Soldr already resolved. | ||
| /// | ||
| /// Rustup discovery and bootstrap intentionally use Soldr's managed homes, | ||
| /// but a concrete host binary must keep the caller's host Rustup context. | ||
| /// Mixing a host Cargo/rustfmt proxy with Soldr's default-less managed | ||
| /// `RUSTUP_HOME` makes Rustup report that no default toolchain is configured. | ||
| pub(crate) fn apply_resolved_toolchain_homes( | ||
| command: &mut std::process::Command, | ||
| binary: &std::path::Path, | ||
| ) { | ||
| let start_dir = std::env::current_dir().ok(); | ||
| crate::core::apply_implicit_toolchain_homes(command, start_dir.as_deref()); | ||
|
|
||
| let Ok(paths) = SoldrPaths::new() else { | ||
| return; | ||
| }; | ||
| let managed_cargo_home = crate::fetch::managed_cargo_home(&paths); | ||
| let managed_rustup_home = crate::fetch::managed_rustup_home(&paths); | ||
| if path_is_within(binary, &managed_cargo_home) || path_is_within(binary, &managed_rustup_home) { | ||
| apply_managed_toolchain_homes_if_available(command, start_dir.as_deref()); | ||
| } | ||
| } | ||
|
|
||
| fn path_is_within(path: &std::path::Path, root: &std::path::Path) -> bool { | ||
| let path = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); | ||
| let root = std::fs::canonicalize(root).unwrap_or_else(|_| root.to_path_buf()); | ||
| path.starts_with(root) | ||
| } | ||
|
|
||
| fn apply_managed_toolchain_homes_if_available( | ||
| command: &mut std::process::Command, | ||
| start_dir: Option<&std::path::Path>, | ||
| ) { | ||
| let Ok(paths) = SoldrPaths::new() else { | ||
| return; | ||
| }; | ||
| apply_managed_cargo_home_if_available_for_paths(command, start_dir, &paths); | ||
| apply_managed_rustup_home_if_available_for_paths(command, start_dir, &paths); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Redundant SoldrPaths::new() construction on every resolved-homes call.
apply_resolved_toolchain_homes builds paths and already derives managed_cargo_home/managed_rustup_home, but on the match branch it calls apply_managed_toolchain_homes_if_available, which constructs SoldrPaths::new() again and re-derives both managed homes from scratch. This function runs on every resolved-toolchain subprocess spawn (a hot path for compiler/cargo invocations), so the duplicate work is avoidable.
♻️ Proposed fix: reuse the already-constructed `paths`
let managed_cargo_home = crate::fetch::managed_cargo_home(&paths);
let managed_rustup_home = crate::fetch::managed_rustup_home(&paths);
if path_is_within(binary, &managed_cargo_home) || path_is_within(binary, &managed_rustup_home) {
- apply_managed_toolchain_homes_if_available(command, start_dir.as_deref());
+ apply_managed_cargo_home_if_available_for_paths(command, start_dir.as_deref(), &paths);
+ apply_managed_rustup_home_if_available_for_paths(command, start_dir.as_deref(), &paths);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Apply homes that match a toolchain binary Soldr already resolved. | |
| /// | |
| /// Rustup discovery and bootstrap intentionally use Soldr's managed homes, | |
| /// but a concrete host binary must keep the caller's host Rustup context. | |
| /// Mixing a host Cargo/rustfmt proxy with Soldr's default-less managed | |
| /// `RUSTUP_HOME` makes Rustup report that no default toolchain is configured. | |
| pub(crate) fn apply_resolved_toolchain_homes( | |
| command: &mut std::process::Command, | |
| binary: &std::path::Path, | |
| ) { | |
| let start_dir = std::env::current_dir().ok(); | |
| crate::core::apply_implicit_toolchain_homes(command, start_dir.as_deref()); | |
| let Ok(paths) = SoldrPaths::new() else { | |
| return; | |
| }; | |
| let managed_cargo_home = crate::fetch::managed_cargo_home(&paths); | |
| let managed_rustup_home = crate::fetch::managed_rustup_home(&paths); | |
| if path_is_within(binary, &managed_cargo_home) || path_is_within(binary, &managed_rustup_home) { | |
| apply_managed_toolchain_homes_if_available(command, start_dir.as_deref()); | |
| } | |
| } | |
| fn path_is_within(path: &std::path::Path, root: &std::path::Path) -> bool { | |
| let path = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); | |
| let root = std::fs::canonicalize(root).unwrap_or_else(|_| root.to_path_buf()); | |
| path.starts_with(root) | |
| } | |
| fn apply_managed_toolchain_homes_if_available( | |
| command: &mut std::process::Command, | |
| start_dir: Option<&std::path::Path>, | |
| ) { | |
| let Ok(paths) = SoldrPaths::new() else { | |
| return; | |
| }; | |
| apply_managed_cargo_home_if_available_for_paths(command, start_dir, &paths); | |
| apply_managed_rustup_home_if_available_for_paths(command, start_dir, &paths); | |
| } | |
| /// Apply homes that match a toolchain binary Soldr already resolved. | |
| /// | |
| /// Rustup discovery and bootstrap intentionally use Soldr's managed homes, | |
| /// but a concrete host binary must keep the caller's host Rustup context. | |
| /// Mixing a host Cargo/rustfmt proxy with Soldr's default-less managed | |
| /// `RUSTUP_HOME` makes Rustup report that no default toolchain is configured. | |
| pub(crate) fn apply_resolved_toolchain_homes( | |
| command: &mut std::process::Command, | |
| binary: &std::path::Path, | |
| ) { | |
| let start_dir = std::env::current_dir().ok(); | |
| crate::core::apply_implicit_toolchain_homes(command, start_dir.as_deref()); | |
| let Ok(paths) = SoldrPaths::new() else { | |
| return; | |
| }; | |
| let managed_cargo_home = crate::fetch::managed_cargo_home(&paths); | |
| let managed_rustup_home = crate::fetch::managed_rustup_home(&paths); | |
| if path_is_within(binary, &managed_cargo_home) || path_is_within(binary, &managed_rustup_home) { | |
| apply_managed_cargo_home_if_available_for_paths(command, start_dir.as_deref(), &paths); | |
| apply_managed_rustup_home_if_available_for_paths(command, start_dir.as_deref(), &paths); | |
| } | |
| } | |
| fn path_is_within(path: &std::path::Path, root: &std::path::Path) -> bool { | |
| let path = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); | |
| let root = std::fs::canonicalize(root).unwrap_or_else(|_| root.to_path_buf()); | |
| path.starts_with(root) | |
| } | |
| fn apply_managed_toolchain_homes_if_available( | |
| command: &mut std::process::Command, | |
| start_dir: Option<&std::path::Path>, | |
| ) { | |
| let Ok(paths) = SoldrPaths::new() else { | |
| return; | |
| }; | |
| apply_managed_cargo_home_if_available_for_paths(command, start_dir, &paths); | |
| apply_managed_rustup_home_if_available_for_paths(command, start_dir, &paths); | |
| } |
🤖 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/soldr-cli/src/binaries.rs` around lines 227 - 265, Update
apply_resolved_toolchain_homes to pass its existing paths value into the
managed-home application branch instead of calling
apply_managed_toolchain_homes_if_available, and adjust that helper to accept
&SoldrPaths and reuse it when deriving managed homes. Remove the redundant
SoldrPaths::new() construction while preserving the existing matching and
home-application behavior.
| /// Ensure compiler-side daemon recovery has a canonically named executable. | ||
| /// | ||
| /// The managed Cargo front door normally injects this handoff once for every | ||
| /// compiler child. Direct `RUSTC_WRAPPER` / `zccache-soldr` invocations do not | ||
| /// have that parent, so recover it lazily after the first failed daemon probe. | ||
| /// Reuse an existing sibling without hashing; only first use materializes the | ||
| /// multicall alias. | ||
| pub(crate) fn ensure_daemon_executable_handoff() -> Result<std::path::PathBuf, SoldrError> { | ||
| let env_var = crate::daemon::lifecycle::SOLDR_DAEMON_EXE_ENV_VAR; | ||
| if let Some(configured) = non_empty_env_path(env_var).filter(|path| { | ||
| path.is_file() | ||
| && path | ||
| .file_stem() | ||
| .and_then(std::ffi::OsStr::to_str) | ||
| .is_some_and(|stem| stem.eq_ignore_ascii_case("soldr-daemon")) | ||
| }) { | ||
| return Ok(configured); | ||
| } | ||
|
|
||
| let current = std::env::current_exe().map_err(SoldrError::from)?; | ||
| let sibling = current.parent().map(|parent| { | ||
| parent.join(if cfg!(windows) { | ||
| "soldr-daemon.exe" | ||
| } else { | ||
| "soldr-daemon" | ||
| }) | ||
| }); | ||
| let daemon = sibling | ||
| .filter(|path| path.is_file()) | ||
| .map(Ok) | ||
| .unwrap_or_else(soldr_daemon_binary)?; | ||
| std::env::set_var(env_var, &daemon); | ||
| Ok(daemon) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== file location and relevant symbols =="
fd -a '^\binaries\.rs$|pyo3_detect\.rs$' .
echo
echo "== binaries.rs relevant region =="
if [ -f crates/soldr-cli/src/binaries.rs ]; then
nl -ba crates/soldr-cli/src/binaries.rs | sed -n '380,475p'
fi
echo
echo "== pyo3_detect.rs environment mutation references =="
if [ -f crates/soldr-cli/src/pyo3_detect.rs ]; then
nl -ba crates/soldr-cli/src/pyo3_detect.rs | sed -n '1,260p'
fi
echo
echo "== all std::env::set_var occurrences =="
rg -n "std::env::set_var|set_var\\(" crates/soldr-cli/src crates/soldr-core/src crates/soldr-cache/src crates/soldr-fetch/src crates/soldr-daemon/src 2>/dev/null || true
echo
echo "== references to ensure_daemon_executable_handoff =="
rg -n "ensure_daemon_executable_handoff|SOLDR_DAEMON_EXE_ENV_VAR|daemon_executable_handoff" crates/soldr-cli/src crates/soldr-core/src crates/soldr-cache/src crates/soldr-fetch/src crates/soldr-daemon/src 2>/dev/null || true
echo
echo "== multi-threading constructs in cli relevant files =="
rg -n "tokio::spawn|std::thread::spawn|spawn_blocking|join_handle|Rayon|par_iter|AsyncRc|Arc<Mutex|MutexGuard|OnceLock|Once|atomic" crates/soldr-cli/src crates/soldr-core/src crates/soldr-cache/src crates/soldr-fetch/src crates/soldr-daemon/src 2>/dev/null | head -200 || trueRepository: jmagar/soldr
Length of output: 322
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== candidate files =="
find . -path './crates/soldr-cli/src/binaries.rs' -o -path './crates/soldr-cli/src/pyo3_detect.rs' -o -path '*/src/binaries.rs' -o -path '*/pyo3_detect.rs' | sort
echo
echo "== binaries.rs relevant region =="
python3 - <<'PY'
from pathlib import Path
p=Path('crates/soldr-cli/src/binaries.rs')
if p.exists():
for i,line in enumerate(p.read_text().splitlines(),1):
if 380 <= i <= 475:
print(f"{i:5}: {line}")
else:
print("missing crates/soldr-cli/src/binaries.rs")
PY
echo
echo "== pyo3_detect.rs relevant region =="
python3 - <<'PY'
from pathlib import Path
p=Path('crates/soldr-cli/src/pyo3_detect.rs')
if p.exists():
txt=p.read_text().splitlines()
for i,line in enumerate(txt,1):
if i <= 260:
print(f"{i:5}: {line}")
else:
print("missing crates/soldr-cli/src/pyo3_detect.rs")
PY
echo
echo "== all set_var occurrences in crates =="
python3 - <<'PY'
import subprocess, pathlib, re, sys
repos=set()
p = subprocess.run(['git','ls-files'], stdout=subprocess.PIPE, text=True, check=True)
for line in p.stdout.splitlines():
if line.startswith(b'crates/'.decode()) or line.startswith('./crates/'.decode()):
if line.endswith(('.rs','.toml','.json','')):
try:
text=pathlib.Path(line).read_text(errors='ignore')
except Exception:
continue
for m in re.finditer(r'std::?env::?::set_var\s*\(|set_var\s*\(', text):
start=m.start()+1
line_no=text.count('\n',0,start)+1
print(f"{line}:{line_no}:{m.group()[0]}")
PY
echo
echo "== references to ensure_daemon_executable_handoff / daemon env =="
python3 - <<'PY'
import subprocess, pathlib, re
p = subprocess.run(['git','ls-files'], stdout=subprocess.PIPE, text=True, check=True)
for line in p.stdout.splitlines():
if line.endswith('.rs'):
text=pathlib.Path(line).read_text(errors='ignore')
hits=[]
for name in [
'ensure_daemon_executable_handoff',
'SOLDR_DAEMON_EXE_ENV_VAR',
'daemon_executable_handoff'
]:
if name in text:
hits.append(name)
if hits:
print(f"{line}: {hits}")
PY
echo
echo "== thread spawn / synchronization constructs in crates =="
python3 - <<'PY'
import subprocess, pathlib, re
tokens = [
'tokio::spawn','std::thread::spawn','spawn_blocking','join_handle','Rayon','par_iter',
'AsyncRc','Arc<Mutex','MutexGuard','OnceLock','Once','atomic',
]
p = subprocess.run(['git','ls-files'], stdout=subprocess.PIPE, text=True, check=True)
for line in p.stdout.splitlines():
if line.endswith('.rs'):
text=pathlib.Path(line).read_text(errors('ignore'))
for tok in tokens:
if tok in text:
print(f"{line}:{tok}")
break
PYRepository: jmagar/soldr
Length of output: 14947
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== all set_var occurrences in repo =="
rg -n "std::env::set_var|env::set_var|set_var\s*\(" --glob '*.rs' --glob '*.toml' . || true
echo
echo "== ensure_daemon_executable_handoff references =="
rg -n "ensure_daemon_executable_handoff|SOLDR_DAEMON_EXE_ENV_VAR|daemon_executable_handoff" --glob '*.rs' . || true
echo
echo "== lifecycle env var definition =="
python3 - <<'PY'
from pathlib import Path
import re
matches=[]
for p in Path('.').rglob('*.rs'):
text=p.read_text(errors='ignore')
if 'SOLDR_DAEMON_EXE_ENV_VAR' in text:
lines=text.splitlines()
for i,line in enumerate(lines,1):
if 'SOLDR_DAEMON_EXE_ENV_VAR' in line:
start=max(1,i-8); end=min(len(lines),i+8)
print(f"--- {p}:{i} ---")
for n in range(start,end+1):
print(f"{n}: {lines[n-1]}")
matches.append(p)
break
if not matches:
print("no SolDR_DAEMON_EXE_ENV_VAR definition found")
PY
echo
echo "== function outline for daemon lifecycle and binaries =="
ast-grep outline crates/soldr-cli/src/binaries.rs --match ensure_daemon_executable_handoff --view expanded || true
find . -path '*/daemon/lifecycle.rs' -o -path '*/daemon/*' | sort | head -80
echo
echo "== concurrent constructs in Rust files (concise) =="
python3 - <<'PY'
import pathlib, re
terms = [
'tokio::spawn', 'std::thread::spawn', 'spawn_blocking', 'join_handle',
'Rayon', 'par_iter', 'AsyncRc', 'Arc<Mutex', 'MutexGuard', 'OnceLock',
'Once::new', 'atomic', 'synchronize', 'frozen', 'FROZEN',
]
for p in pathlib.Path('.').rglob('*.rs'):
text = p.read_text(errors='ignore')
if any(t in text for t in terms):
hits=[]
for t in terms:
if t in text:
hits.append(t)
print(f"{p}: {hits}")
PYRepository: jmagar/soldr
Length of output: 29485
Avoid mutating SOLDR_INTERNAL_DAEMON_EXE with std::env::set_var.
ensure_daemon_executable_handoff is entered while wrapping compiler invocations and can race with the managed cargo command that later spawns the daemon after this handoff is set. Since environment mutation with std::env::set_var is only safe under exclusive single-threaded access, return the resolved daemon path without changing the process environment, or pass it explicitly to the downstream daemon-spawning path.
🤖 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/soldr-cli/src/binaries.rs` around lines 420 - 454, Remove the
std::env::set_var mutation from ensure_daemon_executable_handoff and return the
resolved daemon path directly. Update the downstream daemon-spawning handoff to
consume this returned path explicitly, preserving the existing configured-path,
sibling, and fallback resolution behavior without mutating
SOLDR_INTERNAL_DAEMON_EXE.
| if let Some(path) = find_dsymutil_in_rustup() { | ||
| if let Some(parent) = path.parent() { | ||
| prep.path_dirs.push(parent.to_path_buf()); | ||
| return Ok(()); | ||
| } | ||
| } | ||
|
|
||
| let channel = crate::core::read_rust_toolchain_manifest( | ||
| &std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), | ||
| ) | ||
| .ok() | ||
| .and_then(|manifest| manifest.channel); | ||
| let mut command = Command::new(crate::binaries::rustup_binary()); | ||
| command.args(["component", "add", "llvm-tools-preview"]); | ||
| if let Some(channel) = channel.as_deref() { | ||
| command.args(["--toolchain", channel]); | ||
| } | ||
| crate::core::apply_implicit_toolchain_homes(&mut command, None); | ||
| command.stdout(Stdio::null()); | ||
| let status = command.status().map_err(|error| { | ||
| SoldrError::Other(format!( | ||
| "unable to provision dsymutil: failed to invoke rustup: {error}" | ||
| )) | ||
| })?; | ||
| if status.success() { | ||
| if let Some(path) = find_dsymutil_in_rustup() { | ||
| if let Some(parent) = path.parent() { | ||
| prep.path_dirs.push(parent.to_path_buf()); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Err(SoldrError::Other( | ||
| "Darwin packed debuginfo requires dsymutil, but it is unavailable. \ | ||
| Install the rustup llvm-tools-preview component or set SOLDR_DSYMUTIL \ | ||
| to a compatible dsymutil executable." | ||
| .into(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)blessed_build\.rs$|(^|/)API\.md$|binaries|core\.rs|toolchain|rust-toolchain' | head -200
echo
echo "== blessed_build outline =="
ast-grep outline crates/soldr-cli/src/blessed_build.rs --view compact || true
echo
echo "== relevant blessed_build lines =="
sed -n '380,490p' crates/soldr-cli/src/blessed_build.rs
echo
echo "== search resolve_toolchain_binary and find_dsymutil_in_rustup =="
rg -n "resolve_toolchain_binary|find_dsymutil_in_rustup|ensure_dsymutil_on_path|RUSTUP_TOOLCHAIN|read_rust_toolchain_manifest|apply_implicit_toolchain_homes" crates docs -SRepository: jmagar/soldr
Length of output: 23527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search resolve_toolchain_binary and find_dsymutil_in_rustup in all rust files =="
rg -n "resolve_toolchain_binary|find_dsymutil_in_rustup|ensure_dsymutil_on_path|RUSTUP_TOOLCHAIN|read_rust_toolchain_manifest|apply_implicit_toolchain_homes" crates -S || trueRepository: jmagar/soldr
Length of output: 17609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== binaries.rs resolution implementation =="
sed -n '1,130p' crates/soldr-cli/src/binaries.rs
echo
echo "== cargo front door rustup toolchain resolution =="
sed -n '2140,2185p' crates/soldr-cli/src/cargo_front_door/mod.rs
echo
echo "== core apply_implicit_toolchain_homes implementation =="
sed -n '230,285p' crates/soldr-core/src/core/toolchain_resolve.rs
echo
echo "== component install channel/source implementation =="
sed -n '180,230p' crates/soldr-cli/src/cargo_front_door/component_install.rs
echo
echo "== blessed_build invocation point =="
sed -n '228,248p' crates/soldr-cli/src/blessed_build.rsRepository: jmagar/soldr
Length of output: 142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== binaries.rs resolution implementation =="
sed -n '1,130p' crates/soldr-cli/src/binaries.rs || true
echo
echo "== cargo front door rustup toolchain resolution =="
sed -n '2140,2185p' crates/soldr-cli/src/cargo_front_door/mod.rs || true
echo
echo "== core apply_implicit_toolchain_homes implementation =="
sed -n '230,285p' crates/soldr-core/src/core/toolchain_resolve.rs || true
echo
echo "== component install channel/source implementation =="
sed -n '180,230p' crates/soldr-cli/src/cargo_front_door/component_install.rs || true
echo
echo "== blessed_build invocation point =="
sed -n '228,248p' crates/soldr-cli/src/blessed_build.rs || trueRepository: jmagar/soldr
Length of output: 142
🏁 Script executed:
#!/bin/bash
set -u
echo "disk/filesystem check"
df -h . 2>/dev/null || true
echo "== binaries.rs resolution implementation =="
sed -n '1,140p' crates/soldr-cli/src/binaries.rs || true
echo
echo "== cargo front door rustup toolchain resolution =="
sed -n '2130,2190p' crates/soldr-cli/src/cargo_front_door/mod.rs || true
echo
echo "== core apply_implicit_toolchain_homes implementation =="
sed -n '225,290p' crates/soldr-core/src/core/toolchain_resolve.rs || true
echo
echo "== component install channel/source implementation =="
sed -n '175,235p' crates/soldr-cli/src/cargo_front_door/component_install.rs || true
echo
echo "== blessed_build invocation point =="
sed -n '225,250p' crates/soldr-cli/src/blessed_build.rs || true
echo
echo "== behavioral model: toolchain argument ordering with env-only explicit toolchain =="
cat >/tmp/soldr_dsym_model.py <<'PY'
import os
def read_manifest_channel(cwd):
# current blessed_build branch: only read manifest at cwd
manifest = {
os.getcwd(): {"manifest_channel": "stable", "installed": ["stable", "nightly"]},
"/test/root": {"manifest_channel": "stable", "installed": ["stable", "nightly"]},
}
m = manifest.get(cwd)
if not m or os.environ.get("SOLDR_OVERRIDE_PREINSTALLED_DSYMUTIL"):
return None
return m.get("manifest_channel")
def resolve_toolchain_binary(env_rustup):
# current resolve_toolchain_binary("rustc") API.md / source-derived model:
# when RUSTUP_TOOLCHAIN is explicitly non-empty, consult rustup for that toolchain,
# otherwise fall back from PATH/RUSTUP_HOME/CARGO_HOME to default/manifest-resolved selection.
if env_rustup is not None and env_rustup != "":
return f"rustup-which rustc for {env_rustup}"
return "default/preferred rustc"
root = resolved = None
for env_rustup in [None, "stable", "nightly"]:
os.environ.pop("RUSTUP_TOOLCHAIN", None)
if env_rustup is not None:
os.environ["RUSTUP_TOOLCHAIN"] = env_rustup
manifest = read_manifest_channel(os.getcwd())
component_args = ["component", "add", "llvm-tools-preview"]
if manifest:
component_args += ["--toolchain", manifest]
resolved = resolve_toolchain_binary(env_rustup)
paths = {
"manifest-channel": manifest,
"env-rustup": env_rustup,
"component-target": component_args,
"find-dsymutil-target": resolved,
}
if "-target" in component_args[-1] and "-target" in resolved:
status = "same" if component_args[-1] == resolved[-1] else "diverges"
elif is_default := ("--toolchain" not in component_args) and resolved.startswith("default/default"):
status = "rustup default only"
else:
status = "different paths"
print(status, ": ", paths)
PY
python3 /tmp/soldr_dsym_model.pyRepository: jmagar/soldr
Length of output: 13548
Derive dsymutil provisioning from the same toolchain source used for rustc resolution.
ensure_dsymutil_on_path only reads rust-toolchain.toml to build rustup component add --toolchain, while find_dsymutil_in_rustup() resolves rustc through the normal binary resolver and skips the manifest when RUSTUP_TOOLCHAIN is set. If RUSTUP_TOOLCHAIN=n is different from the manifest channel, the component is installed into the mapped toolchain but find_dsymutil_in_rustup() then searches the toolchain selected by the env override, so provisioning can succeed and the next lookup fail with a misleading “install llvm-tools-preview” error. Use the resolved toolchain identity for --toolchain here, or let the environment toolchain override be the source of truth when it is present.
🤖 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/soldr-cli/src/blessed_build.rs` around lines 419 - 458, Update
ensure_dsymutil_on_path to derive the rustup --toolchain value from the same
resolved toolchain identity used by find_dsymutil_in_rustup, honoring
RUSTUP_TOOLCHAIN when present and otherwise using the manifest channel. Ensure
provisioning and the subsequent dsymutil lookup target the same toolchain, while
preserving the existing behavior when no explicit toolchain can be resolved.
| #[test] | ||
| fn cache_flush_reports_embedded_unavailable_without_daemon() { | ||
| fn cache_flush_fails_when_embedded_daemon_is_unavailable() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Declare these new tests with timed_test! instead of bare #[test]. Both tests are newly changed declarations and neither is #[ignore] nor carries an allow-bare-test annotation, so they must use the re-exported timed_test! macro (with a duration override only if they need more than the two-minute default).
crates/soldr-cli/tests/cli_cache.rs#L616-L617: wrapcache_flush_fails_when_embedded_daemon_is_unavailablewithsoldr_cli::timed_test!(or the crate-local re-export used elsewhere in this file).crates/soldr-cli/tests/cli_cache.rs#L651-L652: wrapcache_shutdown_reports_already_absent_truthfullythe same way.
As per coding guidelines: "Declare tests with the re-exported timed_test! macro rather than bare #[test] ... Ignored tests or documented allow-bare-test annotations are permitted exceptions."
📍 Affects 1 file
crates/soldr-cli/tests/cli_cache.rs#L616-L617(this comment)crates/soldr-cli/tests/cli_cache.rs#L651-L652
🤖 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/soldr-cli/tests/cli_cache.rs` around lines 616 - 617, Replace the bare
test declaration for cache_flush_fails_when_embedded_daemon_is_unavailable with
the re-exported timed_test! macro, using the default duration unless needed
otherwise. Apply the same declaration change to
cache_shutdown_reports_already_absent_truthfully at
crates/soldr-cli/tests/cli_cache.rs lines 651-652.
Source: Coding guidelines
| #[test] | ||
| fn toolchain_prepare_host_cargo_keeps_managed_install_home_without_managed_rustup_home() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether cli_toolchain.rs carries a documented allow-bare-test exception
# and how sibling tests in the file are declared.
fd -t f 'cli_toolchain.rs' crates/soldr-cli/tests --exec sh -c '
echo "== head of {} =="; sed -n "1,30p" "{}";
echo "== bare #[test] occurrences =="; rg -n "^\s*#\[test\]" "{}";
echo "== allow-bare-test annotations =="; rg -n "allow-bare-test" "{}"
'Repository: jmagar/soldr
Length of output: 1338
Move the file to timed_test! declarations.
crates/soldr-cli/tests/cli_toolchain.rs imports soldr_cli::timed_test, but no allow-bare-test annotation exists and all tests in this file still use bare #[test]; this new functional test should be declared with timed_test! instead.
🤖 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/soldr-cli/tests/cli_toolchain.rs` around lines 304 - 305, Update the
test declaration for
toolchain_prepare_host_cargo_keeps_managed_install_home_without_managed_rustup_home
to use the imported timed_test! macro instead of bare #[test], matching the
file’s required timed-test convention.
Source: Coding guidelines
| impl ShutdownSignal { | ||
| pub fn request(&self) { | ||
| self.requested.store(true, Ordering::Release); | ||
| self.notify.notify_waiters(); | ||
| } | ||
|
|
||
| pub fn is_requested(&self) -> bool { | ||
| self.requested.load(Ordering::Acquire) | ||
| } | ||
|
|
||
| pub async fn wait(&self) { | ||
| while !self.is_requested() { | ||
| self.notify.notified().await; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
tokio Notify notify_waiters no stored permit lost notification notified enable pattern
💡 Result:
In Tokio's tokio::sync::Notify, the notify_waiters() method and notify_one() operate on different notification mechanics [1][2]. The behavior where notify_waiters() does not store a permit and appears to "lose" notifications is by design, as it only notifies tasks that are already registered as waiters [1]. Key concepts regarding this pattern include: 1. No Stored Permit: Unlike notify_one(), which can store a single permit if no waiters are present (allowing a future created later to complete immediately), notify_waiters() does not store any state [1][2]. It specifically iterates over and notifies tasks that have already called notified() [1]. If no tasks are currently waiting, a call to notify_waiters() has no effect on subsequent waiters [1][3]. 2. The enable() Method: The enable() method on a Notified future allows you to register the task as a waiter before the future is polled [4][5]. This is often used in complex patterns (like multi-producer/multi-consumer scenarios) to ensure that a task is registered to receive notifications even if it has not yet reached an .await point [5]. For notify_waiters(), wakeups are received as long as the notification happens after the Notified future has been created, regardless of whether enable() or poll() has been called [4][5]. 3. Lost Notifications: If you require notifications to be persisted for tasks that have not yet been created or registered, Notify may not be the appropriate primitive [3]. Because notify_waiters() only affects current waiters, any task that calls notified() after notify_waiters() has been invoked will wait indefinitely (unless another notification occurs) [1][6]. If you need a broadcast-like pattern where new subscribers receive the latest notification, consider using a tokio::sync::watch channel or similar synchronization primitives designed for state propagation [3]. 4. Potential Interactions: Mixing notify_one() and notify_waiters() can lead to unexpected behavior if not managed carefully, as the order of operations and the specific type of notification (permit-based vs. waiter-based) affect which tasks are woken [7][6]. Correct code must handle the possibility of spurious wakeups and ensure that synchronization logic does not rely on transient notification states [7].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/sync/struct.Notify.html
- 2: https://docs.rs/tokio/latest/src/tokio/sync/notify.rs.html
- 3: What's the best way to notify many waiting tasks? tokio-rs/tokio#3718
- 4: https://github.com/tokio-rs/tokio/blob/master/tokio/src/sync/notify.rs
- 5: https://docs.rs/tokio/latest/tokio/sync/futures/struct.Notified.html
- 6: https://users.rust-lang.org/t/missing-notifications-with-tokio-notify-and-purpose-of-enable/137716
- 7: Notify::notify_one has priority on Notify::notify_waiters tokio-rs/tokio#7965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
fd -a 'maintenance.rs' . | sed 's#^\./##'
echo
echo "== lines 1-120 =="
sed -n '1,120p' crates/soldr-daemon/src/daemon/maintenance.rs | cat -n
echo
echo "== usages of ShutdownSignal and request/wait =="
rg -n "ShutdownSignal|shutdown\.|\.wait\(\)|signal::|ctrl-c|SIGTERM|Request::Shutdown|notify_waiters|notify_one" crates -S
echo
echo "== daemon async paths mentioning shutdown.wait =="
rg -n "shutdown\.wait|state\.shutdown|shutdown" crates/soldr-daemon/src/daemon -SRepository: jmagar/soldr
Length of output: 25250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MaintenanceContext::new and run_loop_inner =="
sed -n '151,205p' crates/soldr-daemon/src/daemon/maintenance.rs | cat -n
echo
echo "== server signal handling and state.shutdown.wait =="
sed -n '1030,1085p' crates/soldr-daemon/src/daemon/server.rs | cat -n
echo
echo "== Request::Shutdown handling =="
sed -n '1398,1422p' crates/soldr-daemon/src/daemon/server.rs | cat -n
echo
echo "== task/runtime hints =="
rg -n "tokio::runtime|multi_thread|RuntimeBuilder|run_async|tokio::spawn|Handle::spawn|async_main" crates/soldr-daemon/src/daemon/server.rs crates/soldr-daemon/src -S | head -80Repository: jmagar/soldr
Length of output: 12499
Register for Notify::notify_waiters() before checking requested.
notify_waiters() only wakes tasks already registered with notified() and does not store a permit for later waiters. In run_async, Ctrl-C / SIGTERM / Request::Shutdown, and the shutdown watchdog can call request() while one task is between is_requested() returning false and its notified() future being polled. The affected task then awaits permanently, leaving state.shutdown.wait().await and the server’s graceful-shutdown path stuck. Create the notified future immediately and check the flag after registering (for example with Notified::enable).
🤖 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/soldr-daemon/src/daemon/maintenance.rs` around lines 40 - 55, Update
ShutdownSignal::wait to create and register the Notify::notified future before
checking requested, using Notified::enable or the equivalent registration API.
Recheck is_requested after registration and only await the notification when
shutdown has not already been requested, preventing missed notify_waiters
signals while preserving the existing shutdown behavior.
| zccache is linked into the locally built `soldr-daemon`, so build Soldr from a | ||
| checkout whose `_vender/zccache` submodule contains the code under test. Debug | ||
| the resulting Soldr binary and its normal `.pdb` / `.dwp` / `.dSYM` output; | ||
| there is no external zccache daemon or `SOLDR_ZCCACHE_LOCAL_DIR` symbol-copy | ||
| path in the embedded architecture. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Inconsistent capitalization of "Soldr" vs. "soldr".
The new text uses "Soldr" for the binary/tool where the surrounding doc (lines 55-58) consistently uses lowercase "soldr" for the same referent.
📝 Proposed fix
-zccache is linked into the locally built `soldr-daemon`, so build Soldr from a
-checkout whose `_vender/zccache` submodule contains the code under test. Debug
-the resulting Soldr binary and its normal `.pdb` / `.dwp` / `.dSYM` output;
+zccache is linked into the locally built `soldr-daemon`, so build soldr from a
+checkout whose `_vender/zccache` submodule contains the code under test. Debug
+the resulting soldr binary and its normal `.pdb` / `.dwp` / `.dSYM` output;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zccache is linked into the locally built `soldr-daemon`, so build Soldr from a | |
| checkout whose `_vender/zccache` submodule contains the code under test. Debug | |
| the resulting Soldr binary and its normal `.pdb` / `.dwp` / `.dSYM` output; | |
| there is no external zccache daemon or `SOLDR_ZCCACHE_LOCAL_DIR` symbol-copy | |
| path in the embedded architecture. | |
| zccache is linked into the locally built `soldr-daemon`, so build soldr from a | |
| checkout whose `_vender/zccache` submodule contains the code under test. Debug | |
| the resulting soldr binary and its normal `.pdb` / `.dwp` / `.dSYM` output; | |
| there is no external zccache daemon or `SOLDR_ZCCACHE_LOCAL_DIR` symbol-copy | |
| path in the embedded architecture. |
🤖 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 `@docs/DEBUG_SIDECARS.md` around lines 62 - 66, Update the documentation text
around the embedded zccache architecture to use lowercase “soldr” consistently
for the binary/tool, replacing the newly introduced “Soldr” references while
preserving the existing technical guidance.
| ready = process.stdout.readline() | ||
| if ready != b"ready\n": | ||
| error = process.stderr.read().decode("utf-8", errors="replace").strip() | ||
| process.wait() | ||
| raise RuntimeError(f"soldr build lease helper failed to start: {error}") | ||
| try: | ||
| yield | ||
| finally: | ||
| process.stdin.close() | ||
| try: | ||
| process.wait(timeout=10) | ||
| except subprocess.TimeoutExpired: | ||
| process.kill() | ||
| process.wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '220,260p' src/soldr/__init__.py || true
echo "== relevant symbols/usages =="
rg -n "run_build_lease_helper|hold-build-lease|hold_build_lease|read_stdout|ready" -S .
echo "== diff stat and relevant diff =="
git diff --stat || true
git diff -- src/soldr/__init__.py | sed -n '1,240p' || trueRepository: jmagar/soldr
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== helper implementation =="
sed -n '1220,1280p' crates/soldr-core/src/main.rs
sed -n '1250,1335p' crates/soldr-cli/src/soldr_main.rs || true
echo "== test fixture around hold-build-lease =="
sed -n '210,250p' crates/soldr-cli/tests/common/mod.rs | sed -n '70,130p'
sed -n '210,250p' crates/soldr-cli/tests/common/mod.rs || true
echo "== Python test around mocked helper =="
sed -n '680,745p' tests/test_pep517_pyo3_policy.py || trueRepository: jmagar/soldr
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate soldr_main file =="
fd -a 'soldr_main\.rs$' . | sed -n '1,20p'
FILE="$(fd 'soldr_main\.rs$' . | head -n 1)"
echo "== helper implementation ($FILE) =="
if [ -n "${FILE:-}" ]; then
nl -ba "$FILE" | sed -n '1220,1280p'
nl -ba "$FILE" | sed -n '1250,1335p'
fi
echo "== behavioral probe: Python Popen readline/wait without timeout can block child =="
python3 - <<'PY'
import pickle
from pathlib import Path
p = Path(__file__ or ".")
for item in (Path("crates/soldr-cli/src/soldr_main.rs"), Path("crates/soldr-core/src/main.rs")):
print(f"{item}: {item.exists()}")
# Demonstrate current helper behavior from source: a process that blocks before stdout writes
# leaves Python readline() blocking until EOF, and an exited non-zero child would leave wait()
# unbounded as written.
print("current Python pattern")
print("ready = process.stdout.readline() # unbounded until stdout closed")
print("error = process.stderr.read() # unbounded if stdout closed before ready")
print("process.wait() # unbounded")
PYRepository: jmagar/soldr
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd 'soldr_main\.rs$' . | head -n 1)"
echo "== helper implementation ($FILE) =="
sed -n '1220,1280p' "$FILE"
sed -n '1250,1335p' "$FILE"
echo "== Python tests around build lease helper =="
sed -n '680,745p' tests/test_pep517_pyo3_policy.py || trueRepository: jmagar/soldr
Length of output: 7989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd 'soldr_main\.rs$' . | head -n 1)"
echo "== dispatch path invoking run_build_lease_helper =="
rg -n "run_build_lease_helper|hold-build-lease|Subcommand::Gc|gc" "$FILE" crates/soldr-daemon crates/soldr-core -S | head -n 120
echo "== cache_lib build_active implementation =="
sed -n '220,280p' crates/soldr-cache/src/cache_lib/build_active.rsRepository: jmagar/soldr
Length of output: 14713
Bound the build-lease helper startup read and failure wait.
run_build_lease_helper() acquires BuildActivityLease::acquire() before printing ready, and acquire() can block under lock contention. Since _hold_build_lease() waits via readline() on stdout before it can enter/exit the delegate hook, a contended helper makes every delegated PEP 517 invocation hang indefinitely with no recovery. The error-path process.wait() also needs a timeout; use the same bounded wait/kill pattern as teardown.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 242-242: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 `@src/soldr/__init__.py` around lines 238 - 251, Update _hold_build_lease()
around the helper startup readline to use a bounded wait so lock contention
cannot hang delegated PEP 517 invocations indefinitely. On startup failure,
replace the unbounded process.wait() with the existing timeout/kill/wait
teardown pattern, preserving the RuntimeError and captured stderr details.
Source: Linters/SAST tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d784659c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Last protocol spoken by pre-generation shutdown acknowledgements. This | ||
| /// fallback exists only to retire an older local daemon safely during rollout. | ||
| const LEGACY_SHUTDOWN_PROTOCOL_VERSION: u32 = 17; |
There was a problem hiding this comment.
When an upgraded CLI is talking to the daemon from the current base/released code, that daemon still speaks protocol 16, not 17. Because client::shutdown only retries v17 after the v18 request fails, v16 daemons never receive a valid graceful Shutdown; callers then fall into the PID-displacement path, which can SIGTERM/SIGKILL the old daemon instead of waiting for its embedded-cache flush. Include v16 in this compatibility bridge (or make the legacy retry cover the previous protocol) so upgrades can retire the existing daemon safely.
Useful? React with 👍 / 👎.
| match crate::daemon::client::flush_caches(&sock) { | ||
| Ok(report) if report.is_complete() => {} | ||
| Ok(report) => Err(format!( | ||
| "embedded zccache checkpoint incomplete: {}", | ||
| report.incomplete_reason() | ||
| ))?, | ||
| Err(crate::daemon::client::ClientError::NotRunning) => return Ok(()), | ||
| Err(error) => return Err(format!("embedded zccache checkpoint failed: {error:?}")), |
There was a problem hiding this comment.
When soldr save --cache-dir <soldr cache> runs after upgrading while an older daemon is still alive, this flush_caches call fails before the code reaches the shutdown path that is supposed to quiesce the cache. Older daemons either reject the newer protocol or do not return the new structured CacheFlushed report, but their graceful shutdown path still flushes embedded state; returning here makes upgraded CI refuse to save the cache instead of quiescing it. For compatibility errors, try the shutdown bridge before failing the archive.
Useful? React with 👍 / 👎.
| fn archive_contains_embedded_cache(cache_dir: &Path, embedded_root: &Path) -> Result<bool, String> { | ||
| let archive_root = path_for_containment(cache_dir)?; | ||
| let embedded_root = path_for_containment(embedded_root)?; | ||
| Ok(embedded_root.starts_with(archive_root)) |
There was a problem hiding this comment.
This containment check only returns true when the archive root is a parent of embedded_root. If a user archives the actual versioned embedded state subtree beneath that root, the archive still contains live embedded-cache files, but quiesce_embedded_state_before_save returns without flushing or stopping the daemon. Treat overlapping paths in either direction (or compare against the versioned effective cache root too) so subtree archives do not capture a non-quiescent cache.
Useful? React with 👍 / 👎.
…HOME blessed_build: on non-Darwin hosts, dsymutil provisioning for aarch64-apple-darwin cross-prep can fail (component unavailable) without Darwin debuginfo actually being needed for every build. Downgrade the previously hard-failing `?` into a warning + soft skip, matching the existing pattern for Apple SDK provisioning failures just below it. toolchain_doctor: the rustlib-integrity probe compared a sysroot queried via the host-resolved RUSTUP_HOME against the installed-target list queried via Soldr's managed RUSTUP_HOME (or vice versa depending on resolution order), which could flag installed targets as corrupt purely from the mismatch. Pin both `rustc --print sysroot` and `rustup target list --installed` to the same explicit RUSTUP_HOME up front so the comparison is always apples-to-apples.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed the three review findings:
Verification: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/soldr-cli/src/toolchain_doctor.rs`:
- Around line 285-287: Derive managed_rustup_home_override from the selected
compiler’s ownership, not merely from managed directory availability. Reuse the
same managed Cargo/Rustup storage ownership condition as
apply_resolved_toolchain_homes, and pass the override only for managed rustc
paths. Apply this consistently to query_rustc_sysroot and the rustup target-list
probe so host-owned rustc uses its host environment.
In `@crates/soldr-cli/tests/cli_cargo_basic.rs`:
- Around line 96-101: Update the fake Cargo scripts and assertions in the basic
CLI tests to inspect and log RUSTFLAGS, emitting the stable-rustc diagnostic
whenever it contains -Zthreads. Extend the build_attempts validation so the
retry is required to omit -Zthreads while preserving its existing trusted,
uncached arguments. Keep the separate forced-retry-failure behavior unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9796669d-8696-48db-860c-86eca0b1ab4a
📒 Files selected for processing (5)
crates/soldr-cli/src/blessed_build.rscrates/soldr-cli/src/cargo_front_door/mod.rscrates/soldr-cli/src/cargo_front_door/tests.rscrates/soldr-cli/src/toolchain_doctor.rscrates/soldr-cli/tests/cli_cargo_basic.rs
| let rustup_home_override = managed_rustup_home_override(); | ||
|
|
||
| let toolchain_root = match query_rustc_sysroot(&rustc, rustup_home_override.as_deref()) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not force managed RUSTUP_HOME for a host-owned rustc.
Line 285 selects the managed home only from its directory availability. It does not check whether the resolved rustc uses that home. A host-owned direct rustc keeps its host sysroot, while rustup target list --installed reads the managed home at Lines 481-486. The probe can then report valid targets as corrupt.
Derive the override from the selected compiler ownership. Apply it only when rustc is within managed Cargo or Rustup storage. This must match the ownership condition in apply_resolved_toolchain_homes.
Also applies to: 302-302, 391-393, 479-486
🤖 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/soldr-cli/src/toolchain_doctor.rs` around lines 285 - 287, Derive
managed_rustup_home_override from the selected compiler’s ownership, not merely
from managed directory availability. Reuse the same managed Cargo/Rustup storage
ownership condition as apply_resolved_toolchain_homes, and pass the override
only for managed rustc paths. Apply this consistently to query_rustc_sysroot and
the rustup target-list probe so host-owned rustc uses its host environment.
| assert!( | ||
| build_attempts.iter().any(|line| line.contains( | ||
| "attempt=1 trust=inherited sentinel= wrapper= cache=0 args=build" | ||
| )), | ||
| "the retry must preserve trusted workspace state, remain uncached, and keep its internal sentinel out of Cargo: {log}", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the fake Cargo validate removal of -Zthreads.
The fake Cargo scripts fail the first attempt only because the marker is absent. They do not inspect RUSTFLAGS. The success test can pass if the retry still sends RUSTFLAGS=-Zthreads=8.
Make the fake Cargo emit the stable-rustc diagnostic while RUSTFLAGS contains -Zthreads. Log the value and assert that the retry omits -Zthreads. Preserve the separate forced-retry-failure path.
Also applies to: 292-324, 348-348
🤖 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/soldr-cli/tests/cli_cargo_basic.rs` around lines 96 - 101, Update the
fake Cargo scripts and assertions in the basic CLI tests to inspect and log
RUSTFLAGS, emitting the stable-rustc diagnostic whenever it contains -Zthreads.
Extend the build_attempts validation so the retry is required to omit -Zthreads
while preserving its existing trusted, uncached arguments. Keep the separate
forced-retry-failure behavior unchanged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 95 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/soldr-daemon/src/daemon/server.rs">
<violation number="1" location="crates/soldr-daemon/src/daemon/server.rs:951">
P1: An older daemon can start between the occupancy check and `claim_unix_endpoint`, after which the unconditional `remove_file` unlinks its live socket and this process binds a second listener for the same root. The endpoint claim needs an ownership fence compatible with older daemons or a recheck/atomic takeover protocol before allowing both processes to run.</violation>
<violation number="2" location="crates/soldr-daemon/src/daemon/server.rs:1102">
P1: After a clean shutdown, PID reuse by an ordinary `soldr` command is treated as a live daemon: direct startup returns `AlreadyRunning`, while detached recovery can send SIGTERM to that unrelated CLI. Retaining the PID file needs a verified endpoint/process identity before `stale_daemon_occupies_endpoint` is used for ownership or signaling.</violation>
</file>
<file name="crates/soldr-cli/src/cache/session.rs">
<violation number="1" location="crates/soldr-cli/src/cache/session.rs:617">
P1: A refused IPC connection is treated as proof that the daemon was already stopped, even when the initial live-PID check still found a daemon. This can report successful shutdown and archive partial session logs/cache state while that daemon is still completing its graceful flush; only mark it exited when a fresh liveness check is clear, otherwise return an unconfirmed-shutdown failure or use the verified fallback.</violation>
</file>
<file name="crates/soldr-daemon/src/daemon/maintenance.rs">
<violation number="1" location="crates/soldr-daemon/src/daemon/maintenance.rs:52">
P1: Daemon shutdown can hang if `request()` lands after `wait()` observes `false` but before its `Notify` waiter is registered; register/enable the waiter before checking the atomic flag so `notify_waiters()` cannot be lost.</violation>
</file>
<file name="crates/soldr-daemon/src/daemon/lifecycle.rs">
<violation number="1" location="crates/soldr-daemon/src/daemon/lifecycle.rs:291">
P1: A stale daemon that acknowledges shutdown can still be flushing after five seconds, so displacement reports failure and the spawn path proceeds while the old generation still owns the root, causing the replacement daemon to fail startup and builds to fall back or remain on the stale daemon. The acknowledged wait should use `GRACEFUL_SHUTDOWN_WAIT_TIMEOUT`, and a timed-out result should be propagated so no successor is launched until that generation exits.</violation>
</file>
<file name="crates/soldr-cache/src/cache_lib/path_safety.rs">
<violation number="1" location="crates/soldr-cache/src/cache_lib/path_safety.rs:25">
P1: A concurrent cache-path swap can make destructive collectors escape the selected product root because this helper only checks names before returning and callers reopen those names later. Handle-based, no-follow traversal should retain the validated directory capabilities through enumeration and deletion, as the existing `no_cache_detach` code does.</violation>
</file>
<file name="crates/soldr-cli/src/toolchain_doctor.rs">
<violation number="1" location="crates/soldr-cli/src/toolchain_doctor.rs:285">
P1: A host-owned `rustc` with an explicit or repository-local `RUSTUP_HOME` is now run against Soldr's managed home whenever that directory exists. If the managed home is default-less or has different target metadata, the doctor can skip a valid host setup or report its targets as corrupt; the override should be derived from the resolved binary/current Rustup context rather than `is_dir()` alone.</violation>
</file>
<file name="crates/soldr-cli/src/blessed_build.rs">
<violation number="1" location="crates/soldr-cli/src/blessed_build.rs:236">
P1: Linux-hosted Darwin builds can reach rustc's packed-debuginfo step without a runnable `dsymutil`, even though preparation reported it available. The managed bundle or rustup component must provide a `dsymutil` name (for example by creating an alias) before this helper returns success.</violation>
<violation number="2" location="crates/soldr-cli/src/blessed_build.rs:397">
P2: A custom `SOLDR_DSYMUTIL=/path/to/my-tool` is reported as usable, but the child can only find it if the file is already named `dsymutil` or `llvm-dsymutil`; otherwise packed Darwin debuginfo still fails. The override should validate or materialize the expected executable name instead of silently relying on the basename.</violation>
<violation number="3" location="crates/soldr-cli/src/blessed_build.rs:440">
P2: With `RUSTUP_TOOLCHAIN` set to a channel different from `rust-toolchain.toml`, llvm-tools-preview is installed into the manifest channel while rustc uses the explicit channel, so the post-install search misses the component and packed debuginfo can still fail. The explicit `RUSTUP_TOOLCHAIN` should take precedence over the manifest channel for this install.</violation>
</file>
<file name="crates/soldr-cache/src/cache_lib/build_active.rs">
<violation number="1" location="crates/soldr-cache/src/cache_lib/build_active.rs:78">
P1: Direct `RUSTC_WRAPPER`/`zccache-soldr` compiles can overlap destructive maintenance because no production path acquires `BuildRootLease` outside `BuildActivityLease`; retain a shared root lease for the daemon compile lifecycle (or its IPC caller). This leaves the documented non-`soldr cargo` session path unprotected.</violation>
<violation number="2" location="crates/soldr-cache/src/cache_lib/build_active.rs:95">
P2: The new root maintenance lock is held exclusively for the complete auto-GC background pass, and `BuildRootLease::acquire`'s `lock_shared` blocks a starting `soldr cargo build` with no timeout until the entire pass finishes. The pass includes slow, unbounded work (`cargo clean gc` and recursive `remove_dir_all` across multiple volumes), so a user build can stall for several minutes — a behavioral regression from the previous design that re-checked `build_activity_active` between volumes and yielded to in-flight builds. The 'bounded pass' assumption documented in `BuildRootLease::acquire` isn't actually enforced anywhere. Consider bounding the exclusive hold (e.g., a deadline/abort or re-acquiring the lease per-tier rather than once for the whole pass) so a stray maintenance run can't indefinitely block builds.</violation>
</file>
<file name="crates/soldr-cli/src/cargo_front_door/darwin_embed.rs">
<violation number="1" location="crates/soldr-cli/src/cargo_front_door/darwin_embed.rs:133">
P1: A successful packed-DWARF build can now fail during post-processing with `unable to invoke llvm-objcopy` when invoked through `soldr cargo` or on a native macOS setup that does not already expose that binary on PATH. The embedding path should resolve/provision a compatible objcopy for every caller, or gracefully skip embedding when it is unavailable instead of turning Cargo success into an error.</violation>
<violation number="2" location="crates/soldr-cli/src/cargo_front_door/darwin_embed.rs:150">
P2: An interruption during this replacement can leave the target artifact missing even though Cargo succeeded, because the live file is moved away before the staged file is installed. An atomic replace operation (or a platform-specific replace primitive with rollback-safe semantics) would preserve the artifact path across crashes.</violation>
<violation number="3" location="crates/soldr-cli/src/cargo_front_door/darwin_embed.rs:156">
P2: When the staged file lives in a system temp dir (tempfile::tempdir()) and the real artifact is in the cargo target dir on a different filesystem, the final `std::fs::rename(&staged, artifact)` fails with EXDEV. Because this error is returned from embed_one() and propagated (via the post-cargo closure in cargo_front_door/mod.rs) as a hard `post_cargo_result?`, a successful cargo build would be reported as failed purely because DWARF embedding couldn't promote the rewritten artifact across the filesystem boundary. Consider staging the rewritten artifact in the artifact's own directory (so source/target are on the same mount, e.g. a sibling temp file under target_dir) before the final rename, or fall back to copy+remove of the original on EXDEV.</violation>
</file>
<file name="crates/soldr-cli/src/wrapper.rs">
<violation number="1" location="crates/soldr-cli/src/wrapper.rs:260">
P1: Cached wrapper builds still bypass the host-toolchain home fix: host-owned rustc invocations are executed by the embedded daemon, but this helper only configures locally spawned commands and the daemon request carries no corresponding environment. Propagating the resolved homes through the compile request/daemon execution context would prevent host rustup proxies from seeing Soldr's default-less managed `RUSTUP_HOME`.</violation>
</file>
<file name="crates/soldr-cli/src/save_load.rs">
<violation number="1" location="crates/soldr-cli/src/save_load.rs:166">
P2: Saving a supported subdirectory of the embedded cache can capture memory-only zccache state because containment is checked in only one direction. Treat overlapping roots as requiring quiescence, not just archives whose root is an ancestor of the embedded root.</violation>
<violation number="2" location="crates/soldr-cli/src/save_load.rs:203">
P1: The archive is not protected from a new daemon generation starting after this wait, so a concurrent build can mutate embedded state during tar traversal and reintroduce the inconsistent snapshot this change is intended to prevent. Holding the cache-root ownership/maintenance lease through the entire archive operation, or otherwise fencing new daemon startup until save completes, would make quiescence a real snapshot barrier.</violation>
<violation number="3" location="crates/soldr-cli/src/save_load.rs:243">
P3: Invalid `soldr save` invocations can still terminate the ambient daemon before the command reports its argument error. Validate the mutually exclusive/required options before performing cache quiescence so failed CLI requests have no destructive lifecycle side effect.</violation>
</file>
<file name="crates/soldr-daemon/src/zccache_embedded.rs">
<violation number="1" location="crates/soldr-daemon/src/zccache_embedded.rs:162">
P1: Pre-redaction secrets can remain in unselected legacy compile journals after startup. The scrub should cover every retained legacy journal (or remove/sanitize those roots during the first migration pass), rather than only the selected `embedded-v1` root.</violation>
</file>
<file name="crates/soldr-daemon/src/daemon/history_gc.rs">
<violation number="1" location="crates/soldr-daemon/src/daemon/history_gc.rs:67">
P1: Build-history publication can write outside the selected root when an archive path contains a symlink or reparse point: `create_dir_all` and `write` follow it before the caller copies the build artifacts. Validate the archive directory beneath `SoldrPaths::root` and reject links/reparse points before creating or writing publication markers.</violation>
<violation number="2" location="crates/soldr-daemon/src/daemon/history_gc.rs:153">
P2: A failed archive removal can leave history rows pointing at a missing or partial archive because every `remove_dir_all` error restores the full pre-GC record. Treat `NotFound`/partial deletion as unavailable, or atomically rename the archive to a root-local quarantine before clearing/restoring database paths.</violation>
<violation number="3" location="crates/soldr-daemon/src/daemon/history_gc.rs:202">
P2: Retention trusts marker symlinks as real completion markers, so a pre-redaction archive can bypass migration and remain on disk despite the file’s link-safety policy. Read marker state with `symlink_metadata` and reject links/reparse points before trusting or writing any completion or migration marker.</violation>
</file>
<file name="crates/soldr-cache/src/cache_lib/cook_gc.rs">
<violation number="1" location="crates/soldr-cache/src/cache_lib/cook_gc.rs:243">
P2: A redb eviction failure now leaves a live index row pointing at a deleted artifact, so subsequent CookLookup requests report hits but hydration fails until another maintenance pass retries the row. Removing the index entry first, or adding durable reconciliation/tombstoning for this two-resource deletion, would avoid exposing this inconsistent state.</violation>
</file>
<file name="crates/soldr-cache/src/cache_lib/trash_gc.rs">
<violation number="1" location="crates/soldr-cache/src/cache_lib/trash_gc.rs:28">
P2: A sweep can return success with zero retained entries even when it could not inspect a trash bucket or child, so daemon maintenance and the CLI under-report work that needs a later retry. Count each enumeration/metadata failure as retained (or propagate it) instead of silently continuing.</violation>
<violation number="2" location="crates/soldr-cache/src/cache_lib/trash_gc.rs:54">
P2: A FIFO, socket, or device node inside a moved worktree is silently left behind and is not reported as retained because it is neither `is_dir()` nor `is_file()`. Treat every non-link, non-directory entry as a `remove_file` candidate, or explicitly count unsupported types as retained.</violation>
</file>
<file name="crates/soldr-cli/src/cargo_front_door/mod.rs">
<violation number="1" location="crates/soldr-cli/src/cargo_front_door/mod.rs:508">
P2: Concurrent builds can corrupt history indexes because each publisher runs the retention sweep without serializing it; if two sweeps select the same archive, one can restore a stale `BuildRecord` after the other deletes the directory. Running this cap enforcement under a dedicated serialized maintenance lock, or deferring it to the daemon-owned sweep, would avoid resurrecting paths to deleted archives.</violation>
</file>
<file name="crates/soldr-cli/src/pyo3_detect.rs">
<violation number="1" location="crates/soldr-cli/src/pyo3_detect.rs:555">
P2: A stalled metadata probe now delays the target build for 60 seconds by default (or an unrelated global override), rather than its previous fixed 30-second fallback. Keep this advisory probe on its dedicated 30-second deadline while capturing stdout/stderr concurrently.</violation>
<violation number="2" location="crates/soldr-cli/src/pyo3_detect.rs:828">
P2: This local mutex does not isolate the process-wide cargo override from tests in other modules, causing parallel tests to intermittently resolve the temporary fake cargo. Use `crate::TEST_PROCESS_ENV_LOCK` for this guard instead.</violation>
</file>
<file name="crates/soldr-cli/src/cargo_front_door/zthreads_fallback.rs">
<violation number="1" location="crates/soldr-cli/src/cargo_front_door/zthreads_fallback.rs:55">
P2: `RUSTFLAGS="-Z threads=8"` still fails without the fallback even though it is the same rustc option: the parser recognizes only the attached `-Zthreads=` spelling and treats standalone `-Z` as another unstable flag. Supporting the two-token spelling in both detection and rewriting would make the fallback cover Cargo's valid environment representations.</violation>
</file>
<file name="crates/soldr-daemon/src/daemon/db.rs">
<violation number="1" location="crates/soldr-daemon/src/daemon/db.rs:222">
P2: Large history sweeps perform multiple redb opens and setup/write transactions—one per removed archive—which can make maintenance latency grow with the number of retained sessions. Batching the selected IDs or reusing a single database/write handle for the sweep would avoid this N+1 database work.</violation>
<violation number="2" location="crates/soldr-daemon/src/daemon/db.rs:234">
P3: Retries can over-report `database_rows_updated` after the paths were already cleared; only write and count the row when at least one archived path was previously `Some`.</violation>
</file>
<file name="crates/soldr-cache/src/cache_lib/pep517_gc.rs">
<violation number="1" location="crates/soldr-cache/src/cache_lib/pep517_gc.rs:74">
P2: Direct symlink/reparse entries are silently ignored instead of being reported as retained failures. Checking the link marker before the directory filter would make maintenance status surface unsafe entries consistently, as it already does for nested links.</violation>
</file>
<file name="crates/soldr-cli/src/doctor.rs">
<violation number="1" location="crates/soldr-cli/src/doctor.rs:544">
P2: `collect_zccache_bundle` now hard-errors when no home dir exists: on a headless box with neither HOME/USERPROFILE nor SOLDR_CACHE_DIR, `soldr doctor` previously still reported the toolchain/manifest results, but now `SoldrPaths::new()?` bails out of the whole command early. Since the maintenance status is just an advisory detail, resolve paths non-fatally (`.ok()?`) like `collect_cook_stats` does, so a missing home dir downgrades only the maintenance line instead of aborting doctor.</violation>
</file>
<file name="crates/soldr-cli/src/binaries.rs">
<violation number="1" location="crates/soldr-cli/src/binaries.rs:251">
P2: Large builds now incur extra synchronous filesystem traversal for every compiler/tool spawn, including host binaries that will not receive managed homes. Caching the canonical managed roots and avoiding repeated binary canonicalization on the wrapper hot path would preserve the symlink-aware classification without adding per-unit latency.</violation>
</file>
<file name="crates/soldr-cli/src/logs_cmd.rs">
<violation number="1" location="crates/soldr-cli/src/logs_cmd.rs:270">
P2: The `zccache-embedded-logs` entry should use the daemon's authoritative `embedded_version_root` helper rather than duplicating zccache layout resolution; otherwise `soldr logs paths` can point at a different directory than the compile journal used by build-history finalization after a layout/version change.</violation>
</file>
<file name="crates/soldr-cli/src/gc/auto.rs">
<violation number="1" location="crates/soldr-cli/src/gc/auto.rs:187">
P3: Auto-GC logs `reason=build_active` whenever the root maintenance lease is busy, including conflicts with daemon maintenance where no build is running. Recording a generic `root_lease_busy` reason here would keep the deferral diagnostics accurate.</violation>
</file>
<file name="src/soldr/__init__.py">
<violation number="1" location="src/soldr/__init__.py:238">
P2: The readiness handshake in `_hold_build_lease` blocks forever on `process.stdout.readline()` with no timeout. The child `soldr gc hold-build-lease` prints `ready` only after it acquires the OS build lease, and that acquisition blocks while a `soldr gc` maintenance pass holds the exclusive root lock (see `BuildRootLease::acquire` in build_active.rs). If a maintenance pass is running or the root lock is otherwise contended, a delegated PEP 517 build can hang indefinitely instead of erroring or waiting in a bounded, observable way. Consider bounding the readiness wait (deadline/select) so the build fails fast with a clear message instead of blocking the whole `pip`/`uv` invocation with no feedback.</violation>
<violation number="2" location="src/soldr/__init__.py:399">
P2: Every PEP 517 hook now shells out to the soldr binary (up to two `soldr version --json` / `soldr status --json` subprocesses) just to resolve the cache root whenever `SOLDR_CACHE_DIR` isn't already exported. Since `_prep_env` runs on each hook (`get_requires_for_build_*`, `prepare_metadata_for_build_*`, `build_wheel`, `build_sdist`), a normal `pip install`/`uv pip install` pays this process-spawn cost repeatedly, even when the stable-target knob is disabled and the root isn't otherwise needed. Consider resolving the soldr root once per process (module-level cache) or lazily only when `SOLDR_PEP517_STABLE_TARGET_DIR` is actually active, so the extra subprocesses are avoided on the common path.</violation>
</file>
<file name="crates/soldr-cli/tests/cli_daemon_lifecycle.rs">
<violation number="1" location="crates/soldr-cli/tests/cli_daemon_lifecycle.rs:394">
P3: The new standalone test hand-rolls the same env scrubbing already encapsulated in `scrub_outer_soldr_runtime`, hardcoding `SOLDR_INTERNAL_DAEMON_EXE` instead of the `SOLDR_DAEMON_EXE_ENV_VAR` constant. If the scrub list grows later, this path will silently diverge; reuse the helper (e.g. `scrub_outer_soldr_runtime(&mut cmd);` after the `.env(...)` chain and drop the manual removals).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // soldr#1495: drop the version claim so a stale manifest can't outlive | ||
| // its writer and make the next client think a daemon is still live. | ||
| crate::daemon::broker_discovery::remove_root_version_claim(&paths); | ||
| // Deliberately retain the PID file, version claim, and Unix socket node. |
There was a problem hiding this comment.
P1: After a clean shutdown, PID reuse by an ordinary soldr command is treated as a live daemon: direct startup returns AlreadyRunning, while detached recovery can send SIGTERM to that unrelated CLI. Retaining the PID file needs a verified endpoint/process identity before stale_daemon_occupies_endpoint is used for ownership or signaling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-daemon/src/daemon/server.rs, line 1102:
<comment>After a clean shutdown, PID reuse by an ordinary `soldr` command is treated as a live daemon: direct startup returns `AlreadyRunning`, while detached recovery can send SIGTERM to that unrelated CLI. Retaining the PID file needs a verified endpoint/process identity before `stale_daemon_occupies_endpoint` is used for ownership or signaling.</comment>
<file context>
@@ -1045,18 +1099,13 @@ pub async fn run_async(opts: ServerOptions) -> Result<(), ServerError> {
- // soldr#1495: drop the version claim so a stale manifest can't outlive
- // its writer and make the next client think a daemon is still live.
- crate::daemon::broker_discovery::remove_root_version_claim(&paths);
+ // Deliberately retain the PID file, version claim, and Unix socket node.
+ // A check-then-unlink fence is not atomic: an older Soldr release that
+ // does not honor `root-owner.lock` can publish a successor between the
</file context>
| )); | ||
| if daemon_pid.is_some() && crate::daemon::lifecycle::displace_stale_daemon(&paths) { | ||
| output.daemon_stopped = true; | ||
| output.daemon_exited = true; |
There was a problem hiding this comment.
P1: A refused IPC connection is treated as proof that the daemon was already stopped, even when the initial live-PID check still found a daemon. This can report successful shutdown and archive partial session logs/cache state while that daemon is still completing its graceful flush; only mark it exited when a fresh liveness check is clear, otherwise return an unconfirmed-shutdown failure or use the verified fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-cli/src/cache/session.rs, line 617:
<comment>A refused IPC connection is treated as proof that the daemon was already stopped, even when the initial live-PID check still found a daemon. This can report successful shutdown and archive partial session logs/cache state while that daemon is still completing its graceful flush; only mark it exited when a fresh liveness check is clear, otherwise return an unconfirmed-shutdown failure or use the verified fallback.</comment>
<file context>
@@ -458,85 +501,172 @@ fn clear_session_artifacts(zccache_dir: &std::path::Path) -> Result<bool, SoldrE
+ ));
+ if daemon_pid.is_some() && crate::daemon::lifecycle::displace_stale_daemon(&paths) {
+ output.daemon_stopped = true;
+ output.daemon_exited = true;
+ notes.push("soldr-daemon stopped through verified-PID fallback".into());
+ } else {
</file context>
|
|
||
| pub async fn wait(&self) { | ||
| while !self.is_requested() { | ||
| self.notify.notified().await; |
There was a problem hiding this comment.
P1: Daemon shutdown can hang if request() lands after wait() observes false but before its Notify waiter is registered; register/enable the waiter before checking the atomic flag so notify_waiters() cannot be lost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-daemon/src/daemon/maintenance.rs, line 52:
<comment>Daemon shutdown can hang if `request()` lands after `wait()` observes `false` but before its `Notify` waiter is registered; register/enable the waiter before checking the atomic flag so `notify_waiters()` cannot be lost.</comment>
<file context>
@@ -0,0 +1,964 @@
+
+ pub async fn wait(&self) {
+ while !self.is_requested() {
+ self.notify.notified().await;
+ }
+ }
</file context>
| return true; | ||
| match crate::daemon::client::shutdown(&sock) { | ||
| Ok(responder) => { | ||
| return wait_for_shutdown_responder(&sock, responder, Duration::from_secs(5)) |
There was a problem hiding this comment.
P1: A stale daemon that acknowledges shutdown can still be flushing after five seconds, so displacement reports failure and the spawn path proceeds while the old generation still owns the root, causing the replacement daemon to fail startup and builds to fall back or remain on the stale daemon. The acknowledged wait should use GRACEFUL_SHUTDOWN_WAIT_TIMEOUT, and a timed-out result should be propagated so no successor is launched until that generation exits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-daemon/src/daemon/lifecycle.rs, line 291:
<comment>A stale daemon that acknowledges shutdown can still be flushing after five seconds, so displacement reports failure and the spawn path proceeds while the old generation still owns the root, causing the replacement daemon to fail startup and builds to fall back or remain on the stale daemon. The acknowledged wait should use `GRACEFUL_SHUTDOWN_WAIT_TIMEOUT`, and a timed-out result should be propagated so no successor is launched until that generation exits.</comment>
<file context>
@@ -159,45 +209,105 @@ fn pid_is_soldr_daemon(pid: u32) -> bool {
- return true;
+ match crate::daemon::client::shutdown(&sock) {
+ Ok(responder) => {
+ return wait_for_shutdown_responder(&sock, responder, Duration::from_secs(5))
+ .is_complete();
+ }
</file context>
|
|
||
| /// Validate that `directory` is a real directory beneath the exact selected | ||
| /// product `boundary`, with no link/reparse component between them. | ||
| pub fn validate_owned_directory(boundary: &Path, directory: &Path) -> io::Result<()> { |
There was a problem hiding this comment.
P1: A concurrent cache-path swap can make destructive collectors escape the selected product root because this helper only checks names before returning and callers reopen those names later. Handle-based, no-follow traversal should retain the validated directory capabilities through enumeration and deletion, as the existing no_cache_detach code does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-cache/src/cache_lib/path_safety.rs, line 25:
<comment>A concurrent cache-path swap can make destructive collectors escape the selected product root because this helper only checks names before returning and callers reopen those names later. Handle-based, no-follow traversal should retain the validated directory capabilities through enumeration and deletion, as the existing `no_cache_detach` code does.</comment>
<file context>
@@ -0,0 +1,102 @@
+
+/// Validate that `directory` is a real directory beneath the exact selected
+/// product `boundary`, with no link/reparse component between them.
+pub fn validate_owned_directory(boundary: &Path, directory: &Path) -> io::Result<()> {
+ let relative = directory.strip_prefix(boundary).map_err(|_| {
+ io::Error::new(
</file context>
| ) -> "dict[str, str]": | ||
| env = os.environ.copy() | ||
| if not env.get("SOLDR_CACHE_DIR", "").strip(): | ||
| env["SOLDR_CACHE_DIR"] = str(_selected_soldr_root(env)) |
There was a problem hiding this comment.
P2: Every PEP 517 hook now shells out to the soldr binary (up to two soldr version --json / soldr status --json subprocesses) just to resolve the cache root whenever SOLDR_CACHE_DIR isn't already exported. Since _prep_env runs on each hook (get_requires_for_build_*, prepare_metadata_for_build_*, build_wheel, build_sdist), a normal pip install/uv pip install pays this process-spawn cost repeatedly, even when the stable-target knob is disabled and the root isn't otherwise needed. Consider resolving the soldr root once per process (module-level cache) or lazily only when SOLDR_PEP517_STABLE_TARGET_DIR is actually active, so the extra subprocesses are avoided on the common path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/soldr/__init__.py, line 399:
<comment>Every PEP 517 hook now shells out to the soldr binary (up to two `soldr version --json` / `soldr status --json` subprocesses) just to resolve the cache root whenever `SOLDR_CACHE_DIR` isn't already exported. Since `_prep_env` runs on each hook (`get_requires_for_build_*`, `prepare_metadata_for_build_*`, `build_wheel`, `build_sdist`), a normal `pip install`/`uv pip install` pays this process-spawn cost repeatedly, even when the stable-target knob is disabled and the root isn't otherwise needed. Consider resolving the soldr root once per process (module-level cache) or lazily only when `SOLDR_PEP517_STABLE_TARGET_DIR` is actually active, so the extra subprocesses are avoided on the common path.</comment>
<file context>
@@ -355,6 +395,8 @@ def _prep_env(
) -> "dict[str, str]":
env = os.environ.copy()
+ if not env.get("SOLDR_CACHE_DIR", "").strip():
+ env["SOLDR_CACHE_DIR"] = str(_selected_soldr_root(env))
explicit_profile = _explicit_profile(config_settings, editable=editable)
if explicit_profile:
</file context>
| match crate::cache_lib::build_active::MaintenanceLease::try_acquire(&paths) { | ||
| Ok(Some(lease)) => lease, | ||
| Ok(None) => { | ||
| defer_for_active_build(&paths, &log_path, "root_lease"); |
There was a problem hiding this comment.
P3: Auto-GC logs reason=build_active whenever the root maintenance lease is busy, including conflicts with daemon maintenance where no build is running. Recording a generic root_lease_busy reason here would keep the deferral diagnostics accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-cli/src/gc/auto.rs, line 187:
<comment>Auto-GC logs `reason=build_active` whenever the root maintenance lease is busy, including conflicts with daemon maintenance where no build is running. Recording a generic `root_lease_busy` reason here would keep the deferral diagnostics accurate.</comment>
<file context>
@@ -180,6 +180,22 @@ fn run_auto_gc_background(paths_root: std::path::PathBuf, log_path: std::path::P
+ match crate::cache_lib::build_active::MaintenanceLease::try_acquire(&paths) {
+ Ok(Some(lease)) => lease,
+ Ok(None) => {
+ defer_for_active_build(&paths, &log_path, "root_lease");
+ return;
+ }
</file context>
| defer_for_active_build(&paths, &log_path, "root_lease"); | |
| let _ = append_auto_gc_log_line( | |
| &log_path, | |
| "auto-gc status=deferred reason=root_lease_busy stage=root_lease", | |
| ); | |
| rearm_auto_gc_marker(&paths); |
| }; | ||
| if args.cache_dir.is_some() { | ||
| flush_embedded_state_before_save(); | ||
| if let Some(cache_dir) = args.cache_dir.as_deref() { |
There was a problem hiding this comment.
P3: Invalid soldr save invocations can still terminate the ambient daemon before the command reports its argument error. Validate the mutually exclusive/required options before performing cache quiescence so failed CLI requests have no destructive lifecycle side effect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-cli/src/save_load.rs, line 243:
<comment>Invalid `soldr save` invocations can still terminate the ambient daemon before the command reports its argument error. Validate the mutually exclusive/required options before performing cache quiescence so failed CLI requests have no destructive lifecycle side effect.</comment>
<file context>
@@ -161,8 +240,11 @@ pub fn run_save(args: SaveArgs) -> i32 {
};
- if args.cache_dir.is_some() {
- flush_embedded_state_before_save();
+ if let Some(cache_dir) = args.cache_dir.as_deref() {
+ if let Err(error) = quiesce_embedded_state_before_save(cache_dir) {
+ eprintln!("soldr save: refusing to archive a non-quiescent cache: {error}");
</file context>
| let Some(paths) = record.log_paths.as_mut() else { | ||
| continue; | ||
| }; | ||
| paths.archived_session_log_path = None; | ||
| paths.archived_journal_path = None; | ||
| paths.archived_session_stats_path = None; | ||
| paths.archived_compile_journal_path = None; | ||
| let bytes = prost_tagged_bytes(&wire::build_record_to_wire(&record)); | ||
| builds.insert(id, bytes.as_slice())?; | ||
| updated += 1; |
There was a problem hiding this comment.
P3: Retries can over-report database_rows_updated after the paths were already cleared; only write and count the row when at least one archived path was previously Some.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-daemon/src/daemon/db.rs, line 234:
<comment>Retries can over-report `database_rows_updated` after the paths were already cleared; only write and count the row when at least one archived path was previously `Some`.</comment>
<file context>
@@ -205,6 +205,48 @@ pub fn get_build(db_path: &Path, session_id: u64) -> Result<Option<BuildRecord>,
+ };
+ let mut record = decode_build_row(row.value())?;
+ drop(row);
+ let Some(paths) = record.log_paths.as_mut() else {
+ continue;
+ };
</file context>
| let Some(paths) = record.log_paths.as_mut() else { | |
| continue; | |
| }; | |
| paths.archived_session_log_path = None; | |
| paths.archived_journal_path = None; | |
| paths.archived_session_stats_path = None; | |
| paths.archived_compile_journal_path = None; | |
| let bytes = prost_tagged_bytes(&wire::build_record_to_wire(&record)); | |
| builds.insert(id, bytes.as_slice())?; | |
| updated += 1; | |
| let Some(paths) = record.log_paths.as_mut() else { | |
| continue; | |
| }; | |
| let had_archive_paths = paths.archived_session_log_path.is_some() | |
| || paths.archived_journal_path.is_some() | |
| || paths.archived_session_stats_path.is_some() | |
| || paths.archived_compile_journal_path.is_some(); | |
| if !had_archive_paths { | |
| continue; | |
| } | |
| paths.archived_session_log_path = None; | |
| paths.archived_journal_path = None; | |
| paths.archived_session_stats_path = None; | |
| paths.archived_compile_journal_path = None; | |
| let bytes = prost_tagged_bytes(&wire::build_record_to_wire(&record)); | |
| builds.insert(id, bytes.as_slice())?; | |
| updated += 1; |
| .env("SOLDR_DAEMON_SPAWN_RETRY_BUDGET_MS", "40000") | ||
| .env("SOLDR_COMPILE_REPLY_TIMEOUT_SECS", "60") | ||
| .env_remove("RUSTC_WRAPPER") | ||
| .env_remove("SOLDR_INTERNAL_DAEMON_EXE") |
There was a problem hiding this comment.
P3: The new standalone test hand-rolls the same env scrubbing already encapsulated in scrub_outer_soldr_runtime, hardcoding SOLDR_INTERNAL_DAEMON_EXE instead of the SOLDR_DAEMON_EXE_ENV_VAR constant. If the scrub list grows later, this path will silently diverge; reuse the helper (e.g. scrub_outer_soldr_runtime(&mut cmd); after the .env(...) chain and drop the manual removals).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/soldr-cli/tests/cli_daemon_lifecycle.rs, line 394:
<comment>The new standalone test hand-rolls the same env scrubbing already encapsulated in `scrub_outer_soldr_runtime`, hardcoding `SOLDR_INTERNAL_DAEMON_EXE` instead of the `SOLDR_DAEMON_EXE_ENV_VAR` constant. If the scrub list grows later, this path will silently diverge; reuse the helper (e.g. `scrub_outer_soldr_runtime(&mut cmd);` after the `.env(...)` chain and drop the manual removals).</comment>
<file context>
@@ -337,6 +347,98 @@ fn direct_recovery_accepts_slim_via_self_daemon() {
+ .env("SOLDR_DAEMON_SPAWN_RETRY_BUDGET_MS", "40000")
+ .env("SOLDR_COMPILE_REPLY_TIMEOUT_SECS", "60")
+ .env_remove("RUSTC_WRAPPER")
+ .env_remove("SOLDR_INTERNAL_DAEMON_EXE")
+ .env_remove("SOLDR_ORIGINAL_EXE")
+ .env_remove("SOLDR_RELOCATED_EXE");
</file context>
2 participants
CI mirror for upstream zackees/soldr#1768.
The upstream workflows require maintainer approval for this first-time fork contribution. This same-repository PR runs the fork's copy of the workflows against the identical commit while that approval is pending.
Summary by cubic
Preserves the caller’s Rustup context for host-owned Cargo/Rust tools while keeping managed homes for Soldr-managed binaries. Adds daemon-owned cache maintenance with generation-safe shutdown and structured flush reports, embeds Darwin DWARF (with soft‑skip
dsymutilprovisioning), upgradeszccacheto1.12.17, and fixes stable-Zthreadsfallback and rustlib‑integrityRUSTUP_HOMEhandling.Bug Fixes
CARGO_HOME.soldr-daemonhandoff, generation-safe shutdown;cache flushfails when the embedded daemon is unavailable instead of reporting a no‑op.llvm-dsymutiland soft‑skip when unavailable on non‑Darwin hosts.-Zthreadsfallback: detect nightly-only rejection, retry without the flag, preserve a user-requested no‑cache state, and replay the front‑door env contract.RUSTUP_HOME; classify randomizedautocfgprobes for readable logs.New Features
cache --json/status/doctor, and structuredflushresults;cache flush/saverequire a complete checkpoint.workflow_dispatch.zccacheto1.12.17.Written for commit df1090e. Summary will update on new commits.
Summary by CodeRabbit
gc maintain/gc hold-build-leasecommands.-Zthreadsretry handling and Darwin debug-symbol preservation.