Skip to content

feat(generations): recover a generate_media result across a reload - #5630

Merged
georgi merged 4 commits into
mainfrom
claude/generate-media-resume
Sep 6, 2026
Merged

feat(generations): recover a generate_media result across a reload#5630
georgi merged 4 commits into
mainfrom
claude/generate-media-resume

Conversation

@georgi

@georgi georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

What changed

A generate_media reply is an rpc_response carrying no job_id and no thread_id, so WebSocketClientSession.sendMessage falls past both replay branches and writes it to the socket that asked, dropping it if that socket has gone. Re-subscribing after a browser reload therefore recovers nothing — the frame was already delivered to a socket that no longer exists. Both the storyboard board and the timeline sequence had this, and both documented it as the limit of what reattachment could do (raised as F1 on #5612).

The result was never actually lost. Every generate_media call, metered or BYOK, opens a predictions row through createGenerationRun before the provider is called and closes it with its status, its assets and its error; generation-seam-audit.test.ts fails on a provider media call outside that seam. What was missing was the link: the client persisted a request id, the server persisted a row, and nothing connected them. So origin.request_id now carries the client's own RPC request id onto the row, indexed as (user_id, request_id), and a new lookup_generations RPC answers {status, asset_ids, error} for a batch of them — the caller's rows only, capped at MAX_REQUEST_ID_LOOKUP. Both flows resolve their persisted entries through it on open: a row that settled while the client was away lands from the row, and only what the row still calls running is subscribed. This also removes the 30-minute deadline that used to fail such a clip — it no longer has to wait out a window to learn what already happened.

Two degradations are deliberate. An id with no row comes back absent, not failed, and is subscribed for: the row may not be open yet, and reading unknown as failed would discard a render still in flight. A lookup that cannot run at all — an older server, a socket that will not connect — yields an empty map, so every entry falls through to a subscription and the behaviour is exactly what it was before this existed.

lookup_generations is not in APP_SESSION_COMMANDS, so a deployed app's visitor cannot call it. app-session-scope.test.ts pins that set's size, so adding it later has to be deliberate.

Verification

  • npm run test:affectedpackages/models 905/905, packages/execution 17/17, packages/websocket rpc-readonly-commands 13/13, web 526 tests across 61 suites
  • npm run typecheck — web exit 0; packages/{models,execution,websocket} tsc --noEmit exit 0
  • npm run lint — exit 0, and .oxlintrc.anti-slop-enforced.json exit 0 on every changed tree
  • npm run dev:nodetool -- harness gate --base origin/main6/6 selfchecks passed
  • npm run capabilities:check — current at 275 (no capability added or re-declared)

Every behavioural change was observed failing first:

Inverted Failing output
Drop request_id from openRow AssertionError: expected null to be 'req-abc'
Drop the lookup from reattachSequenceJobs 3 failed — the reload cases land no asset
Drop the lookup from reattachBoardJobs 2 failed — the shot keeps no version
Drop clearInFlight from landDirectGen Received length: 2 — the same take appended twice
Drop request_id from the bootstrap DDL SqliteError: no such column: "request_id"

The migration was applied to a real table lacking the column: it adds the column and the index, keeps existing rows, runs twice without error, and the (user_id, request_id) query the lookup makes returns the row.

Three schema checks each caught a real omission before I found it myself — the bootstrap DDL, then the Postgres schema (schema-pg/predictions.ts, which I had wrongly assumed did not exist), then the migration chain. That is the parity suite doing its job, and the reason the column reaches all four places.

Agent capabilities

No capability added, and no capability's declared contract changed. capabilities:check passes at 275.

New checks

No new rule or audit — the new tests pin behaviour rather than adding a check. Each was inverted once and observed failing; the output is in the table above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4


Generated by Claude Code

A `generate_media` reply is an `rpc_response` carrying no `job_id` and no
`thread_id`, so `WebSocketClientSession.sendMessage` falls past both replay
branches and writes it to the socket that asked, dropping it if that socket has
gone. Re-subscribing after a browser reload therefore recovers nothing: the
frame was already delivered to a socket that no longer exists. The storyboard
board and the timeline sequence both had this, and both documented it as a
limit of what reattachment could do.

The result was never actually lost. Every `generate_media` call — metered or
BYOK — opens a `predictions` row through `createGenerationRun` before the
provider is called and closes it with its status, its assets and its error;
`generation-seam-audit.test.ts` fails on a provider media call outside that
seam. What was missing was the link: the client persisted a request id, the
server persisted a row, and nothing connected them.

So `origin.request_id` now carries the client's own RPC request id onto the
row, indexed as `(user_id, request_id)`, and `lookup_generations` answers
`{status, asset_ids, error}` for a batch of them — the caller's rows only,
capped at `MAX_REQUEST_ID_LOOKUP`. Both flows resolve their persisted entries
through it on open: a row that settled while the client was away lands from the
row, and only what the row still calls `running` is subscribed. An id with no
row comes back absent and is subscribed for, because the row may not be open
yet and reading unknown as failed would discard a render still in flight. A
lookup that cannot run at all yields an empty map, so an older server degrades
to exactly the behaviour it had before.

This removes the 30-minute deadline that previously failed such a clip: the
clip no longer has to wait out a window to learn what already happened.

Landing from the row also clears any subscription still open for that clip.
Reopening a sequence inside one page session leaves the earlier subscription
live, and without the clear its late reply appended the same version a second
time — the test fails with two identical takes.

`lookup_generations` is not in `APP_SESSION_COMMANDS`, so a deployed app's
visitor cannot call it; `app-session-scope.test.ts` pins that set's size, so
adding it later has to be deliberate.

Verified: the migration applied to a table without the column adds it and its
index, keeps existing rows, and runs twice without error; the schema-parity,
dialect-parity and migration-chain tests each caught a real omission first (the
bootstrap DDL, then the Postgres schema, then the chain).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
…iption

Blocking bug in the previous commit, found in review. Recovering a request the
row already called terminal worked; the `running` case did not, and it was the
common one.

`GlobalWebSocketManager.subscribe` is a client-side map with no replay buffer,
and the server writes an `rpc_response` to the socket that asked. So a reply
that lands while that socket is gone reaches nobody, and a handler installed
afterwards on a new socket has nothing to receive. Subscribing to a request the
row still calls `running` therefore waits forever — and because the previous
commit removed the reattachment deadline, it waited forever instead of failing
after thirty minutes. That was a regression on this path.

The row is now what recovers. `watchGeneration` polls `lookup_generations` for
every outstanding id in one batched call, backing off from 2s to 15s, until
each row reaches a terminal state or its own thirty-minute window runs out —
after which the surface fails and offers Retry rather than rendering forever.
The subscription runs alongside and wins when it can, which is only when the
socket is the same one the request went out on. That also closes the window
between a lookup and the subscription that follows it, where a reply arriving
in between reached no handler.

Watching starts at the send, not only at reattachment. A socket that drops and
reconnects without a page reload leaves the reply addressed to a server session
that is gone in exactly the same way, and nothing re-runs reattachment in that
case.

The test that let this through asserted the subscription was installed and
fired the handler by hand, which is not something that can happen after a
reload. The replacements never touch the handler map: the only thing that can
settle them is the row being read. Against the previous commit they fail —
2 in the timeline suite, 2 in the storyboard suite, 1 for the reconnect case.

Two faults in the poller itself, each with a test: a settle callback that threw
abandoned every other request settling in the same tick, which is a whole
board; and a newly watched request inherited the backed-off interval, for which
resetting the delay was not enough because a timer was already armed at the old
one.

Typecheck caught a dead line in the new test — `loadSequenceId` does not exist,
and optional chaining had made the call a silent no-op.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

You're right on both counts, and the test I wrote is the tell. Fixed in 40e57068.

"subscribes when the row still says running, so the reply can land" fired the handler by hand. That proved the subscription was installed and nothing about whether a frame could ever reach it — after a reload none can, because the reply went to a socket that no longer exists and subscribe is a client-side map with no replay. And since the first commit removed the reattachment deadline, the result was a permanent hang rather than a thirty-minute one. That path was a regression, not just an incomplete fix.

The row is now the recovery mechanism; the subscription is only a latency optimization. watchGeneration polls lookup_generations for every outstanding id in one batched call, backing off 2s → 15s, until each row is terminal or its own thirty-minute window runs out — then the surface fails and offers Retry. The subscription runs alongside and wins when it can, which is only when the socket is the same one the request went out on. That covers the lookup → subscribe race you named too: the poll doesn't care when the handler was installed.

Watching starts at the send, not only at reattachment. Fixing yours surfaced an adjacent hole: a socket that drops and reconnects without a page reload leaves the reply addressed to a dead server session in exactly the same way, and nothing re-runs reattachment in that case. Making the row authoritative everywhere closes both and leaves one invariant instead of two.

The replaced tests never touch the handler map — the only thing that can settle them is the row being read. Against the previous commit: 2 red in the timeline suite, 2 red in the storyboard suite, 1 red for the reconnect case.

Two faults in the poller itself, found re-reading it and each now tested: a settle callback that threw abandoned every other request settling in the same tick (a whole board), and a newly watched request inherited the backed-off interval — resetting the delay wasn't enough, since a timer was already armed at the old one. Typecheck also caught a dead line in the new test (loadSequenceId doesn't exist; optional chaining had made the call a silent no-op).

