perf: prioritize critical packages in go enrichment scans#4308
Conversation
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
|
|
|
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR tunes the Go package enrichment sweeps in packages_worker to be more resilient under upstream rate limiting. It (1) reorders getGoBatch so is_critical packages are selected before non-critical ones in each batch, on the theory that if the run hits rate limits mid-batch the non-critical tail is what gets skipped, and (2) adds an in-process 429 retry loop to fetchLatest (proxy.golang.org), honoring Retry-After or falling back to exponential backoff, before finally returning the existing RATE_LIMIT error.
Changes:
getGoBatchnow orders byis_critical DESCahead oflast_synced_at ASC NULLS FIRST, purl ASC, and the stale TODO is replaced with an explanatory comment.fetchLatestretries up to 5 times on HTTP 429, waiting onRetry-Afterwhen present or1000 * 2**attemptotherwise, via a new localsleephelper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
services/apps/packages_worker/src/go/activities.ts |
Adds is_critical DESC as the leading sort key in the shared getGoBatch selector used by both the version and status sweeps. |
services/apps/packages_worker/src/go/proxyClient.ts |
Wraps the fetch/classify logic in a retry loop that retries 429s with Retry-After/backoff and adds a sleep helper. |
Key review notes:
- The purl-based keyset cursor (
WHERE purl > $(after)+return rows[rows.length - 1].purl) requirespurlto be the leadingORDER BYkey (as PyPI's sweep does). Placingis_critical DESC(andlast_synced_at) ahead ofpurlmakes the cursor skip and re-scan packages, and the redundant re-fetches consume the same proxy rate budget this PR is protecting. - The uncapped
Retry-Afterwait can exceed the activity's 2-minuteheartbeatTimeout, causing Temporal to retry the whole batch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
| } | ||
| const retryAfterSec = parseInt(res.headers.get('retry-after') ?? '', 10) | ||
| const waitMs = Number.isNaN(retryAfterSec) ? 1000 * 2 ** attempt : retryAfterSec * 1000 | ||
| await sleep(waitMs) |
There was a problem hiding this comment.
Long Retry-After misses heartbeats
Medium Severity
On HTTP 429, fetchLatest may sleep for the full numeric Retry-After value in seconds before retrying. The go enrichment activity only heartbeats at the start of each package; a wait longer than the configured two-minute heartbeat timeout can fail the activity mid-batch.
Reviewed by Cursor Bugbot for commit 76e9914. Configure here.
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ 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 13a1cda. Configure here.
| cursor = next | ||
| } | ||
| await continueAsNew<typeof enrichGoVersions>({ cursor }) | ||
| await continueAsNew<typeof enrichGoVersions>(cursor) |
There was a problem hiding this comment.
Legacy workflow cursor shape breaks
High Severity
The Go enrichment workflows now take a GoScanCursor as the first argument and pass that object to continueAsNew, but previously they used { cursor: string }. Replay or continuation from an in-flight run still supplies the old shape, so criticalAfter and after are undefined in getGoPriorityBatch, pagination breaks, and the workflow can exit early as if the scan finished.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 13a1cda. Configure here.
| if (res.status === 429) { | ||
| if (attempt === MAX_429_RETRIES) { | ||
| return { kind: 'RATE_LIMIT', statusCode: 429, message: '429 after retries' } | ||
| } | ||
| const retryAfterSec = parseInt(res.headers.get('retry-after') ?? '', 10) | ||
| const waitMs = Number.isNaN(retryAfterSec) ? 1000 * 2 ** attempt : retryAfterSec * 1000 | ||
| await sleep(waitMs) | ||
| continue |
This pull request improves the handling of Go package batches and enhances the resilience of the Go proxy client against rate limiting. The main changes ensure that critical packages are prioritized during batch processing and introduce a retry mechanism for handling HTTP 429 (rate limit) responses from the Go proxy.
Batch prioritization and rate limit handling improvements:
Critical package prioritization:
The
getGoBatchfunction now sorts packages so that critical packages are processed first in each batch. This ensures that if rate limiting occurs, non-critical packages are the ones most likely to be skipped, preserving the processing of critical ones. [1] [2]Retry logic for rate limits:
The
fetchLatestfunction inproxyClient.tsnow retries up to 5 times when receiving HTTP 429 responses. It respects theRetry-Afterheader if present, or uses exponential backoff otherwise. If all retries are exhausted, it returns a clear rate limit error. [1] [2] [3] [4]Utility improvements:
Introduced a
sleephelper function to facilitate waiting between retries in the rate limit handling logic.These changes increase the reliability of Go package processing, especially under conditions where the upstream proxy may enforce rate limits.
Note
Medium Risk
Scan pagination and cursor semantics changed, which can alter which Go packages sync and when under load; proxy retry behavior also extends activity duration on rate limits.
Overview
Go enrichment scans no longer paginate with a single
purlcursor sorted bylast_synced_at. They useGoScanCursorwith separatecriticalAfterandafterkeysets,getGoPriorityBatchfills each batch with critical packages first then non-critical, and Temporal workflowscontinueAsNewwith the full cursor object.fetchLatestagainst the Go proxy now retries HTTP 429 up to five times, honoringRetry-Afterwhen set or exponential backoff otherwise, instead of failing immediately on rate limit.Reviewed by Cursor Bugbot for commit 13a1cda. Bugbot is set up for automated code reviews on this repo. Configure here.