fix(jobs): atomic dot-path state_data writes (distribution + audio-edit races) - #974
Merged
Merged
Conversation
…edit paths Follow-up sweep to the file_urls/state_data lost-update fix (#970). Converts the remaining read-copy-mutate-write-whole-map sites to atomic Firestore dot-path writes so a stale snapshot can't clobber a concurrent sibling key: - video_worker distribution/redistribution/finalize (3 sites): the most dangerous — they rewrote the whole state_data map and popped visibility_change_in_progress, so a stale write could revert the worker_generation supersession fence (an Increment) or resurrect the visibility guard. Now dot-path writes + DELETE_FIELD for the guard. - audio-editor undo/redo/apply/upload (review.py): full-map rewrites that could race with rapid clicks or the idle-reminder scheduler active during audio-edit. - deferred YouTube-URL write (youtube_queue_processor). Left the two low-risk fresh-dict-replace sites (create-from-search / admin re-search) — single-request, early-lifecycle — out of scope. Tests: updated the 2 unit tests that asserted the old shape; added an emulator regression test proving the distribution write preserves the worker_generation supersession fence and clears the visibility guard (with a contrast test showing the old full-map write reverted the fence). Full backend suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…abbit) Address CodeRabbit findings on the audio-editor concurrency: - Uploads: write only this upload's entry keyed by its uuid via FieldPath(...).to_api_repr() (backtick-escapes the hyphenated uuid, keeps the key a string so it doesn't clash with update_job's 'updated_at'). Concurrent uploads now merge instead of the last write dropping the other's entry (which orphaned a GCS file and 404'd on join_start/join_end). Emulator test added. - Edit-stack lists: documented as a known limitation — they remain last-write-wins across concurrent edits to the same job (a list can't be dot-path-merged); acceptable for the single-reviewer sequential UI, a transaction/CAS would be needed for true multi-writer safety. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
beveradb
enabled auto-merge (squash)
September 2, 2026 20:27
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
1 participant
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
Follow-up hardening sweep to the
file_urls/state_datalost-update fix (#970). An audit found the same read-copy-mutate-write-whole-map pattern in 8 more sites; this converts them to atomic Firestore dot-path writes so a stale snapshot can't clobber a concurrent sibling key.Changes
video_workerdistribution / redistribution / finalize (3 sites) — the most dangerous: they rewrote the wholestate_datamap and.pop('visibility_change_in_progress'), so a stale write could revert theworker_generationsupersession fence (an atomicIncrement) or resurrect the visibility guard — the "visibility-recycle-dataloss" surface. Now dot-path writes, withDELETE_FIELDfor the guard.review.py) — full-map rewrites that could race with rapid clicks or the idle-reminder scheduler active during audio-edit → dot-path writes.FieldPath(...).to_api_repr()(backtick-escapes the hyphenated uuid, string key), so concurrent uploads merge instead of one dropping the other (which orphaned a GCS file and 404'd on join). (CodeRabbit finding Improve readme Windows instructions. #1.)youtube_queue_processor) → dot-path.Out of scope (documented)
audio_edit_stack/audio_edit_redo_stack) remain last-write-wins across concurrent edits to the same job — a list can't be dot-path-merged. Acceptable for the single-reviewer sequential audio-editor UI; true multi-writer safety needs a transaction/CAS. Documented inline. (CodeRabbit finding Major refactor, rename to Karaoke Gen, implement tests for everything #2 — deliberately deferred.)Testing
worker_generationfence + clears the visibility guard (with a contrast test showing the old full-map write reverted the fence); concurrent audio-edit uploads both survive.Review
@coderabbitai ignore
🤖 Generated with Claude Code