I chose polling over a server-side replay mechanism: it needs no change to the session lifecycle and reuses the row this PR already adds. Steady-state cost is one batched RPC per tick while anything is in flight, and none otherwise.

Verified: harness gate 6/6, tsc -p web/tsconfig.json exit 0, 426 tests across the 44 suites this touches (4930 across 440 for the wider web tree), lint and enforced anti-slop exit 0.


Generated by Claude Code

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

CI on the superseded head 28efc176

Two Quality Gate jobs failed on that head. Neither is this PR's, and the head has since moved to 0ae252f4 (main merged in), which is re-running everything.

harness-gate — fixed. Not a test failure: all 6 selfchecks passed with exitCode: 0. It failed on two mapping violations:

preview_timeline_frame changed its declared contract, but its coverage mapping is unchanged
render_timeline changed its declared contract, but its coverage mapping is unchanged

Both come from main, not from here. Main moved 20 commits ahead (ba61f940dbc39dad) and changed packages/agents/src/capabilities/timelines.ts and timelines.specs.ts, which declare them; this branch predated that, so the gate diffing it against current main read those contracts as changed backwards to their pre-main form. Reproduced locally with harness gate --base origin/main --dry-run (identical two violations), then merged main and reproduced the absence — 6/6, no mapping violations. Those files are no longer in this PR's diff.

test-packages-nodes — not this PR's. @nodetool-ai/blender-nodestests/render-animation.test.ts"renders an orbit sweep with the expected frame count that differs frame to frame", Error: Test timed out in 300000ms. A timeout in a real Blender render, not an assertion.

git diff ba61f940..dbc39dad -- packages/blender-nodes/ is empty, and this PR touches no blender, 3D, video-encode or render code — its diff is a predictions.request_id column, a lookup_generations RPC, and two web recovery paths. The test itself runs two full 5-frame orbit renders, each carrying its own timeoutMs: 300_000 job budget, inside a single 300s vitest timeout — so it cannot absorb a slow runner. Its siblings in the same file took 25–37s each and passed, the job took 403s overall, and the queue was deep at the time (9 Test runs pending repo-wide, including one on main).

I have not re-run that job: the head it ran on is superseded, and test-packages-nodes is re-running on 0ae252f4 now, which is the re-run that matters. If it fails there too I will treat it as reproducible and dig further rather than call it flaky again.

Verified locally on the merge result before pushing it: build:packages 62/62 · harness gate --base origin/main 6/6 · web typecheck 0 errors · models 907/907 · execution 17/17 · websocket 24/24 · web 426 across 44 suites · lint and enforced anti-slop clean.


Generated by Claude Code

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Run workflow suite in browser on 0ae252f4

Failing check: Run workflow suite in browsertests/e2e-runner/suite.spec.ts"workflow suite executes and records artifacts", with manifest should list workflows — Expected: > 0, Received: 0. It passed on both earlier heads (28efc176, 40e57068) and failed on the merge, so I treated it as mine until shown otherwise.

It is a timing budget, not a logic error. The spec waits for the manifest and deliberately swallows the timeout before asserting:

await page
  .waitForFunction(() => (window.__E2E__?.manifest().length ?? 0) > 0, undefined, {
    timeout: 90_000
  })
  .catch(() => {});

const total = await page.evaluate(() => window.__E2E__?.manifest().length ?? 0);
expect(total, "manifest should list workflows").toBeGreaterThan(0);

So a manifest that arrives after 90s produces exactly this failure — Received: 0 with no other diagnostic. The CI run failed at 1.6m ≈ 96s, just past that budget, and its own globalSetup had already logged Prepared suite with 180 workflows with the backend up on 2837 nodes. The suite was prepared; the browser had not received it inside the window.

Reproduced the opposite locally on this exact commit: ✓ 1 [chromium] › tests/e2e-runner/suite.spec.ts:18:1 › workflow suite executes and records artifacts (2.0m) — 1 passed, exit 0. The whole test took 2.0m here, i.e. the manifest landed inside 90s and the workflow execution filled the rest.

Nothing in this PR can slow that path. The diff adds a predictions.request_id column, a lookup_generations RPC, and two client recovery paths; generationWatch only does work once watchGeneration is called from a direct-gen send or a reattach, neither of which the workflow suite exercises. And if any of the new modules threw at import the app bundle would fail everywhere, not just here — it passes locally on the same code.

I have re-run the failed job once (the single re-run these rules allow, previously unspent — I did not spend it on the superseded 28efc176 blender timeout). If it fails again I will treat it as reproducible and root-cause it rather than call it timing twice.


Generated by Claude Code

@georgi
georgi enabled auto-merge September 6, 2026 09:10

