Skip to content

Commit 836d719

Browse files
chore: capture parity edge-case decisions for Tom
1 parent a885618 commit 836d719

File tree

3 files changed

+102
-0
lines changed

3 files changed

+102
-0
lines changed
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# Wish: Decide empty BLOB PK encoding parity (WF-028)
2+
3+
## Context
4+
Our Zig extension currently diverges from the Rust/C oracle for **empty BLOB primary keys** (PK value `X''`) in the **encoded `pk` blob** emitted by `crsql_changes`.
5+
6+
This shows up in the parity suite as:
7+
- `zig/harness/test-pk-blob-parity.sh`**WF-028 FAIL**
8+
9+
## Repro
10+
```bash
11+
cd /Users/tom/Developer/effect-native/cr-sqlite
12+
bash zig/harness/test-pk-blob-parity.sh
13+
```
14+
15+
Current observed output:
16+
- Zig: `0105`
17+
- Rust/C: `0104`
18+
19+
## Why this matters
20+
The `pk` encoding is part of the sync wire format. Divergence means:
21+
- Zig↔Rust/C cross-impl sync might mis-address rows for this edge case
22+
- future tooling/tests that assume byte-identical PK encoding will keep failing
23+
24+
## Recommendation
25+
**Fix Zig to match Rust/C**.
26+
27+
Even though empty BLOB PKs are rare, this is an encoding-level contract, and it’s cheap to keep deterministic parity.
28+
29+
## Likely implementation scope (if approved)
30+
- Investigate `zig/src/pack_columns.zig` handling of empty BLOBs
31+
- Compare to `core/rs/core/src/pack_columns.rs` behavior
32+
- Ensure `X''` is distinguished from `NULL` and matches oracle’s tag/length encoding
33+
34+
## Cross-links
35+
- Existing triage task: `.tasks/triage/TASK-203-empty-blob-pk-encoding-divergence.md`
36+
- Test: `zig/harness/test-pk-blob-parity.sh` (WF-028)
37+
38+
## Decision requested from Tom
39+
- Accept divergence (document + adjust tests), OR
40+
- Approve parity fix in Zig (recommended)
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Wish: Decide behavior for batch apply when some incoming changes are ignorable
2+
3+
## Context
4+
We recently decided **unknown columns during sync are ignored** (lenient behavior) to support rolling upgrades.
5+
6+
That decision interacts with our existing **merge atomicity** expectations.
7+
8+
Today:
9+
- `zig/harness/test-merge-atomicity.sh` reports **2 failing checks**
10+
- Those failures are caused by using “unknown column” as the error injection mechanism, but unknown columns are now **ignored**, so the batch doesn’t fail and the first row legitimately persists.
11+
12+
## Repro
13+
```bash
14+
cd /Users/tom/Developer/effect-native/cr-sqlite
15+
bash zig/harness/test-merge-atomicity.sh
16+
```
17+
18+
Failing checks:
19+
- Test 2: “Invalid column in batch causes entire statement to fail”
20+
- Test 7: “Base table integrity after failed batch”
21+
22+
## What’s the actual decision point?
23+
When applying a batch of incoming changes (often shipped in a single SQL statement with multiple VALUES rows):
24+
25+
If some rows are **ignored by policy** (unknown column), do we want:
26+
27+
1) **Apply the valid subset** (current behavior)
28+
- “best effort apply”
29+
- matches lenient schema mismatch policy
30+
31+
2) **Fail the statement / rollback entire batch** if *any* row is unapplicable
32+
- stricter atomicity semantics
33+
- but conflicts with “ignore unknown columns” unless we special-case
34+
35+
## Recommendation
36+
**Keep applying the valid subset** when the “failure” is an ignorable policy case (unknown column).
37+
38+
Then update `test-merge-atomicity.sh` to inject errors using something that remains a hard error even under lenient schema mismatch, e.g.
39+
- invalid table name
40+
- invalid PK blob / malformed pk encoding
41+
- invalid site_id length (if we decide to add validation)
42+
43+
This maintains a useful atomicity guarantee:
44+
- real errors rollback
45+
- intentionally ignored changes don’t poison the whole batch
46+
47+
## Likely follow-up work (if approved)
48+
- Update `zig/harness/test-merge-atomicity.sh` to align with the chosen policy
49+
- Possibly add a new test that explicitly validates “unknown column rows are ignored but known columns still apply” (this is now part of the contract)
50+
51+
## Cross-links
52+
- Related decision already implemented: `.tasks/done/TASK-186-schema-mismatch-unknown-column-behavior.md`
53+
- Existing spec task: `.tasks/done/TASK-087-spec-merge-atomicity.md`
54+
- Test: `zig/harness/test-merge-atomicity.sh`
55+
56+
## Decision requested from Tom
57+
- Confirm that “unknown columns ignored” implies “best effort apply” within a batch (recommended)
58+
- Or require strict all-or-nothing batch failure semantics even for unknown columns

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ No active tasks. Core sync functionality is complete and working.
6060
### Known Limitations
6161
- **crsql_changes SELECT perf**: ~2-7x slower on wide tables vs Rust/C (COUNT is fast, SELECT * is slow)
6262

63+
### Blocked on Tom (edge-case parity decisions)
64+
- `./.wishes/blocked-on-tom/zig-empty-blob-pk-encoding-parity.md` — decide whether to fix empty BLOB PK encoding (recommended: fix parity)
65+
- `./.wishes/blocked-on-tom/zig-merge-atomicity-vs-lenient-schema-mismatch.md` — decide atomicity semantics under lenient unknown-column policy (recommended: best-effort apply)
66+
6367
### Completed Round 76 (2025-12-25) — seq divergence + schema mismatch fixes
6468
- [x] **TASK-199**: Fix seq value divergence (Zig=1, Rust=0) ✓
6569
- Root cause: `crsqlAfterInsertFunc` called `getNextSeq()` unconditionally for `maybeMarkReinserted`, wasting seq=0

0 commit comments

Comments
 (0)