Skip to content

fix(unreal): suppress spurious OnInsert on overlapping subscription refcount bump#4903

Open
SamuelE-Arvika wants to merge 1 commit intoclockworklabs:masterfrom
SamuelE-Arvika:fix/unreal-sdk-overlapping-sub-double-insert
Open

fix(unreal): suppress spurious OnInsert on overlapping subscription refcount bump#4903
SamuelE-Arvika wants to merge 1 commit intoclockworklabs:masterfrom
SamuelE-Arvika:fix/unreal-sdk-overlapping-sub-double-insert

Conversation

@SamuelE-Arvika
Copy link
Copy Markdown
Contributor

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:

// 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:

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.

  • Reviewer: confirm an existing real-update (different bytes, same PK) test still produces OnUpdate and not OnInsert+OnDelete.
  • 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.

…efcount bump

ClientCache::ApplyDiff phase 2 unconditionally appended every incoming
row to Diff.Inserts, even when the row was already cached and only the
refcount needed to bump. BroadcastDiff then fired OnInsert for every
entry, so any table subscribed via two overlapping queries (e.g. a
global SELECT plus a per-row WHERE) re-fired its insert handler on
every later subscription apply.

Mirror the delete path: only emit a Diff.Inserts entry when the
row-bytes key transitions from absent to refcount=1. Real updates are
unaffected — update bytes differ from old bytes, take the !Entry path,
and DeriveUpdatesByPrimaryKey still reconciles them into OnUpdate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants