Skip to content

fix: abort hung OG metadata fetches and archive metrics - #3471

Open
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/og-fetch-timeout
Open

fix: abort hung OG metadata fetches and archive metrics#3471
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/og-fetch-timeout

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Public OG routes (/og/skill, /og/plugin, /og/profile) fetched registry/Convex metadata with no abort. A stalled backend parks the OG function until the platform kills it. Sibling image fetch already aborts at 1.5s.

Archive downloads awaited an untimed metric POST before returning the first zip byte, so a hung Convex metric blocked the download. The comment already said metrics must not interrupt bytes.

Evidence

$ bunx vitest run server/og/fetchSkillOgMeta.test.ts server/og/fetchPluginOgMeta.test.ts server/og/fetchPublisherOgMeta.test.ts server/convexProxy.test.ts
# 4 files, 24 tests passed

Red: hanging fetch never settled / zip first byte blocked at 500ms. Green: abort at 1.5s; zip first chunk is not delayed by a hanging metric POST.

Real behavior proof

Behavior addressed: OG metadata fetches abort at 1.5s; archive metric POST is fire-and-forget with the same abort and cannot delay first zip byte.
Real environment tested: macOS, Bun from the worktree, clawhub /tmp/oc-impl-clawhub-og head 536df529.
Exact steps or command run after this patch: bunx vitest run on the four files above.
Evidence after fix: 24 tests passed.
Observed result after fix: hung fetch returns null after 1.5s; zip stream starts without waiting for metrics.
What was not tested: A live production Convex outage.

Public /og/skill, /og/plugin, and /og/profile now abort metadata
fetches after 1.5s, matching trusted OG image fetches.

Hosted archive download metrics are fire-and-forget with the same
abort so a hung metric POST cannot block the first zip byte.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

@vercel

vercel Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

@SebTardif is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal backlog priority 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 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 15, 2026, 6:35 PM ET / 22:35 UTC.

ClawSweeper review

What this changes

Adds 1.5-second bounds to Open Graph metadata lookups and records archive-download metrics asynchronously so a stalled metric request cannot delay ZIP bytes.

Merge readiness

Blocked until real behavior proof from a real setup is added - 4 items remain

Keep open: the archive metric change is non-blocking, but the prior P2 finding remains—OG timeouts end after response headers, so a stalled JSON body can still hold the public card route. The PR also needs real server-boundary proof before merge.

Priority: P2
Reviewed head: 546b13e6177dae324284b37eefcafd13b7db4e32

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and has useful regression tests, but it retains a concrete timeout gap and lacks real behavior proof.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The only after-fix evidence is focused Vitest output; add redacted real ClawHub server-boundary evidence for the timed-out card path and non-blocking archive stream before merge. 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 only after-fix evidence is focused Vitest output; add redacted real ClawHub server-boundary evidence for the timed-out card path and non-blocking archive stream before merge. 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 5 items Timeout ends before JSON parsing: The skill helper wraps only fetch in the timeout; after headers arrive, the helper clears its timer and the following response.json() can wait indefinitely. The plugin helper has the same structure.
Existing bounded-body pattern: The established OG image path retains its timeout while it fetches and consumes the response body, demonstrating the needed full-operation boundary.
Current main does not already provide this fix: Current main directly awaits the public skill API fetch and then its JSON body without an abort deadline, so this PR remains necessary if corrected.
Findings 1 actionable finding [P2] Keep the deadline active through metadata body parsing
Security None None.

How this fits together

Public Open Graph routes obtain live skill, plugin, or publisher metadata before rendering social-card PNGs. The archive proxy streams ZIP bytes from signed storage manifests and separately records a best-effort download metric.

flowchart LR
  A[Browser OG request] --> B[OG route]
  B --> C[Metadata service]
  C --> D[Deadline handling]
  D --> E[Social-card PNG]
  F[Browser archive request] --> G[Archive proxy]
  G --> H[ZIP byte stream]
  G --> I[Best-effort metric]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The only after-fix evidence is focused Vitest output; add redacted real ClawHub server-boundary evidence for the timed-out card path and non-blocking archive stream before merge. 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.
  • Keep the deadline active through metadata body parsing (P2) - withOgFetchTimeout resolves as soon as fetch returns headers and clears its timer before the next-line response.json(). A backend can therefore send headers and stall its body forever, leaving the OG route hung; wrap the full fetch-and-parse operation and add that regression case for the matching plugin helper too.
  • Resolve merge risk (P2) - A metadata origin that sends headers but never completes its JSON body will still keep the public OG request pending despite the advertised 1.5-second timeout.
  • Complete next step (P2) - The deadline-scope defect has a narrow, mechanical repair; contributor-supplied real behavior proof remains required after that repair.

