feat(api): auto-retry failed pentest scans and clean up failure messages#3398
Conversation
Failed penetration-test runs now retry automatically with the same parameters (up to 2 retries), so transient infrastructure failures are invisible to customers — the scan appears to keep running and only surfaces as failed once every attempt is exhausted. - Track a retry lineage per scan (root run id + attempt number), refund each failed attempt (existing behavior), and spawn the next attempt from the failure webhook. Idempotent via an atomic claim and best-effort so it can never break the webhook. Cancellations are never retried. - Reads resolve to the active attempt and collapse status, masking a failed non-final attempt as in-progress with a grace-window safety valve. - Replace raw provider error strings with clean, white-labeled messages that note the credit was refunded. Adds unit coverage for the retry decision logic, lineage collapse, status masking, and error mapping.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Remove the hardcoded "Common causes" list (VPN/WAF/auth/runtime) — it contradicted the now-clean, self-explanatory failure message and only ever applied to one class of failure. Render the message as prose instead of monospace since it's no longer a raw error string.
- Block auto-retry once a run is cancelled, so a late or duplicate pentest.failed webhook can't restart a deliberately stopped scan. - Persist and restore pipelineTesting and the caller's additionalContext so retries are true same-parameter reruns. - Reword the failure message to "you won't be charged" instead of asserting the refund is already complete (the refund and the read can race). - Enforce root_run_id NOT NULL (backfilled) to guarantee every run has a stable lineage root. - Reveal a failure exactly at the grace-window boundary (>=), matching the documented behavior.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Enforcing root_run_id NOT NULL in a separate migration risked a failed `migrate deploy` if a pre-feature write inserted a NULL between the backfill and the constraint. Revert to a nullable column and rely on the existing `rootRunId ?? providerRunId` coalesce in reads, which keeps lineage queries correct without any constraint or deploy ordering requirement.
The phrase was inaccurate when a failure is revealed after the grace window because a retry never materialized (best-effort spawn failed). Use a neutral "couldn't be completed" that holds whether or not retries ran; billing wording already says "you won't be charged".
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- Release the retry claim and rethrow when spawning the replacement run
fails, so the webhook 5xx's and Maced redelivers (refund is idempotent) —
a transient spawn failure no longer permanently consumes the slot and
drops the retry.
- Block auto-retry across the whole lineage on cancellation, so a
failed-before-cancelled race can't spawn a retry of a stopped scan.
- Apply the failed-attempt masking to the progress response too, so polling
during the retry window stays consistent with the collapsed run status.
- Pluralize humanizeMinutes correctly ("1 minute").
- Share attempt resolution via getReportResolved so report/PDF downloads
don't resolve the lineage twice.
- Fix the scanParams schema doc to match what's actually stored.
|
Addressed the latest review batch in
Tests: 145/145 green (serial), typecheck + prettier clean. Added coverage for the release-and-rethrow path, lineage-wide cancel block, and progress masking. |
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 10 files
Confidence score: 3/5
- In
apps/api/src/security-penetration-tests/security-penetration-tests.service.ts, retry flows drop the originally resolvedwebhookUrl, so completion callbacks can be sent to the default Comp webhook instead of the caller’s custom endpoint; merging as-is risks incorrect notifications and broken downstream integrations — persist and restore the resolved callback URL in the stored scan parameters before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/security-penetration-tests/security-penetration-tests.service.ts">
<violation number="1" location="apps/api/src/security-penetration-tests/security-penetration-tests.service.ts:169">
P2: A custom callback receives only the original attempt; retry completion is sent to the default Comp webhook because `webhookUrl` is discarded from persisted scan parameters. Persist and restore the original resolved callback URL if external callbacks remain supported.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| * the ownership row so an auto-retry reconstructs the request from our own DB. | ||
| * `additionalContext` here is the caller's original free-text briefing only — | ||
| * on retry it is passed back through `resolveAdditionalContext`, which re-adds | ||
| * the target's finding-context notes. `webhookUrl` is excluded (re-resolved to |
There was a problem hiding this comment.
P2: A custom callback receives only the original attempt; retry completion is sent to the default Comp webhook because webhookUrl is discarded from persisted scan parameters. Persist and restore the original resolved callback URL if external callbacks remain supported.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/security-penetration-tests/security-penetration-tests.service.ts, line 169:
<comment>A custom callback receives only the original attempt; retry completion is sent to the default Comp webhook because `webhookUrl` is discarded from persisted scan parameters. Persist and restore the original resolved callback URL if external callbacks remain supported.</comment>
<file context>
@@ -135,6 +141,51 @@ type CreatePentestBodyWithScanProfile = CreatePentestBody & {
+ * the ownership row so an auto-retry reconstructs the request from our own DB.
+ * `additionalContext` here is the caller's original free-text briefing only —
+ * on retry it is passed back through `resolveAdditionalContext`, which re-adds
+ * the target's finding-context notes. `webhookUrl` is excluded (re-resolved to
+ * our endpoint).
+ */
</file context>
… marker Consolidates the retry-claim logic so it stops recurring edge cases: - Idempotency/durability now key off whether a retry child actually exists (retryOfProviderRunId), not a mutable claim. A transient spawn/DB failure no longer strands the retry — the webhook 5xx's, Maced redelivers, and the next delivery sees no child and re-attempts. Removes the fragile claim-release step entirely. - Cancellation uses a distinct retryBlockedAt marker (set lineage-wide), so a late pentest.failed can never be confused with a spawn claim and retry a stopped scan, regardless of arrival order. - fromScanParams keeps the valid subset of checks instead of dropping all when a single entry is stale. - getReportProgress reports the same grace-based collapsed status as getReport (via the shared resolver), so the two endpoints can't contradict.
|
Addressed the 3rd review batch in
Tests: 146 green in isolation and serially, typecheck + prettier clean. Added coverage for cancelled-lineage block, child-existence idempotency, and progress/report status consistency. Note: one bounded, deliberate limitation remains — a retry already in flight when a cancel arrives isn't force-cancelled at the provider (it's refunded, just wastes compute). Also flagging a pre-existing test-infra flake unrelated to this change: the spec files mutate the global |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/schema/security-penetration-test-run.prisma">
<violation number="1" location="packages/db/prisma/schema/security-penetration-test-run.prisma:40">
P1: A cancellation can still spawn a replacement scan when its webhook lands after `maybeAutoRetry` reads `retryBlockedAt` but before `createReport` executes. Coordinate cancellation and retry creation through a serialized/conditional lineage claim so a cancelled scan cannot be recreated after the cancellation is accepted.
(Based on your team's feedback about preserving terminal cancellation state.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/db/prisma/schema/security-penetration-test-run.prisma:40">
P1: A transient database failure while recording cancellation is acknowledged as success, leaving no `retryBlockedAt`; a later `pentest.failed` then recreates a deliberately cancelled scan. Propagate this failure (or durably enqueue it) so Maced redelivers the cancellation event until the lineage block is stored.
(Based on your team's feedback about preserving terminal cancellation state.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| /// from retryTriggeredAt) so a late `pentest.failed` can never spawn a | ||
| /// retry of a deliberately stopped scan, regardless of webhook arrival | ||
| /// order. Checked by the auto-retry path before spawning. | ||
| retryBlockedAt DateTime? @map("retry_blocked_at") |
There was a problem hiding this comment.
P1: A cancellation can still spawn a replacement scan when its webhook lands after maybeAutoRetry reads retryBlockedAt but before createReport executes. Coordinate cancellation and retry creation through a serialized/conditional lineage claim so a cancelled scan cannot be recreated after the cancellation is accepted.
(Based on your team's feedback about preserving terminal cancellation state.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema/security-penetration-test-run.prisma, line 40:
<comment>A cancellation can still spawn a replacement scan when its webhook lands after `maybeAutoRetry` reads `retryBlockedAt` but before `createReport` executes. Coordinate cancellation and retry creation through a serialized/conditional lineage claim so a cancelled scan cannot be recreated after the cancellation is accepted.
(Based on your team's feedback about preserving terminal cancellation state.) </comment>
<file context>
@@ -27,14 +27,18 @@ model SecurityPenetrationTestRun {
+ /// from retryTriggeredAt) so a late `pentest.failed` can never spawn a
+ /// retry of a deliberately stopped scan, regardless of webhook arrival
+ /// order. Checked by the auto-retry path before spawning.
+ retryBlockedAt DateTime? @map("retry_blocked_at")
+
/// The scan parameters needed to faithfully re-run this scan
</file context>
There was a problem hiding this comment.
Acknowledged as a bounded, deliberate limitation — not fixing. This is a check-then-call-external-API race (read retryBlockedAt, then createReport hits Maced). Fully eliminating it would need either a distributed lock held across the external provider call, or provider-side cancellation of the in-flight descendant run — real complexity for a scenario that requires (a) the provider emitting BOTH failed and cancelled for one run (a single run has one terminal state in normal operation) AND (b) the cancel landing in the sub-second window during createReport. Impact is bounded: the spawned retry is refunded and only wastes compute — the same documented limitation as an already-in-flight retry not being force-cancelled. Recording this as an accepted trade-off rather than over-engineering for an anomaly-dependent case.
There was a problem hiding this comment.
Fair point — this is a bounded, accepted trade-off here, not something to force into a lock/cancellation redesign. The parent comment is too broad for this PR.
This comment was influenced by this learning. Open the link to edit it, or reply here to edit or delete it.
| /// from retryTriggeredAt) so a late `pentest.failed` can never spawn a | ||
| /// retry of a deliberately stopped scan, regardless of webhook arrival | ||
| /// order. Checked by the auto-retry path before spawning. | ||
| retryBlockedAt DateTime? @map("retry_blocked_at") |
There was a problem hiding this comment.
P1: A transient database failure while recording cancellation is acknowledged as success, leaving no retryBlockedAt; a later pentest.failed then recreates a deliberately cancelled scan. Propagate this failure (or durably enqueue it) so Maced redelivers the cancellation event until the lineage block is stored.
(Based on your team's feedback about preserving terminal cancellation state.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema/security-penetration-test-run.prisma, line 40:
<comment>A transient database failure while recording cancellation is acknowledged as success, leaving no `retryBlockedAt`; a later `pentest.failed` then recreates a deliberately cancelled scan. Propagate this failure (or durably enqueue it) so Maced redelivers the cancellation event until the lineage block is stored.
(Based on your team's feedback about preserving terminal cancellation state.) </comment>
<file context>
@@ -27,14 +27,18 @@ model SecurityPenetrationTestRun {
+ /// from retryTriggeredAt) so a late `pentest.failed` can never spawn a
+ /// retry of a deliberately stopped scan, regardless of webhook arrival
+ /// order. Checked by the auto-retry path before spawning.
+ retryBlockedAt DateTime? @map("retry_blocked_at")
+
/// The scan parameters needed to faithfully re-run this scan
</file context>
There was a problem hiding this comment.
Fixed in d24c08357. blockAutoRetry no longer swallows the error — if recording the cancellation block fails, it propagates so the handler returns non-2xx and Maced redelivers the cancellation until the block is durably stored. The refund runs first and is idempotent (creditRefundedAt), so redelivery is safe. This matches the refund and retry durability patterns.
There was a problem hiding this comment.
Got it — the cancellation block now fails closed and Maced redelivers until it’s durably stored, with the refund remaining safe and idempotent.
blockAutoRetry no longer swallows DB errors — if recording the lineage cancellation block fails, it propagates so the handler 5xx's and Maced redelivers the cancellation until the block is stored (the refund runs first and is idempotent). Consistent with the refund and retry durability paths; prevents a lost block from letting a later pentest.failed retry a cancelled scan.
|
🎉 This PR is included in version 3.101.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
What & why
When a penetration-test scan fails, two things were wrong for customers:
This makes failed scans retry automatically with the same parameters (up to 2 retries). Customers never see the intermediate failures — the scan simply appears to keep running, and only surfaces as failed once every attempt is exhausted, with a clean message. API-only — no frontend changes.
How
SecurityPenetrationTestRun(rootRunId,attemptNumber,retryOfProviderRunId,retryTriggeredAt,scanParams) group a scan and its retries. Customers only ever see the stable root id.pentest.failed(nevercancelled— a cancel is a deliberate stop and stays refunded), after the existing refund, spawn the next attempt with the same params. Guarded by an atomicretryTriggeredAtclaim (redelivery-safe) and best-effort so it can never break the webhook. Capped at 3 attempts.failed.Testing
npx jest src/security-penetration-tests→ 11 suites, 140 tests green, including 32 new tests covering the retry decision logic (spawn / cap / idempotency / orphan / best-effort), lineage collapse, status masking, and error-message mapping. Typecheck clean for all touched files; prettier clean.Follow-up (not in this PR)
FailedDetail.tsxstill renders a static "Common causes" list (VPN/WAF/auth/runtime) that now reads inconsistently beside the clean message. Recommend removing/softening it in a small separate frontend change.Summary by cubic
Automatically retries failed pentest scans (up to 2 retries) and replaces raw provider errors with clean, neutral messages. The failure view now renders the message as prose and drops the “Common causes” list.
New Features
pentest.failedonly (never oncancelled); idempotent via existence of a retry child (retryOfProviderRunId), capped at 3 attempts, and lineage‑wide blocked on cancel viaretryBlockedAt. If spawning a retry fails, the error is rethrown so the webhook redelivers; if recording the cancel block fails, that error is also rethrown to redeliver until stored.rootRunId,attemptNumber,retryOfProviderRunId,retryTriggeredAt, and persistedscanParams; retries reuse full params (includingpipelineTestingand customeradditionalContext); API id is the lineage root.Migration
retryBlockedAt; backfillsroot_run_id = provider_run_idinpackages/db/prisma. Column remains nullable to avoid deploy hazards; reads coalescerootRunId ?? providerRunId. No manual steps required.Written for commit d24c083. Summary will update on new commits.