feat(cc): detect cc-compatible compilers via -E preprocessing probe

[Ysh204]

Resolves #535.

kache currently recognizes C/C++ compilers using a hardcoded basename allowlist (cc, gcc, clang, etc.). Compiler wrappers and custom toolchains that expose a compatible compiler interface but use different executable names are not recognized and therefore cannot benefit from caching.

Solution

Add a dynamic fallback that detects the compiler family by running a lightweight preprocessor probe when basename-based detection fails.

The implementation:

• preserves the existing basename allowlist as the fast path;
• probes unknown compiler names using -E -P -x c -;
• detects Clang vs GNU by preprocessing a small source snippet containing __clang__ / __GNUC__ markers;
• reuses the existing probe cache under a dedicated cc-family prober without changing the cache schema;
• caches the detected compiler family to avoid repeated process invocations;
• avoids unnecessary probes for kache CLI subcommands and Rust workspace wrapper chains;
• falls back to the probed family in ToolFamily::detect() when name-based detection cannot classify the compiler.

Documentation has also been updated to describe dynamic detection for non-standard compiler wrappers.

Testing

Added tests covering:

• detection of the system C compiler;
• rejection of non-compiler executables;
• probe cache reuse;
• recognition of unknown compiler wrappers via a symlinked compiler;
• ToolFamily detection for dynamically probed compilers;
• verification that kache CLI subcommands do not trigger probing.

[emmanuelm41]

Thanks for taking on #535 — this is a real gap and the -E marker probe is a sound way to identify a compiler family. The #if defined(__clang__) / #elif defined(__GNUC__) ordering is exactly right (clang defines __GNUC__ too, so the order matters and you got it the right way round). One thing worth knowing that isn't in your description: this incidentally closes the zigcxx gap left open by #520, since zigcxx misses the name allowlist and falls through to your probe. That's a genuine win.

I'd like to settle two design questions before you put more work into this, because I don't think either is fixable by tweaking the current shape. I read the code and traced it statically; I did not build or run the branch.

1. The probe executes before kache's re-entrancy guard is installed.

detect_log_mode runs at src/main.rs:401, near the top of main, and calls detect_compiler at :300 — which now reaches CcCompiler::recognizes and can spawn the candidate binary. But KACHE_ACTIVE isn't checked until :605 and isn't set until :613, both inside run_wrapper_mode. The comment at :601 says what that guard is for: "looping kache → cc → kache → …".

So a wrapper that itself invokes kache can recurse during log-mode classification, before the thing that stops the loop has run. There's also no timeout, no output cap, and no negative memoization, so a candidate that hangs or writes unbounded stdout hangs the wrapper. kache ./some-script executes ./some-script just to classify it — and a script that ignores unknown flags gets run once to probe and once for real.

The narrow fix is to move probing after the guard is set and bound it (timeout + output limit + cache negatives). The broader point is that log-mode classification should probably stay pure and never execute anything.

2. What identity does a dynamically admitted wrapper have?

probe_key (src/probe/cache.rs:26-53) keys on the resolved path plus a stat fingerprint of the binary kache invoked. For a real compiler that's sound: upgrade it, the fingerprint moves, the probe reruns. For an indirection wrapper it isn't. If /tools/mycc delegates to /opt/toolchain/bin/cc and you upgrade the latter, /tools/mycc's fingerprint is unchanged, so the memoized --version and -### records are still served — and since the cc key hashes the program basename and those probe outputs, the object key can come out identical and a stale object gets served.

I want to be fair about this: the weakness already exists today if someone names a wrapper cc. The difference is that naming your wrapper cc is a deliberate "I am the compiler" claim, whereas this change silently extends the same trust to any binary that happens to preprocess C. That turns a narrow, opted-into sharp edge into the default for a whole class of tools.

