feat: packagist transitive dependent counts - #4422
Conversation
Signed-off-by: anilb <[email protected]>
|
Your PR title doesn't contain a Jira issue key. Consider adding it for better traceability. Example:
Projects:
Please add a Jira issue key to your PR title. |
|
|
PR SummaryMedium Risk Overview Data path: DAL snapshots packagist Orchestration: New Temporal workflows Also documents the decision in ADR-0009, adds Reviewed by Cursor Bugbot for commit 3076a1d. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds weekly Packagist transitive-dependent computation and integrates it with the packages worker.
Changes:
- Adds PostgreSQL snapshot, closure, merge, and run-ledger DAL operations.
- Adds Temporal workflow/activity orchestration, manual triggering, and metadata-drain chaining.
- Adds migrations, tests, ADR updates, and operational documentation.
Review notes: The PR has five unresolved findings. Its title also lacks the required JIRA key, and the diff exceeds the recommended 1,000-line target.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
services/libs/data-access-layer/tsconfig.json |
Enables BigInt test syntax. |
services/libs/data-access-layer/src/packages/transitiveDependents.ts |
Implements snapshot, closure, and merge queries. |
services/libs/data-access-layer/src/packages/transitiveDependents.integration.test.ts |
Tests graph and ledger behavior. |
services/libs/data-access-layer/src/packages/packagistTransitiveRuns.ts |
Adds run-ledger operations. |
services/libs/data-access-layer/src/packages/index.ts |
Exports new DAL modules. |
services/libs/data-access-layer/src/osspckgs/ingestJobs.ts |
Adds pending-job lookup. |
services/apps/packages_worker/src/workflows/index.ts |
Exports the workflow. |
services/apps/packages_worker/src/scripts/triggerPackagistSeed.ts |
Adds manual transitive trigger. |
services/apps/packages_worker/src/packagist/workflows.ts |
Orchestrates preparation and merge draining. |
services/apps/packages_worker/src/packagist/README.md |
Documents the new lane. |
services/apps/packages_worker/src/packagist/activities.ts |
Implements Temporal activities. |
services/apps/packages_worker/src/packagist/__tests__/wiring.test.ts |
Verifies worker exports. |
services/apps/packages_worker/src/packagist/__tests__/transitiveDependents.test.ts |
Tests workflow orchestration. |
services/apps/packages_worker/src/criticality/activities.ts |
Reuses pending-job lookup. |
services/apps/packages_worker/src/activities.ts |
Registers new activities. |
docs/adr/0009-packagist-worker-design-decisions.md |
Records the architecture decision. |
backend/src/osspckgs/migrations/V1785740540__packagist_transitive_runs.sql |
Creates the run ledger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: anilb <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
services/libs/data-access-layer/src/osspckgs/ingestJobs.ts:80
- This coerces a PostgreSQL
bigserialidentifier to a JavaScript number even though the packages-db connection intentionally leaves int8 values as strings. Once an ID exceedsNumber.MAX_SAFE_INTEGER, the rounded value can update or query the wrong job. Keep the ID as a string and aligncreateIngestJob,markJobStatus, and their callers with that representation.
// id is bigserial (pg returns int8 as a string) — convert so the declared type is true.
return row ? Number(row.id) : null
services/apps/packages_worker/src/packagist/workflows.ts:70
batch.changedis not retry-stable. If the merge activity commits its UPDATE but Temporal loses the completion, the retry processes the same cursor after the rows already match and returnschanged: 0; this accumulator then permanently under-reportschanged_rows. Persist per-batch/cumulative change counts atomically with the merge (keyed by run/cursor), or otherwise make the returned count deterministic across activity retries.
const batch = await acts.mergePackagistTransitiveBatch(cursor, TRANSITIVE_MERGE_BATCH)
processed += batch.processed
changed += batch.changed
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
services/libs/data-access-layer/src/packages/transitiveDependents.ts:23
statement_timeoutapplies to each statement, not the whole prepare activity. This helper runs CTAS, index creation, and ANALYZE twice sequentially, so a valid attempt can exceed the 45-minute start-to-close timeout while its transaction remains active. Temporal may then retry the full scan concurrently and contend on these global staging tables. Split the phases into separately timed activities or enforce an end-to-end database deadline that terminates the original attempt before Temporal retries it.
await tx.result(`SET LOCAL statement_timeout = '40min'`)
docs/adr/0009-packagist-worker-design-decisions.md:191
- This edit leaves ADR-0009 outside the repository's mandatory ADR structure: it has
## Decisionsrather than## Decisionand lacks## Alternatives Consideredplus the required## Consequencessubsections. Please bring the ADR into compliance with.claude/rules/adr-format.md:18-29while updating it.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
services/apps/packages_worker/src/packagist/activities.ts:520
- This marks the ledger row
failedafter every failed activity attempt. Since Temporal retries prepare up to three times andfindUnfinishedPackagistTransitiveRunexcludes failed rows, the next attempt creates a new row; one workflow run can therefore produce several failed rows before succeeding, defeating the documented retry reuse and “one row per run” lifecycle. Keep the row unfinished for retryable, non-final attempts and mark it failed only for a non-retryable error or after retries are exhausted.
} catch (err) {
await failRunInLedger(qx, runId, (err as Error).message)
throw err
Signed-off-by: anilb <[email protected]>
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/libs/data-access-layer/tsconfig.json:9
- Raising the shared DAL target to ES2020 for one test removes the ES2017 syntax check inherited from
services/base.tsconfig.json:3, even though this comment says production sources must remain ES2017-compatible. Keep the library target at ES2017 and isolate or rewrite the test's BigInt literal instead of weakening checks for every DAL source file.
// target/lib raised over base's es2017 for the BigInt literals in the packagist
// transitive integration test (include pulls *.test.ts into tsc-check). Consumers
// compile DAL sources under their own (es2017) configs, so src itself must stay
// free of post-es2017 syntax.
"compilerOptions": {
"target": "es2020",
"lib": ["es2020", "ES2021.String"]
docs/adr/0009-packagist-worker-design-decisions.md:191
- This adds another decision as a
###subsection, but.claude/rules/adr-format.md:18-29requires edited ADRs to use the mandatory## Decision,## Alternatives Considered, and## Consequencesstructure. This ADR still lacks those required sections and should be restructured before merging.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
|
@cursor review |
Signed-off-by: anilb <[email protected]>
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)
services/libs/data-access-layer/src/osspckgs/ingestJobs.ts:80
osspckgs_ingest_jobs.idisbigserial, and the packages DB intentionally leaves int8 values as strings (services/libs/database/src/connection.ts:81-85). Converting it withNumberloses identity once IDs exceedNumber.MAX_SAFE_INTEGER, potentially updating the wrong job. Keep the ID as a string and align the existing create/mark job signatures and callers accordingly.
return row ? Number(row.id) : null
docs/adr/0009-packagist-worker-design-decisions.md:191
- The repository ADR rule (
.claude/rules/adr-format.md:18-28) requires edited numbered ADRs to contain## Decision,## Alternatives Considered, and## Consequencesin order. This adds another###entry under## Decisionswhile those mandatory sections remain absent, so the ADR still violates the enforced format. Restructure the living ADR or record this as a conforming separate ADR.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
services/libs/data-access-layer/src/packages/transitiveDependents.integration.test.ts:73
- This cleanup casts
bigserialjob IDs toint[], so it will fail withinteger out of rangeonce the sequence exceeds the 32-bit range. Cast tobigint[]to match the table schema.
await qx.result(`DELETE FROM osspckgs_ingest_jobs WHERE id = ANY($(jobIds)::int[])`, {
|
@cursor review |
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)
services/libs/data-access-layer/src/packages/transitiveDependents.ts:34
statement_timeoutapplies separately to CTAS, index creation, andANALYZE, so their combined runtime can exceed the 45-minute activity timeout. Temporal can then start a retry while the timed-out attempt is still rebuilding these global tables, duplicating the 1.5B-row scan and contending on DDL locks. The activity deadline must exceed the whole transaction's worst-case runtime, or the phases need separate bounded activities/timeouts.
// Below the activity's 45-min deadline: Temporal timeouts don't kill an in-flight
// statement, so without this a timed-out CTAS would keep running alongside its retry.
await tx.result(`SET LOCAL statement_timeout = '40min'`)
backend/src/osspckgs/migrations/V1785740540__packagist_transitive_runs.sql:12
- This new persisted table has no schema-aligned row type in
@crowd/types. ADR-0006 requires one row type per table and says those canonical types live in@crowd/types, so add the ledger row type and use it for DAL results instead of leaving the new table represented only by ad hoc primitives.
CREATE TABLE packagist_transitive_runs (
id serial PRIMARY KEY,
status text NOT NULL DEFAULT 'pending'
CHECK (status IN ('pending', 'merging', 'done', 'failed')),
docs/adr/0009-packagist-worker-design-decisions.md:193
- This decision is being added to an ADR that still lacks the mandatory
## Decision,## Alternatives Considered, and## Consequencesstructure required by.claude/rules/adr-format.md:18-29. Please restructure the ADR to the enforced template (or move this decision into a compliant ADR) rather than extending the nonconforming living format.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
A fifth lane, `computePackagistTransitiveDependents`, populates
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 24fbd73. 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 1 comment.
Suppressed comments (3)
services/libs/data-access-layer/src/osspckgs/ingestJobs.ts:80
osspckgs_ingest_jobs.idisbigserial, and this connection intentionally leaves int8 values as strings. Converting it tonumbercannot represent the column's full 64-bit range and can silently target a different job after precision is lost. Keep job IDs as strings and align this helper,createIngestJob, and their callers on that type.
// id is bigserial (pg returns int8 as a string) — convert so the declared type is true.
return row ? Number(row.id) : null
docs/adr/0009-packagist-worker-design-decisions.md:191
- This edit leaves ADR-0009 outside the mandatory ADR structure.
.claude/rules/adr-format.md:18-29requires exact## Decision,## Alternatives Considered, and## Consequencessections (including Positive/Negative/Risks), but this file has none of them. Restructure the living decision record to satisfy those required headings when adding this decision.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
services/libs/data-access-layer/src/packages/transitiveDependents.integration.test.ts:73
- This cleanup casts
bigserialjob IDs toint[], so it will fail with an integer-out-of-range error for valid IDs above the int4 limit. Cast the fixture IDs tobigint[], matchingosspckgs_ingest_jobs.id.
await qx.result(`DELETE FROM osspckgs_ingest_jobs WHERE id = ANY($(jobIds)::int[])`, {
Signed-off-by: anilb <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
services/libs/data-access-layer/src/packages/transitiveDependents.ts:34
statement_timeoutapplies separately to every statement, not to this transaction/activity as a whole. A 40-minute snapshot followed by index/ANALYZE/closure work can exceed the 45-minute start-to-close timeout; Temporal may retry while the original SQL continues, duplicating this expensive work and contending on the staging DDL. Split these phases into separately timed activities or enforce a true end-to-end deadline below the activity timeout.
await tx.result(`SET LOCAL max_parallel_workers_per_gather = 4`)
// Below the activity's 45-min deadline: Temporal timeouts don't kill an in-flight
// statement, so without this a timed-out CTAS would keep running alongside its retry.
await tx.result(`SET LOCAL statement_timeout = '40min'`)
services/libs/data-access-layer/src/packages/packagistTransitiveRuns.ts:74
- This unconditional terminal update can overwrite
failedif an earlier, timed-out finish activity eventually reaches the database after failure handling. Restrict the transition to the activemergingstate so delayed attempts cannot change a terminal outcome.
WHERE id = $(runId)`,
services/libs/data-access-layer/src/packages/packagistTransitiveRuns.ts:89
- This can change an already
donerun tofailedwhen the finish update committed but its activity completion was lost and subsequent retries exhausted. Keepdoneterminal by allowing failure only from pending/merging.
WHERE id = $(runId)`,
services/apps/packages_worker/src/packagist/workflows.ts:155
- The backstop only checks for a completed transitive run; it does not check whether the fixed
packagist-metadata-drainis still running. If metadata legitimately takes beyond Monday 04:41, this starts the snapshot while edges are still being refreshed. The later metadata chain then either skips (leaving partial-week counts) or starts a second full scan after this child closes. Gate the backstop on metadata completion/execution status as well.
export async function backstopPackagistTransitiveDrain(): Promise<void> {
if (await acts.packagistTransitiveRanRecently(TRANSITIVE_BACKSTOP_FRESH_DAYS)) return
await chainTransitiveDrain()
docs/adr/0009-packagist-worker-design-decisions.md:191
- Editing this ADR leaves it outside the required repository ADR structure:
.claude/rules/adr-format.md:18-28requires## Decision,## Alternatives Considered, and## Consequenceswith Positive/Negative/Risks subsections in order. ADR-0009 currently uses## Decisionsand lacks the other mandatory sections; restructure it as part of this edit.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
services/apps/packages_worker/src/packagist/activities.ts:572
- This comment documents terminal failure marking, but it currently annotates
packagistTransitiveRanRecently; the failure-marking function starts below it. Move the comment so the recent-run activity is not documented with unrelated behavior.
// Terminal failure marking for the merge phase — called from the workflow's catch so a
// permanently failed drain reads 'failed' instead of sitting in 'merging' forever.
export async function packagistTransitiveRanRecently(withinDays: number): Promise<boolean> {
Signed-off-by: anilb <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/apps/packages_worker/src/packagist/workflows.ts:77
- The accumulated
changedtotal is not retry-safe. If a merge activity commits its UPDATE but its completion is lost, Temporal retries the same cursor;IS DISTINCT FROMthen reportschanged = 0, so the run is marked done with an undercount even though the first attempt changed rows. Persist each batch result transactionally (keyed by run/cursor) and return it on retries, or stop treating this ledger field as an exact total.
const batch = await acts.mergePackagistTransitiveBatch(cursor, TRANSITIVE_MERGE_BATCH)
processed += batch.processed
changed += batch.changed
|
@cursor review |
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 8ac8deb. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
services/libs/data-access-layer/src/packages/transitiveDependents.ts:103
- The emptiness guard and zero-fill update run as separate statements. A PostgreSQL crash can truncate this UNLOGGED table after the guard succeeds but before the update reconnects, so the safety check can still allow real counts to be wiped; run both operations in one transaction or statement.
const guard = await qx.selectOne(
`SELECT EXISTS (SELECT 1 FROM staging.packagist_transitive_counts) AS populated`,
)
if (!guard.populated) {
throw new EmptyPackagistTransitiveCountsError()
services/libs/data-access-layer/src/osspckgs/ingestJobs.ts:80
idisbigserial, and this connection leaves int8 values as strings; converting tonumbercan silently round valid IDs aboveNumber.MAX_SAFE_INTEGER, causing later status updates to target the wrong row. Keep the ID as a string and align the ingest-job APIs accordingly.
// id is bigserial (pg returns int8 as a string) — convert so the declared type is true.
return row ? Number(row.id) : null
docs/adr/0009-packagist-worker-design-decisions.md:191
- Editing this ADR must preserve the mandatory structure in
.claude/rules/adr-format.md:18-28, but ADR-0009 still lacks## Decision,## Alternatives Considered, and## Consequenceswith the required subsections. Restructure the living decisions into that template as part of this edit.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
services/libs/data-access-layer/src/packages/transitiveDependents.ts:34
- This timeout is per statement, but each builder runs CTAS, index creation, and ANALYZE, and prepare invokes two builders. The total can exceed the 90-minute activity deadline, causing Temporal to retry while prior SQL still runs; split the phases or enforce a total budget below the activity timeout.
// Temporal timeouts don't kill in-flight SQL — this bounds each statement so a hung
// CTAS dies well inside the 90-min activity deadline (2 statements + slack).
await tx.result(`SET LOCAL statement_timeout = '40min'`)
| export async function backstopPackagistTransitiveDrain(): Promise<void> { | ||
| if (await acts.packagistTransitiveRanRecently(TRANSITIVE_BACKSTOP_FRESH_DAYS)) return | ||
| await chainTransitiveDrain() |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/adr/0009-packagist-worker-design-decisions.md:191
- This edit leaves ADR-0009 outside the required ADR structure.
.claude/rules/adr-format.md:18-28requires## Decision,## Alternatives Considered, and## Consequences(with Positive/Negative/Risks) whenever an ADR is edited; this file still has only## Decisionsand no required alternatives/consequences sections. Please restructure the ADR before merging.
### Transitive dependent counts: weekly materialized reverse closure over our own edges
Signed-off-by: anilb <[email protected]>
No description provided.