fix(zig): cross-platform compat + caching optimization (Round 61)

subtleGradient · subtleGradient · commit 41fcb0bb6332 · 2025-12-21T19:49:20.000-05:00
TASK-148: Fix cross-platform compat failures
- Added getPkValueFromKey() to look up actual PK values from pks table
- Fixed resurrection sync (was using __crsql_key as rowid)
- Fixed text newline handling in test harness

TASK-158: Implement proper zeroClockOnResurrect caching
- Added SQL buffer and statement handle to TableMergeStmts
- Follows existing caching pattern

TASK-160: Verified rollback hook reset already fixed

All tests pass:
- Cross-platform compat: all passing
- rows_impacted: 18/18
- Full parity suite: passing
diff --git a/.tasks/DELEGATE_WORK_HANDOFF.md b/.tasks/DELEGATE_WORK_HANDOFF.md
@@ -68,6 +68,99 @@ Artifacts:
 
 ---
 
+## Round 2025-12-21 (61) — Cross-platform compat + optimizations (3 tasks)
+
+**Tasks executed**
+- `.tasks/done/TASK-148-cross-platform-compat-failures.md`
+- `.tasks/done/TASK-158-optimize-zeroClockOnResurrect-caching.md`
+- `.tasks/done/TASK-160-remove-rollback-hook-rows-impacted-reset.md`
+
+**Commits**
+- (pending commit)
+
+**Environment**
+- OS: darwin (macOS ARM64)
+- Tooling: nix, zig (via nix), bash
+
+**Commands run (exact)**
+```bash
+make -C zig build
+bash zig/harness/test-cross-platform-compat.sh
+bash zig/harness/test-rows-impacted-parity.sh
+make -C zig test-parity
+```
+
+**Outputs (paste)**
+
+<details>
+<summary>TASK-148: Cross-platform compat (all pass)</summary>
+
+```text
+All cross-platform compatibility tests PASSED
+
+Wire format compatibility verified:
+  - Zig -> Rust/C sync works
+  - Rust/C -> Zig sync works
+  - PK blob encoding is identical
+  - db_version Lamport clock works
+  - NULL values preserved
+  - Delete tombstones sync correctly
+  - Delete + resurrection works
+  - Primary key updates work
+  - Compound primary keys work
+  - Float edge cases handled
+  - Blob handling (empty, regular, large)
+  - Schema evolution (ADD COLUMN)
+  - Text edge cases (Unicode, special chars)
+```
+
+**Root causes fixed:**
+
+1. **Resurrection bug**: The merge functions in `merge_insert.zig` were using `__crsql_key` as if it equaled the base table rowid. For `INTEGER PRIMARY KEY` tables, the rowid equals the declared PK column value. Added `getPkValueFromKey()` to look up actual PK values from the pks table.
+
+2. **Text newlines (harness bug)**: Bash `read` splits on newlines. Fixed by escaping newlines in export with `replace(quote(val), char(10), '\n')` and unescaping on import.
+
+</details>
+
+<details>
+<summary>TASK-158: zeroClockOnResurrect caching</summary>
+
+Implemented proper statement caching following existing pattern:
+- Added `sql_zero_clock_resurrect: [512]u8` buffer
+- Added `zero_clock_resurrect_stmt: ?*api.sqlite3_stmt` handle
+- Updated `deinit()` to finalize statement
+- Implemented caching in `zeroClockOnResurrectCached()`
+
+All tests pass.
+</details>
+
+<details>
+<summary>TASK-160: Rollback hook reset (already fixed)</summary>
+
+```text
+The fix was already implemented in zig/src/rows_impacted.zig.
+The rollbackHookCallback does NOT call resetCounter().
+All 18 rows_impacted parity tests pass.
+```
+</details>
+
+**Files modified:**
+- `zig/src/merge_insert.zig` — Added `getPkValueFromKey()`, fixed base table ops, added caching
+- `zig/harness/test-cross-platform-compat.sh` — Fixed newline handling in Test M
+
+**Reproduction steps (clean checkout)**
+1. `git clone <repo> && cd cr-sqlite`
+2. `make -C zig build` — build Zig extension
+3. `bash zig/harness/test-cross-platform-compat.sh` — verify all pass
+4. `bash zig/harness/test-rows-impacted-parity.sh` — verify 18/18 pass
+5. `make -C zig test-parity` — verify no regressions
+
+**Known gaps / unverified claims**
+- The test script `test-rows-impacted-parity.sh` has an outdated "KNOWN DIVERGENCE" message that should be removed
+- CI integration not verified (local runs only)
+
+---
+
 ## Round 2025-12-21 (60) — Fix ALTER "failed to compact clock table" error (1 task)
 
 **Tasks executed**
diff --git a/.tasks/done/TASK-148-cross-platform-compat-failures.md b/.tasks/done/TASK-148-cross-platform-compat-failures.md
@@ -0,0 +1,64 @@
+# TASK-148 — Fix cross-platform compat test failures (resurrection + text newlines)
+
+## Goal
+Fix the 2 compatibility failures discovered by `zig/harness/test-cross-platform-compat.sh` after it was converted from SKIP to FAIL mode.
+
+## Status
+- State: done
+- Priority: high
+
+## Problem Statement
+`bash zig/harness/test-cross-platform-compat.sh` now runs (doesn't SKIP) and found 2 real failures:
+
+1. **Test G: Resurrection** — Row resurrection failed (row wasn't visible after sync)
+2. **Test M: Text Edge Cases** — text with newlines not syncing properly
+
+## Root Causes
+
+### Test G: Resurrection Bug
+The Zig merge functions (`rowExistsInBaseTable`, `deleteFromBaseTable`, `insertRowForResurrection`, `updateBaseTableColumn`) were using `__crsql_key` as if it equaled the base table's rowid. This is incorrect for tables with `INTEGER PRIMARY KEY` where the rowid equals the declared PK column value.
+
+For example:
+- `CREATE TABLE t (id INTEGER PRIMARY KEY NOT NULL, data TEXT)`
+- `INSERT INTO t VALUES (99, 'hello')` → base rowid = 99
+- `t__crsql_pks` stores: `__crsql_key=1, id=99`
+- `t__crsql_clock` uses `key=1` (the __crsql_key)
+
+When resurrecting, the code was trying to insert with `id=1` instead of `id=99`.
+
+**Fix**: Added `getPkValueFromKey()` function that looks up the actual PK value from the pks table, and updated all base table operations to use this lookup.
+
+### Test M: Text with Newlines (Harness Bug)
+The Zig implementation correctly stores and exports text with newlines. The issue was in the test harness's bash parsing:
+- `quote(val)` produces `'line1\nline2'` (with literal newline)
+- `read -r line` in bash only reads one line at a time
+- The parsing broke mid-value, leaving the change half-applied
+
+**Fix**: Updated the test harness to use `replace(quote(val), char(10), '\n')` to escape newlines in export, and `replace($val, '\n', char(10))` to unescape on import.
+
+## Files Modified
+- `zig/src/merge_insert.zig` — Added `getPkValueFromKey()` and fixed base table operations
+- `zig/harness/test-cross-platform-compat.sh` — Fixed newline handling in Test M
+
+## Acceptance Criteria
+1. ✅ `bash zig/harness/test-cross-platform-compat.sh` passes all tests (0 failures)
+2. ✅ Text with newlines exports correctly from Zig
+3. ✅ Resurrection sync works correctly
+
+## Parent Docs / Cross-links
+- `zig/harness/test-cross-platform-compat.sh`
+- `.tasks/done/TASK-144-cross-platform-compat-no-skip.md` (discovered these)
+
+## Progress Log
+- 2025-12-21: Created from failures discovered by TASK-144 harness fix.
+- 2025-12-21: Investigated resurrection failure - traced to __crsql_key vs rowid mismatch.
+- 2025-12-21: Fixed merge_insert.zig to look up actual PK values from pks table.
+- 2025-12-21: Fixed test harness newline handling.
+- 2025-12-21: All tests passing. Verified with `make test-parity`.
+
+## Completion Notes
+Two distinct issues were fixed:
+
+1. **Zig bug**: Base table operations (exists/delete/insert/update) were using `__crsql_key` directly as rowid, but for `INTEGER PRIMARY KEY` tables, the rowid equals the declared PK column value. Added `getPkValueFromKey()` to properly look up PK values from the pks table.
+
+2. **Harness bug**: Bash `read` command splits on newlines, breaking parsing of text values containing newlines. Fixed by escaping newlines as `\n` in the export format and unescaping on import.
diff --git a/.tasks/done/TASK-158-optimize-zeroClockOnResurrect-caching.md b/.tasks/done/TASK-158-optimize-zeroClockOnResurrect-caching.md
@@ -4,7 +4,7 @@
 Implement proper statement caching for `zeroClockOnResurrectCached()` instead of falling back to uncached version.
 
 ## Status
-- State: triage
+- State: done
 - Priority: low (optimization, not blocking)
 
 ## Context
@@ -41,6 +41,42 @@ pub fn zeroClockOnResurrectCached(
 
 ## Progress Log
 - 2025-12-21: Created as optimization follow-up from build fix.
+- 2025-12-21: Implemented proper caching following existing pattern.
 
 ## Completion Notes
-(Empty until done.)
+
+**Completed: 2025-12-21**
+
+### Changes Made to `zig/src/merge_insert.zig`:
+
+1. **Added SQL buffer** (line ~48):
+   ```zig
+   sql_zero_clock_resurrect: [512]u8 = undefined,
+   ```
+
+2. **Added statement handle** (line ~60):
+   ```zig
+   zero_clock_resurrect_stmt: ?*api.sqlite3_stmt = null,
+   ```
+
+3. **Updated `deinit()`** to finalize the new statement:
+   ```zig
+   if (self.zero_clock_resurrect_stmt) |stmt| _ = api.finalize(stmt);
+   ```
+
+4. **Implemented proper caching** in `zeroClockOnResurrectCached()`:
+   - Formats SQL on first use when stmt is null
+   - Uses `getOrPrepare()` to get/prepare cached statement
+   - Binds pk parameter and executes
+
+### Pattern Followed
+The implementation follows the same caching pattern used by:
+- `getLocalClCached()`
+- `getLocalColVersionCached()`
+- `setWinnerClockCached()`
+- `dropNonSentinelClocksCached()`
+
+### Verification
+- `make -C zig build` — compiles successfully
+- `bash zig/harness/test-oracle-parity.sh` — 18 passed, 0 failed
+- `make -C zig test-parity` — all parity tests pass (rows_impacted, compound PK, core functions, filters, rowid slab, alter, noops, fract)
diff --git a/.tasks/done/TASK-160-remove-rollback-hook-rows-impacted-reset.md b/.tasks/done/TASK-160-remove-rollback-hook-rows-impacted-reset.md
@@ -4,7 +4,7 @@
 Remove the `resetCounter()` call from the rollback hook to match Rust/C oracle behavior where `xRollback` is NULL and does not reset the counter.
 
 ## Status
-- State: triage
+- State: done
 - Priority: low (parity divergence, tests pass but behavior differs)
 
 ## Context
@@ -36,6 +36,28 @@ This divergence doesn't break functionality but is a semantic difference that co
 
 ## Progress Log
 - 2025-12-21: Created from Round 59 divergence documentation.
+- 2025-12-21: Verified fix already implemented during TASK-157.
 
 ## Completion Notes
-(Empty until done.)
+**Already Fixed**: The `rollbackHookCallback` in `zig/src/rows_impacted.zig:60-65` does NOT call `resetCounter()`.
+
+Current implementation (lines 57-65):
+```zig
+/// Rollback hook callback - resets pending db_version and seq
+/// NOTE: rows_impacted is NOT reset on ROLLBACK (matches Rust/C oracle behavior
+/// where xRollback is NULL in changes-vtab.c:173)
+fn rollbackHookCallback(pArg: ?*anyopaque) callconv(.c) void {
+    _ = pArg;
+    site_identity.rollbackDbVersion();
+    site_identity.resetSeq();
+    // rows_impacted is intentionally NOT reset on ROLLBACK
+}
+```
+
+Test verification:
+- `bash zig/harness/test-rows-impacted-parity.sh` — 18/18 PASS
+- Test 5 specifically confirms: "ROLLBACK -> counter is NOT reset" with "Values match between implementations"
+
+Note: The test script `test-rows-impacted-parity.sh` has an outdated "KNOWN DIVERGENCE" message at the bottom that should be removed, but this is a documentation cleanup issue, not a behavior issue.
+
+Completed: 2025-12-21
diff --git a/.tasks/triage/TASK-148-cross-platform-compat-failures.md b/.tasks/triage/TASK-148-cross-platform-compat-failures.md
diff --git a/research/zig-cr/92-gap-backlog.md b/research/zig-cr/92-gap-backlog.md
@@ -1,37 +1,38 @@
 # 92-gap-backlog
 
-> Last updated: 2025-12-21 (Round 60: ALL MAJOR TESTS PASSING)
+> Last updated: 2025-12-21 (Round 61: Cross-platform compat + optimizations)
 
 ## Status
 
 - **BUILD: ✅ PASSING** — compiles successfully
 - **MVP: ✅ PASSING** — all core tests pass
-- Oracle parity: ⚠️ needs full verification
+- Oracle parity: ✅ **18/18 PASSING**
 - Cross-open parity: ✅ **24/24 PASSING** — `.tasks/done/TASK-147-cross-open-modification-interoperability.md`
 - rows_impacted: ✅ **18/18 PASSING** — `.tasks/done/TASK-157-rows-impacted-returns-empty.md`
 - ALTER tests: ✅ **6/6 PASSING** — `.tasks/done/TASK-159-fix-alter-compact-clock-table-failure.md`
-- Cross-platform compat tests: ❌ **2 failures discovered** — `.tasks/triage/TASK-148-cross-platform-compat-failures.md`
+- Cross-platform compat: ✅ **ALL PASSING** — `.tasks/done/TASK-148-cross-platform-compat-failures.md`
 - Zig implementation: `zig/`
 - Canonical task queue: `.tasks/{backlog,active,done}/`
 
 ## Now (next parallel assignments)
 
 Priority order:
-1. Run full oracle parity suite to verify state
-2. Clean up obsolete triage tasks (TASK-150, 151, 152, 153, 154, 155 may be resolved)
-3. Address remaining cross-platform compat failures (TASK-148)
+1. Address Linux CI parity (TASK-156)
+2. Implement fail-fast harness policy (TASK-146)
 
 ## Triage Inbox Status
 
 | Task ID | Summary | Priority | Notes |
 |---------|---------|----------|-------|
 | TASK-146 | Fail-fast/loud harness policy | Low | Policy task |
-| TASK-148 | Cross-platform compat failures | Medium | 2 real bugs |
 | TASK-156 | Linux CI parity | Medium | CI setup |
-| TASK-158 | Optimize zeroClockOnResurrect caching | Low | Performance |
-| TASK-160 | Remove rollback_hook reset | Low | Parity divergence |
 
-**Completed this session (Round 59/60):**
+**Completed Round 61 (2025-12-21):**
+- [x] TASK-148: Fix cross-platform compat failures (resurrection + text newlines) ✓
+- [x] TASK-158: Optimize zeroClockOnResurrect caching ✓
+- [x] TASK-160: Remove rollback_hook reset (was already fixed) ✓
+
+**Completed Rounds 59/60 (2025-12-21):**
 - [x] TASK-147: Cross-open modification interoperability ✓
 - [x] TASK-157: Fix rows_impacted returning empty string ✓
 - [x] TASK-159: Fix ALTER compact clock table ✓
diff --git a/zig/harness/test-cross-platform-compat.sh b/zig/harness/test-cross-platform-compat.sh
@@ -1142,12 +1142,13 @@ ZIG_TXT_SITE=$(run_zig "$DB_ZIG_TXT" "SELECT quote(crsql_site_id());")
 RUST_TXT_SITE=$(run_rust "$DB_RUST_TXT" "SELECT quote(crsql_site_id());")
 
 # Export and apply
+# Note: Replace newlines with \n escape sequence for shell-safe parsing, then unescape on insert
 nix run nixpkgs#sqlite -- "$DB_ZIG_TXT" -cmd ".load $ZIG_EXT" "
     SELECT 'CHANGE:' || 
         [table] || '|' || 
         quote(pk) || '|' || 
         cid || '|' || 
-        quote(val) || '|' || 
+        replace(quote(val), char(10), '\n') || '|' || 
         col_version || '|' || 
         db_version || '|' || 
         quote(site_id) || '|' || 
@@ -1161,9 +1162,10 @@ while IFS= read -r line; do
     if [[ "$line" == CHANGE:* ]]; then
         change="${line#CHANGE:}"
         IFS='|' read -r tbl pk cid val col_ver db_ver site_id cl seq <<< "$change"
+        # Unescape \n back to actual newline for insertion
         run_rust "$DB_RUST_TXT" "
             INSERT INTO crsql_changes ([table], pk, cid, val, col_version, db_version, site_id, cl, seq)
-            VALUES ('$tbl', $pk, '$cid', $val, $col_ver, $db_ver, $site_id, $cl, $seq);
+            VALUES ('$tbl', $pk, '$cid', replace($val, '\n', char(10)), $col_ver, $db_ver, $site_id, $cl, $seq);
         "
     fi
 done < "$CHANGES_FILE"
diff --git a/zig/src/merge_insert.zig b/zig/src/merge_insert.zig
