Sph 2026 03 19.02

MEMORY_LEAK_REVIEW_FINDINGS.md

@@ -0,0 +1,72 @@
+# DecentDB Memory Leak Hunt Review Findings
+
+### 1. Overall Verdict
+**Improved but still risky.**
+
+The PR successfully addresses the most egregious success-path memory leaks (specifically breaking reference cycles under ARC), but it introduces a critical thread-safety vulnerability (Time-of-Check to Time-of-Use use-after-free) and completely misses error-path cleanup symmetry. Additionally, the leak detection framework has a major silent-pass gap on non-Linux platforms.
+
+### 2. What the PR did well
+- **Identified and broke the core ARC cycles:** Successfully added `clearPageOverlay()` in `closeDb` to sever the `Db -> Pager -> closure -> Db` cycle.
+- **Fixed `Wal -> cachedWriter` cycle:** Explicitly decoupled the WAL cached writer during close.
+- **C API safety:** Addressed ephemeral pointer risks in the bindings by introducing `textScratch` to explicitly tie null-terminated string lifecycles to the `StmtHandle`.
+- **Thorough regression suite design:** The `runLeakAmplification` structure is well-conceived for catching leaks across various internal components (Temp tables, savepoints, WAL sharing).
+
+### 3. Findings
+
+#### Finding 1: Critical Data Race and Use-After-Free in WAL Reader Flags
+- **Severity:** Critical
+- **Category:** ARC retention risk / Data Race
+- **Affected files:** `src/wal/wal.nim` (`cleanupReaderFlagsForClose`, `isAborted`, `endRead`)
+- **Why it is a problem:** To prevent a leak of `ptr Atomic[bool]`, the PR added a manual `dealloc` loop in `cleanupReaderFlagsForClose` which is called when a WAL is closed. To prevent lock-free readers from crashing, it added a `freedReaderFlags` check in `isAborted`. However, this is a Time-of-Check to Time-of-Use (TOCTOU) race. A reader thread might evaluate `wasFreed == false`, get preempted, and then the WAL is closed (freeing the memory). When the reader resumes, it dereferences `txn.aborted[].load()`, causing a use-after-free segfault.
+- **Recommendation:** Do not use `alloc0`/`dealloc` for atomic flags. Change `ReadTxn.aborted` to a Nim `ref object` containing the `Atomic[bool]`. ARC's thread-safe reference counting will naturally ensure the memory is kept alive exactly as long as either the WAL or the active `ReadTxn` holds a reference, completely eliminating the need for manual tracking, the `freedReaderFlags` registry, and the data race.
+
+#### Finding 2: `openDb` Failure Paths Leak `Db` and `Pager` (ARC Cycle)
+- **Severity:** High
+- **Category:** Error-path cleanup
+- **Affected files:** `src/engine.nim` (`openDb`)
+- **Why it is a problem:** The `Db -> Pager -> closure -> Db` reference cycle is established *during* `openDb`. If an error occurs after the overlay is set (e.g., during `initCatalog(pager)` or reading the root page), `openDb` calls `closePager(pager)` and returns an error. However, `closePager` does NOT clear the overlay closure. The `Db` and `Pager` instances are dropped by `openDb` but will be permanently retained by ARC due to the unbroken cycle.
+- **Recommendation:** In `openDb`, explicitly call `dbRef.pager.clearPageOverlay()` in all error paths that occur after the overlay closure is configured, before returning the error.
+
+#### Finding 3: Non-Linux Leak Tests Silently Pass (Asserting Against Zero)
+- **Severity:** High
+- **Category:** Test Gap
+- **Affected files:** `tests/nim/lifecycle_test_support.nim`, `tests/nim/test_engine_lifecycle_leaks.nim`
+- **Why it is a problem:** The tests compile with `-d:useMalloc`. Under this flag with `--mm:arc`, Nim's internal allocator tracking is bypassed, and `getOccupiedMem()` permanently returns `0`. The tests rely on `deltaOccupied(samples) < 2_000_000`, which evaluates to `0 < 2_000_000` (always true). Because macOS and Windows do not have RSS assertions in these tests, the leak tests provide zero actual regression coverage on those platforms.
+- **Recommendation:** Implement OS-specific memory polling for macOS (`task_info`) and Windows (`GetProcessMemoryInfo`), or clearly mark the test suite as Linux-only. Do not rely on `getOccupiedMem()` when compiling with `-d:useMalloc`.
+
+#### Finding 4: Partial Cleanup on `closeDb` Error
+- **Severity:** Medium
+- **Category:** Error-path cleanup
+- **Affected files:** `src/engine.nim` (`closeDb`)
+- **Why it is a problem:** If `closeWalHandle` or `closePager` returns an error (e.g., due to an underlying VFS I/O error), `closeDb` aborts immediately. Crucially, it skips clearing the `sqlCache`, `tempTables`, `tempViews`, and `savepointStack`, and it fails to set `db.isOpen = false`. The database is left in an inconsistent half-closed state, retaining memory indefinitely.
+- **Recommendation:** Restructure `closeDb` to always clear its caches and toggle `isOpen = false`, even if the underlying `vfs.close` operations fail. 
+
+#### Finding 5: Extremely Inefficient Shared WAL Initialization
+- **Severity:** Low
+- **Category:** Lifecycle/Performance
+- **Affected files:** `src/engine.nim` (`acquireSharedWal`)
+- **Why it is a problem:** When opening a database, `acquireSharedWal` creates a new WAL instance and calls `recover(wal)` (which scans the entire WAL file) *before* checking `walRegistry` to see if a shared WAL already exists. If 50 concurrent connections open the database, all 50 will perform full I/O recovery, and 49 of them will immediately discard the result. 
+- **Recommendation:** Check the `walRegistryLock` first to see if a WAL exists for the path. Only create and recover a new WAL if the registry lookup returns a miss.
+
+### 4. Suspected Remaining Leak Paths
+- **Concurrent Statement Execution during Close:** Because `closeDb` clears `wal.readers` while read transactions might still be in flight, the C API bindings (`StmtHandle`) may attempt to finalize against a detached or partially-freed WAL state.
+
+### 5. Test Coverage Gaps
+- **Error-path `closeDb` testing:** No tests verify that memory is correctly released if `vfs.close` is mocked to fail.
+- **Error-path `openDb` testing:** Tests check for a corrupted WAL header, but not for failures during catalog initialization (where the ARC cycle is already formed).
+- **Concurrency testing:** No tests execute concurrent readers alongside a thread calling `closeDb`, which would have easily caught the TOCTOU crash.
+
+### 6. Suggested Follow-up Tests
+- `test "openDb failure during catalog init breaks ARC cycle"`
+- `test "closeDb clears cache even if vfs.close fails"`
+- `test "concurrent reader abort does not segfault when WAL closes"`
+- Add RSS-based assertions for macOS and Windows to replace the dummy `getOccupiedMem()` checks.
+
+### 7. Suggested Code Fixes
+1. Replace `ptr Atomic[bool]` with `ref Atomic[bool]` in `ReadTxn` and remove `cleanupReaderFlagsForClose`.
+2. Add `dbRef.pager.clearPageOverlay()` to the `not catalogRes.ok` block in `openDb`.
+3. Move `sqlCache.clear()` and related teardowns to the top of `closeDb` (or a `defer` block) so they execute unconditionally.
+4. Move `walRegistry` lookup in `acquireSharedWal` to happen *before* calling `newWal` and `recover`.
+
+### 8. Confidence Level
+**Low-Medium**. While the primary reported leaks are resolved for the "happy path," the PR introduces a severe thread-safety regression and leaves significant error-path memory leaks unresolved. The test suite's failure to actually measure memory on non-Linux platforms gives false confidence.

bindings/dart/dart/lib/src/database.dart

@@ -104,7 +104,7 @@ class Database {
     return open(':memory:', libraryPath: libraryPath, bindings: bindings);
   }
 
-  /// The engine version string (e.g. "1.8.0").
+  /// The engine version string (e.g. "1.8.1").
   String get engineVersion {
     final ptr = _bindings.engineVersion();
     return ptr == nullptr ? 'unknown' : ptr.toDartString();

bindings/dart/dart/pubspec.yaml

@@ -1,6 +1,6 @@
 name: decentdb
 description: Dart FFI bindings for DecentDB – an embedded ACID database engine.
-version: 1.8.0
+version: 1.8.1
 repository: https://github.com/nicholasgasior/decentdb
 homepage: https://github.com/nicholasgasior/decentdb/tree/main/bindings/dart
 

bindings/dart/examples/console/pubspec.lock

@@ -7,7 +7,7 @@ packages:
       path: "../../dart"
       relative: true
     source: path
-    version: "1.8.0"
+    version: "1.8.1"
   ffi:
     dependency: transitive
     description:

bindings/dart/native/decentdb.h

@@ -14,7 +14,7 @@ extern "C" {
 // Returns the ABI version number. Callers should check at load time.
 int decentdb_abi_version(void);
 
-// Returns the engine version string (e.g. "1.8.0"). Static; do NOT free.
+// Returns the engine version string (e.g. "1.8.1"). Static; do NOT free.
 const char* decentdb_engine_version(void);
 
 // --------------------------------------------------------------------------

bindings/java/dbeaver-extension/META-INF/MANIFEST.MF

@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: DecentDB DBeaver Extension
 Bundle-SymbolicName: org.jkiss.dbeaver.ext.decentdb;singleton:=true
-Bundle-Version: 1.8.0
+Bundle-Version: 1.8.1
 Bundle-Activator: org.jkiss.dbeaver.ext.decentdb.DecentDBActivator
 Bundle-Vendor: DecentDB Contributors
 Require-Bundle: org.eclipse.core.runtime,
@@ -11,5 +11,5 @@ Require-Bundle: org.eclipse.core.runtime,
  org.jkiss.dbeaver.ext.generic
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Bundle-ClassPath: .,
- lib/decentdb-jdbc-1.8.0.jar
+ lib/decentdb-jdbc-1.8.1.jar
 Export-Package: org.jkiss.dbeaver.ext.decentdb.model

bindings/java/dbeaver-extension/build.gradle

@@ -3,7 +3,7 @@ plugins {
 }
 
 group = 'org.jkiss.dbeaver.ext'
-version = '1.8.0'
+version = '1.8.1'
 
 java {
     sourceCompatibility = JavaVersion.VERSION_21

bindings/java/driver/build.gradle

@@ -3,7 +3,7 @@ plugins {
 }
 
 group = 'com.decentdb'
-version = '1.8.0'
+version = '1.8.1'
 
 java {
     sourceCompatibility = JavaVersion.VERSION_17

bindings/java/driver/src/main/java/com/decentdb/jdbc/DecentDBDriver.java

@@ -27,7 +27,7 @@
 public final class DecentDBDriver implements Driver {
 
     public static final String URL_PREFIX = "jdbc:decentdb:";
-    public static final String DRIVER_VERSION = "1.8.0";
+    public static final String DRIVER_VERSION = "1.8.1";
     public static final int DRIVER_MAJOR_VERSION = 1;
     public static final int DRIVER_MINOR_VERSION = 8;
 

bindings/node/decentdb/package.json

@@ -1,6 +1,6 @@
 {
   "name": "decentdb-native",
-  "version": "1.8.0",
+  "version": "1.8.1",
   "private": true,
   "description": "DecentDB Node.js native addon (N-API) + thin JS wrapper",
   "main": "index.js",

bindings/node/knex-decentdb/package.json

@@ -1,6 +1,6 @@
 {
   "name": "knex-decentdb",
-  "version": "1.8.0",
+  "version": "1.8.1",
   "private": true,
   "description": "Knex client/dialect for DecentDB",
   "main": "index.js",

bindings/python/pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "decentdb"
-version = "1.8.0"
+version = "1.8.1"
 description = "Python DB-API 2.0 driver and SQLAlchemy dialect for DecentDB"
 readme = "README.md"
 authors = [