This is a maintainer call rather than something you should just go implement — the options (fold the resolved underlying compiler into the identity, add a ccache-style compiler_check hook, or decline to cache wrappers whose compiler closure can't be identified) are all design decisions. @emmanuelm41, this one's yours.

Half the diff is currently inert. ToolFamily::dialect() maps Gnu | Clang => Dialect::Gnu (src/compiler/cc.rs:68), and .family has no consumer anywhere in the crate other than .dialect(). So the probe fallback inside ToolFamily::detect computes a value that cannot change any behavior. The upside is that it's also why there's no cache-key risk from the classification itself and no CACHE_KEY_VERSION bump needed. The downside is you're paying for it — I'd drop that hunk entirely and keep the recognizes() half, which is where the actual feature lives. If Clang and Gnu should diverge, that's a separate PR and would need a key bump.

Cost on the hot path. There are three family lookups per invocation: detect_log_mode → detect_compiler (main.rs:300), run_wrapper_mode → detect_compiler (main.rs:632), and CcArgs::parse → ToolFamily::detect. Each calls Config::load() before consulting the memo, so a warm hit still pays full config load, PATH resolution, a stat, a whole-environment hash, and a JSON read — three times. Worth noting probe_key includes the environment fingerprint, and run_wrapper_mode sets KACHE_ACTIVE, so the pre-guard and post-guard lookups land on different keys and both miss on a cold build. With no cross-process single-flight, a cold parallel build can stampede. (One thing that saves you: COMPILER_ADAPTERS is [rustc, cc] and .find() short-circuits, so rustc builds never reach this.)

Tests. family_probe_cached_result_roundtrips sets KACHE_CACHE_DIR process-globally with unsafe set_var and no lock, while Rust runs tests in parallel. The repo already has config_path_lock() for exactly this, used by six-plus tests; no existing test mutates KACHE_CACHE_DIR at all. It also asserts files.len() == 1 in a directory other tests can write to, removes the variable rather than restoring it, and isn't panic-safe. That's a flake waiting to happen in CI.

Two others pass without testing anything: family_probe_detects_system_cc and recognizes_unknown_wrapper_via_probe both return early when no compiler is found, and the latter's ToolFamily assertion accepts either Gnu or Clang — so deleting the family-detection code entirely still leaves it green. A deterministic fake compiler script with an invocation counter would test the argv/stdin protocol and let you assert how many times the probe ran, which is the property that actually matters here.

Docs. The README's ccache example doesn't work: ccache's direct mode is ccache <compiler> [options], so ccache -E -P -x c - puts -E in the compiler position and the probe fails. (A ccache symlink named gcc is already handled by the allowlist, not by this path.) "Any other compiler or wrapper names" in c-cpp.mdx is also broader than what's delivered — an unknown clang-cl wrapper can't be detected, since ProbedFamily has no ClangCl and a GNU-style probe can't recover MSVC driver mode. zigcxx is your strongest real example; I'd lead with it.

src/link.rs — no action needed, just rebase. Those four wording changes are already on main as aeb53e0; they'll disappear once you're up to date.

Sorry to hand back a long list on a solid idea. The recognizes() probe is the right core and I'd like to see it land — I just don't want you polishing tests and docs around a shape that may need to change once the two design questions above are answered.

(AI-assisted review — a maintainer still owns the call.)

[jleni]

Summary

PR #538 adds a dynamic -E -P -x c - preprocessor probe so unknown compiler basenames (custom wrappers, triple-prefixed drivers, etc.) can be recognized as C-family compilers and classified as Clang vs GNU. The approach is sound in principle: marker-based #if defined(__clang__) / __GNUC__ order

Issue counts by severity

• bugs: 4
• suggestions: 3
• nits: 2

Issues outside the diff

These findings reference lines that are not present in the diff and could not be posted as inline comments:

• [suggestion] src/probe/mod.rs:301 — Test coverage gaps relative to the risk surface: (1) no test that the probe distinguishes real clang vs real gcc when both exist; (2) no hermetic fixture that a non-compiler accepting -E and exiting 0 without markers returns None; (3) no coverage for triple-prefixed names (x86_64-linux-gnu-gcc) that this PR is meant to unlock; (4) no MSVC cl.exe / clang-cl Windows cases; (5) family_probe_returns_none_for_non_compiler uses ambient cargo (spawn-dependent); (6) family_probe_cached_result_roundtrips mutates process-global KACHE_CACHE_DIR via unsafe set_var, which is flake-prone under parallel cargo test; (7) recognizes_unknown_wrapper_via_probe is a good integration check but skips entirely when no system compiler is present (CI without a C toolchain learns nothing).
  • Suggestion: Add hermetic shell/exe fixtures for compiler-like and non-compiler-like programs; assert clang vs gnu with controlled macros if possible; cover triple-prefix recognition; avoid global env mutation (pass cache_dir into the probe API); run at least one non-skipping unit test of run_family_probe parsing with a fake stdout fixture by factoring pure parse logic.
• [nit] src/compiler/cc.rs:79 — Doc comment on ToolFamily::detect still states that everything other than clang/clang-cl/zigcc is Gnu, but the new code can return Clang for arbitrary basenames after probing. Stale docs will mislead the next change.
  • Suggestion: Update the doc to describe the three-stage algorithm: cl-mode → clang-name → optional -E probe → default Gnu.

[jleni]

[bug] ToolFamily::detect inserts the -E probe after the clang-name fast path and before the historical default of ToolFamily::Gnu. That means well-known names cc, c++, gcc, g++ (and any other non-clang basename) no longer short-circuit to Gnu; they always call probe_compiler_family. On macOS/Linux where cc (or even gcc) is Apple/LLVM clang, the probe returns Clang instead of the previous hard-coded Gnu. Existing tests pin the old contract (tool_family_detects_clang_cl_and_dialects asserts f("/usr/bin/cc", &[]) and f("gcc", &[]) are Gnu). Dialect is currently shared (Gnu and Clang both use Dialect::Gnu), so object caching may still work, but this is an unintended behavior change, adds a process/cache round-trip on every gcc/cc parse (via Config::load + probe keying), and contradicts the function’s own doc comment (“Everything else (gcc, cc, g++, c++) is Gnu”).

Suggestion: Only probe when the basename is not on the known allowlist. Restore an explicit early return ToolFamily::Gnu for cc/c++/gcc/g++ (and versioned forms), and call probe_compiler_family only for truly unknown stems (the same set that falls through recognizes()’s slow path). Keep the probe out of the hot path for canonical names.

[jleni]

[bug] After name allowlist misses, CcCompiler::recognizes runs probe_compiler_family(arg0) for any absolute path, or any bare name that resolves on PATH. That probe is reachable from detect_compiler → detect_log_mode on every kache startup whose argv[1] is not an exact allowlisted compiler and not a clap subcommand. Negative results are not stored (only successful families are written to the probe cache), so a non-compiler on PATH (make, ninja, cmake, python, ccache, …) is re-spawned with -E -P -x c - on every such invocation. Besides latency, this can have side effects: e.g. GNU make implements -E/--eval, so the probe args are not necessarily a clean no-op. Tests like recognizes_rejects_non_c_compilers and detect_compiler_returns_none_for_unrelated_argv still expect false/None for make/ccache/cargo, but they now depend on those tools failing the probe rather than on name rejection alone.

Suggestion: (1) Do not probe from detect_log_mode’s cheap classification path if avoidable—keep name allowlist for log-mode, or gate the slow path behind wrapper-mode only. (2) Memoize negative results (version_line: "none" / dedicated sentinel) so a given binary is not re-executed every process. (3) Consider requiring a path separator or an explicit “looks like a compiler” heuristic before probing bare PATH names. (4) Update recognizes_rejects_non_c_compilers to use a hermetic non-compiler fixture instead of ambient PATH tools.

[jleni]

[bug] On cache hit, probe_compiler_family returns parse_family(&hit.version_line) immediately. If version_line is anything other than "clang"/"gnu" (corruption, hand-edited cache, future format drift, empty string), parse_family yields None and the function never re-runs run_family_probe. For recognizes(), that permanently treats a real compiler as unrecognized until the binary fingerprint (mtime/ctime/inode) changes. For ToolFamily::detect, None falls through to ToolFamily::Gnu, masking the failure with a possibly wrong family. This differs from the main probe() path, where a usable record is either fully returned or the prober re-runs on miss—here a “hit with bad payload” is a silent sticky failure.

Suggestion: On cache hit, if parse_family fails (or hit.prober != "cc-family"), treat as miss and re-probe (and overwrite the record). Optionally store negatives under an explicit sentinel and only short-circuit on known values.

[jleni]

[bug] ProbedFamily only has Gnu and Clang. A renamed clang-cl (or any MSVC-driver clang whose basename is not exactly clang-cl) that still accepts the GNU-style probe will be classified as ToolFamily::Clang (GNU dialect), not ClangCl, unless argv also contains --driver-mode=cl. That matters on Windows: dialect selects flag parsing and windows_aware for the -### probe (windows_aware: family.dialect() != Dialect::Cl). Mis-classifying clang-cl as Clang can change path blanking / cache-key portability for objects that still embed native paths—the opposite of the #299/#312 clang-cl rules. Real MSVC cl.exe correctly fails the GNU -E -P -x c - probe (out of scope), but the renamed clang-cl case is a false-positive recognition with wrong dialect.

Suggestion: Document that probe-based detection never yields ClangCl and requires --driver-mode=cl (or the clang-cl basename) for MSVC-driver mode. Optionally extend the probe (e.g. also try a cl-style /E snippet, or sniff --driver-mode / target triple) if non-standard clang-cl names are a supported goal. Add a Windows-focused test that a path ending in clang-cl still wins over probe results, and that a custom-named binary without driver-mode does not silently get Cl dialect wrong—or is refused.

[jleni]

[suggestion] Only successful probe outcomes are written to the cache. Combined with Issue 2, any argv0 that is executable but not a C compiler pays a full spawn on every kache process. Family detection itself also re-hashes the entire process environment into the probe key (cache::probe_key → env_fingerprint), even though __clang__/__GNUC__ predefined macros do not depend on SDKROOT/CPATH/etc. That causes unnecessary cache fragmentation and re-probes across env-changing build steps.

Suggestion: Cache negatives; for the cc-family prober consider a narrower key (binary fingerprint + prober id only, no full env / empty key_args is already empty). Reuse the caller’s already-loaded cache_dir instead of calling Config::load() inside probe_compiler_family on every detect/recognize.

[jleni]

[suggestion] Docs say unrecognized names “such as custom scripts, ccache, or cargo-zigbuild shims” are detected via -E probing. Bare ccache is still in recognizes_rejects_non_c_compilers as not a compiler, and a real ccache argv0 will fail the probe (ccache expects ccache <compiler> …). cargo-zigbuild’s zigcc-* names were already on the allowlist. The docs oversell what the probe does for nested wrappers (kache ccache gcc still has argv0 ccache).

Suggestion: Rephrase to “custom compiler wrappers whose executable itself accepts the usual preprocessor CLI (forwards -E and defines __clang__/__GNUC__)”, and give a concrete example (symlink/copy of gcc, or a thin shim). Mention that nested multi-token wrappers are not expanded.

[jleni]

[nit] Unrelated wording changes (“share storage” → “share storage blocks”) from #533 are bundled into this feature PR. They obscure the review surface of the compiler-detection change and are not covered by the PR title/testing notes.

Suggestion: Split drive-by warning-copy edits into their own commit/PR, or call them out explicitly in the PR body so reviewers do not treat them as part of the probe design.

[jleni]

Follow-up to my review: main has moved since this branch was cut. #566 now recognizes target-prefixed cross compilers (arm-linux-gnueabihf-gcc and friends) through a static allowlist and rewrote ToolFamily::detect, and #550 added CUDA refusal and nvcc passthrough in the same file. So the triple-prefix motivation is mostly covered on main already, and the probe's remaining value is genuinely unknown wrapper names that the allowlist cannot know about. After a rebase, the earlier point about probing before the Gnu default should be re-read against named_tool_family: canonical and target-prefixed names should resolve from the allowlist without ever spawning the probe. The link.rs wording edits also overlap lines #548 touched, so they will conflict; probably easiest to drop them from this PR.

[emmanuelm41]

Thanks for the thorough rework — dropping the ToolFamily::detect half and the link.rs diffs is exactly right, and the corrupt-cache-recovery fall-through reads cleanly.

The KACHE_ACTIVE guard, though, breaks the feature in the top-level process. run_wrapper_mode sets KACHE_ACTIVE=1 at src/main.rs:613, then calls detect_compiler at :632. So on the dispatch call, recognizes() hits your new guard and returns false — the unknown wrapper is recognized during detect_log_mode (main.rs:401, before the var is set) but rejected at actual dispatch, and falls through to run_compiler_directly. Net effect: the wrapper this PR exists to cache runs uncached.

I proved it by stubbing the probe to succeed and calling recognizes() twice: log_mode(no KACHE_ACTIVE)=true, dispatch(KACHE_ACTIVE set)=false. The new test recognizes_unknown_wrapper_via_probe doesn't catch it because it only makes the log-mode-style call, with KACHE_ACTIVE unset.

The underlying issue is that KACHE_ACTIVE answers "am I a nested kache?" but you're reading it after this same process sets it. Recursion across processes is already handled by the existing guard at main.rs:605 (a nested kache exits via run_compiler_directly before dispatch). What your guard is really trying to prevent is the nested process probing during its own detect_log_mode — so it needs to distinguish "inherited from a parent" from "I just set it myself." One option: capture whether KACHE_ACTIVE was set at process entry (before :613) and gate the probe on that, rather than reading the live var.

Two smaller things:

• is_workspace_wrapper_chain(args) is called twice in recognizes() — once at the top (cc.rs:3647) and again in the slow-path block. The first returns early, so the second is dead code.
• Heads-up on macOS: recognizes_unknown_wrapper_via_probe fails on my macOS box because Apple's cc re-dispatches on argv[0] (xcodebuild -find my-custom-compiler), so it can't be probed under a renamed symlink. The test doesn't skip that case, so it'll go red on a macOS just check. Worth gating to non-Darwin or using a compiler that tolerates renaming.

The unaddressed design items (wrapper-identity staleness, probe timeout/output cap, negative caching for non-compilers) I'm leaving for @jleni's call — you flagged them as open questions and I agree that's where they belong.

I read the code and reproduced the guard behaviour locally; I didn't run a full Linux build.

(AI-assisted review — a maintainer still owns the call.)

[Ysh204]

I've updated the PR (f2dfda6):

1. KACHE_ACTIVE Guard Removal: Removed KACHE_ACTIVE checking from CcCompiler::recognizes(). As you noted, KACHE_ACTIVE=1 is set by run_wrapper_mode in src/main.rs before detect_compiler runs during wrapper dispatch, so checking it in recognizes() was causing dispatch to reject the compiler. Process-level recursion is already handled at src/main.rs:605.
2. Dead Code Cleanup: Removed the duplicate is_workspace_wrapper_chain call in recognizes().
3. macOS Test Fix: Added a macOS guard to recognizes_unknown_wrapper_via_probe to avoid Apple's xcode-select symlink argv[0] re-dispatch error.
4. Test Coverage: Expanded recognizes_unknown_wrapper_via_probe to test recognizes() both with KACHE_ACTIVE unset and with KACHE_ACTIVE=1 set.

[emmanuelm41]

Thanks for the fast turnaround. Removing the guard does fix the dispatch rejection — I confirmed an unknown wrapper is now recognized at both the log-mode call and the post-KACHE_ACTIVE dispatch call. The dead is_workspace_wrapper_chain is gone and the macOS test guard is a good touch.

New blocker, though — and it's the reason the guard existed: removing it reopens an unbounded fork bomb. The probe fires in detect_log_mode (main.rs:401), which runs before run_wrapper_mode sets KACHE_ACTIVE and before the recursion guard at main.rs:605. So a wrapper that re-invokes kache during preprocessing never reaches that guard. I reproduced it with a self-referential wrapper (wrapper → kache wrapper -E -P -x c -), capped at 5 levels for safety: the counter climbed 0→5, each kache spawning the wrapper which spawned the next kache. Uncapped it's a linear process-chain exhaustion. It also triggers with two mutually-referential unknown names, and can start even with KACHE_ACTIVE unset.

Gating on KACHE_ACTIVE (live or at-entry) can't fix this cleanly, because the chain runs before that var is set. What works is a dedicated breadcrumb on the probe's own child: set an env marker when spawning the probe, and have probe_compiler_family return None when it sees it. I applied exactly that and re-ran the repro — the counter stops at 1. Minimal shape:

// probe_compiler_family(): fail closed if we're already inside a probe
if std::env::var_os("KACHE_FAMILY_PROBE_ACTIVE").is_some() { return None; }
// run_family_probe(): mark the child
Command::new(program).args(["-E","-P","-x","c","-"]).env("KACHE_FAMILY_PROBE_ACTIVE","1")

Two more things worth folding in:

• No timeout / output cap on the probe. wait_with_output() (probe/mod.rs:263) can block forever, and it buffers unbounded stdout. Since this runs during mode detection for any unknown PATH-resolvable arg0, a hanging or chatty candidate hangs the wrapper. I'd treat a bounded wait + output cap as pre-merge.
• The probe runs twice per build. probe_key hashes the whole environment, and KACHE_ACTIVE differs between the log-mode call and the dispatch call, so they compute different keys and the dispatch call misses the record the log-mode call just wrote. Excluding KACHE_ACTIVE (and the breadcrumb above) from the probe fingerprint fixes both the double-spawn and keeps the memo warm.

Smaller notes:

• The src/daemon.rs PowerShell change is unrelated to compiler detection — it's a separate tip commit and would be cleaner as its own PR.
• The wrapper-identity staleness point from last round is still open (an opaque wrapper whose delegated compiler is upgraded in place keeps its cached identity). That's a maintainer-scope call, but since this PR advertises arbitrary custom scripts as cacheable, it's worth an explicit decision rather than silent deferral. @jleni.

I read the code and reproduced the recursion and its fix locally on Linux/macOS; I didn't run the full CI suite.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

Good progress — this revision genuinely fixes the fork bomb, and I confirmed it: the KACHE_FAMILY_PROBE_ACTIVE breadcrumb stops a self-referential wrapper cold (my repro drove the recursion counter to 1 instead of climbing). Dropping the daemon.rs change and adding the fingerprint de-dup are both the right calls. Thank you for the thorough turnaround.

The new timeout, though, isn't actually a hard bound — I could drive kache past it to a full hang. Two paths:

1. A descendant holding stdout defeats the 2s deadline. I probed a wrapper that spawns a child and the child inherits stdout (sh -c 'sleep 60'): 12s wall (killed by my outer timeout), versus 2s when the candidate is exec'd directly. Cause: on timeout you child.kill() the direct PID only, then read_thread.join() blocks — the grandchild still holds the pipe write end, so the reader never sees EOF. And probed wrappers spawn children by nature (ccache→cc, cargo-zigbuild→zig), so this is the common shape, not a corner case.
2. The 8 KB cap path never even consults the timeout. Once the reader hits 8192 bytes it returns immediately, then you call an unbounded child.wait(). A candidate that writes 8 KB, keeps stdout open, and sleeps forever hangs there regardless of the 2s.

Both hang kache during detect_log_mode for any unknown PATH-resolvable argv[0], which is a bad place to hang. The fix is to own the whole process tree and bound every wait against one deadline: spawn in a new process group (Unix) / Job Object (Windows) and kill the group; never join()/wait() unbounded before you've established termination. (recv_timeout on the reader isn't enough on its own.)

Two more while you're in there:

• The probe ignores exit status. A candidate that prints KACHE_PROBE_GNU then exit 1 is accepted as a GNU compiler. Requiring status.success() is a cheap guard.
• 2 seconds is likely too tight, and the failure mode is worse than "uncached." A cold toolchain, a network-mounted compiler, or a loaded CI worker can exceed it — and because this runs in detect_log_mode, a None here can misclassify the invocation (CLI/Clap path or "no adapter matched") and fail the build rather than just skip caching. Worth a more generous, configurable deadline, and ideally distinguishing "probe timed out" from "definitely not a compiler" so only the latter declines caching.

Smaller notes, not blocking:

• The breadcrumb is only checked inside probe_compiler_family, so it guards the unknown-name slow path but not a nested kache that resolves a known name. Checking it once at main() entry (bail if set) would make it a hard boundary regardless of path.
• The fingerprint de-dup is correct as long as no wrapper branches on KACHE_ACTIVE; the cleaner long-term shape is to run the probe only after KACHE_ACTIVE is set (in wrapper mode), which removes the duplicate probe and the need to filter the var.
• The recursion test sets the marker in-process and calls the function directly — it doesn't spawn a wrapper or exercise inheritance/timeout. A couple of real-process tests (self-referential wrapper under an outer deadline; a candidate that emits 8 KB then sleeps; one that leaves a descendant on stdout) would lock all of this down.

