Skip to content

fix: chunk rank_packages() apply to avoid Sequin replication slot growth [CM-1374] - #4510

Merged
mbani01 merged 6 commits into
mainfrom
fix/chunk_rank_packages
Aug 27, 2026
Merged

fix: chunk rank_packages() apply to avoid Sequin replication slot growth [CM-1374]#4510
mbani01 merged 6 commits into
mainfrom
fix/chunk_rank_packages

Conversation

@mbani01

@mbani01 mbani01 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

This pull request updates the package ranking process to use a new chunked procedure, improving reliability and performance for large-scale updates. The main change is switching from the old rank_packages() function to the new rank_packages_chunked() procedure, which applies updates in manageable batches to avoid replication slot issues and long-running transactions. Associated scripts, worker activities, and workflow timeouts are updated to match the new approach.

Database migration and ranking process improvements:

  • Added a new migration (V1787733800__rank_packages_chunked_apply.sql) that introduces the rank_packages_chunked() procedure, which stages ranking results in an unlogged table and applies them to packages in committed keyset chunks for better replication and transaction handling.

Worker and script updates:

  • Updated the rankPackages activity to call rank_packages_chunked() instead of rank_packages(), returning the number of applied rows and updating job status reporting accordingly. [1] [2]
  • Modified the run-impact.ts script to trigger rank_packages_chunked() with support for a new --chunk argument, and updated logging to reflect the new procedure and its output. [1] [2]

Workflow configuration:

  • Increased the workflow activity timeout from 30 minutes to 90 minutes to accommodate the chunked ranking process.

Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
@mbani01 mbani01 self-assigned this Aug 26, 2026
Copilot AI balanced review requested due to automatic review settings August 26, 2026 09:21
@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Bulk updates to core packages ranking fields remain, but chunking and shorter transactions reduce replication risk; advisory-lock + dedicated connection behavior must deploy cleanly to avoid stuck locks or partial applies on failure.

Overview
Replaces the single ~9.66M-row packages UPDATE from rank_packages() with rank_packages_chunked(), which writes the same ranking into an UNLOGGED staging.package_rank table and applies it in keyset batches (default 25k) with COMMIT between batches so logical replication can advance instead of holding one huge transaction under REPLICA IDENTITY FULL. The procedure keeps the existing scoring CTEs, adds an advisory lock, validates chunk size / non-empty staging, and returns applied_rows.

The packages worker rankPackages activity now CALLs the new procedure on a dedicated pooled connection (discarded in finally so the lock is not left on a reused connection), sets a 75min statement timeout, and reports appliedRows in job metadata. The run:impact CLI calls the same procedure and adds --chunk. The Temporal rankPackages activity timeout moves from 30 to 90 minutes. rank_packages() remains in the DB until the worker rollout completes.

Reviewed by Cursor Bugbot for commit 9c44ee5. Bugbot is set up for automated code reviews on this repo. Configure here.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces chunked package ranking to reduce replication-slot growth from large transactions.

Changes:

  • Adds staged, keyset-chunked ranking updates.
  • Updates worker and CLI procedure calls.
  • Extends the activity timeout to 90 minutes.

Reviewed changes

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

File Description
backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql Adds the chunked ranking procedure.
services/apps/packages_worker/src/criticality/activities.ts Calls the procedure and reports applied rows.
services/apps/packages_worker/src/criticality/run-impact.ts Adds CLI chunk-size support.
services/apps/packages_worker/src/criticality/workflow.ts Increases the activity timeout.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Copilot AI review requested due to automatic review settings August 26, 2026 09:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:5

  • This header exceeds the repository's two-line limit for permitted performance/concurrency comments, and the deployment-status note will become stale. Keep only the non-obvious replication constraint in at most two lines.
-- rank_packages() applied its ranking to all of `packages` in one UPDATE (9.66M
-- rows), which under REPLICA IDENTITY FULL blew up the Sequin replication slot.
-- rank_packages_chunked() stages the same ranking into an UNLOGGED table, then
-- applies it in committed keyset chunks so the slot advances continuously.
-- rank_packages() is left in place until the new worker deploys.

