fix: resolve advisory_packages join in postgres, skip 1.5TB purl scan (CM-1362) - #4433
Conversation
… (CM-1362) Signed-off-by: Uroš Marolt <uros@marolt.me>
PR SummaryMedium Risk Overview Advisory ingest: Resilience: Bug fix: Reviewed by Cursor Bugbot for commit befa6d6. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Reduces advisory-package BigQuery scanning by resolving package identities in PostgreSQL while allowing downstream jobs to continue after ceiling breaches.
Changes:
- Removes the costly BigQuery purl lookup and adds shared package-name parsing.
- Resolves package IDs through PostgreSQL’s indexed package identity.
- Adds typed, non-retryable ceiling failures and lowers the scan limit.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
services/libs/data-access-layer/src/packages/osv.ts |
Fixes scoped npm identity reconstruction. |
services/apps/packages_worker/src/deps-dev/workflows/ingestPackages.ts |
Uses shared identity parsing. |
services/apps/packages_worker/src/deps-dev/workflows/ingestAdvisories.ts |
Resolves package IDs in PostgreSQL and handles ceiling failures. |
services/apps/packages_worker/src/deps-dev/workflows/bootstrapOsspckgs.ts |
Soft-fails advisory ceiling breaches. |
services/apps/packages_worker/src/deps-dev/README.md |
Documents revised scan ceilings. |
services/apps/packages_worker/src/deps-dev/queries/pgIdentity.ts |
Adds shared identity SQL generation. |
services/apps/packages_worker/src/deps-dev/queries/advisoriesSql.ts |
Removes the BigQuery purl scan. |
services/apps/packages_worker/src/deps-dev/activities/bqExportToGcs.ts |
Makes ceiling failures typed and non-retryable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
services/libs/data-access-layer/src/packages/osv.ts:316
- The scoped-npm catch-up fix has no regression coverage. The existing
resolveMissingPackageIdsintegration case only uses an npm package withnamespace: null, so it would pass with the old double-@expression. Add a scoped fixture (for example namespace@types, namenode, package name@types/node) and assert thatpackage_idresolves.
WHEN p.ecosystem = 'npm' THEN p.namespace || '/' || p.name
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:20
- This alert runs after the advisory-header step has already merged rows, so “Existing data untouched” is inaccurate; only the advisory-package step was skipped. The message can mislead operators investigating a partially completed run.
text: `Ingest skipped for this run so scorecard/ranking still complete. Existing data untouched. ${input.message}`,
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:20
- The ceiling breach occurs after Step 1 has already merged advisory header rows, so “Existing data untouched” is inaccurate and may mislead operators. State specifically that advisory-package/range data was preserved and that advisory headers may already have been ingested.
text: `Ingest skipped for this run so scorecard/ranking still complete. Existing data untouched. ${input.message}`,
Signed-off-by: Uroš Marolt <uros@marolt.me>
…-bq-ceiling-CM-1362
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:13
sendSlackNotificationis explicitly fire-and-forget, so this Temporal activity can complete before the webhook request finishes. A worker shutdown after activity completion can lose the only alert before the parent swallows the ceiling failure. AwaitsendSlackNotificationAsyncso activity completion follows the send attempt.
export async function notifyBqCeilingSkip(input: NotifyBqCeilingSkipInput): Promise<void> {
sendSlackNotification(
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:20
- The alert says existing data was untouched, but advisory headers are exported and merged before the advisory-packages ceiling check. A breach can therefore leave newly inserted
advisoriesrows while preserving the prior package/range data; the current message gives operators an inaccurate recovery state.
text: `Ingest skipped for this run so scorecard/ranking still complete. Existing data untouched. ${input.message}`,
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:20
- For an
advisory_packagesceiling breach, the advisories header export has already been loaded, so “Existing data untouched” is inaccurate and can mislead operators assessing a partial run. State that earlier ingest steps may have completed.
text: `Ingest skipped for this run so scorecard/ranking still complete. Existing data untouched. ${input.message}`,
mbani01
left a comment
mbani01
left a comment
There was a problem hiding this comment.
@themarolt LGTM, just a suggestion to change target slack channel
…M-1362) Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:13
- This activity completes before Slack delivery because
sendSlackNotificationis explicitly fire-and-forget. Temporal therefore records the alert activity as successful while the only operational signal is still pending and can be lost if the worker stops. AwaitsendSlackNotificationAsyncso the activity does not complete until the send attempt finishes.
sendSlackNotification(
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
services/apps/packages_worker/src/deps-dev/workflows/ingestAdvisories.ts:40
- This comment embeds review history and spans seven lines, contrary to the repository rule that comments should capture only non-obvious rationale in at most two lines. Condense it to the durable reason for preserving the failure type and job kind.
// bqExportToGcs throws ApplicationFailure.nonRetryable('BQ_CEILING_EXCEEDED') directly from
// activity code, which the SDK surfaces here as ActivityFailure.cause. Rethrow as a workflow-level
// ApplicationFailure carrying the job kind as a detail so bootstrapOsspckgs's unwrap (err.cause on
// the resulting ChildWorkflowFailure) can match it, soft-fail, and alert on whichever export
// actually breached — mirroring the DEPENDENT_COUNTS_GUARD / EDGE_SNAPSHOT_GUARD pattern there.
…-bq-ceiling-CM-1362
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 c0dd769. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/services/bq-dataset-ingest.yaml:8
- This namespace-default change, repeated across eight worker compose files, is unrelated to the advisory-package scan described by CM-1362 and changes local worker routing behavior. The repository requires PRs to contain one focused bug fix (
.claude/rules/commit-workflow.md:58-61); please move these compose changes to a separately tracked PR or document why they are required for this fix.
CROWD_TEMPORAL_NAMESPACE: ${CROWD_PACKAGES_TEMPORAL_NAMESPACE:-default}
services/libs/data-access-layer/src/packages/osv.ts:341
- The existing catch-up integration test covers only an unscoped npm name (
cxp-jquery), so it would not catch a regression of the scoped-name bug fixed here. Add a fixture wherenamespace = '@scope'andpackage_name = '@scope/name', then assert this update resolves itspackage_id.
WHEN p.ecosystem = 'npm' THEN p.namespace || '/' || p.name
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:13
- This activity returns before the Slack request completes because
sendSlackNotificationis explicitly fire-and-forget. Temporal can therefore mark the alert activity successful and continue after swallowing the ceiling failure even if the process stops before delivery; the configured activity retries also cannot observe the send. AwaitsendSlackNotificationAsync, as the packages-worker activity interceptor does, so the delivery attempt finishes before this activity succeeds.
sendSlackNotification(
…eiling breach (CM-1362) Signed-off-by: Uroš Marolt <uros@marolt.me>
…-bq-ceiling-CM-1362
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:22
- This Temporal activity uses the fire-and-forget Slack API, so it can be recorded successful before the webhook request completes; a worker shutdown can lose the only signal after the parent swallows the ceiling failure. Await
sendSlackNotificationAsyncinstead, as the packages-worker activity interceptor does, so activity completion follows the delivery attempt.
sendSlackNotification(
services/libs/data-access-layer/src/packages/osv.ts:341
- The existing catch-up integration test only exercises an unscoped npm package, so it would pass with the old double-
@expression. Add a scoped fixture with namespace@scopeand package name@scope/name, then assert thatresolveMissingPackageIdspopulates itspackage_id.
WHEN p.ecosystem = 'npm' THEN p.namespace || '/' || p.name
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (8)
services/apps/packages_worker/src/deps-dev/workflows/ingestAdvisories.ts:97
- This six-line block documents the change rather than a lasting invariant and exceeds the two-line limit. Retain only the performance constraint that prevents reintroducing the expensive lookup.
// package_id is resolved here by reconstructing the same (ecosystem, namespace, name) identity
// ingestPackages.ts writes into `packages` (packageNameSplitSql, shared by both), rather than by
// a BQ-sourced purl (CM-1362 — the purl_map scan cost ~1.5 TB per run for a join key we already
// had locally). COALESCE(p.namespace,'') mirrors the unique index expression verbatim, so this
// stays an index lookup. Still a LEFT JOIN: package_id stays nullable, resolveMissingPackageIds
services/apps/packages_worker/src/deps-dev/workflows/bootstrapOsspckgs.ts:335
- This block exceeds the two-line comment limit and includes review-history details. Condense it to the operational reason for the explicit alert.
// Unlike checkDependentCountsGuard/checkEdgeSnapshotQuality, this failure happens before
// any ingest-job row is created, so there's no failed-job row for an operator to notice —
// alert explicitly or repeated skips go unnoticed (review comment on CM-1362).
// ingestAdvisories carries the failing export's jobKind as the failure detail ('advisories'
// or 'advisory_packages') so the alert names the export that actually breached.
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:17
- This five-line comment narrates the branch immediately below and includes review history. The two impact strings are already self-explanatory, so remove the block.
// ingestAdvisories merges `advisories` to Postgres before exporting `advisory_packages`
// (bootstrapOsspckgs.ts), so a breach on the latter still leaves this run's new advisory rows
// committed with no package links — "existing data untouched" would misstate that (bugbot
// review on CM-1362). A breach on `advisories` itself precedes any write this run, so that
// claim stays accurate there.
services/apps/packages_worker/src/deps-dev/workflows/ingestAdvisories.ts:40
- This implementation note exceeds the two-line comment limit. Keep only the non-obvious Temporal wrapping invariant; the function already shows the rethrow and detail propagation.
This issue also appears on line 93 of the same file.
// bqExportToGcs throws ApplicationFailure.nonRetryable('BQ_CEILING_EXCEEDED') directly from
// activity code, which the SDK surfaces here as ActivityFailure.cause. Rethrow as a workflow-level
// ApplicationFailure carrying the job kind as a detail so bootstrapOsspckgs's unwrap (err.cause on
// the resulting ChildWorkflowFailure) can match it, soft-fail, and alert on whichever export
// actually breached — mirroring the DEPENDENT_COUNTS_GUARD / EDGE_SNAPSHOT_GUARD pattern there.
services/apps/packages_worker/src/deps-dev/workflows/bootstrapOsspckgs.ts:326
- This explanation exceeds the two-line comment limit. The only non-obvious constraint is which failure may be swallowed; the surrounding control flow communicates the rest.
This issue also appears on line 331 of the same file.
// Only soft-fail on the BQ byte-ceiling guard (CM-1362), mirroring the dependent_counts /
// package_dependencies handling above. advisories is the last data kind — letting a ceiling
// breach here propagate unhandled used to strand scorecard + ranking below for the whole
// run. All other errors (BQ timeout, DB failure, etc.) still propagate.
services/apps/packages_worker/src/deps-dev/queries/pgIdentity.ts:3
- This header restates what the helper and its call sites do. Removing it keeps the new utility self-explanatory and avoids a change-history comment.
// The (namespace, name) split ingestPackages.ts writes into `packages` (see MERGE_SQL there).
// Any consumer that needs to join back onto that identity by name must derive it identically,
// so both sides share this one definition instead of drifting apart.
services/apps/packages_worker/src/deps-dev/queries/advisoriesSql.ts:19
- This change-history block exceeds the two-line comment limit. Keep only the durable performance constraint that explains why the apparently simpler BigQuery join must not return.
// No purl here — package_id is resolved in Postgres against the already-ingested `packages`
// table (see ADVISORY_PACKAGES_MERGE_SQL in workflows/ingestAdvisories.ts). Pulling purl from
// BigQuery required scanning all of PackageVersionsLatest (~1.5 TB) just for a join key we
// already have locally (CM-1362).
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:11
- This block exceeds the two-line comment limit and records review history and nearby-file comparisons. The function name and selected channel already make its purpose clear, so remove it.
This issue also appears on line 13 of the same file.
// The advisories ceiling breach is caught one level up in bootstrapOsspckgs instead of inside a
// guard activity, so it needs its own alert call to keep repeated skips from going unnoticed
// (CM-1362 review). Uses CDP_AKRITES_ALERTS — the packages_worker team's own channel (matches
// service.ts / blast-radius-worker.ts), per reviewer request — rather than CDP_CRITICAL_ALERTS.
…-bq-ceiling-CM-1362
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (9)
services/apps/packages_worker/src/deps-dev/workflows/ingestAdvisories.ts:97
- This six-line change narrative exceeds the repository's two-line limit for comments (
CLAUDE.md:72-84). Keep only the non-obvious index-alignment constraint.
// package_id is resolved here by reconstructing the same (ecosystem, namespace, name) identity
// ingestPackages.ts writes into `packages` (packageNameSplitSql, shared by both), rather than by
// a BQ-sourced purl (CM-1362 — the purl_map scan cost ~1.5 TB per run for a join key we already
// had locally). COALESCE(p.namespace,'') mirrors the unique index expression verbatim, so this
// stays an index lookup. Still a LEFT JOIN: package_id stays nullable, resolveMissingPackageIds
services/apps/packages_worker/src/deps-dev/workflows/bootstrapOsspckgs.ts:335
- This review-history block exceeds the two-line limit in
CLAUDE.md:72-84. Reduce it to the non-obvious failure-detail contract between the child and parent workflows.
// Unlike checkDependentCountsGuard/checkEdgeSnapshotQuality, this failure happens before
// any ingest-job row is created, so there's no failed-job row for an operator to notice —
// alert explicitly or repeated skips go unnoticed (review comment on CM-1362).
// ingestAdvisories carries the failing export's jobKind as the failure detail ('advisories'
// or 'advisory_packages') so the alert names the export that actually breached.
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:17
- This five-line implementation-history note exceeds the two-line limit in
CLAUDE.md:72-84. Keep only the ordering invariant needed to understand the different alert text.
// ingestAdvisories merges `advisories` to Postgres before exporting `advisory_packages`
// (bootstrapOsspckgs.ts), so a breach on the latter still leaves this run's new advisory rows
// committed with no package links — "existing data untouched" would misstate that (bugbot
// review on CM-1362). A breach on `advisories` itself precedes any write this run, so that
// claim stays accurate there.
services/apps/packages_worker/src/deps-dev/queries/pgIdentity.ts:3
- This explanatory block exceeds the repository's two-line limit for allowed comments (
CLAUDE.md:72-84). Keep only the non-obvious cross-consumer invariant; the helper name and implementation already describe the split.
// The (namespace, name) split ingestPackages.ts writes into `packages` (see MERGE_SQL there).
// Any consumer that needs to join back onto that identity by name must derive it identically,
// so both sides share this one definition instead of drifting apart.
services/apps/packages_worker/src/deps-dev/queries/advisoriesSql.ts:19
- This is a multi-line note about the current change, which
CLAUDE.md:72-84disallows. Retain only the performance constraint that would otherwise look removable.
// No purl here — package_id is resolved in Postgres against the already-ingested `packages`
// table (see ADVISORY_PACKAGES_MERGE_SQL in workflows/ingestAdvisories.ts). Pulling purl from
// BigQuery required scanning all of PackageVersionsLatest (~1.5 TB) just for a join key we
// already have locally (CM-1362).
services/apps/packages_worker/src/deps-dev/workflows/ingestAdvisories.ts:40
- This seven-line implementation/review note violates the two-line comment limit in
CLAUDE.md:72-84. Condense it to the non-obvious Temporal wrapping contract.
This issue also appears on line 93 of the same file.
// bqExportToGcs throws ApplicationFailure.nonRetryable('BQ_CEILING_EXCEEDED') directly from
// activity code, which the SDK surfaces here as ActivityFailure.cause. Rethrow as a workflow-level
// ApplicationFailure carrying the job kind as a detail so bootstrapOsspckgs's unwrap (err.cause on
// the resulting ChildWorkflowFailure) can match it, soft-fail, and alert on whichever export
// actually breached — mirroring the DEPENDENT_COUNTS_GUARD / EDGE_SNAPSHOT_GUARD pattern there.
services/apps/packages_worker/src/deps-dev/workflows/bootstrapOsspckgs.ts:326
- This four-line description of the newly added catch exceeds the two-line comment limit in
CLAUDE.md:72-84. The shorter form preserves the operational reason without documenting the change history.
This issue also appears on line 331 of the same file.
// Only soft-fail on the BQ byte-ceiling guard (CM-1362), mirroring the dependent_counts /
// package_dependencies handling above. advisories is the last data kind — letting a ceiling
// breach here propagate unhandled used to strand scorecard + ranking below for the whole
// run. All other errors (BQ timeout, DB failure, etc.) still propagate.
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:22
- This activity returns while the Slack request is still in flight because
sendSlackNotificationis explicitly fire-and-forget (services/libs/slack/src/notify.ts:207-225). Temporal can therefore record the notification activity as successful before the alert finishes, so a worker shutdown can lose the only signal for a swallowed ceiling breach. AwaitsendSlackNotificationAsync, as the existing Temporal alert activity does inservices/apps/packages_worker/src/activities/index.ts:6-13.
sendSlackNotification(
services/apps/packages_worker/src/deps-dev/activities/notifyBqCeilingSkip.ts:11
- This four-line note records change/reviewer history rather than a non-obvious invariant, contrary to
CLAUDE.md:72-84. The function and channel enum already make the behavior clear.
This issue also appears on line 13 of the same file.
// The advisories ceiling breach is caught one level up in bootstrapOsspckgs instead of inside a
// guard activity, so it needs its own alert call to keep repeated skips from going unnoticed
// (CM-1362 review). Uses CDP_AKRITES_ALERTS — the packages_worker team's own channel (matches
// service.ts / blast-radius-worker.ts), per reviewer request — rather than CDP_CRITICAL_ALERTS.
Summary
bootstrapOsspckgshas failed its last 5 weekly runs atingestAdvisories— a BQ dry-run guard aborts becauseadvisory_packagesscans ~1.5 TB. Root cause: apurl_mapCTE joins the entirePackageVersionsLatesttable just to attach apurlused for one join key, which we already have locally in Postgres. This drops the BQ scan for that step to ~1.4 GB and stops the failure from strandingscorecard/ranking downstream.Changes
purl_mapCTE frombuildAdvisoryPackagesSql— no BQ-side purl lookup.advisory_packages.package_idin Postgres instead, joining on the same(ecosystem, namespace, name)identitypackagesalready carries a unique index on (COALESCE(namespace,'')matches the index expression, so this stays an index lookup, not a scan).queries/pgIdentity.ts(packageNameSplitSql) so the namespace/name split logic isn't duplicated betweeningestPackages.tsandingestAdvisories.ts.purlcolumn from theadvisory_packagesstaging DDL/columns; switch that DDL toDROP TABLE IF EXISTS+CREATE(matchesingestDependentCounts.ts) so a stalepurlcolumn doesn't silently surviveCREATE ... IF NOT EXISTS.BQ_DATASET_INGEST_ADVISORY_PACKAGES_MAX_BQ_GB1500 → 50 (measured actual ~1.4 GB; keeps the ceiling a real regression gate).bqExportToGcsnow throws a typedApplicationFailure.nonRetryable(..., 'BQ_CEILING_EXCEEDED')on ceiling breach instead of a plainError(a dry-run byte count is deterministic, so retrying is pure waste).ingestAdvisoriesunwraps that fromActivityFailureand rethrows a workflow-levelApplicationFailureof the same type;bootstrapOsspckgsnow soft-fails only on that type around the advisories step (mirrors the existingdependent_counts/package_dependenciesguards) — a ceiling breach no longer strandsscorecard/ranking for the whole run.resolveMissingPackageIds(osv.ts:316) built'@' || namespace || '/' || namefor npm, butnamespacealready contains the@— every scoped npm advisory package was silently unresolvable in this catch-up. One-line fix, same identity-reconstruction logic as the rest of this PR.Type of change
JIRA ticket
https://linuxfoundation.atlassian.net/browse/CM-1362