Skip to content

Port traffic-source sanitization and queue fixes from the web SDK - #83

Merged
yosriady merged 12 commits into
mainfrom
fix/port-web-sdk-attribution-and-queue-fixes
Aug 10, 2026
Merged

Port traffic-source sanitization and queue fixes from the web SDK#83
yosriady merged 12 commits into
mainfrom
fix/port-web-sdk-attribution-and-queue-fixes

Conversation

@yosriady

@yosriady yosriady commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Ports three fixes that landed in getformo/sdk over the last two weeks and apply equally here, plus one bug found while verifying them.

What's here

1. Sanitize traffic-source values — sdk#307

parseTrafficSource captured utm_* and ref verbatim, persisted them as sticky session attribution, and replayed them onto every subsequent event. No validation anywhere.

On mobile the hostile input isn't a scanner crawling a website — it's:

  • Deep links. Anyone who can get a user to open myapp://x?utm_source=<script>alert(1)</script> controls the value, and it sticks for the whole session.
  • The Android Play install referrer, which is derived from the referrer parameter of a Play Store URL and is equally attacker-supplied.

New src/utils/sanitize.ts drops values that can't be legitimate — a strict token allowlist for ref, and markup/quote/control-character plus dangerous-scheme rejection for utm_*. It runs on both the fresh extraction and the stored replay, so values persisted by a pre-fix build get flushed rather than re-emitted.

Two deliberate differences from the web version, both documented in the file:

  • No click-ID rule. This SDK doesn't capture gclid/fbclid/etc., so that rule would be dead code. Noted inline for whoever adds click-ID capture later.
  • referrer is sanitized, where web leaves it alone. Web's referrer is a browser-set document.referrer; here it holds the raw deep-link URL the attacker supplied, so scrubbing only utm_*/ref would leave the payload sitting in referrer.

2. Flush the first event of an app session immediately — sdk#326

The web fix un-deadened a branch that already existed. There was no such branch here at all, so this is new behavior rather than a revival.

A cold start opened from an ad click or deep link now ships its attribution events as a batch of one, instead of holding them for flushAt or the 30s timer. Those are exactly the events lost to a force-quit from the app switcher, an OS memory kill, or a crash — none of which give AppState a chance to report background. Everything after the first event keeps batching normally.

The same commit's callback hardening comes along: flush() invoked consumer callbacks bare, so one throwing callback escaped the SDK and skipped the rest of the batch's callbacks. They now go through safeCall.

3. Verify src/version.ts against package.json in CI — sdk#321

Same generated-file setup that let four web releases ship reporting 1.30.1. In sync today at 1.0.2, but the value is compiled into the bundle and sent as library_version on every event, so silent drift costs version visibility for the entire release.

4. Fix an unhandled rejection from the interval flush

Found while running the examples app against a deliberately failing collector. The batch timer passed flush to setTimeout bare, and flush() rethrows once sendWithRetry is exhausted — so every failed interval flush surfaced in the host app as Uncaught (in promise). The threshold and background paths already logged and swallowed; this one now does too.

Pre-existing and not part of the ports, but it lives in the same file and the same failure paths this PR is already touching, so it's included rather than deferred.

5. Unblock the audit job — pnpm-workspace.yaml

Unrelated to the ports, but the audit job started failing on this PR and would fail on any open PR (CI only runs on pull_request, so there is no main run to catch it).

Two new image-size advisories (GHSA-w3rx-r6r6-pgpr, GHSA-5p2g-fcmc-qvqq) both name >=2.0.3 as patched — but 2.0.3 has never been published. npm's latest is 2.0.2, which is itself inside the vulnerable <=2.0.2 range, so an overrides entry would be unresolvable. There is no fix to take, breaking or otherwise.

image-size reaches the graph only as metro > image-size (metro pins ^1.0.2, resolving 1.2.1) — the bundler's asset pipeline, which is build tooling and never ships in the SDK's runtime output. Both advisories are decode-time infinite loops on malformed ICNS/JXL/HEIF input, so reaching one would mean bundling a hostile image at build time.