@georgi georgi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff. Well-engineered feature that solves a real problem (lost generate_media results across page reloads). The implementation is sound:

  • landDirectGen as a single landing function for both socket reply and row lookup, with clearInFlight at the top preventing duplicates
  • lookupGenerations never throws; falls through to subscription-only behavior on failure
  • generationWatch batches all watched ids into one RPC per tick
  • Schema migration is safe and idempotent (nullable column + IF NOT EXISTS index)
  • Prediction.byRequestIds filters by userId (no cross-user leakage), bounded by MAX_REQUEST_ID_LOOKUP = 128
  • Correctly excluded from APP_SESSION_COMMANDS

One issue: StoryboardGenerationStore.ts has an indentation error in handleShotJobMessage. The lines after const response = ... inside the if (message.type === "rpc_response") block are indented at the outer function body level instead of inside the if block. Behavior is unaffected (braces scope correctly) but it misleads readers.

CI: test-app failure is pre-existing on main. Some other checks are still running.


Generated by Claude Code

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

test-app on 0ae252f4 — red on main, not from this PR

Quality Gate / test-app fails on 1 suite of 1360: web/src/components/timeline/Inspector/__tests__/TimelineInspector.motionControls.test.tsx, 4 of its 14 tests (15009 other tests pass).

It fails identically on clean origin/main. I checked out dbc39dad in a separate worktree with none of this branch's commits and ran the same suite:

✕ writes the stagger unit and the unit it starts from
✕ captions a stagger that runs past the clip
✕ says when in and out together outrun the clip
✕ captions an easing this build cannot read
Test Suites: 1 failed, 1 total   Tests: 4 failed, 10 passed, 14 total

Same four, same order, same counts as on this branch. The file arrived here in the origin/main merge (0ae252f4); this PR touches no Inspector code.

What is actually wrong

All four assert UI that does not exist. Not a regression — the tests describe unbuilt behaviour:

Test Assertion State of the source
stagger unit combobox named /in animation stagger unit/i No unit control. ClipAnimations.tsx:330 hardcodes unit: "word", and the panel renders only the offset field, labelled "<role> animation word stagger offset"
stagger past clip text /staggered over 4 words this runs 3100ms/i String appears nowhere outside the test
in + out outrun clip text /in and out together run 1400ms/i String appears nowhere outside the test
unreadable easing /not an easing this build reads; plays linear/i on a custom-curve keyframe UNPARSEABLE_EASING_HINT exists (InspectorMotionFields.tsx:82) and renders on the animation's own easing field — that sibling test passes — but the custom-curve keyframe rows never render it

grep for those three caption strings across web/src outside __tests__ returns nothing.

Proposed patch (for whoever owns the motion-graphics work)

Two coherent options, and the choice is a product call about what actually shipped, not something to guess at from a red test:

  1. Build the UI the tests describe — add a stagger-unit select bound to animation.stagger.unit (replacing the hardcoded "word" at ClipAnimations.tsx:330, with the aria-label the test names), add the two over-run captions, and render UNPARSEABLE_EASING_HINT on custom-curve keyframe easing fields as well as the animation's own.
  2. Bring the tests to what shipped — drop or adjust the four assertions to the controls that exist.

I have not made either change here: it would widen a PR about generate_media recovery into the timeline Inspector, and it is not mine to decide which way it goes.

I have not re-run this job — it is deterministic and reproduces on main, so a re-run would only repeat it. My one re-run went to the Run workflow suite in browser timing failure above.

This blocks more than this PR. test-app is a required check, so every PR merging into main is red on it until this is resolved — including anything with auto-merge armed.


Generated by Claude Code

Review finding. Splitting `handleDirectResponse` into `settleDirectShotJob`
moved its body into the `rpc_response` branch of `handleShotJobMessage` but left
the lines at the outer function's indent, so the block reads as if it runs
unconditionally. Braces scope it correctly and behaviour is unchanged; it only
misleads whoever reads it next.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Indentation fixed in aadfd065.

You were right about the cause: splitting handleDirectResponse into settleDirectShotJob moved its body into the rpc_response branch of handleShotJobMessage and I left the lines at the outer function's indent, so the block reads as unconditional. Braces scoped it correctly, so it was purely a reading hazard — now indented inside the if.

Verified: src/stores/storyboard 112/112 across 5 suites, tsc -p web/tsconfig.json 0 errors, oxlint clean.

On CI, agreeing with your read and adding the evidence: test-app is red on main itself. I ran the failing suite against dbc39dad in a worktree with none of this branch's commits and got the same four failures, same counts — details and a proposed patch in the comment above. The quality job that just went red is the rollup of that same test-app, so no separate cause.


Generated by Claude Code

@georgi
georgi merged commit 7b91719 into main Sep 6, 2026
23 of 24 checks passed
@georgi
georgi deleted the claude/generate-media-resume branch September 6, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants