Skip to content

Commit 561a6af

Browse files
subtleGradientsubtleGradient
committed
fix(zig): schema_alter pk→key + merge resolution parity (TASK-125, TASK-126)
TASK-125: Fixed schema_alter.zig pk→key column rename regression - Changed all clock table 'pk' references to 'key' - Added STRICT mode and _dbv_idx index to match as_crr.zig - test-alter.sh: 6/6 pass, test-noops.sh: 4/4 pass TASK-126: Fixed merge resolution parity with oracle - Root cause: setWinnerClock was storing site_id BLOBs in INTEGER column - Fix: Convert site_id blob to ordinal via site_identity.getOrCreateSiteOrdinal() - test-oracle-parity.sh: 18/18 pass (all divergences fixed) Zero oracle divergences remaining.
1 parent df10d23 commit 561a6af

File tree

6 files changed

+303
-15
lines changed

6 files changed

+303
-15
lines changed

‎.tasks/DELEGATE_WORK_HANDOFF.md‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,123 @@ Artifacts:
6868

6969
---
7070

71+
## Round 2025-12-20 (53) — Fix schema_alter pk→key + merge resolution (2 tasks)
72+
73+
**Tasks executed**
74+
- `.tasks/done/TASK-125-fix-schema-alter-pk-to-key-rename.md`
75+
- `.tasks/done/TASK-126-fix-merge-resolution-parity.md`
76+
77+
**Commits**
78+
- (pending commit)
79+
80+
**Environment**
81+
- OS: darwin (macOS ARM64)
82+
- Tooling: nix, zig (via nix), bash
83+
84+
**Commands run (exact)**
85+
```bash
86+
bash zig/harness/test-alter.sh
87+
bash zig/harness/test-noops.sh
88+
bash zig/harness/test-oracle-parity.sh
89+
make -C zig test-parity
90+
```
91+
92+
**Outputs (paste)**
93+
94+
<details>
95+
<summary>TASK-125: test-alter.sh (6/6 pass)</summary>
96+
97+
```text
98+
╔═══════════════════════════════════════════════════════════════════════╗
99+
║ TEST SUMMARY ║
100+
╠═══════════════════════════════════════════════════════════════════════╣
101+
║ PASSED: 6 ║
102+
║ FAILED: 0 ║
103+
║ SKIPPED: 0 ║
104+
╚═══════════════════════════════════════════════════════════════════════╝
105+
106+
✓ All implemented tests PASSED
107+
```
108+
109+
**Fix:** Updated `zig/src/schema_alter.zig`:
110+
- Changed all clock table `"pk"` references to `"key"`
111+
- Added STRICT mode to clock table creation
112+
- Added `_dbv_idx` index on `db_version`
113+
</details>
114+
115+
<details>
116+
<summary>TASK-125: test-noops.sh (4/4 pass)</summary>
117+
118+
```text
119+
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
120+
No-op Tests Summary: 4 passed, 0 failed
121+
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
122+
All noop tests passed!
123+
```
124+
</details>
125+
126+
<details>
127+
<summary>TASK-126: test-oracle-parity.sh (18/18 pass)</summary>
128+
129+
```text
130+
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
131+
Oracle Parity Test Summary
132+
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
133+
134+
Results: 18 passed, 0 failed, 0 skipped
135+
136+
All oracle parity tests PASSED
137+
138+
Wire format and behavioral parity verified:
139+
- pack_columns encoding matches
140+
- Clock table schema matches
141+
- Merge resolution matches
142+
- Site ID storage matches
143+
- Changes vtab format matches
144+
- db_version behavior matches
145+
```
146+
147+
**Fix:** Updated `zig/src/merge_insert.zig`:
148+
- Added import for `site_identity` module
149+
- Fixed `setWinnerClock()` to convert site_id blob → ordinal via `site_identity.getOrCreateSiteOrdinal()`
150+
- Fixed `setWinnerClockCached()` with same conversion
151+
152+
Root cause: The clock table's `site_id` column is INTEGER (for ordinals), but the merge functions were trying to store 16-byte BLOBs directly. This caused STRICT constraint violations that silently failed all remote merges.
153+
</details>
154+
155+
<details>
156+
<summary>Parity suite summary</summary>
157+
158+
```text
159+
Filter tests: 12 passed
160+
Rowid slab tests: 8 passed
161+
Alter tests: 6 passed
162+
Noop tests: 4 passed
163+
Fract tests: 8 passed
164+
rows_impacted tests: 9 passed (including ValueWin)
165+
```
166+
167+
No regressions detected.
168+
</details>
169+
170+
**Files modified:**
171+
- `zig/src/schema_alter.zig` — pk→key rename, STRICT mode, _dbv_idx index
172+
- `zig/src/merge_insert.zig` — site_id blob→ordinal conversion in setWinnerClock functions
173+
174+
**Reproduction steps (clean checkout)**
175+
1. `git clone <repo> && cd cr-sqlite`
176+
2. `bash zig/harness/test-alter.sh` — verify 6/6 pass
177+
3. `bash zig/harness/test-noops.sh` — verify 4/4 pass
178+
4. `bash zig/harness/test-oracle-parity.sh` — verify 18/18 pass
179+
5. `make -C zig test-parity` — verify no regressions
180+
181+
**Known gaps / unverified claims**
182+
- **ZERO oracle divergences remaining** — all 18 parity tests pass
183+
- No coverage captured
184+
- CI integration not verified this round (local runs only)
185+
186+
---
187+
71188
## Round 2025-12-20 (52) — Fix clock schema + site_id cross-open parity (2 tasks)
72189

73190
**Tasks executed**

‎.tasks/done/TASK-125-fix-schema-alter-pk-to-key-rename.md‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# TASK-125: Fix schema_alter.zig pk→key column rename for clock table
2+
3+
## Status
4+
- [x] Completed
5+
6+
## Priority
7+
high (blocks alter and merge tests)
8+
9+
## Assigned To
10+
(completed)
11+
12+
## Parent Docs / Cross-links
13+
- Caused by: `.tasks/done/TASK-123-fix-clock-table-schema-parity.md`
14+
- Test failures: `zig/harness/test-alter.sh` (Test 2b), `zig/harness/test-noops.sh` (Test 3)
15+
- Gap backlog: `research/zig-cr/92-gap-backlog.md`
16+
17+
## Description
18+
TASK-123 renamed the clock table column from `pk` to `key` in `as_crr.zig`, but missed updating `schema_alter.zig`. This causes SQL errors when:
19+
20+
1. **test-alter.sh Test 2b**: "table bar2__crsql_clock has no column named pk"
21+
2. **test-noops.sh Test 3**: "setWinnerClockCached failed" (SQL logic error)
22+
23+
The issue is that `schema_alter.zig` still references `"pk"` in clock table SQL at:
24+
- Line 264: CREATE TABLE clock definition
25+
- Line 270: PRIMARY KEY clause
26+
- Lines 468-473: INSERT INTO clock
27+
- Lines 578, 590: INSERT OR REPLACE INTO clock
28+
29+
## Files to Modify
30+
- `zig/src/schema_alter.zig` — update all clock table `"pk"` references to `"key"`
31+
32+
## Acceptance Criteria
33+
- [x] All `"pk"` references in clock table SQL changed to `"key"`
34+
- [x] Clock table creation includes `STRICT` and index (matching TASK-123)
35+
- [x] `bash zig/harness/test-alter.sh` — 6/6 pass
36+
- [x] `bash zig/harness/test-noops.sh` — 4/4 pass
37+
- [x] No regressions in `make -C zig test-parity`
38+
39+
## Progress Log
40+
41+
### 2024-12-20: Completed
42+
43+
#### Changes Made to `zig/src/schema_alter.zig`:
44+
45+
1. **Line 264-272**: Changed clock table CREATE TABLE definition
46+
- `"pk"` → `"key"` in column definition
47+
- `PRIMARY KEY ("pk", "col_name")` → `PRIMARY KEY ("key", "col_name")`
48+
- Added `STRICT` mode to match as_crr.zig
49+
50+
2. **Added db_version index** (new lines after clock table creation):
51+
- Added `CREATE INDEX IF NOT EXISTS "{s}__crsql_clock_dbv_idx" ON "{s}__crsql_clock" ("db_version")`
52+
- Matches as_crr.zig schema
53+
54+
3. **Lines 468-473 (backfillColumn)**: Updated INSERT INTO clock
55+
- `("pk", ...)` → `("key", ...)`
56+
- `WHERE c."pk" = p."pk"` → `WHERE c."key" = p."pk"`
57+
- Note: `p."pk"` stays as is because it references the pks table's pk column
58+
59+
4. **Lines 578, 590 (createInsertTrigger)**: Updated INSERT OR REPLACE INTO clock
60+
- `("pk", ...)` → `("key", ...)`
61+
62+
#### Test Results:
63+
- `bash zig/harness/test-alter.sh`: 6/6 pass
64+
- `bash zig/harness/test-noops.sh`: 4/4 pass
65+
- No regressions in parity tests (filter, rowid-slab, fract tests all pass)
66+
67+
## Completion Notes
68+
- Date: 2024-12-20
69+
- All acceptance criteria met
70+
- The clock table in schema_alter.zig now matches the schema defined in as_crr.zig:
71+
- Uses `"key"` column (not `"pk"`)
72+
- Includes `WITHOUT ROWID, STRICT`
73+
- Includes `_dbv_idx` index on `db_version`

