Skip to content

v0.3.9 — Code Review Remediation

Choose a tag to compare

@timsehn timsehn released this 08 Apr 14:22

Code Review Remediation

This release addresses every issue raised in Aaron's code review. 45 commits, 72 files changed, net -3,500 lines after deduplication and comment cleanup.

Correctness Fixes

  • Durability bug fixed. The commit protocol no longer writes to the manifest at offset 0 before fsync. The WAL root record at EOF is the single durability point.
  • Error propagation fixed. csDeserializeRefs errors are now propagated at all call sites. A database with commits but missing refs returns SQLITE_CORRUPT instead of silently losing all branches.
  • Concurrent commit detection. dolt_commit acquires an exclusive file lock, refreshes from disk, and checks the branch tip before writing. Two connections committing to the same branch get a clear error instead of silent data loss.
  • All commit graph mutations serialized. dolt_commit, dolt_reset, dolt_branch, dolt_tag, dolt_merge, push, pull — all acquire the graph lock via chunkStoreLockAndRefresh. Working-tree DML (INSERT/UPDATE/DELETE) requires no lock.
  • Dead rootHash field removed. The DoltliteCommit.rootHash field was always empty in V2 but was being copied into p->root, zeroing the working tree on branch reopen. Fixed and regression-tested.

Code Quality

  • Comment rewrite. Stripped all ~3,300 lines of Claude-generated comments, added back ~170 lines that score 8/10+ on necessity. Binary format docs, invariant warnings, integration constraints. Nothing that restates the code.
  • Deduplication. struct TableEntry consolidated from 10 files to 1 header. 15+ duplicated extern declarations eliminated. 5 copy-pasted varint readers replaced with shared dlReadVarint. Merge iteration (4 near-identical functions) replaced with parameterized mergeScan. Diff merge-walk and value comparison deduplicated.
  • Dead code removed. btreeDeleteDeferred, V1 compat in GC, csJournalPath, unused DiffRow fields, ProllyChunker.nTotalItems, fallback generic schemas in virtual tables.
  • C89 enforced. 69 mixed-declaration violations fixed. -Wdeclaration-after-statement added to compiler flags.
  • Layering linter. test/lint_layers.sh runs during make and blocks the build on: prolly layer including doltlite headers, prolly layer using sqlite3_prepare_v2, sizeof(ChunkStore) pointer arithmetic, inline extern declarations, duplicate struct definitions.

Architecture

  • dolt_log and dolt_history BFS all parents. Merge commits now show both branches' history in topological order, matching git log behavior.
  • Streaming merge handles tall trees. Recursive streamingMergeNode replaces the O(N) cursor-walk fallback for height >= 2 trees, preserving O(M*L) complexity at any depth.
  • Hardcoded GC offsets replaced with named constants (CAT_HEADER_SIZE, CAT_NUM_TABLES_OFF, etc.) shared between serializer and GC.
  • README corrected. Fixed inaccurate claims about BtShared sharing and connection isolation. Documents actual concurrency model.

Test Coverage

Over 1,000 new test assertions:

Suite Assertions What it proves
crash_recovery_test 116 Atomicity, durability, WAL recovery across 30 crash scenarios
invariant_test 729 Internal consistency after every operation type
corruption_test 60 Corruption detection (zeroed refs, bad chunks, invalid metadata)
concurrent_commit_test 25 No silent commit loss (Aaron's exact scenario)
concurrent_stress_test 116 Conflict detection, cross-branch commits, read-during-write
review_regression_test 30 Guards for every specific fix from the review

Plus all 1,344 existing shell tests continue to pass.