yuris/spacetimedb-defold
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases 9 Packages Wiki Activity Actions

Identity-keyed tables are broken in the cache (pk decodes to a table) #35

New issue
Closed
opened 2026-08-19 16:11:51 +00:00 by yuris · 1 comment
yuris commented 2026-08-19 16:11:51 +00:00
Owner
Copy link

Found while reviewing #34 (update-event synthesis). A table whose primary key is an Identity — the canonical SpacetimeDB game pattern, player keyed by the connecting identity — does not work correctly in the cache today, and the whole test corpus dodges it.

The bug

protocol.decode_row (stdb/protocol.lua:142-167) never calls protocol.unwrap on field values. A special-type column therefore decodes to the raw wrapper table, so {"__identity__":"0x…"} becomes a Lua table and the primary key becomes a table reference.

Measured against both main and the #34 branch — delete plus 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. Two distinct table references are never equal, so:

  • cache.get(state, name, key) cannot find the row by identity
  • store / remove treat every arrival as a new key, so the row set grows without bound
  • #34's pk pairing never fires, so an identity-keyed table keeps emitting delete+insert — the exact despawn/respawn footgun #21 exists to remove

dev/test/fixtures/chat_schema.lua is u64 / string / string, so nothing in the suite touches it.

Same class, same issue

  • A u64 pk above 2^53 collides in Lua 5.1's doubles. Two distinct keys become one, silently, and the cache merges two rows.
  • A pk arriving as "1" in one array and 1 in the other never pairs — Lua's "1" ~= 1 — so the cache holds both.

Why this is not just "fix decode_row"

Unwrapping row fields changes the type of row field values, which is exactly the surface #24 freezes at 1.0. A consumer reading row.owner today gets a table; after the fix they get a string. That is a breaking change, and it must land before 1.0 rather than after.

It also overlaps two issues already open:

  • #25 teaches the generator to emit column types, which is what tells a decoder which columns are special.
  • #23 adds the special-type conversion helpers.

So this is the third leg: decide whether decode_row applies those conversions automatically, or whether rows stay raw and consumers convert. The cache forces the question, because a pk must be a comparable scalar for the store to work at all — regardless of what happens to non-key columns.

Work

  • Decide the representation: automatic conversion of special-typed columns in decode_row (needs #25's types), versus raw rows plus explicit helpers (#23) plus a pk-only unwrap in the cache
  • Whichever way, make an Identity-keyed table work: one logical row, one cache entry, cache.get finds it by identity, and #34's pairing produces an update
  • Add an identity-keyed table to dev/server/index.js and capture a fixture, so this is proven against the wire rather than argued
  • Decide and document the u64 > 2^53 limit — Lua 5.1 has no integers, so this cannot be fully solved, only bounded and documented
  • If any part will not be fixed for 1.0, say so in the README next to the update event row

Acceptance

A fixture-backed test where a player table keyed by Identity receives an insert, then a pk-preserving change, and the cache holds exactly one row throughout while the event stream reports one update.

Priority

Blocker for 1.0. Not because it is common in the current test module — it is absent there — but because it is common in real modules, it is silent, and the fix changes a frozen type. Fixing it after 1.0 costs a major version.

Found while reviewing #34 (update-event synthesis). A table whose primary key is an `Identity` — the canonical SpacetimeDB game pattern, `player` keyed by the connecting identity — does not work correctly in the cache today, and the whole test corpus dodges it. ## The bug `protocol.decode_row` (`stdb/protocol.lua:142-167`) never calls `protocol.unwrap` on field values. A special-type column therefore decodes to the raw wrapper table, so `{"__identity__":"0x…"}` becomes a Lua **table** and the primary key becomes a table *reference*. Measured against both `main` and the #34 branch — delete plus 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. Two distinct table references are never equal, so: - `cache.get(state, name, key)` cannot find the row by identity - `store` / `remove` treat every arrival as a new key, so the row set grows without bound - #34's pk pairing never fires, so an identity-keyed table keeps emitting delete+insert — the exact despawn/respawn footgun #21 exists to remove `dev/test/fixtures/chat_schema.lua` is `u64` / `string` / `string`, so nothing in the suite touches it. ## Same class, same issue - **A `u64` pk above 2^53** collides in Lua 5.1's doubles. Two distinct keys become one, silently, and the cache merges two rows. - **A pk arriving as `"1"` in one array and `1` in the other** never pairs — Lua's `"1" ~= 1` — so the cache holds both. ## Why this is not just "fix decode_row" Unwrapping row fields **changes the type of row field values**, which is exactly the surface #24 freezes at 1.0. A consumer reading `row.owner` today gets a table; after the fix they get a string. That is a breaking change, and it must land before 1.0 rather than after. It also overlaps two issues already open: - **#25** teaches the generator to emit column types, which is what tells a decoder *which* columns are special. - **#23** adds the special-type conversion helpers. So this is the third leg: decide whether `decode_row` applies those conversions automatically, or whether rows stay raw and consumers convert. The cache forces the question, because a pk **must** be a comparable scalar for the store to work at all — regardless of what happens to non-key columns. ## Work - [ ] Decide the representation: automatic conversion of special-typed columns in `decode_row` (needs #25's types), versus raw rows plus explicit helpers (#23) plus a pk-only unwrap in the cache - [ ] Whichever way, make an `Identity`-keyed table work: one logical row, one cache entry, `cache.get` finds it by identity, and #34's pairing produces an `update` - [ ] Add an identity-keyed table to `dev/server/index.js` and capture a fixture, so this is proven against the wire rather than argued - [ ] Decide and document the `u64 > 2^53` limit — Lua 5.1 has no integers, so this cannot be fully solved, only bounded and documented - [ ] If any part will not be fixed for 1.0, say so in the README next to the `update` event row ## Acceptance A fixture-backed test where a `player` table keyed by `Identity` receives an insert, then a pk-preserving change, and the cache holds exactly one row throughout while the event stream reports one `update`. ## Priority **Blocker for 1.0.** Not because it is common in the current test module — it is absent there — but because it is common in real modules, it is silent, and the fix changes a frozen type. Fixing it after 1.0 costs a major version.
yuris commented 2026-08-19 18:24:26 +00:00
Author
Owner
Copy link

Sequencing established: #26 makes this cheap, and it now gates #28's headline feature

Verified across three working trees during the #24 push.

Both #23 and #26 deliberately declined to fix this, independently and correctly

  • #37 (#23, merged) added explicit accessor helpers and left decode_row alone. protocol.lua:380-381: "unwrap stays a generic wrapper-stripper rather than doubling as the Identity accessor."
  • #46 (#26, in review) built a full typed decoder and deliberately did not wire it into decode_row. protocol.lua:618-624: "DELIBERATELY NOT WIRED INTO decode_row … exactly the cross-cutting, frozen-surface call #35 exists to make."

Two authors reached the same conclusion without coordinating, which is a good sign the boundary is real: per-column coercion on the shared path changes the type of values consumers already receive, and that is a major-version break after 1.0.

The consequence nobody had written down

The cache decodes through decode_row (cache.lua:155-164), never decode_typed. So no cached row ever receives typed decoding. Measured on the #46 branch: row.identity is still a table, two decodes of identical bytes are unequal, and neither matches the raw identity string.

This gates #28's motivating feature entirely

cache.find(state, "player", "identity", me) — the "which row is me?" lookup #28 exists to serve — returns nil for every spelling a consumer would try: the identity string IdentityToken hands you, a re-wrapped { __identity__ = … }, and even a row.identity table kept across a drop_all + resubscribe (the row is re-decoded into a fresh table). Only row.identity off that same row works, which is circular.

So the honest order is:

#26 → #35 → #28's motivating case works.

Merging #28 does not deliver that lookup at any point until this issue lands, independent of #28's own correctness. String and integer unique columns (handle, slug, email, an account number) work fully today, which is real value — just not the value the feature was pitched on.

The fix is now cheap, and the mechanism is specific

Once #26 lands, decode_typed already performs exactly the conversion needed. The remaining work is plumbing: give the cache access to the module's typespace and route row decoding through the typed path, rather than writing any new conversion logic. That is a much smaller change than this issue originally assumed, and it is why the decision should be made after #26 merges rather than before.

Cost flag, for #30

Routing cache decode through decode_typed coerces every column of every row on the main thread. For scale, #28's index maintenance alone measured ~0.33 µs/row and +8% on a 20k-row InitialSubscription. Per-column coercion will be materially more than that, and it lands on the same path — so whoever schedules this should measure it against #30's budget question rather than assuming it is free.

Still a 1.0 blocker

Nothing here reduces the priority. It raises it: three merged or in-flight PRs have now each routed around this, and each one added a comment explaining why. That is the signature of a decision that needs making rather than deferring again.

## Sequencing established: #26 makes this cheap, and it now gates #28's headline feature Verified across three working trees during the #24 push. ### Both #23 and #26 deliberately declined to fix this, independently and correctly - **#37 (#23, merged)** added explicit accessor helpers and left `decode_row` alone. `protocol.lua:380-381`: *"unwrap stays a generic wrapper-stripper rather than doubling as the Identity accessor."* - **#46 (#26, in review)** built a full typed decoder and deliberately did **not** wire it into `decode_row`. `protocol.lua:618-624`: *"DELIBERATELY NOT WIRED INTO decode_row … exactly the cross-cutting, frozen-surface call #35 exists to make."* Two authors reached the same conclusion without coordinating, which is a good sign the boundary is real: per-column coercion on the shared path changes the type of values consumers already receive, and that is a major-version break after 1.0. ### The consequence nobody had written down The cache decodes through `decode_row` (`cache.lua:155-164`), **never** `decode_typed`. So no cached row ever receives typed decoding. Measured on the #46 branch: `row.identity` is still a table, two decodes of identical bytes are unequal, and neither matches the raw identity string. ### This gates #28's motivating feature entirely `cache.find(state, "player", "identity", me)` — the "which row is me?" lookup #28 exists to serve — returns **nil** for every spelling a consumer would try: the identity string `IdentityToken` hands you, a re-wrapped `{ __identity__ = … }`, and even a `row.identity` table kept across a `drop_all` + resubscribe (the row is re-decoded into a fresh table). Only `row.identity` off that same row works, which is circular. So the honest order is: **#26 → #35 → #28's motivating case works.** Merging #28 does not deliver that lookup at any point until this issue lands, independent of #28's own correctness. String and integer unique columns (`handle`, `slug`, `email`, an account number) work fully today, which is real value — just not the value the feature was pitched on. ### The fix is now cheap, and the mechanism is specific Once #26 lands, `decode_typed` already performs exactly the conversion needed. The remaining work is plumbing: give the cache access to the module's `typespace` and route row decoding through the typed path, rather than writing any new conversion logic. That is a much smaller change than this issue originally assumed, and it is why the decision should be made *after* #26 merges rather than before. ### Cost flag, for #30 Routing cache decode through `decode_typed` coerces **every column of every row on the main thread**. For scale, #28's index maintenance alone measured ~0.33 µs/row and +8% on a 20k-row `InitialSubscription`. Per-column coercion will be materially more than that, and it lands on the same path — so whoever schedules this should measure it against #30's budget question rather than assuming it is free. ### Still a 1.0 blocker Nothing here reduces the priority. It raises it: three merged or in-flight PRs have now each routed around this, and each one added a comment explaining why. That is the signature of a decision that needs making rather than deferring again.
yuris closed this issue 2026-08-20 03:22:25 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
release/v4.0.1
fix/hold-pre-open-frames
v4.0.2
v4.0.1
v4.0.0
v3.1.0
v3.0.0
v2.0.0
v1.1.0
v1.0.0
v0.1.0
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
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#35
No description provided.