Skip to content

Commit 3eb4584

Browse files
fix(unreal): suppress spurious OnInsert on overlapping subscription refcount bump (#4903)
# 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.
1 parent 7332ece commit 3eb4584

1 file changed

Lines changed: 9 additions & 5 deletions

File tree

  • sdks/unreal/src/SpacetimeDbSdk/Source/SpacetimeDbSdk/Public/DBCache

sdks/unreal/src/SpacetimeDbSdk/Source/SpacetimeDbSdk/Public/DBCache/ClientCache.h

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,20 +120,24 @@ class UClientCache
120120

121121
TSharedPtr<RowType> NewRow = MakeShared<RowType>(Row);
122122

123-
// This is a new row or an update to an existing row that was not deleted.
124123
FRowEntry<RowType>* Entry = Table->Entries.Find(Key);
125124
if (!Entry)
126125
{
127-
// True insert
126+
// True insert — these row-bytes are not cached yet. Either a
127+
// genuinely new row, or the insert half of an update (the
128+
// paired delete of different bytes is tracked in phase 1/3;
129+
// DeriveUpdatesByPrimaryKey reconciles them by PK afterward).
128130
Table->Entries.Add(Key, FRowEntry<RowType>{NewRow, 1});
131+
Diff.Inserts.Add(Key, *NewRow);
129132
}
130133
else
131134
{
132-
// True update
135+
// Refcount bump — an overlapping subscription brought an
136+
// identical row already in cache. Mirror the delete path
137+
// (which only emits Diff.Deletes on refcount == 0) by not
138+
// emitting a spurious Diff.Inserts entry here.
133139
Table->Entries.Add(Key, FRowEntry<RowType>{NewRow, Entry->RefCount + 1});
134140
}
135-
136-
Diff.Inserts.Add(Key, *NewRow);
137141
}
138142

139143
// Phase 3: Finalize Deletes and Update Indices

0 commit comments

Comments
 (0)