‎.tasks/done/TASK-126-fix-merge-resolution-parity.md‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# TASK-126: Fix merge resolution parity with oracle
2+
3+
## Status
4+
- [x] Completed
5+
6+
## Priority
7+
medium
8+
9+
## Assigned To
10+
(completed)
11+
12+
## Parent Docs / Cross-links
13+
- Test: `zig/harness/test-oracle-parity.sh` (Test 3a, 3b)
14+
- Test: `zig/harness/test-parity.sh` (ValueWin test)
15+
- Gap backlog: `research/zig-cr/92-gap-backlog.md`
16+
17+
## Description
18+
The Zig implementation's merge resolution differs from the Rust/C oracle:
19+
20+
**Test 3a: Remote wins with higher col_version**
21+
- Zig: returns "local" (local value kept)
22+
- Rust/C: returns "remote_winner" (remote value applied)
23+
24+
**Test 3b: site_id tiebreaker (lower site_id wins on equal col_version)**
25+
- Zig: returns empty
26+
- Rust/C: returns "low_site"
27+
28+
**ValueWin test (test-parity.sh)**
29+
- Expected rows_impacted = 1
30+
- Got: empty (merge not applied)
31+
32+
This suggests the merge logic in `changes_vtab.zig` or `merge_insert.zig` is not correctly determining when remote wins.
33+
34+
## Files to Modify
35+
- `zig/src/changes_vtab.zig` — merge resolution logic
36+
- `zig/src/merge_insert.zig` — merge helpers
37+
38+
## Acceptance Criteria
39+
- [x] `zig/harness/test-oracle-parity.sh` Test 3a passes
40+
- [x] `zig/harness/test-oracle-parity.sh` Test 3b passes
41+
- [x] `zig/harness/test-parity.sh` ValueWin test passes
42+
- [x] No regressions in other parity tests
43+
44+
## Progress Log
45+
46+
### 2024-12-20: Root Cause Found and Fixed
47+
48+
**Root Cause:**
49+
The `setWinnerClock` and `setWinnerClockCached` functions in `merge_insert.zig` were attempting to bind a 16-byte site_id BLOB directly to the clock table's `site_id` column, but that column is declared as `INTEGER` (for storing ordinals, not raw blobs).
50+
51+
When applying remote changes via `INSERT INTO crsql_changes`, the incoming site_id is a 16-byte BLOB. The Rust/C oracle correctly converts this to an ordinal via the `crsql_site_id` table before storing in the clock table. The Zig implementation was missing this conversion step, causing a STRICT constraint violation ("cannot store BLOB value in INTEGER column").
52+
53+
This caused ALL remote change merges to fail silently (SQL error at the clock update step), which is why:
54+
- Test 3a failed: remote change with higher col_version couldn't be applied
55+
- Test 3b failed: site_id tiebreaker couldn't work because merges failed
56+
- ValueWin failed: remote value couldn't win because clock update failed
57+
58+
**Fix Applied:**
59+
Modified `merge_insert.zig`:
60+
1. Added import for `site_identity` module
61+
2. Updated `setWinnerClock()` to convert site_id blob to ordinal using `site_identity.getOrCreateSiteOrdinal()` before binding
62+
3. Updated `setWinnerClockCached()` with the same fix
63+
64+
The clock table stores site_id as an integer ordinal (0 = local, 1+ = remote sites), and the `crsql_site_id` table maintains the mapping between ordinals and actual 16-byte site_id blobs.
65+
66+
## Completion Notes
67+
- Date: 2024-12-20
68+
- Files modified:
69+
- `zig/src/merge_insert.zig` (added site_identity import, fixed setWinnerClock and setWinnerClockCached)
70+
- All acceptance criteria verified:
71+
- test-oracle-parity.sh: 18 passed, 0 failed
72+
- test-parity.sh: All rows_impacted tests pass including ValueWin
73+
- No regressions in test-filters.sh, test-rowid-slab.sh, test-alter.sh, test-noops.sh

‎research/zig-cr/92-gap-backlog.md‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,33 @@
11
# 92-gap-backlog
22

3-
> Last updated: 2025-12-20 (Round 52 — TASK-123, TASK-124 complete — clock schema + site_id cross-open fixed)
3+
> Last updated: 2025-12-20 (Round 53 — TASK-125, TASK-126 complete — schema_alter pk→key + merge resolution fixed)
44
55
## Status
66

77
- MVP: ✅ complete (154/154 tests passing)
8+
- Oracle parity: ✅ **18/18 pass** (all divergences fixed)
89
- Zig implementation: `zig/`
910
- Canonical task queue: `.tasks/{backlog,active,done}/`
1011

1112
## Now (next parallel assignments)
1213

13-
Goal: invalidate the hypothesis that "Zig is done" by expanding cross-implementation parity coverage and adding real-system tests.
14+
All oracle parity tests pass. Zig implementation is now wire-compatible with Rust/C oracle.
1415

1516
### Open Gaps (Parity Divergences)
1617
- [x] **TASK-123** — Fix clock table schema parity (pk vs key, index) ✓ `.tasks/done/TASK-123-fix-clock-table-schema-parity.md`
1718
- Column renamed `pk` → `key`, added STRICT mode, added `_dbv_idx` index
1819
- [x] **TASK-124** — Fix site_id preservation on cross-open ✓ `.tasks/done/TASK-124-fix-site-id-cross-open-parity.md`
1920
- Added `crsqlite_version|160300` to `crsql_master` on init
21+
- [x] **TASK-125** — Fix schema_alter.zig pk→key column rename ✓ `.tasks/done/TASK-125-fix-schema-alter-pk-to-key-rename.md`
22+
- Updated all clock table `"pk"` references to `"key"` in schema_alter.zig
23+
- Added STRICT mode and _dbv_idx index to match as_crr.zig
24+
- [x] **TASK-126** — Fix merge resolution parity with oracle ✓ `.tasks/done/TASK-126-fix-merge-resolution-parity.md`
25+
- Fixed site_id blob→ordinal conversion in setWinnerClock/setWinnerClockCached
26+
- All merge resolution tests now pass (Test 3a, 3b, ValueWin)
2027

2128
### Remaining Divergences (from oracle-parity test)
22-
- [ ] Merge resolution (Test 3a/3b): Remote wins / site_id tiebreaker differs — needs investigation
29+
- ~~[ ] Merge resolution (Test 3a/3b): Remote wins / site_id tiebreaker differs — needs investigation~~
30+
- **NONE** — All 18 oracle parity tests pass ✓
2331

2432
### Parity/Coverage Tasks (ready to assign)
2533
- [x] **TASK-070** — Cover missing C suites: ext-data + sandbox ✓ `.tasks/done/TASK-070-zig-parity-extdata-sandbox.md`

‎zig/src/merge_insert.zig‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
const std = @import("std");
2121
const api = @import("ffi/api.zig");
2222
const codec = @import("codec.zig");
23+
const site_identity = @import("site_identity.zig");
2324

2425
/// Error set for merge operations
2526
pub const MergeError = error{
@@ -334,10 +335,14 @@ pub fn setWinnerClock(
334335
_ = api.bind_int64(stmt, 3, col_version);
335336
_ = api.bind_int64(stmt, 4, db_version);
336337

338+
// Convert site_id blob to ordinal for clock table storage
339+
// Clock table stores site_id as INTEGER ordinal, not BLOB
337340
if (site_id_blob) |sid| {
338-
_ = api.bind_blob(stmt, 5, sid, @intCast(site_id_len), api.getTransientDestructor());
341+
const site_slice = sid[0..site_id_len];
342+
const ordinal = site_identity.getOrCreateSiteOrdinal(db, site_slice) orelse 0;
343+
_ = api.bind_int64(stmt, 5, ordinal);
339344
} else {
340-
_ = api.bind_int64(stmt, 5, 0); // Local site_id = 0
345+
_ = api.bind_int64(stmt, 5, 0); // Local site_id = ordinal 0
341346
}
342347

343348
_ = api.bind_int64(stmt, 6, seq);
@@ -918,10 +923,14 @@ pub fn setWinnerClockCached(
918923
_ = api.bind_int64(stmt, 3, col_version);
919924
_ = api.bind_int64(stmt, 4, db_version);
920925

926+
// Convert site_id blob to ordinal for clock table storage
927+
// Clock table stores site_id as INTEGER ordinal, not BLOB
921928
if (site_id_blob) |sid| {
922-
_ = api.bind_blob(stmt, 5, sid, @intCast(site_id_len), api.getTransientDestructor());
929+
const site_slice = sid[0..site_id_len];
930+
const ordinal = site_identity.getOrCreateSiteOrdinal(stmts.db, site_slice) orelse 0;
931+
_ = api.bind_int64(stmt, 5, ordinal);
923932
} else {
924-
_ = api.bind_int64(stmt, 5, 0); // Local site_id = 0
933+
_ = api.bind_int64(stmt, 5, 0); // Local site_id = ordinal 0
925934
}
926935

927936
_ = api.bind_int64(stmt, 6, seq);

0 commit comments

Comments
 (0)