fix: dedup advisory_affected_ranges via diff-based upsert (CM-1258)

[themarolt]

Summary

Fixes two duplication bugs in advisory_affected_ranges for the OSV/deps.dev sync: zombie rows from hard-delete+reinsert not propagating deletes to Tinybird, and OSV/deps.dev rows for the same range never colliding.

Changes

• Add deleted_at soft-delete column to advisory_affected_ranges (migration).
• Replace the OSV worker's hard-delete + reinsert sweep with a diff-based upsert (reconcileOsvRanges): unchanged tuples are left untouched (no write, no Sequin/Tinybird event), new tuples are upserted, stale tuples are soft-deleted instead of hard-deleted.
• Add supersedeDepsDevRanges to soft-delete live deps.dev raw rows once OSV writes structured ranges for the same advisory_package, per the existing "OSV overwrites deps.dev over time" source-of-truth rule.
• Add deletedAt to the Tinybird advisoryAffectedRanges datasource and filter it in ossPackages_enriched.pipe's join — FINAL alone doesn't collapse two distinct PKs claiming the same real-world range.
• Record the decided strategy in docs/adr/0001-oss-packages-design-decisions.md.

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Performance improvement
• Chore / dependency update
• Documentation

JIRA ticket

CM-1258

[cursor]

PR Summary

Medium Risk
Touches advisory ingestion, vulnerability derivation, and Tinybird enrichment; rollout should include the documented Tinybird resnapshot, and concurrent OSV/deps.dev writes rely on new locking behavior.

Overview
Fixes duplicate advisory affected ranges in Postgres/Tinybird by replacing OSV’s hard-delete + reinsert with diff-based reconcile and deleted_at soft-delete (migration + partial live index).

OSV path: reconcileOsvRanges upserts new/changed tuples, leaves unchanged rows alone (no updated_at bump), and tombstones removed ranges; supersedeDepsDevRanges retires live deps.dev raw rows once OSV owns the package. upsertAdvisory also reconciles advisory_packages dropped from the payload, locks advisory_packages in id order, and coordinates with deps.dev via FOR UPDATE.

deps.dev path: Range merge skips packages that already have live OSV structured ranges; can revive soft-deleted raw tuples; chunk merge runs ADVISORY_PACKAGES_LOCK_SQL before writes.

Read paths: API/DAL joins and deriveCriticalFlag range loads filter deleted_at IS NULL. Tinybird adds deletedAt and filters it in ossPackages_enriched (ADR notes a one-time resnapshot for existing zombie rows).

