Reviewing Shared State by Asking What Happens Under Contention

The code review read fine. A shared counter, a mutex around the update, a cache with a lock guarding the refresh. In the single threaded narrative of reading the diff, every access is protected and the logic is sound. Under load, it deadlocked, or thundered, or returned stale values, because review is a single reader and production is a thousand.

Shared state defects are the hardest class to review because the defect is not in the code as written, it is in the code as scheduled. The reviewer must simulate contention in their head, and this is the checklist I use to do that systematically.

This spans Go 1.23, Node 22.14 and Postgres 16.3, because shared state is shared state regardless of the runtime.

The first question: is the state actually shared

Half of shared state bugs are state that does not need to exist. Before reviewing the locking, review the sharing. A package level map, a module level cache, a global client with mutable fields. Ask whether the state can be scoped to the request or the connection instead. The cheapest lock is the one you delete by removing the sharing.

In Node, the single threaded model hides this. There is no data race on a variable, but there is still shared state across async calls, and the classic check then cache pattern is still a race between two awaits, which is the shape in the cache that returned another customer's data.

The lock scope question

When there is a lock, the scope is the defect surface. The bug is rarely the lock itself. It is the work being done while holding it.

A lock held across a network call, a database query or a slow computation serialises that entire operation, turning the lock into a global bottleneck. The review question is: what is the minimum critical section, and is everything inside it necessary. If the critical section contains IO, that is a blocking comment, because the lock now couples every holder to the latency of the slowest IO.

The flip side is a lock released too early, between a check and an act, which is the check then act race. If the code checks a condition under the lock, releases, then acts, the condition may have changed. The check and the act must share one critical section, or the check must be revalidated at the act.

The lock ordering question

Deadlock needs two locks and two orders. The review technique is to find every place that holds more than one lock, and write down the acquisition order. If any two code paths acquire the same pair in opposite orders, that is a deadlock waiting for the right timing, and it will reproduce once in a thousand runs and never in CI.

The fix is a global ordering, documented once, and enforced by review. If the codebase cannot maintain an ordering because locks are acquired deep in helpers, that is itself the signal that the locking design is too entangled and should be simplified, perhaps by having a single owner of the combined state.

The thundering herd question

A lock that guards an expensive refresh produces a herd. The first caller refreshes, and every caller that arrived while it was refreshing waits, then finds the value fresh and does nothing. If instead the lock is released before the refresh, every caller refreshes at once.

The review wants the single flight pattern: one in flight computation per key, and waiters attach to its result rather than re computing. If the code has a cache with a lock but no single flight, the contention behaviour is either serialised waiting or duplicated work, and both are defects worth naming.

The database version of the same questions

In Postgres, the shared state is rows and the locks are row locks and isolation levels. The review questions translate directly. Is the read modify write atomic, or is it a select then update across which another transaction can interleave, the shape in postgres deadlock detected. Is the transaction holding locks across application level work, which is the held lock across IO defect in a suit. Is there a consistent lock ordering across transactions that touch the same rows, or will two jobs deadlock each other nightly.

SELECT ... FOR UPDATE with a defined access order is the database's lock ordering discipline, and a transaction that does slow work between the select and the commit is the held lock defect.

What I look for as proof

The strongest signal that shared state was reviewed rather than hoped for is a test or a comment that names the contention. A test that spawns concurrent callers and asserts a single refresh, a comment explaining the lock order, a lint that bans IO in the critical section. Absence of any of these means the contention behaviour has never been examined, and the review is the first and only chance.

I also ask the author to narrate the schedule: two callers arrive simultaneously, walk me through what each sees. This is the same technique as asking "what happens when this fails" for error paths in reviewing error handling paths, applied to time instead of failure. The author either describes it cleanly or discovers the gap, and both outcomes are valuable.

The rule

Shared state is reviewed by simulating schedules, not by reading code. Delete the sharing you can, shrink the critical section to exclude IO, keep check and act atomic, enforce one lock order, and make refreshes single flight. If the diff cannot answer what two simultaneous callers see, it has not been reviewed for the condition under which it will actually run.

The observable symptom of getting this wrong at runtime is the latency tail and the herd, covered in event loop lag for the single threaded runtime and p99 and why averages lie for the queueing it produces.