From 5349cf9ed6ae202f2891381db65e610004fd8312 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 13 Jul 2026 22:05:44 -0400 Subject: [PATCH 1/3] fix(api): pentest retry follow-ups from production review - Enforce one auto-retry child per parent (unique index on retry_of_provider_run_id, dedup-guarded migration) so concurrent duplicate failure webhooks can't create two retry scans or a nondeterministic active attempt. - Preserve an explicit empty check selection ([]) on retry instead of dropping it (which let the provider fall back to its default check set). - Only mask a failure as in-progress when the run is actually retry-eligible (under the attempt cap and has stored scan params), so pre-feature runs with no scan params reveal their failure immediately instead of showing provisioning for the grace window. - Treat a malformed non-positive runtime cap (e.g. "cap 0m") as unrecognized and return the generic message instead of "maximum runtime of its time limit". --- .../pentest-lineage.util.spec.ts | 12 ++++ .../pentest-lineage.util.ts | 11 +++- .../pentest-run-error.util.spec.ts | 8 +++ .../pentest-run-error.util.ts | 7 ++- ...security-penetration-tests.service.spec.ts | 56 +++++++++++++++++++ .../security-penetration-tests.service.ts | 45 ++++++++++----- .../migration.sql | 19 +++++++ .../security-penetration-test-run.prisma | 5 ++ 8 files changed, 146 insertions(+), 17 deletions(-) create mode 100644 packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql diff --git a/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts b/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts index c29e1ead18..797e5c049f 100644 --- a/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts +++ b/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts @@ -109,5 +109,17 @@ describe('collapsedStatus', () => { }), ).toBe('failed'); }); + + it('reveals failed immediately when not retry-eligible (e.g. no scan params)', () => { + expect( + collapsedStatus({ + activeStatus: 'failed', + attemptNumber: 1, + retryEligible: false, + failedAtMs: now - 5_000, // well within grace, but can't retry + nowMs: now, + }), + ).toBe('failed'); + }); }); }); diff --git a/apps/api/src/security-penetration-tests/pentest-lineage.util.ts b/apps/api/src/security-penetration-tests/pentest-lineage.util.ts index 92134dc0bd..b2e3cebd8b 100644 --- a/apps/api/src/security-penetration-tests/pentest-lineage.util.ts +++ b/apps/api/src/security-penetration-tests/pentest-lineage.util.ts @@ -34,7 +34,9 @@ export const RETRY_GRACE_MS = 10 * 60 * 1000; * - `completed` / in-progress statuses pass through unchanged. * - `cancelled` is terminal and never retried → always revealed. * - `failed`: - * - lineage exhausted (`attemptNumber >= maxAttempts`) → revealed. + * - not retry-eligible (lineage exhausted, or the run can't actually be + * re-run — e.g. a pre-feature row with no stored scan params) → revealed + * immediately, so it's never masked as in-progress pointlessly. * - otherwise masked as `provisioning` (a retry is expected) while inside * the grace window; revealed once the window elapses without a retry * appearing, or when the failure time is unknown (can't bound the mask). @@ -44,19 +46,24 @@ export function collapsedStatus(params: { attemptNumber: number; failedAtMs: number | null; nowMs: number; + /** Whether this run can actually be auto-retried (has re-runnable params). */ + retryEligible?: boolean; maxAttempts?: number; graceMs?: number; }): PentestRunStatus { const { activeStatus, attemptNumber, failedAtMs, nowMs } = params; const maxAttempts = params.maxAttempts ?? MAX_ATTEMPTS; const graceMs = params.graceMs ?? RETRY_GRACE_MS; + const retryEligible = params.retryEligible ?? true; // Only `failed` triggers an auto-retry, so only `failed` is ever masked. if (activeStatus !== 'failed') { return activeStatus; } - if (attemptNumber >= maxAttempts) { + // Reveal immediately if no retry can happen — lineage exhausted or the run + // isn't re-runnable — so a failure that will never retry isn't masked. + if (attemptNumber >= maxAttempts || !retryEligible) { return 'failed'; } if (failedAtMs === null || nowMs - failedAtMs >= graceMs) { diff --git a/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts b/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts index 63fa802dd8..9d2e8c499f 100644 --- a/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts +++ b/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts @@ -68,6 +68,14 @@ describe('toCustomerFacingError', () => { expect(message).not.toContain('1 minutes'); }); + it('falls back to the generic message for a malformed zero cap', () => { + const message = toCustomerFacingError( + 'Hard timeout: workflow ran 0m (cap 0m)', + ); + expect(message).not.toContain('maximum runtime'); + expect(message).toContain("The scan couldn't be completed"); + }); + it('classifies the "; Daytona unreachable" variant as infra, not runtime cap', () => { const message = toCustomerFacingError( 'Hard timeout: 4m (cap 120m); Daytona unreachable', diff --git a/apps/api/src/security-penetration-tests/pentest-run-error.util.ts b/apps/api/src/security-penetration-tests/pentest-run-error.util.ts index 5662e7c046..d7ae162979 100644 --- a/apps/api/src/security-penetration-tests/pentest-run-error.util.ts +++ b/apps/api/src/security-penetration-tests/pentest-run-error.util.ts @@ -76,8 +76,11 @@ export function toCustomerFacingError( // has already claimed the "; Daytona unreachable" variant. const capMatch = /hard timeout:\s*workflow ran\s+\d+m\s*\(cap\s+(\d+)m\)/i.exec(raw); - if (capMatch) { - const cap = humanizeMinutes(Number(capMatch[1])); + const capMinutes = capMatch ? Number(capMatch[1]) : NaN; + // Only use the runtime-cap message for a sane, positive cap — a malformed + // "cap 0m" would otherwise render "maximum runtime of its time limit". + if (Number.isFinite(capMinutes) && capMinutes > 0) { + const cap = humanizeMinutes(capMinutes); return ( `Your scan reached its maximum runtime of ${cap} before completing. ` + "You won't be charged for this scan. Large targets may need a lighter " + diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts index a062f465f4..809be7b588 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts @@ -164,6 +164,8 @@ describe('SecurityPenetrationTestsService', () => { return Promise.resolve({ providerRunId: where.rootRunId, attemptNumber: 1, + // Retry-eligible by default (has stored scan params). + scanParams: { targetUrl: 'https://app.example.com' }, }); } return Promise.resolve(null); @@ -1175,6 +1177,28 @@ describe('SecurityPenetrationTestsService', () => { ); }); + it('preserves an explicit empty check selection on retry', async () => { + mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValueOnce({ + organizationId: 'org_123', + attemptNumber: 1, + rootRunId: 'run_orig', + scanParams: { targetUrl: 'https://app.example.com', checks: [] }, + }); + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ id: 'run_retry', status: 'provisioning' }), + { status: 200 }, + ), + ); + + await service['maybeAutoRetry']('run_orig'); + + // Empty selection is sent through, not dropped (which would let the + // provider fall back to its default check set). + const createBody = await getRequestBody(); + expect(createBody.checks).toEqual([]); + }); + it('blocks auto-retry across the whole lineage when a run is cancelled', async () => { mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValueOnce({ rootRunId: 'run_orig', @@ -1329,6 +1353,7 @@ describe('SecurityPenetrationTestsService', () => { mockedDb.securityPenetrationTestRun.findFirst.mockResolvedValueOnce({ providerRunId: 'run_orig', attemptNumber: 1, + scanParams: { targetUrl: 'https://a.com' }, // retry-eligible }); const recent = new Date().toISOString(); fetchMock.mockResolvedValueOnce( @@ -1352,6 +1377,37 @@ describe('SecurityPenetrationTestsService', () => { expect(report.failedReason).toBeNull(); }); + it('reveals a legacy failed run (no scan params) immediately instead of masking', async () => { + mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ + organizationId: 'org_123', + rootRunId: 'run_legacy', + }); + // Pre-feature row: attempt 1 but no scanParams → not retry-eligible. + mockedDb.securityPenetrationTestRun.findFirst.mockResolvedValueOnce({ + providerRunId: 'run_legacy', + attemptNumber: 1, + scanParams: null, + }); + const recent = new Date().toISOString(); + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ + id: 'run_legacy', + status: 'failed', + targetUrl: 'https://a.com', + createdAt: recent, + updatedAt: recent, + error: 'some failure', + }), + { status: 200 }, + ), + ); + + const report = await service.getReport('org_123', 'run_legacy'); + + expect(report.status).toBe('failed'); + }); + it('reveals a clean, white-labeled failure once the lineage is exhausted', async () => { mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ organizationId: 'org_123', diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts index b5faac7bb6..2d396afebf 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts @@ -285,7 +285,12 @@ export class SecurityPenetrationTestsService { ): Promise { const rows = await db.securityPenetrationTestRun.findMany({ where: { organizationId }, - select: { providerRunId: true, rootRunId: true, attemptNumber: true }, + select: { + providerRunId: true, + rootRunId: true, + attemptNumber: true, + scanParams: true, + }, }); if (rows.length === 0) { return []; @@ -295,7 +300,7 @@ export class SecurityPenetrationTestsService { // customer sees one entry per scan — retries never appear as separate rows. const activeByRoot = new Map< string, - { providerRunId: string; attemptNumber: number } + { providerRunId: string; attemptNumber: number; hasScanParams: boolean } >(); for (const row of rows) { const root = row.rootRunId ?? row.providerRunId; @@ -304,6 +309,7 @@ export class SecurityPenetrationTestsService { activeByRoot.set(root, { providerRunId: row.providerRunId, attemptNumber: row.attemptNumber, + hasScanParams: row.scanParams != null, }); } } @@ -324,6 +330,8 @@ export class SecurityPenetrationTestsService { this.collapseRun(report, { rootRunId, attemptNumber: active.attemptNumber, + retryEligible: + active.attemptNumber < MAX_ATTEMPTS && active.hasScanParams, }), ); } @@ -586,14 +594,18 @@ export class SecurityPenetrationTestsService { id: string, ): Promise<{ run: SecurityPenetrationTest; activeProviderRunId: string }> { await this.assertRunOwnership(organizationId, id); - const { rootRunId, activeProviderRunId, attemptNumber } = + const { rootRunId, activeProviderRunId, attemptNumber, retryEligible } = await this.resolveActiveAttempt(organizationId, id); const report = await this.callMaced( () => this.macedClient.pentests.get(activeProviderRunId), `fetching penetration test ${activeProviderRunId}`, ); return { - run: this.collapseRun(report, { rootRunId, attemptNumber }), + run: this.collapseRun(report, { + rootRunId, + attemptNumber, + retryEligible, + }), activeProviderRunId, }; } @@ -1199,7 +1211,7 @@ export class SecurityPenetrationTestsService { */ private collapseRun( report: Pentest | PentestWithProgress, - ctx: { rootRunId: string; attemptNumber: number }, + ctx: { rootRunId: string; attemptNumber: number; retryEligible: boolean }, ): SecurityPenetrationTest { const mapped = this.mapMacedRunToSecurityPenetrationTest(report); const activeStatus: PentestRunStatus = mapped.status; @@ -1210,6 +1222,7 @@ export class SecurityPenetrationTestsService { const status = collapsedStatus({ activeStatus, attemptNumber: ctx.attemptNumber, + retryEligible: ctx.retryEligible, failedAtMs: Number.isNaN(parsedFailedAt) ? null : parsedFailedAt, nowMs: Date.now(), }); @@ -1301,14 +1314,14 @@ export class SecurityPenetrationTestsService { dto.evidenceLevel = raw.evidenceLevel; } if (Array.isArray(raw.checks)) { - const checks = raw.checks.filter((check): check is PentestCheck => + // Preserve the original selection faithfully: assign the validated array + // whenever checks were stored, including an explicit empty selection + // (`[]`) — omitting it would let the provider fall back to its default + // check set. Stale entries (e.g. a check enum value removed between + // deploys) are filtered out rather than dropping the whole selection. + dto.checks = raw.checks.filter((check): check is PentestCheck => this.isPentestCheck(check), ); - // Keep whatever survived validation rather than dropping the whole - // selection if a single entry is stale (e.g. a check enum value removed - // between deploys) — a retry with the valid subset beats silently - // running the full default check set. - if (checks.length > 0) dto.checks = checks; } if (typeof raw.additionalContext === 'string') { dto.additionalContext = raw.additionalContext; @@ -1601,6 +1614,7 @@ export class SecurityPenetrationTestsService { rootRunId: string; activeProviderRunId: string; attemptNumber: number; + retryEligible: boolean; }> { const row = await db.securityPenetrationTestRun.findUnique({ where: { providerRunId: requestedId }, @@ -1610,12 +1624,17 @@ export class SecurityPenetrationTestsService { const active = await db.securityPenetrationTestRun.findFirst({ where: { organizationId, rootRunId }, orderBy: { attemptNumber: 'desc' }, - select: { providerRunId: true, attemptNumber: true }, + select: { providerRunId: true, attemptNumber: true, scanParams: true }, }); + const attemptNumber = active?.attemptNumber ?? 1; return { rootRunId, activeProviderRunId: active?.providerRunId ?? requestedId, - attemptNumber: active?.attemptNumber ?? 1, + attemptNumber, + // A failure can only be masked as in-progress if a retry could actually + // happen: under the attempt cap AND re-runnable (has stored scan params; + // pre-feature rows have none). + retryEligible: attemptNumber < MAX_ATTEMPTS && active?.scanParams != null, }; } diff --git a/packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql b/packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql new file mode 100644 index 0000000000..bb8f7fc3db --- /dev/null +++ b/packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql @@ -0,0 +1,19 @@ +-- Dedup any accidental duplicate retry children (keep the earliest per parent), +-- so the unique index below can be created safely. This is a no-op in normal +-- operation — it only fires if concurrent duplicate `pentest.failed` webhooks +-- managed to persist more than one retry child for the same parent before this +-- constraint existed. +DELETE FROM "security_penetration_test_runs" a +USING "security_penetration_test_runs" b +WHERE a."retry_of_provider_run_id" IS NOT NULL + AND a."retry_of_provider_run_id" = b."retry_of_provider_run_id" + AND ( + a."created_at" > b."created_at" + OR (a."created_at" = b."created_at" AND a."id" > b."id") + ); + +-- Enforce at most one auto-retry child per parent attempt. Postgres treats +-- NULLs as distinct, so original runs (null parent) are unaffected; concurrent +-- duplicate failure webhooks can no longer persist two retry children. +CREATE UNIQUE INDEX "security_penetration_test_runs_retry_of_provider_run_id_key" + ON "security_penetration_test_runs" ("retry_of_provider_run_id"); diff --git a/packages/db/prisma/schema/security-penetration-test-run.prisma b/packages/db/prisma/schema/security-penetration-test-run.prisma index ed6df67d24..646d46c55e 100644 --- a/packages/db/prisma/schema/security-penetration-test-run.prisma +++ b/packages/db/prisma/schema/security-penetration-test-run.prisma @@ -65,6 +65,11 @@ model SecurityPenetrationTestRun { organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade) @@unique([providerRunId]) + /// At most one auto-retry child per parent attempt. Postgres treats NULLs as + /// distinct, so originals (null parent) don't conflict; this makes concurrent + /// duplicate `pentest.failed` webhooks safe — only one retry child can be + /// persisted, keeping active-attempt selection deterministic. + @@unique([retryOfProviderRunId]) @@index([organizationId]) @@index([rootRunId]) @@map("security_penetration_test_runs") From 97820f454b3ce3bc6b189e774c420e6f328bf2e2 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 13 Jul 2026 22:18:53 -0400 Subject: [PATCH 2/3] fix(api): dedupe pentest retries at the provider via idempotency key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the DB unique-constraint approach with a Maced idempotency key so concurrent duplicate failure webhooks dedupe at the provider — the key fix, since the constraint only prevented duplicate rows AFTER both requests had already launched a provider scan (orphaning the loser). - Auto-retries pass a deterministic Idempotency-Key (`retry:`); Maced returns the same run for concurrent duplicates, so only one scan runs and only one ownership row is written (via the existing providerRunId upsert). - Drop the retry_of_provider_run_id unique index + its dedup migration (no longer needed, and the migration's row-delete could orphan provider runs). - Retry-eligibility (for status masking) now reuses fromScanParams validation instead of a shallow null check, so malformed stored params reveal the failure immediately rather than masking it. - Runtime-cap message now requires a safe integer cap, so absurd oversized caps fall through to the generic message. --- .../pentest-run-error.util.spec.ts | 8 +++ .../pentest-run-error.util.ts | 8 ++- ...security-penetration-tests.service.spec.ts | 57 +++++++++++++++++++ .../security-penetration-tests.service.ts | 31 +++++++--- .../migration.sql | 19 ------- .../security-penetration-test-run.prisma | 5 -- 6 files changed, 94 insertions(+), 34 deletions(-) delete mode 100644 packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql diff --git a/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts b/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts index 9d2e8c499f..114bf22eca 100644 --- a/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts +++ b/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts @@ -76,6 +76,14 @@ describe('toCustomerFacingError', () => { expect(message).toContain("The scan couldn't be completed"); }); + it('falls back to the generic message for an absurd oversized cap', () => { + const message = toCustomerFacingError( + 'Hard timeout: workflow ran 1m (cap 999999999999999999999999m)', + ); + expect(message).not.toContain('maximum runtime'); + expect(message).toContain("The scan couldn't be completed"); + }); + it('classifies the "; Daytona unreachable" variant as infra, not runtime cap', () => { const message = toCustomerFacingError( 'Hard timeout: 4m (cap 120m); Daytona unreachable', diff --git a/apps/api/src/security-penetration-tests/pentest-run-error.util.ts b/apps/api/src/security-penetration-tests/pentest-run-error.util.ts index d7ae162979..1c863b6c67 100644 --- a/apps/api/src/security-penetration-tests/pentest-run-error.util.ts +++ b/apps/api/src/security-penetration-tests/pentest-run-error.util.ts @@ -77,9 +77,11 @@ export function toCustomerFacingError( const capMatch = /hard timeout:\s*workflow ran\s+\d+m\s*\(cap\s+(\d+)m\)/i.exec(raw); const capMinutes = capMatch ? Number(capMatch[1]) : NaN; - // Only use the runtime-cap message for a sane, positive cap — a malformed - // "cap 0m" would otherwise render "maximum runtime of its time limit". - if (Number.isFinite(capMinutes) && capMinutes > 0) { + // Only use the runtime-cap message for a sane, positive whole-minute cap — a + // malformed "cap 0m" would render "maximum runtime of its time limit", and an + // absurd oversized cap would render e.g. "1e+24 minutes". Anything else falls + // through to the generic message. + if (Number.isSafeInteger(capMinutes) && capMinutes > 0) { const cap = humanizeMinutes(capMinutes); return ( `Your scan reached its maximum runtime of ${cap} before completing. ` + diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts index 809be7b588..5bf91e1904 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts @@ -217,6 +217,13 @@ describe('SecurityPenetrationTestsService', () => { return input instanceof Request ? input.url : String(input); } + function getRequestHeader(name: string, callIndex = 0): string | null { + const [input, init] = fetchMock.mock.calls[callIndex]; + const headers = + input instanceof Request ? input.headers : new Headers(init?.headers); + return headers.get(name); + } + it('lists reports with organization context', async () => { const expectedPayload = [ { @@ -1159,6 +1166,9 @@ describe('SecurityPenetrationTestsService', () => { pipelineTesting: true, }), ); + // Deterministic idempotency key tied to the parent → Maced dedupes + // concurrent duplicate failure webhooks into one provider scan. + expect(getRequestHeader('Idempotency-Key')).toBe('retry:run_orig'); // The user's briefing survives the round-trip (re-persisted for any // further retry). expect(mockedDb.securityPenetrationTestRun.upsert).toHaveBeenCalledWith( @@ -1199,6 +1209,21 @@ describe('SecurityPenetrationTestsService', () => { expect(createBody.checks).toEqual([]); }); + it('does not send an idempotency key for user-initiated creates', async () => { + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ id: 'run_new', status: 'provisioning' }), + { status: 200 }, + ), + ); + + await service.createReport('org_123', { + targetUrl: 'https://app.example.com', + }); + + expect(getRequestHeader('Idempotency-Key')).toBeNull(); + }); + it('blocks auto-retry across the whole lineage when a run is cancelled', async () => { mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValueOnce({ rootRunId: 'run_orig', @@ -1408,6 +1433,38 @@ describe('SecurityPenetrationTestsService', () => { expect(report.status).toBe('failed'); }); + it('reveals a failed run with malformed (non-null) scan params instead of masking', async () => { + mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ + organizationId: 'org_123', + rootRunId: 'run_bad', + }); + // Non-null but invalid params (no targetUrl) → fromScanParams rejects → + // not retry-eligible. + mockedDb.securityPenetrationTestRun.findFirst.mockResolvedValueOnce({ + providerRunId: 'run_bad', + attemptNumber: 1, + scanParams: { nonsense: true }, + }); + const recent = new Date().toISOString(); + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ + id: 'run_bad', + status: 'failed', + targetUrl: 'https://a.com', + createdAt: recent, + updatedAt: recent, + error: 'some failure', + }), + { status: 200 }, + ), + ); + + const report = await service.getReport('org_123', 'run_bad'); + + expect(report.status).toBe('failed'); + }); + it('reveals a clean, white-labeled failure once the lineage is exhausted', async () => { mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ organizationId: 'org_123', diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts index 2d396afebf..4fb1b6a0ce 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts @@ -300,7 +300,7 @@ export class SecurityPenetrationTestsService { // customer sees one entry per scan — retries never appear as separate rows. const activeByRoot = new Map< string, - { providerRunId: string; attemptNumber: number; hasScanParams: boolean } + { providerRunId: string; attemptNumber: number; retryable: boolean } >(); for (const row of rows) { const root = row.rootRunId ?? row.providerRunId; @@ -309,7 +309,9 @@ export class SecurityPenetrationTestsService { activeByRoot.set(root, { providerRunId: row.providerRunId, attemptNumber: row.attemptNumber, - hasScanParams: row.scanParams != null, + // Re-runnable only if the stored params actually validate (same check + // as the retry path), not merely non-null. + retryable: this.fromScanParams(row.scanParams) != null, }); } } @@ -331,7 +333,7 @@ export class SecurityPenetrationTestsService { rootRunId, attemptNumber: active.attemptNumber, retryEligible: - active.attemptNumber < MAX_ATTEMPTS && active.hasScanParams, + active.attemptNumber < MAX_ATTEMPTS && active.retryable, }), ); } @@ -444,10 +446,22 @@ export class SecurityPenetrationTestsService { }, }; + // For an auto-retry, use a deterministic idempotency key tied to the parent + // run so concurrent duplicate `pentest.failed` webhooks dedupe AT THE + // PROVIDER — both create calls return the same run instead of launching two + // scans (one of which would be orphaned). User-initiated creates pass none. + const idempotencyKey = lineage.retryOfProviderRunId + ? `retry:${lineage.retryOfProviderRunId}` + : undefined; + let createdReport: PentestCreated; try { createdReport = await this.callMaced( - () => this.macedClient.pentests.create(body), + () => + this.macedClient.pentests.create( + body, + idempotencyKey ? { idempotencyKey } : undefined, + ), 'creating penetration test', ); } catch (error) { @@ -1632,9 +1646,12 @@ export class SecurityPenetrationTestsService { activeProviderRunId: active?.providerRunId ?? requestedId, attemptNumber, // A failure can only be masked as in-progress if a retry could actually - // happen: under the attempt cap AND re-runnable (has stored scan params; - // pre-feature rows have none). - retryEligible: attemptNumber < MAX_ATTEMPTS && active?.scanParams != null, + // happen: under the attempt cap AND re-runnable. Reuse the same params + // validation `maybeAutoRetry` uses, so malformed (not just null) scan + // params are treated as ineligible and revealed immediately. + retryEligible: + attemptNumber < MAX_ATTEMPTS && + this.fromScanParams(active?.scanParams) != null, }; } diff --git a/packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql b/packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql deleted file mode 100644 index bb8f7fc3db..0000000000 --- a/packages/db/prisma/migrations/20260714020000_pentest_unique_retry_child/migration.sql +++ /dev/null @@ -1,19 +0,0 @@ --- Dedup any accidental duplicate retry children (keep the earliest per parent), --- so the unique index below can be created safely. This is a no-op in normal --- operation — it only fires if concurrent duplicate `pentest.failed` webhooks --- managed to persist more than one retry child for the same parent before this --- constraint existed. -DELETE FROM "security_penetration_test_runs" a -USING "security_penetration_test_runs" b -WHERE a."retry_of_provider_run_id" IS NOT NULL - AND a."retry_of_provider_run_id" = b."retry_of_provider_run_id" - AND ( - a."created_at" > b."created_at" - OR (a."created_at" = b."created_at" AND a."id" > b."id") - ); - --- Enforce at most one auto-retry child per parent attempt. Postgres treats --- NULLs as distinct, so original runs (null parent) are unaffected; concurrent --- duplicate failure webhooks can no longer persist two retry children. -CREATE UNIQUE INDEX "security_penetration_test_runs_retry_of_provider_run_id_key" - ON "security_penetration_test_runs" ("retry_of_provider_run_id"); diff --git a/packages/db/prisma/schema/security-penetration-test-run.prisma b/packages/db/prisma/schema/security-penetration-test-run.prisma index 646d46c55e..ed6df67d24 100644 --- a/packages/db/prisma/schema/security-penetration-test-run.prisma +++ b/packages/db/prisma/schema/security-penetration-test-run.prisma @@ -65,11 +65,6 @@ model SecurityPenetrationTestRun { organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade) @@unique([providerRunId]) - /// At most one auto-retry child per parent attempt. Postgres treats NULLs as - /// distinct, so originals (null parent) don't conflict; this makes concurrent - /// duplicate `pentest.failed` webhooks safe — only one retry child can be - /// persisted, keeping active-attempt selection deterministic. - @@unique([retryOfProviderRunId]) @@index([organizationId]) @@index([rootRunId]) @@map("security_penetration_test_runs") From d0b3d4ee799385a40f4b200d23fd6e8bb25f689d Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 13 Jul 2026 22:29:40 -0400 Subject: [PATCH 3/3] fix(api): make pentest retry billing reservation idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrent duplicate failure webhooks converged on one provider run (via the idempotency key) but still reserved two subscription allowances — each call used a random reservation id, so only one landed on the shared ownership row and the other allowance was orphaned (never refunded), double-charging the customer. Retries now use a deterministic reservation id (`pending:retry:`); billing consumption is idempotent on it, so duplicates debit exactly once. --- .../security-penetration-tests.service.spec.ts | 11 +++++++++++ .../security-penetration-tests.service.ts | 10 +++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts index 5bf91e1904..f04e7ab1d3 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts @@ -1169,6 +1169,17 @@ describe('SecurityPenetrationTestsService', () => { // Deterministic idempotency key tied to the parent → Maced dedupes // concurrent duplicate failure webhooks into one provider scan. expect(getRequestHeader('Idempotency-Key')).toBe('retry:run_orig'); + // Deterministic billing reservation id (idempotent on sourceResourceId) + // so concurrent duplicates debit the allowance exactly once. + expect( + mockBillingEntitlementsService.tryConsumeIncludedUsageForProduct, + ).toHaveBeenCalledWith( + expect.objectContaining({ + organizationId: 'org_123', + productKey: 'pentest', + sourceResourceId: 'pending:retry:run_orig', + }), + ); // The user's briefing survives the round-trip (re-persisted for any // further retry). expect(mockedDb.securityPenetrationTestRun.upsert).toHaveBeenCalledWith( diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts index 4fb1b6a0ce..bdd49010de 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts @@ -354,7 +354,15 @@ export class SecurityPenetrationTestsService { organizationId, payload, ); - const billingUsageSourceId = `pending:${randomUUID()}`; + // For a retry, derive a DETERMINISTIC reservation id from the parent run so + // concurrent duplicate failure webhooks reserve the same allowance. Billing + // consumption is idempotent on this id (billingUsageEvent.idempotencyKey), + // so duplicates debit exactly once — otherwise two random ids would debit + // twice and only one would land on the shared ownership row, orphaning the + // other. User-initiated creates keep a unique random id. + const billingUsageSourceId = lineage.retryOfProviderRunId + ? `pending:retry:${lineage.retryOfProviderRunId}` + : `pending:${randomUUID()}`; let consumedSubscriptionAllowance = false; // Reserve subscription allowance before calling Maced so fast double-clicks