Address storyboard scenes by durable id, and cancel the orphaned job when a stage write rejects - #3426
Merged
Conversation
…write reject (#3413) Scenes were addressed purely by array index end to end — the route took sceneIndex, the media-job owner encoded scene<idx>, and the stage write patched scenes[index]. #3400 closed the clobbering race but left the mis-targeting one: a reorder or delete landing between the caller's read and the locked write retargeted the job id onto whatever scene occupied that slot whenever the index was still in range, and the completion hook routed the finished render by the same stale index. Scenes (and shots) now carry a durable, deterministic id stamped by sanitizeVisualStage and backfilled by migration 222 (file store) plus db-migration 007 (Postgres). Deterministic rather than random so two concurrent readers of an un-migrated record — and two federated peers running the backfill independently — derive identical ids. Every write region re-resolves the captured id against the fresh array and 409s (PIPELINE_SCENE_STALE_TARGET / PIPELINE_SHOT_STALE_TARGET) when it is gone, distinct from the 404 for an index that never existed; the index stays as the fallback only for pre-migration records. Owners carry the ids percent-encoded, and the filename hook still attaches jobs queued under the legacy index-only owner format. The render surfaces all enqueue the job before persisting it, so a rejected write left the job queued with nothing referencing it — burning GPU time for an artifact nothing points at. persistRenderOrCancel wraps the persist step for storyboard scene/shot renders, comic page slots, and issue/volume covers: on reject it cancels the job it just enqueued and rethrows the original error.
…malization (#3413) Review findings on the first pass: - The file-store migration rewrote each issue with a truncating writeFile. An interrupted rewrite would leave unparseable JSON that the next pass silently skips — permanent loss for that issue, since the migration is already marked applied. Uses atomicWrite (temp + rename) instead. - A list that ALREADY carried duplicate ids kept them: the resolver tie-breaks on the captured index, so once two same-id entries are reordered the write lands on the wrong one — the exact mis-target this change prevents. Later copies are now re-stamped; the first occurrence keeps the id, matching sanitizeShots' first-wins resolution of continuityFromShotId.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3413. Follow-up to #3400 / PR #3411.
Storyboard scenes were addressed purely by array index end to end — the route took
sceneIndex, the media-job owner encodedscene<idx>, and (post-#3400) the locked write patchedscenes[index]. Two consequences remained:comicPages.js#persistComicPageSlotandcovers.js#renderComicCoverLikehad the identical shape.1. Durable scene ids
server/lib/storyboardScenes.js(new):ensureStoryboardIds(scenes)stamps a missingidon each scene and eachshots[]entry;resolveStoryboardTarget(list, { id, index })→{ index, record, matchedBy, stale }.sanitizeVisualStage's storyboards branch calls the stamp, so every record grows ids on its next write.Ids are deterministic (
scene-01,shot-02, collision-escaped toscene-01-2) rather than random, on purpose: two concurrent readers of an un-migrated record must derive the same id, and every federated peer runs the backfill over its own copy of a shared issue — random ids would make each peer stamp something different and churn conflicts forever.2. Backfill migrations (both stores)
scripts/migrations/222-storyboard-scene-durable-ids.js— the file-backed store (data/pipeline-issues/{id}/index.json): still live underMEMORY_BACKEND=file/tests, and still the source the one-timemigrateIssuesToDBimport reads on installs not yet on Postgres, so stamping here first carries ids across the import.server/scripts/db-migrations/007-storyboard-scene-durable-ids.js— thepipeline_issuesrows. The file runner executes before the DB pool exists, so a row transform cannot live inscripts/migrations/.Both are idempotent and deliberately do not bump
updated_at/updatedAt— a derived normalization must not advance the LWW clock and out-race real remote edits.3. Resolve by id inside the write region
patchStoryboardScenenow takes{ id, index }captured during the caller's read and re-resolves insideupdateStageWithLatest. A captured id that is gone is 409PIPELINE_SCENE_STALE_TARGET(shots:PIPELINE_SHOT_STALE_TARGET), distinct from the existing 404 for an index that never existed; the index is used only when no id was captured. The three callers (scene video, shot start-frame, prompt refine) return the index the write landed on, not the stale read index.4. Completion path
buildStoryboardsShotOwnernow emits…:scene<N>:shot<M>:sid<sceneId>:tid<shotId>(ids percent-encoded so a:inside one can't break the parse), plus abuildStoryboardsSceneOwnerfor the scene-level video render.parseStoryboardsShotOwnerparses legacy index-only owners withsceneId: null, shotId: null, andstoryboardsFilenameHook.jsresolves by id with an index fallback — so jobs already sitting in the queue at upgrade time still attach.5. Orphan cancel
persistRenderOrCancel(jobId, persist, label)invisualStageHelpers.jswraps the persist step: on reject itcancelJobs the job it just enqueued, logs the cleanup, and rethrows the original error. Applied to the storyboard scene/shot enqueues,persistComicPageSlot,renderComicCoverLike, andrenderVolumeCoverLike.Test plan
cd server && NODE_ENV=test npx vitest run— full suite green (1183 files / 24,369 tests; DB suites skip as designed, no writes to the realportosDB).server/lib/storyboardScenes.test.js— determinism, collision escape, idempotence-by-reference, id-vs-index resolution, duplicate-id tiebreak, absent-index sentinel.server/services/pipeline/visualStages.test.js— driven through the interleaved-write harness from fix: merge storyboard scene writes against fresh state so sibling renders don't clobber job ids (#3400) #3411: a reorder that keeps the index in range does not move the job id onto the wrong scene (scene and shot level); a scene/shot removed between read and write 409s instead of mis-writing; a rejected write leaves noqueuedjob for that owner (asserted via alistJobs({ owner })stand-in) and the job is recorded canceled; a pre-migration record with no ids still resolves by index; rejected comic-page and cover writes cancel their jobs too.server/services/pipeline/storyboardsFilenameHook.test.js(new) — id routing after a reorder, legacy index-only owner still attaches, stale-jobId and vanished-scene skips.server/services/pipeline/owners.test.js— new owner round-trip, percent-encoding, legacy parse.