Conversation
`#[prebindgen]` named its capture file `{group}_{pid}_{thread}.jsonl`, so
every rustc process wrote a fresh one. `build.rs` wipes the directory, but
only when Cargo re-runs the build script — plain `cargo check` / `build` /
`test` / `clippy` cycles re-run rustc against an untouched directory, so the
files piled up forever (a full copy of the crate's captured items each time).
Key the file on the compilation unit instead: rustc's own `-C metadata` hash,
which is stable across rebuilds of a unit and distinct between units built
concurrently (lib vs. test). rustdoc, which compiles doctests without
`-C metadata`, falls back to a hash of its command line. The file is reset
once per process, on the first record written, so a rebuild replaces the
unit's records rather than appending them again.
Since threads of one rustc now share a file, write each batch of records with
a single `write_all` — a line split across writes could interleave.
Keep each capture file's first reset and every append under one per-file lock, so a concurrent macro expansion cannot have its record unlinked. Propagate reset and lock failures as compiler diagnostics and cover the race with synchronized writers.\n\nAlso parse rustc unit metadata from OS-native arguments, accept every codegen-option spelling, and drop the no-longer-needed rand dependency.
| }) | ||
| .rfind(|metadata| !metadata.is_empty()) | ||
| { | ||
| return metadata.to_string(); |
There was a problem hiding this comment.
The parsed metadata must be encoded or hashed before it becomes a path component. rustc accepts path separators in this value: both a direct rustc -Cmetadata=foo/bar probe and the real consumer invocation
cargo rustc -p example-flat -- -Cmetadata=foo/bar
accept the compiler option. At this line, however, foo/bar is returned verbatim, so get_prebindgen_jsonl_path produces .../prebindgen/default_foo/bar.jsonl. The example-flat build then fails all 47 annotated items with failed to append .../default_foo/bar.jsonl: Failed to open file. A backslash has the analogous risk on Windows.
Please hash the extracted metadata (as the fallback already does), or otherwise turn it into a platform-independent single filename component, and cover a separator-containing value in unit_id_from_args tests.
Other checks at 574a98b passed: focused proc-macro concurrency/parser tests, focused JSONL tests, three repeated check/build/test/clippy/doc lifecycle rounds (the warmed file set stayed fixed and every capture held exactly 44 or 3 records), cargo test --all --all-features, strict all-target/all-feature Clippy, rustfmt, diff-check, and examples/regen-check.sh.
— Codex (GPT-5)
There was a problem hiding this comment.
Pull request overview
Bounds JSONL capture growth by keying files to compilation units and synchronizing writes.
Changes:
- Derives hashed capture IDs from rustc/rustdoc arguments.
- Resets and locks captures before atomic batch appends.
- Adds concurrency/error tests and removes
rand.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
prebindgen/src/api/utils/jsonl.rs |
Serializes batches before appending. |
prebindgen-proc-macro/src/lib.rs |
Implements unit-keyed synchronized captures and tests. |
prebindgen-proc-macro/Cargo.toml |
Removes rand. |
Cargo.toml |
Removes workspace rand. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .or_else(|| arg.strip_prefix("--codegen=metadata=")), | ||
| } | ||
| }) | ||
| .rfind(|metadata| !metadata.is_empty()) |
There was a problem hiding this comment.
I do not think the empty-value case is a bug. rustc models metadata as a Vec<String> parsed by parse_list, rather than a scalar last-value option. On both Rust 1.85.0 and stable 1.97.1, -Cmetadata=first -Cmetadata= produced the same mangled symbol as -Cmetadata=first; the empty occurrence adds no component, so searching backward for the last non-empty component matches the compiler here. The related actionable problem is the opposite: every non-empty occurrence accumulates, while this parser hashes only the last one (see the new thread above).
— Codex (GPT-5)
There was a problem hiding this comment.
Resolved by e6e2a33, though not as last-value precedence. -C metadata is a list option (parse_list): occurrences accumulate, each value splits on whitespace, and an empty value contributes nothing — confirmed against rustc 1.85.0 and 1.97.1, where -Cmetadata=first -Cmetadata= produces the same crate hash as -Cmetadata=first.
The id now hashes the whole sorted component list, so there is no "final occurrence" to select: -Cmetadata=first -Cmetadata= keys on [first], and a metadata list that is empty overall (-Cmetadata= alone) leaves nothing to key on and falls through to the argument fallback, which is what you asked for at the end. unit_id_follows_rustc_list_parsing_of_metadata covers all three cases.
| { | ||
| break path; | ||
|
|
||
| hash_unit_id(args) |
There was a problem hiding this comment.
Confirmed at fea397d5 with the real example-flat crate. After cargo test -p example-flat --doc alpha, changing only the filter to beta added default_21413f49f69bdcb9.jsonl and structs_21413f49f69bdcb9.jsonl alongside the existing complete captures; no prior file was reused. Repeating with arbitrary filters therefore still grows this directory by two files per invocation, so this remains a blocker for the stated leak fix.
— Codex (GPT-5)
There was a problem hiding this comment.
Fixed in e6e2a33. The fallback now hashes the command line minus the options that only run the compiled tests or render diagnostics: --test-args, --test-run-directory, --runtool, --runtool-arg, --nocapture, --color, --error-format, --json, --diagnostic-width — each in both the --opt value and --opt=value spelling, dropping the carried value with the flag.
It is a deny-list of provably run-time-only options rather than an allow-list of compile-identifying ones, on purpose. Dropping an argument that does identify a unit merges two units onto one capture file, where one silently resets the other's records (see the metadata thread for what that costs); keeping an argument that does not identify a unit only leaves one extra file behind. So an option nobody anticipated stays in the id. unit_id_fallback_ignores_arguments_that_only_run_the_tests asserts both directions, including that an unknown option still separates ids.
Verified on the real crate: three lifecycle rounds of check, build, test, two cargo test -p example-flat --doc <filter> runs with a different filter in each round, cargo test --doc -- --nocapture, all-target clippy and cargo doc. The capture directories are identical after rounds 1, 2 and 3 — same file names, every default file at 44 records and every structs file at 3 — where at fea397d5 each new filter added a pair.
| fn unit_id_from_args(args: &[OsString]) -> String { | ||
| // rustc accepts both short/long and joined/separate codegen options. Use | ||
| // the last occurrence, matching how repeated compiler options are applied. | ||
| if let Some(metadata) = args |
There was a problem hiding this comment.
[P1] Hash the complete metadata list, not only its last occurrence
rustc defines this option as metadata: Vec<String> with parse_list, so repeated -C metadata occurrences accumulate; they do not use ordinary last-value precedence. On Rust 1.85, a probe compiled with only -Cmetadata=second produced symbol hash h67633aed..., while -Cmetadata=first -Cmetadata=second produced h30e3a460..., confirming that the earlier value still changes the compilation unit. Here, however, both unit_a, shared and unit_b, shared hash only shared. Valid invocations for different targets that append the same user metadata can therefore reset the same JSONL file (and the process-local mutex cannot protect concurrent rustc processes), silently dropping one unit records. Please hash all metadata components in order, and add a regression where two argument lists have the same final metadata but different earlier metadata.
— Codex (GPT-5)
There was a problem hiding this comment.
Fixed in e6e2a33. The id now hashes every -C metadata component, sorted, instead of the last one.
Reproduced first, with actual record loss and no exotic invocation — RUSTFLAGS="-Cmetadata=custom" reaches every unit, so the lib and test units of example-flat (which share one out/prebindgen) ended up with the same trailing component. With a temporary #[cfg(test)] #[prebindgen] pub fn test_only_probe() added to example-flat:
RUSTFLAGS=-Cmetadata=custom cargo test -p example-flat --lib
RUSTFLAGS=-Cmetadata=custom cargo build -p example-flat
- at
fea397d5: one file,default_35de3c917bb9a9c3.jsonlwith 44 records;test_only_probeis in no file — the build unit reset the test unit's capture. - at
e6e2a33c:default_d1c6e7b1eed6ba16.jsonl(45 records) anddefault_9de8e1b72630ef9d.jsonl(44); the probe survives.
I also re-derived the list semantics from rustc rather than from the option table, on 1.85.0 (legacy symbol hash) and 1.97.1 (v0 crate disambiguator):
| arguments | 1.85.0 | 1.97.1 |
|---|---|---|
-Cmetadata=b |
h67633aed03fee1fb |
CseyYMGf8ElKw_ |
-Cmetadata=a -Cmetadata=b |
h5a1047a0533b34ea |
Cs1XVTM2TReHw_ |
-Cmetadata=b -Cmetadata=a |
h5a1047a0533b34ea |
Cs1XVTM2TReHw_ |
-Cmetadata='a b' |
h5a1047a0533b34ea |
Cs1XVTM2TReHw_ |
-Cmetadata=a -Cmetadata= |
h3dcdd8ed3cea6839 |
Cs2CqKFEsjyPW_ |
-Cmetadata=a |
h3dcdd8ed3cea6839 |
Cs2CqKFEsjyPW_ |
Components accumulate, each value splits on whitespace, an empty value contributes nothing, and order does not matter — StableCrateId::new sorts the list, so -Cmetadata=b -Cmetadata=a is the same unit as -Cmetadata=a -Cmetadata=b. unit_id_from_args now sorts too: matching rustc there is exact rather than merely conservative, since arguments differing only in order really are one unit and produce identical records.
unit_id_hashes_every_metadata_component_not_only_the_last covers the two argument lists sharing a trailing component (and the order case); unit_id_follows_rustc_list_parsing_of_metadata covers whitespace splitting and the empty value.
|
Review outcome: I would not merge this revision yet. Two blocking correctness cases remain:
The separate empty-metadata report is not reproducible as a bug: on Rust 1.85.0 and 1.97.1, an empty occurrence adds no component and does not override an earlier non-empty value. Focused proc-macro and JSONL tests pass, — Codex (GPT-5) |
Two review blockers, both cases where the capture id was coarser than the compilation unit — the failure mode that loses records rather than files. `-C metadata` is a list, not a scalar: rustc parses it with `parse_list`, so occurrences accumulate and each contributes its whitespace-separated words, and `StableCrateId::new` sorts them. Hashing only the last occurrence mapped `unit_a shared` and `unit_b shared` onto one file, so one unit reset the other's capture. Reproduced with `RUSTFLAGS=-Cmetadata=custom`, which reaches every unit: the lib and test units of example-flat then shared one file, and a `#[cfg(test)]`-only item captured by the test unit was silently erased by the following build. Collect every component, sort as rustc does, hash the list. The rustdoc fallback hashed the whole command line, including the doctest filter Cargo forwards as `--test-args`, so `cargo test --doc alpha` and `--doc beta` minted a capture pair each. Drop the options that only run the tests or render diagnostics before hashing. The exclusion is a deny-list of provably run-time-only options rather than an allow-list: an unknown option stays in the id, where it can cost an extra file but never a record. Verified `-C metadata` list semantics (accumulate, whitespace-split, sorted, empty ignored) against probe-crate symbol hashes on 1.85.0 and 1.97.1.
Closes #201.
What leaked
#[prebindgen]named each capture{group}_{pid}_{thread}.jsonl. Every rustc process therefore created another full capture. The build script clears its prebindgen directory only when Cargo reruns that script, so repeatedcargo check,build,test,clippy, and rustdoc compilations could keep adding JSONL files to an unchanged Cargo build-output directory.For a crate the size of
zenoh-flat(310 marked items), each repeated capture is roughly 0.5 MiB per build-hash directory.What this PR fixes
Capture names are now keyed to the compilation unit rather than the rustc process:
-C metadata/--codegen metadatacomponent. rustc parses this option withparse_list, so occurrences accumulate, each contributes its whitespace-separated words, andStableCrateId::newsorts them; the id mirrors that exactly.foo/baror a Windows-style backslash safe.--test-args,--test-run-directory,--runtool,--runtool-arg,--nocapture,--color,--error-format,--json,--diagnostic-width), so a doctest filter no longer mints a new capture.The number of files depends on which distinct units Cargo asks rustc/rustdoc to compile, but it is bounded per unit rather than growing per invocation.
Both halves of the id deliberately err towards more ids. An id finer than the unit costs a stale file; an id shared by two units lets one unit reset the other's capture and silently drop items, which is the failure that breaks a downstream build. Accordingly the rustdoc filter is a deny-list of provably run-time-only options rather than an allow-list of compile-identifying ones: an unrecognized option stays in the id.
No reader change is needed:
Sourcealready discovers groups by the prefix before the first underscore and deduplicates records by name plus cfg.Review fixes (latest revision)
Both blockers found on
fea397d5are fixed and covered by regressions.1. Repeated
-C metadatacomponents were collapsed (#396 (comment) r3787045196). Reproduced end-to-end, with silent record loss, using nothing more exotic thanRUSTFLAGS="-Cmetadata=custom"— which reaches every unit, so the lib and test units ofexample-flatshared one trailing component:with a temporary
#[cfg(test)] #[prebindgen] pub fn test_only_probe()inexample-flat:out/prebindgentest_only_probefea397d5default_35de3c917bb9a9c3.jsonl(44 records)default_d1c6e7b1eed6ba16.jsonl(45),default_9de8e1b72630ef9d.jsonl(44)The list semantics were re-verified directly against rustc, on 1.85.0 (legacy symbol hash) and 1.97.1 (v0 crate disambiguator), with a probe crate:
-Cmetadata=bh67633aed03fee1fbCseyYMGf8ElKw_-Cmetadata=a -Cmetadata=bh5a1047a0533b34eaCs1XVTM2TReHw_-Cmetadata=b -Cmetadata=ah5a1047a0533b34eaCs1XVTM2TReHw_-Cmetadata='a b'h5a1047a0533b34eaCs1XVTM2TReHw_-Cmetadata=a -Cmetadata=h3dcdd8ed3cea6839Cs2CqKFEsjyPW_-Cmetadata=ah3dcdd8ed3cea6839Cs2CqKFEsjyPW_So components accumulate, values split on whitespace, order does not matter (rustc sorts), and an empty value contributes nothing — which is what the id now implements, and which also settles the separate empty-value report: an empty occurrence adds no component, and a metadata list that is empty overall falls through to the argument fallback.
2. The rustdoc fallback hashed runtime-only arguments (#396 (comment) r3779811135).
cargo test --doc alphaand--doc betareach rustdoc as--test-args alpha/--test-args beta, which do not change what is compiled. Those options are now dropped before hashing. Three full lifecycle rounds (check,build,test, two doctest runs with a different filter each round,cargo test --doc -- --nocapture, all-target clippy,doc) leave a byte-for-byte identical file set after every round, each capture complete at 44 / 3 records.Follow-up: stale Cargo build-hash directories
This PR itself does not remove stale
target/*/build/<pkg>-<hash>/directories. The remaining prebindgen-owned accumulation is now addressed by stacked PR #398 (issue #397).PR #398 adds an explicit
cargo prebindgen cleancommand. It acquires all discovered Cargo profile locks before mutation, removes only state-validatedout/prebindgenpayloads across old build hashes, and invalidates each exact producer so the capture is regenerated on next use. It deliberately leaves Cargo's<pkg>-<hash>directories and all unrelated artifacts intact.Current split:
<pkg>-<hash>/out/prebindgendirectory.<pkg>-<hash>directories themselves.Verification
#[prebindgen]after a source edit, so a reset capture is always rewritten complete.cargo test --all --all-featurescargo clippy --all-targets --all-features -- --deny warningsgit diff --checkexamples/regen-check.sh: committed generated output remains byte-identical.