Findings

  • [P2] Keep the deadline active through metadata body parsing — server/og/fetchSkillOgMeta.ts:47-51
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Runtime versus regression coverage production +64/-15, tests +183/-1 The shared timeout and proxy changes are accompanied by substantially larger focused regression coverage, though that coverage does not exercise a real server boundary.

Merge-risk options

Maintainer options:

  1. Bound the complete metadata read (recommended)
    Move JSON parsing inside the 1.5-second deadline and cover a response that returns headers but never resolves its body before merging.
  2. Pause for real route evidence
    After the code correction, require a redacted local ClawHub server trace showing the fallback card returns on deadline and ZIP streaming begins while metric delivery is stalled.

Technical review

Best possible solution:

Keep one shared deadline around each complete metadata operation—including response-body parsing—and add a stalled-body regression test for both public HTTP metadata helpers.

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

Yes, from source: make fetch resolve an OK response whose json() never settles, then advance past 1.5 seconds; the skill and plugin helpers remain pending because parsing is outside the timeout.

Is this the best way to solve the issue?

No. The proposed helper bounds only header acquisition; it should wrap the full fetch-and-parse operation, following the existing image-fetch pattern that keeps its deadline active during body consumption.

Full review comments:

  • [P2] Keep the deadline active through metadata body parsing — server/og/fetchSkillOgMeta.ts:47-51
    withOgFetchTimeout resolves as soon as fetch returns headers and clears its timer before the next-line response.json(). A backend can therefore send headers and stall its body forever, leaving the OG route hung; wrap the full fetch-and-parse operation and add that regression case for the matching plugin helper too.
    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 4117154ecac4.

Labels

Label justifications:

  • P2: This is a bounded public-route and download-stream reliability fix with limited, but user-visible, availability impact.
  • merge-risk: 🚨 availability: The incomplete deadline can leave Open Graph rendering stalled when a metadata body stops after headers.
  • 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 only after-fix evidence is focused Vitest output; add redacted real ClawHub server-boundary evidence for the timed-out card path and non-blocking archive stream before merge. 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] bunx vitest run server/og/fetchSkillOgMeta.test.ts server/og/fetchPluginOgMeta.test.ts server/og/fetchPublisherOgMeta.test.ts server/convexProxy.test.ts.
  • [P1] bun run ci:unit.
  • [P1] bun run ci:types-build.

What I checked:

  • Timeout ends before JSON parsing: The skill helper wraps only fetch in the timeout; after headers arrive, the helper clears its timer and the following response.json() can wait indefinitely. The plugin helper has the same structure. (server/og/fetchSkillOgMeta.ts:47, 546b13e6177d)
  • Existing bounded-body pattern: The established OG image path retains its timeout while it fetches and consumes the response body, demonstrating the needed full-operation boundary. (server/og/fetchImageDataUrl.ts:64, 4117154ecac4)
  • Current main does not already provide this fix: Current main directly awaits the public skill API fetch and then its JSON body without an abort deadline, so this PR remains necessary if corrected. (server/og/fetchSkillOgMeta.ts:47, 4117154ecac4)
  • Archive ownership provenance: The available history identifies this as recent archive-streaming work; deeper historical traversal was limited by unavailable promisor objects, so OG ownership is not over-attributed. (server/convexProxy.ts:217, 60b02c09f936)
  • Proof is test-only: The PR body reports four focused Vitest files and 24 passing tests, but no after-fix request through a real ClawHub server boundary. The prepared media manifest contains only a failed download of an example image URL, not runtime proof.

Likely related people:

  • Patrick Erichsen: Authored the recent main-branch archive streaming change that owns the proxy path touched here; available local history was insufficient to assign the OG helpers more precisely. (role: recent area contributor; confidence: medium; commits: 60b02c09f936; files: server/convexProxy.ts)

Rank-up moves

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

  • Wrap metadata body parsing in the shared deadline and add a stalled-body regression test.
  • Attach redacted local-server output proving the card fallback and first ZIP byte under stalled upstream calls.

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 (2 earlier review cycles)
  • reviewed 2026-08-15T20:59:48.839Z sha 536df52 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-15T21:15:35.623Z sha 546b13e :: needs real behavior proof before merge. :: [P2] Keep the OG deadline active while parsing the response body

TypeScript lib.dom types RequestInit.signal as AbortSignal | null.
Assigning init?.signal into AbortSignal | undefined failed tsc.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
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. P2 Normal backlog priority 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