Skip to content

fix(fal): bound video generation JSON response reads#96977

Closed
Monkey-wusky wants to merge 2 commits into
openclaw:mainfrom
Monkey-wusky:fix/fal-video-generate-bound-json
Closed

fix(fal): bound video generation JSON response reads#96977
Monkey-wusky wants to merge 2 commits into
openclaw:mainfrom
Monkey-wusky:fix/fal-video-generate-bound-json

Conversation

@Monkey-wusky

@Monkey-wusky Monkey-wusky commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

fetchFalJson in extensions/fal/video-generation-provider.ts reads the video generation submit, poll, and completion responses with an unbounded await response.json(). The shared fetch layer enforces an abort timeout but does not bound body size, so a misbehaving or compromised fal endpoint can stream an arbitrarily large (or Content-Length-less, never-ending) JSON body that res.json() buffers whole into memory — an OOM vector on the video generation queue path.

Changes

  • Route all JSON reads in fetchFalJson through the shared bounded reader readProviderJsonResponse (16 MiB cap = PROVIDER_JSON_RESPONSE_MAX_BYTES, re-exported via openclaw/plugin-sdk/provider-http): it reads under the shared 16 MiB cap, cancels the underlying stream on overflow, and throws fal video generation failed: JSON response exceeds <N> bytes.
  • Removes the manual try { return await response.json() } catch in favour of readProviderJsonResponse which already provides label-scoped error messages.
  • No new abstraction — reuses the same bounded reader as the recently merged provider response-limit PRs.

Real behavior proof

  • Behavior addressed: An unbounded fal video generation response with no Content-Length and an oversized / never-ending body must not be buffered whole; the read must stop at the cap, cancel the stream, and throw a bounded error — while valid small responses still parse unchanged.
  • Real environment tested: A ReadableStream Response delivering 32 MiB in 1 MiB chunks (double the 16 MiB cap) drives the real provider.generateVideo. The test asserts the bounded reader cancels the stream mid-flight and never pulls the full advertised payload.
  • Observed outcome: The provider throws a bounded JSON error. The underlying stream is canceled. bytesPulled < 32 MiB confirms no full buffering.

@openclaw-barnacle openclaw-barnacle Bot added extensions: fal size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 26, 2026
@clawsweeper

clawsweeper Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

This PR is superseded by a broader open fal hardening PR that covers the same video JSON path plus fal image/music, is cleanly mergeable, and has stronger proof.

Root-cause cluster
Relationship: superseded
Canonical: #96886
Summary: The broader open fal response-limit PR is the viable canonical landing path for this same video response-read hardening.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Close this PR and keep the remaining review/landing decision on #96886.

So I’m closing this here and keeping the remaining discussion on #96886.

Review details

Best possible solution:

Close this PR and keep the remaining review/landing decision on #96886.

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

Yes. Current main calls response.json() inside fetchFalJson, so fal video submit/status/result success bodies are not bounded by the shared provider JSON response cap.

Is this the best way to solve the issue?

No. Reusing readProviderJsonResponse is the right fix shape, but the broader PR is the better landing path because it covers all fal success JSON reads and preserves the video malformed-response mapping.

Security review:

Security review cleared: The diff attempts to reduce untrusted provider response-body memory exposure and does not add dependencies, workflow permissions, secret handling, or downloaded-code execution.

AGENTS.md: found and applied where relevant.

What I checked:

  • Current main unbounded fal video JSON path: Current main still calls response.json() inside fetchFalJson, so fal video submit, status, and result success bodies are not capped by the shared provider JSON reader. (extensions/fal/video-generation-provider.ts:483, 9a735bea03f6)
  • Shared bounded JSON reader contract: readProviderJsonResponse reads through readResponseWithLimit with the shared 16 MiB default cap and labelled overflow/malformed JSON errors. (src/agents/provider-http-errors.ts:312, 9a735bea03f6)
  • Current PR video-only diff: The current PR changes only the fal video JSON helper and test, and its production diff removes the local malformed JSON wrapper instead of preserving FAL_VIDEO_MALFORMED_RESPONSE. (extensions/fal/video-generation-provider.ts:483, 0860ac4d8616)
  • Canonical broader PR diff: The broader PR changes fal image, music, and video success JSON reads to readProviderJsonResponse, updates test doubles to real Response bodies, and preserves fal video malformed JSON mapping while letting overflow errors propagate. (extensions/fal/video-generation-provider.ts:481, dcea6b4de44b)
  • Canonical PR live state: The broader PR is open, non-draft, mergeable/CLEAN, has proof: sufficient, and its Real behavior proof checks are successful, making it a viable canonical landing path. (dcea6b4de44b)
  • Earlier fal bounded JSON attempt: The earlier fal bounded JSON PR is closed unmerged, so it is historical context rather than a viable replacement; its comments also point toward newer bound-JSON PRs. (043a4a585800)

Likely related people:

  • Alix-007: Added the shared bounded provider JSON reader and authored the broader open fal response-limit PR that supersedes this branch. (role: shared-helper contributor and canonical PR author; confidence: high; commits: 2592f8a51a4e, dcea6b4de44b; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.ts, extensions/fal/video-generation-provider.ts)
  • steipete: Git history shows Peter Steinberger added provider video support, fal queue handling, and later fal video model updates in the affected module. (role: fal video area contributor; confidence: high; commits: 932194b7d58c, 6d34a1c814b7, b56cd114e745; files: extensions/fal/video-generation-provider.ts, extensions/fal/video-generation-provider.test.ts)

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

@clawsweeper clawsweeper Bot added 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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 26, 2026
@Monkey-wusky

Monkey-wusky commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by the broader fal hardening PR #96886 which covers video + image + music with stronger proof.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: fal merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Monkey-wusky