cache: report a pk row change as one update event #34

Merged
yuris merged 4 commits from feat/21-update-events into main 2026-08-19 17:42:14 +00:00
Owner

Closes #21. Body rewritten after two review rounds — the original evidence claims are superseded.

A one-column change on a primary-key table reached the consumer as delete + insert. The cache state was always right — deletes are applied before inserts so a pk change lands as an upsert — but the event stream reported the row leaving and a different row arriving. In Defold that maps to despawn + respawn: lost tweens, and a go.delete/factory.create pair every tick of movement instead of a go.set_position.

apply_database_update now decodes each batch up front, pairs deletes with inserts by primary key within one updates entry, suppresses the paired delete event, and emits:

{ type = "update", table = name, row = new_row, old = old_row }

The premise is now observed, not inferred

This was the blocker that held the PR: nothing in the repo demonstrated that a real server packs a pk change into a single updates entry. If it split them, the feature would have been inert with a green suite.

A new edit_message reducer, driven over a real v1.json.spacetimedb socket, one subscription — captured twice, independently, by two different agents on separate servers:

"tables":[{"table_name":"message","num_rows":2,
  "updates":[{"deletes":["[2,\"probe\",\"before edit\"]"],
              "inserts":["[2,\"probe\",\"after edit\"]"]}]}]

tables.length == 1, updates.length == 1, both halves of the same key in that one entry.

The edges, probed

scenario wire result
three rows changed in one reducer call one entry, deletes=3 inserts=3 3 × update
primary key value changed (9 → 9001) one entry, keys differ delete + insert — pairing declines, fails safe
delete one row, insert a different row one entry, keys differ delete + insert, no pairing
a row leaving one of two distinct queries entry 0 full pair, entry 1 bare delete of the same key update + delete — pre-existing, now filed as #43

The pk-value case is the one that looked dangerous going in, since pairing by key cannot work there. It reports delete + insert, byte-identical to main, and is now pinned by a test whose two rows are identical apart from the key — so it fails against any implementation that paired by field similarity rather than by key.

That last row is also why cache.lua was not widened to pair across entries: with two distinct queries, entry 1 can be a bare delete of the same key, and any cross-entry join must guess whether it pairs with entry 0's insert. Either guess fabricates or destroys information.

Fan-out, measured

Fan-out is per distinct query text, not per subscription — two SubscribeMulti sets sharing a query text produce one entry between them, and the legacy Subscribe set counts separately from SubscribeMulti sets. That legacy/multi split is why fixture frame 24 has two entries. Both sets were proven live in the identical-query case, so the single entry is real dedup rather than a dead subscription.

Left as-is deliberately: insert and delete already fan out identically (fixture frame 23), counts.inserts/counts.deletes count row operations, and applying the same update twice is a store no-op.

Deliberate boundaries

  • Pairing scope is one upd batch — only there is the insert guaranteed applied last, so a synthesized update can never contradict the store
  • Pairing is by key, not position
  • Tables without a pk keep today's behaviour exactly
  • The batch is now decoded before any mutation, so a malformed row leaves the store untouched — a deliberate improvement over old behaviour, which applied the delete and then raised

Guarded at three levels

Reverting cache.lua to origin/main turns all three red:

  • dev/test/test_cache.lua — FAIL pk delete+insert pair is one event: expected 1, got 2
  • dev/test/test_fixtures.lua — FAIL a pk change is exactly one event: expected 1, got 2
  • Conformance — PASS on the branch, both runs, first try. Phase 1 prints PHASE1 OK light=true, own update still full; pk change is one update event; cached row id=1, and phase 2 confirms the same pk survives the restart. Reverted, it fails end to end through the engine's own socket: SMOKE: FAIL a pk change produced 2 event(s), first of type delete - expected exactly one update → [conformance] FAIL - phase 1 never completed. Phase 1 uses exactly one subscription, so #events == 1 there is stable, not incidental.

5 passed, 0 failed; stylua clean. Fixture recapture verified stable — 24 frames, identical tag sequence, across a fresh and a dirty database.

Known limitation, documented

update is not emitted for tables keyed by an Identity column — that pk decodes to a table, so pairing never fires. Filed as #35, a 1.0 blocker, and noted in the README next to the update row.

Closes #21. **Body rewritten after two review rounds — the original evidence claims are superseded.** A one-column change on a primary-key table reached the consumer as `delete` + `insert`. The cache state was always right — deletes are applied before inserts so a pk change lands as an upsert — but the event stream reported the row leaving and a different row arriving. In Defold that maps to despawn + respawn: lost tweens, and a `go.delete`/`factory.create` pair every tick of movement instead of a `go.set_position`. `apply_database_update` now decodes each batch up front, pairs deletes with inserts **by primary key within one `updates` entry**, suppresses the paired delete event, and emits: ```lua { type = "update", table = name, row = new_row, old = old_row } ``` ## The premise is now observed, not inferred This was the blocker that held the PR: nothing in the repo demonstrated that a real server packs a pk change into a **single** `updates` entry. If it split them, the feature would have been inert with a green suite. A new `edit_message` reducer, driven over a real `v1.json.spacetimedb` socket, one subscription — captured twice, independently, by two different agents on separate servers: ```json "tables":[{"table_name":"message","num_rows":2, "updates":[{"deletes":["[2,\"probe\",\"before edit\"]"], "inserts":["[2,\"probe\",\"after edit\"]"]}]}] ``` `tables.length == 1`, `updates.length == 1`, both halves of the same key in that one entry. ## The edges, probed | scenario | wire | result | |---|---|---| | three rows changed in one reducer call | one entry, `deletes=3 inserts=3` | 3 × `update` | | **primary key *value* changed** (9 → 9001) | one entry, keys differ | `delete` + `insert` — pairing declines, fails safe | | delete one row, insert a different row | one entry, keys differ | `delete` + `insert`, no pairing | | a row leaving one of two distinct queries | entry 0 full pair, entry 1 bare delete of the same key | `update` + `delete` — pre-existing, now filed as #43 | The pk-value case is the one that looked dangerous going in, since pairing by key *cannot* work there. It reports `delete` + `insert`, byte-identical to `main`, and is now pinned by a test whose two rows are identical apart from the key — so it fails against any implementation that paired by field similarity rather than by key. That last row is also why `cache.lua` was **not** widened to pair across entries: with two distinct queries, entry 1 can be a bare delete of the same key, and any cross-entry join must guess whether it pairs with entry 0's insert. Either guess fabricates or destroys information. ## Fan-out, measured Fan-out is per **distinct query text**, not per subscription — two `SubscribeMulti` sets sharing a query text produce **one** entry between them, and the legacy `Subscribe` set counts separately from `SubscribeMulti` sets. That legacy/multi split is why fixture frame 24 has two entries. Both sets were proven live in the identical-query case, so the single entry is real dedup rather than a dead subscription. Left as-is deliberately: `insert` and `delete` already fan out identically (fixture frame 23), `counts.inserts`/`counts.deletes` count row operations, and applying the same `update` twice is a store no-op. ## Deliberate boundaries - Pairing scope is one `upd` batch — only there is the insert guaranteed applied last, so a synthesized `update` can never contradict the store - Pairing is by key, not position - Tables without a `pk` keep today's behaviour exactly - The batch is now decoded before any mutation, so a malformed row leaves the store untouched — a deliberate improvement over old behaviour, which applied the delete and then raised ## Guarded at three levels Reverting `cache.lua` to `origin/main` turns all three red: - `dev/test/test_cache.lua` — `FAIL pk delete+insert pair is one event: expected 1, got 2` - `dev/test/test_fixtures.lua` — `FAIL a pk change is exactly one event: expected 1, got 2` - **Conformance — PASS on the branch, both runs, first try.** Phase 1 prints `PHASE1 OK light=true, own update still full; pk change is one update event; cached row id=1`, and phase 2 confirms the same pk survives the restart. Reverted, it fails end to end through the engine's own socket: `SMOKE: FAIL a pk change produced 2 event(s), first of type delete - expected exactly one update` → `[conformance] FAIL - phase 1 never completed`. Phase 1 uses exactly one subscription, so `#events == 1` there is stable, not incidental. `5 passed, 0 failed`; stylua clean. Fixture recapture verified stable — 24 frames, identical tag sequence, across a fresh and a dirty database. ## Known limitation, documented `update` is not emitted for tables keyed by an `Identity` column — that pk decodes to a table, so pairing never fires. Filed as **#35**, a 1.0 blocker, and noted in the README next to the `update` row.
cache: report a pk row change as one update event
All checks were successful
ci / ci (pull_request) Successful in 19s
51ebf3c40b
The cache already got the state right — deletes are applied before inserts so a
pk change lands as an upsert — but the event stream still described it as a row
leaving and a different row arriving. In Defold the natural mapping is event to
game object, so a player moving one tile ran go.delete plus factory.create every
tick: the sprite flickers and any tween or animation state on it is lost. The
alternative was every consumer re-implementing delete/insert diffing by pk,
which is exactly the bookkeeping an SDK exists to absorb.

A delete and an insert of the same key inside one batch are now one
{ type = "update", row, old } event. Pairing is by key, never by position: the
server sends deletes first today, but the two wire arrays carry no ordering
guarantee and depending on one would be a silent trap later. Scope is a single
upd, which is also the only scope where the synthesized event cannot contradict
the store — within one upd the insert is always applied last, so the paired row
is guaranteed present afterwards. Across upds or messages an insert can precede
its delete, and an "update" for a row that ended up gone would be worse than the
two events it replaced.

Tables without a pk keep both events. There is no defensible pairing without a
key, and matching on field equality would fold genuine same-shape churn into a
bogus update; the official SDKs draw the line in the same place, with on_update
living only on TableWithPrimaryKey.

counts.inserts and counts.deletes still count row operations rather than
synthesized events, so an update adds one to each and the conformance
assertions that read those numbers keep their meaning. The store is untouched.

The pairing tests were checked falsifiable by reverting cache.lua: the pair,
keyed-not-positional and array-order cases all fail without it. The pk-less,
unpaired-delete, cross-table and cross-message cases necessarily pass against
the old code — they assert behaviour that must not change, and guard against a
future over-eager pairing rather than against the old code.
Author
Owner

Review — automated reviewer, posted for the record

Review: PR #34 — cache: report a pk row change as one update event

Verified against a clone of feat/21-update-events (51ebf3c) with origin/main's cache.lua as the control. Suite: 5 passed, 0 failed under Lua 5.1.5. All probe/benchmark numbers below are from runs I did, not from reading.

Verdict: approve after should-fixes. I tried hard to break the pairing and could not. Everything I found is in the tests, the docs, or the PR body's claims — with one exception (#11) that is pre-existing but decides whether this feature does anything in production, and therefore has to be decided before the freeze.


What I verified as correct

  • Pairing is keyed, within one upd batch (cache.lua:131-139). Per-upd rather than per-TableUpdate is the right scope, and for a sharper reason than the PR body gives: the apply loop runs deletes-then-inserts per batch, so upd[2]'s delete can remove a row upd[1] inserted. Widening the pairing scope to the whole TableUpdate would let a synthesized update contradict the final store. Probe case 8 (delete in batch 1, insert of the same pk in batch 2) → delete + insert, store correct. The conservative scope is load-bearing, not just cautious.
  • nil pk never indexes with a nil key. pair_by_key guards on the write (cache.lua:102), and every read — paired[row[def.pk]] at cache.lua:148, 155, 157 — is a nil-key read, which is legal in Lua 5.1. Probe case 3 (delete row with id=null) → plain delete, no error, byte-identical to old. Probe case 4 (insert with id=null) raises the same store error at the same point as old code. No silent pairing of two keyless rows: both are skipped from the map.
  • pk = false pairs correctly (probe case 5 → one update). This works because cache.lua:102/109 test key ~= nil rather than truthiness. Worth a one-word comment so a future "simplification" to if key then doesn't break it.
  • Decoding up front cannot change the store. decode_rows touches no state, and the pk-less remove field-wise scan (cache.lua:64-79) compares row contents, which decode order cannot affect; its pairs(row) loop is iteration-order-independent because it only ever breaks on a mismatch. Probe case 10 (pk-less, 2 deletes of duplicate content + 1 insert) gives identical events and identical store on old and new.
  • Duplicate deletes of one key lose no event. Probe case 2 (del(v0), del(v1), ins(v2)) → delete(v0) + update(old=v1), ins=1 del=2. The identity comparison at cache.lua:148 does what the PR body says. (But see #3 — it is untested.)
  • counts.inserts / counts.deletes still count row operations in every probe case.
  • update genuinely needs nothing in connection.lua. The only consumers of the return value are connection.lua:235, 268, 307, 325, which pass counts through opaquely; grep finds no branch on event type anywhere in stdb/. The README example at README.md:179 prints e.row.sender for every type and works unchanged for update — which is a good argument for having named the new row row rather than new. Endorsing that naming for the freeze: one uniform e.row across all three types is worth more than symmetry with old.
  • Purity and 5.1 compliance: no Defold globals, no 5.2+ constructs in the new code.

Findings

1. should-fix — README.md:285: "The conformance test proves this" now dangles, and is false for what it now follows

The two new blocks were inserted between the evt.type table and the paragraph that referred to it. "This" now reads as if conformance proves the e.type table — and per finding #4, conformance proves nothing about update. Move the new blocks below README.md:285-286, or re-anchor the sentence ("The conformance test proves the reconnect path against a real server restart…").

2. should-fix — block E is a vacuous test; tbl_deletes_first is not a different fixture

test_cache.lua:119-125. Lua table constructors with named keys have no order: { deletes = d, inserts = i } and { inserts = i, deletes = d } build structurally identical tables, and tbl_deletes_first even has the same signature as tbl. I confirmed it (same key set, same contents). cache.lua reads upd.deletes / upd.inserts by name — there is no array order to vary. The comment at test_cache.lua:117-118 ("The server lists deletes first today; pairing must not depend on that") describes a property this test cannot exercise; it is a re-run of block A with different strings.

The PR body lists E among "the real guards". It does fail when reverted — but only because it re-asserts A. The real guard for keyed-not-positional is D. Fix: delete block E and its helper, and spend the lines on the untested cases in #3.

3. should-fix — the subtlest line in the diff has no test

cache.lua:146-148 (paired[row[def.pk]] == row) is the one non-obvious line, and the PR body asserts its behaviour in prose ("Duplicate deletes of one key are handled by identity comparison, so no event is silently dropped") with nothing exercising it. I verified it works — but a future refactor to the more natural-looking if not (paired and paired[row[def.pk]]) then silently drops an event and the suite stays green.

Add, in place of block E:

  • two deletes of one key + one insert → delete then update, deletes == 2 (guards the identity comparison)
  • one delete + two inserts of one key → update then insert, inserts == 2
  • a delete whose pk field is null → still a delete, no error (guards the nil-key path against a future paired[key] write)

4. should-fix — nothing establishes that a real server packs the delete and the insert into one updates entry

dev/server/index.js has exactly two reducers: sendMessage (insert) and clearAll (delete). There is no update reducer anywhere in the repo, and every batch in dev/test/fixtures/session.jsonl is deletes-only or inserts-only. The premise this whole PR rests on — inherited from a pre-existing comment, not from an observation — has never been seen on the wire.

This matters because updates[] is a list of QueryUpdates, one per matching subscription query. If the server splits a pk change across two entries, probe case 8 is what consumers get: delete + insert, feature inert, no test failing anywhere. "Conformance not run (needs a live server)" is exactly the wrong caveat for the one behaviour that has no other evidence.

Fix: add editMessage (ctx.db.message.id.update({...})) to dev/server/index.js, assert one update event in dev/example/main/smoke.script, recapture session.jsonl. At an API freeze this is the difference between a feature and a plausible feature.

5. should-fix — "the cache's final state is byte-identical" is over-claimed

Probe case 7: batch with deletes = [row 1], inserts = ["not json"].

store after the raise
old {2} — delete applied, then decode_row raised
new {1,2} — raised before any delete was applied

connection.lua:400 raises to its caller, so this is reachable from any malformed frame. The new behaviour is better (all-or-nothing per batch), but it is a change, and "byte-identical in every case" is the kind of claim a reviewer checks. Either drop it or claim the improvement deliberately.

6. should-fix — event ordering is now frozen contract and is written down nowhere

Consumers iterate counts.events, so the order is API. It is deterministic — probe case 6 (two pairs, inserts in reverse order of deletes) → two updates in insert order, reproducibly — but the rule is only inferable from the code. One README clause:

Entries follow wire order. An unpaired delete keeps its position ahead of the inserts; a synthesized update appears at its insert's position.

The tradeoff that hides in that sentence, which looks deliberate and correct to me: a paired row's "went away" notification moves later relative to other inserts in the same batch. Position relative to the insert is preserved exactly, and this matches where peer SDKs fire on_update. Worth stating so nobody re-litigates it post-freeze.

7. nit — an unchanged row now reports as update

Probe case 9: del [1,"a","v0"] + ins [1,"a","v0"] → one update whose old content-equals row. README.md:280 says "changed a row in place". Relevant because apply_database_update is also the Subscribe/Unsubscribe-applied path (connection.lua:268), where a delete+insert of one key is query churn rather than a change — so the doc comment at cache.lua:135 ("the server's own framing for one transaction's changes") is narrower than the code path it describes. One clause in the README, one word in the comment.

8. nit — a trailing insert for a key that already exists

Probe case 1: del(1) + ins(1) + ins(1) → update(v1), insert(v2). A consumer mapping insert → spawn creates a second game object for a key it just updated. Old code was equally wrong (insert+insert) and the server should never emit this, so it is defensive only. Mention or ignore.

9. nit — with duplicate deletes, old is the last one (cache.lua:103 overwrites). Arbitrary but harmless; one word in the comment if you keep the handling.

10. Performance — not materially worse; a wash to a small win

Measured, lua 5.1.5, new vs old:

scenario new old
20k-row InitialSubscription 444–452 ms 448–460 ms
200 pk updates × 600 batches 860–866 ms 886–890 ms
1 pk update × 20000 batches 213–215 ms 194–197 ms
empty batch × 20000 22.0 ms 18.8 ms

The up-front decode adds two table allocations per (table × upd) unconditionally, plus two more when pairing is attempted. On a large InitialSubscription pair_by_key early-returns on #deleted == 0, so the only cost is one N-slot array of references (~8% more allocation churn, no measurable time). On a batch with many pairs it is a net win — halving the event count saves more tables than the pairing allocates. The +7–10% on single-row batches is a microbenchmark where the cache is the only work; against JSON decode and the socket it is noise. Nothing here changes the #30 picture.

One lazy improvement if you want the unconditional cost back: a module-level local EMPTY = {} returned by decode_rows for nil/empty input. Neither array is ever mutated (pair_by_key and both loops only read), so sharing is safe, and it also kills the throwaway table #(encoded or {}) allocates at cache.lua:85. While there, the or nil at cache.lua:139 is dead — def.pk and pair_by_key(...) is already nil when def.pk is nil.

11. Pre-existing, but it decides whether #21's value lands: an Identity-typed pk never pairs

protocol.decode_row (protocol.lua:142-167) never calls protocol.unwrap on field values, so {"__identity__":"0x…"} decodes to a table and the pk becomes a table reference. Run against both old and new cache.lua, delete+insert of one identity on a pk = "owner" table:

decoded pk type: table  0xabc
events=2  type1=delete  rows_in_store=2

Two rows in the store for one logical row, and no update. A player table keyed by Identity is the canonical SpacetimeDB game pattern, and it is exactly the despawn/respawn footgun the issue opens with. The whole test corpus dodges it: chat_schema.lua is u64/string/string.

Not this PR's bug and not this PR's fix — unwrapping row fields changes row field types, i.e. the surface being frozen. But it has to be decided before 1.0, and the honest framing of this PR is that its headline value is conditional on it. File it now; if it will not be fixed for 1.0, say so in the README update row. Same class, worth one line in that issue: a u64 pk above 2^53 collides in Lua 5.1's doubles, and a pk arriving as "1" in one array and 1 in the other never pairs.


Falsifiability report: honest, with one wrong reason

I reproduced it — each new block extracted and run in isolation (prelude + block) against origin/main's cache.lua:

block reverted PR
A pair is one event FAIL expected 1, got 2 PASS
B lone delete PASS PASS
C pk-less keeps two PASS PASS
D keyed not positional FAIL expected 3, got 4 PASS
E array order FAIL expected 1, got 2 PASS
F across tables PASS PASS
G across messages PASS PASS

Exactly the claimed classification, and the quoted failure message is verbatim. Specifically checked:

  • A cannot pass against old code. Its first failing assert is #counts.events == 1 (2 on old). It also asserts counts.events[1].old.text — which on old code would index a nil field on the delete event, so it fails twice over.
  • A and D both assert on old as well as row (test_cache.lua:76, 112). E asserts only row — moot once E goes.
  • "positional pairing trips D" — correct. Positional zip of deletes=[1,2] with inserts=[3,2!] yields 2 events; D asserts 3.
  • "field-equality pairing on pk-less tables trips C" — wrong. C's rows are ["first"] → ["second"], sharing no field, so any equality-based pairing finds no pair and C passes. What C actually trips is positional pairing on pk-less tables (1 event vs the asserted 2). Small, but that paragraph exists to be precise.

Does it close #21?

Yes — every acceptance criterion is met:

criterion where
unit test: delete+insert of one pk in one batch → exactly one update with correct old and row test_cache.lua:68-82
pk-less table still emits two separate events test_cache.lua:89-96
unpaired delete stays a delete test_cache.lua:84-87
pairing by key within the batch, not by position cache.lua:95-116, tested at test_cache.lua:98-115
counts.inserts / counts.deletes keep counting row operations test_cache.lua:78-79, 114-115
no correlation across tables or across messages test_cache.lua:136-153
README gains the update row and notes pk-less tables never produce one README.md:276-283

Nothing in the issue's acceptance list is unmet. The two gaps are things the issue did not think to ask for: finding #4 (no real-server evidence for the premise) and finding #3 (a claimed behaviour with no test).


Suggested landing order

  1. #1, #6, #7 — README/comment accuracy (minutes).
  2. #2 + #3 — swap block E for the three real edge cases.
  3. #5 — correct the PR body, or claim the atomicity improvement.
  4. #4 — editMessage reducer + recaptured fixture. I would not freeze this API without it.
  5. #11 — file separately, decide before 1.0.
  6. #8, #9, #10 — optional.

Verdict: Approve after should-fixes. The pairing logic itself is correct — I could not break it. Six should-fixes, all in tests/docs/claims rather than in cache.lua, plus one pre-existing bug that decides whether this feature actually does anything in production.

Closes issue: Yes. Every acceptance criterion in #21 is met. Two gaps beyond the stated criteria: no real-server evidence that the server packs a pk change into one updates entry, and the PR body's duplicate-delete claim has no test.

## Review — automated reviewer, posted for the record # Review: PR #34 — `cache: report a pk row change as one update event` Verified against a clone of `feat/21-update-events` (51ebf3c) with `origin/main`'s `cache.lua` as the control. Suite: `5 passed, 0 failed` under `Lua 5.1.5`. All probe/benchmark numbers below are from runs I did, not from reading. **Verdict: approve after should-fixes.** I tried hard to break the pairing and could not. Everything I found is in the tests, the docs, or the PR body's claims — with one exception (#11) that is pre-existing but decides whether this feature does anything in production, and therefore has to be decided before the freeze. --- ## What I verified as correct - **Pairing is keyed, within one `upd` batch** (`cache.lua:131-139`). Per-`upd` rather than per-`TableUpdate` is the *right* scope, and for a sharper reason than the PR body gives: the apply loop runs deletes-then-inserts **per batch**, so `upd[2]`'s delete can remove a row `upd[1]` inserted. Widening the pairing scope to the whole `TableUpdate` would let a synthesized `update` contradict the final store. Probe case 8 (delete in batch 1, insert of the same pk in batch 2) → `delete` + `insert`, store correct. The conservative scope is load-bearing, not just cautious. - **`nil` pk never indexes with a nil key.** `pair_by_key` guards on the *write* (`cache.lua:102`), and every read — `paired[row[def.pk]]` at `cache.lua:148`, `155`, `157` — is a nil-key *read*, which is legal in Lua 5.1. Probe case 3 (delete row with `id=null`) → plain `delete`, no error, byte-identical to old. Probe case 4 (insert with `id=null`) raises the same `store` error at the same point as old code. No silent pairing of two keyless rows: both are skipped from the map. - **`pk = false` pairs correctly** (probe case 5 → one `update`). This works *because* `cache.lua:102`/`109` test `key ~= nil` rather than truthiness. Worth a one-word comment so a future "simplification" to `if key then` doesn't break it. - **Decoding up front cannot change the store.** `decode_rows` touches no state, and the pk-less `remove` field-wise scan (`cache.lua:64-79`) compares row *contents*, which decode order cannot affect; its `pairs(row)` loop is iteration-order-independent because it only ever breaks on a mismatch. Probe case 10 (pk-less, 2 deletes of duplicate content + 1 insert) gives identical events and identical store on old and new. - **Duplicate deletes of one key lose no event.** Probe case 2 (`del(v0)`, `del(v1)`, `ins(v2)`) → `delete(v0)` + `update(old=v1)`, `ins=1 del=2`. The identity comparison at `cache.lua:148` does what the PR body says. (But see #3 — it is untested.) - **`counts.inserts` / `counts.deletes` still count row operations** in every probe case. - **`update` genuinely needs nothing in `connection.lua`.** The only consumers of the return value are `connection.lua:235, 268, 307, 325`, which pass `counts` through opaquely; grep finds no branch on event type anywhere in `stdb/`. The README example at `README.md:179` prints `e.row.sender` for every type and works unchanged for `update` — which is a good argument for having named the new row `row` rather than `new`. **Endorsing that naming for the freeze**: one uniform `e.row` across all three types is worth more than symmetry with `old`. - **Purity and 5.1 compliance:** no Defold globals, no 5.2+ constructs in the new code. --- ## Findings ### 1. should-fix — `README.md:285`: "The conformance test proves this" now dangles, and is false for what it now follows The two new blocks were inserted *between* the `evt.type` table and the paragraph that referred to it. "This" now reads as if conformance proves the `e.type` table — and per finding #4, conformance proves nothing about `update`. Move the new blocks below `README.md:285-286`, or re-anchor the sentence ("The conformance test proves the reconnect path against a real server restart…"). ### 2. should-fix — block E is a vacuous test; `tbl_deletes_first` is not a different fixture `test_cache.lua:119-125`. Lua table constructors with named keys have no order: `{ deletes = d, inserts = i }` and `{ inserts = i, deletes = d }` build structurally identical tables, and `tbl_deletes_first` even has the same signature as `tbl`. I confirmed it (same key set, same contents). `cache.lua` reads `upd.deletes` / `upd.inserts` by name — there is no array order to vary. The comment at `test_cache.lua:117-118` ("The server lists deletes first today; pairing must not depend on that") describes a property this test cannot exercise; it is a re-run of block A with different strings. The PR body lists E among "the real guards". It does fail when reverted — but only because it re-asserts A. **The real guard for keyed-not-positional is D.** Fix: delete block E and its helper, and spend the lines on the untested cases in #3. ### 3. should-fix — the subtlest line in the diff has no test `cache.lua:146-148` (`paired[row[def.pk]] == row`) is the one non-obvious line, and the PR body asserts its behaviour in prose ("Duplicate deletes of one key are handled by identity comparison, so no event is silently dropped") with nothing exercising it. I verified it works — but a future refactor to the more natural-looking `if not (paired and paired[row[def.pk]]) then` silently drops an event and the suite stays green. Add, in place of block E: - two deletes of one key + one insert → `delete` then `update`, `deletes == 2` (guards the identity comparison) - one delete + two inserts of one key → `update` then `insert`, `inserts == 2` - a delete whose pk field is `null` → still a `delete`, no error (guards the nil-key path against a future `paired[key]` write) ### 4. should-fix — nothing establishes that a real server packs the delete and the insert into one `updates` entry `dev/server/index.js` has exactly two reducers: `sendMessage` (insert) and `clearAll` (delete). There is **no update reducer anywhere in the repo**, and every batch in `dev/test/fixtures/session.jsonl` is deletes-only or inserts-only. The premise this whole PR rests on — inherited from a pre-existing comment, not from an observation — has never been seen on the wire. This matters because `updates[]` is a list of *QueryUpdates*, one per matching subscription query. If the server splits a pk change across two entries, probe case 8 is what consumers get: `delete` + `insert`, feature inert, no test failing anywhere. "Conformance not run (needs a live server)" is exactly the wrong caveat for the one behaviour that has no other evidence. Fix: add `editMessage` (`ctx.db.message.id.update({...})`) to `dev/server/index.js`, assert one `update` event in `dev/example/main/smoke.script`, recapture `session.jsonl`. At an API freeze this is the difference between a feature and a plausible feature. ### 5. should-fix — "the cache's final state is byte-identical" is over-claimed Probe case 7: batch with `deletes = [row 1]`, `inserts = ["not json"]`. | | store after the raise | |---|---| | old | `{2}` — delete applied, then `decode_row` raised | | new | `{1,2}` — raised before any delete was applied | `connection.lua:400` raises to its caller, so this is reachable from any malformed frame. The new behaviour is **better** (all-or-nothing per batch), but it is a change, and "byte-identical in every case" is the kind of claim a reviewer checks. Either drop it or claim the improvement deliberately. ### 6. should-fix — event ordering is now frozen contract and is written down nowhere Consumers iterate `counts.events`, so the order is API. It is deterministic — probe case 6 (two pairs, inserts in reverse order of deletes) → two `update`s in *insert* order, reproducibly — but the rule is only inferable from the code. One README clause: > Entries follow wire order. An unpaired `delete` keeps its position ahead of the inserts; a synthesized `update` appears at its insert's position. The tradeoff that hides in that sentence, which looks deliberate and correct to me: a paired row's "went away" notification moves *later* relative to other inserts in the same batch. Position relative to the insert is preserved exactly, and this matches where peer SDKs fire `on_update`. Worth stating so nobody re-litigates it post-freeze. ### 7. nit — an unchanged row now reports as `update` Probe case 9: `del [1,"a","v0"]` + `ins [1,"a","v0"]` → one `update` whose `old` content-equals `row`. `README.md:280` says "changed a row in place". Relevant because `apply_database_update` is also the Subscribe/Unsubscribe-applied path (`connection.lua:268`), where a delete+insert of one key is query churn rather than a change — so the doc comment at `cache.lua:135` ("the server's own framing for one transaction's changes") is narrower than the code path it describes. One clause in the README, one word in the comment. ### 8. nit — a trailing `insert` for a key that already exists Probe case 1: `del(1) + ins(1) + ins(1)` → `update(v1)`, `insert(v2)`. A consumer mapping `insert` → spawn creates a second game object for a key it just updated. Old code was equally wrong (`insert`+`insert`) and the server should never emit this, so it is defensive only. Mention or ignore. ### 9. nit — with duplicate deletes, `old` is the last one (`cache.lua:103` overwrites). Arbitrary but harmless; one word in the comment if you keep the handling. ### 10. Performance — not materially worse; a wash to a small win Measured, `lua 5.1.5`, new vs old: | scenario | new | old | |---|---|---| | 20k-row `InitialSubscription` | 444–452 ms | 448–460 ms | | 200 pk updates × 600 batches | 860–866 ms | 886–890 ms | | 1 pk update × 20000 batches | 213–215 ms | 194–197 ms | | empty batch × 20000 | 22.0 ms | 18.8 ms | The up-front decode adds two table allocations per (table × `upd`) unconditionally, plus two more when pairing is attempted. On a large `InitialSubscription` `pair_by_key` early-returns on `#deleted == 0`, so the only cost is one N-slot array of references (~8% more allocation churn, no measurable time). On a batch with many pairs it is a **net win** — halving the event count saves more tables than the pairing allocates. The +7–10% on single-row batches is a microbenchmark where the cache is the only work; against JSON decode and the socket it is noise. Nothing here changes the #30 picture. One lazy improvement if you want the unconditional cost back: a module-level `local EMPTY = {}` returned by `decode_rows` for nil/empty input. Neither array is ever mutated (`pair_by_key` and both loops only read), so sharing is safe, and it also kills the throwaway table `#(encoded or {})` allocates at `cache.lua:85`. While there, the `or nil` at `cache.lua:139` is dead — `def.pk and pair_by_key(...)` is already `nil` when `def.pk` is `nil`. ### 11. Pre-existing, but it decides whether #21's value lands: an `Identity`-typed pk never pairs `protocol.decode_row` (`protocol.lua:142-167`) never calls `protocol.unwrap` on field values, so `{"__identity__":"0x…"}` decodes to a **table** and the pk becomes a table reference. Run against both old and new `cache.lua`, delete+insert of one identity on a `pk = "owner"` table: ``` decoded pk type: table 0xabc events=2 type1=delete rows_in_store=2 ``` Two rows in the store for one logical row, and no `update`. A `player` table keyed by `Identity` is *the* canonical SpacetimeDB game pattern, and it is exactly the despawn/respawn footgun the issue opens with. The whole test corpus dodges it: `chat_schema.lua` is `u64`/`string`/`string`. Not this PR's bug and not this PR's fix — unwrapping row fields changes row field types, i.e. the surface being frozen. But it has to be *decided* before 1.0, and the honest framing of this PR is that its headline value is conditional on it. File it now; if it will not be fixed for 1.0, say so in the README `update` row. Same class, worth one line in that issue: a `u64` pk above 2^53 collides in Lua 5.1's doubles, and a pk arriving as `"1"` in one array and `1` in the other never pairs. --- ## Falsifiability report: honest, with one wrong reason I reproduced it — each new block extracted and run in isolation (prelude + block) against `origin/main`'s `cache.lua`: | block | reverted | PR | |---|---|---| | A pair is one event | **FAIL** `expected 1, got 2` | PASS | | B lone delete | PASS | PASS | | C pk-less keeps two | PASS | PASS | | D keyed not positional | **FAIL** `expected 3, got 4` | PASS | | E array order | **FAIL** `expected 1, got 2` | PASS | | F across tables | PASS | PASS | | G across messages | PASS | PASS | Exactly the claimed classification, and the quoted failure message is verbatim. Specifically checked: - **A cannot pass against old code.** Its first failing assert is `#counts.events == 1` (2 on old). It also asserts `counts.events[1].old.text` — which on old code would index a `nil` field on the delete event, so it fails twice over. - **A and D both assert on `old` as well as `row`** (`test_cache.lua:76`, `112`). E asserts only `row` — moot once E goes. - **"positional pairing trips D"** — correct. Positional zip of `deletes=[1,2]` with `inserts=[3,2!]` yields 2 events; D asserts 3. - **"field-equality pairing on pk-less tables trips C" — wrong.** C's rows are `["first"]` → `["second"]`, sharing no field, so *any* equality-based pairing finds no pair and C passes. What C actually trips is **positional** pairing on pk-less tables (1 event vs the asserted 2). Small, but that paragraph exists to be precise. --- ## Does it close #21? **Yes — every acceptance criterion is met:** | criterion | where | |---|---| | unit test: delete+insert of one pk in one batch → exactly one `update` with correct `old` and `row` | `test_cache.lua:68-82` | | pk-less table still emits two separate events | `test_cache.lua:89-96` | | unpaired delete stays a `delete` | `test_cache.lua:84-87` | | pairing by key within the batch, not by position | `cache.lua:95-116`, tested at `test_cache.lua:98-115` | | `counts.inserts` / `counts.deletes` keep counting row operations | `test_cache.lua:78-79, 114-115` | | no correlation across tables or across messages | `test_cache.lua:136-153` | | README gains the `update` row and notes pk-less tables never produce one | `README.md:276-283` | Nothing in the issue's acceptance list is unmet. The two gaps are things the issue did not think to ask for: finding #4 (no real-server evidence for the premise) and finding #3 (a claimed behaviour with no test). --- ## Suggested landing order 1. #1, #6, #7 — README/comment accuracy (minutes). 2. #2 + #3 — swap block E for the three real edge cases. 3. #5 — correct the PR body, or claim the atomicity improvement. 4. #4 — `editMessage` reducer + recaptured fixture. I would not freeze this API without it. 5. #11 — file separately, decide before 1.0. 6. #8, #9, #10 — optional. --- **Verdict:** Approve after should-fixes. The pairing logic itself is correct — I could not break it. Six should-fixes, all in tests/docs/claims rather than in cache.lua, plus one pre-existing bug that decides whether this feature actually does anything in production. **Closes issue:** Yes. Every acceptance criterion in #21 is met. Two gaps beyond the stated criteria: no real-server evidence that the server packs a pk change into one `updates` entry, and the PR body's duplicate-delete claim has no test.
cache: test the pairing edge cases, document the frozen contract
All checks were successful
ci / ci (pull_request) Successful in 19s
9358d0267c
Review follow-up on #34.

Tests: block E (`tbl_deletes_first`) was vacuous — Lua table constructors
with named keys have no order, so it built a table structurally identical
to `tbl` and only re-asserted block A. Deleted, and the lines spent on the
three cases that were actually untested:

- two deletes of one key + one insert: the delete that cannot pair must
  still surface. This is the guard for the `paired[row[pk]] == row`
  identity comparison, the subtlest line in the change. Verified it fails
  (`expected 2, got 1`) against the natural-looking
  `if not (paired and paired[row[pk]])`.
- one delete + two inserts of one key: the pair is consumed once, so the
  trailing insert stays an insert.
- a delete with an absent pk alongside a real pair: still a delete, and
  the nil key stays a read rather than a write into the pair map.
- a malformed row mid-batch: raises with the store untouched.

That last one corrects a claim rather than pinning existing behaviour.
Decoding up front made the batch all-or-nothing: on `deletes=[valid]` +
`inserts=[garbage]` the old code applied the delete and then raised, the
new code raises first. That is better, and it is now stated in the comment
and pinned by a test.

README: event order is contract now that consumers iterate
`counts.events`, so the rule is written down. Also noted that a row
re-sent unchanged reports as an `update` (the subscribe path is query
churn, not edits), that identity-keyed tables do not pair yet (#35), and
re-anchored the conformance sentence the new blocks had orphaned.

decode_rows returns a shared EMPTY for the nil/empty case — neither array
is ever mutated — which drops two allocations per batch and the throwaway
table `#(encoded or {})` built. Dropped a dead `or nil`.
Author
Owner

Review actioned — verdict: NEEDS ANOTHER REVIEW

Every should-fix in tests, docs and claims is done. The suite is green, stylua is clean, and the new identity-comparison guard is proven to go red against the plausible refactor. 9358d02 — cache: test the pairing edge cases, document the frozen contract, 3 files, +79 -25.

Actioned

  • Block E deleted. Confirmed independently: it built { deletes = d, inserts = i } versus { inserts = i, deletes = d }, which are the same table — named keys in a Lua constructor carry no order, and cache.lua reads them by name. There was no array order to vary; it was block A with different strings. Block D remains the real keyed-not-positional guard.
  • Three edge-case tests added in its place: two deletes + one insert (guards the paired[row[pk]] == row identity comparison), one delete + two inserts (guards pair consumption, and pins the trailing-insert behaviour as intentional), and a delete with an absent pk. The last one was made deliberately stronger than suggested: with a delete-only batch paired is nil and Lua short-circuits before row[def.pk] is read, so the nil-key path would never be exercised — it now sits alongside a real pair so the paired[nil] read actually happens.
  • Atomicity claimed deliberately rather than hand-waved, plus a test: a batch of deletes=[valid] + inserts=["not json"] raises and leaves the store untouched. Old code applied the delete first. The corrected PR-body wording is below.
  • Event ordering written down in the README — it is frozen contract now.
  • Dangling "The conformance test proves this" sentence re-anchored; unchanged-row-reports-as-update documented; duplicate-delete old semantics recorded; both perf nits taken (local EMPTY = {}, dead or nil dropped).
  • #35 caveat added to the README's update row: not yet emitted for tables keyed by an Identity column.

Falsifiability, verified by mutation

Replaced the identity comparison with the natural-looking if not (paired and paired[row[def.pk]]) then:

FAIL two deletes and one insert of a key are two events: expected 2, got 1 — 4 passed, 1 failed

Restored: 5 passed, 0 failed.

Corrected claim for finding 5

[object Object]

Why this still needs another pass

Not because of anything in this commit. Finding 4 is unresolved and cannot be resolved on this branch: nothing in the repo demonstrates that a real server packs a pk change into a single updates entry. dev/server/index.js has only sendMessage (insert) and clearAll (delete) — there is no update reducer anywhere, and every batch in session.jsonl is deletes-only or inserts-only. The premise this PR rests on was inherited from a pre-existing comment, not from an observation.

If the server splits a pk change across two updates entries, consumers get delete + insert, this feature is inert, and no test fails. At an API freeze that is the difference between a feature and a plausible feature.

dev/server/, dev/test/fixtures/ and dev/tools/ are owned by feat/25-codegen-types (PR #36) right now, so this is sequenced: once #36 merges, add an editMessage reducer, assert exactly one update in smoke.script, recapture session.jsonl, and confirm the delete and insert arrive in one entry. Holding this PR until that is proven.

## Review actioned — verdict: **NEEDS ANOTHER REVIEW** Every should-fix in tests, docs and claims is done. The suite is green, stylua is clean, and the new identity-comparison guard is proven to go red against the plausible refactor. `9358d02 — cache: test the pairing edge cases, document the frozen contract`, 3 files, +79 -25. ### Actioned - **Block E deleted.** Confirmed independently: it built `{ deletes = d, inserts = i }` versus `{ inserts = i, deletes = d }`, which are the same table — named keys in a Lua constructor carry no order, and `cache.lua` reads them by name. There was no array order to vary; it was block A with different strings. Block D remains the real keyed-not-positional guard. - **Three edge-case tests added** in its place: two deletes + one insert (guards the `paired[row[pk]] == row` identity comparison), one delete + two inserts (guards pair consumption, and pins the trailing-insert behaviour as intentional), and a delete with an absent pk. The last one was made **deliberately stronger** than suggested: with a delete-only batch `paired` is nil and Lua short-circuits before `row[def.pk]` is read, so the nil-key path would never be exercised — it now sits alongside a real pair so the `paired[nil]` read actually happens. - **Atomicity claimed deliberately** rather than hand-waved, plus a test: a batch of `deletes=[valid]` + `inserts=["not json"]` raises and leaves the store untouched. Old code applied the delete first. The corrected PR-body wording is below. - **Event ordering written down** in the README — it is frozen contract now. - Dangling "The conformance test proves this" sentence re-anchored; unchanged-row-reports-as-`update` documented; duplicate-delete `old` semantics recorded; both perf nits taken (`local EMPTY = {}`, dead `or nil` dropped). - **#35 caveat added** to the README's `update` row: not yet emitted for tables keyed by an `Identity` column. ### Falsifiability, verified by mutation Replaced the identity comparison with the natural-looking `if not (paired and paired[row[def.pk]]) then`: ``` FAIL two deletes and one insert of a key are two events: expected 2, got 1 — 4 passed, 1 failed ``` Restored: `5 passed, 0 failed`. ### Corrected claim for finding 5 > [object Object] ### Why this still needs another pass Not because of anything in this commit. **Finding 4 is unresolved and cannot be resolved on this branch**: nothing in the repo demonstrates that a real server packs a pk change into a single `updates` entry. `dev/server/index.js` has only `sendMessage` (insert) and `clearAll` (delete) — there is no update reducer anywhere, and every batch in `session.jsonl` is deletes-only or inserts-only. The premise this PR rests on was inherited from a pre-existing comment, not from an observation. If the server splits a pk change across two `updates` entries, consumers get `delete` + `insert`, this feature is inert, and no test fails. At an API freeze that is the difference between a feature and a plausible feature. `dev/server/`, `dev/test/fixtures/` and `dev/tools/` are owned by `feat/25-codegen-types` (PR #36) right now, so this is sequenced: once #36 merges, add an `editMessage` reducer, assert exactly one `update` in `smoke.script`, recapture `session.jsonl`, and confirm the delete and insert arrive in one entry. **Holding this PR until that is proven.**
yuris force-pushed feat/21-update-events from 9358d0267c
All checks were successful
ci / ci (pull_request) Successful in 19s
to 783015f5d0
All checks were successful
ci / ci (pull_request) Successful in 20s
2026-08-19 17:20:20 +00:00
Compare
Author
Owner

Blocker cleared — the premise is ONE ENTRY, observed, not inferred

783015f, rebased on a1e716d.

A primary-key change arrives as a delete plus an insert of the same key packed into a single updates entry. The feature is real, not plausible.

New edit_message reducer published to the live 2.8.2 server, driven over a real v1.json.spacetimedb socket (clear_all → Subscribe → send_message → edit_message), one subscription:

"tables":[{"table_id":4096,"table_name":"message","num_rows":2,
  "updates":[{"deletes":["[2,\"probe\",\"before edit\"]"],
              "inserts":["[2,\"probe\",\"after edit\"]"]}]}]

tables.length = 1, updates.length = 1, that entry carrying deletes=1 and inserts=1.

The two-entry case, and why it confirms rather than contradicts

The recaptured fixture's frame 24 has updates.length = 2:

"updates":[{"deletes":["[5,\"editor\",\"before edit\"]"],"inserts":["[5,\"editor\",\"after edit\"]"]},
           {"deletes":["[5,\"editor\",\"before edit\"]"],"inserts":["[5,\"editor\",\"after edit\"]"]}]

That is not the delete and insert being split. Each entry independently carries both halves. Two live subscriptions match at that point in the session — the legacy Subscribe from step 2, never replaced, plus query set 200 — and updates[] is one QueryUpdate per matching query. Frame 23, a plain insert at the same point, likewise has two entries each with one insert. So the duplication is per-subscription fan-out, identical to pre-existing insert behaviour, and not introduced here.

This settles the correctness question twice:

  1. Sufficient — no pair ever spans two entries, so per-entry pairing catches every change.
  2. Also the only correct scope — the two entries in frame 24 belong to two independent subscriptions, so pairing across entries would have fabricated an update from unrelated queries. That is exactly the hazard flagged in review. cache.lua was not widened and was not touched at all.

Consumer consequence, now documented

A row matched by two live subscriptions reports two update events — exactly as it already reports two inserts. Written up in the README next to the events table.

Changes

  • dev/server/index.js — editMessage(id, text) via ctx.db.message.id.find + .update, in the existing reducers' style. The message declaration and every pre-existing reducer/procedure untouched; profile/claim_profile untouched.
  • dev/server/capture-fixtures.mjs — three steps appended at the end, so frames 1–21 keep their indices, plus a last_inserted_id() helper that reads the pk out of the preceding insert frame rather than predicting the autoinc counter.
  • session.jsonl recaptured, 21 → 24 frames. chat_schema.lua regenerated (+4 lines, the edit_message entry).
  • test_fixtures.lua — frame count and tags updated; new replay section asserting a single entry yields exactly one update carrying old.text="before edit" / row.text="after edit", zero stray delete/insert events, counts.inserts=1 / counts.deletes=1 still counted, one row upserted in place, and that the full frame yields one update per matching query.

Verification

  • 5 passed, 0 failed; stylua clean
  • Native conformance PASS both runs, first try, no #39 flake. Phase 1 printed PHASE1 OK light=true, own update still full; pk change is one update event; cached row id=1
  • Phase 2 confirmed the same pk survived the restart in each run

Also corrected the dev/server/index.js frozen-table header per #32's finding: rows are decoded by column order where the wire is positional — TransactionUpdate, on both the insert and delete side — not everywhere.

## Blocker cleared — the premise is **ONE ENTRY**, observed, not inferred `783015f`, rebased on `a1e716d`. A primary-key change arrives as a delete plus an insert of the same key packed into a **single `updates` entry**. The feature is real, not plausible. New `edit_message` reducer published to the live 2.8.2 server, driven over a real `v1.json.spacetimedb` socket (`clear_all` → `Subscribe` → `send_message` → `edit_message`), one subscription: ```json "tables":[{"table_id":4096,"table_name":"message","num_rows":2, "updates":[{"deletes":["[2,\"probe\",\"before edit\"]"], "inserts":["[2,\"probe\",\"after edit\"]"]}]}] ``` `tables.length = 1`, `updates.length = 1`, that entry carrying `deletes=1` and `inserts=1`. ### The two-entry case, and why it confirms rather than contradicts The recaptured fixture's frame 24 has `updates.length = 2`: ```json "updates":[{"deletes":["[5,\"editor\",\"before edit\"]"],"inserts":["[5,\"editor\",\"after edit\"]"]}, {"deletes":["[5,\"editor\",\"before edit\"]"],"inserts":["[5,\"editor\",\"after edit\"]"]}] ``` That is **not** the delete and insert being split. Each entry independently carries *both* halves. Two live subscriptions match at that point in the session — the legacy `Subscribe` from step 2, never replaced, plus query set 200 — and `updates[]` is one QueryUpdate per matching query. Frame 23, a plain insert at the same point, likewise has two entries each with one insert. So the duplication is per-subscription fan-out, identical to pre-existing insert behaviour, and not introduced here. This settles the correctness question **twice**: 1. **Sufficient** — no pair ever spans two entries, so per-entry pairing catches every change. 2. **Also the only correct scope** — the two entries in frame 24 belong to two *independent* subscriptions, so pairing across entries would have fabricated an `update` from unrelated queries. That is exactly the hazard flagged in review. `cache.lua` was not widened and was not touched at all. ### Consumer consequence, now documented A row matched by two live subscriptions reports two `update` events — exactly as it already reports two `insert`s. Written up in the README next to the events table. ### Changes - `dev/server/index.js` — `editMessage(id, text)` via `ctx.db.message.id.find` + `.update`, in the existing reducers' style. The `message` declaration and every pre-existing reducer/procedure untouched; `profile`/`claim_profile` untouched. - `dev/server/capture-fixtures.mjs` — three steps **appended at the end**, so frames 1–21 keep their indices, plus a `last_inserted_id()` helper that reads the pk out of the preceding insert frame rather than predicting the autoinc counter. - `session.jsonl` recaptured, 21 → 24 frames. `chat_schema.lua` regenerated (+4 lines, the `edit_message` entry). - `test_fixtures.lua` — frame count and tags updated; new replay section asserting a single entry yields exactly one `update` carrying `old.text="before edit"` / `row.text="after edit"`, zero stray delete/insert events, `counts.inserts=1` / `counts.deletes=1` still counted, one row upserted in place, and that the full frame yields one update per matching query. ### Verification - `5 passed, 0 failed`; stylua clean - **Native conformance PASS both runs, first try, no #39 flake.** Phase 1 printed `PHASE1 OK light=true, own update still full; pk change is one update event; cached row id=1` - Phase 2 confirmed the same pk survived the restart in each run Also corrected the `dev/server/index.js` frozen-table header per #32's finding: rows are decoded by column order where the wire is positional — `TransactionUpdate`, on both the insert and delete side — not everywhere.
Author
Owner

Second-pass review — automated reviewer, posted for the record

Second-pass review: PR #34 — cache: report a pk row change as one update event

Reviewed at 783015f, rebased on a1e716d. Every number below is from a run I did on an isolated server (STDB_PORT=3100, STDB_STATE_DIR=/tmp/spacetimedb-review34b), not from the author's report.

Verdict: MERGE. The premise reproduces exactly. Two doc-accuracy should-fixes, neither blocking. Two follow-up issues, neither this PR's.


The premise: independently re-captured, CONFIRMED

Published the branch's dev/server/index.js to a live 2.8.2, drove a v1.json.spacetimedb socket myself (clear_all -> legacy Subscribe -> send_message -> edit_message), one subscription:

tables=1
table message num_rows=2 updates=1
  entry 0: deletes=1 ["[3,\"probe\",\"before edit\"]"] | inserts=1 ["[3,\"probe\",\"after edit\"]"]

tables.length == 1, updates.length == 1, that one entry carrying both halves of the same key. The feature is real. The blocker is genuinely cleared.

The edges the author did not claim to have tested

A throwaway probe module with edit_all, rekey and swap reducers, each scenario on a fresh socket. Wire shapes, then the same shapes fed through cache.lua on the branch and on origin/main as a control.

probe wire branch cache.lua origin/main control
S3 three rows changed in ONE reducer call one entry, deletes=3 inserts=3 3 x update 3 delete + 3 insert
S4 primary KEY VALUE change (9 -> 9001) one entry, d1i1, keys differ delete(9) + insert(9001) identical
S5 delete one row, insert a different row one entry, d1i1, keys differ delete(10) + insert(11) identical
S2b two matching subscriptions 2 entries, each a full pair 2 x update, store 1 row 4 events, store 1 row
  • S3: all pairs land in one entry, so the keyed map handles them together. Correct.
  • S4 is the case you asked about, and the answer is the right one. A pk-value change is one entry whose delete and insert carry different keys, so pair_by_key declines and the SDK reports delete + insert. It does not fabricate a misleading update, and it is byte-identical to origin/main. Pairing-by-key fails safe here.
  • S5: same, no pairing. Correct.

The fan-out question — and a correction to the author's stated rule

The author says two subscriptions produce two entries each carrying both halves. That is true of the captured frame but not because of "two subscriptions". Measured:

configuration updates entries
two SubscribeMulti sets, identical query text 1
legacy Subscribe + SubscribeMulti, same query text 2 <- this is what fixture frame 24 actually is
one SubscribeMulti carrying two distinct matching queries 2
three distinct matching queries 3

The identical-query dedup is not an artifact of a dead subscription — I proved both sets live: dropping query set 500 deleted nothing (501 still covered the row), then dropping 501 deleted it (d1i0). Two genuinely live subscriptions, one entry, one update event.

So fan-out is per distinct query text, with the legacy Subscribe set counted separately from SubscribeMulti sets. Frame 24's two entries come from that legacy/multi pairing, exactly as test_fixtures.lua:340 guesses — the frame reading is right, the general rule stated around it is not.

Should it be deduplicated before freezing? No — freeze it

  1. It isn't new. Fixture frame 23 is a plain send_message and it has two entries too, so insert already fans out identically. Deduplicating update alone would make it the one event type that behaves differently.
  2. Dedup requires cross-entry logic, and T4 proves that is unsafe. With two distinct queries where the edit makes the row leave one of them, the wire is entry 0 = the full pair, entry 1 = a bare delete of the same key (d1i0). Any cross-entry join must guess whether that delete pairs with entry 0's insert; either guess fabricates or destroys information. cache.lua not being widened is load-bearing, and now there is a captured shape proving it.
  3. counts.inserts/counts.deletes count row operations; dedup would desync events from counts.
  4. Applying the same update twice is a store no-op (verified: S2b leaves exactly one row), so an idempotent consumer — which a cache consumer must be regardless — is unaffected.

The new work

1. dev/server/index.js — clean. git diff a1e716d HEAD is exactly two hunks: the header-comment reword, and a pure insertion of editMessage after sendMessage. The message and profile table declarations, sendMessage, claimProfile, clearAll, countMessages, echo and alwaysFails are byte-identical. profile/claim_profile untouched, as claimed.

2. capture-fixtures.mjs — frame indices did not move. Structural signature (tag, reducer name, request_id, query_id, row lists) of frames 1-21 against a1e716d's fixture: identical for 20 of 21. The single difference is frame 20's embedded stack-trace line number (spacetimedb_module:8933 -> :8998), an unavoidable consequence of the module gaining a reducer; test_fixtures.lua:312-316 asserts only :find("procedure failed on purpose"), so nothing depends on it.

last_inserted_id() under fan-out: sound. Fan-out duplicates the same row across entries, so updates[0].inserts[0] is always the right row — and this is not theoretical, it is the real capture: frame 23 has two entries, both [5,"editor","before edit"], and the helper produced the correct id 5. Its other assumptions (tables[0] is message; the previous frame is a committed TransactionUpdate) fail with a loud TypeError rather than silently capturing the wrong shape, which is the stated design goal. Good enough.

3. session.jsonl — recapture is stable. I captured twice on my own server, once against a freshly published db and once against a dirty one, and compared both to the checked-in file. All three: 24 frames, byte-identical tag sequence, identical structural shape for frames 22-24 including updates=2 on both 23 and 24. Not order-dependent. Only the autoinc ids drift when the db is not freshly published (5 vs 10), and no assertion hardcodes them.

Bonus: regenerating chat_schema.lua from my published copy of the branch module reproduced the checked-in file byte-for-byte apart from the server URL, so the committed schema really does match the branch module, and #36's profile/typespace content survived the rebase intact.

4. The new replay assertions do depend on the pairing. With cache.lua reverted to origin/main:

FAIL dev/test/test_cache.lua:    FAIL pk delete+insert pair is one event: expected 1, got 2
FAIL dev/test/test_fixtures.lua: FAIL a pk change is exactly one event: expected 1, got 2
3 passed, 2 failed

Re-running the new block with non-fatal asserts to classify every line: 7 of 17 are hard guards (#events == 1, event type, old.text, row.text, same-key, #events == #updates, all-updates). The other 10 are fixture-shape assertions and must-not-change invariants (counts.deletes/inserts, one row upserted) — correct to keep, they defend against an over-eager future implementation.

5. Conformance — PASS, and the negative control fires. Full run on the branch: PASS both runs, first try, no #39 flake, phase 1 printing PHASE1 OK light=true, own update still full; pk change is one update event; cached row id=1. Phase 2 confirmed the same pk survived the restart.

The guard is real. Same run with cache.lua reverted to origin/main:

SMOKE: FAIL a pk change produced 2 event(s), first of type delete - expected exactly one update
[conformance] FAIL - phase 1 never completed, server was never restarted

Phase 1 uses exactly one subscription, so #events == 1 is a stable assertion, not an accident.

Rebase check

Base side (a1e716d): nothing dropped. Two-hunk index.js diff; chat_schema.lua +4 lines only, with #36's profile/typespace intact and byte-identical to a fresh regen; session.jsonl frames 1-21 structurally intact; README purely additive plus one reworded table cell and the re-anchored conformance sentence.

Branch side: could not diff directly. The pre-rebase commits were force-pushed away — git fetch origin 51ebf3c and 9358d02 both return couldn't find remote ref, and neither is reachable from any ref on the server. So I verified the previous round's work by content instead, and all of it is present: block E gone (tbl_deletes_first absent repo-wide), the three edge-case tests present, the atomicity test present, the README event-ordering paragraph present, local EMPTY = {} present, the dead or nil gone from the pairing line, the key ~= nil rationale commented, and the #35 caveat present (issue #35 exists and is open). Nothing from round one was lost.

Merge-order note: feat/32-column-order (#32) is not merged and independently rewrites capture-fixtures.mjs's header comment and chat_schema.lua's generated header, and reworks test_fixtures.lua. #34's third comment says it "corrected the index.js frozen-table header per #32's finding" — that is in index.js, which #32 does not touch, so no conflict there. But both branches touch capture-fixtures.mjs (different regions) and test_fixtures.lua, so whichever lands second wants a look.


Should-fix (docs only, not blocking)

1. README.md:296-297 overstates the fan-out trigger. "a row matched by two live subscriptions reports two update events" is false as written — two live subscriptions with the same query text produce one entry and one update. The "exactly as it already reports two inserts" symmetry IS correct. Suggested replacement:

The consequence to know is that a row can report more than one update for a single change: updates[] carries one QueryUpdate per distinct query the row matches, and the legacy Subscribe set counts separately from SubscribeMulti sets. That is exactly how insert and delete already behave, so a consumer that was already idempotent needs no change.

Same imprecision in test_fixtures.lua:342's label ("one QueryUpdate per matching subscription") and its comment at 335-341. The assertion itself is fine — it reads a checked-in fixture and cannot fail spuriously — but the label teaches the wrong rule. Note this errs in the safe direction: it warns consumers about more duplication than actually occurs, so no consumer written against it is wrong.

2. The PR body is stale. It still says "A, D, E fail when reverted" (block E was deleted in the actioned round) and "Conformance not run (needs a live server)" (conformance is now the headline evidence and passes). The third comment supersedes both, but the body is what a squash-merge carries into history.

Follow-up issues, not this PR

1. A pk change that makes a row leave one of several matching queries drops it from the cache. Wire (two distinct queries, edit makes the row stop matching one): entry 0 = full pair, entry 1 = bare delete of the same key. Through the cache: update then delete, final store empty — though the row still exists and still matches the other live query. origin/main produces delete, insert, delete and the same empty store, so this is pre-existing and #34 neither causes nor worsens it; it is the documented overlapping-subscription hazard ("clients would need to track which rows exist in which queries") showing up on the transaction path rather than the unsubscribe path. #34 only changes the reported events.

2. Optional one-clause README addition for the pk-VALUE change. The behaviour is correct — delete + insert, no fabricated update — but it is the one case where pairing-by-key deliberately declines, and a frozen contract may as well say so.


MERGE. The premise is observed, not inferred, and I reproduced it. The unit assertions, the fixture replay and the conformance phase-1 assertion all go red when the pairing is removed, so the feature is guarded at three levels including end to end through the engine's own socket. The four unclaimed edges all behave correctly, and the one that looked dangerous going in — a pk-value change, where pairing by key cannot work — fails safe and reports delete + insert. The fan-out behaviour is acceptable to freeze and should not be deduplicated. Nothing found requires a code change; the two should-fixes are prose.


Verdict: MERGE

## Second-pass review — automated reviewer, posted for the record # Second-pass review: PR #34 — `cache: report a pk row change as one update event` Reviewed at `783015f`, rebased on `a1e716d`. Every number below is from a run I did on an isolated server (`STDB_PORT=3100`, `STDB_STATE_DIR=/tmp/spacetimedb-review34b`), not from the author's report. **Verdict: MERGE.** The premise reproduces exactly. Two doc-accuracy should-fixes, neither blocking. Two follow-up issues, neither this PR's. --- ## The premise: independently re-captured, CONFIRMED Published the branch's `dev/server/index.js` to a live 2.8.2, drove a `v1.json.spacetimedb` socket myself (`clear_all` -> legacy `Subscribe` -> `send_message` -> `edit_message`), one subscription: ``` tables=1 table message num_rows=2 updates=1 entry 0: deletes=1 ["[3,\"probe\",\"before edit\"]"] | inserts=1 ["[3,\"probe\",\"after edit\"]"] ``` `tables.length == 1`, `updates.length == 1`, that one entry carrying both halves of the same key. The feature is real. The blocker is genuinely cleared. ## The edges the author did not claim to have tested A throwaway `probe` module with `edit_all`, `rekey` and `swap` reducers, each scenario on a fresh socket. Wire shapes, then the same shapes fed through `cache.lua` on the branch and on `origin/main` as a control. | probe | wire | branch `cache.lua` | `origin/main` control | |---|---|---|---| | S3 three rows changed in ONE reducer call | **one** entry, `deletes=3 inserts=3` | 3 x `update` | 3 `delete` + 3 `insert` | | S4 primary **KEY VALUE** change (9 -> 9001) | one entry, `d1i1`, keys differ | `delete(9)` + `insert(9001)` | identical | | S5 delete one row, insert a different row | one entry, `d1i1`, keys differ | `delete(10)` + `insert(11)` | identical | | S2b two matching subscriptions | 2 entries, each a full pair | 2 x `update`, store 1 row | 4 events, store 1 row | - **S3**: all pairs land in one entry, so the keyed map handles them together. Correct. - **S4 is the case you asked about, and the answer is the right one.** A pk-value change is one entry whose delete and insert carry *different* keys, so `pair_by_key` declines and the SDK reports `delete` + `insert`. It does **not** fabricate a misleading `update`, and it is byte-identical to `origin/main`. Pairing-by-key fails safe here. - **S5**: same, no pairing. Correct. ### The fan-out question — and a correction to the author's stated rule The author says two subscriptions produce two entries each carrying both halves. That is true of the captured frame but **not** because of "two subscriptions". Measured: | configuration | `updates` entries | |---|---| | two `SubscribeMulti` sets, **identical** query text | **1** | | legacy `Subscribe` + `SubscribeMulti`, same query text | **2** <- this is what fixture frame 24 actually is | | one `SubscribeMulti` carrying **two distinct** matching queries | 2 | | three distinct matching queries | 3 | The identical-query dedup is not an artifact of a dead subscription — I proved both sets live: dropping query set 500 deleted nothing (501 still covered the row), then dropping 501 deleted it (`d1i0`). Two genuinely live subscriptions, one entry, one `update` event. So fan-out is per **distinct query text**, with the legacy `Subscribe` set counted separately from `SubscribeMulti` sets. Frame 24's two entries come from that legacy/multi pairing, exactly as `test_fixtures.lua:340` guesses — the frame reading is right, the general rule stated around it is not. ### Should it be deduplicated before freezing? No — freeze it 1. **It isn't new.** Fixture frame 23 is a plain `send_message` and it has two entries too, so `insert` already fans out identically. Deduplicating `update` alone would make it the one event type that behaves differently. 2. **Dedup requires cross-entry logic, and T4 proves that is unsafe.** With two distinct queries where the edit makes the row leave one of them, the wire is entry 0 = the full pair, entry 1 = a **bare delete of the same key** (`d1i0`). Any cross-entry join must guess whether that delete pairs with entry 0's insert; either guess fabricates or destroys information. `cache.lua` not being widened is load-bearing, and now there is a captured shape proving it. 3. `counts.inserts`/`counts.deletes` count row operations; dedup would desync events from counts. 4. Applying the same `update` twice is a store no-op (verified: S2b leaves exactly one row), so an idempotent consumer — which a cache consumer must be regardless — is unaffected. ## The new work **1. `dev/server/index.js` — clean.** `git diff a1e716d HEAD` is exactly **two hunks**: the header-comment reword, and a pure insertion of `editMessage` after `sendMessage`. The `message` and `profile` table declarations, `sendMessage`, `claimProfile`, `clearAll`, `countMessages`, `echo` and `alwaysFails` are byte-identical. `profile`/`claim_profile` untouched, as claimed. **2. `capture-fixtures.mjs` — frame indices did not move.** Structural signature (tag, reducer name, request_id, query_id, row lists) of frames 1-21 against `a1e716d`'s fixture: identical for 20 of 21. The single difference is frame 20's embedded stack-trace line number (`spacetimedb_module:8933` -> `:8998`), an unavoidable consequence of the module gaining a reducer; `test_fixtures.lua:312-316` asserts only `:find("procedure failed on purpose")`, so nothing depends on it. `last_inserted_id()` under fan-out: **sound.** Fan-out duplicates the same row across entries, so `updates[0].inserts[0]` is always the right row — and this is not theoretical, it is the real capture: frame 23 has two entries, both `[5,"editor","before edit"]`, and the helper produced the correct id 5. Its other assumptions (`tables[0]` is `message`; the previous frame is a committed `TransactionUpdate`) fail with a loud `TypeError` rather than silently capturing the wrong shape, which is the stated design goal. Good enough. **3. `session.jsonl` — recapture is stable.** I captured twice on my own server, once against a freshly published db and once against a dirty one, and compared both to the checked-in file. All three: **24 frames, byte-identical tag sequence, identical structural shape for frames 22-24** including `updates=2` on both 23 and 24. Not order-dependent. Only the autoinc ids drift when the db is not freshly published (5 vs 10), and no assertion hardcodes them. Bonus: regenerating `chat_schema.lua` from my published copy of the branch module reproduced the checked-in file **byte-for-byte apart from the server URL**, so the committed schema really does match the branch module, and `#36`'s `profile`/`typespace` content survived the rebase intact. **4. The new replay assertions do depend on the pairing.** With `cache.lua` reverted to `origin/main`: ``` FAIL dev/test/test_cache.lua: FAIL pk delete+insert pair is one event: expected 1, got 2 FAIL dev/test/test_fixtures.lua: FAIL a pk change is exactly one event: expected 1, got 2 3 passed, 2 failed ``` Re-running the new block with non-fatal asserts to classify every line: **7 of 17 are hard guards** (`#events == 1`, event type, `old.text`, `row.text`, same-key, `#events == #updates`, all-updates). The other 10 are fixture-shape assertions and must-not-change invariants (`counts.deletes/inserts`, one row upserted) — correct to keep, they defend against an over-eager future implementation. **5. Conformance — PASS, and the negative control fires.** Full run on the branch: PASS both runs, first try, no #39 flake, phase 1 printing `PHASE1 OK light=true, own update still full; pk change is one update event; cached row id=1`. Phase 2 confirmed the same pk survived the restart. The guard is real. Same run with `cache.lua` reverted to `origin/main`: ``` SMOKE: FAIL a pk change produced 2 event(s), first of type delete - expected exactly one update [conformance] FAIL - phase 1 never completed, server was never restarted ``` Phase 1 uses exactly one subscription, so `#events == 1` is a stable assertion, not an accident. ## Rebase check **Base side (`a1e716d`): nothing dropped.** Two-hunk `index.js` diff; `chat_schema.lua` +4 lines only, with #36's `profile`/`typespace` intact and byte-identical to a fresh regen; `session.jsonl` frames 1-21 structurally intact; README purely additive plus one reworded table cell and the re-anchored conformance sentence. **Branch side: could not diff directly.** The pre-rebase commits were force-pushed away — `git fetch origin 51ebf3c` and `9358d02` both return `couldn't find remote ref`, and neither is reachable from any ref on the server. So I verified the previous round's work by content instead, and all of it is present: block E gone (`tbl_deletes_first` absent repo-wide), the three edge-case tests present, the atomicity test present, the README event-ordering paragraph present, `local EMPTY = {}` present, the dead `or nil` gone from the pairing line, the `key ~= nil` rationale commented, and the #35 caveat present (issue #35 exists and is open). Nothing from round one was lost. **Merge-order note:** `feat/32-column-order` (#32) is not merged and independently rewrites `capture-fixtures.mjs`'s header comment and `chat_schema.lua`'s generated header, and reworks `test_fixtures.lua`. #34's third comment says it "corrected the `index.js` frozen-table header per #32's finding" — that is in `index.js`, which #32 does not touch, so no conflict there. But both branches touch `capture-fixtures.mjs` (different regions) and `test_fixtures.lua`, so whichever lands second wants a look. --- ## Should-fix (docs only, not blocking) **1. `README.md:296-297` overstates the fan-out trigger.** "a row matched by *two* live subscriptions reports *two* `update` events" is false as written — two live subscriptions with the same query text produce one entry and one `update`. The "exactly as it already reports two `insert`s" symmetry IS correct. Suggested replacement: > The consequence to know is that a row can report more than one `update` for a single change: `updates[]` carries one QueryUpdate per *distinct* query the row matches, and the legacy `Subscribe` set counts separately from `SubscribeMulti` sets. That is exactly how `insert` and `delete` already behave, so a consumer that was already idempotent needs no change. Same imprecision in `test_fixtures.lua:342`'s label ("one QueryUpdate per matching subscription") and its comment at 335-341. The assertion itself is fine — it reads a checked-in fixture and cannot fail spuriously — but the label teaches the wrong rule. Note this errs in the safe direction: it warns consumers about more duplication than actually occurs, so no consumer written against it is wrong. **2. The PR body is stale.** It still says "**A, D, E fail when reverted**" (block E was deleted in the actioned round) and "Conformance not run (needs a live server)" (conformance is now the headline evidence and passes). The third comment supersedes both, but the body is what a squash-merge carries into history. ## Follow-up issues, not this PR **1. A pk change that makes a row leave one of several matching queries drops it from the cache.** Wire (two distinct queries, edit makes the row stop matching one): entry 0 = full pair, entry 1 = bare `delete` of the same key. Through the cache: `update` then `delete`, final store **empty** — though the row still exists and still matches the other live query. `origin/main` produces `delete, insert, delete` and the **same empty store**, so this is pre-existing and #34 neither causes nor worsens it; it is the documented overlapping-subscription hazard ("clients would need to track which rows exist in which queries") showing up on the transaction path rather than the unsubscribe path. #34 only changes the reported events. **2. Optional one-clause README addition for the pk-VALUE change.** The behaviour is correct — `delete` + `insert`, no fabricated `update` — but it is the one case where pairing-by-key deliberately declines, and a frozen contract may as well say so. --- **MERGE.** The premise is observed, not inferred, and I reproduced it. The unit assertions, the fixture replay and the conformance phase-1 assertion all go red when the pairing is removed, so the feature is guarded at three levels including end to end through the engine's own socket. The four unclaimed edges all behave correctly, and the one that looked dangerous going in — a pk-value change, where pairing by key cannot work — fails safe and reports `delete` + `insert`. The fan-out behaviour is acceptable to freeze and should not be deduplicated. Nothing found requires a code change; the two should-fixes are prose. --- **Verdict:** MERGE
yuris force-pushed feat/21-update-events from 783015f5d0
All checks were successful
ci / ci (pull_request) Successful in 20s
to 4cdf1e9e6f
All checks were successful
ci / ci (pull_request) Successful in 1m28s
2026-08-19 17:40:33 +00:00
Compare
yuris merged commit 70a5a00291 into main 2026-08-19 17:42:14 +00:00
Sign in to join this conversation.
No description provided.