fix: wait for pending user updates before evaluating targeting [ENG-1817] - #56
Merged
Dhruwang merged 2 commits intoSep 15, 2026
Conversation
…817] `setAttribute(...)` immediately followed by `track(...)` evaluated targeting against the segment membership the contact had *before* the write. Attribute updates are debounced ~0.5s while `track()` read the cached `filteredSurveys` straight away, so the survey the host was trying to trigger did not show. A first `setUserId` was worse: the id is not persisted until the response lands, so `filterSurveys()` still saw an anonymous user and dropped every segment-targeted survey. `track()` now resolves the action class first — that comes from workspace state, so a typo'd action still fails fast without a round trip — then waits for the queue to settle before picking a survey. If the update did not land, surveys with segment filters are skipped rather than shown on stale membership; surveys without them are unaffected. Mirrors what the JS SDK's command queue does for `CommandType.GeneralAction`. The wait is callback-based, not blocking. The debounce `Timer` lives on the main run loop and `track()` is normally called from the main thread, so a semaphore there would stall the run loop the timer needs and guarantee a timeout. Three further bugs had to be fixed for the wait to mean anything: - Writes made while a request was out were silently lost. The values stayed queued during the request and the success path cleared them wholesale, so the later commit sent nothing. `commit()` now moves its values out of the queue, and a failure hands them back. - `commit()` had no in-flight guard — only `requestUserStateRefresh` did — so a mid-flight `setAttribute` fired a second, racing `POST /user`. It is now deferred and re-armed once the first settles. - After that move the queue no longer held the user id, leaving a follow-up commit without an identity. `syncDidFinish` restores it for the follow-up. A failure deliberately does not re-arm the debounce timer: retrying itself would turn a dead network into a request every half second, and `UserManager.scheduleSyncRetry()` already owns that backoff.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The wait held `track()` until the queued update landed, and then evaluated against the survey list computed from the user state that predated it — so a `setUserId` immediately followed by `track()` still showed nothing, which is the bug the wait was added to fix. `syncDidFinish` resolves its waiters onto the main queue while `syncUser`'s completion runs on URLSession's background queue, so releasing a waiter before `filterSurveys()` let it read `filteredSurveys` while the re-filter was still running. `filterSurveys()` never won that race either: the `displays` getter decodes JSON out of UserDefaults on every access, so it lost reliably rather than intermittently. Re-filter first, release the waiters after. That also gives the main queue a happens-before edge on the write, which matters because `filteredSurveys` has no synchronisation of its own.
|
Dhruwang
approved these changes
Sep 15, 2026
Merged
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
2 participants
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.
Ref ENG-1817
What & why
Was:
setAttribute("plan", "pro")immediately followed bytrack("...")evaluated targeting against the segment membership the contact had before the write, so the survey the host was trying to trigger did not show. Attribute writes are debounced ~0.5s, whiletrack()read the cachedfilteredSurveysstraight away. A firstsetUserIdwas worse — the id is not persisted until the response lands, so the contact still looked anonymous and every segment-targeted survey was dropped.Now:
track()waits for the queued update to land, then picks a survey. If the update fails, surveys with segment filters are skipped rather than shown on stale membership; surveys without them are unaffected.Reported by the SJ app team. Mirrors what the JS SDK's command queue already does for
CommandType.GeneralAction.The wait is callback-based, not blocking: the debounce
Timeris installed on the main run loop andtrack()is normally called from the main thread, so a semaphore there would stall the run loop the timer needs and guarantee a timeout.Where to look
UpdateQueue.waitForPendingWork— the wait, its anonymous-user short circuit, and the timeout.UpdateQueue.commit/syncDidFinish— values now move out of the queue, plus the in-flight guard.SurveyManager.track/evaluate— the split, and why theisShowingSurveylatch is claimed before the wait.Three further bugs this had to fix first
commit()now moves its values out; a failure hands them back.commit()had no in-flight guard — onlyrequestUserStateRefreshdid — so asetAttributeduring a request fired a second, racingPOST /user. Now deferred and re-armed.syncDidFinishrestores the user id for it. Caught bytestAttributeSetDuringAnInFlightSyncIsStillSentagainst my own first cut.A failure deliberately does not re-arm the debounce timer — self-retrying would turn a dead network into a request every half second, and
UserManager.scheduleSyncRetry()already owns that backoff.Coverage
setUserId()→track()back-to-back shows a segment-targeted surveyPOST /userelse if hasNewWorkbranch inUpdateQueue.syncDidFinishto turn it redguard !isSyncInFlightinUpdateQueue.commitRerun:
xcodebuild test -scheme FormbricksSDK -destination 'platform=iOS Simulator,name=iPhone 17,OS=latest' -only-testing:FormbricksSDKTests/UpdateQueueTestsFull suite: 120 tests, 0 failures.
Open gaps
No
unit (red on main)row:waitForPendingWorkandsyncDidFinish(success:)do not exist on main, so these tests cannot compile against it. The end-to-end case is covered manually instead — the automated version needsSurveyManagerdriven with a seeded workspace and a mock present manager, and is not in this PR.The race is masked on a loopback server.
track()spends ~700ms in a freshNWPathMonitorbefore it evaluates anything, while a 50ms localhostPOST /userlands in ~550ms including the debounce — so the update wins by accident and the bug is invisible. Reproducing it needs latency injected into the route (1.5s was enough); against a real server the update loses on its own.Worth stating plainly: for the
setAttributepath this PR makes the ordering deterministic rather than fixing a reliably-reproducible failure. ThesetUserIdpath does fail outright — the id is not persisted until the response lands, so the contact reads as anonymous until then and every segment-targeted survey is dropped.Still unverified: the failure path (server down → segment-targeted survey skipped, unsegmented one still shows), coalescing into a single request, and the double-track latch.
A second bug the first commit introduced
Releasing the waiter before
filterSurveys()let it read the survey list computed from the previous user state, sosetUserId→track()still showed nothing: the wait worked, and everything after it read stale data anyway.filterSurveys()runs first now, which also gives the main queue a happens-before edge on the write.Found by manual testing, not by the unit tests — they never exercise that ordering. Related:
filteredSurveyshas no synchronisation at all, written on URLSession's background queue and read on the main queue.Breaking changes
No public API changes.
track()keeps its signature; it can now take longer to return control before a survey appears, bounded byConfig.User.pendingUpdateTimeoutInSeconds(5s).Agent:
claude-opus-5(Claude Code), reasoningunknown.