fix: serialize report saves, CAS response-id chain, request-shape contract tests#22
fix: serialize report saves, CAS response-id chain, request-shape contract tests#22m-szymanska wants to merge 4 commits into
Conversation
Two concurrent POST /api/save requests interleaved their read-modify-write of report.json (load -> merge session markers + posted human review -> atomic write). The atomic write (BH30 fsync+replace) guards against a torn file, but the second writer's load could predate the first writer's replace, so it overwrote report.json with a stale snapshot and dropped a reviewer's verdict (last-writer-wins). The session threading.RLock cannot fix this: every save coroutine runs on the one event-loop thread, so the reentrant RLock never blocks a second coroutine. Wrap the whole load -> merge -> write cycle in a dedicated per-report asyncio.Lock so overlapping saves serialize. Saves are short local file I/O, so full serialization is cheap. Add a concurrency test (real asyncio.gather over an ASGITransport client) asserting the file load/dump events serialize as [load, dump, load, dump] rather than the interleaved [load, load, dump, dump], and that the reviewer's verdict survives a concurrent body-less save. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both dev servers chain VLM/STT calls through previous_response_id: an operation reads the current session.last_response_id, runs a long UNLOCKED upstream call, then writes the new id back under the lock. Two overlapping operations both read the same head; the one that wrote second clobbered the id the first already committed, so the next analysis chained from a stale or overwritten id (wrong response_id in the conversation chain). Replace the unconditional write-back with a compare-and-set: advance the head only when the id is non-empty (the existing BH29/BH45/BH46/BH49 empty-id guard, folded in) AND the head still equals the value read at the start of the operation. A concurrent operation that finished first owns the newer id; the loser keeps it and logs at debug instead of overwriting. The STT paths now also snapshot the head before the unlocked call so they compare-and-set too. The shared helper advance_response_id_cas lives in server_common so both route-twin servers apply one implementation across all six call sites (review + analyze, VLM + STT). Add real asyncio.gather concurrency tests for both servers proving a slow analysis returning later loses the CAS instead of clobbering the newer head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces concurrency controls to prevent race conditions and data clobbering in both the analyze and review servers. Specifically, it implements a compare-and-set (CAS) mechanism (advance_response_id_cas) to safely update the shared conversation-chain head (last_response_id) under a lock. Additionally, it serializes the read-modify-write cycle of the review state saves using an asyncio.Lock to prevent concurrent saves from overwriting each other. Comprehensive concurrency and request-shape contract tests are also added. Feedback suggests using Python's async with context manager for save_lock instead of manual acquisition and release to make the code cleaner and more robust.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Serialize the entire load->merge->write cycle: a concurrent save must | ||
| # not load report.json before this one's atomic replace lands, or it | ||
| # would overwrite with a stale snapshot and drop this save's verdicts. | ||
| await save_lock.acquire() |
There was a problem hiding this comment.
While the manual acquire() and release() implementation is correct and safe here, using Python's async with save_lock: context manager is the idiomatic and robust way to manage asyncio.Lock. It automatically handles lock acquisition and release, ensuring the lock is always released even if unexpected exceptions (such as BaseException or asyncio.CancelledError) are raised, and makes the code cleaner and more maintainable.
1 participant
Summary
Three commits hardening the server concurrency contract and pinning the outgoing request shapes:
894e1e5) — two overlapping/api/savecalls could interleave load→merge→write and silently drop the first writer's changes (last-writer-wins). The existingsession.lock(a threading RLock) never protected this path: all save coroutines run on the same event-loop thread, so a reentrant lock never blocks a second coroutine. A dedicatedasyncio.Locknow serializes the full cycle.f96e9e4) — the response-chain head was read under lock, the VLM/STT call ran outside it, and the write-back could clobber a newer id. A sharedadvance_response_id_cashelper (server_common.py) writes only when the head still matches the value read at operation start; the losing racer keeps the newer id. Applied symmetrically to all six read/write sites across both servers.5ec507d) — offline contract tests (existing MockTransport pattern, no network) pinning method/path, Bearer auth, multipart file field, chat-completions message structure, stream flag, and the base64 image part — so client-side schema drift fails in CI instead of surfacing in the live nightly.Test plan
asyncio.gatherraces and were mutation-verified: with the fixes stashed, all three fail; with the fixes, green 3× in a row.make lintclean,uv run mypy screenscribeclean, fulluv run pytest -q: 1207 passed, 51 skipped.🤖 Generated with Claude Code