This matches the audit policy already recorded in that file and the existing @babel/core entry's rationale. Scoped to these two GHSAs so nothing else in the prod graph is masked — pnpm audit --prod now reports 2 high (2 ignored) and exits 0. Easy to split out into its own PR if you'd rather keep this one pure.

Worth a separate look: @react-native-community/netinfo is the only hard dependency here, and pnpm walks its react-native peer, which is what drags the whole Metro toolchain into a --prod audit. Every other native module in this SDK (react-native-device-info, expo-device, expo-application, react-native-play-install-referrer) is already an optional peer loaded via require in a try/catch; NetInfo is the lone static import. Making it an optional peer too would take build tooling out of the prod graph entirely and likely retire several existing overrides — but it changes what consumers must install, so I left it alone here.

Not ported

Testing

Unit: 331 pass (300 before). Typecheck and lint clean. The interval-flush test was confirmed to fail with the fix reverted.

End-to-end against examples/with-react-native under Metro, with apiHost pointed at a local collector so the exact wire payloads could be asserted on rather than inferred from logs:

Scenario Result
Poisoned deep link utm_source, utm_campaign, ref dropped from the wire; legitimate utm_medium=cpc and utm_term="spring shoes" preserved
Clean marketing link all six values arrive intact — no false positives
Cold start batching first batch on the wire is exactly 1 event despite flushAt: 10, followed by batches of 4 and 2
Collector returning 500 both Failed to flush on threshold and Failed to flush on interval log; zero Uncaught (in promise) in a clean console

Reviewer notes

  • referrer still carries the landing URL as received, including any percent-encoded payload in its query string. This is intentional and matches the web SDK, whose redactUrl only strips denylisted params and likewise doesn't scrub the referrer. The sanitizer's job is the extracted attribution fields; RN is in fact slightly stricter than web here, since it rejects raw markup and dangerous schemes in referrer that web never checks.
  • Invalid values become "", not undefined — the same representation as "parameter absent", so they lose the per-field last-touch merge in updateStoredTrafficSource and never overwrite a previously captured clean value.
  • Separately, jest-expo ships a custom resolver that overrides moduleNameMapper, so require("@formo/analytics-react-native") under that preset doesn't expose FormoAnalytics. Identical on the published 1.0.2 build and unrelated to this PR, but it does mean consumers can't easily unit-test against the SDK under jest-expo — worth its own look.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Review in cubic

Brings across three fixes that landed in getformo/sdk over the last two
weeks and apply equally here, plus one bug found while verifying them.

