fix(api): pentest retry follow-ups from production review

Merged
TO
3mo ago
+227-19

Follow-ups from the review on the production deploy PR (#3399) for the pentest auto-retry feature. Investigated all 5 findings; fixed the 4 real ones, left 1 that's a deliberate product decision.

Fixed

  • Concurrent duplicate failure webhooks → duplicate retries (P1). At-least-once webhook delivery could let two pentest.failed deliveries each spawn a retry child for the same parent (duplicate scans + nondeterministic active-attempt pick). Added a unique index on retry_of_provider_run_id (Postgres treats NULLs as distinct, so originals are unaffected) — at most one retry child per parent. The migration dedups first so it's deploy-safe. The losing concurrent spawn's ownership insert fails and is refunded by the existing error path.
  • Explicit empty check selection dropped on retry (P2). checks: [] was omitted on retry, letting the provider fall back to its default check set. Now the stored selection is preserved faithfully (including empty); stale entries are filtered out without dropping the whole list.
  • Legacy runs masked pointlessly (P2). A failure is now masked as in-progress only when the run is actually retry-eligible (under the attempt cap and has stored scan params). Pre-feature rows (no scan params) reveal their failure immediately instead of showing "provisioning" for the grace window.
  • Malformed zero runtime cap (P3). "cap 0m" produced "maximum runtime of its time limit"; a non-positive/non-finite cap now falls through to the generic message.

Not changed (deliberate)

  • "Retries all failures, including runtime-cap/config." This is the intended behavior — we chose to retry uniformly with the same parameters rather than classify transient vs permanent (agent runtime varies run-to-run, and it's a temporary posture while we're on the current sandbox provider). Not a bug.

Testing

npx jest src/security-penetration-tests → 151 tests green (isolation + serial), typecheck + prettier clean. Added coverage for empty-check preservation, retry-eligibility masking, and the zero-cap message.


Summary by cubic

Fixes pentest auto-retries to dedupe duplicates at the provider, preserve check selection, only mask failures when a retry can happen, and make billing reservations idempotent to avoid double charges. Tightens the runtime-cap message.

  • Bug Fixes
    • Dedupe concurrent failure webhooks at the provider (Idempotency-Key: retry:<parentRunId>) and make retry billing reservation idempotent (pending:retry:<parentRunId>), so duplicates create one scan and debit once; user-initiated creates send no key and keep a random reservation id.
    • Preserve an explicit empty check selection (checks: []) on retry.
    • Mask failures as “provisioning” only when retry-eligible: under the attempt cap and when stored params validate via fromScanParams; legacy or malformed params reveal “failed” immediately.
    • Show the generic error for non‑positive or absurd runtime caps; only positive, safe‑integer caps use the “maximum runtime” message.