Comment thread services/apps/packages_worker/src/criticality/activities.ts Outdated
…hunked()

Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Copilot AI review requested due to automatic review settings August 26, 2026 09:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:5

  • This five-line header exceeds the repository's two-line limit for allowed performance/invariant comments. Keep the operational rationale and rollout constraint, but condense them.
-- rank_packages() applied its ranking to all of `packages` in one UPDATE (9.66M
-- rows), which under REPLICA IDENTITY FULL blew up the Sequin replication slot.
-- rank_packages_chunked() stages the same ranking into an UNLOGGED table, then
-- applies it in committed keyset chunks so the slot advances continuously.
-- rank_packages() is left in place until the new worker deploys.

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:151

  • The session-level advisory lock is released only on the successful path. Any scoring error, statement timeout, or failed chunk bypasses this line; transaction rollback does not release session locks, so the pooled connection can permanently block later executions (or re-enter the lock on that same session). Ensure every caller releases this lock on the same pinned connection in a finally, or restructure lock ownership so failures always release it.
    PERFORM pg_advisory_unlock(hashtextextended('rank_packages_chunked', 0));

services/apps/packages_worker/src/criticality/activities.ts:84

  • SET changes the physical session, and task() returns that session to the shared pool without restoring it. Unrelated queries that later reuse this connection will inherit the 75-minute timeout; reset it in finally on both success and failure.
      await t.none(`SET statement_timeout = '75min'`)

Comment thread services/apps/packages_worker/src/criticality/activities.ts Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

❌ 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 e4fabc2. Configure here.

Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Copilot AI review requested due to automatic review settings August 26, 2026 09:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:50

  • This comment only notes that the code was copied unchanged from an earlier migration, which is a change-history note rather than a non-obvious invariant. Remove it; version control already preserves that provenance.
    -- Scoring CTE chain, unchanged from rank_packages() (V1783123201).

@mbani01
mbani01 requested a review from epipav August 26, 2026 10:00

@epipav epipav left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copilot AI review requested due to automatic review settings August 27, 2026 07:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:5

  • This five-line change note exceeds the repository’s two-line limit for allowed comments and partly restates the implementation. Keep only the non-obvious replication constraint and rollout invariant.
-- rank_packages() applied its ranking to all of `packages` in one UPDATE (9.66M
-- rows), which under REPLICA IDENTITY FULL blew up the Sequin replication slot.
-- rank_packages_chunked() stages the same ranking into an UNLOGGED table, then
-- applies it in committed keyset chunks so the slot advances continuously.
-- rank_packages() is left in place until the new worker deploys.

Comment on lines +82 to +89
let result
try {
await conn.none(`SET statement_timeout = '75min'`)
;[result] = await conn.query(`CALL rank_packages_chunked(0.90, NULL, 25000, 0)`)
} finally {
conn.done(true)
}
const appliedRows = Number(result.applied_rows ?? 0)
Copilot AI review requested due to automatic review settings August 27, 2026 09:24
@mbani01
mbani01 merged commit cf8820c into main Aug 27, 2026
13 checks passed
@mbani01
mbani01 deleted the fix/chunk_rank_packages branch August 27, 2026 09:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:5

  • This performance rationale is useful, but the project permits necessary comments only when they are at most two lines; the deploy-state note will also become stale immediately after rollout. Keep only the durable constraint in a concise form.
-- rank_packages() applied its ranking to all of `packages` in one UPDATE (9.66M
-- rows), which under REPLICA IDENTITY FULL blew up the Sequin replication slot.
-- rank_packages_chunked() stages the same ranking into an UNLOGGED table, then
-- applies it in committed keyset chunks so the slot advances continuously.
-- rank_packages() is left in place until the new worker deploys.

backend/src/osspckgs/migrations/V1787733800__rank_packages_chunked_apply.sql:50

  • This is a note describing how the change was produced, which the project’s comment policy explicitly disallows and which can become misleading if the ranking implementation changes. The versioned migration already provides the audit trail, so remove the note.
    -- Scoring CTE chain, unchanged from rank_packages() (V1783123201).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants