concurrency-review

Concurrency and data-integrity review lens

Race and crash defects pass every sequential test, so they sail through review unless someone plays the bad schedule on purpose. Live runs shipped exactly this class twice: concurrent writers sharing one temp-file name (HTTP 500), an unsynchronised read-modify-write (lost updates), and a time-only stale-lock lease that let a second process break the lock of a holder that was merely paused — losing a write the first process had already acknowledged. Every reviewer scored correctness 4/5. This lens exists so that does not happen again.

A finding here is a concrete defect only when you can write the schedule: the ordered steps (who reads, who writes, who pauses or crashes, when) that end in a wrong, observable outcome — a lost acknowledged write, a corrupt or partial file, a duplicate effect, a 500, a deadlock. "Consider adding a lock" without a schedule is a note.

Steps

1. Establish the concurrency shape. Can the changed code run twice at once? An HTTP handler can (servers serve requests concurrently; Node interleaves at every await); so can workers, cron jobs that overlap, CLI commands a user can start twice, multiple processes or instances sharing one file or database. Check how the app is deployed (worker count, cluster mode, replicas) and call search_lore for the repo's locking, transaction and persistence conventions. If there is provably one writer and one thread, write "single writer — n/a" and stop.
2. List the shared state the diff touches: files and directories, database rows, in-memory maps/counters/caches, lock files, queues, external resources. For each, name who reads it, who writes it, and what guards it.
3. Walk the checklist below against the actual code, opening the files. For each relevant item write "ok — " or a finding.
4. Play the four schedules against every write path: (a) two writers interleave step by step; (b) the process is paused (GC, SIGSTOP, VM suspend) for longer than any timeout or lease, then resumes and continues; (c) the process crashes between any two writes, and between "write" and "acknowledge"; (d) the caller retries after a timeout, so the operation runs twice. Write the schedule for each failure.
5. Check the tests reach the path. If an AC names concurrency, locking, atomicity, durability, crash safety or retries, a test must actually run that schedule: real parallel writers (separate processes when the AC says cross-process), an injected pause or kill, a forced retry, with an assertion on the invariant (count equals N, every acknowledged write present, file parses, one effect per key). A sequential test of a concurrency AC is a missing test (the test-quality-review skill).
6. Rate each finding. Blocking: a written schedule reachable in the deployed shape that loses or corrupts data, loses an acknowledged write, duplicates a side effect, returns a 500, or deadlocks; or an AC about concurrency/durability with no test that exercises it. Should-fix: the defect needs an unusual but possible schedule (a crash at one exact point, a pause beyond a generous lease). Note: hardening with no schedule. Only blocking and should-fix justify RECOMMEND CHANGES.
7. Record each finding with record_ac_evidence (evidence_type: manual_note, naming the AC, file, line, the schedule, and the single concrete fix), then let the review-ticket verdict carry the result. Never patch the code yourself.