^{Reviewed by Cursor Bugbot for commit 77d4017. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

Pull request overview

Fixes duplicate and stale advisory ranges across PostgreSQL and Tinybird.

Changes:

• Introduces soft deletion and diff-based OSV range reconciliation.
• Supersedes deps.dev ranges when OSV data exists.
• Filters deleted ranges in Tinybird and documents the strategy.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
ossPackages_enriched.pipe	Excludes deleted ranges.
advisoryAffectedRanges.datasource	Adds deletedAt.
packages/osv.ts	Reconciles and soft-deletes ranges.
osv/upsertAdvisory.ts	Uses the new reconciliation flow.
0001-oss-packages-design-decisions.md	Records the strategy.
V1784567585__advisory_affected_ranges_soft_delete.sql	Adds soft-delete schema support.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

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

• Mandatory: add database-backed coverage for this new reconciliation flow. The existing upsertAdvisory.test.ts only tests dedupeRanges, so regressions in the SQL/orchestration would go undetected. Cover unchanged tuples producing no update, stale tuples being soft-deleted, a deleted tuple being reactivated, and deps.dev raw rows being superseded while readers exclude deleted rows.

    await reconcileOsvRanges(
      qx,
      advisoryPackageId,
      dedupeRanges(entry.ranges).map((range) => ({

docs/adr/0001-oss-packages-design-decisions.md:367

• This newly recorded architecture decision does not follow the repository's ADR contract. .claude/rules/adr-format.md:18-28 requires ## Decision, ## Alternatives Considered, and ## Consequences (plus an allowed ADR status), but this is added as another subsection in the legacy living document without those sections. Record the decision as the next standalone ADR and add it to docs/adr/README.md, or bring ADR-0001 into the required structure.

### `advisory_affected_ranges` delete/dedup strategy

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

docs/adr/0001-oss-packages-design-decisions.md:367

• This new decision is appended as an ad-hoc subsection, but .claude/rules/adr-format.md:18-28 requires numbered ADRs to include the standard Context, Decision, Alternatives Considered, and Consequences sections and an allowed status. Please record this as a conforming ADR (and update the ADR index), or bring ADR-0001 into the required format while editing it.

### `advisory_affected_ranges` delete/dedup strategy

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

• The core behavior introduced here is not exercised by the existing upsertAdvisory tests, which only cover dedupeRanges. Please add database-backed coverage for unchanged tuples producing no update, stale tuples being soft-deleted, deleted tuples being revived, and deps.dev rows being superseded; otherwise regressions in the SQL that implements this bug fix will pass the current suite.

    await reconcileOsvRanges(
      qx,
      advisoryPackageId,
      dedupeRanges(entry.ranges).map((range) => ({
        advisoryPackageId,

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

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

• Sorting here is too late to establish a canonical lock order. Each earlier upsertAdvisoryPackage executes ON CONFLICT ... DO UPDATE (services/libs/data-access-layer/src/packages/osv.ts:132-135), which already takes and retains an advisory_packages row lock in OSV payload order. A concurrent deps.dev transaction can therefore lock the lower ID, wait on OSV's higher ID, while OSV's next upsert waits on the lower ID—the same deadlock this sort is intended to prevent. Acquire/lock the existing rows in ID order before any updating upsert, or add reliable deadlock retries around both transactions.

  entries.sort((a, b) => a.advisoryPackageId - b.advisoryPackageId)

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

• The new database reconciliation paths are untested: this module's current suite only exercises dedupeRanges, so regressions in unchanged-row no-ops, stale-row soft deletion, tombstone revival, deps.dev supersession, or transaction ordering would not be detected. Add database-backed scenarios for these state transitions, especially because the implementation relies on ON CONFLICT predicates and cross-writer locking.

    await reconcileOsvRanges(qx, advisoryPackageId, ranges)
    await supersedeDepsDevRanges(qx, advisoryPackageId)

docs/adr/0001-oss-packages-design-decisions.md:375

• This records a new architectural decision in an ADR that does not satisfy the repository's current ADR contract: .claude/rules/adr-format.md:18-28 requires an allowed status plus ## Decision, ## Alternatives Considered, and ## Consequences sections. ADR-0001 still uses the unsupported living status and lacks the required alternatives/consequences structure. Record CM-1258 as a new sequential ADR using the required template (and add it to docs/adr/README.md), or migrate this file to the required format.

**Decided fix — diff-based upsert with soft-delete, not sweep-and-reinsert:**

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

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

• Sorting here does not establish the claimed canonical lock order. The preceding upsertAdvisoryPackage calls execute ON CONFLICT ... DO UPDATE (services/libs/data-access-layer/src/packages/osv.ts:132-139), so they already acquire and retain locks on existing advisory_packages rows in arbitrary OSV payload order. OSV can therefore lock a higher ID first while deps.dev locks the lower ID first, recreating the deadlock this block is intended to prevent; deps.dev's maximumAttempts: 1 then makes the merge fail. Acquire/update these rows in a shared canonical order before any lock-taking upsert, or retry the whole idempotent transaction on 40P01.

  entries.sort((a, b) => a.advisoryPackageId - b.advisoryPackageId)

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

• The newly added absent-package reconciliation path is not covered by the integration test: that test calls reconcileOsvRanges directly and never exercises findRemovedAdvisoryPackageIds through upsertAdvisoryBatch. A regression for an OSV record whose packages becomes empty would therefore leave prior ranges live without failing tests. Add a DB-backed workflow test that syncs a package, resyncs the advisory without it, and verifies the old OSV ranges are tombstoned while deps.dev rows remain eligible.

  const removedIds = await findRemovedAdvisoryPackageIds(
    qx,
    advisoryId,
    entries.map((e) => e.advisoryPackageId),
  )

docs/adr/0001-oss-packages-design-decisions.md:367

• This edit leaves ADR-0001 outside the mandatory ADR format. .claude/rules/adr-format.md:18-28 permits only proposed, accepted, deprecated, or superseded by ADR-NNNN status and requires ## Decision, ## Alternatives Considered, and ## Consequences; this file still uses Status: living and a ## Decisions layout. Bring it into the required format or move the living design log outside docs/adr/ before extending it.

### `advisory_affected_ranges` delete/dedup strategy

[joanagmaia]

LGTM! I think it addresses both issues as it should. As for Tinybird yeah I would say the easiest way woulld be to truncate the existing datasource, and then trigger a backfill from Sequin.
You can check this documentation https://github.com/linuxfoundation/crowd.dev/blob/main/services/libs/tinybird/README.md on how to achieve both

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

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

• The all-packages-removed path still never reaches this query: osvSyncEcosystem drops normalized records with packages.length === 0 at activities.ts:121-129 instead of buffering them. Therefore an OSV correction that removes every usable package/range leaves the previous live ranges untouched. Send empty normalized records through the upsert path and cover that flow end-to-end.

  const removedIds = await findRemovedAdvisoryPackageIds(
    qx,
    advisoryId,
    entries.map((e) => e.advisoryPackageId),
  )

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

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

• The all-packages-removed case still never reaches this reconciliation path. osvSyncEcosystem skips records with normalized.packages.length === 0 at services/apps/packages_worker/src/osv/activities.ts:121-129, so when upstream drops the final package, these empty-set removals are not processed and the old OSV ranges remain live. Empty normalized records must be passed through upsertAdvisoryBatch (with a regression test for removing the last package).

  // Reconcile them too, with an empty range set — reconcileOsvRanges soft-deletes
  // every live OSV row when nothing in the new set matches. Not superseding
  // deps.dev here: OSV no longer covers the package, so a live deps.dev row (if
  // any) should stay live rather than be torn down alongside it.

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

• This sort happens after upsertAdvisoryPackage has already run for every entry. That function uses ON CONFLICT ... DO UPDATE (services/libs/data-access-layer/src/packages/osv.ts:152-159), which acquires and retains the same row locks in arbitrary payload order. For example, OSV can lock ID 2, deps.dev's ordered lock query can lock ID 1 and wait on 2, then OSV waits on 1, recreating the deadlock this block intends to prevent. Acquire the existing package-row locks before any conflict-updates in canonical order, or add deadlock retries around the transactions.

  entries.sort((a, b) => a.advisoryPackageId - b.advisoryPackageId)

backend/src/osspckgs/migrations/V1784567585__advisory_affected_ranges_soft_delete.sql:1

• The new migration version is older than existing packages-db migrations such as V1784745489__sequin_backfill_sort_column_indexes.sql. Project migration guidance requires a new version to be greater than all existing versions (.claude/skills/review-pr/references/sql-checklist.md:19-21). Rename this unapplied migration with a fresh epoch so migration ordering remains unambiguous.

ALTER TABLE advisory_affected_ranges ADD COLUMN IF NOT EXISTS deleted_at TIMESTAMPTZ;

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

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

• The all-packages-removed path is still unreachable. osvSyncEcosystem drops normalized records when packages.length === 0 instead of buffering them (services/apps/packages_worker/src/osv/activities.ts:121-125), so this reconciliation never runs when an upstream correction removes the final package. The previously live ranges therefore remain false positives. Pass an explicit removal signal or suitable metadata-only records through to upsertAdvisoryBatch, while preserving the intended handling of records filtered only because their ecosystem is unsupported.

  const removedIds = await findRemovedAdvisoryPackageIds(
    qx,
    advisoryId,
    entries.map((e) => e.advisoryPackageId),
  )

backend/src/osspckgs/migrations/V1784567585__advisory_affected_ranges_soft_delete.sql:1

• This migration version is lower than existing V1784745489__sequin_backfill_sort_column_indexes.sql. The repository migration rule requires each new version to be greater than all existing versions (.claude/skills/review-pr/references/sql-checklist.md:19-21). Rename this migration with a newer unique epoch so its ordering remains unambiguous.

ALTER TABLE advisory_affected_ranges ADD COLUMN IF NOT EXISTS deleted_at TIMESTAMPTZ;

docs/adr/0001-oss-packages-design-decisions.md:367

• This decision is being added to an ADR that still uses the unsupported living status and lacks the mandatory ## Decision, ## Alternatives Considered, and ## Consequences structure. .claude/rules/adr-format.md:18-29 requires those sections and an allowed status whenever a numbered ADR is edited. Record CM-1258 in a new compliant ADR (and update the index), or migrate ADR-0001 to the required format.

### `advisory_affected_ranges` delete/dedup strategy

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

• advisory_packages.id is bigserial, and the packages DB does not register an OID 20 parser, so these IDs are runtime strings (services/libs/database/src/connection.ts:75-85; V1779710880__initial_schema.sql:612-615). The new numeric subtraction used to establish canonical lock order therefore coerces them through IEEE-754; distinct bigint IDs can eventually compare equal and invalidate the deadlock-prevention invariant. Type IDs as strings end-to-end and compare them with BigInt, consistent with ADR-0006's schema-aligned type rule.

  entries.sort((a, b) => a.advisoryPackageId - b.advisoryPackageId)

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

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

• The canonical sort happens after upsertAdvisoryPackage has already executed INSERT ... ON CONFLICT DO UPDATE for every entry. Those updates lock existing advisory_packages rows until commit, so OSV still acquires locks in arbitrary payload order before reaching this sort. If deps.dev locks a lower ID while OSV has already updated a higher ID, each transaction can wait for the other's next row and deadlock; deps.dev's maximumAttempts: 1 then turns this into a failed merge. Establish the canonical order before any conflict-upsert acquires row locks, or add an idempotent deadlock retry.

  entries.sort((a, b) => a.advisoryPackageId - b.advisoryPackageId)

services/libs/data-access-layer/src/packages/osv.ts:193

• Number(r.id) is the precision-losing conversion that the preceding comment warns about. advisory_packages.id is bigint/OID 20, and this DAL already preserves such IDs as strings to avoid silent precision loss (osv.ts:125-126; osspckgs/types.ts:4). Once an ID exceeds Number.MAX_SAFE_INTEGER, reconciliation can lock or update a rounded, incorrect row. Keep these IDs as strings end-to-end and compare their numeric values with BigInt when sorting.

  return rows.map((r: { id: string }) => Number(r.id))

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

• The absent-package reconciliation path is not covered by the added integration suite: those tests call reconcileOsvRanges directly and never exercise findRemovedAdvisoryPackageIds through upsertAdvisoryBatch. Add a case where an advisory is first synced with a package and then synced without it, asserting that the old OSV range is tombstoned and a deps.dev raw row remains live. This is the regression path that prevents removed packages from remaining false positives.

  const removedIds = await findRemovedAdvisoryPackageIds(
    qx,
    advisoryId,
    entries.map((e) => e.advisoryPackageId),
  )

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 77d4017. Configure here.}

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

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

• Sorting here does not establish a canonical lock order: each earlier upsertAdvisoryPackage uses ON CONFLICT DO UPDATE, which already takes and holds a row lock in the original OSV payload order. An OSV transaction can therefore lock a higher ID during upsert, while deps.dev locks a lower ID first, recreating the exact deadlock this sort is intended to prevent. Acquire existing advisory-package locks in canonical order before any conflicting updates, or add a transaction-level deadlock retry.

  entries.sort((a, b) => a.advisoryPackageId - b.advisoryPackageId)

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

• The all-packages-removed case still never reaches this reconciliation. osvSyncEcosystem discards every normalized record whose packages.length === 0 at services/apps/packages_worker/src/osv/activities.ts:121-125, so removing upsertOne's early return does not make an empty corrected record call findRemovedAdvisoryPackageIds; its old ranges remain live and continue blocking deps.dev. Pass these records through to upsertAdvisoryBatch (while preserving the intended filtering semantics) and cover this end-to-end case.

  const removedIds = await findRemovedAdvisoryPackageIds(
    qx,
    advisoryId,
    entries.map((e) => e.advisoryPackageId),
  )