Skip to content

fix(runner): cap HuggingFace Retry-After backoff at 30s - #50

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-huggingface-retry-after-cap
Open

fix(runner): cap HuggingFace Retry-After backoff at 30s#50
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-huggingface-retry-after-cap

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

clawscan benchmark clawhub-security-signals fetches HuggingFace dataset rows and retries on 429/5xx. huggingFaceRowsBackoff honors Retry-After as integer seconds or an HTTP-date with no ceiling, while the quadratic fallback is already capped at 30s.

The public CLI builds HuggingFaceBenchmarkClient with a nil Context, so requestContext() is context.Background() and the retry select never expires. A 429 with Retry-After: 3600 blocks the process for one hour per attempt (up to five waits). This change caps Retry-After at the same 30s ceiling. It does not add a process-wide signal context.

The uncapped header path was introduced in #3 (2026-06-25). #46 made the wait cancelable when a context is set; it did not cap the delay.

Evidence

Red (unfixed huggingFaceRowsBackoff):

$ go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
=== RUN   TestHuggingFaceRowsBackoffCapsRetryAfter
    submission_test.go:276: Retry-After 3600 = 1h0m0s, want 30s
--- FAIL: TestHuggingFaceRowsBackoffCapsRetryAfter (0.00s)
FAIL
FAIL    github.com/openclaw/clawscan/internal/runner    0.248s
FAIL

Green (after the 30s cap):

$ go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
=== RUN   TestHuggingFaceRowsBackoffCapsRetryAfter
--- PASS: TestHuggingFaceRowsBackoffCapsRetryAfter (0.00s)
PASS
ok      github.com/openclaw/clawscan/internal/runner    0.263s

Retry-After: 3600 and an HTTP-date two hours ahead both return 30s. The quadratic fallback path is unchanged.

Real behavior proof

  • Behavior or issue addressed: HuggingFace row-fetch Retry-After delays (integer seconds and HTTP-date) are capped at 30s, matching the existing fallback ceiling, so a 429 cannot stall clawscan benchmark for an hour per attempt.

  • Real environment tested: macOS 26.6.2 (Darwin 25.6.0 arm64), go1.27.0 darwin/arm64, checkout /tmp/oc-pr-clawscan-F003 on fix/f003-huggingface-retry-after-cap.

  • Exact steps or command run after this patch:

    go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
  • Evidence after fix: terminal output from the patched tree:

    $ go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
    === RUN   TestHuggingFaceRowsBackoffCapsRetryAfter
    --- PASS: TestHuggingFaceRowsBackoffCapsRetryAfter (0.00s)
    PASS
    ok      github.com/openclaw/clawscan/internal/runner    0.263s
  • Observed result after fix: Retry-After: 3600 is 30s (was 1h). An HTTP-date two hours ahead is also 30s. The helper no longer returns a multi-hour wait.

  • What was not tested: A live HuggingFace 429 with a long Retry-After header. Network fetch of datasets-server.huggingface.co during this change.

Command: go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/

Observed: unfixed delay was 1h0m0s for Retry-After: 3600; patched delay is 30s for both 3600 and a future HTTP-date.

Expected: both header forms cap at 30s.

Time: 11:07:34 PDT (2026-08-29 18:07:34 UTC)

Date: 2026-08-29

Environment: macOS 26.6.2, Darwin 25.6.0 arm64, go1.27.0 darwin/arm64

Honor Retry-After integer seconds and HTTP-date values only up to the
existing 30s quadratic fallback ceiling. A 429 with Retry-After: 3600
could block clawscan benchmark for an hour per attempt when the client
context is Background.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested review from a team and Patrick-Erichsen as code owners August 29, 2026 18:09
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. 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 Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 9, 2026, 9:57 PM ET / September 10, 2026, 01:57 UTC (Revision 31).

ClawSweeper review

What this changes

Caps Hugging Face dataset retry delays at 30 seconds for both Retry-After formats and adds assertions for each.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 5 items remain

The change remains absent from main and v0.1.7, so this PR is still relevant. The prior cooldown regression remains unresolved, and the supplied helper-test transcript does not establish real HTTP recovery.

