Skip to content

fix: bound optional provider pricing requests - #1630

Draft
steipete wants to merge 1 commit into
mainfrom
codex/bound-provider-pricing
Draft

fix: bound optional provider pricing requests#1630
steipete wants to merge 1 commit into
mainfrom
codex/bound-provider-pricing

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Optional provider pricing is awaited before lease reservation, final activation, and workspace recovery. A stalled pricing response can therefore block these flows indefinitely, even though they already use fallback rates when a quote fails.

This change gives AWS and Hetzner pricing HTTP requests a five-second lifetime, including response-body reads. It aborts the owned request and awaits settlement before the existing fallback continues. AWS quotes use a separate EC2/STS client so their deadline cannot cancel another operation's cached identity check; only these optional quotes disable automatic retries. Ordinary provider requests, identity validation, provisioning deadlines, and cost precedence remain unchanged. Node AWS credential resolution stays owned by the SDK chain, and an expired quote starts no HTTP request after credentials return.

The adjacent usage example now uses the flat cost-override keys actually accepted by the existing parser.

Validation:

  • Two stalled-header regressions failed before the fix; 802 tests passed across the pricing, AWS, private AWS, usage, and full Fleet suites.
  • Real local HTTP tests verify connection closure for stalled headers and incomplete bodies. Coverage also checks successful quotes and timer cleanup, 429/503 without implicit quote retries, isolated STS cancellation, delayed credentials, and fallback continuation through all three Fleet callers.
  • Worker and Node typechecks, targeted lint, and diff checks passed. Independent P0–P2 review reported no actionable findings.
  • The candidate made a real authenticated Hetzner pricing request for cx53 in fsn1, returning $0.06/hour in 898 ms without allocating a resource.

The candidate AWS API path has not been verified against real AWS credentials. The local cancellation test is HTTP transport proof, not live AWS proof. No coordinator deployment was performed; merge is on hold pending the maintainer's live-proof decision. This does not claim to fix an observed long-running coordinator request whose blocked server stack was not available, nor the separate fixed-ID cancellation race.

Production delta: +45 lines for request-lifetime ownership; tests: +368; docs/changelog: +11. No config, storage/schema, protocol, or dependency changes. Exact-lockfile CI is required; local tests used the existing prepared dependency installation.

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 3, 2026
@clawsweeper

clawsweeper Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 3, 2026, 12:07 PM ET / 16:07 UTC.

ClawSweeper review

What this changes

This PR adds five-second deadlines to optional AWS and Hetzner price lookups, updates cost-rate documentation, and adds Worker regression coverage for stalled provider responses.

Merge readiness

Blocked before merge - 8 items remain

Keep open: current main still awaits AWS pricing without a deadline, so the work remains necessary, but the proposed AWS timeout cannot release callers when Node credential resolution never settles and the branch is currently conflicted with main.

Priority: P1
Reviewed head: 1f0b2aed8fee97c9032ee034e75ff87a518c8cc7

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The PR has a focused availability objective and extensive test additions, but a P1 unbounded AWS path and insufficient real behavior proof prevent merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The changed production owners are the AWS and Hetzner price lookups; the supplied tests include a local HTTP-server scenario, but the reviewed context contains no captured redacted after-fix coordinator trace showing fallback continuation. The reported successful Hetzner quote does not demonstrate stalled-request recovery. Attach redacted runtime output or logs after the repair; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The changed production owners are the AWS and Hetzner price lookups; the supplied tests include a local HTTP-server scenario, but the reviewed context contains no captured redacted after-fix coordinator trace showing fallback continuation. The reported successful Hetzner quote does not demonstrate stalled-request recovery. Attach redacted runtime output or logs after the repair; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Current main still has the unbounded AWS path: Current main directly awaits the AWS spot-price request and has no provider-pricing deadline helper, so the central availability problem is not already implemented upstream.
The introduced AWS wrapper cannot preempt credentials: The new deadline wraps a quote that constructs a signal-bearing client, but that client awaits its credential provider without an abortable contract. If credentials never resolve, the quote never reaches its post-await abort check or settles.
Admission remains blocked on an unsettled quote: The coordinator awaits the provider price promise before continuing; its catch only handles rejection, not a promise that remains pending after the deadline fires.
Findings 1 actionable finding [P1] Make the AWS deadline preempt unresolved credential resolution
Security None None.

How this fits together

The Worker coordinator asks provider adapters for optional hourly prices before lease admission, activation, and recovery, then combines them with fallback rates for cost records. These price lookups must not delay provisioning workflows when provider metadata is unavailable.

flowchart LR
A[Lease or workspace request] --> B[Worker coordinator]
B --> C[Provider price lookup]
C --> D{Price response arrives}
D -->|Yes| E[Cost calculation]
D -->|Timeout or failure| F[Fallback rate]
E --> G[Lease admission or activation]
F --> G
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The changed production owners are the AWS and Hetzner price lookups; the supplied tests include a local HTTP-server scenario, but the reviewed context contains no captured redacted after-fix coordinator trace showing fallback continuation. The reported successful Hetzner quote does not demonstrate stalled-request recovery. Attach redacted runtime output or logs after the repair; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Make the AWS deadline preempt unresolved credential resolution (P1) - The new wrapper starts a timer here, but the signal-bearing client still awaits credentials() without any cancellation path. If the Node credential chain never returns, the quote never settles and Fleet keeps awaiting it, so the advertised five-second bound is not met. Return the fallback at the deadline even for this case and add a regression that leaves credentials pending permanently.
  • Resolve merge risk (P1) - An AWS credential chain that never resolves still leaves lease admission, final activation, or workspace recovery waiting indefinitely despite the new five-second timer.
  • Resolve merge risk (P1) - The current head is dirty against main, so its large Worker-suite changes must be rebased before merge behavior is trustworthy.
  • Complete next step (P2) - Repair the AWS credential-resolution timeout, rebase the dirty branch onto current main, and attach redacted coordinator-level fallback proof before merge.
  • Improve patch quality - Make the deadline return fallback while Node credentials remain permanently pending, then add a Fleet-level regression.
  • Improve patch quality - Rebase the currently conflicted branch onto current main.
  • Improve patch quality - Attach redacted coordinator-level after-fix output showing a stalled quote aborts and the fallback path continues.

Findings

  • [P1] Make the AWS deadline preempt unresolved credential resolution — worker/src/aws.ts:1292-1295
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus regression coverage production +45 net, tests +368, docs +11 The small provider-path change has substantial focused coverage, but it omits the permanently pending credential case.

Merge-risk options

Maintainer options:

  1. Make the AWS deadline release callers (recommended)
    Return to the fallback price at the deadline even if credential resolution remains pending, add a permanent-credential-stall regression, and rebase the branch onto current main.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Rebase onto current main, make optional AWS pricing fall back when credential resolution never settles, and add a Fleet-level regression proving admission completes after five seconds.

Technical review

Best possible solution:

Rebase onto current main and make optional AWS pricing return the existing fallback after five seconds even when credential resolution never settles, with regression coverage through a Fleet caller and redacted runtime proof of fallback continuation.

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

Yes, source-reproducible: configure an AWS credential provider that never resolves, request a spot quote, and advance five seconds. The abort signal cannot interrupt the outstanding credential await, while Fleet remains awaiting the quote.

Is this the best way to solve the issue?

No. Propagating an abort signal to AWS HTTP is useful, but the deadline must also release the optional-price caller when credential resolution never settles while preventing any post-timeout provider request.

Full review comments:

  • [P1] Make the AWS deadline preempt unresolved credential resolution — worker/src/aws.ts:1292-1295
    The new wrapper starts a timer here, but the signal-bearing client still awaits credentials() without any cancellation path. If the Node credential chain never returns, the quote never settles and Fleet keeps awaiting it, so the advertised five-second bound is not met. Return the fallback at the deadline even for this case and add a regression that leaves credentials pending permanently.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P1: The incomplete timeout can still stall real lease admission and workspace recovery on AWS.
  • add merge-risk: 🚨 availability: Merging as written preserves an unbounded AWS dependency in flows the PR claims to bound.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owners are the AWS and Hetzner price lookups; the supplied tests include a local HTTP-server scenario, but the reviewed context contains no captured redacted after-fix coordinator trace showing fallback continuation. The reported successful Hetzner quote does not demonstrate stalled-request recovery. Attach redacted runtime output or logs after the repair; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: The incomplete timeout can still stall real lease admission and workspace recovery on AWS.
  • merge-risk: 🚨 availability: Merging as written preserves an unbounded AWS dependency in flows the PR claims to bound.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owners are the AWS and Hetzner price lookups; the supplied tests include a local HTTP-server scenario, but the reviewed context contains no captured redacted after-fix coordinator trace showing fallback continuation. The reported successful Hetzner quote does not demonstrate stalled-request recovery. Attach redacted runtime output or logs after the repair; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] npm test --prefix worker -- provider-pricing-deadline.
  • [P1] npm test --prefix worker -- fleet.
  • [P1] npm run check --prefix worker.
  • [P1] npm run lint --prefix worker.

What I checked:

  • Current main still has the unbounded AWS path: Current main directly awaits the AWS spot-price request and has no provider-pricing deadline helper, so the central availability problem is not already implemented upstream. (worker/src/aws.ts:1313, a2ee4be96759)
  • The introduced AWS wrapper cannot preempt credentials: The new deadline wraps a quote that constructs a signal-bearing client, but that client awaits its credential provider without an abortable contract. If credentials never resolve, the quote never reaches its post-await abort check or settles. (worker/src/aws.ts:1292, 1f0b2aed8fee)
  • Admission remains blocked on an unsettled quote: The coordinator awaits the provider price promise before continuing; its catch only handles rejection, not a promise that remains pending after the deadline fires. (worker/src/fleet.ts:4000, a2ee4be96759)
  • Credential test does not cover the permanent stall: The added credential test advances the timer but then resolves the credential promise, proving no subsequent HTTP request rather than that the Fleet caller proceeds while credentials remain unresolved. (worker/test/provider-pricing-deadline.test.ts:161, 1f0b2aed8fee)
  • Current-main merge state: GitHub reports the submitted head as dirty against main; the branch needs a rebase before its repaired behavior can be reviewed as a merge candidate. (1f0b2aed8fee)
  • Relevant Worker history: Feature-history sampling shows Peter Steinberger as a long-running contributor to the AWS, Hetzner, and Fleet provider paths, including recent Worker lifecycle work. (worker/src/aws.ts, 299faee17ebe)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

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

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant