…ders don't clobber job ids (#3400)
enqueueStoryboardSceneVideo, enqueueStoryboardShotStartFrame, and
refineStoryboardScenePrompt each read stages.storyboards.scenes at entry and
wrote the whole pre-read array back through updateStage. updateStage does
serialize on the series write tail, but it shallow-merges the `scenes` value
the caller built OUTSIDE that lock — so two enqueues for different scenes that
both read before either wrote would revert each other's freshly-stamped
sceneVideoJobId / startFrameJobId, and the losing media job would run to
completion with nothing referencing it.
All three now go through a shared patchStoryboardScene() that passes a mutator
to updateStageWithLatest, keeping the read-modify-write of `scenes` inside the
write region so only the targeted index is replaced. Mirrors the existing
persistComicPageSlot treatment in comicPages.js. A scene (or shot) that
disappeared between the caller's read and the write now 404s rather than
resurrecting itself into the persisted array.
The audit finding was marked UNCERTAIN pending "does updateStage already merge
serially" — it serializes but does not merge, so the race was real.
Summary
The audit finding was marked UNCERTAIN pending "verify
updateStage's internal serialization first." The race is real —updateStageserializes the writes but does not merge them, so serialization never helped here.updateStage(issueId, stageId, patch)is justupdateStageWithLatest(issueId, stageId, () => patch), and that does queue on the per-series write tail. But all three storyboard write paths readstages.storyboards.scenesat entry, mutated a copy, and passed the whole pre-read array as the patch — a value computed outside the lock. The tail then shallow-merges that stale array over the freshest stage, so the second of two concurrent enqueues reverts the first scene's freshly-stamped job id. The media job keeps running with nothing left pointing at it, and the filename hook (which only attaches a completed render when the target slot already carries the job id) silently drops the result.Fixed by routing all three through a shared
patchStoryboardScene(issueId, index, mutate)that handsupdateStageWithLatesta mutator, keeping the read-modify-write ofscenesinside the write region so only the targeted index is replaced. This mirrors the existingpersistComicPageSlottreatment incomicPages.jsandrenderComicCoverLikeincovers.js— the same fix, already applied to the comic-page surface.Paths changed:
enqueueStoryboardSceneVideo—scenes[idx].sceneVideoJobIdenqueueStoryboardShotStartFrame—scenes[sIdx].shots[tIdx].startFrameJobIdrefineStoryboardScenePrompt—scenes[idx].description. Beyond the issue's stated scope, included deliberately: it is the identical bug in the same file, and with an LLM round-trip sitting between the read and the write it has by far the widest clobber window of the three. Leaving it would have left the fix half-applied.Single-call behavior is unchanged. One deliberate behavior change: a scene or shot that disappeared between the caller's read and the write now 404s (
PIPELINE_SCENE_NOT_FOUND/PIPELINE_SHOT_NOT_FOUND) instead of resurrecting itself into the persisted array — validating against the stale snapshot was the only reason that ever "worked."Per the trust model this is a same-process re-entrancy race from one user's pipeline run (two render buttons, or a fan-out over scenes), not a cross-human race — squarely the "serializing two write paths that mutate the same record" case CLAUDE.md calls expected.
Test plan
cd server && NODE_ENV=test npm test— 24297 passed, 211 skipped, 0 failed. (A first run also flaggeddataManager.categories.test.jswithENOENT … server/test-data; it passes in isolation and on re-run — a parallel-run race over a gitignored runtime dir in a fresh worktree, unrelated to this change.)describe('concurrent storyboard scene enqueues (#3400)')inserver/services/pipeline/visualStages.test.jsinstalls a genuinely stateful serialized write tail (one shared persisted stage, one mutex,computeFnevaluated inside the lock) and drives enqueues concurrently viaPromise.all:sceneVideoJobIds survivestartFrameJobIds survivestoryboards.jsto the pre-fix version and re-ran — all 4 new tests fail with exactly the clobber symptom (expected undefined to be 'job-scene-0'), plus the 3 existing happy-path assertions that now pin the mutator path. Restored, all 126 pass.updateStageWithLatestmock now reads from a per-testpersistedStages(reset inbeforeEach) so the outergetIssueread and the write tail agree on state, andbeforeEachrestores mock implementations rather than only clearing call logs — otherwise the stateful tail installed by these tests would leak into the rest of this 1500-line file.Closes #3400