refactor(sid): drop legacy instances_matching fallback + fix capability-matching flake - #208
Merged
Merged
Conversation
…ty-matching flake Two related cleanups bundled because both touch deterministic sid lookup and they're each small. ## 1. Drop `instances_matching` fallback in ASAPQueryEngine.execute PR #206 wired the content-addressed fast path with a fallback to the legacy `idx.instances_matching(metric, gbk)` walk for sids registered with `PolicyFingerprint::UNSET`. With PRs #203 + #205 populating the fp on every production registration path, the fallback's only consumers are test fixtures and the raw-mode fast-path that the sink already drops. Removing it makes "capability miss" mean exactly one thing — no policy in the registry satisfies the candidate — instead of overloading the miss path between "no policy" and "no sid metadata". ## 2. Make `aggregation_priority` a total order `capability_matching::tests::avg_finds_sum_and_count` had been flaky because `aggregation_priority` returned `Equal` on equal `window_size`, leaving `Vec::sort_by` order dependent on the underlying `HashMap` iteration. The test inserts both a `Sum` and a `CountMinSketch` config for the same metric; `Statistic::Sum` matches both, and when the multi-pop `CountMinSketch` sorted first the downstream key-aggregation lookup (only needed for multi-pop value types) missed and the whole match returned `None`. Sort keys now: 1. Larger `window_size` (coarser windows answer finer-grained queries via re-aggregation). 2. Single-population types beat multi-population (avoids the key-aggregation hunt when both shapes serve the statistic). 3. `aggregation_id()` (the policy fingerprint u64) tie-break — deterministic across runs and hosts. The result for the failing test: `Sum` always wins the Sum-stat candidate, no key-aggregation lookup fires, the function returns `Some(...)` deterministically. ## Engine test update `execute_returns_capability_miss_when_classify_is_ghost` previously asserted the detail string contained "ghost" / "unknown". With the fallback removed, the engine short-circuits at the policy-resolution step (the test fixture's `dd_meta` helper registers with `PolicyFingerprint::UNSET`, so the policy lookup never finds the sid). The `CapabilityMiss` outcome is preserved; the detail-string assertion is dropped since it pinned implementation, not contract. ## Test plan - [x] `cargo check --workspace` clean - [x] `cargo test --workspace --lib --bins` green - [x] `avg_finds_sum_and_count` ran 5× in a row, all pass — flake gone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related cleanups bundled.
Drop legacy
instances_matchingfallback inASAPQueryEngine.execute. Every production registration path now populatespolicy_fp; the fallback was only catching test fixtures and the raw-mode fast-path that the sink drops anyway. CapabilityMiss now means exactly one thing: no policy in the registry satisfies the candidate.Fix
avg_finds_sum_and_countflake.aggregation_priorityreturnedEqualon equalwindow_size, leavingsort_byorder dependent on HashMap iteration. New sort keys: window_size DESC → single-pop wins →aggregation_id()tie-break.Test plan
cargo check --workspacecleancargo test --workspace --lib --binsgreen🤖 Generated with Claude Code