feat(experiment-tag): Wire relay iframe on start#335
Open
tyiuhc wants to merge 11 commits into
Open
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unavailable relay leaves hidden iframe
- Added cleanup logic to destroy the RelayClient and set it to null when relay init times out without relayAvailable becoming true, removing the orphaned iframe and message listener.
- ✅ Fixed: Pass two apply errors swallowed
- Separated error handling by catching handleRelayPass2 errors with an inner catch that logs via console.warn, distinguishing Pass 2 failures from relay sync failures.
Or push these changes by commenting:
@cursor push 9d836aae4a
Preview (9d836aae4a)
diff --git a/packages/experiment-tag/src/experiment.ts b/packages/experiment-tag/src/experiment.ts
--- a/packages/experiment-tag/src/experiment.ts
+++ b/packages/experiment-tag/src/experiment.ts
@@ -737,9 +737,23 @@
void this.behavioralTargetingManager
.beginRelaySync(this.relayClient)
- .then((behaviorsChanged) => this.handleRelayPass2(behaviorsChanged))
+ .then((behaviorsChanged) => {
+ // Cleanup if relay init timed out without becoming available
+ if (!this.relayClient?.relayAvailable) {
+ this.relayClient?.destroy();
+ this.relayClient = null;
+ return;
+ }
+ // Pass 2: re-apply variants when behaviors changed
+ return this.handleRelayPass2(behaviorsChanged).catch((pass2Error) => {
+ // Pass 2 failure after successful relay sync - log separately
+ console.warn('Experiment relay Pass 2 failed:', pass2Error);
+ });
+ })
.catch(() => {
- // relay failure is local-only fallback
+ // relay sync failure is local-only fallback - cleanup
+ this.relayClient?.destroy();
+ this.relayClient = null;
});
}You can send follow-ups to the cloud agent here.
4 tasks
tyiuhc
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale relay promise destroys replacement client
- Captured the RelayClient at sync start and added checks in both .then and .catch handlers to ignore completions when this.relayClient has been replaced by a subsequent scheduleRelaySync call.
Or push these changes by commenting:
@cursor push 0d99765367
Preview (0d99765367)
diff --git a/packages/experiment-tag/src/experiment.ts b/packages/experiment-tag/src/experiment.ts
--- a/packages/experiment-tag/src/experiment.ts
+++ b/packages/experiment-tag/src/experiment.ts
@@ -729,17 +729,21 @@
}
this.relayClient?.destroy();
- this.relayClient = new RelayClient(
+ const startedClient = new RelayClient(
this.apiKey,
webExpIdV2,
getRelayUrl(this.apiKey),
);
+ this.relayClient = startedClient;
void this.behavioralTargetingManager
- .beginRelaySync(this.relayClient)
+ .beginRelaySync(startedClient)
.then((behaviorsChanged) => {
- if (!this.relayClient?.relayAvailable) {
- this.relayClient?.destroy();
+ if (this.relayClient !== startedClient) {
+ return;
+ }
+ if (!startedClient.relayAvailable) {
+ startedClient.destroy();
this.relayClient = null;
return;
}
@@ -748,7 +752,10 @@
});
})
.catch(() => {
- this.relayClient?.destroy();
+ if (this.relayClient !== startedClient) {
+ return;
+ }
+ startedClient.destroy();
this.relayClient = null;
});
}You can send follow-ups to the cloud agent here.
Inject RelayClient from experiment.ts when RTBT rules are present: non-blocking beginRelaySync after Pass 1 applyVariants, Pass 2 re-apply when behaviors change (storage sync activates with #334). Co-authored-by: Cursor <cursoragent@cursor.com>
Destroy the RelayClient when relay stays unavailable after init, and log Pass 2 re-apply failures separately from relay sync errors. Co-authored-by: Cursor <cursoragent@cursor.com>
…-130) Tie beginRelaySync completion to the RelayClient instance that started it so a replaced client is not destroyed by an older promise. Co-authored-by: Cursor <cursoragent@cursor.com>
Relay sync only requires a non-empty web_exp_id_v2 or web_exp_id. Co-authored-by: Cursor <cursoragent@cursor.com>
tyiuhc
force-pushed
the
web/rtbt-relay-iframe
branch
from
c091003 to
c13dbe1
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Late relay ready skips sync
- Added logic in scheduleRelaySync to call setRelayClient and syncFromRelay when relayAvailable becomes true after beginRelaySync returned false due to init timeout.
Or push these changes by commenting:
@cursor push 9c02dbe5d1
Preview (9c02dbe5d1)
diff --git a/packages/experiment-tag/src/experiment.ts b/packages/experiment-tag/src/experiment.ts
--- a/packages/experiment-tag/src/experiment.ts
+++ b/packages/experiment-tag/src/experiment.ts
@@ -737,7 +737,7 @@
void this.behavioralTargetingManager
.beginRelaySync(relayClient)
- .then((behaviorsChanged) => {
+ .then(async (behaviorsChanged) => {
if (this.relayClient !== relayClient) {
return;
}
@@ -746,6 +746,13 @@
this.relayClient = null;
return;
}
+ // Late relay ready: beginRelaySync returned false because init() timed
+ // out, but the relay became available afterward. Perform the skipped setup.
+ if (!behaviorsChanged && this.behavioralTargetingManager) {
+ this.behavioralTargetingManager.setRelayClient(relayClient);
+ behaviorsChanged =
+ await this.behavioralTargetingManager.syncFromRelay();
+ }
return this.handleRelayPass2(behaviorsChanged).catch((pass2Error) => {
console.warn('Experiment relay Pass 2 failed:', pass2Error);
});You can send follow-ups to the cloud agent here.
Co-authored-by: Cursor <cursoragent@cursor.com>
Add waitForAvailable on RelayClient and attach storage before init so Pass 2 sync runs when the relay iframe becomes ready after the init timeout. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Orphan relay after sync abort
- Added relayClient.destroy() call in beginRelaySync when syncFromRelay returns false to properly clean up the orphaned iframe and message listener.
Or push these changes by commenting:
@cursor push dea245079c
Preview (dea245079c)
diff --git a/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts b/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts
--- a/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts
+++ b/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts
@@ -100,6 +100,7 @@
const synced = await this.syncFromRelay();
if (!synced) {
this.setRelayClient(null);
+ relayClient.destroy();
return false;
}
return behaviorsBefore !== this.serializeMatchedBehaviors();You can send follow-ups to the cloud agent here.
Keep pending writes until RPC confirms and retain relay client on sync failure. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Relay not cleared on sync failure
- Added setRelayClient(null) call in the catch block of scheduleRelaySync to clear the relay reference from behavioral targeting manager before destroying the client, preventing event storage from holding a destroyed client.
Or push these changes by commenting:
@cursor push edffb1cacc
Preview (edffb1cacc)
diff --git a/packages/experiment-tag/src/experiment.ts b/packages/experiment-tag/src/experiment.ts
--- a/packages/experiment-tag/src/experiment.ts
+++ b/packages/experiment-tag/src/experiment.ts
@@ -754,6 +754,7 @@
if (this.relayClient !== relayClient) {
return;
}
+ this.behavioralTargetingManager?.setRelayClient(null);
relayClient.destroy();
this.relayClient = null;
});You can send follow-ups to the cloud agent here.
Typed RelaySyncResult, defer storage attachment until after init/sync, centralize teardownRelay, dedupe pending writes, and add event-storage relay stubs until the storage sync PR lands. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Confirmed writes use reference equality
- Changed removeConfirmedWrite from indexOf (reference equality) to findIndex with value equality on id, event_type, and timestamp, matching the deduplication logic in writeEvent.
Or push these changes by commenting:
@cursor push 64f81ac14c
Preview (64f81ac14c)
diff --git a/packages/experiment-tag/src/behavioral-targeting/relay-client.ts b/packages/experiment-tag/src/behavioral-targeting/relay-client.ts
--- a/packages/experiment-tag/src/behavioral-targeting/relay-client.ts
+++ b/packages/experiment-tag/src/behavioral-targeting/relay-client.ts
@@ -245,7 +245,12 @@
}
private removeConfirmedWrite(event: RelayEventRecord): void {
- const idx = this.pendingWrites.indexOf(event);
+ const idx = this.pendingWrites.findIndex(
+ (queued) =>
+ queued.id === event.id &&
+ queued.event_type === event.event_type &&
+ queued.timestamp === event.timestamp,
+ );
if (idx !== -1) {
this.pendingWrites.splice(idx, 1);
}You can send follow-ups to the cloud agent here.
Match pending write removal by event id, settle waitForAvailable on destroy, and clear storage when replacing relay via teardownRelay. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Relay attached after failed sync
- Moved setRelayClient call after the synced check so relay is only attached after a successful sync operation.
- ✅ Fixed: Stale client attached after teardown
- Added relayAvailable recheck after the async syncFromRelay call to ensure the client wasn't torn down during the operation.
Or push these changes by commenting:
@cursor push 46a6ea61b8
Preview (46a6ea61b8)
diff --git a/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts b/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts
--- a/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts
+++ b/packages/experiment-tag/src/behavioral-targeting/behavioral-targeting-manager.ts
@@ -92,12 +92,19 @@
}
const synced = await this.eventStorage.syncFromRelay(relayClient);
- this.setRelayClient(relayClient);
+ // Verify relay is still available after async sync (could be torn down by newer sync)
+ if (!relayClient.relayAvailable) {
+ return { status: 'unavailable' };
+ }
+
if (!synced) {
return { status: 'sync_failed' };
}
+ // Only attach relay client after successful sync to avoid dual-write during failed merge
+ this.setRelayClient(relayClient);
+
this.evaluateAll();
return behaviorsBefore !== this.serializeMatchedBehaviors()
? { status: 'behaviors_changed' }You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 6794a74. Configure here.
…aySync beginRelaySync now only reads/merges relay state and reports the outcome; it no longer attaches the relay for dual-write. The orchestrator attaches via setRelayClient only when sync succeeds and this client is still the active one, so a failed sync or a client superseded by a newer scheduleRelaySync is never attached. Removes the now-dead no-arg syncFromRelay wrapper. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacks on #333 (
web/rtbt-relay-client). Injects the CDN relay iframe from the live SDKstart()path when RTBT rules are present.experiment.ts:scheduleRelaySync()after Pass 1applyVariants(non-blocking); createsRelayClient(apiKey, webExpIdV2, getRelayUrl(apiKey)); Pass 2 re-apply viahandleRelayPass2when behaviors changeBehavioralTargetingManager:beginRelaySync()—relay.init(), attach client, runsyncFromRelaywhen #334 storage hooks landWebExperimentUser: optionalweb_exp_id_v2field (usesweb_exp_idfallback until feat(experiment-tag): add cross-subdomain web_exp_id_v2 and first_seen cookies #332 cookie resolve lands)#334 (
web/rtbt-use-relay) stacks on this branch for event storage dual-write.Test plan
npx jest test/behavioral-targeting/behavioral-targeting-manager-relay.test.tsnpx jest test/experiment-relay-iframe.test.tsnpx jest test/behavioral-targeting/relay-client.test.ts(base feat(experiment-tag): add RTBT relay protocol and RelayClient #333)Merge order
web/rtbt-relay-client— RelayClient foundation)web/rtbt-use-relay— event storage)Note
Medium Risk
Pass 2 can refetch remote flags and re-apply web variants after initial render, which affects experiment delivery timing; wiring is async with teardown guards but still touches core start() and bucketing inputs.
Overview
When behavioral targeting rules exist,
start()now kicks off a non-blocking CDN relay iframe sync after Pass 1 variant application (local + remote paths).scheduleRelaySyncbuilds aRelayClientfromweb_exp_id_v2(falling back toweb_exp_id), runsbeginRelaySync(iframe init, wait for ready, merge via storage hook), and only then attaches the client for future dual-write. Failed or unavailable relays are torn down; stale clients are ignored if sync is superseded.If merged relay state changes matched behaviors, Pass 2 updates
behavioral_targetingon the user, re-fetches remote flags when needed, and re-applies variants for RTBT-affected flags.BehavioralTargetingManagerexposessetRelayClient/beginRelaySyncand returns aRelaySyncResult;EventStorageManageradds stubsetRelayClient/syncFromRelayuntil the storage dual-write PR lands.Reviewed by Cursor Bugbot for commit 324117a. Bugbot is set up for automated code reviews on this repo. Configure here.