Skip to content

fix(worker): stop invalid Spot requests from retrying On-Demand - #1252

Merged
steipete merged 5 commits into
mainfrom
fix/worker-spot-fatal-fallback
Aug 10, 2026
Merged

fix(worker): stop invalid Spot requests from retrying On-Demand#1252
steipete merged 5 commits into
mainfrom
fix/worker-spot-fatal-fallback

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Aug 6, 2026

Copy link
Copy Markdown
Member

What problem this solves

AWS Spot-to-On-Demand fallback could retry market-independent request failures as On-Demand, while also missing AWS's UnfulfillableCapacity Spot-capacity error.

What changed

  • Retry On-Demand only for the individual candidates whose Spot failures are market-recoverable.
  • Preserve fallback for Spot capacity and quota failures, including UnfulfillableCapacity, and request errors that explicitly identify Spot as unsupported.
  • Do not retry market-independent parameter, image, block-device, or opaque HTTP 400 failures as On-Demand.
  • Keep direct AWS and Worker coordinator behavior aligned.
  • Document the per-candidate fallback contract and add the 0.41.3 changelog entry.

Verification

  • go vet ./...
  • go test -race ./...
  • focused direct AWS classifier and mixed-candidate tests
  • npm run format:check --prefix worker
  • npm run lint --prefix worker
  • npm run check --prefix worker
  • npm run check:node --prefix worker
  • focused Worker AWS tests
  • npm test --prefix worker
  • all Worker build targets
  • scripts/check-docs.sh
  • release workflow tests
  • final adversarial review: clean, no actionable findings

The original Worker fix and regression harness were contributed by @vincentkoc; the branch was refreshed and extended with the market-specific direct/Worker alignment and mixed-chain coverage.

@vincentkoc vincentkoc self-assigned this Aug 6, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels Aug 6, 2026
@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: passed. Reviewed August 10, 2026, 3:01 PM ET / 19:01 UTC.

ClawSweeper review

What this changes

The PR restricts AWS Spot-to-On-Demand fallback to candidates whose individual failures are capacity, quota, or explicitly Spot-specific recoverable errors.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open. Current main still performs an On-Demand pass over every candidate after retryable Spot failures; this PR replaces that behavior in both direct AWS and Worker provisioning, with focused regression coverage and no blocking patch defect found.

Priority: P2
Reviewed head: c8e6f3aedbf1413de943921fd89f6bbf5ecc17fd

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The focused implementation and substantial regression coverage support a normal good merge-quality rating, with no blocking review finding.
Proof confidence 🌊 off-meta tidepool Not applicable: The external-contributor real-behavior-proof gate does not apply to this MEMBER-authored PR; the supplied CI and focused test claims remain supplemental validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The external-contributor real-behavior-proof gate does not apply to this MEMBER-authored PR; the supplied CI and focused test claims remain supplemental validation.
Evidence reviewed 6 items Current-main behavior: Current main starts the On-Demand pass for the full candidate list whenever Spot fallback is enabled, so a retryable but market-independent failure can reach an On-Demand launch.
Proposed per-candidate guard: The PR records only market-recoverable Spot candidates and uses that list for the Worker On-Demand pass; the direct CLI adds the matching classifier and candidate collection.
Regression coverage: Worker tests cover fatal, opaque, and market-independent failures, a mixed candidate chain, and UnfulfillableCapacity fallback.
Findings None None.
Security None None.

How this fits together

Crabbox provisions remote AWS machines through both its Go CLI and Cloudflare Worker coordinator. Capacity configuration feeds candidate instance launches; classified Spot failures decide whether an eligible candidate receives an On-Demand retry or the request returns an error.

flowchart LR
  A[Capacity configuration] --> B[AWS instance candidates]
  B --> C[Spot launch attempts]
  C --> D[Failure classification]
  D -->|Market recoverable| E[Eligible On-Demand retry]
  D -->|Terminal or market independent| F[Return provisioning error]
  E --> G[Provisioned machine or error]
Loading

Before merge

  • Resolve merge risk (P2) - This intentionally narrows the documented default fallback: a genuinely Spot-recoverable AWS error with an unrecognized message will now fail instead of receiving an On-Demand retry.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Runtime, docs, and regression coverage runtime +60/-4; docs +12/-5; tests +225 Most added lines are focused direct and Worker regression coverage for the changed fallback boundary.

Merge-risk options

Maintainer options:

  1. Land the bounded fallback contract (recommended)
    Accept the intended compatibility change because the direct and Worker paths now retain named capacity, quota, and explicitly Spot-specific fallbacks.
  2. Preserve broader fallback
    Keep the prior full-candidate On-Demand retry only if maintainers prefer availability over preventing invalid request duplication.

Technical review

Best possible solution:

Land the per-candidate classifier after required checks complete, preserving the documented capacity, quota, and explicit-Spot cases and adding new AWS error forms only with matching direct and Worker regression coverage.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main’s full-candidate On-Demand loop is directly visible in source, and the PR’s focused harness exercises invalid, opaque, mixed-candidate, and UnfulfillableCapacity paths; this review did not execute the harness.

Is this the best way to solve the issue?

Yes. Per-candidate market classification is the narrowest maintainable repair: it preserves known recoverable Spot cases while preventing On-Demand retries for request errors unrelated to market selection.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f445f43a7696.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • remove rating: 🦞 diamond lobster: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This corrects a bounded AWS provisioning fallback behavior without evidence of an emergency-wide outage.
  • merge-risk: 🚨 compatibility: Existing Spot configurations with On-Demand fallback will no longer retry market-independent or unrecognized errors on On-Demand.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Not applicable: The external-contributor real-behavior-proof gate does not apply to this MEMBER-authored PR; the supplied CI and focused test claims remain supplemental validation.

Evidence

What I checked:

  • Current-main behavior: Current main starts the On-Demand pass for the full candidate list whenever Spot fallback is enabled, so a retryable but market-independent failure can reach an On-Demand launch. (internal/cli/aws.go:587, f445f43a7696)
  • Proposed per-candidate guard: The PR records only market-recoverable Spot candidates and uses that list for the Worker On-Demand pass; the direct CLI adds the matching classifier and candidate collection. (worker/src/aws.ts:1045, c8e6f3aedbf1)
  • Regression coverage: Worker tests cover fatal, opaque, and market-independent failures, a mixed candidate chain, and UnfulfillableCapacity fallback. (worker/test/aws.test.ts:1203, c8e6f3aedbf1)
  • Compatibility contract: Current documentation describes a full On-Demand rerun after Spot rejection; the PR deliberately narrows that established behavior and updates the contract. (docs/features/capacity-fallback.md:105, f445f43a7696)
  • History and release context: The current fallback lines blame to the v0.41.2 release commit, and current main is the subsequent v0.41.3 development-opening commit; the proposed fix is not already on main. (internal/cli/aws.go:587, dcb6f6b4c2ba)
  • Structured review helper limitation: The repository autoreview helper was invoked for the PR head but could not run because TruffleHog is unavailable; the functional review above is based on direct source, diff, tests, and history inspection. (c8e6f3aedbf1)

Likely related people:

  • steipete: Peter Steinberger authored the current PR-head implementation and the current-main fallback lines trace through the v0.41.2 release commit authored by the same contributor. (role: recent AWS fallback contributor; confidence: high; commits: c8e6f3aedbf1, dcb6f6b4c2ba, f445f43a7696; files: internal/cli/aws.go, worker/src/aws.ts, docs/features/capacity-fallback.md)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (35 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-09T01:43:59.518Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T06:02:11.534Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T08:26:27.418Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T10:07:51.701Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T11:43:04.868Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T15:06:21.413Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T18:30:56.902Z sha cc42526 :: passed. :: none
  • reviewed 2026-08-09T19:59:30.950Z sha cc42526 :: passed. :: none

@vincentkoc
vincentkoc marked this pull request as ready for review August 6, 2026 10:28
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper automerge

@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper saw the passing review, but did not merge yet.

Source: clawsweeper[bot]
Feedback: structured ClawSweeper verdict: pass (sha=c8e6f3aedbf1413de943921fd89f6bbf5ecc17fd)
Merge status: merge state status is BLOCKED

I left the PR open for the remaining gate instead of bypassing it.

Automerge progress:

  • 2026-08-08 08:16:39 UTC review queued d03ceb53446b (after repair)
  • 2026-08-08 08:23:29 UTC review requested repair d03ceb53446b (structured ClawSweeper marker: fix-required (finding=review-feedback sha=d03ceb...)
  • 2026-08-08 08:29:24 UTC review queued cc425267c603 (after repair)
  • 2026-08-09 20:00:10 UTC review passed cc425267c603 (structured ClawSweeper verdict: pass (sha=cc425267c603456a666e82740a979ec861ff5...)
  • 2026-08-06 10:30:00 UTC review result reused cc425267c603 (complete)
  • 2026-08-06 10:30:00 UTC review result reused c8e6f3aedbf1 (complete)
  • 2026-08-10 19:02:00 UTC review passed c8e6f3aedbf1 (structured ClawSweeper verdict: pass (sha=c8e6f3aedbf1413de943921fd89f6bbf5ecc1...)

@clawsweeper clawsweeper Bot added clawsweeper:automerge PR opted into ClawSweeper review and automerge status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 6, 2026
@vincentkoc
vincentkoc requested a review from steipete August 6, 2026 10:38
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 6, 2026
@clawsweeper
clawsweeper Bot force-pushed the fix/worker-spot-fatal-fallback branch from 4c4549d to 3de9496 Compare August 6, 2026 23:56
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 7, 2026
@clawsweeper
clawsweeper Bot force-pushed the fix/worker-spot-fatal-fallback branch from 3de9496 to d03ceb5 Compare August 8, 2026 08:16
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 8, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 10, 2026
@steipete
steipete merged commit b563503 into main Aug 10, 2026
25 checks passed
@steipete
steipete deleted the fix/worker-spot-fatal-fallback branch August 10, 2026 19:20
@steipete

Copy link
Copy Markdown
Contributor

Merged and verified.

The final fix uses a market-specific, per-candidate fallback contract in both direct AWS and the Worker coordinator. During review we corrected three subtle gaps in the original patch: UnfulfillableCapacity now remains eligible for On-Demand, opaque HTTP 400 and market-independent request/image errors do not switch markets, and one candidate's terminal market classification no longer suppresses a different candidate's valid capacity fallback.

Local proof:

  • go vet ./...
  • go test -race ./...
  • focused direct AWS market-classifier and mixed-candidate tests
  • npm run format:check --prefix worker
  • npm run lint --prefix worker
  • npm run check --prefix worker
  • npm run check:node --prefix worker
  • focused Worker AWS tests
  • npm test --prefix worker
  • all Worker build targets
  • scripts/check-docs.sh
  • release workflow tests
  • final adversarial review: no actionable findings

GitHub proof:

No live AWS instance was created. The deterministic request harness verifies the actual Spot versus On-Demand RunInstances sequence, including mixed candidate chains and initial On-Demand requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:automerge PR opted into ClawSweeper review and automerge merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants