Review systems

Review Systems -- Systems-Level Code Review

This extends the standard code-review skill (.claude/skills/code-review/SKILL.md) with systems-level checks. Run the standard review first, then apply these checks on top.

Trigger

• /review-systems command
• After standard /review when the code involves I/O, concurrency, or will run in production
• Coach judgment: any code that interacts with the OS (files, sockets, processes, threads)

Prerequisite

Run the standard code review first:

1. Understanding check (walk through the code)
2. TDD compliance
3. Correctness
4. Standards compliance (type hints, error handling, naming, docstrings)

Then apply these systems-level checks.

Systems Review Checklist

1. Resource Cleanup

Every OS resource opened must be closed. No exceptions.

Resource	Correct Pattern	Failure Mode
Files	with open(...) as f:	fd leak, hits ulimit, OSError: Too many open files
DB connections	Context manager or connection pool .close()	Connection pool exhaustion, DB max_connections hit
Sockets	with socket.socket() as s: or explicit .close() in finally	fd leak, TIME_WAIT accumulation, port exhaustion
Subprocesses	with subprocess.Popen() as p: or explicit .wait()	Zombie processes, fd leak (stdin/stdout/stderr pipes)
Thread pools	with ThreadPoolExecutor() as pool:	Threads never join, process hangs on exit
Temp files	with tempfile.NamedTemporaryFile() as f:	Disk fills up, /tmp exhaustion
Locks	with lock:	Deadlock if exception before release()

Review question: "What happens if an exception fires between the resource open and the resource close? Does the cleanup still run?"

2. Memory Footprint

What does this code cost in memory?

Check for:

• list where generator would suffice (materializing 10M rows vs streaming them)
• String concatenation in a loop (+= creates a new string each time, O(n^2) total)
• Accumulating results without bound (appending to a list that never gets cleared)
• Large default arguments (def f(data=[]) -- mutable default, lives forever on the function object)
• Module-level containers that grow (caches without eviction, registries without cleanup)

Review question: "If this runs on a 10M-row dataset, what's the peak memory? Does it need to hold everything in memory at once?"

pymalloc note: Python's memory allocator (pymalloc) uses arenas of 256KB. An arena is only released to the OS when ALL blocks in it are freed. One surviving object pins the entire arena. This is why peak RSS rarely decreases even after del. For long-running servers, this means: avoid creating and destroying millions of small objects in a cycle -- the arenas pin.

3. File Descriptor Hygiene

Check for:

• Every open() has a matching close() (or is in a with block)
• Database connections returned to the pool (not held open across request boundaries)
• Sockets closed in error paths (not just happy path)
• Subprocess pipes closed (stdout, stderr, stdin)
• No fd inheritance to child processes (close_fds=True is default in Python 3, but verify for subprocess)

Review question: "If this endpoint handles 1000 requests/second, how many file descriptors are open at steady state?"

4. GIL Analysis

Python's Global Interpreter Lock means: one thread executes Python bytecode at a time. Threading gives I/O concurrency, not CPU parallelism.

Check for:

• CPU-bound work in threads (will not parallelize -- use ProcessPoolExecutor)
• I/O-bound work in processes (unnecessary overhead -- use threads or async)
• GIL-releasing C extensions: numpy, torch operations, hashlib, zlib -- these DO run in parallel in threads
• time.sleep() releases the GIL (good for testing concurrency, misleading for benchmarking CPU work)
• Shared mutable state between threads (GIL prevents data races at bytecode level, NOT at operation level -- dict[key] += 1 is not atomic)

Review question: "Is this I/O-bound or CPU-bound? Does the concurrency model match?"

5. Scale Projection (10x / 100x / 1000x)

Walk through these scenarios:

Factor	Question
10x data	"If the input file is 10x larger, does the code still work? Does it OOM?"
100x concurrency	"If 100 users hit this endpoint simultaneously, what breaks first? Connections? Memory? CPU?"
1000x records	"If the database has 1M rows instead of 1K, which queries become slow? Is there an index?"
Slow network	"If the external API takes 30 seconds instead of 300ms, what happens? Timeout? Thread pool exhaustion?"
Disk full	"If the disk is full, does the write fail gracefully or corrupt state?"
Dependency down	"If the database/cache/API is unreachable, does the code hang, crash, or degrade gracefully?"

6. Failure Mode Analysis

For each external dependency, check:

• Is there a timeout? (No timeout = hang forever. Every network call needs a timeout.)
• Is there a retry with backoff? (Immediate retry = thundering herd.)
• Is there a circuit breaker or fallback? (For non-critical dependencies.)
• What's the error message? (A bare except: pass swallows production fires.)
• Is the failure logged? (Silent failures are the worst failures.)

Review question: "What happens at 3 AM when the database goes down? Does this code tell you what happened, or do you have to guess?"

Output Format

After the standard review feedback, add:

## Systems Review

**Resource cleanup:** [PASS/ISSUE: description]
**Memory footprint:** [estimated for typical input / concern at scale]
**FD hygiene:** [PASS/ISSUE: description]
**GIL analysis:** [I/O-bound: threads OK / CPU-bound: needs processes / Mixed: needs refactor]
**Scale projection:** [what breaks first at 10x/100x]
**Failure modes:** [what happens when dependencies fail]

**Systems challenge question:** [one question that tests production thinking]

Rules

• This review layer is additive. Never skip the standard code review.
• Be specific. "Memory might be an issue" is not useful. "This list comprehension materializes 10M rows (~800MB) when a generator would stream them" is useful.
• Connect to real incidents from .claude/rules/systems-thinking.md when a failure mode matches.
• The student must answer the systems challenge question. It is not rhetorical.
• If the student's code passes all systems checks cleanly, say so. Don't invent problems. Honest review means honest praise too.