fix(dylint): give each boundary-lint process a unique observation identity - #1382
Conversation
…ntity
`Dylint (windows)` failed on an fbuild-cli-only change with:
platform-boundary: actual Dylint observations disagree for pid=2636
source=crates/fbuild-toolchain/src/toolchain/esp_qemu.rs:
expected={('attr_cfg', 'windows'): 6} actual={('attr_cfg', 'windows'): 12}
Nothing in that PR touched fbuild-toolchain, both counts were exactly
double, and the Linux leg of the same commit was green.
`cargo dylint --all-targets` runs one driver process per crate-target, so a
crate's lib and lib-test targets compile the same sources twice. Each
process stamped its observations with `std::process::id()` alone — and
Windows hands out a recycled PID readily enough that the second process can
be given the first one's number after it exits. The checker keys findings by
that PID, so two runs merged into one bucket and every count for the shared
sources read double. The same collision on Linux is rare enough that the
gate looked stable.
Replayed against a real observation file, collapsing every identity to a
single PID reproduces the CI failure exactly — same two files, same 6 -> 12
and 3 -> 6:
awk -F'\t' 'BEGIN{OFS="\t"} {$1="2636"; print}' observed.tsv > collide.tsv
uv run --no-project python ci/enforce_platform_boundary.py \
--dylint-observed collide.tsv
The identity is now `<pid>-<nanos>`, taken once per process. Reusing a PID
requires the original holder to have exited first, so two processes cannot
share a start nanosecond. The checker needs no change: it already treats the
first field as an opaque key.
Crate version bumped so the cached `.so` is rebuilt with the new emitter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 30 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 |
Symptom
Dylint (windows)failed on #1379 — a change confined tofbuild-cli:Three things pointed away from the PR: nothing in it touched
fbuild-toolchain, both counts were exactly double, and the Linux leg of thesame commit was green.
Cause
dylint --all-targetsruns one driver process per crate-target, so a crate'slib and lib-test targets compile the same sources twice. Each process stamped
its observation lines with
std::process::id()and nothing else. Windowsrecycles PIDs readily enough that the second process can be handed the first
one's number once it has exited — and
compare_dylint_observationskeysfindings by
(process, source, kind, normalized), so the two runs merged intoone bucket and every count for the sources they share read double.
The same collision is possible on Linux; the PID space is large enough that it
effectively never happens, which is why the gate has looked stable.
Verification
Replaying a real observation file with every identity collapsed to one PID
reproduces the CI failure exactly — the same two files, the same 6 → 12 and
3 → 6:
With the fix in place the same sweep emits distinct identities and the checker
passes:
Fix
The identity is
<pid>-<nanos>, taken once per process via aLazyLock.Reusing a PID requires the original holder to have exited first, so two
processes cannot share a start nanosecond.
The Python checker needs no change — it already treats the first field as an
opaque key. Deduplicating findings would have been the wrong fix: the counter
legitimately counts repeated
(kind, normalized)pairs within one file.Crate version bumped so the cached
.sois rebuilt with the new emitter.