Sanitize traffic-source values (web #307)

parseTrafficSource captured utm_* and ref verbatim, persisted them as
sticky session attribution, and replayed them onto every subsequent
event. On mobile the hostile input is not a scanner crawling a website:
anyone who can get a user to open myapp://x?utm_source=<script>… controls
the value, and the Android Play install referrer is derived from an
attacker-supplied Play Store URL.

utils/sanitize.ts drops values that cannot be legitimate — a strict token
allowlist for ref, and markup/quote/control-character plus dangerous-scheme
rejection for utm_*. It runs on both the fresh extraction and the stored
replay, so values persisted by a pre-fix build are flushed rather than
re-emitted. Two deliberate differences from the web version, documented in
the file: no click-ID rule (this SDK does not capture gclid/fbclid), and
referrer IS sanitized, because here it holds the raw deep-link URL rather
than a browser-set document.referrer.

Flush the first event of an app session immediately (web #326)

The web fix un-deadened an existing branch; there was no such branch here
at all. A cold start opened from an ad click or deep link now ships its
attribution events as a batch of one instead of holding them for flushAt
or the 30s timer — exactly the events lost to a force-quit, an OS memory
kill, or a crash, none of which give AppState a chance to report
background. Subsequent events keep batching.

The same commit's callback hardening comes along: flush() invoked consumer
callbacks bare, so one throwing callback escaped the SDK and skipped the
rest of the batch's callbacks. They now go through safeCall.

Verify src/version.ts against package.json in CI (web #321)

Same generated-file setup that let four web releases ship reporting
1.30.1. In sync today at 1.0.2; now it cannot drift silently, since the
value is compiled into the bundle as library_version on every event.

Fix an unhandled rejection from the interval flush

Found running the examples app against a failing collector. The batch
timer passed flush to setTimeout bare, and flush() rethrows once
sendWithRetry is exhausted, so every failed interval flush surfaced in the
host app as "Uncaught (in promise)". The threshold and background paths
already logged and swallowed; this one now does too.

Verified end-to-end against examples/with-react-native under Metro, with
events pointed at a local collector so the exact wire payloads could be
asserted on:

  - poisoned deep link -> utm_source/utm_campaign/ref dropped from the
    wire, legitimate utm_medium=cpc and utm_term="spring shoes" preserved
  - clean marketing link -> all six values arrive intact, no false
    positives
  - first batch on the wire contains exactly 1 event despite flushAt: 10,
    with following batches of 4 and 2
  - against a 500, both "Failed to flush on threshold" and "Failed to
    flush on interval" log and no unhandled rejection reaches the app

331 unit tests pass (300 before), typecheck and lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@yosriady
yosriady force-pushed the fix/port-web-sdk-attribution-and-queue-fixes branch from 765d1b1 to 10673c7 Compare August 10, 2026 02:31
@yosriady

Copy link
Copy Markdown
Contributor Author

@codex review

GHSA-w3rx-r6r6-pgpr and GHSA-5p2g-fcmc-qvqq started failing the audit job.
Both name ">=2.0.3" as the patched range, but that version has never been
published: npm's latest image-size is 2.0.2, which is itself inside the
vulnerable "<=2.0.2" range. An overrides entry would be unresolvable, so
there is no fix to take, breaking or otherwise.

image-size reaches the graph only as metro > image-size (metro pins ^1.0.2,
resolving 1.2.1) — the bundler's asset pipeline, which is build tooling and
never ships in the SDK's runtime output. Both advisories are decode-time
infinite loops on malformed ICNS/JXL/HEIF input, so reaching them would mean
bundling a hostile image at build time.

This is the same situation the existing @babel/core entry documents, and it
follows the audit policy already recorded in this file: pin to a patched
version where one exists, ignore build-tooling advisories that have no
non-breaking fix. Scoped to these two GHSAs, so nothing else in the prod
graph is masked — `pnpm audit --prod` now reports "2 high (2 ignored)" and
exits 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 10673c71b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib/event/EventQueue.ts
Addresses Codex review feedback on #83.

The first-event branch returns before the timer-arming block, and flush()
clears the timer on entry. So when that immediate flush exhausted its
retries, the items were unshifted back onto the queue with nothing
scheduled to retry them: a foregrounded app that emitted no further event
stranded them until the next enqueue or a background transition, neither
of which is guaranteed.

That is the worst case for this particular flush. Sending the first event
immediately means it goes out during cold start, when the radio may still
be waking, a VPN reconnecting, or a captive portal intercepting — so it is
the attempt most likely to fail, carrying the attribution event the
immediate flush exists to protect.

The timer-arming logic moves into scheduleFlush(), which no-ops when there
is nothing queued or a timer is already pending, and both failure paths now
call it. Retries become self-sustaining: a queue that outlives a transient
outage drains on the next interval once connectivity returns.

The same gap existed for the flushAt/maxQueueSize thresholds before this
branch was added; routing every session's first event through it is what
made it likely rather than theoretical.

Both new tests fail without the re-arm and pass with it. 333 pass (331
before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@yosriady

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d23f154146

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib/event/EventQueue.ts Outdated
Comment thread src/lib/event/EventQueue.ts
Addresses two Codex review findings on #83, both fallout from re-arming the
interval timer in d23f154.

Permanent send failures (P1)

sendWithRetry deliberately does not retry a 4xx other than 429 — an invalid
write key or a malformed batch — but flush() re-queued the items regardless,
and the new re-arm then scheduled another attempt. The same rejected payload
would be re-posted every interval for the lifetime of the process,
re-invoking its callbacks and burning the user's battery and data each time.
This was visible in the earlier examples-app run against the live API, where
a placeholder write key returned 403 on every attempt.

sendWithRetry now tags the error with whether another attempt could ever
succeed, and flush() drops a batch marked retryable: false, releasing its
payload hashes so equivalent events are not blocked from being enqueued
later. Unexpected errors leave the flag undefined and are treated as
retryable, so an unrecognised fault never silently discards events. Network
errors and 5xx/429 keep the existing re-queue behaviour. This also moves
closer to the web SDK, which does not re-queue a failed batch at all.

Cleanup versus an in-flight flush (P2)

flush() splices its items out of the queue before sending, so a cleanup that
began while the immediate first-event flush was still in flight saw an empty
queue, skipped its drain loop, and returned without waiting. The flush could
then fail, re-queue, and arm a timer on an instance that had already been
torn down — firing network calls after cleanup.

cleanup() now sets a closed flag that scheduleFlush() honours, clears the
timer up front, and awaits the flush mutex so an in-flight send settles
before the drain loop measures what is left.

Verified end-to-end against examples/with-react-native with a collector
returning 400 on every request: three POSTs, each carrying distinct new
events, then silence — the batches are dropped ("Dropping N event(s),
permanently rejected by the API") rather than re-posted on a loop, and no
unhandled rejection reaches the app.

Each new test fails with its corresponding fix reverted. 336 pass (333
before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@yosriady

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9a6c27046

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib/event/EventQueue.ts
Comment thread src/lib/event/EventQueue.ts Outdated
Seven rounds of adversarial review over the queue and sanitizer. Each fix
below has a regression test that was confirmed to fail with that fix — and
only that fix — reverted.

Sanitizer

The referrer rule only caught RAW markup, but `Linking` and the browser
percent-encode, so the realistic attack sailed through: it was close to a
no-op. Values are now compared decoded, and the decoder had to get several
things right before that was true.

- Decode to a fixed point rather than a set number of layers. A fixed cap is
  reachable: re-encoding `javascript:alert(1)` grows its single `%` by two
  characters a layer, so 66 layers fit in 151 characters. The bound is now
  the input length, which is always sufficient because every productive pass
  turns three characters into one — that also proves termination.
- Decode runs of consecutive escapes together. Byte-wise decoding can never
  reconstruct a multi-byte character, so `%E3%80%80javascript%3A...` never
  produced the U+3000 that lets the anchored scheme rule match.
- Fall back to the largest decodable group when a run is not valid UTF-8.
  Otherwise a single `%C0` beside a `%3C` hid the `<` behind it.
- Never let a malformed escape abort the decode. `decodeURIComponent` rejects
  the whole string on one stray `%`, which skipped every decoded check.
- Test each separator-delimited segment for a dangerous scheme, not just the
  whole value. The rule is anchored, so an encoded `=` in
  `?utm_source%3Djavascript%253Aalert(1)` hid the payload mid-string.
- Inspect query keys as well as values: a parameter with no `=` parses
  entirely as a key.
- Apply the decoded checks to utm_* too, not only referrer. URLSearchParams
  strips one layer, so a twice-encoded payload arrived still encoded and
  passed a raw-only test — a bypass of the primary defence.

The decoded form is checked against a narrower character set than the raw
form, so an encoded quote in `?q=%22running%20shoes%22` is still kept.

Queue

- Do not resurrect events cleared mid-flush. flush() splices its batch out
  before sending, so clear() during opt-out could not see it and a later
  failure unshifted it back — delivering events after consent was withdrawn.
  A generation counter now invalidates those batches, and guards the success
  path too, where deleting hashes would have stripped the dedup entry of an
  identical event enqueued since.
- Abort retries whose events were cleared during backoff. Retries span
  seconds, so opting out between attempts previously still posted the next.
- Drop events enqueued across a clear() or cleanup(). enqueue() suspends on
  the async message-id hash; a caller that did not await it could resume
  after teardown and fire a request on a dead instance.
- Ignore AppState transitions once cleanup starts, and detach the listener up
  front, so a backgrounding app cannot queue a flush that outlives teardown.
- Re-arm from flush()'s own catch instead of each caller's, so every entry
  point is covered — the background flush cleared the interval on entry and
  left nothing scheduled, stranding its events.
- Retry 408. It is transient, and dropping non-retryable statuses made it
  silent loss.
- Catch rejected async callbacks. safeCall only caught synchronous throws.

363 tests pass (331 before), typecheck, lint and `pnpm audit --prod` clean.

Two findings were deliberately not acted on, both recorded for follow-up:
rejecting apostrophes and `javascript:`-prefixed campaign text is inherited
verbatim from the web SDK, and diverging here alone would make the two SDKs
report different values for the same campaign; and HTML entities / %uXXXX
are not decoded by any URL consumer and do not yield executable markup, so
covering them is an arms race with no stopping point.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@yosriady

Copy link
Copy Markdown
Contributor Author

@codex review

Context for this round: seven local review rounds have already been applied to the sanitizer and the event queue. Every fix has a regression test verified to fail with that fix — and only that fix — reverted. 363 tests pass (331 at the first review), typecheck, lint and pnpm audit --prod clean.

Two findings were deliberately not acted on, both recorded as follow-ups rather than oversights:

  1. FORBIDDEN_CHARS rejects apostrophes, and FORBIDDEN_SCHEME_PREFIX rejects campaign text beginning with javascript:/data:. Real false positives ("Mother's Day", "JavaScript: Beginner Course"), but these character rules are inherited verbatim from the Formo web SDK. Diverging here alone would make web and React Native report different values for the same campaign, so it wants a coordinated change across both.
  2. HTML entities and %uXXXX escapes. Neither is decoded by decodeURIComponent, URLSearchParams or any standard URL consumer, and entity-decoded < renders as text rather than markup, so there is no exploit path an input sanitizer should be closing. Covering every theoretical downstream encoding has no stopping point.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: afe6cff2a5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

yosriady and others added 2 commits August 10, 2026 11:42
Addresses Codex review feedback on #83.

cleanup() awaits the flush mutex so an in-flight batch — whose items are
already spliced out of the queue — settles before the drain loop measures
what is left. That wait was unbounded, and React Native's fetch has no
request timeout, so a connection that never settles hung teardown forever.

The consequence reaches further than a stuck cleanup():
FormoAnalyticsProvider awaits the pending cleanup before constructing a
replacement instance (FormoAnalyticsProvider.tsx), so the SDK could never be
reconfigured again for the life of the process.

The wait is now capped at 5s. Skipping straight to the drain loop on timeout
would not have helped — flush() awaits the same stalled mutex and would hang
identically — so a timed-out wait abandons the queued events and finishes
teardown instead, which matches how the existing safety-limit path gives up.

The regression test stalls fetch with a promise that never settles; it hits
Jest's timeout with the bound removed and completes with it in place.

364 tests pass (363 before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to 741333d, which bounded cleanup()'s wait but emptied the queue
by hand instead of going through clear().

Only clear() bumps the generation, and that bump is what actually
invalidates a batch still held by a stalled flush. Without it:

  flush A splices event 1 and stalls mid-send. flush B queues behind it on
  the mutex. cleanup() waits, times out after 5s, empties the queue and
  returns. A finally fails retryably, sees an unchanged generation, and
  unshifts event 1 back onto the queue teardown just emptied. B then wakes,
  finds it, sends it, and invokes its callback a second time — after
  cleanup() had already resolved and the provider had moved on to a
  replacement instance.

Both discard paths in cleanup() now call clear(), so the stalled flush takes
its discard branch and the chained flush finds nothing to send.

The regression test stalls the first send, queues a second flush behind it,
lets cleanup time out, and only then fails the stalled request: the event's
callback fires twice without this fix and once with it.

365 tests pass (364 before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@yosriady

Copy link
Copy Markdown
Contributor Author

@codex review

Two commits have landed since the last pass, which reviewed afe6cff:

  • 741333d bounds cleanup()'s wait on an in-flight flush at 5s. React Native's fetch has no request timeout, so a stalled connection hung teardown forever — and FormoAnalyticsProvider awaits the pending cleanup before constructing a replacement instance, so the SDK could never be reconfigured again.
  • 124347b fixes a defect in that commit, found reviewing it: on timeout it emptied the queue by hand rather than via clear(), so the generation counter was never bumped. The stalled flush could then fail, unshift its batch back onto the emptied queue, and a flush chained behind it would send those events and re-invoke their callbacks after cleanup() had already resolved. Both discard paths now route through clear().

Worth a close look at cleanup() and its interaction with a stalled or chained flush specifically, since that is where the last two defects were.

365 tests pass (331 at the first review); typecheck, lint and pnpm audit --prod clean. The two previously deferred items are unchanged: the apostrophe / javascript:-prefix character rules are inherited verbatim from the web SDK and want a coordinated change across both, and HTML entities / %uXXXX are not decoded by any URL consumer and do not produce executable markup.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 124347b30f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib/event/EventQueue.ts Outdated
yosriady and others added 5 commits August 10, 2026 12:04
Addresses Codex review feedback on #83.

741333d bounded the wait on a flush that was already in flight, but that is
the less common case and left the ordinary one unbounded: with nothing in
flight the wait returns immediately, and the drain loop then opens a fresh
request of its own. React Native's fetch has no request timeout, so a stall
there hung teardown exactly as before — and with it the provider, which
awaits the pending cleanup before constructing a replacement instance.

cleanup() now runs against a single deadline covering every send it waits
on, rather than a bound per wait. That also keeps total teardown bounded
instead of scaling at 5s per flush attempt.

The drain loop's flush is folded into that race as a settled promise rather
than a rejecting one, so only the deadline can win on error and no rejection
escapes unhandled. Its error branch keeps the previous behaviour of logging
and breaking out of the loop.

Both give-up paths — deadline exceeded and the pre-existing safety limit —
now share abandonQueuedEvents(), which routes through clear() so a send
still holding a batch cannot put it back after teardown returns.

The regression test lets the first send succeed so nothing is in flight,
leaving the stall to the request the drain loop itself opens: it hits Jest's
timeout with the bound removed and completes with it in place.

366 tests pass (365 before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to 3b352b5. Bumping the generation stopped an abandoned batch
being re-queued, but not its callbacks: the request cleanup() gave up on has
no timeout, so it can settle long after teardown resolved, and flush() then
still ran done() on it. The consumer's per-event callback fired against an
instance the app had already torn down, and which the provider may since
have replaced.

done() now returns early when the queue is closed and the batch's generation
is stale — that pair means specifically "abandoned during teardown". A plain
clear() for opt-out is unaffected, so a consumer still learns those events
were dropped.

This also tightens the earlier resurrection test: after teardown the correct
expectation is no callback at all rather than exactly one, and it still
fails if the abandoned batch is resurrected.

367 tests pass (366 before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second Codex finding on 3b352b5: a flush waiting on the mutex when teardown
begins could still send.

Scenario: cleanup()'s drain flush is in flight; a consumer flush() (or a
second cleanup) queues behind it on the mutex; the drain flush fails
retryably and re-queues its events; cleanup breaks out of its loop and
resolves. The queued flush then takes the mutex and sends those events, and
invokes their callbacks, after teardown had finished.

Two changes:

Every give-up path in cleanup now empties and invalidates the queue, not
just the safety-limit one, so nothing is left behind for a later flush to
find. On its own this was not sufficient, which the first version of the
regression test failed to show: the queued flush takes the mutex the instant
the drain flush releases it — before cleanup has even observed that failure
— so the events are already gone by the time cleanup clears anything.

So flush() now abandons itself if teardown began while it was waiting its
turn. That check has to identify cleanup's own drain calls specifically
rather than a window of time: cleanup spends most of its drain loop awaiting
one of them, and a `draining` flag covering that window classified the
racing flush as internal. The flush body moved to a private runFlush(),
which cleanup calls with duringCleanup: true; the public flush() is
unchanged for callers.

368 tests pass (367 before), typecheck, lint and `pnpm audit --prod` clean.

The regression test asserts absolute send counts rather than a delta
measured after cleanup() resolves, because the racing send lands before
that: 3 with the guard, 5 without.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Last finding on caecaba. cleanup()'s own drain flushes are exempt from the
"abandon flushes that outlive teardown" check, which is correct for one
teardown but not for two: a second cleanup() waiting on the mutex resumed
before the first observed its failed drain, spliced the events that drain
had just re-queued, and sent them after the first cleanup() resolved. The
first run's final abandon could not help — by then the queue was empty
because the second run had taken the events.

cleanup() now returns the run already under way instead of starting a
competing one. Teardown should be idempotent regardless, and cleanup() is
public API as well as being called by the provider.

369 tests pass (368 before); the regression test issues two concurrent
teardowns and expects three sends, which is five without this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two follow-ups on 7a282d6.

A rejection from runCleanup() was memoised, so every later cleanup() would
return that same rejected promise and never retry — and the provider, which
awaits the pending cleanup before constructing a replacement, would reject
with it forever. Teardown is best-effort and now always settles, clearing
the queue on the failure path so nothing is left for a straggling flush.
The AppState listener removal is also guarded, since that native call is the
one thing that can throw before the queue is drained or invalidated.

The abandon path in runFlush() no longer invokes the caller's callback. It
resumes only once the flush ahead of it settles, which can be long after
cleanup() resolved, and nothing may call back into a torn-down instance. The
returned promise still resolves, so an awaiting caller is not left hanging.

371 tests pass (369 before), typecheck, lint and `pnpm audit --prod` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@yosriady

Copy link
Copy Markdown
Contributor Author

Update on the P1 thread above — that finding cascaded into four more, all now fixed and each with a regression test verified to fail with that fix alone reverted:

  • 3b352b5 — the earlier bound covered only an already-in-flight flush, which is the less common case. With nothing in flight the wait returns immediately and the drain loop opens a fresh request that could stall. cleanup() now runs against a single deadline covering every send it waits on.
  • b80f53c — the generation bump stopped an abandoned batch being re-queued but not done(), so a request settling after teardown still invoked consumer callbacks.
  • caecaba — a consumer flush() queued behind cleanup's drain flush took the mutex the instant that drain released it, before cleanup observed the failure, and sent the events anyway. Flushes now abandon themselves if teardown began while they were waiting their turn.
  • 7a282d6 — a second cleanup() was exempt from that check (its drain flush is internal by definition), so it could send what the first run had just re-queued. Teardown is now idempotent.
  • 79699da — a rejection from teardown was memoised and would have poisoned every later cleanup(), permanently blocking provider re-initialization; and the abandon path still invoked the caller's flush callback.

371 tests pass (331 at the first review); typecheck, lint and pnpm audit --prod clean.

Two things I did not do, both deliberate:

  • No request-level timeout or AbortController in sendWithRetry. That changes behaviour for every send — including legitimately slow uploads on poor mobile connections — to fix something that only bites at teardown. Happy to add it separately if you want it.
  • The apostrophe / javascript:-prefix character rules and HTML-entity encodings remain as previously explained.

Also worth flagging, found while writing these tests and unrelated to this PR: options.retryCount || DEFAULT_RETRY treats an explicit 0 as unset, so a consumer disabling retries silently gets 3. Same for flushAt, flushInterval and maxQueueSize.

@yosriady
yosriady merged commit 493db39 into main Aug 10, 2026
13 checks passed
@yosriady
yosriady deleted the fix/port-web-sdk-attribution-and-queue-fixes branch August 10, 2026 17:38
yosriady added a commit to getformo/examples that referenced this pull request Aug 10, 2026
)

Picks up getformo/sdk-react-native#83: traffic-source sanitization for
values captured from deep links and the Android install referrer,
immediate delivery of the first event of an app session, and a batch of
queue and teardown reliability fixes.

No app code changes are needed — 1.1.0 has no API, type or export changes.

Verified against the published package end to end, with the example running
under Metro and events pointed at a local collector:

  - a clean marketing link keeps all six utm_*/ref values, no false
    positives, and events report library_version 1.1.0
  - the first batch on the wire carries exactly one event, so cold-start
    attribution is not held for the batch timer
  - double-encoded, scheme-in-value, multi-byte and raw-markup payloads in
    a deep link are all dropped, while clean values alongside them survive
  - UI-driven events batch rather than sending one request per interaction
  - against a collector rejecting every request, batches are dropped once
    rather than re-posted forever, and no unhandled rejection reaches the app
  - three unmount -> cleanup -> re-init cycles each re-initialize and deliver

`pnpm install --frozen-lockfile`, `pnpm typecheck` and the example's tests
all pass.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant