Systems & Concurrency Code Review Protocol

Systems & Concurrency Code Review Protocol

This skill defines the mandatory protocol for conducting rigorous, adversarial code reviews on Filament source code. Standard single-pass reviews focusing on style or superficial idioms are strictly prohibited. Reviews must prioritize correctness, concurrency invariants, memory models, lifecycle safety, API contracts, arithmetic boundaries, and test integrity.

1. Core Review Philosophy

1. Adversarial Stance: Assume the code contains subtle race conditions, memory ordering errors, and unhandled edge cases until proven otherwise.
2. Zero Superficial Feedback: Do not waste review bandwidth on formatting, whitespace, or obvious variable naming unless it actively impairs correctness or violates CODE_STYLE.md.
3. Cross-Function & State-Machine Tracing: Never review functions in isolation. Concurrency bugs (e.g., lost wakeups, ABA races) live at the intersection of multiple functions (e.g., put(), wakeOne(), waitForWork(), loop()). Trace full execution state machines across caller/callee boundaries.
4. False-Positive Elimination: Before reporting a finding, attempt to disprove it by searching for compensating synchronization, refcount credits, or structural invariants elsewhere in the file or call chain.

2. Mandatory Analytical Lenses (The 6 Pillars)

Every review must systematically evaluate the change through these six analytical lenses:

Pillar 1: Concurrency, Synchronization & Memory Models

• Atomic Memory Ordering & Barrier Direction:
  • Inspect every std::atomic operation and its std::memory_order (relaxed, acquire, release, acq_rel, seq_cst).
  • Verify barrier directions: memory_order_release prevents prior memory writes from sinking below the release store, but does not prevent subsequent non-atomic writes from floating above it. memory_order_acquire prevents subsequent reads from floating above the acquire load, but does not protect prior reads.
• Lost Wakeups & Condition Variable Invariants:
  • Check if condition variable signals (notify_one() / wakeOne()) or waiting loops (wait()) are gated by unlocked relaxed loads (e.g., relaxed check of waiter counts).
  • Verify that the happens-before relationship between producer publication and consumer sleep is never bypassed.
• ABA, Slot-Reuse & Torn Reads:
  • In wait-free, lock-free, or seqlock data structures, trace multi-step read-then-validate patterns.
  • Ensure that slot identity (e.g., thread ID, generation counter) is re-validated after copying payload data, not just the slot state, to prevent returning payload data read during an erase/re-emplace.
• Data Races in Lock-Free / Wait-Free APIs:
  • Verify that plain (non-atomic) fields are never read concurrently with plain writes in other threads without a formal synchronization barrier.
• Deadlock & Lock Invariants:
  • Verify lock acquisition order. Ensure code uses utils::Mutex / utils::Condition with LockGuard const as mandated by skills/cpp_static_thread_safety.

Pillar 2: Arithmetic, Narrowing & Numerical Boundaries

• Integer Truncation & Narrowing Conversions:
  • Check all numeric assignments and casts between types of differing bit widths (e.g., uint32_t $\to$ uint16_t, size_t $\to$ uint32_t).
  • Determine what happens when values equal or exceed the narrower type's capacity (e.g., exact multiples of 65,536 truncating to 0 and inducing infinite loops).
• Boundary Conditions:
  • Evaluate behavior at count == 0, count == 1, $2^{16}$, $2^{32}$, INT_MAX, and overflow/underflow points.