Skip to content

fix(audience): atomic ConsentState for Full→Anonymous userId race (SDK-233)#700

Merged
ImmutableJeffrey merged 6 commits into
mainfrom
feat/audience-consent-state
Apr 23, 2026
Merged

fix(audience): atomic ConsentState for Full→Anonymous userId race (SDK-233)#700
ImmutableJeffrey merged 6 commits into
mainfrom
feat/audience-consent-state

Conversation

@ImmutableJeffrey

@ImmutableJeffrey ImmutableJeffrey commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Introduces ConsentState — an immutable record packing (ConsentLevel, UserId). _state is a volatile ConsentState, so a single reference swap publishes both fields atomically and readers never observe (Anonymous, oldUserId).
  • Takes _initLock in Identify, Reset, and SetConsent around the state swap so the pair stays atomic against concurrent callers.
  • Changes EnqueueChecked signature: replaces Func<bool>? stillAllowed with Func<Dictionary<string, object>, Dictionary<string, object>?>? transform. The transform runs under _drainLock and returns the (possibly mutated) msg, or null to drop.
  • Re-reads _state inside _drainLock at enqueue time: EnqueueTrack strips userId when the current level is not Full; EnqueueIdentity drops when the current level is not Full. Closes the race where a Track racing SetConsent(Full → Anonymous) could land an old-userId msg on disk after ApplyAnonymousDowngrade had already rewritten the queue.
  • Adds a 200-iteration × 4-thread stress test: concurrent Tracks racing SetConsent(Full → Anonymous) after Identify, asserting zero userId leaks on disk.
  • Polyfills IsExternalInitrecord { init; } requires it and netstandard2.1 (Unity) does not ship it.

Linear:

  • SDK-233 — primary
  • SDK-143 — contributes thread-safety: atomic ConsentState makes SetConsent callable from any thread

Note

Medium Risk
Touches consent/identity handling and concurrency around event queuing, where mistakes could cause userId leakage or dropped events. Changes are localized and backed by new stress coverage, but affect a privacy-sensitive path.

Overview
Prevents privacy-related race conditions by replacing separate volatile _consent/_userId fields with a single volatile ConsentState record and updating Identify, Reset, Shutdown, ResetState, and SetConsent to swap this state under _initLock.

Hardens queuing against concurrent consent changes by changing EventQueue.EnqueueChecked to accept an under-_drainLock transform (mutate/drop message), and routing Track/Identify/Alias through new EnqueueTrack/EnqueueIdentity helpers that re-read current consent to drop events under None and strip userId when not Full. Adds a stress test covering the Full→Anonymous race and a Unity IsExternalInit polyfill to support the new record type.

Reviewed by Cursor Bugbot for commit a1ee630. Bugbot is set up for automated code reviews on this repo. Configure here.

@ImmutableJeffrey ImmutableJeffrey requested review from a team as code owners April 23, 2026 04:54
@ImmutableJeffrey ImmutableJeffrey force-pushed the feat/audience-consent-state branch 6 times, most recently from 015b13f to 2037716 Compare April 23, 2026 05:22
Comment thread src/Packages/Audience/Runtime/ImmutableAudience.cs Outdated
Comment thread src/Packages/Audience/Runtime/ImmutableAudience.cs Outdated
nattb8
nattb8 previously approved these changes Apr 23, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 08cfe25. Configure here.

Comment thread src/Packages/Audience/Runtime/ImmutableAudience.cs Outdated
ImmutableJeffrey added a commit that referenced this pull request Apr 23, 2026
Fires the fire-and-forget consent PUT after the lock releases. JSON
serialisation, dictionary allocation, and Task.Run scheduling no longer
block concurrent SetConsent / Identify / Reset callers.

Why: the call's three args are already local snapshots and the control
client it reads is not _initLock-protected anyway (Shutdown disposes it
without the lock, relying on _shutdownCancellationSource for safety).

Tests: all 181 audience unit tests pass. No test changes — Identity.Reset
still runs before the PUT, so the revocation regression guard still holds.

Addresses Cursor Bugbot comment on #700.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nattb8
nattb8 previously approved these changes Apr 23, 2026
ImmutableJeffrey and others added 6 commits April 23, 2026 22:08
The prior design had `_consent` and `_userId` as separate volatiles.
SetConsent(Full → Anonymous) wrote them in two steps; a concurrent Track
that read _userId before the null-write and _consent after the flip
could enqueue a userId-bearing track that ApplyAnonymousDowngrade's
one-shot disk rewrite never saw.

- Pack (Level, UserId) as an immutable ConsentState record. A single
  volatile reference swap publishes both together; readers see a
  consistent pair.
- EnqueueChecked takes a transform callback that runs under _drainLock.
  Track's transform re-reads _state and strips userId when the level
  is not Full, serialised against ApplyAnonymousDowngrade.
- Identify, Reset, and SetConsent take _initLock for the state swap
  so writes don't lose the pair's atomicity.
- Identify/Alias enqueue with a gate that drops the event if consent
  has since downgraded below Full.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors SetConsent_DowngradeToNone_StressTest_NoLeak but exercises the
Full → Anonymous path: an Identify-set userId must not leak through
a racing Track past ApplyAnonymousDowngrade.

Without the EnqueueChecked transform (commit c14cd391), this leaks
reproducibly. With the fix, zero leaks across 200 × 4 iterations.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrites jargon-heavy comments (polyfill, atomic reference, release/
acquire, drain-lock, serialise) into plain language.

No code changes. All 181 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dead code: every callsite uses the extension method
ConsentLevel.CanTrack() directly. No call to the private
static CanTrack() remained.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two comments still named the pre-refactor _consent field; one class-level
comment enumerated only Identify / Reset as taking _initLock, which is
now true of every _state write.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fires the fire-and-forget consent PUT after the lock releases. JSON
serialisation, dictionary allocation, and Task.Run scheduling no longer
block concurrent SetConsent / Identify / Reset callers.

Why: the call's three args are already local snapshots and the control
client it reads is not _initLock-protected anyway (Shutdown disposes it
without the lock, relying on _shutdownCancellationSource for safety).

Tests: all 181 audience unit tests pass. No test changes — Identity.Reset
still runs before the PUT, so the revocation regression guard still holds.

Addresses Cursor Bugbot comment on #700.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ImmutableJeffrey ImmutableJeffrey merged commit 3c47cbe into main Apr 23, 2026
19 checks passed
@ImmutableJeffrey ImmutableJeffrey deleted the feat/audience-consent-state branch April 23, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants