cache: count how many queries hold a row, so an overlapping one survives #51

Merged

yuris merged 1 commit from feat/43-overlap into main 2026-08-20 07:15:04 +00:00

Conversation 1 Commits 1 Files changed 10 +847 -67

yuris commented 2026-08-20 05:23:04 +00:00

Owner

Copy link

Closes #43 — and corrects two of the issue's own premises.

With two live subscriptions matching a row by distinct queries, a change that makes the row stop matching one of them evicted it from the cache entirely, even though it still exists on the server and still matches the other query. Nothing errored. Pre-existing, reproduced against main before this push.

The cache now counts how many queries hold each primary key (state.refs), removes a row only at the last hold, and suppresses the delete event exactly when the row survives.

Premise correction 1 — the association does not exist on an updates entry

#43 said to thread query_id into apply_database_update. That is impossible on the protocol this SDK speaks: upstream crates/client-api-messages/src/websocket/v1.rs at v2.8.2 is QueryUpdate { deletes, inserts }, with TableUpdate.updates a bare list — no per-entry query identifier. Positional association is unusable too, because the legacy Subscribe set counts apart while two identical SubscribeMulti texts collapse to one entry.

Message level identity does exist — SubscribeApplied, SubscribeMultiApplied, Unsubscribe*Applied and SubscriptionError all carry a query_id, so a whole DatabaseUpdate on the subscribe/unsubscribe path can be attributed to one set. What is missing is per-ENTRY attribution inside a TransactionUpdate, which is exactly the #43 failure path.

The v2 BSATN wire does carry it (TransactionUpdate { query_sets: [{ query_set_id, tables }] }), which is how the official SDKs could attribute a delta — and they still only count, discarding the id (Rust RowEntry.ref_count, TypeScript Map<pk,[row,n]>).

So a plain multiset hold count needs no identity, and apply_database_update's frozen signature is untouched while cache.lua stays pure Lua.

Premise correction 2 — the bare delete is entry 0, not entry 1

Captured live, the ordering in the issue is backwards. And the old cache's failure was one-sided, not two symmetric ones:

entry order	old behaviour
bare, pair	count=1, row = v1, the correct current value — but a spurious delete event fired beside the update, for a row cache.get still answers for
pair, bare	count=0, row gone — remove nils the key regardless of value and drops the v1 the pair just wrote

So in bare-then-pair order main is accidentally right about the data and wrong only about the event; the data loss needs pair-then-bare. Live 2.8.2 emits bare-first in every shape reproducible, which is why the loss shows up one frame later instead — at the unsubscribe of one of N (see below). Nothing on the wire promises the order (SmallVec, server-internal), so both orders are asserted and the fix is arithmetic rather than positional.

Server behaviour, verified on an isolated server

• A second distinct-text query set matching a row the client already holds re-sends it as a bare insert → hold count rises to 2.
• Dropping one of two distinct overlapping sets sends that set's delete → back to 1, row stays. On main this is where the row is lost outright, with no dependence on entry order.
• Dropping a set that matches nothing still names the table, carrying an entry with neither deletes nor inserts.
• Two sets with identical query text share one query, so the row arrives once and the first drop deletes nothing — session.jsonl frames 14–17. Same rule, because the count is of queries, not subscriptions.
• Legacy Subscribe called twice re-sends every matching row as a bare insert with no deletes, and narrowing the query sends no delete for rows that left.

The captured fixture (#43's first work item)

session.jsonl gains an overlap block (frames 43–50, a fourth socket) via capture-fixtures.mjs's existing BLOCKS append. It is the shape the entire design rests on and it existed nowhere in the repo — only in hand-built frames and transient captures, and smoke.script PHASE3 guards the unsubscribe release path rather than this framing. So nothing committed would have noticed if 2.8.2's per-query delta framing changed.

Two distinct-text queries (sender = 'probe43', text = 'alpha') over one row, then an edit that takes it out of one of them. Frame 48, from the live server:

message  e0: -[[1,"probe43","alpha"]] +[]                          <- BARE DELETE
         e1: -[[1,"probe43","alpha"]] +[[1,"probe43","beta"]]      <- PAIR

Frame 47 is the other half nothing committed had: a distinct-text set re-sending a row the client already holds, which is the arrival a hold count counts. Frames 14–17 cannot show it — identical text is one query.

Appended, not recaptured: frames 1–42 are byte-identical (cmp clean), so nothing pinned above needed re-verifying, and chat_schema.lua re-derives clean under conformance.sh's drift gate. test_fixtures.lua asserts frame 48 on the raw bytes, then replays those two entries both ways and requires convergence — same store, same value, same single update event — and follows each order through frames 49 and 50 so the surviving hold is proved by what the socket did next rather than by reading refs.

Not vacuous: swapping main's cache.lua under this test fails at captured order, bare delete first: exactly one event: expected 1, got 2, and with the orders reversed at reversed order, pair first: the row survives: expected 1, got 0.

The four interactions

• #21's update pairing — untouched, still per updates entry; paired-update events fire independently of the count. The single-query case is asserted byte-identical in events, counts and store.
• #28's unique indexes — the surviving branch never calls remove, so a decremented-but-kept row keeps its index entries and a truly removed row loses them. Covered by a hand-written case plus a seeded randomised property test asserting both directions — the same technique that caught a blocker in #44.
• #35's typed decode / #42's undescribed skip — not on this path, unchanged.
• Entries ≠ subscriptions — the implementation counts arrivals and never subscriptions, so it is immune by construction.

The legacy Subscribe path — a REGRESSION, now reported rather than silent

Correcting the earlier claim in this description: this is not wholly a pre-existing limitation. It is two consequences, and only one of them predates the count.

Pre-existing: narrowing a legacy Subscribe sends no delete for the row that left, so a stale row survives. Reproduced identically on main and here; no refcount needed to make it appear.

New here: calling subscribe twice with the same text re-sends every matching row as a bare insert with no deletes, so the count reaches 2, and the row's real deletion then only decrements it. Replayed through both caches on live captured frames:

	main	this branch (before the fix below)
after clear_all really deletes the row	store {}, delete event fired — correct	store {4097:1}, no event
afterwards	—	a phantom row served forever; no second delete ever comes and on_delete never fires

main was correct on that exact sequence. A silent wrong answer out of a method frozen at 1.0 is the same class of failure #43 itself is about, so connection.subscribe now reports it: a second legacy subscribe on one connection emits a subscription_replaced event before the frame goes out. Not a raise — the call is legal, if unwise — and it follows #42's undescribed precedent rather than adding a mechanism. resubscribe_all replaying one live set is silent (it clears subscriptions first), and a retry after a SubscriptionError is silent too, because a rejected set leaves the map.

The count still cannot recover the inflated hold; the fix remains "call subscribe once per connection, use subscribe_query for anything you may change". What changed is that the consumer is told.

Same shape documented for pk-less tables. Both in the README.

Two documentation gaps closed

• stdb/tables.lua's "things a game has to know that no signature states" list gains a fourth item: on_insert fires per arrival, on_delete only at the last release. Measured through the per-table API — one row delivered by two distinct queries then dropped by each in turn gives on_insert 2, on_delete 1 (on main: 2 and 2, with the row wrongly evicted at the first drop). So n=n+1/n=n-1 counting drifts by +1 per extra overlapping query, and unlike the reconnect drift already listed there, this one never settles.
• M.subscribe's own docstring now names the repeat-call hazard and points at the README, instead of documenting it only in the README and cache.lua's comment block.

Cost, measured against #30

Lua 5.1 via .#ci, best-of-7, both builds on one machine:

	main	this branch	delta
20k-row InitialSubscription (pk)	113.8 ms	123.2 ms	+9.4 ms (+8.3%), 5.7 → 6.2 µs/row
2000 paired-update frames on a warm 20k cache	22.3 ms	22.8 ms	+0.5 ms (+1.9%)
one frame carrying 2000 updates entries	9.4 ms	9.6 ms	+0.2 ms (+2.1%)
20k-row InitialSubscription (pk-less)	56.3 ms	50.5 ms	within noise — no holds taken

Memory: one integer per primary key, measured by dropping the holds table in-process and re-measuring — 26.2 B/key at 20k rows, 21.0 B/key at 100k (per-key cost falls because Lua sizes the hash part to a power of two). About 6% on top of what the rows already occupy. pk-less relations pay nothing.

Against #30: a 20k-row initial subscription already costs ~114 ms on main — roughly 7 dropped frames at 60 fps — so this adds ~9 ms to a figure already far over budget without changing the shape of that problem. One big initial frame is the cost; the steady-state transaction path pays ~2%.

Verification

6 passed, 0 failed; stylua clean. Native conformance PASS both runs including PHASE5, with chat_schema.lua re-derived and matched (no fixture drift). HTML5 PASS both loads including PHASE5.

Three committed tests needed reshaping, all because they replayed shapes the wire does not produce — and all keep every original assertion:

• test_cache.lua — a synthetic bare re-insert followed by one delete now asserts the new contract explicitly (two holds need two deletes).
• test_fixtures.lua — the player block replayed frame 36 before frame 35, manufacturing a two-hold state the socket never had. Now in wire order, with a comment on why order matters where it did not before.
• smoke.script's PHASE3 now asserts the row survives the query-set drop, because the legacy Subscribe still matches it — the server proves the survival is right by continuing to ship that row's deltas afterwards. The drain and delete assertions moved onto phase 5's clear_all, which gained a cache.count check the old script did not have.

Closes #43 — **and corrects two of the issue's own premises.** With two live subscriptions matching a row by distinct queries, a change that makes the row stop matching one of them evicted it from the cache entirely, even though it still exists on the server and still matches the other query. Nothing errored. Pre-existing, reproduced against `main` before this push. The cache now counts **how many queries hold each primary key** (`state.refs`), removes a row only at the last hold, and suppresses the delete event exactly when the row survives. ## Premise correction 1 — the association does *not* exist on an `updates` entry #43 said to thread `query_id` into `apply_database_update`. That is impossible on the protocol this SDK speaks: upstream `crates/client-api-messages/src/websocket/v1.rs` at v2.8.2 is `QueryUpdate { deletes, inserts }`, with `TableUpdate.updates` a bare list — **no per-entry query identifier**. Positional association is unusable too, because the legacy `Subscribe` set counts apart while two identical `SubscribeMulti` texts collapse to one entry. Message *level* identity does exist — `SubscribeApplied`, `SubscribeMultiApplied`, `Unsubscribe*Applied` and `SubscriptionError` all carry a `query_id`, so a whole DatabaseUpdate on the subscribe/unsubscribe path can be attributed to one set. What is missing is per-ENTRY attribution inside a `TransactionUpdate`, which is exactly the #43 failure path. The v2 BSATN wire *does* carry it (`TransactionUpdate { query_sets: [{ query_set_id, tables }] }`), which is how the official SDKs could attribute a delta — and they still only **count**, discarding the id (Rust `RowEntry.ref_count`, TypeScript `Map<pk,[row,n]>`). So a plain multiset hold count needs no identity, and **`apply_database_update`'s frozen signature is untouched** while `cache.lua` stays pure Lua. ## Premise correction 2 — the bare delete is entry 0, not entry 1 Captured live, the ordering in the issue is backwards. And the old cache's failure was **one-sided**, not two symmetric ones: | entry order | old behaviour | |---|---| | bare, pair | `count=1`, row = **`v1`, the correct current value** — but a spurious `delete` event fired beside the `update`, for a row `cache.get` still answers for | | pair, bare | `count=0`, row **gone** — `remove` nils the key regardless of value and drops the `v1` the pair just wrote | So in bare-then-pair order `main` is accidentally right about the *data* and wrong only about the *event*; the data loss needs pair-then-bare. Live 2.8.2 emits bare-first in every shape reproducible, which is why the loss shows up one frame later instead — at the unsubscribe of one of N (see below). Nothing on the wire promises the order (`SmallVec`, server-internal), so **both orders are asserted** and the fix is arithmetic rather than positional. ## Server behaviour, verified on an isolated server - A second **distinct-text** query set matching a row the client already holds re-sends it as a bare insert → hold count rises to 2. - Dropping one of two distinct overlapping sets sends **that set's** delete → back to 1, row stays. On `main` this is where the row is lost outright, with no dependence on entry order. - Dropping a set that matches **nothing** still names the table, carrying an entry with neither deletes nor inserts. - Two sets with **identical** query text share one query, so the row arrives once and the first drop deletes nothing — `session.jsonl` frames 14–17. Same rule, because the count is of *queries*, not subscriptions. - Legacy `Subscribe` called twice re-sends every matching row as a bare insert with **no** deletes, and narrowing the query sends no delete for rows that left. ## The captured fixture (#43's first work item) `session.jsonl` gains an **overlap block** (frames 43–50, a fourth socket) via `capture-fixtures.mjs`'s existing `BLOCKS` append. It is the shape the entire design rests on and it existed nowhere in the repo — only in hand-built frames and transient captures, and `smoke.script` PHASE3 guards the unsubscribe *release* path rather than this framing. So nothing committed would have noticed if 2.8.2's per-query delta framing changed. Two distinct-text queries (`sender = 'probe43'`, `text = 'alpha'`) over one row, then an edit that takes it out of one of them. Frame 48, from the live server: ``` message e0: -[[1,"probe43","alpha"]] +[] <- BARE DELETE e1: -[[1,"probe43","alpha"]] +[[1,"probe43","beta"]] <- PAIR ``` Frame 47 is the other half nothing committed had: a **distinct**-text set re-sending a row the client already holds, which is the arrival a hold count counts. Frames 14–17 cannot show it — identical text is one query. Appended, not recaptured: **frames 1–42 are byte-identical** (`cmp` clean), so nothing pinned above needed re-verifying, and `chat_schema.lua` re-derives clean under `conformance.sh`'s drift gate. `test_fixtures.lua` asserts frame 48 on the raw bytes, then replays those two entries **both ways** and requires convergence — same store, same value, same single `update` event — and follows each order through frames 49 and 50 so the surviving hold is proved by what the socket did next rather than by reading `refs`. Not vacuous: swapping `main`'s `cache.lua` under this test fails at `captured order, bare delete first: exactly one event: expected 1, got 2`, and with the orders reversed at `reversed order, pair first: the row survives: expected 1, got 0`. ## The four interactions - **#21's update pairing** — untouched, still per `updates` entry; paired-update events fire independently of the count. The single-query case is asserted byte-identical in events, counts *and* store. - **#28's unique indexes** — the surviving branch never calls `remove`, so a decremented-but-kept row keeps its index entries and a truly removed row loses them. Covered by a hand-written case **plus a seeded randomised property test** asserting both directions — the same technique that caught a blocker in #44. - **#35's typed decode / #42's undescribed skip** — not on this path, unchanged. - **Entries ≠ subscriptions** — the implementation counts arrivals and never subscriptions, so it is immune by construction. ## The legacy `Subscribe` path — a REGRESSION, now reported rather than silent Correcting the earlier claim in this description: this is not wholly a pre-existing limitation. It is two consequences, and only one of them predates the count. **Pre-existing:** narrowing a legacy `Subscribe` sends no delete for the row that left, so a stale row survives. Reproduced identically on `main` and here; no refcount needed to make it appear. **New here:** calling `subscribe` twice with the same text re-sends every matching row as a bare insert with no deletes, so the count reaches 2, and the row's *real* deletion then only decrements it. Replayed through both caches on live captured frames: | | `main` | this branch (before the fix below) | |---|---|---| | after `clear_all` really deletes the row | store `{}`, `delete` event fired — **correct** | store `{4097:1}`, **no event** | | afterwards | — | a phantom row served forever; no second delete ever comes and `on_delete` never fires | `main` was correct on that exact sequence. A silent wrong answer out of a method frozen at 1.0 is the same class of failure #43 itself is about, so `connection.subscribe` now **reports it**: a second legacy `subscribe` on one connection emits a `subscription_replaced` event before the frame goes out. Not a raise — the call is legal, if unwise — and it follows #42's `undescribed` precedent rather than adding a mechanism. `resubscribe_all` replaying one live set is silent (it clears `subscriptions` first), and a retry after a `SubscriptionError` is silent too, because a rejected set leaves the map. The count still cannot recover the inflated hold; the fix remains "call `subscribe` once per connection, use `subscribe_query` for anything you may change". What changed is that the consumer is told. Same shape documented for pk-less tables. Both in the README. ## Two documentation gaps closed - `stdb/tables.lua`'s "things a game has to know that no signature states" list gains a fourth item: **`on_insert` fires per arrival, `on_delete` only at the last release**. Measured through the per-table API — one row delivered by two distinct queries then dropped by each in turn gives `on_insert` 2, `on_delete` 1 (on `main`: 2 and 2, with the row wrongly evicted at the first drop). So `n=n+1`/`n=n-1` counting drifts by +1 per extra overlapping query, and unlike the reconnect drift already listed there, this one never settles. - `M.subscribe`'s own docstring now names the repeat-call hazard and points at the README, instead of documenting it only in the README and `cache.lua`'s comment block. ## Cost, measured against #30 Lua 5.1 via `.#ci`, best-of-7, both builds on one machine: | | `main` | this branch | delta | |---|---|---|---| | 20k-row `InitialSubscription` (pk) | 113.8 ms | 123.2 ms | +9.4 ms (**+8.3%**), 5.7 → 6.2 µs/row | | 2000 paired-update frames on a warm 20k cache | 22.3 ms | 22.8 ms | +0.5 ms (**+1.9%**) | | one frame carrying 2000 `updates` entries | 9.4 ms | 9.6 ms | +0.2 ms (+2.1%) | | 20k-row `InitialSubscription` (pk-less) | 56.3 ms | 50.5 ms | within noise — no holds taken | Memory: one integer per **primary key**, measured by dropping the holds table in-process and re-measuring — 26.2 B/key at 20k rows, 21.0 B/key at 100k (per-key cost falls because Lua sizes the hash part to a power of two). About **6%** on top of what the rows already occupy. pk-less relations pay nothing. Against #30: a 20k-row initial subscription already costs ~114 ms on `main` — roughly 7 dropped frames at 60 fps — so this adds ~9 ms to a figure already far over budget without changing the shape of that problem. One big initial frame is the cost; the steady-state transaction path pays ~2%. ## Verification `6 passed, 0 failed`; stylua clean. Native conformance PASS both runs including PHASE5, with `chat_schema.lua` re-derived and matched (no fixture drift). HTML5 PASS both loads including PHASE5. Three committed tests needed reshaping, all because they replayed shapes the wire does not produce — and all keep every original assertion: - `test_cache.lua` — a synthetic bare re-insert followed by one delete now asserts the new contract explicitly (two holds need two deletes). - `test_fixtures.lua` — the `player` block replayed frame 36 *before* frame 35, manufacturing a two-hold state the socket never had. Now in wire order, with a comment on why order matters where it did not before. - `smoke.script`'s PHASE3 now asserts the row **survives** the query-set drop, because the legacy `Subscribe` still matches it — the server proves the survival is right by continuing to ship that row's deltas afterwards. The drain and delete assertions moved onto phase 5's `clear_all`, which gained a `cache.count` check the old script did not have.

yuris added 1 commit 2026-08-20 05:23:04 +00:00

cache: count how many queries hold a row, so an overlapping one survives (#43) ... fa23a4ab40

`updates[]` carries one entry per DISTINCT query matching a table, and each
entry is that query's own delta. A row two queries match therefore arrives
twice and leaves twice, and a change that makes it stop matching only one of
them sends a bare delete in that query's entry beside a full delete+insert
pair in the other's. Applied straight through, that bare delete evicted a row
the server still held and the other query still matched — silently, and
recoverable only by resubscribing. The store now counts holds per primary key
and removes a row at the last one, which is what the official Rust and
TypeScript SDKs do (`RowEntry.ref_count`, `Map<pk, [row, refcount]>`).

WHY A COUNT AND NOT A QUERY SET, which is where #43's plan needed correcting.
The issue says the association is on the wire and the work is threading it into
`apply_database_update`. It is not: the v1 JSON protocol this SDK speaks carries
no query identifier on an `updates` entry at all — upstream `websocket/v1.rs` at
v2.8.2 has `QueryUpdate { deletes, inserts }` and `TableUpdate.updates` is a bare
list. The v2 BSATN wire does carry one (`TransactionUpdate { query_sets: [{
query_set_id, tables }] }`), which is how the official SDKs could attribute a
delta — and they still only count, discarding the id. Counting needs no identity,
so `apply_database_update`'s frozen signature is untouched and cache.lua stays
pure Lua with no new parameter.

The scheme rests on the server shipping one delta per query and nothing else, so
that was verified against live 2.8.2 on the v1 socket rather than assumed:

  * a second, DISTINCT-text query set matching a row the client already holds
    re-sends it as a bare insert -> the count rises to 2
  * dropping one of two distinct overlapping sets sends THAT set's delete
    -> back to 1, and the row stays
  * two sets whose query TEXT is identical share one query, so the row arrives
    once and the first drop deletes nothing (session.jsonl frames 14-17). That
    is the same rule, not an exception: the count is of QUERIES, not of
    subscriptions, and the legacy `Subscribe` set counts as one more query
    (frame 24 is that legacy/multi split — two entries for one edit).

#43'S ENTRY ORDER IS BACKWARDS, and it changed what the bug looked like. Captured
against a live server — sets `WHERE sender = 'probe43'` and `WHERE text = 'alpha'`,
then an edit of `text` — the BARE DELETE arrived in entry 0 and the pair in entry
1, the opposite of the write-up. Measured against the old cache.lua on those exact
frames:

  bare, pair (captured)   count=1 row=beta events=[delete,update]
  pair, bare (reported)   count=0 row=GONE events=[update,delete]

One order kept the row and lied about it; the other lost it. Nothing on the wire
promises either order, so both are asserted, and the fix is arithmetic rather than
positional so it cannot depend on which entry came first.

A delete that only releases a hold now emits no event either: a `delete` beside a
row `cache.get` still answers for is a contradiction, not a notification. With
nothing multiply held every delete is the last one, so the single-query case is
unchanged in events, counts and store — asserted directly, not inferred.
`counts.deletes` still counts row operations, including a release that removed
nothing.

The four interactions this had to leave alone:

  * #21's pairing is untouched — still per `updates` entry, and the paired-update
    events fire independently of the count.
  * #28's unique indexes survive a decrement because the surviving branch never
    calls `remove` at all. That is the exact place a naive refcount strips the
    index while `state.tables` keeps the row, so beside the hand-written case
    there is a seeded 200-step randomised sequence asserting both directions:
    every index entry points at a stored row filed under its own value, and every
    stored row is reachable through each unique column.
  * #35's typed decode and #42's undescribed-relation skip are not on this path.
  * "entries" is not "subscriptions" anywhere in the implementation, since it
    counts arrivals and never subscriptions.

TWO COMMITTED TESTS LEARNED THE NEW RULE rather than being rewritten to agree
with the code; both were replaying shapes the wire does not produce.
test_cache.lua's upsert sequence did a bare re-insert of a key and then ONE
delete — every original assertion is kept, with the new contract (two holds need
two deletes) now asserted explicitly between them. test_fixtures.lua's `player`
block replayed frame 36 before frame 35, manufacturing a two-hold state the
socket never had; it now replays them in wire order, with a comment saying why
order matters, since it did not before.

CONFORMANCE PHASE3 WAS ASSERTING THE BUG. Its comment claimed "the legacy
subscription was replaced by the query set, so nothing matches these rows any
more" — but a query set does not replace the legacy `Subscribe`; they coexist,
which is what frame 24 pins and what the capture re-confirms. So the cache
draining on that unsubscribe was #43 happening inside our own harness. PHASE3 now
asserts the row SURVIVES the drop with no on_delete owed, and the drain plus the
delete half of the per-table API moved onto phase 5's `clear_all`, which is a real
delete. Reverting cache.lua alone makes the harness report `FAIL the drop evicted
a row the legacy subscription still matches` against a live server.

Two limitations documented in README rather than fixed, because both are
pre-existing and neither is what #43 is about:

  * the legacy `Subscribe` ships no delta for the set it replaces — called twice
    it re-sends every row as a bare insert with no deletes, and narrowing sends
    no delete for the rows that left. It therefore leaves both a stale row and a
    count one too high. It was already not cache-coherent for the same reason;
    the guidance is to call it once per connection and use `subscribe_query`
    otherwise. `resubscribe_all` is safe: it empties the cache first.
  * a pk-less relation has no key to count on, and two arrivals of one row are
    already two array entries, so the store behaves correctly without help — but
    a unique index files one row per value, so deleting the copy it holds leaves
    the survivor reachable by `iter` and not by `find`. Pinned as a test and
    verified byte-identical against the previous cache.lua on the same frames.

6 passed, 0 failed; stylua clean; native and HTML5 conformance both PASS
including PHASE5. Every added line was mutation-tested: eleven mutations of the
count, the gone-gate, the guards and the two drop paths each fail with a named
assertion.

yuris commented 2026-08-20 06:21:50 +00:00

Author

Owner

Copy link

Review — automated reviewer, posted for the record

Verdict: APPROVE with 3 should-fix. No blockers. The design is correct and forced by the wire; both premise corrections hold. Closes #43 in code, but one of the issue's four work items is unmet.

scratch file

/tmp/r51-findings.md (353 lines, full evidence)

artifacts

/tmp/probe/{probe,sim,diff,bench,mem,replay,edge,tbl}.lua + captured live frames {overlap,order2,order3,legacy,legacy_worse,phase3}.jsonl. /tmp/review-51 left clean; server :5400 stopped; my probe file removed from the tree.

premise 1 no per entry query id

status — CONFIRMED — not a blocker

source — upstream websocket/v1.rs @ v2.8.2, fetched raw: L570-583 TableUpdate { table_id, table_name, num_rows, updates: SmallVec<[F::QueryUpdate; 1]> } (bare list); L631-645 QueryUpdate { deletes, inserts } (two fields, no id). Upstream corroborates itself at L340-342 on UnsubscribeApplied.rows: 'To remove this in the future, we would need to send query_ids with rows in transaction updates, and we would need clients to track which rows exist in which queries.'

wire — All 25 QueryUpdate objects in the committed session.jsonl have key set exactly ["deletes","inserts"]; all 25 TableUpdates exactly ["num_rows","table_id","table_name","updates"]. No identifier in a real frame either.

v2 — v2.rs L304-314 TransactionUpdate { query_sets: [{ query_set_id, tables }] } — the asymmetry the author describes is real.

conclusion — Threading query_id in, as #43 asked, is impossible on this protocol. Keeping apply_database_update's signature is right. No identifier exists that the author missed.

premise 2 bare delete is entry 0

status — CONFIRMED LIVE against 2.8.2

evidence — Own probe, q100=WHERE sender='probe43', q101=WHERE text='alpha', then edit text->beta. Captured TransactionUpdate: e0: -[[3,"probe43","alpha"]] +[] (BARE DELETE) / e1: -[[3,"probe43","alpha"]] +[[3,"probe43","beta"]] (PAIR). #43's write-up has it backwards.

order stability — Bare-delete-first in every shape I could produce (dropping-query subscribed 2nd, subscribed 1st, and 3 overlapping queries). Nothing on the wire promises it (SmallVec, server-internal), so asserting both orders is correct.

two distinct failures claim — Partly overstated — see nit 1. Reproduced on origin/main: order A (bare,pair) -> events delete,update, count=1, row = v1, the CORRECT value; order B (pair,bare) -> events update,delete, count=0, row LOST. So main's data loss inside a transaction frame is one-sided, not two different failures. The PR removes the order dependence: both orders converge on refs=1, row=v1, events=[update].

the live data loss is real and i reproduced it independently

On real captured bytes (order3.jsonl, 3 overlapping distinct queries, replayed through both caches): PR f9 unsub q300 -> holds 2->1, store {7:1}; f11 unsub q303 -> delete(7), store {}. origin/main f9 -> store {} while q303 still matches, and f11 emits a SECOND delete(7) for a row already reported gone. My randomised server simulator reproduces the same on main on every seed tried (seed 2 step 21: 'store has 0 rows, expected 4'). The deleted README bullet ('Drop one of two overlapping subscriptions and it deletes nothing... Clients need no query-to-row bookkeeping of their own') is flatly wrong for distinct-text sets — its removal is a correction, not a convenience.

hold count attack all seven cases pass

row twice in one batch then one delete — Survives (count 1), no delete event, counts.deletes still 1 (row ops). Second delete removes and emits. Fixture frame 23 is exactly this shape (pk 5 twice, two bare inserts) and frame 24 then edits it as two pairs -> refs stays 2, events update,update, identical to main.

leak across reconnect — No leak. drop_all and drop_table both clear refs; reconnect path is init.lua:262 resubscribe_all -> connection.lua:542 cache.drop_all. close() (init.lua:281) touches no cache, and a post-close reconnect is a fresh M.connect -> fresh cache.new. Grep-verified: drop_all/drop_table are the only writers of state.refs outside retain/release.

spurious or duplicated delete — Cannot go negative. refs[key] is only ever written as n-1 under n>1, or nil'd. 4 spurious deletes in a row leave the slot absent and each emits a delete event exactly as before; the next insert starts at 1 and one delete removes it.

paired update on a doubly held row — One update event with old carried, new value stored, hold count stays 2, counts 1/1 — and it still takes two deletes to leave.

pk less table — No refs[name] entry is created at all. Two arrivals = two array entries; one delete removes one copy; events byte-identical to main (same probe run on the main worktree).

same key two tables — refs is keyed per table (state.refs[name]); one table drains while the other keeps its hold.

42 undescribed skip — Skipped before table_def/retain is reached (cache.lua:431-442): counts 0/0, no events, and no refs["ghost"] table created. No holds for rows never stored.

extra edges i added — BOOLEAN pk (false) counts correctly — the def.pk and row[def.pk] idiom is safe because release compares against nil. An insert whose pk decodes to nil still RAISES and leaves no refs table, because retain calls store (which guards) BEFORE touching refs — that ordering is load-bearing. A delete whose pk is nil takes the remove path, emits, and leaves the real row and its hold untouched.

interactions verified independently

21 byte identical — VERIFIED by differential, not trusted. /tmp/probe/diff.lua dumps every frame's counts and full event list (including each update's old), then the final store, pk-less counts and index contents. 5 seeds x 400 steps, single live query plus pk-less and undescribed traffic: output byte-identical between main and the PR (cmp clean, 367-383 lines each).

28 indexes own randomised check — Wrote a faithful SERVER SIMULATOR (/tmp/probe/sim.lua): true row set + live query predicates, derives each query's own delta (started matching -> insert, stopped -> delete, matched-but-changed -> pair), emits one shuffled updates entry per query, and drives subscribe/unsubscribe. Asserts store, values, find(handle), find(id), no dangling index entry, and the EXACT expected hold count. PR: 12 seeds x 800 steps = 9,600 frames, 9,612 verifications, ALL INVARIANTS HELD. main: fails every seed by step 11-60. #44's blocker class (decremented-but-kept row losing its index entry) does not reproduce. Note: my first generator was wrong — it handed two live rows the same unique handle, an impossible state reindex documents; fixed with in-frame handle reservation, then clean.

35 and 42 — Untouched. The cache.lua diff is confined to the ROW HOLDS comment, the refs field, retain/release, the two call sites in the delete/insert loops, and drop_all/drop_table. decode_rows/decode_typed are not in the diff; the typed path is covered by test_fixtures.lua (passing) and by conformance running against the generated chat_schema.lua.

the three changed tests

test cache upsert block — LEGITIMATE. Both original assertions are kept verbatim (count stays 2, sender becomes victor). The old test's setup had delivered key 2 twice as bare inserts, which under the new contract is two holds, so the following pk-change assertion ('old pk removed') needed one hold released first. A bare insert of an already-cached key really is the second-query shape (live-confirmed). The new intermediate step is required by the contract, not bent around it.

test fixtures player reorder — LEGITIMATE. Wire order in the capture is 35 (unsubscribe) then 36 (resubscribe readback); the old test replayed 36 then 35, which under the new contract puts TWO holds on one key and asserts a state the socket never produced. All four original assertions survive; the drain is now asserted from frame 35 in wire order and the block gains an index-refill assertion.

smoke script phase3 — CORRECT, and I scrutinised it hardest. Reproduced PHASE3's exact shape live: legacy Subscribe matches the row (holds 1); a SubscribeMulti of the SAME TEXT does NOT collapse with it and re-sends the row as a bare insert (holds 2); UnsubscribeMulti sends a delete (holds 1) -> the row survives; and clear_all later sends ONE entry deleting it (holds 0) -> the cache drains. The server itself proves the survival is right: after the drop it still ships that row's deltas to this connection. On main the drop evicted it, so clear_all's delete would land on a key the cache no longer had. THE OLD ASSERTION WAS ASSERTING THE BUG. Cross-swapped both ways to prove neither assertion is vacuous: PR smoke.script + main's cache.lua -> conformance FAILS 'the drop evicted a row the legacy subscription still matches'; main's smoke.script + PR's cache.lua -> FAILS 'unsubscribed but 1 row(s) remain cached'. The drain assertion genuinely MOVED (old PHASE4 tbl.deletes == 0 check re-homed onto PHASE5's subscribed branch) and coverage INCREASED (PHASE5 gains a new cache.count(...) ~= 0 assertion inside the clear_all callback that the old script did not have).

cost against 30

measured — Lua 5.1 via .#ci, best-of-7, both builds same machine. 20k-row InitialSubscription (pk): main 113.8 ms -> PR 123.2 ms (+9.4 ms, +8.3%; 5.7 -> 6.2 us/row). 2000 paired-update frames on a warm 20k cache: 22.3 -> 22.8 ms (+1.9%, 11.4 us/frame). One frame carrying 2000 updates entries: 9.4 -> 9.6 ms (+2.1%). pk-less 20k: no holds taken, within noise.

memory — Holds table measured by dropping it in-process and re-measuring: 20k pk rows 6864.7 -> 7377.0 KiB = 512.2 KiB = 26.2 B/key; 100k rows 33126.6 -> 35174.9 KiB = 2048.2 KiB = 21.0 B/key (per-key cost falls because Lua sizes the hash part to a power of two). One integer per PRIMARY KEY, ~6% on top of what the rows already occupy. pk-less relations pay nothing.

judgement — Report, do not block, per the brief. A 20k-row initial subscription already costs ~114 ms on main — about 7 dropped frames at 60 fps — so #51 adds ~9 ms to a figure already far over budget and does not change the shape of #30. The steady-state transaction path pays ~2%.

documented limitations accuracy

legacy subscribe — All three claims confirmed live: repeat Subscribe with the same text re-sends every matching row as a bare insert with NO deletes; narrowing sends no delete for the row that left; and a legacy Subscribe does NOT collapse with a SubscribeMulti of identical text (so 'the legacy set counts as one more query' is right). Also confirmed on the committed fixture that two SubscribeMulti sets of identical text DO share one query (frames 14-17: subscribe q100 -> 2 rows, q101 -> nothing, unsub q100 -> nothing, unsub q101 -> 2 deletes). Placement: README.md:344 and cache.lua:71-80 — accurate, but not on M.subscribe itself (see should-fix 2).

pk less — Accurate and executed. Two arrivals = two array entries; one delete removes one copy; a unique index files only one row per value so cache.find returns nil for the survivor while cache.iter still reaches it — exactly as README.md:337 states. Byte-identical to main. Placement is fine.

sanity

suite — 6 passed, 0 failed

stylua — clean, exit 0 on stdb dev/test

native conformance — PASS both runs, PHASE1-PHASE5, including the new 'real delete drained the cache and fired 1 on_delete callback(s)' line. Schema re-derived and matched: no fixture drift.

html5 conformance — PASS both loads, PHASE5 included.

fixture drift — none; session.jsonl and chat_schema.lua untouched

Findings

• should-fix — PR body ('Documented limitations') vs stdb/cache.lua:71-80 and README.md:344
  • The refcount makes the legacy Subscribe path WORSE, not merely 'pre-existing, now written down'. Captured live (legacy_worse.jsonl) and replayed through both caches: Subscribe twice with the same text delivers the row twice (holds 2), then clear_all genuinely deletes it and ONE delete arrives. main: store {}, delete event fired — CORRECT. PR: store {4097:1}, NO event — a PHANTOM ROW the cache serves forever, with no second delete ever coming and on_delete never firing. main was correct on this exact sequence. The stale-row-on-narrowing half IS pre-existing (legacy.jsonl f7: row 9 stale on both builds), but the deletion half is a new silent wrong answer from a frozen public method.
  • Fix: The README text is already accurate ('a hold count one too high, which can outlive the row's real deletion') — it is the PR body that is not. At minimum call it a regression rather than a pre-existing limitation. Better, make it loud: connection.lua knows when it issues a second legacy Subscribe while one is live (c.subscriptions is keyed by request_id), so it can emit an unhandled/warning event instead of silently inflating every hold. Not a blocker only because the trigger is a pattern the README now explicitly steers away from and resubscribe_all drops the cache first.
• should-fix — stdb/tables.lua:44-51 and stdb/connection.lua:353-358
  • The on_insert/on_delete asymmetry is not stated where a consumer looks. Measured through the real per-table API: one row delivered by two distinct queries then dropped by each in turn gives on_insert=2, on_delete=1 on the PR (main gave 2 and 2, with the row wrongly evicted at the first drop). on_insert fires per ARRIVAL while on_delete fires only at the LAST release, so a consumer doing n=n+1/n=n-1 drifts by +1 per extra query, permanently. tables.lua's 'Three things a game has to know that no signature states' list already documents the reconnect drift in exactly these terms and omits this one. Separately, M.subscribe's own docstring says nothing about the repeat-call hazard — it is documented only in README's unsubscribe section and cache.lua's comment block.
  • Fix: Add a fourth bullet to tables.lua's list, and one line to M.subscribe's docstring pointing at the README paragraph.
• should-fix — dev/test/fixtures/session.jsonl (untouched by this PR)
  • #43's first work item — 'Reproduce with a captured fixture' — is unmet. The #43 transaction shape (a bare delete beside a full delete+insert pair) exists NOWHERE in the repo; it lives only in hand-built frames in test_cache.lua and in the author's (and my) transient captures. smoke.script PHASE3 guards the UNSUBSCRIBE release path, not that transaction shape, so nothing committed would notice if 2.8.2's per-query delta framing changed. This is the one wire shape the whole design rests on.
  • Fix: Append a block via dev/server/capture-fixtures.mjs's existing BLOCKS mechanism, which exists for this. Real cost acknowledged: appending invalidates every later block and forces a recapture.
• nit — PR body, premise-correction-2 table
  • The table says order bare, pair left the row 'present but stale — kept the row and lied about it'. Reproduced against origin/main: it leaves v1, the CORRECT current value. The lie was the spurious delete EVENT beside a row that remains, not a stale row. So main's failure is one-sided (loses the row only in pair, bare order), not 'two different failures'.
  • Fix: Prose only. The test's own comment (test_cache.lua, bare, pair (captured) count=1 row=beta events=[delete,update]) records this correctly, so no code or test change is needed.
• nit — PR body heading 'Premise correction 1 — the association does *not* exist on the wire'
  • Broader than the (correct) code comment. Message-level query_id DOES exist on SubscribeApplied (v1.rs:323), SubscribeMultiApplied (386), UnsubscribeApplied (338), UnsubscribeMultiApplied (401) and SubscriptionError (358), so on the subscribe/unsubscribe paths a whole DatabaseUpdate can be attributed to one query set. What is missing is per-ENTRY attribution inside a TransactionUpdate — which is exactly the #43 failure path, and exactly what cache.lua's comment says ('no query identifier on an updates entry').
  • Fix: Tighten the heading to say 'on an updates entry'. No code change.

## Review — automated reviewer, posted for the record **Verdict:** APPROVE with 3 should-fix. No blockers. The design is correct and forced by the wire; both premise corrections hold. Closes #43 in code, but one of the issue's four work items is unmet. ### scratch file /tmp/r51-findings.md (353 lines, full evidence) ### artifacts /tmp/probe/{probe,sim,diff,bench,mem,replay,edge,tbl}.lua + captured live frames {overlap,order2,order3,legacy,legacy_worse,phase3}.jsonl. /tmp/review-51 left clean; server :5400 stopped; my probe file removed from the tree. ### premise 1 no per entry query id **status** — CONFIRMED — not a blocker **source** — upstream websocket/v1.rs @ v2.8.2, fetched raw: L570-583 `TableUpdate { table_id, table_name, num_rows, updates: SmallVec<[F::QueryUpdate; 1]> }` (bare list); L631-645 `QueryUpdate { deletes, inserts }` (two fields, no id). Upstream corroborates itself at L340-342 on `UnsubscribeApplied.rows`: 'To remove this in the future, we would need to send query_ids with rows in transaction updates, and we would need clients to track which rows exist in which queries.' **wire** — All 25 QueryUpdate objects in the committed `session.jsonl` have key set exactly ["deletes","inserts"]; all 25 TableUpdates exactly ["num_rows","table_id","table_name","updates"]. No identifier in a real frame either. **v2** — v2.rs L304-314 `TransactionUpdate { query_sets: [{ query_set_id, tables }] }` — the asymmetry the author describes is real. **conclusion** — Threading `query_id` in, as #43 asked, is impossible on this protocol. Keeping `apply_database_update`'s signature is right. No identifier exists that the author missed. ### premise 2 bare delete is entry 0 **status** — CONFIRMED LIVE against 2.8.2 **evidence** — Own probe, q100=`WHERE sender='probe43'`, q101=`WHERE text='alpha'`, then edit text->beta. Captured TransactionUpdate: `e0: -[[3,"probe43","alpha"]] +[]` (BARE DELETE) / `e1: -[[3,"probe43","alpha"]] +[[3,"probe43","beta"]]` (PAIR). #43's write-up has it backwards. **order stability** — Bare-delete-first in every shape I could produce (dropping-query subscribed 2nd, subscribed 1st, and 3 overlapping queries). Nothing on the wire promises it (`SmallVec`, server-internal), so asserting both orders is correct. **two distinct failures claim** — Partly overstated — see nit 1. Reproduced on origin/main: order A (bare,pair) -> events `delete,update`, count=1, row = **v1, the CORRECT value**; order B (pair,bare) -> events `update,delete`, count=0, row LOST. So main's data loss inside a transaction frame is one-sided, not two different failures. The PR removes the order dependence: both orders converge on refs=1, row=v1, events=[update]. ### the live data loss is real and i reproduced it independently On real captured bytes (`order3.jsonl`, 3 overlapping distinct queries, replayed through both caches): PR f9 unsub q300 -> holds 2->1, store {7:1}; f11 unsub q303 -> delete(7), store {}. origin/main f9 -> store {} while q303 still matches, and f11 emits a SECOND delete(7) for a row already reported gone. My randomised server simulator reproduces the same on main on every seed tried (seed 2 step 21: 'store has 0 rows, expected 4'). The deleted README bullet ('Drop one of two overlapping subscriptions and it deletes *nothing*... Clients need no query-to-row bookkeeping of their own') is flatly wrong for distinct-text sets — its removal is a correction, not a convenience. ### hold count attack all seven cases pass **row twice in one batch then one delete** — Survives (count 1), no delete event, `counts.deletes` still 1 (row ops). Second delete removes and emits. Fixture frame 23 is exactly this shape (pk 5 twice, two bare inserts) and frame 24 then edits it as two pairs -> refs stays 2, events `update,update`, identical to main. **leak across reconnect** — No leak. `drop_all` and `drop_table` both clear `refs`; reconnect path is init.lua:262 `resubscribe_all` -> connection.lua:542 `cache.drop_all`. `close()` (init.lua:281) touches no cache, and a post-close reconnect is a fresh `M.connect` -> fresh `cache.new`. Grep-verified: `drop_all`/`drop_table` are the only writers of `state.refs` outside `retain`/`release`. **spurious or duplicated delete** — Cannot go negative. `refs[key]` is only ever written as `n-1` under `n>1`, or nil'd. 4 spurious deletes in a row leave the slot absent and each emits a delete event exactly as before; the next insert starts at 1 and one delete removes it. **paired update on a doubly held row** — One `update` event with `old` carried, new value stored, hold count stays 2, counts 1/1 — and it still takes two deletes to leave. **pk less table** — No `refs[name]` entry is created at all. Two arrivals = two array entries; one delete removes one copy; events byte-identical to main (same probe run on the main worktree). **same key two tables** — `refs` is keyed per table (`state.refs[name]`); one table drains while the other keeps its hold. **42 undescribed skip** — Skipped before `table_def`/`retain` is reached (cache.lua:431-442): counts 0/0, no events, and no `refs["ghost"]` table created. No holds for rows never stored. **extra edges i added** — BOOLEAN pk (`false`) counts correctly — the `def.pk and row[def.pk]` idiom is safe because `release` compares against nil. An insert whose pk decodes to nil still RAISES and leaves no `refs` table, because `retain` calls `store` (which guards) BEFORE touching `refs` — that ordering is load-bearing. A delete whose pk is nil takes the `remove` path, emits, and leaves the real row and its hold untouched. ### interactions verified independently **21 byte identical** — VERIFIED by differential, not trusted. `/tmp/probe/diff.lua` dumps every frame's counts and full event list (including each `update`'s `old`), then the final store, pk-less counts and index contents. 5 seeds x 400 steps, single live query plus pk-less and undescribed traffic: output byte-identical between main and the PR (`cmp` clean, 367-383 lines each). **28 indexes own randomised check** — Wrote a faithful SERVER SIMULATOR (`/tmp/probe/sim.lua`): true row set + live query predicates, derives each query's own delta (started matching -> insert, stopped -> delete, matched-but-changed -> pair), emits one shuffled `updates` entry per query, and drives subscribe/unsubscribe. Asserts store, values, find(handle), find(id), no dangling index entry, and the EXACT expected hold count. PR: 12 seeds x 800 steps = 9,600 frames, 9,612 verifications, ALL INVARIANTS HELD. main: fails every seed by step 11-60. #44's blocker class (decremented-but-kept row losing its index entry) does not reproduce. Note: my first generator was wrong — it handed two live rows the same unique `handle`, an impossible state `reindex` documents; fixed with in-frame handle reservation, then clean. **35 and 42** — Untouched. The cache.lua diff is confined to the ROW HOLDS comment, the `refs` field, `retain`/`release`, the two call sites in the delete/insert loops, and `drop_all`/`drop_table`. `decode_rows`/`decode_typed` are not in the diff; the typed path is covered by test_fixtures.lua (passing) and by conformance running against the generated `chat_schema.lua`. ### the three changed tests **test cache upsert block** — LEGITIMATE. Both original assertions are kept verbatim (count stays 2, sender becomes victor). The old test's *setup* had delivered key 2 twice as bare inserts, which under the new contract is two holds, so the following pk-change assertion ('old pk removed') needed one hold released first. A bare insert of an already-cached key really is the second-query shape (live-confirmed). The new intermediate step is required by the contract, not bent around it. **test fixtures player reorder** — LEGITIMATE. Wire order in the capture is 35 (unsubscribe) then 36 (resubscribe readback); the old test replayed 36 then 35, which under the new contract puts TWO holds on one key and asserts a state the socket never produced. All four original assertions survive; the drain is now asserted from frame 35 in wire order and the block gains an index-refill assertion. **smoke script phase3** — CORRECT, and I scrutinised it hardest. Reproduced PHASE3's exact shape live: legacy `Subscribe` matches the row (holds 1); a SubscribeMulti of the SAME TEXT does NOT collapse with it and re-sends the row as a bare insert (holds 2); UnsubscribeMulti sends a delete (holds 1) -> the row survives; and `clear_all` later sends ONE entry deleting it (holds 0) -> the cache drains. The server itself proves the survival is right: after the drop it still ships that row's deltas to this connection. On main the drop evicted it, so `clear_all`'s delete would land on a key the cache no longer had. THE OLD ASSERTION WAS ASSERTING THE BUG. Cross-swapped both ways to prove neither assertion is vacuous: PR smoke.script + main's cache.lua -> conformance FAILS 'the drop evicted a row the legacy subscription still matches'; main's smoke.script + PR's cache.lua -> FAILS 'unsubscribed but 1 row(s) remain cached'. The drain assertion genuinely MOVED (old PHASE4 `tbl.deletes == 0` check re-homed onto PHASE5's `subscribed` branch) and coverage INCREASED (PHASE5 gains a new `cache.count(...) ~= 0` assertion inside the clear_all callback that the old script did not have). ### cost against 30 **measured** — Lua 5.1 via `.#ci`, best-of-7, both builds same machine. 20k-row InitialSubscription (pk): main 113.8 ms -> PR 123.2 ms (+9.4 ms, +8.3%; 5.7 -> 6.2 us/row). 2000 paired-update frames on a warm 20k cache: 22.3 -> 22.8 ms (+1.9%, 11.4 us/frame). One frame carrying 2000 `updates` entries: 9.4 -> 9.6 ms (+2.1%). pk-less 20k: no holds taken, within noise. **memory** — Holds table measured by dropping it in-process and re-measuring: 20k pk rows 6864.7 -> 7377.0 KiB = 512.2 KiB = 26.2 B/key; 100k rows 33126.6 -> 35174.9 KiB = 2048.2 KiB = 21.0 B/key (per-key cost falls because Lua sizes the hash part to a power of two). One integer per PRIMARY KEY, ~6% on top of what the rows already occupy. pk-less relations pay nothing. **judgement** — Report, do not block, per the brief. A 20k-row initial subscription already costs ~114 ms on main — about 7 dropped frames at 60 fps — so #51 adds ~9 ms to a figure already far over budget and does not change the shape of #30. The steady-state transaction path pays ~2%. ### documented limitations accuracy **legacy subscribe** — All three claims confirmed live: repeat `Subscribe` with the same text re-sends every matching row as a bare insert with NO deletes; narrowing sends no delete for the row that left; and a legacy `Subscribe` does NOT collapse with a `SubscribeMulti` of identical text (so 'the legacy set counts as one more query' is right). Also confirmed on the committed fixture that two *SubscribeMulti* sets of identical text DO share one query (frames 14-17: subscribe q100 -> 2 rows, q101 -> nothing, unsub q100 -> nothing, unsub q101 -> 2 deletes). Placement: README.md:344 and cache.lua:71-80 — accurate, but not on `M.subscribe` itself (see should-fix 2). **pk less** — Accurate and executed. Two arrivals = two array entries; one delete removes one copy; a unique index files only one row per value so `cache.find` returns nil for the survivor while `cache.iter` still reaches it — exactly as README.md:337 states. Byte-identical to main. Placement is fine. ### sanity **suite** — 6 passed, 0 failed **stylua** — clean, exit 0 on `stdb dev/test` **native conformance** — PASS both runs, PHASE1-PHASE5, including the new 'real delete drained the cache and fired 1 on_delete callback(s)' line. Schema re-derived and matched: no fixture drift. **html5 conformance** — PASS both loads, PHASE5 included. **fixture drift** — none; session.jsonl and chat_schema.lua untouched ### Findings - **should-fix** — `PR body ('Documented limitations') vs stdb/cache.lua:71-80 and README.md:344` - The refcount makes the legacy `Subscribe` path WORSE, not merely 'pre-existing, now written down'. Captured live (`legacy_worse.jsonl`) and replayed through both caches: `Subscribe` twice with the same text delivers the row twice (holds 2), then `clear_all` genuinely deletes it and ONE delete arrives. main: store {}, `delete` event fired — CORRECT. PR: store {4097:1}, NO event — a PHANTOM ROW the cache serves forever, with no second delete ever coming and `on_delete` never firing. main was correct on this exact sequence. The stale-row-on-narrowing half IS pre-existing (`legacy.jsonl` f7: row 9 stale on both builds), but the deletion half is a new silent wrong answer from a frozen public method. - *Fix:* The README text is already accurate ('a hold count one too high, which can outlive the row's real deletion') — it is the PR body that is not. At minimum call it a regression rather than a pre-existing limitation. Better, make it loud: `connection.lua` knows when it issues a second legacy `Subscribe` while one is live (`c.subscriptions` is keyed by request_id), so it can emit an `unhandled`/warning event instead of silently inflating every hold. Not a blocker only because the trigger is a pattern the README now explicitly steers away from and `resubscribe_all` drops the cache first. - **should-fix** — `stdb/tables.lua:44-51 and stdb/connection.lua:353-358` - The on_insert/on_delete asymmetry is not stated where a consumer looks. Measured through the real per-table API: one row delivered by two distinct queries then dropped by each in turn gives on_insert=2, on_delete=1 on the PR (main gave 2 and 2, with the row wrongly evicted at the first drop). `on_insert` fires per ARRIVAL while `on_delete` fires only at the LAST release, so a consumer doing `n=n+1`/`n=n-1` drifts by +1 per extra query, permanently. tables.lua's 'Three things a game has to know that no signature states' list already documents the reconnect drift in exactly these terms and omits this one. Separately, `M.subscribe`'s own docstring says nothing about the repeat-call hazard — it is documented only in README's unsubscribe section and cache.lua's comment block. - *Fix:* Add a fourth bullet to tables.lua's list, and one line to `M.subscribe`'s docstring pointing at the README paragraph. - **should-fix** — `dev/test/fixtures/session.jsonl (untouched by this PR)` - #43's first work item — 'Reproduce with a captured fixture' — is unmet. The #43 transaction shape (a bare delete beside a full delete+insert pair) exists NOWHERE in the repo; it lives only in hand-built frames in test_cache.lua and in the author's (and my) transient captures. smoke.script PHASE3 guards the UNSUBSCRIBE release path, not that transaction shape, so nothing committed would notice if 2.8.2's per-query delta framing changed. This is the one wire shape the whole design rests on. - *Fix:* Append a block via `dev/server/capture-fixtures.mjs`'s existing BLOCKS mechanism, which exists for this. Real cost acknowledged: appending invalidates every later block and forces a recapture. - **nit** — `PR body, premise-correction-2 table` - The table says order `bare, pair` left the row 'present but stale — kept the row and lied about it'. Reproduced against origin/main: it leaves `v1`, the CORRECT current value. The lie was the spurious `delete` EVENT beside a row that remains, not a stale row. So main's failure is one-sided (loses the row only in `pair, bare` order), not 'two different failures'. - *Fix:* Prose only. The test's own comment (test_cache.lua, `bare, pair (captured) count=1 row=beta events=[delete,update]`) records this correctly, so no code or test change is needed. - **nit** — `PR body heading 'Premise correction 1 — the association does *not* exist on the wire'` - Broader than the (correct) code comment. Message-level `query_id` DOES exist on `SubscribeApplied` (v1.rs:323), `SubscribeMultiApplied` (386), `UnsubscribeApplied` (338), `UnsubscribeMultiApplied` (401) and `SubscriptionError` (358), so on the subscribe/unsubscribe paths a whole DatabaseUpdate can be attributed to one query set. What is missing is per-ENTRY attribution inside a `TransactionUpdate` — which is exactly the #43 failure path, and exactly what cache.lua's comment says ('no query identifier on an `updates` entry'). - *Fix:* Tighten the heading to say 'on an updates entry'. No code change.

yuris force-pushed feat/43-overlap from fa23a4ab40 to 70cf4d9400 2026-08-20 06:57:37 +00:00 Compare

yuris merged commit 13073c6ec2 into main 2026-08-20 07:15:04 +00:00

yuris referenced this pull request from a commit 2026-08-20 07:15:05 +00:00

cache: count how many queries hold a row, so an overlapping one survives (#51)

yuris referenced this pull request 2026-08-20 07:18:14 +00:00

v1.0.0 release checklist #24

yuris referenced this pull request 2026-08-20 08:48:25 +00:00

docs: correct every behavioural claim measurement contradicted #56

yuris referenced this pull request 2026-08-20 11:04:11 +00:00

docs: correct every behavioural claim measurement contradicted #56

yuris referenced this pull request 2026-08-20 11:46:16 +00:00

v1.0.0 release checklist #24

yuris referenced this pull request 2026-08-20 13:06:40 +00:00

v1.0.0 release checklist #24

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:defold

Defold engine integration

  

area:docs

Documentation and examples

  

area:infra

Flake, CI, secretspec

  

area:protocol

Wire protocol conformance

  

area:testing

Test harness and fixtures

  

kind:bug

Something broken

  

kind:chore

Maintenance

  

phase:breadth

UnsubscribeMulti, procedures, BSATN

  

phase:core

Core SDK layers (protocol/cache/connection)

  

phase:hardening

Reconnect, token persistence, errors

  

phase:slice

Vertical slice: first playable end-to-end

No labels

area:defold

area:docs

area:infra

area:protocol

area:testing

kind:bug

kind:chore

phase:breadth

phase:core

phase:hardening

phase:slice

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

yuris/spacetimedb-defold!51

No description provided.