And the wrapper-identity staleness from last round is still open — I'll leave that for @jleni, but flagging that it's a genuine cache-correctness risk (a script whose downstream compiler is upgraded keeps its cached identity), not just a nicety, given the PR advertises arbitrary custom scripts.

I read the code and reproduced the recursion fix and both timeout hangs locally on macOS; I didn't build a full Linux/Windows run.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

This is real progress on the hard part — I verified the timeout hangs are genuinely fixed now. Spawning the probe in its own process group (process_group(0)) and killing -pid bounds both cases I could hang before: the grandchild-holds-stdout wrapper now returns at ~5s instead of hanging, the 8KB-then-sleep case is bounded, the descendant is actually reaped (I checked — no leaked sleep), and detaching the reader thread instead of joining it is the right call. Exit status is checked, the breadcrumb is now a hard boundary at main() entry, and there are real-process tests. Nice work.

A few things before it's mergeable, though — starting with two that fail CI outright:

1. cargo test doesn't compile. parse_family was inlined into the match hit.version_line.as_str() block (src/probe/mod.rs:190), but the unit tests still call it — probe/mod.rs:532-535 and :592 give error[E0425]: cannot find function parse_family. The just check/cargo test gate will fail. Either restore a small parse_family helper or update those tests to the new inline form. (cargo build and the integration-test crate don't surface this because neither compiles the bin's #[cfg(test)] module — cargo test --bin kache --no-run does.)

2. cargo fmt --check will fail — trailing whitespace at tests/unknown_compiler_probe_test.rs:16, 40, 66 (git diff --check flags them).

3. A transient timeout becomes a permanent mis-detection. This is the one I'd most want fixed. A None from run_family_probe — including a 5s timeout — is cached as "none" (:193, :204) under the wrapper's stat key. So a wrapper that's merely slow once (cold toolchain, network FS, loaded CI, AV scan) gets cached as "not a compiler," and every later build fails fast — detect_log_mode falls to LogMode::Cli and Clap rejects the compiler path. A one-time slowness turns into a sticky build failure until the fingerprint changes. Timeouts / spawn / IO errors should be treated as inconclusive (pass through uncached, don't persist), distinct from a definitive "probed and it's not a compiler." A fixed 5s is fine once inconclusive ≠ negative.

Smaller, non-blocking:

• The cap test's printf 'A%.0s' {1..9000} is Bash brace expansion under #!/bin/sh. On Ubuntu's Dash (the Linux CI shell) {1..9000} is literal, so it emits one byte and the test silently exercises only the timeout path, not the cap. head -c 9000 /dev/zero is portable.
• The descendant test asserts only that kache returned in time, not that the descendant died — making kill_process_group a no-op would still leave it green. Recording the descendant PID and asserting it's gone would give it teeth (I confirmed the kill works today, but the test doesn't lock it in).
• Windows kill_process_group shells out to taskkill /F /T via .output(), which is itself an unbounded synchronous wait — so the 5s bound can spill into an unbounded taskkill on Windows, and there's no Windows test. A Job Object would be the hard-bounded equivalent, but I couldn't exercise any of this from macOS, so treat it as a flag rather than a proven defect.

