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 5, 2026, 9:59 AM ET / 13:59 UTC.

ClawSweeper review

What this changes

Caps Hugging Face retry delays from numeric and HTTP-date Retry-After headers at 30 seconds and adds tests for both formats.

Regression provenance

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

Merge readiness

Blocked before merge - 5 items remain

Keep open: current main and v0.1.7 do not implement this limit. The prior cooldown-recovery finding 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 focused, but premature retry exhaustion and helper-only proof leave it unready to merge.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The captured macOS transcript exercises only two delay-helper assertions. It does not show HuggingFaceBenchmarkClient using a real HTTP transport, request timing, or after-fix recovery; a local injected HTTP fault can supply that evidence without live Hugging Face access. 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 two delay-helper assertions. It does not show HuggingFaceBenchmarkClient using a real HTTP transport, request timing, or after-fix recovery; a local injected HTTP fault can supply that evidence without live Hugging Face access. 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 12 items Repository policy and scope: Read the full root AGENTS.md. Its benchmark boundaries, targeted-helper guidance, and Go verification expectations informed the review. No ancestor-scoped policy or maintainer notes apply to these paths; the nested autoreview policy is outside their subtree. No bundled reviewer or tests were executed.
Verified introduced change: The pinned base-to-head comparison contains only two production-line replacements and thirteen added test lines. The original head’s raw parent matches the pinned base.
Current main still honors long cooldowns: Current main returns the parsed server delay without this cap. The surrounding retry loop still allows six attempts and supports context cancellation.
Findings 1 actionable finding [P1] Preserve the server cooldown before retrying
Security None None.

How this fits together

ClawScan’s benchmark client downloads datasets for scanner evaluation and submission validation. Its HTTP retry loop converts server responses into delays, then returns dataset rows or a fetch error.

flowchart TD
  A[Benchmark or submission validation] --> B[Dataset HTTP request]
  B --> C{Response retriable?}
  C -->|No| F[Return rows or error]
  C -->|Yes| D[Calculate retry delay]
  D --> E{Canceled or attempts exhausted?}
  E -->|Yes| F
  E -->|No, after waiting| B
Loading

Decision needed

Question Recommendation
Should long server cooldowns remain honored by default, or should benchmark fetching deliberately sacrifice recovery for a shorter waiting limit? Preserve cooldown-compatible recovery: Retain server-directed retry spacing and handle shorter waiting limits through cancellation or an explicitly chosen budget.

Why: The current implementation explicitly honors server delays; changing that tradeoff affects unattended benchmark and submission workflows and needs maintainer intent.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The captured macOS transcript exercises only two delay-helper assertions. It does not show HuggingFaceBenchmarkClient using a real HTTP transport, request timing, or after-fix recovery; a local injected HTTP fault can supply that evidence without live Hugging Face access. 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 the server returns 429 until its advertised Retry-After: 3600 deadline, this cap makes the six-attempt loop retry at approximately 0, 30, 60, 90, 120, and 150 seconds, then fail before recovery is possible. Current main waits for the cooldown and can succeed afterward. HTTP-date headers have the same problem. Preserve server-directed spacing and bound caller waiting through cancellation or an explicitly approved timeout policy.
  • Resolve merge risk (P1) - Existing benchmark and submission-validation runs that recover after a long server cooldown may instead exhaust their attempts and require an operator rerun.
  • Complete next step (P2) - Agree on a cooldown-preserving waiting policy, address premature retry exhaustion, and provide redacted real-HTTP timing and recovery evidence before merge.
  • 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

None.

Merge-risk options

Maintainer options:

  1. Preserve recovery before bounding waits (recommended)
    Keep server-directed spacing and validate the maintainer-selected cancellation or timeout behavior before merging.
  2. Pause the policy change
    Retain the current default until the recovery-versus-latency tradeoff is agreed.

Technical review

Best possible solution:

Preserve server cooldowns by default and use a maintainer-approved cancellation or wait-budget policy to bound latency without premature retries.

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

Yes, from source: a 3600-second header produces an hour-long wait on main, while fast persistent throttling exhausts the patched client’s six attempts in roughly 150 seconds. No runtime reproduction was executed.

Is this the best way to solve the issue?

No. A per-wait cap defeats server-directed recovery within the fixed retry budget; cancellation or an explicitly approved waiting budget provides a safer boundary.

Full review comments:

  • [P1] Preserve the server cooldown before retrying — internal/runner/benchmark.go:932-936
    When the server returns 429 until its advertised Retry-After: 3600 deadline, this cap makes the six-attempt loop retry at approximately 0, 30, 60, 90, 120, and 150 seconds, then fail before recovery is possible. Current main waits for the cooldown and can succeed afterward. HTTP-date headers have the same problem. Preserve server-directed spacing and bound caller waiting through cancellation or an explicitly approved timeout policy.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a bounded benchmark retry improvement without evidence of an urgent current outage.
  • merge-risk: 🚨 compatibility: The cap changes existing cooldown recovery into premature retry exhaustion for sufficiently long throttling windows.
  • 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 two delay-helper assertions. It does not show HuggingFaceBenchmarkClient using a real HTTP transport, request timing, or after-fix recovery; a local injected HTTP fault can supply that evidence without live Hugging Face access. 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:

  • Repository policy and scope: Read the full root AGENTS.md. Its benchmark boundaries, targeted-helper guidance, and Go verification expectations informed the review. No ancestor-scoped policy or maintainer notes apply to these paths; the nested autoreview policy is outside their subtree. No bundled reviewer or tests were executed. (AGENTS.md:1, b7be605e23df)
  • Verified introduced change: The pinned base-to-head comparison contains only two production-line replacements and thirteen added test lines. The original head’s raw parent matches the pinned base. (internal/runner/benchmark.go:932, b7be605e23df)
  • Current main still honors long cooldowns: Current main returns the parsed server delay without this cap. The surrounding retry loop still allows six attempts and supports context cancellation. (internal/runner/benchmark.go:929, 6432c40f2340)
  • Latest release check: The local v0.1.7 tag points to the supplied current-main SHA, whose retry helper remains uncapped. Neither current main nor the supplied latest release implements the requested limit. (6432c40f2340)
  • Cooldown regression mechanism: The loop sends at most six requests and waits after the first five. With fast responses and a cooldown beyond 150 seconds, the proposed cap can consume every attempt before the server permits recovery. (internal/runner/benchmark.go:878, b7be605e23df)
  • CLI cancellation boundary: The CLI intentionally leaves SIGINT at its process default until all benchmark phases observe context. A nil context permits long unattended waits but does not establish that Ctrl+C cannot terminate the CLI. (cmd/clawscan/main.go:286, 6432c40f2340)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; 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.
  • Add a real HTTP timing and final-result transcript for both header forms, covering the existing default and chosen bounded-wait behavior. Redact private endpoints, IP addresses, credentials, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review.

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 (29 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-03T11:06:41.815Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Preserve valid server-directed Retry-After delays | [P2] Clamp seconds before Duration conversion
  • 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

@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