aman-bhandari 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-systemscommand- After standard
/reviewwhen 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:
- Understanding check (walk through the code)
- TDD compliance
- Correctness
- 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:
listwheregeneratorwould 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 matchingclose()(or is in awithblock) - 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=Trueis default in Python 3, but verify forsubprocess)
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] += 1is 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: passswallows 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.mdwhen 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.