mirror of
https://github.com/clockworklabs/SpacetimeDB.git
synced 2026-05-06 15:49:35 -04:00
samuel.engstrom@arvikasoft.se
3eb4584118
# Description of Changes
`UClientCache::ApplyDiff` (`sdks/unreal/.../DBCache/ClientCache.h`) has
an asymmetry between its insert and delete paths.
**Phase 1 (deletes)** correctly only emits a `Diff.Deletes` entry when
the row's refcount transitions to 0 — overlapping subscriptions just
decrement.
**Phase 2 (inserts)** always appends to `Diff.Inserts`, regardless of
whether the row was already cached:
```cpp
// before
FRowEntry<RowType>* Entry = Table->Entries.Find(Key);
if (!Entry) { /* refcount = 1 */ }
else { /* refcount + 1 */ }
Diff.Inserts.Add(Key, *NewRow); // ← fires for both branches
```
`BroadcastDiff` then fires `OnInsert` for every `Diff.Inserts` entry, so
any table subscribed by two overlapping queries (e.g. a global `SELECT *
FROM t` plus a per-row `WHERE id = ...`) re-fires its insert handler on
every later subscription apply — once per cached row, every time. Game
code that does work in `OnInsert` (positioning, spawning, snapping to
terrain) re-runs and clobbers state that was meant to be set once.
The intent is documented in `RowEntry.h`: *"Wrapper storing a row value
with a reference count used by overlapping subscriptions."* Phase 1
follows that design; phase 2 doesn't.
### Fix
Move the `Diff.Inserts.Add` into the `!Entry` branch only, so it fires
only on the absent → refcount=1 transition:
```cpp
if (!Entry) {
Table->Entries.Add(Key, FRowEntry<RowType>{NewRow, 1});
Diff.Inserts.Add(Key, *NewRow);
}
else {
Table->Entries.Add(Key, FRowEntry<RowType>{NewRow, Entry->RefCount + 1});
}
```
### Why real updates still work
Cache keys are BSATN row-bytes, not primary keys. A real update arrives
as a `(delete old_bytes, insert new_bytes)` pair where `old_bytes ≠
new_bytes` — so the insert side still takes the `!Entry` branch and gets
a `Diff.Inserts` entry. `FTableAppliedDiff::DeriveUpdatesByPrimaryKey`
then pairs the delete and insert by PK into
`UpdateInserts`/`UpdateDeletes`, and `OnUpdate` (not `OnInsert`) fires,
exactly as today.
Edge cases:
| Scenario | Phase 2 branch | Result | Correct? |
|---|---|---|---|
| New row | `!Entry` | `OnInsert` | ✓ |
| Real update (different bytes) | `!Entry` | `OnInsert`+`OnDelete`
reconciled to `OnUpdate` by PK | ✓ |
| Overlapping sub re-delivers cached row | `else` | refcount bump, no
event | ✓ (was broken — fired duplicate `OnInsert`) |
| Trivial update (identical bytes) | `else` | refcount bump | irrelevant
— server diffs identical rows away before emitting |
# API and ABI breaking changes
None. Purely internal cache bookkeeping. Existing
`OnInsert`/`OnDelete`/`OnUpdate` semantics are preserved for all
non-overlapping cases; the only behavior change is that overlapping
subscriptions stop emitting duplicate `OnInsert` events for
already-cached rows — which matches the documented `RowEntry` refcount
contract.
# Expected complexity level and risk
**1.** Two lines moved into a branch; comments updated. Mirrors logic
already present and known-correct in phase 1.
# Testing
Reproduced and validated downstream in an Unreal project. The repro
setup is straightforward to replicate against any module:
1. A table `t` with ~150 rows.
2. A global subscription `SELECT * FROM t`, applied first.
3. A diagnostic actor that binds `t.OnInsert`, then every 10s submits a
new overlapping subscription (e.g. `SELECT * FROM t` again, or any
`SELECT * FROM t WHERE 'id' = X` covering already-cached rows) and
counts the `OnInsert` events that arrive in that round.
Expected: round 1 fires once per row that is *new to the cache*;
subsequent rounds against already-cached rows fire 0.
Observed (~161 rows cached after initial load):
```
Pre-fix Post-fix
Global sub (empty → 161) 161 161 ← genuine inserts; unchanged
Round 1 (overlapping) 134 0 ← 134 cached dupes
Round 2 (overlapping) 134 0
Round 3 (overlapping) 134 0
Round 4–6 (overlapping) 134 each 0 each
```
The genuine "empty cache → 161 entries" wave on the initial global
subscription is unaffected — same `OnInsert` count both pre- and
post-fix. Only the duplicate fires from later overlapping subscriptions
on already-cached rows are eliminated. `OnUpdate` still fires correctly
when underlying rows actually change.
- [x] Reviewer: confirm an existing real-update (different bytes, same
PK) test still produces `OnUpdate` and not `OnInsert`+`OnDelete`.
- [x] Reviewer: confirm any test that relies on `OnInsert` firing on
every subscription apply (rather than only on cache 0→1 transition) is
not present — if it exists, it was relying on the bug.