feat: implment bulk radius api (CM-1328) - #4390
Conversation
PR SummaryMedium Risk Overview Worker / pipeline changes (supporting concurrent batch load): dedicated blast-radius Reviewed by Cursor Bugbot for commit 8bbea63. Bugbot is set up for automated code reviews on this repo. Configure here. |
d281f53 to
bc1a7fc
Compare
![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 bulk submission and polling endpoints for blast-radius analyses.
Changes:
- Adds batch submit and paginated batch poll handlers.
- Adds validation, pagination helpers, and unit tests.
- Registers routes and updates the OpenAPI contract.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
submitBlastRadiusJobBatch.ts |
Submits multiple Temporal workflows. |
getBlastRadiusJobBatch.ts |
Polls multiple analyses. |
blastRadiusBatch.ts |
Defines batch schemas and pagination. |
blastRadiusBatch.test.ts |
Tests schemas and pagination. |
openapi.yaml |
Documents batch endpoints. |
akrites-external/index.ts |
Registers batch routes and limiters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
getPackagesTemporalClient()establishes the Temporal connection and can reject when Temporal is unreachable (backend/src/db/packagesTemporal.ts:18-42). Because that happens before the per-jobtryblocks, the entire request fails instead of returning a 202 with failed entries. Move client acquisition intosubmitOneJob's failure boundary (the getter already caches the connection promise) so this runtime failure follows the documented per-job semantics.
const packagesTemporal = await getPackagesTemporalClient()
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:92
- If
failAnalysisrejects,submitOneJobstill rejects andPromise.allfails the whole batch. This directly defeats the stated isolation forcreateAnalysisfailures (a database error may also make this follow-up write fail). Guard the failure-recording write separately, log that persistence failure, and still return this job's failed entry if the endpoint contract must always isolate per-job runtime failures.
await blastRadiusDal.failAnalysis(qx, analysisInput, errorMessage)
| const results: BlastRadiusAnalysisBulkEntry[] = pagedAnalysisIds.map((requestedAnalysisId) => { | ||
| const analysis = analysisById.get(requestedAnalysisId) | ||
| if (!analysis) { | ||
| return { requestedAnalysisId, found: false, analysis: null } | ||
| } | ||
|
|
||
| return { | ||
| requestedAnalysisId, | ||
| found: true, | ||
| analysis: toBlastRadiusAnalysis( | ||
| analysis, | ||
| verdictsByAnalysisId.get(requestedAnalysisId) ?? [], | ||
| excludedByRangeCountByAnalysisId.get(requestedAnalysisId) ?? 0, | ||
| ), |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
getPackagesTemporalClient()establishes the connection and can reject when Temporal is unreachable. Because it runs beforejobs.map(...), a cold-start outage fails the entire HTTP request instead of returning onefailedentry per job with 202 as the endpoint contract promises. Move client acquisition into the per-jobtrypath (the cached_initpromise still prevents 20 independent successful connections).
const packagesTemporal = await getPackagesTemporalClient()
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:92
- This awaited cleanup can itself reject, which escapes
submitOneJoband makesPromise.allreject the whole batch. In particular, the newly handledcreateAnalysisDB failure is likely to make this second DB write fail too, so the advertised per-job isolation is not guaranteed. Handle/log failure persistence separately sosubmitOneJobstill resolves to its failed entry.
await blastRadiusDal.failAnalysis(qx, analysisInput, errorMessage)
backend/src/api/public/v1/packages/getBlastRadiusJobBatch.ts:51
- Valid UUID input is case-insensitive, while PostgreSQL serializes UUID columns in lowercase. An uppercase
analysisIdtherefore passesz.uuid()but misses these case-sensitive maps, returningfound: false(or empty verdict/count data) for an existing analysis. Normalize lookup keys while preservingrequestedAnalysisIdfor the response echo.
const results: BlastRadiusAnalysisBulkEntry[] = pagedAnalysisIds.map((requestedAnalysisId) => {
const analysis = analysisById.get(requestedAnalysisId)
There was a problem hiding this comment.
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)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
- Temporal client initialization happens before the per-job boundary.
getPackagesTemporalClient()performsConnection.connect()and can reject when Temporal is unreachable, so this path returns a batch-wide 5xx before creating any analysis rows instead of the documented 202 with failed entries. Acquire/await the client insidesubmitOneJob'stry(the cached initializer will still share one connection attempt) so this failure is mapped per job.
const packagesTemporal = await getPackagesTemporalClient()
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:92
- This awaited recovery write can itself reject, causing
submitOneJobto reject andPromise.allto fail the entire request. That breaks the endpoint's stated per-job isolation, especially for thecreateAnalysisdatabase-error path this catch is intended to handle. Handle failure persistence separately (with appropriate logging/response semantics) so the helper cannot unexpectedly reject.
await blastRadiusDal.failAnalysis(qx, analysisInput, errorMessage)
backend/src/api/public/v1/akrites-external/index.ts:31
- This also raises the existing single-job endpoint's default limit from 5 to 50 requests/hour. Combined with 20 jobs per batch, the default now permits up to 1,000 workflow/LLM starts per hour, while the PR describes retaining the strict limiter and does not document this separate 10× relaxation. Keep the previous default unless this capacity and cost increase was explicitly approved.
: 50,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:38
- Temporal connection setup happens before the per-job failure boundary.
getPackagesTemporalClient()callsConnection.connect, so if Temporal is unreachable on a cold connection this rejects the entire request with a 500 and no per-job results, contrary to the documented isolation semantics. Acquire the client inside each job's guarded path (the shared initializer will still deduplicate the connection), or map an initialization failure to failed entries for every job.
const packagesTemporal = await getPackagesTemporalClient()
const results: BlastRadiusJobEntry[] = await Promise.all(
jobs.map((body) => submitOneJob(qx, packagesTemporal, body)),
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:92
- A rejection from
failAnalysisstill escapessubmitOneJoband rejects the outerPromise.all, so the promised per-job isolation is not guaranteed. This is especially likely aftercreateAnalysisfailed because the same database problem can make this recovery write fail. Handle and log the secondary persistence failure while still returning the failed entry, or use an all-settled aggregation that explicitly maps every rejection.
await blastRadiusDal.failAnalysis(qx, analysisInput, errorMessage)
backend/src/api/public/v1/akrites-external/index.ts:31
- Raising this request-based limit from 5 to 50 lets one client enqueue up to 1,000 workflows per hour through 20-item batches, versus five workflows per hour before this PR. That materially increases Temporal/LLM load and cost while the PR description explicitly notes that workflow-weighted limiting was not implemented. Keep the existing default unless the limiter is changed to account for batch size.
: 50,
backend/src/api/public/v1/akrites-external/openapi.yaml:462
- The response can contain
status: failedprecisely because a job was not successfully submitted (for example,workflow.startfailed), so saying every job is submitted is inaccurate for API consumers. Document that every requested job receives an entry and that failed entries may represent submission failures.
description: >
Plain array in request order, one entry per submitted job — unlike the
read batches there is no found/not-found case, every job is submitted.
| const { blastRadiusReport } = proxyActivities<typeof activities>({ | ||
| startToCloseTimeout: '2 minutes', | ||
| heartbeatTimeout: '1 minute', | ||
| retry: { maximumAttempts: 3 }, | ||
| }) |
| // Unlike the read batches (purls that may or may not resolve to a package), every | ||
| // job in a submit batch is genuinely submitted — there is no "not found" case, so | ||
| // the response is a plain array in request order, not a found/not-found wrapper. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
getPackagesTemporalClient()establishes the Temporal connection before entering any per-jobtry/catch. On a cold request while Temporal is unreachable, this rejects the whole handler with a 500, rather than returning a 202 with one failed result per job as the batch contract promises. Move client acquisition into the per-job failure boundary (while preserving the shared cached connection) or explicitly convert acquisition failure into per-job failed entries.
const packagesTemporal = await getPackagesTemporalClient()
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:92
- This recovery write can itself reject. If the database remains unavailable after
createAnalysisfails, or if marking a failed workflow start fails,submitOneJobrejects and the surroundingPromise.allaborts the entire response, contradicting the documented per-job isolation. Handle and log afailAnalysisfailure without allowing it to reject the batch, or use an all-settled aggregation that still produces a result for every input.
const errorMessage = err instanceof Error ? err.message : String(err)
await blastRadiusDal.failAnalysis(qx, analysisInput, errorMessage)
backend/src/api/public/v1/packages/getBlastRadiusJobBatch.ts:51
- UUID text is case-insensitive, but PostgreSQL returns UUID columns in canonical lowercase while
z.uuid()accepts uppercase hex. An uppercaseanalysisIdtherefore queries successfully but misses these case-sensitive Maps, returningfound: false(and would also miss verdict/count buckets). Normalize only the lookup key while preservingrequestedAnalysisIdin the echoed response.
const results: BlastRadiusAnalysisBulkEntry[] = pagedAnalysisIds.map((requestedAnalysisId) => {
const analysis = analysisById.get(requestedAnalysisId)
services/apps/packages_worker/src/blast-radius/workflows.ts:41
- The report activity only calls
heartbeat()afterrunReportStagehas completed. Adding a one-minute heartbeat timeout therefore makes any otherwise-valid report taking 1–2 minutes time out before its first heartbeat, reducing the effective limit below the existing two-minute start-to-close timeout. Remove this heartbeat timeout, or pass a heartbeat callback into the report stage and invoke it during the work.
heartbeatTimeout: '1 minute',
backend/src/api/public/v1/akrites-external/index.ts:31
- The bulk route consumes one limiter token while starting up to 20 workflows, and this same change also raises the shared limiter default from 5 to 50. At the hard batch size that permits 1,000 costly workflows/hour (and increases the existing single-job allowance 10×), so the limiter no longer bounds workflow-start cost as described. Keep the prior default unless the increase is explicitly required, and make bulk submissions consume tokens proportional to
jobs.lengthor use a separate weighted limiter.
max:
Number.isSafeInteger(blastRadiusRateLimitMax) && blastRadiusRateLimitMax > 0
? blastRadiusRateLimitMax
: 50,
d6a945d to
c22413a
Compare
c22413a to
5860ee3
Compare
| vulnerableVersions, | ||
| topN: 25, | ||
| onProgress, | ||
| signal, |
There was a problem hiding this comment.
Cancel completes incomplete dependents stage
High Severity
Observing cancellationSignal abort makes scanDependents return a partial result instead of failing the activity. runDependentsStage then persists that incomplete set and calls completeStageRun, so a timed-out or cancelled attempt can mark dependents as succeeded. A Temporal retry then sees succeeded and skips the stage, leaving a permanently incomplete analysis.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 630f10f. Configure here.
| maxConcurrentActivityTaskExecutions: 16, | ||
| } | ||
|
|
||
| const svc = new ServiceWorker(config, options) |
There was a problem hiding this comment.
Worker alerts wrong Slack channel
Medium Severity
The dedicated blast-radius ServiceWorker omits alertChannel. ActivityMonitoringInterceptor then falls back to CDP_ALERTS instead of the previous CDP_AKRITES_ALERTS from the shared service.ts instance, so high-retry alerts for this worker go to the wrong channel.
Reviewed by Cursor Bugbot for commit 630f10f. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
getPackagesTemporalClient()establishes the Temporal connection, so a cold-client outage rejects here before anysubmitOneJobcall reaches its per-jobtry/catch. The entire request returns an error with no analysis rows, contrary to the batch contract that Temporal failures produce per-jobfailedentries and a 202. Acquire the client inside each per-job failure boundary (the cached initializer will still share one connection attempt).
const packagesTemporal = await getPackagesTemporalClient()
services/apps/packages_worker/src/blast-radius/workflows.ts:46
- The report activity does not heartbeat while
runReportStageis running; its only heartbeat occurs after that function returns. Consequently, any slow aggregation/finalization lasting over one minute now times out and retries even though the existing start-to-close timeout allows two minutes. Remove this heartbeat timeout unless progress heartbeats are added inside the stage.
heartbeatTimeout: '1 minute',
| vulnerableVersions, | ||
| topN: 25, | ||
| onProgress, | ||
| signal, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
- Temporal client initialization is awaited before entering the per-job
tryblocks. On the first request after startup,getPackagesTemporalClient()can reject when Temporal is unreachable, causing the whole endpoint to return an error instead of the documented 202 with onefailedresult per job. Move/propagate client initialization intosubmitOneJob's guarded path while still sharing one initialization promise.
const packagesTemporal = await getPackagesTemporalClient()
services/apps/packages_worker/src/blast-radius/workflows.ts:46
- A heartbeat timeout requires heartbeats during the activity, but
blastRadiusReportonly callsheartbeat()afterrunReportStagehas completed. This makes one minute the effective timeout for the report (stricter than its two-minute start-to-close timeout) without detecting a stuck query. Remove this option or heartbeat while the report work is in progress.
heartbeatTimeout: '1 minute',
backend/src/api/public/v1/akrites-external/openapi.yaml:1259
- The new poll route is rate-limited and can return 429, but this operation omits that response even though the single-job poll contract documents it. Add the 429 error response so generated clients and consumers have the complete contract.
$ref: '#/components/schemas/Error'
| let next = 0 | ||
| async function worker() { | ||
| while (next < items.length) { | ||
| while (next < items.length && !signal?.aborted) { |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
92352ce to
8bbea63
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
backend/src/api/public/v1/packages/submitBlastRadiusJobBatch.ts:35
getPackagesTemporalClient()performsConnection.connect(), so awaiting it beforesubmitOneJob's try/catch makes a Temporal outage reject the entire request with a 500 and no per-job results. This contradicts the documented 202/failed-entry semantics. Acquire the shared client inside each per-job try (the cached promise still prevents duplicate connections) so every job can be marked and returned as failed.
const packagesTemporal = await getPackagesTemporalClient()
backend/src/api/public/v1/packages/getBlastRadiusJobBatch.ts:51
- PostgreSQL serializes UUID values in lowercase, while
z.uuid()accepts uppercase hexadecimal. An uppercase but valid requested ID therefore exists inanalysisByIdunder its lowercase DB representation and is incorrectly returned asfound: false. Normalize only for the lookup while continuing to echo the original ID.
const analysis = analysisById.get(requestedAnalysisId)
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:411
- Cancellation is treated as a normal loop exit here. The scan then returns partial results, and
runDependentsStagepersists them and marks the stage succeeded; a retry can consequently skip the incomplete stage. Propagate cancellation withsignal.throwIfAborted()rather than silently breaking, including before the scan returns.
if (analyzed.length >= topN || signal?.aborted) break
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ 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 8bbea63. Configure here.
| const packument = await fetchPackument(name) | ||
| const packument = await packumentCache(name, () => | ||
| withRateLimitRetry(() => fetchPackument(name, undefined)), | ||
| ) |
There was a problem hiding this comment.
Cancelled scan can succeed incomplete
High Severity
cancellationSignal makes scanDependents stop early via signal.aborted, but it still returns partial results instead of throwing. runDependentsStage then persists that incomplete set and marks the stage succeeded. Temporal treats a returned value as success, so a later retry sees succeeded and skips the scan, leaving reachability with a quietly truncated dependent set.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 8bbea63. Configure here.
Summary
Adds bulk (batch) endpoints for the akrites-external blast-radius API, mirroring the
existing batch pattern already used by packages/advisories/contacts detail lookups:
POST /blast-radius/jobs:batchto submit multiple blast-radius analysis jobs in onerequest, and
POST /blast-radius/jobs:batch/pollto poll multiple analyses by id inone paginated request. The OpenAPI spec is updated to document both.
Changes
POST /blast-radius/jobs:batch— submits up toMAX_BLAST_RADIUS_JOBS_PER_BATCH(20, hard limit agreed after a cost test; 10 is the recommended/documented default,
not schema-enforced) jobs per request, each starting its own Temporal workflow.
POST /blast-radius/jobs:batch/poll— polls up to 100 analysisIds per request,paginated (
page/pageSize), reusing the found/not-found echo pattern from theother batch endpoints (unknown id →
{ found: false, analysis: null }).ecosystem) is atomic across the wholejobsarray — one invalid entry rejects the entire batch with a 400, none of the jobs
are submitted.
entry comes back
status: 'failed', the rest of the batch still submits, and theresponse is still 202.
blastRadiusRateLimiteras the single-jobroute (not the regular rate limiter), since each request can multiply Temporal
workflow starts up to 20x. Bulk poll is read-only and uses the regular rate limiter.
blastRadiusBatch.ts— reuses the existingsingle-job Zod schema (
blastRadiusJobRequestSchema) and mapper (toBlastRadiusJobEntry,toBlastRadiusAnalysis) rather than duplicating validation/response-shaping logic./code-review --fix): fixed a bug wherecreateAnalysissat outside the per-job try/catch (one job's DB error could rejectthe whole batch's
Promise.all), parallelized two independent DB reads in the pollhandler, and deduplicated a repeated object literal. Flagged-but-not-fixed items
(documented in review discussion, not applied here): the rate limiter counts
requests rather than workflow starts (20x throughput vs. single-job route), and
submitOneJob/paginateAnalysisIdsduplicate logic from pre-existingsubmitBlastRadiusJob.ts/purl.ts— left alone since fixing those would requiretouching working code outside this diff's scope.
Type of change
JIRA ticket
CM-1328