Priority: P2
Reviewed head: b7be605e23df7db4203400710d007f0a71c2cd2e
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is small and understandable, but recovery compatibility remains unresolved and proof is limited to helper assertions.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The captured macOS transcript exercises only the delay helper, not HuggingFaceBenchmarkClient through a real HTTP transport. A production-client trace with injected HTTP failures, request timing, and the final recovery or deadline result is still needed; live Hugging Face access is unnecessary. 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 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The captured macOS transcript exercises only the delay helper, not HuggingFaceBenchmarkClient through a real HTTP transport. A production-client trace with injected HTTP failures, request timing, and the final recovery or deadline result is still needed; live Hugging Face access is unnecessary. 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 8 items Verified introduced patch: The pinned delta changes two delay-return expressions and adds one test containing two helper assertions; it does not change the six-attempt retry budget.
Current main retains full server delays: Main returns the full numeric or future HTTP-date delay. The caller waits, retries up to six times, and returns the final error response when attempts are exhausted.
Release comparison: The v0.1.7 tag points at fetched main, whose inspected retry implementation remains uncapped for valid Retry-After headers.
Findings 1 actionable finding [P1] Preserve the server cooldown before retrying
Security None None.

How this fits together

ClawScan fetches Hugging Face dataset rows before running benchmark scans and evaluating results. Its shared HTTP retry loop determines whether temporary server failures recover or abort the benchmark.

flowchart TD
  A[Benchmark command] --> B[Dataset HTTP client]
  B --> C[Server response]
  C --> D{Retryable failure?}
  D -->|Yes| E[Retry delay and attempt limit]
  E --> B
  D -->|No| F[Dataset rows or error]
  F --> G[Benchmark evaluation]
Loading

Decision needed

Question Recommendation
Should bounded benchmark waiting preserve the existing cooldown-compatible default or intentionally retry before the server's requested time? Preserve cooldown-compatible recovery: Keep the existing default and, if desired, add an explicit bounded-wait policy that exits without premature retries.

Why: A fixed 30-second fallback ceiling does not establish permission to shorten server-directed cooldowns; the change trades existing recovery behavior for shorter failure latency.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The captured macOS transcript exercises only the delay helper, not HuggingFaceBenchmarkClient through a real HTTP transport. A production-client trace with injected HTTP failures, request timing, and the final recovery or deadline result is still needed; live Hugging Face access is unnecessary. 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.
  • Preserve the server cooldown before retrying (P1) - When a server returns Retry-After: 3600 and continues rejecting requests until that cooldown expires, this cap sends all six attempts within approximately 150 seconds plus request time. The existing implementation can recover after waiting an hour; the branch instead exhausts its budget and fails the benchmark. The HTTP-date branch has the same problem. Preserve server-directed waiting by default and use an explicit deadline/cancellation outcome to bound total waiting. This prior finding remains unresolved.
  • Resolve merge risk (P1) - Existing benchmark runs that recover after a server cooldown can instead exhaust their attempts before that cooldown ends; compatibility and final-result proof are absent.
  • Complete next step (P2) - Resolve the cooldown compatibility finding and obtain real HTTP recovery evidence before merge. Redact private endpoints, IP addresses, credentials, and other private details. Update the PR body with proof to trigger re-review; if needed, ask a maintainer to comment @clawsweeper re-review.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Preserve the server cooldown before retrying — internal/runner/benchmark.go:932-936
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Long-cooldown retry window 6 attempts; 5 waits capped at 30 seconds Ignoring request duration, the branch can consume its entire retry budget in 150 seconds despite a one-hour server cooldown.

Merge-risk options

Maintainer options:

  1. Retain the default recovery window (recommended)
    Preserve valid Retry-After delays and obtain approval plus compatibility proof for any separate bounded-wait behavior.
  2. Defer the waiting-policy change
    Pause this proposal until the intended latency-versus-recovery tradeoff is agreed.

Technical review

Best possible solution:

Preserve server-directed cooldowns by default, with any maintainer-approved bounded-wait mode ending explicitly at its deadline rather than retrying early.

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

Yes, source establishes both the existing one-hour wait for Retry-After: 3600 and the branch's premature retry-budget exhaustion when a server enforces that cooldown; neither scenario was executed during this review.

Is this the best way to solve the issue?

No. Shortening a requested cooldown changes recovery semantics; an explicit deadline or cancellation policy can bound waiting without issuing requests before the server is ready.

Full review comments:

  • [P1] Preserve the server cooldown before retrying — internal/runner/benchmark.go:932-936
    When a server returns Retry-After: 3600 and continues rejecting requests until that cooldown expires, this cap sends all six attempts within approximately 150 seconds plus request time. The existing implementation can recover after waiting an hour; the branch instead exhausts its budget and fails the benchmark. The HTTP-date branch has the same problem. Preserve server-directed waiting by default and use an explicit deadline/cancellation outcome to bound total waiting. This prior finding remains unresolved.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 6432c40f2340.

Labels

Label justifications:

  • P2: This is a bounded benchmark reliability improvement without evidence of an urgent user-facing outage.
  • merge-risk: 🚨 compatibility: Shortening valid server delays can make previously recoverable benchmark runs fail before the cooldown expires.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The captured macOS transcript exercises only the delay helper, not HuggingFaceBenchmarkClient through a real HTTP transport. A production-client trace with injected HTTP failures, request timing, and the final recovery or deadline result is still needed; live Hugging Face access is unnecessary. 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

What I checked:

  • Verified introduced patch: The pinned delta changes two delay-return expressions and adds one test containing two helper assertions; it does not change the six-attempt retry budget. (internal/runner/benchmark.go:932, b7be605e23df)
  • Current main retains full server delays: Main returns the full numeric or future HTTP-date delay. The caller waits, retries up to six times, and returns the final error response when attempts are exhausted. (internal/runner/benchmark.go:929, 6432c40f2340)
  • Release comparison: The v0.1.7 tag points at fetched main, whose inspected retry implementation remains uncapped for valid Retry-After headers. (internal/runner/benchmark.go:932, 6432c40f2340)
  • Existing recovery intent and related work: The merged ci: back off Hugging Face rate limits #3 added both Retry-After handling and six attempts. Its recorded commit message explicitly describes surviving rate limits and short outages; the GitHub commit patch confirms those changes. (internal/runner/benchmark.go:930, 7a97f1cf114e)
  • Cancellation is separate from delay policy: The merged fix(runner): honor cancel during HuggingFace retry backoff #46 added context-aware waits while intentionally retaining default process SIGINT handling in the CLI. It did not cap server delays and therefore does not supersede this proposal. (cmd/clawscan/main.go:286, 113c1a8ede9f)
  • Proof coverage and review continuity: The complete supplied body at sourceRevision 7c2e1b3b1e4babead732a575bc814a31018bba366b9d3c3df1cf18efb6a862c1 reports only TestHuggingFaceRowsBackoffCapsRetryAfter on macOS. That test calls the helper directly; it does not exercise the HTTP client, elapsed retry timing, or recovery. The prior completed review covered this same head, and the comparison is empty. Earlier overflow findings concern multiplication already present in the base and are not re-raised as introduced defects. (internal/runner/submission_test.go:273, b7be605e23df)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve the waiting-policy choice while preserving cooldown-compatible recovery.
  • Provide redacted real HTTP timing and final-result evidence for both header formats, covering the existing default and any approved bounded-wait behavior; terminal output or a recording is suitable.

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 (30 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-03T15:43:01.448Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor valid server-directed Retry-After delays | [P2] Clamp numeric Retry-After before duration conversion
  • reviewed 2026-09-03T21:50:59.464Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor valid server-directed Retry-After delays | [P2] Clamp numeric Retry-After before duration conversion
  • reviewed 2026-09-04T01:57:32.183Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve valid server Retry-After values | [P2] Clamp numeric Retry-After before duration conversion
  • reviewed 2026-09-04T08:55:55.222Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve the server cooldown before retrying
  • reviewed 2026-09-05T04:59:46.867Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve the server cooldown before retrying
  • reviewed 2026-09-05T08:00:45.359Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve the server cooldown before retrying
  • reviewed 2026-09-05T11:00:39.448Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve the server cooldown before retrying
  • reviewed 2026-09-05T13:59:51.250Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve the server cooldown before retrying

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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. P2 Normal priority bug or improvement with limited blast radius. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. labels Aug 29, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. P2 Normal priority bug or improvement with limited blast radius. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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. labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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: 🦪 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