Skip to content

Commit 0b7ccb4

Browse files
committed
fix: preserve distinct ranges in advisory_affected_ranges
The unique index on advisory_affected_ranges shipped in V1779710880 keyed on (advisory_package_id, COALESCE(introduced_version, '')) — strictly narrower than the natural uniqueness of a range tuple, and narrower than the principle locked in osv-plan §2 #1 ("one package has many version ranges; no denormalization"). dedupeRanges in upsertAdvisory.ts was keying on introduced_version alone to match that index, with the side effect that two ranges sharing an introduced_version but differing in fixed_version or last_affected (cross-distro patches, partial fixes) silently collapsed to the first occurrence. When the surviving range was the narrower one, isInRange returned FALSE for versions inside the wider window — a missed critical alert. Three changes: - V1779897650__widen_advisory_affected_ranges_unique_index.sql: drop the narrow unique index (located via pg_indexes since the initial migration didn't name it) and replace with the full-tuple unique index over (advisory_package_id, COALESCE(introduced_version,''), COALESCE(fixed_version,''), COALESCE(last_affected,'')). - upsertAdvisory.ts dedupeRanges: key on the full tuple so the application-side pre-flight matches the database constraint. Exported for unit testing. - upsertAdvisory.test.ts: 5 cases pinning the new semantics (same-introduced-different-fixed preserved, same-introduced- different-last_affected preserved, identical-tuple collapsed, null-introduced disambiguated by other fields, first-wins on truly identical tuples). ADR-0006 captures the decision and the alternatives considered (coalesce-to-widest at parse time, drop the constraint, dedup at query time). Cursor's bot review on 1b978ac surfaced the bug. Signed-off-by: Joan Reyero <joan@reyero.io>
1 parent ebfc47b commit 0b7ccb4

5 files changed

Lines changed: 200 additions & 6 deletions

File tree

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
-- Widens the unique index on advisory_affected_ranges from
2+
-- (advisory_package_id, COALESCE(introduced_version, ''))
3+
-- to the full range tuple
4+
-- (advisory_package_id,
5+
-- COALESCE(introduced_version, ''),
6+
-- COALESCE(fixed_version, ''),
7+
-- COALESCE(last_affected, ''))
8+
-- so OSV records that emit multiple `affected[]` blocks for the same package
9+
-- sharing an introduced_version but differing in fixed_version / last_affected
10+
-- (cross-distro patches, partial fixes) no longer collide on insert and lose
11+
-- the wider range. Restores the osv-plan §2 decision #1 invariant: "one
12+
-- package has many version ranges, no denormalization." See ADR-0006.
13+
DO $$
14+
DECLARE
15+
idx_name text;
16+
BEGIN
17+
SELECT indexname INTO idx_name
18+
FROM pg_indexes
19+
WHERE schemaname = current_schema()
20+
AND tablename = 'advisory_affected_ranges'
21+
AND indexdef ILIKE '%UNIQUE%'
22+
AND indexdef ILIKE '%advisory_package_id%'
23+
AND indexdef ILIKE '%COALESCE(introduced_version%'
24+
AND indexdef NOT ILIKE '%fixed_version%'
25+
AND indexdef NOT ILIKE '%last_affected%';
26+
IF idx_name IS NOT NULL THEN
27+
EXECUTE 'DROP INDEX ' || quote_ident(idx_name);
28+
END IF;
29+
END $$;
30+
31+
CREATE UNIQUE INDEX ON advisory_affected_ranges (
32+
advisory_package_id,
33+
COALESCE(introduced_version, ''),
34+
COALESCE(fixed_version, ''),
35+
COALESCE(last_affected, '')
36+
);
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
# ADR-0006: Uniqueness scope of `advisory_affected_ranges`
2+
3+
**Date**: 2026-05-27
4+
**Status**: accepted
5+
**Deciders**: CDP/Insights team
6+
7+
## Context
8+
9+
OSV records describe vulnerable version windows as one or more `affected[]` blocks per advisory, each carrying its own `ranges[]`. The `parseOsvRecord` step in osv-sync coalesces multiple `affected[]` blocks targeting the same `(ecosystem, package_name)` into a single `NormalizedPackageEntry`, accumulating every range across those blocks (`services/apps/packages_worker/src/osv/parseOsvRecord.ts`). This is needed because OSV emits cross-distro patches, partial-fix advisories, and platform-specific windows as separate `affected[]` blocks for the same package.
10+
11+
The initial schema (`V1779710880__initial_schema.sql`, shipped in PR #4146) declared the uniqueness key for `advisory_affected_ranges` as:
12+
13+
```sql
14+
CREATE UNIQUE INDEX ON advisory_affected_ranges
15+
(advisory_package_id, COALESCE(introduced_version, ''));
16+
```
17+
18+
That is strictly narrower than the natural uniqueness of a range tuple — it forces any two ranges sharing an `introduced_version` to collapse into one row, even if they have different `fixed_version` or `last_affected` values. The `dedupeRanges` pre-flight in `upsertAdvisory.ts` keyed on `introduced_version` alone to avoid INSERT conflicts, with the side effect that the wider range was silently discarded for any pair that shared an `introduced_version`. This directly contradicts the principle locked in `osv-plan.md §2 #1`: _"one package has many version ranges. No denormalization."_
19+
20+
Cursor's bot review on commit `1b978ac22` surfaced the data-loss path: in cross-distro / partial-fix OSV records, the surviving range may be the _narrower_ one, leading `isInRange` (and thus `packages.has_critical_vulnerability`) to return FALSE for versions inside the wider window — a missed critical alert. The bug is real and not theoretical; this category of OSV record exists in the live dataset.
21+
22+
## Decision
23+
24+
Widen the unique index on `advisory_affected_ranges` to cover the full range tuple:
25+
26+
```sql
27+
CREATE UNIQUE INDEX ON advisory_affected_ranges (
28+
advisory_package_id,
29+
COALESCE(introduced_version, ''),
30+
COALESCE(fixed_version, ''),
31+
COALESCE(last_affected, '')
32+
);
33+
```
34+
35+
Update `dedupeRanges` to key on the full tuple `(introducedVersion, fixedVersion, lastAffected)` so the application-side pre-flight matches the database-side constraint. Truly identical tuples (the original "OSV emits a redundant event on the same line" case) still collapse; ranges that differ in any component are all preserved.
36+
37+
The change is captured in migration `V1779897650__widen_advisory_affected_ranges_unique_index.sql`, which drops the narrow index by definition (via `pg_indexes` lookup, since the initial migration did not name it) and recreates the wider one. The drop is idempotent: re-running on a database that already has only the wide index is a no-op.
38+
39+
## Alternatives Considered
40+
41+
### Alternative 1: Keep the narrow index, coalesce-to-widest at parse time
42+
43+
Merge ranges sharing an `introduced_version` in `parseOsvRecord` by taking the broadest `fixed_version` (max) and `last_affected` (max), so the surviving row always describes the union of the inputs.
44+
45+
- **Pros**: No migration. The narrow unique index stays as-is. Default skews toward over-reporting (broader vulnerable window) rather than under-reporting — better for the Osprey threat model where missed alerts are worse than false positives.
46+
- **Cons**: This _is_ denormalization, which `osv-plan §2 #1` explicitly excluded. The merged row loses per-distro provenance: a downstream consumer can no longer tell that the wider window came from distro A while distro B was patched earlier. Comparator semantics also become ambiguous when ecosystems use different version orderings on different platforms.
47+
- **Why not**: Encodes a workaround for a schema mistake in business logic. The schema is the contract; making the application reshape its data to fit a too-narrow constraint is the wrong direction.
48+
49+
### Alternative 2: Drop the unique index entirely, rely on the FK + dedupe in TS
50+
51+
Treat the table as an append log of ranges, with dedupe handled purely in the upsert path.
52+
53+
- **Pros**: Fewer constraints to maintain. Migration is one `DROP INDEX` line.
54+
- **Cons**: Real duplicate ranges (the OSV "redundant event" case the original dedupe was designed for) now land in the table and inflate scans during derivation. The query in `deriveCriticalFlag.ts` joins through this table per package; row count matters. Also loses the safety net that catches an application bug producing accidental duplicates — without the index, a malformed insert path can silently flood the table.
55+
- **Why not**: The unique constraint _should_ exist; the only question is what its scope should be. Removing it sacrifices the safety net to avoid choosing the right key.
56+
57+
### Alternative 3: Track the constraint loosely, deduplicate at query time
58+
59+
Keep the table un-constrained, then `SELECT DISTINCT` at read time when derivation reads ranges.
60+
61+
- **Pros**: Zero migration effort. Insert path becomes trivially append-only.
62+
- **Cons**: Same row-count inflation as Alternative 2, plus query overhead that scales with duplicate count. The derive step already iterates ranges for every package in the corpus (hundreds of thousands of packages); making each iteration paginate through more rows than necessary is wasteful, and ADR-0003 already calls out derivation cadence as a sensitive path.
63+
- **Why not**: Trades storage and per-query cost for an implementation shortcut.
64+
65+
## Consequences
66+
67+
### Positive
68+
69+
- `osv-plan §2 #1` ("one package has many version ranges; no denormalization") is now actually enforced by the schema, not just intended.
70+
- Cross-distro and partial-fix OSV records no longer lose ranges. `has_critical_vulnerability` derivation sees every vulnerable window OSV reports, so missed-alert false negatives caused by this specific class of OSV data are eliminated.
71+
- The dedup contract in `upsertAdvisory.ts` matches the database constraint exactly, making the invariant easy to audit ("the dedup key is the unique-index key").
72+
- The migration is idempotent in both directions — running it on a fresh DB with only the wide index is a no-op (the lookup finds nothing to drop).
73+
74+
### Negative
75+
76+
- Adds a small migration to the slice (one DO-block + one CREATE UNIQUE INDEX, ~20 LOC). Flyway has to apply it; databases with the narrow index need the drop-then-create to run successfully.
77+
- The wide index is slightly larger on disk than the narrow one (~3 columns of additional COALESCE expression evaluation per row). Negligible at OSV scale (~250k–500k rows across npm + Maven; the table is dwarfed by `packages`).
78+
- The application-side dedup key has to stay in sync with the index key. If a future migration adds another range column (e.g. introducing `last_affected_inclusive` boundaries), `dedupeRanges` needs the same column added. Captured here so future readers know to update both.
79+
80+
### Risks
81+
82+
- The `DO $$ ... $$` block uses heuristics (`indexdef` LIKE patterns) to find the old index by definition because the initial migration didn't name it. If a future deployment ever ends up with multiple unique indexes on `advisory_package_id` that match the pattern, only the first found is dropped. Mitigation: the pattern is specific (`NOT ILIKE '%fixed_version%' AND NOT ILIKE '%last_affected%'` rules out the wide index itself, and `ILIKE '%COALESCE(introduced_version%'` rules out any unrelated index). Verified by inspection against the initial schema and the new wide index; no other unique index on this table exists.
83+
- A pathological OSV record with hundreds of overlapping ranges per package could now produce hundreds of rows where the narrow index would have folded them. Real-world OSV records have at most a handful of ranges per package; the row-count blowup is bounded.
84+
- The change is forward-compatible with Postgres versioning (`pg_indexes` and dynamic SQL via `EXECUTE` are stable since 9.x), but the migration relies on `current_schema()` matching the schema the table lives in. The `packages-db` worker uses the default `public` schema (per ADR-0001), so this matches; documented here so a future schema split doesn't break the lookup silently.

docs/adr/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ Use the `/adr` skill in Claude Code to record new ADRs or query past decisions.
1212
| [ADR-0003](./0003-has-critical-vulnerability-semantics.md) | Semantics of `packages.has_critical_vulnerability` | accepted | 2026-05-27 |
1313
| [ADR-0004](./0004-standalone-bin-vs-temporal-for-batch-sub-workers.md) | Standalone bin vs Temporal for batch sub-workers in packages_worker | accepted | 2026-05-27 |
1414
| [ADR-0005](./0005-cvss-scoring-strategy.md) | CVSS scoring strategy for OSV ingestion (inline v3.1, defer v4) | accepted | 2026-05-27 |
15+
| [ADR-0006](./0006-advisory-affected-ranges-uniqueness-scope.md) | Uniqueness scope of `advisory_affected_ranges` | accepted | 2026-05-27 |
1516

1617
## Why ADRs?
1718

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import { describe, expect, it } from 'vitest'
2+
3+
import { NormalizedRange } from '../types'
4+
import { dedupeRanges } from '../upsertAdvisory'
5+
6+
// dedupeRanges is the pre-flight pass that keeps inserts from colliding on the
7+
// (advisory_package_id, introduced, fixed, last_affected) unique index. The
8+
// scope of the dedup key is the load-bearing detail — keying only on
9+
// introduced_version (the historical bug guarded by ADR-0006) silently dropped
10+
// ranges that share an introduced_version but differ in fixed_version or
11+
// last_affected, which OSV emits for cross-distro patches and partial-fix
12+
// scenarios.
13+
14+
describe('dedupeRanges', () => {
15+
it('preserves two ranges sharing introduced but with different fixed_version', () => {
16+
// Realistic OSV scenario: two affected[] blocks for the same package
17+
// patched at different upstream commits across distros.
18+
const ranges: NormalizedRange[] = [
19+
{ introducedVersion: '1.0.0', fixedVersion: '1.5.0', lastAffected: null },
20+
{ introducedVersion: '1.0.0', fixedVersion: '2.0.0', lastAffected: null },
21+
]
22+
expect(dedupeRanges(ranges)).toEqual(ranges)
23+
})
24+
25+
it('preserves two ranges sharing introduced but with different last_affected', () => {
26+
const ranges: NormalizedRange[] = [
27+
{ introducedVersion: '1.0.0', fixedVersion: null, lastAffected: '1.4.9' },
28+
{ introducedVersion: '1.0.0', fixedVersion: null, lastAffected: '1.9.9' },
29+
]
30+
expect(dedupeRanges(ranges)).toEqual(ranges)
31+
})
32+
33+
it('collapses two truly identical tuples to one', () => {
34+
// Same OSV record occasionally emits redundant events for the same line —
35+
// the Set should still fold those.
36+
const ranges: NormalizedRange[] = [
37+
{ introducedVersion: '1.0.0', fixedVersion: '2.0.0', lastAffected: null },
38+
{ introducedVersion: '1.0.0', fixedVersion: '2.0.0', lastAffected: null },
39+
]
40+
expect(dedupeRanges(ranges)).toEqual([
41+
{ introducedVersion: '1.0.0', fixedVersion: '2.0.0', lastAffected: null },
42+
])
43+
})
44+
45+
it('treats null introducedVersion the same as the empty string for keying', () => {
46+
// Two MAL-style "always vulnerable" entries with different fix lines
47+
// (rare, but the keying still has to disambiguate them).
48+
const ranges: NormalizedRange[] = [
49+
{ introducedVersion: null, fixedVersion: null, lastAffected: null },
50+
{ introducedVersion: null, fixedVersion: '1.0.0', lastAffected: null },
51+
]
52+
expect(dedupeRanges(ranges)).toEqual(ranges)
53+
})
54+
55+
it('keeps first occurrence when a tuple is repeated', () => {
56+
const first: NormalizedRange = {
57+
introducedVersion: '1.0.0',
58+
fixedVersion: '2.0.0',
59+
lastAffected: null,
60+
}
61+
const ranges: NormalizedRange[] = [
62+
first,
63+
{ introducedVersion: '1.0.0', fixedVersion: '2.0.0', lastAffected: null },
64+
]
65+
const out = dedupeRanges(ranges)
66+
expect(out).toHaveLength(1)
67+
expect(out[0]).toBe(first)
68+
})
69+
})

services/apps/packages_worker/src/osv/upsertAdvisory.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,19 @@ import { QueryExecutor } from '@crowd/data-access-layer/src/queryExecutor'
22

33
import { NormalizedRange, NormalizedRecord } from './types'
44

5-
// Drop duplicate ranges that would collide on the
6-
// (advisory_package_id, COALESCE(introduced_version,'')) unique index.
7-
// We keep the first occurrence; OSV occasionally emits redundant events for
8-
// the same introduced=X line.
9-
function dedupeRanges(ranges: NormalizedRange[]): NormalizedRange[] {
5+
// Drop duplicate ranges that would collide on the unique index over
6+
// (advisory_package_id, COALESCE(introduced_version,''),
7+
// COALESCE(fixed_version,''), COALESCE(last_affected,'')).
8+
// We key on the full range tuple so two ranges sharing an introduced_version
9+
// but differing in fixed_version or last_affected both survive — per
10+
// osv-plan §2 decision #1 ("one package has many version ranges; no
11+
// denormalization") and ADR-0006. OSV still occasionally emits redundant
12+
// events for the exact same tuple, which the Set collapses to one row.
13+
export function dedupeRanges(ranges: NormalizedRange[]): NormalizedRange[] {
1014
const seen = new Set<string>()
1115
const out: NormalizedRange[] = []
1216
for (const r of ranges) {
13-
const key = r.introducedVersion ?? ''
17+
const key = `${r.introducedVersion ?? ''}|${r.fixedVersion ?? ''}|${r.lastAffected ?? ''}`
1418
if (seen.has(key)) continue
1519
seen.add(key)
1620
out.push(r)

0 commit comments

Comments
 (0)