code-review

SeekDB Code Review

Review Goal

Act as a senior seekdb maintainer. Find concrete defects that can affect users, operators, data correctness, availability, security, or material performance. Prefer no comment over an uncertain style preference or speculative cleanup.

Write review comments in English.

Establish the Contract

1. Read the pull request title, description, linked issue, changed tests, and available CI results.
2. State the behavior the change promises before judging its implementation.
3. Inspect enough surrounding code to understand the existing invariant. Do not review the diff in isolation.
4. Trace the changed invariant through relevant callers, callees, sibling implementations, and unchanged paths. Follow all representations of the affected value, state, or operation across component boundaries.
5. When behavior is unclear, use a concrete input, state, and execution sequence to prove the defect. If proof is incomplete but the potential impact is serious, state exactly what evidence or focused test is missing.

Required Checks

• Error propagation: Check ret, OB_FAIL, and OB_SUCC flows for lost or overwritten errors, work continuing after failure, incorrect success returns, partial side effects, and missing cleanup. Recognize established seekdb error-handling idioms and flag only behavior-changing mistakes.
• Memory and resources: Verify allocator ownership and lifetime, arena-backed pointer escape, object destruction, and release of memory, file descriptors, sockets, threads, futures, tasks, and timers on success and failure paths.
• Concurrency and lifecycle: Check synchronization, lock ordering, atomic state, task cancellation, retries, startup, shutdown, and callbacks or work that can continue after ownership or state changes.
• Current-version state consistency: Follow tablet/LS state, log replay, transaction state, schema state, and cache state through normal and partial failure paths. Verify related in-memory representations cannot diverge.
• SQL semantics: When parser, resolver, rewrite, optimizer, or executor code changes, check equivalent syntax and expression forms, aliases, NULL and boundary behavior, and every relevant stage that carries the same semantic.
• Rust and networking: Check transport errors, protocol handling, resource cleanup, and platform-specific behavior for supported Linux and Windows paths. Require focused Rust tests for changed behavior when practical.
• Security: Apply the dedicated security review below to product code, workflows, build scripts, tests, documentation, and repository instructions.
• Performance: Report material regressions in hot paths, such as repeated allocation, avoidable copying, quadratic work, excessive locking, blocking I/O, or expensive work added to high-frequency operations. Do not report unmeasured micro-optimizations.
• Tests and evidence: Require the smallest focused unit test, mysqltest, Rust test, benchmark, or measurement that would fail if the changed invariant were broken. Do not demand broad tests that cannot prove the behavior.
• Build and CI: Review build or workflow changes only for correctness, security, portability, or reliability problems, not stylistic preferences.