And wrapper-identity staleness is still open (@jleni) — the probe key fingerprints the wrapper file, not the compiler it ultimately calls; the new "none" caching makes a stale entry a bit stickier, but it's a separate issue.

I read the code, reproduced the timeout fixes and the compile/fmt failures locally on macOS, and cross-checked the control flow; I didn't run Linux or Windows CI.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

This revision clears everything actionable from the last round, and I verified each locally:

• Compiles / fmt clean. parse_family is restored (cargo test --bin kache builds), cargo fmt --check passes, and the cap test now uses head -c 9000 /dev/zero instead of the Bash-only {1..9000}, so it exercises the cap on Dash too.
• The transient-timeout concern is fixed — nicely. run_family_probe now returns a Result, and probe_compiler_family distinguishes Ok(None) (definitively not a compiler → cached as "none") from Err(_) (transient → return None without caching). I confirmed the behavior: a wrapper that sleeps past the 5s deadline times out and is not cached — a second probe re-runs rather than being permanently declined — while a fast non-compiler is cached. That's exactly the inconclusive-vs-negative split I was hoping for.
• Timeout still bounded. The grandchild-holds-stdout case returns in ~2s and the descendant is reaped (no leak); recursion still stops at one level.

One new thing, and it's the only thing I'd ask you to fix before this is mergeable:

The new tests make the probe suite flaky under parallel cargo test. On main, cargo test --bin kache probe is clean across repeated runs; on this branch it intermittently fails probe::tests::probe_runs_prober_once_then_serves_from_cache (I saw ~1 in 3). The cause is recognizes_unknown_wrapper_via_probe calling std::env::set_var("KACHE_ACTIVE", ...) / remove_var without taking config::config_path_lock() — your KACHE_CACHE_DIR test in the same file already uses that lock, so the pattern's right there. Since CI runs the suite in parallel, this will surface as random red runs. Wrapping the KACHE_ACTIVE mutation in the shared lock (and restoring the previous value on drop) should settle it.

Two things I'm carrying forward rather than blocking on:

• Windows kill_process_group shells out to taskkill /F /T via .output(), itself an unbounded synchronous wait — so the 5s bound can still spill on Windows. I couldn't exercise it from macOS, so I'm flagging it, not asserting it; a Job Object would be the hard-bounded equivalent if you want to close it.
• Wrapper-identity staleness (@jleni's call) is unchanged — the probe key fingerprints the wrapper file, not the compiler it delegates to.

Genuinely good iteration on this — the timeout/recursion machinery is in solid shape now. I read the code and reproduced the fixes and the flake locally on macOS; I didn't run Linux/Windows CI.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

Thanks — locking the KACHE_ACTIVE/KACHE_CACHE_DIR mutations with config_path_lock and restoring the prior value is the right direction. But the flake isn't gone: running cargo test --bin kache probe a few times still fails probe_runs_prober_once_then_serves_from_cache (I saw it ~2 in 5), with:

assertion `left == right` failed: second probe must be served from the on-disk cache
  left: 2
 right: 1

The prober ran twice — the second probe missed the cache. The reason the lock didn't fix it: this victim test (src/probe/mod.rs:432) is a reader, not a mutator, so it never takes config_path_lock. It runs two probes and asserts the second is a cache hit — but probe_key folds the entire process environment into the key (env_fingerprint). When one of the new env-mutating tests sets or clears KACHE_ACTIVE / KACHE_CACHE_DIR / KACHE_FAMILY_PROBE_ACTIVE between this test's two probes, the env fingerprint changes, the second probe computes a different key, misses, and re-runs the prober → runs == 2.

So locking only the mutators isn't enough — any test that asserts a probe cache hit/miss is also env-sensitive and has to be serialized against them. The fix is to have probe_runs_prober_once_then_serves_from_cache (and any sibling that asserts run counts across probes) take the same config_path_lock for its duration, so no env mutation can land between its two probes. Alternatively, exclude the test-only vars from env_fingerprint, but the lock is the smaller change and matches what the rest of the file does.

Everything else from the last round still checks out — compiles, fmt clean, the timeout/recursion fixes hold, and the transient-timeout-not-cached behavior is correct. This flake is the only thing left, and it's a test-isolation issue, not a problem with the feature itself.

I reproduced this locally on macOS (repeated cargo test --bin kache probe); it's intermittent, so a single green run won't disprove it.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

The collapsible_if at tests/unknown_compiler_probe_test.rs:101 is collapsed now, thanks. But two things still stand — I'd hold on merge:

The flake isn't fixed. Adding config_path_lock() to probe_runs_prober_once_then_serves_from_cache didn't do it — running cargo test --bin kache probe repeatedly still fails ~1 in 6, and when it does it takes down five tests at once, including the one that got the lock:

probe_runs_prober_once_then_serves_from_cache
family_probe_cached_result_roundtrips
family_probe_detects_system_cc
probe_falls_back_to_running_when_compiler_is_unresolvable
family_probe_returns_none_for_non_compiler

The reason a single lock can't fix it: probe_key folds the whole process environment into the key (env_fingerprint), so any test that mutates any env var races with any test that asserts a probe cache hit — the probe before the mutation gets a different key than the probe after. A per-test lock only helps if every env-mutating and every cache-asserting probe test takes the same lock, and the new family_probe_* tests don't, so the race just moves around the suite. Two ways out that actually converge:

• exclude the kache-internal/test vars (KACHE_ACTIVE, KACHE_FAMILY_PROBE_ACTIVE, and arguably KACHE_CACHE_DIR) from env_fingerprint, so a concurrent mutation can't flip an unrelated probe's key; or
• give the probe-cache tests an explicit cache dir + fixed env and stop routing them through process-global env at all.

This is worth getting right because it's not just test noise — it says the on-disk probe cache silently misses whenever the ambient environment differs by any variable, which is a real property of the feature, not only the tests.

Clippy — please lean on CI, not a local run, for this branch. The repo pins Rust 1.95, but clippy lints drift by version and the branch is behind main; running clippy locally on a mismatched toolchain flagged issues in remote_backend.rs and wrapper.rs, which this PR doesn't touch — pure staleness artifacts. So I can't give you a clean local clippy verdict; the CI run on the matching toolchain is the authority. A rebase onto current main would also clear the unrelated-file noise.

Everything else from the prior rounds still holds — the timeout/recursion machinery and the transient-timeout-not-cached behavior are solid. It's really just the test-isolation flake left (plus letting CI confirm clippy).

I reproduced the flake locally (repeated cargo test --bin kache probe, ~1/6); the clippy situation I could only observe through the toolchain mismatch, which is why I'm deferring it to CI.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

The flake is fixed 🎉 — and the right way. Excluding KACHE_CACHE_DIR/KACHE_CONFIG from the probe fingerprint (alongside KACHE_ACTIVE/KACHE_FAMILY_PROBE_ACTIVE) is the systemic fix: those are kache's own bookkeeping, not compiler configuration, so a parallel test mutating them can no longer flip an unrelated probe's cache key. I hammered cargo test --bin kache probe 10× in a row and it's clean every time (it was failing ~1 in 6 before). fmt is clean too, and your changed files are clippy-clean.

One thing left, and it's not really about your code: CI's cargo clippy --workspace --all-targets will still trip, but on remote_backend.rs:191 and wrapper.rs:2593 — files this PR doesn't touch. That's branch drift (you're a good way behind main, and current clippy flags older versions of those files). A rebase onto main should clear it and get CI honest. Same note @jleni left on #592.

So: flake resolved and verified, code clean — a rebase is the last step to green CI. Nice work chasing this one down.

I re-ran the probe suite and clippy locally on macOS; the clippy toolchain differs a little from CI's, so treat CI as the final word there.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

Ran CI on the rebased head — good news, the rebase cleared the earlier clippy noise (those were untouched files behind main), and the flake fix holds. It's red on one thing only: fmt-check. Three spots rustfmt wants rewrapped:

Diff in src/compiler/cc.rs:4905
Diff in src/probe/mod.rs:750
Diff in src/probe/mod.rs:779

A plain cargo fmt (or just fmt) fixes all three, and then Check (Linux) goes green. That should be the last thing standing between this and mergeable — everything else (the systemic env-fingerprint fix for the flake, the timeout/recursion machinery) checked out. Thanks for sticking with it!

(AI-assisted note — a maintainer still owns the merge.)

[Ysh204]

i hope this finally works, lmao

[emmanuelm41]

Thanks — and the instinct here is right: .cmd/.bat wrappers are common for CC/CXX on Windows, so probing them matters. The fmt fix is clean too. But this revision isn't green yet, and the batch handling has a safer (and simpler) shape. Two blockers:

1. The cmd.exe construction is a command-injection surface — and it's also unnecessary. The batch path builds format!("\"{program}\" -E -P -x c -") and hands it to cmd.exe /C. cmd doesn't follow normal argv escaping, so a program path containing &, |, ^, %VAR%, or a stray " can compose or inject commands — and it'd run during classification, even if no compiler exists at that path. The important part: you don't need to build this string at all. std::process::Command::new(program) already routes .bat/.cmd through cmd on Windows and escapes arguments safely — that's the CVE-2024-24576 fix from Rust 1.77.2, and this repo is on 1.95. So the whole #[cfg(windows)] branch can go away; plain Command::new(program).args(["-E","-P","-x","c","-"]) handles batch wrappers correctly and safely on every platform. (I confirmed the timeout tree-kill and the KACHE_FAMILY_PROBE_ACTIVE breadcrumb still work through cmd for a normal synchronous wrapper, so nothing else depends on the manual routing.)

2. Check (Linux) will fail on clippy. The recognizes() change un-collapsed the && chain into nested if / if / if, which trips clippy::collapsible_if under -D warnings (I ran it — cc.rs:3744). So the fmt fix traded one gate failure for another. Reverting that block to the original !a && !b && c form clears it (and there's no behavior change either way — find('/').is_none() == !contains('/')).

Deleting the Windows branch fixes #1 and, since the nested-if was the only new clippy hit, reverting it fixes #2 — after that, just check should be green.

One test note, not blocking: the .cmd test uses a benign filename, so it wouldn't have caught the injection. A wrapper name with a space and an & in it is the regression case worth adding — with Command::new(program) it must still invoke the intended wrapper; with the string form it parses as command syntax.

The loop restructures (reader + timeout) I checked — behaviorally identical, no bug, just some churn.

I verified the clippy failure and the equivalence locally on macOS; the Windows cmd/injection behavior I'm going off the docs + the stdlib's batch handling, not a Windows run.

(AI-assisted review — a maintainer still owns the call.)

[emmanuelm41]

CI ran, and it's down to one Windows failure — but the good news is the native-Command switch was the right call, and the remaining issue is narrow and fixable.

Test (Windows) fails only on recognizes_unknown_wrapper_via_probe (cc.rs:4872, recognizes() returned false). Every other batch-probe test passes on Windows, which pins the cause precisely:

• run_family_probe_handles_large_output (a self-contained .cmd that echoes the marker) — passes, so Command::new(program) launches and reads .cmd files fine. Deleting the manual cmd.exe branch was correct.
• family_probe_detects_system_cc (a real compiler probed directly, via stdin) — passes, so the real compiler emits the marker from stdin when invoked directly.
• recognizes_unknown_wrapper_via_probe is the only one that does both at once: a .cmd that wraps a real compiler and forwards %*, with the C snippet piped to stdin. That's the failing combination.

So the root cause isn't the routing — it's the stdin. When Command runs a .cmd, it goes through cmd.exe /c, and the piped stdin reaches cmd.exe but doesn't make it down to the compiler the batch actually invokes. Direct compiler = stdin works; self-contained .cmd = no stdin needed; .cmd-wraps-compiler + stdin = the snippet never reaches the compiler, no marker, None.

The clean fix is to stop feeding the probe source on stdin: write the little #if defined(__clang__)… snippet to a temp file and pass the path instead of -, e.g. -E -P -x c <tempfile>. No stdin forwarding needed, so it works for a direct compiler and a .cmd/.bat wrapper — which is the case this feature is actually for. The tempfile crate handles cleanup. (If you'd rather not, the alternative is to accept that stdin-piped probing can't see through a batch wrapper and scope the docs accordingly — but the temp-file route makes the feature genuinely work.)

fmt and clippy are both green now, so this stdin/temp-file change should be the last thing. I diagnosed this from the CI log's pass/fail pattern rather than a live Windows run, so treat it as a strong hypothesis worth confirming when you push — but the evidence lines up cleanly.

(AI-assisted review — a maintainer still owns the call.)