perf(scrub): hot-path guards, recall gaps, and corrected benchmark claims
Summary
Split out of !26 (closed), which bundled a performance regression with work that has none. This is the half with no regression and no open questions: two recall gaps, and a hot-path cleanup that makes the scrubber measurably faster than main. No new value patterns, no behavior change to any existing redaction — nothing here can redact text that main wouldn't, except via key names.
Everything with a trade-off attached went to !28, which stacks on this branch. That includes the string-headed-list fix that was originally here: three fresh-context reviewers turned up two genuine judgment calls in it, so it moved rather than hold this up.
Commits
-
credskey token +errorssection walk — unchanged from !26 (closed).credsis a more common variable name thancredentialsand was matched by neither list.errorsis persisted verbatim intoIssueEvent.dataand needs the same recursive walk as every other section. -
Perf: guard the value patterns, stop cloning on no-match — unchanged from !26 (closed).
CARD_REran unconditionally over every string in every scrubbed section; each pattern now has a byte-level precondition that is a strict superset of what its regex can match. Separately,replace_allreturnsCow::Borrowedon no-match and the old code called.into_owned()regardless. -
Docs: correct three claims review proved wrong — new, documentation only.
What review found
Three fresh-context reviewers re-derived this from the diff alone. The guards — the part that would be a security bug if wrong — came back clean, and the verification was thorough enough to be worth recording:
- A full
0..=0x10FFFFsweep confirming every char the regex crate's\dmatches also satisfieschar::is_numeric(), so the non-ASCII fallback inhas_min_digitsis a genuine superset ofNd. - ~1.1M differential cases of guarded vs unguarded
scrub_string, including aplaceholder:""config that can splice two digit runs into a new 13-digit run after PEM substitution. Zero divergence. Cow::Borrowedis returned only on the genuine no-match path, so no replacement can be dropped.
Three things this file asserted did not survive, and commit 3 corrects them:
The errors rationale was backwards. The comment claimed the worker regenerates its entries from the already-scrubbed payload. It does not — EventProcessingError(type, name, value=v) is built during schema validation, before scrubbing, and stores the entire unvalidated field in value. The fields wrapped that way are exactly the PII-bearing ones.
The benchmark cannot measure what it appears to measure. The fixture has no @ anywhere, and its only ≥13-digit runs sit under the key session, which is key-redacted before scrub_string is reached. Instrumented, CARD_RE and EMAIL_RE execute zero times across all 385 strings per event. The speedup is real and reproduces in both run orders — −24% on defaults, −36% with emails, plus −62%/−75% fewer transient bytes allocated, which is the stronger north-star argument and the commit didn't even claim it — but it is a best case, and is now labelled as such in the fixture doc.
Two benchmark comments were wrong. The fixture is ~16 KB, not ~8 KB. Cloning it costs ~70 µs against a ~78–85 µs scrub — comparable, not the claimed order of magnitude.
One correction to the earlier numbers: the Cow change contributes 0 ns and 0 allocations on this fixture, because the guards prevent replace_all from being called at all. Ablation builds put guards-only and guards-plus-Cow within noise of each other. The Cow change is still right — it pays off on strings that pass the guard and don't match — but the −24%/−36% should be attributed to the guards alone, not read as additive.
Known follow-up, not in this MR
is_sensitive_key is the real hot path — ~55.6 µs of the ~78–85 µs scrub, roughly 4× everything this MR touches. normalize_key allocates twice for every key including the 82% that are already plain lowercase ASCII. A prototyped allocation-free fast path measured −43% allocations/event and −13.9% wall clock with all tests passing. Deliberately left for its own MR.
Risk
Confined to crates/gt-ingest/src/scrub.rs; nothing outside the crate constructs ScrubConfig. Per the exposure table in AGENTS.md the ingest pipeline is mothballed and GLITCHTIP_RUST_INGEST still defaults to False, so none of this is live in any deployment yet.
cargo fmt, cargo clippy --all-targets -D warnings and 139 tests clean.
AI disclosure: Claude Opus 5 (Claude Code) split this branch out of !26 (closed), wrote commit 3, drafted this description, and ran three independent fresh-context review passes whose findings are summarised above. Commits 1 and 2 are byte-identical to their reviewed form in !26 (closed). Reviewed by @bufke before publishing.