perf(security): seek token gaps in C and stop scanning confusables as a regex class

[smoy]

Follow-up to #569 and #570, rebased onto current main. Three whole-text fast paths, all behaviour-preserving, plus the cache-lifecycle fixes from two review rounds.

Summary of Changes

1. Seek token gaps in C. _token_bridging_gap_spans stepped through the text one character at a time in Python looking for the next gap character. It now seeks with a compiled regex. Every token-gap character lies outside printable ASCII and tab/newline/carriage-return, so the seek class is a superset and each hit is still confirmed by the exact predicate. Seeking jumps over the offset % 4096 checkpoints the walk would hit, so _check_skipped_checkpoints replays one check per checkpoint crossed and cooperative cancellation is observed exactly as before.

2. Confusable containment as a set, not a regex class. _ASCII_CONFUSABLE_PATTERN was a character class built from 1,515 individually escaped code points spanning 528 disjoint ranges. The only question ever asked of it is "does this text contain any of these", which a frozenset answers in one C-level pass. On 180 KB of ASCII prose:

regex character class	164.30 ms
range-compressed class	44.97 ms
frozenset.isdisjoint	0.70 ms

The class contains no ASCII code point at all, so ordinary text answers with a single disjointness check.

3. Memoize two builders. normalized_security_view and the letter-spacing span scan, in the manner of #570. The view is a frozen dataclass whose source_offsets array is only ever read — sliced, or copied into a fresh array — so callers can share one instance. A caller passing check_runtime bypasses the cached path, so runtime budgets are still enforced.

4. The view cache is bounded, synchronized, and released. It is bounded by stored characters rather than entries, because NFKC expands one U+FDFA into 18 characters each carrying a four-byte offset, so an entry count bounds nothing. Every operation takes a lock: LangGraph fans the analyzer nodes out through a thread pool under invoke and ainvoke alike, while scan teardown clears the cache. And cleanup_result now releases the content-keyed caches whether or not the scan produced a result, so a scan that raises or is cancelled does not leave the derived views — or the scanned text itself, held as a predicate-cache key — alive in a long-lived server.

Root Cause Analysis

Profiling an LLM-enabled scan on main after #569/#570 left these as the remaining per-character primitives on a single 237 KB skill: str.isascii 12.5M calls, str.isalpha 5.4M, str.translate 4.8M. Item 1 removes the first of those — it was the fast-path dispatch #569 introduced, which made the per-character check cheap without removing the per-character step.

Item 2 was a surprise. _requires_normalized_security_view cost 166.31 ms on 180 KB of ASCII, and 164.30 ms of that was the confusable regex alone — the other four checks in that function total under 2 ms. A character class with hundreds of disjoint ranges costs a bounded scan at every position; set membership is a hash lookup over the text's distinct characters. The predicate now runs in 2.78 ms, a 60× improvement.

Worth noting what this replaced: I had expected to gate the normalization path behind a pure-ASCII prefilter, on the theory that NFKC normalization and homoglyph skeletons cannot fire on ASCII. That turned out to be unnecessary — the normalization checks were never the cost. One pathological regex was.

Test Coverage & Verification

tests/nodes/analyzers/test_security_scan_fast_paths.py:

• The seek class covers every token-gap code point in Unicode — the property the C-level seek depends on, asserted exhaustively.
• Gap spans are identical to the character-wise scan across 9 hand-written texts and 1,500 randomized ones drawn from an alphabet of zero-width joiners, bidi overrides, soft hyphens, replacement characters, controls, accented Latin and CJK.
• Confusable membership matches the original character class across 1,500 more randomized texts.
• Memoized letter-spacing spans equal the uncached scan; a caller passing check_runtime still has it invoked.

tests/nodes/analyzers/test_security_scan_runtime_and_memory.py covers what the two review rounds found: checkpoint cadence at six lengths, cancellation across sparse in-word gaps and on candidate-free text, the stored-character bound, the oversized-single-view case, eviction-then-rebuild identity, both cache races, and teardown after a graph that raises, a scan that is cancelled, and a CLI scan that fails.

Full suite: 6883 passed, 14 skipped, 134 deselected, 4 xfailed. ruff check and ruff format --check clean; mypy reports nothing new in the files this touches.

End-to-end on real skills. 901 real skills, two alternating runs per arm on an idle 8-core host, against the main this branch is rebased onto:

	main (224ba29)	this branch	change
p50	1.594s	1.591s	-0.2%
p95	3.838s	3.382s	-11.9%
p99	10.884s	9.376s	-13.9%
total scan time	1799.4s	1731.2s	-3.8%

Every one of the 901 units agrees across the two arms on risk score, issue count, recommendation and coverage -- 8,158 findings and 176 coverage-ledger outcomes on both, zero errors -- and each arm is deterministic across its own two runs.

Scope

The corpus-level gain is much smaller than the microbenchmarks, and I would rather say so than lead with the 60×. The predicate that improves 60× is already memoized by #570, so it runs about 23 times per scan rather than 780. It remains the single most expensive thing left inside that predicate, and the regex class would cost proportionally more on larger files than this corpus contains.

The two largest remaining costs are untouched: _letter_spacing_run_spans and, in security_reconstruction.py, _encoded_tag_directives and _quoted_directives. Those look like genuine algorithmic work rather than incidental overhead, and I did not want to restructure detection logic without a maintainer's view. Happy to take a look if that would be useful.

Two retention paths are out of scope here and pre-date this PR: python_ast_cache_key and temp_dir_for_cleanup surface only in the returned state, so a scan that raises cannot release them, and _scan_multi_skill's per-skill except has no finally at all. Happy to take either in this PR or a follow-up.

[yashrajp22]

Two reproducible regressions need fixes: sparse token-gap seeking can skip cooperative cancellation checks, and the normalized-view cache retains hundreds of MiB after a modest Unicode input. Verified against main 8028ce5 using exact source snapshots and fresh installed wheels; all 397 selected repository tests passed. Details are inline.

[yashrajp22]

The seek can jump over every offset % 4096 == 0 checkpoint. With ('a\u115fa ' * 8192), a callback that raises on its second call aborts on main but is called only once here. _contextual_default_ignorable_boundary_spans rejects all these in-word gaps, so artifact-integrity's initial next(spans, None) consumes the entire file without another budget check. Could we check runtime after seeking, or track crossed checkpoints, so large inputs still respect cancellation?

[smoy]

Confirmed and fixed in 7f0f1ab — thank you, this was a real regression and your reproducer landed it exactly.

Reproduced first: with 'aᅟa ' * 8192 and a callback raising on its second call, main aborts and this branch called the callback once.

Rather than a single check after seeking, _check_skipped_checkpoints now fires one check per offset % 4096 checkpoint the seek crossed, so the cadence matches the character-by-character walk it replaced rather than merely approximating it. A test asserts the count equals len // 4096 at 0, 4095, 4096, 10k, 32k and 100k characters.

I also found the whole-string early return (_TOKEN_GAP_CANDIDATE.search(text) is None) had the same gap — it returned without firing any checkpoint on text with no candidates at all. That path is fixed and covered by its own test.

[yashrajp22]

Could we bound this cache by stored size or give it a scan-scoped lifetime? With a roughly 5 MB UTF-8 file of numbered U+FDFA runs, the existing windowed static runner leaves 14 normalized views holding about 349 MiB of text/offset arrays after returning; main releases all of them. NFKC expands each U+FDFA to 18 characters, each with a four-byte offset. The 64-entry limit does not bound that expansion, and cleanup_result never clears this cache, so the added memory remains in long-lived scanner processes even after the input is released.

[smoy]

Confirmed and fixed in 7f0f1ab. You are right that entry count bounds nothing here — I reproduced the shape of it locally (14 views of U+FDFA runs retaining tens of MiB, scaling exactly as you describe).

Two changes:

1. The cache is now bounded by stored characters (4M budget) rather than entries, and declines to retain any single view larger than the whole budget. Forty expanding inputs settle at 2.88M stored characters instead of growing without limit.
2. clear_security_text_caches() is called from cleanup_result, alongside the existing clear_python_ast_cache — following the precedent already there rather than inventing a new hook. That also releases the text keys the predicate caches from perf(security): memoize the pure text predicates behind security views #570 hold, which is a retention path I had missed independently of this PR.

Tests cover the size bound, the oversized-single-view case, clearing, teardown through cleanup_result, and that a view evicted and rebuilt is identical to the original.

Performance is preserved: p95 −10.3%, p99 −12.7% against main over 901 real skills, findings byte-identical. The mean gain narrows from −4.1% to −2.6%, which is the cost of the restored checkpoints and is the right trade.

[rng1995]

[SkillSpector Review]

Reviewed exact head 9dc3c201018c4dfe4fa221cc02fb6f7c56c2ccc9 and the complete fast-path/cache diff.

Two current-head regressions already documented inline remain blocking:

1. Sparse regex seeking can jump across all 4,096-character cooperative runtime checkpoints, so cancellation may not be observed while a long input is scanned.
2. The new 64-entry normalized-view cache is count-bounded but not byte-bounded; modest expanding Unicode inputs can leave hundreds of MiB retained after scanning.

Please preserve the performance improvements while restoring periodic cancellation and giving the derived-view cache a byte bound or scan-scoped lifetime. I verified the control flow and did not duplicate the existing inline comments.

[smoy]

Both blocking findings are fixed in 7f0f1ab. @yashrajp22 @rng1995 — thank you both, these were real regressions and the reproducers made them quick to confirm. I reproduced each against this branch before changing anything.

1. Cooperative cancellation. Seeking jumps over the offset % 4096 checkpoints a character walk would hit. On 'aᅟa ' * 8192 with a callback raising on its second call, main aborts and this branch called it once.

_check_skipped_checkpoints now fires one check per checkpoint crossed, so the cadence matches the walk rather than approximating it — asserted equal to len // 4096 at 0, 4095, 4096, 10k, 32k and 100k characters. The whole-string early-return path had the same gap and is fixed too.

2. Derived-view cache. Entry count bounds nothing when NFKC expands one U+FDFA into 18 characters each carrying a four-byte offset. The cache is now bounded by stored characters (4M budget) and refuses any single view larger than the budget; 40 expanding inputs settle at 2.88M stored characters instead of growing without limit.

3. Retention after a scan. clear_security_text_caches() is called from cleanup_result, next to the existing clear_python_ast_cache — reusing the precedent rather than adding a hook. This also releases the text keys held by the predicate caches introduced in #570, a retention path that exists independently of this PR and that I had missed there.

Performance is preserved, measured over 901 real skills with two alternating runs per arm against main: p95 −10.3%, p99 −12.7%, total scan time −2.6%. Findings byte-identical (8,152 both arms), coverage-ledger outcomes identical (176), zero errors. The mean gain narrows from −4.1% to −2.6% — that is the cost of the restored checkpoints, and it is the right trade.

13 new tests: checkpoint cadence at six lengths, cancellation across sparse in-word gaps and on candidate-free text, the size bound, the oversized-single-view case, clearing, teardown via cleanup_result, and that an evicted-then-rebuilt view is identical to the original. Full suite 5821 passed; ruff and mypy clean.

I kept 9dc3c20 intact and stacked the fix on top rather than force-pushing a rebase, so your inline threads stay anchored to the lines you reviewed.

[rng1995]

[SkillSpector Review]

Re-reviewed exact head 7f0f1ab4e8917f6932a0ed452c347d06510bc955.

The prior cooperative-cancellation and count-bounded retention findings are resolved: skipped checkpoints are replayed, derived views have a stored-character budget, and successful scan teardown clears the caches. I did not repeat those resolved findings.

Two new cache-lifecycle blockers remain. The custom global cache is not synchronized even though analyzers and MCP scans can overlap, and the cleanup hook is skipped when a scan raises or is cancelled before producing a result. The inline comments describe the required corrections and regression coverage.

All six hosted CI checks pass. GitHub reports MERGEABLE / BLOCKED; both earlier threads also remain unresolved, including one outdated thread. This review does not merge the PR.

[rng1995]

[P1] Synchronize the global cache. The graph runs analyzers in parallel, and MCP scans can overlap, but get, store, and clear mutate _entries, _sizes, and _total through multi-step operations without a lock. Concurrent stores can lose or double-count _total; cleanup racing get() can remove the key before move_to_end(), raising KeyError. That invalidates the memory bound and can fail scans. Protect each operation with a shared lock (without holding it during the expensive view build), or make the cache scan-scoped, and add concurrent get/store/clear regression coverage.

[smoy]

Confirmed and fixed in SHA. Both halves of this were real, and both are now reproduced by tests that widen the interleaving window rather than race for it, so they fail on every run without the lock.

get vs clear. A clear landing between the lookup and the move_to_end that follows it leaves move_to_end with a key that is gone: KeyError('key'), raised inside whichever analyzer happened to be reading.

store vs clear. A clear landing after the entry is recorded but before its size is leaves the cache reporting characters it is not holding — 100 stored characters against an empty _entries in the test. That is the failure you called out: the bound is then wrong for the life of the process, not just for that scan.

Every operation now takes a lock, following the per-scan AST registry in python_ast rather than inventing a pattern. Building a view stays outside it, so two threads racing the same uncached text still build in parallel — that costs the work twice and never yields a different view. The lru_cache predicates cleared alongside it need nothing; CPython's C lru_cache, cache_clear included, is internally thread-safe.

On the alternative you offered: I kept the cache global rather than scan-scoped. normalized_security_view is reached from pure text helpers several layers below any node that holds SkillspectorState, so keying it per scan means threading a scan id through the whole security-text API and re-keying the lru_cache predicates from #570 to match — a larger change than this PR, and one I would rather not smuggle into a perf branch. Worth saying that these caches are pure memoizations of text: the only thing a cross-scan clear can cost is recomputation, never a different finding. If you would rather have the scan-scoped version, I am happy to do it — here or as a follow-up.

[rng1995]

[P1] Clear caches when a scan fails or is cancelled. This hook only runs once callers have a result: mcp_server.run_scan and the CLI guard cleanup_result with result is not None. If graph.ainvoke raises or is cancelled after these caches have been populated, a long-lived server retains the full input keys and derived views. Put security-cache cleanup in an unconditional per-scan finally/lifecycle (or use scan-scoped caches), and add a regression where graph execution exits exceptionally after cache population.

[smoy]

Confirmed and fixed in SHA. You are right that the hook only ran once a caller had a result.

cleanup_result now takes dict | None and releases the content-keyed caches before it looks at the result, so the three scan entry points — mcp_server.run_scan, the CLI scan and baseline — call it from their existing finally with no guard. CancelledError propagates through finally like any other exception, so cancellation is covered by the same change. Four tests cover it: a graph that raises and one that is cancelled under the MCP server, a CLI scan whose graph raises, and cleanup_result(None) on its own.

I did not widen the scope past the caches, and two retention paths remain that I want to name rather than leave for you to find:

• python_ast_cache_key and temp_dir_for_cleanup surface only in the returned state, so a scan that raises still cannot release either. The AST registry self-heals (per-scan keyed, LRU-bounded at 32 abandoned scans); the temp dir does not.
• _scan_multi_skill's per-skill except has no finally at all, so a child skill that raises leaks its temp dir.

Both pre-date this PR. Recovering them needs a scan-scoped handle created before invoke, which is a different change from this one. Happy to do it here if you would rather it landed together.

[smoy]

Both P1s are fixed in 57dface, and the branch is rebased onto 224ba29 so everything below is measured against current main.

1. The view cache is synchronized. Reproduced both halves first, with tests that widen the interleaving window rather than race for it -- they fail on every run without the lock and finish in well under a second. A clear landing inside get raises KeyError in whichever analyzer was reading; a clear landing inside store, after the entry is recorded but before its size is, leaves the cache reporting characters it is not holding, which is the failure mode that invalidates the bound for the life of the process. Every operation now takes a lock, following python_ast's registry rather than inventing a pattern; building a view stays outside it. The lru_cache predicates cleared alongside it need nothing -- CPython's C lru_cache, cache_clear included, is thread-safe. I kept the cache global rather than scan-scoped, for the reason in the inline thread, and am happy to do the scan-scoped version if you prefer it.

2. Teardown runs on every exit path. cleanup_result takes dict | None and releases the content-keyed caches before it looks at the result, so the three scan entry points call it from their existing finally with no guard. CancelledError propagates through finally like any other, so cancellation is covered by the same change. Covered by four tests: a graph that raises and one that is cancelled under the MCP server, a CLI scan whose graph raises, and cleanup_result(None).

Two retention paths I did not touch, named here rather than left to be found: python_ast_cache_key and temp_dir_for_cleanup surface only in the returned state, so a raising scan still cannot release them, and _scan_multi_skill's per-skill except has no finally at all. Both pre-date this PR and need a scan-scoped handle created before invoke. Say the word and they land here instead of a follow-up.

Performance, re-measured against the new base -- 901 real skills, two alternating runs per arm on an idle 8-core host:

	main (224ba29)	this branch	change
p50	1.594s	1.591s	-0.2%
p95	3.838s	3.382s	-11.9%
p99	10.884s	9.376s	-13.9%
total scan time	1799.4s	1731.2s	-3.8%

Every one of the 901 units agrees across the two arms on risk score, issue count, recommendation and coverage -- 8,158 findings and 176 coverage-ledger outcomes on both, zero errors -- and each arm is deterministic across its own two runs. Full suite 6883 passed, 14 skipped, 4 xfailed.

The rebase does mean the two earlier threads are now anchored to rewritten commits. I kept the history otherwise intact -- the fast-path commit, the cancellation/bound fix, and this one -- rather than squashing, so the sequence of what each round changed is still readable.