Repository navigation
fix(browser): cut per-capture storage reads for Meta identifiers - #5433
Merged
Merged
Conversation
…ead of parsing persistence on every capture
…n write without re-parsing persistence
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed a8b0781: 2 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
Contributor
Coherence📄 0 spec file errors · ⛔ 0 broken rules · 🚪 2 unchecked inputs · 🐛 0 bugs that came back What is Coherence?Coherence compares the code with its spec files ( This check read 44 parts of the code and 7 rules. Note These findings do not make this check fail. None of these findings are in files that this pull request changes. You do not need to fix them in this pull request. 🚪 Unchecked inputs (2)
What this means and how to fix itInput from outside this code comes in at these places. The spec does not say what checks that input. 10 more places were already like this when Coherence was added. This list does not show them. To close one of these, find the place by its name in the
To get the line to add, run |
Contributor
Contributor
|
Size Change: +23.1 kB (+0.09%) Total Size: 26.5 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
posthog-js Compliance ReportDate: 2026-11-09 08:42:27 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
…ng Meta identifiers
ioannisj
marked this pull request as ready for review
October 26, 2026 13:04
marandaneto
reviewed
Oct 27, 2026
marandaneto
left a comment
marandaneto
left a comment
Member
There was a problem hiding this comment.
Automated advisory code review. Three reproduced regressions from the reviewed patch; the pre-write deletion issue is already described in an existing review thread, so it is not duplicated here.
| } | ||
|
|
||
| const channel = this._facebookClickIdChannel | ||
| this.persistence.refreshMetaIdentifiers() |
Member
There was a problem hiding this comment.
blocking: Refresh cookie identifiers before constructing debounced captures — With persistence_save_debounce_ms enabled, removing this refresh leaves a sibling subdomain's cookie identifier unseen while the conversion event is constructed; refreshing on a later flush cannot amend that event. This affects pure-cookie persistence when a new pending $fbc replaces an already-delivered value, and default localStorage+cookie when the cookie fills a missing identifier. Neither receives cross-subdomain storage events to compensate. Reproduction: reproduced — the two cookie-mode cases in review-meta-regressions.test.ts emit no expected $set.$fbc on reviewed head, while the identical tests pass on the merge base.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Fixed in ddae297. capture() now calls refreshMetaIdentifiersBeforeCapture() once before the Meta block, so $set.$fbc is built after the cookie is read, whatever the debounce. Pure cookie (and sessionStorage) go through the snapshot-gated refresh, localStorage+cookie goes through the cookie compare, and both only parse when the raw text changed, so it's one document.cookie read per capture and still no persistence parse.
Added both of your cases, pure cookie with a delivered $fbc replaced by a new pending one, and localStorage+cookie filling a missing one, with persistence_save_debounce_ms on.
Contributor
There was a problem hiding this comment.
[P1] Reconcile an already-cached identifier before constructing deferred captures
Location: packages/browser/src/posthog-persistence.ts:1384–1392; anchor line 1391, RIGHT. Related fill-only condition: :519.
Change relationship: introduced by the full PR; incompletely repaired by the follow-up.
Severity/disposition: IMPORTANT / REQUIRED.
A tab holding automatically discovered click A can construct a conversion with A after a sibling has durably persisted click B. The merge base constructs that same conversion with B.
This remains reproducible with supported configuration:
{
persistence: 'localStorage+cookie',
defaults: '2026-05-30',
person_profiles: 'always'
}Those dated defaults select 250 ms persistence debounce and disable cookie-wins precedence (posthog-core.ts:301–305).
Trigger and source explanation
- P discovers A from
fbclid, captures, and flushes; its identifier is no longer a pending local persistence mutation. - Q discovers B and flushes. Shared localStorage contains B.
- P captures a conversion on a clean URL before notification or its next deferred write.
- The new pre-capture hybrid branch checks only the shared cookie.
_adoptMetaIdentifiersFromCookie()refuses to replace a key already present inprops, leaving cached A.- The conversion is constructed before later write/event reconciliation can adopt B.
“localStorage wins” should select current durable localStorage B here, not stale in-memory A. This is not a conflicting subdomain-cookie case.
The governing contract is established merge-base browser behavior and compatibility-preserving capture semantics. No sdk-spec requirement for synchronous cross-tab freshness is asserted.
Executed public-entry regression
The following is the retained failing case, using real SDK caches, public capture(), automatic URL discovery, and actual jsdom storage:
import { uuidv7 } from '@posthog/browser-common/utils/uuidv7'
import { defaultPostHog } from './helpers/posthog-instance'
import { PERSISTENCE_FACEBOOK_CLICK_ID as FBC } from '../constants'
import type { PostHogConfig } from '../types'
const route = (click?: string) =>
window.history.replaceState({}, '', click ? `/?fbclid=${click}` : '/')
const init = (token: string, config: Partial<PostHogConfig>) =>
defaultPostHog().init(token, {
person_profiles: 'always',
capture_pageview: false,
request_batching: false,
...config,
}, uuidv7())!
beforeEach(() => {
vi.useFakeTimers()
vi.setSystemTime(new Date('2026-10-26T12:00:00Z'))
route()
})
afterEach(() => {
route()
vi.clearAllTimers()
vi.useRealTimers()
vi.restoreAllMocks()
localStorage.clear()
sessionStorage.clear()
})
it('uses an automatically discovered sibling click for a later conversion', () => {
const config = {
persistence: 'localStorage+cookie',
defaults: '2026-05-30',
} as const
const token = uuidv7()
const key = `ph_${token}_posthog`
const p = init(token, config)
route('first-click')
expect(p.capture('first automatic click')?.$set?.$fbc)
.toBe(`fb.1.${Date.now()}.first-click`)
p.persistence!.flush()
route()
const q = init(token, config)
vi.setSystemTime(Date.now() + 1000)
route('second-click')
const expected = `fb.1.${Date.now()}.second-click`
expect(q.capture('second automatic click')?.$set?.$fbc).toBe(expected)
q.persistence!.flush()
expect(JSON.parse(localStorage.getItem(key)!)[FBC].value).toBe(expected)
route()
expect(p.capture('conversion on clean URL')?.$set?.$fbc).toBe(expected)
})Tested with Node 24.13.0, Vitest 1.6.1, and jsdom under equivalent conditions on each pinned revision. Put the regression alongside the existing browser source tests and run from packages/browser:
pnpm exec vitest run src/__tests__/automatic-meta-review.test.ts --maxWorkers=1 --minWorkers=1| Revision | Result |
|---|---|
| Merge base | All four automatic cases pass |
| Previous head | All four fail |
| Current head | This case fails; other three pass |
Current expected: fb.1.1793016001000.second-click
Current received: fb.1.1793016000000.first-click
The three additional controls—notification first, zero debounce, and cookie-wins mode—pass on both base and head.
Impact, limits, and recommendation
This can send the previous click as a person-property update instead of the newer durable click. Later reconciliation can correct subsequent captures; permanent loss of B, consent bypass, and live Meta misattribution were not demonstrated.
The test models separate SDK caches in one document; no current real multi-document execution is claimed. The queued browser storage-notification contract supports the ordering.
Consider reconciling current durable Meta state before building deferred captures when a sibling change is detected, preserving pending-local priority and configured cookie/localStorage precedence. The solution need not restore unconditional full-entry parsing on unchanged captures.
Retain the automatic cached-A→B test and its three controls alongside the new missing-key and cookie-only cases.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Fixed in 10cb54d. refreshMetaIdentifiersBeforeCapture() now also compares the stored text against the snapshot on localStorage and localStorage+cookie, and re-reads the Meta keys when it changed, so a capture can't be built from cached A once B is readable. A pending local change still wins.
An unchanged capture adds raw text reads but no parse. A sibling write costs one parse on localStorage and two on localStorage+cookie, because that store's _parse writes the merged entry back.
I forced this ordering in real tabs too (Chromium 136, Firefox 137, WebKit 18.4, capture before the storage event) and all 24 runs match the merge base. That also narrowed the impact: the previous head only sent stale A while A's request was in flight, otherwise it sent no $fbc and B went out on the next capture.
There's one difference I haven't explained. In a free-running tab loop the merge base stops re-sending an in-flight A and this branch keeps sending it. It's the same value so I think it's harmless, but I haven't traced why.
Added cached A to B, sibling reset and pending-local cases to packages/browser/src/__tests__/capture-hot-path.test.ts for both stores, plus your three controls.
| if (!this.persistence) { | ||
| return | ||
| } | ||
| this.persistence.refreshMetaIdentifiers() |
Member
There was a problem hiding this comment.
blocking: Reconcile before confirming an old request's identifier — Removing the callback refresh lets an older successful request overwrite a newer sibling identifier before its storage event arrives. The callback sees the stale in-memory identifier, registers it as delivered and marks it locally pending; pre-write reconciliation then skips the newer stored value. This loses the new click and its pending-delivery state rather than merely delaying propagation. Reproduction: reproduced — the delayed-response case in review-meta-regressions.test.ts replaces the newer sibling identifier with the old delivered identifier on reviewed head, while the identical test passes on the merge base.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Fixed in ddae297. _markMetaIdentifierDelivered refreshes the Meta identifiers from storage before the compare again, so a newer sibling value fails the stored.value === value check and keeps its pending state instead of being overwritten by the old delivered one.
On localStorage stores that refresh now always parses rather than trusting the snapshot, since the snapshot can't see the cookie half of localStorage+cookie or an A→B→A change. It runs once per confirming response, not per capture. Test covers localStorage and cookie.
| // event, the pre-write reconciliation and the cookie compare below. Only the | ||
| // cookie store, capped at 4 KB, still re-reads here; sessionStorage and memory | ||
| // have no siblings. | ||
| if (this._usesCookieMetaIdentifierRefresh()) { |
Member
There was a problem hiding this comment.
blocking: Preserve sessionStorage synchronization between same-origin frames — Same-origin iframes within one tab share sessionStorage, so this store does have sibling writers. After a frame writes a pending identifier, this instance ignores its storage event because it is not split-storage eligible; with the capture refresh removed and this write refresh now restricted to cookies, the parent's next conversion omits the identifier and can overwrite it with stale state. Keep a reconciliation path for shared sessionStorage. Reproduction: reproduced — the actual-iframe case in review-meta-regressions.test.ts confirms that the parent sees the frame's stored value but omits $set.$fbc on reviewed head, while the identical test passes on the merge base.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
I had the premise wrong here, same-origin frames share sessionStorage and our listener drops their storage events. Fixed in ddae297: sessionStorage now gets the same snapshot-gated refresh as the cookie store, before capture and before writes, and the snapshot is remembered after our own writes so it doesn't parse on every capture. I fixed the code comment too, and the PR description is next.
Test runs two instances over the shared sessionStorage and checks the second one's next capture sends the first one's $fbc.
dustinbyrne
reviewed
Oct 27, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
I independently reproduced the older-response overwrite and deferred-capture freshness issues through automatic fbclid discovery, so I have not duplicated those inline findings. The pre-write deletion/reset gap also still reproduces on this head; details are inline.
All four focused automatic-discovery cases pass on merge base 64f8a4325bde1b2c00338a1c13ba6c9bbbfa9a4f and fail on reviewed head 4e2cc29b40dc74b02883ec5445be3baf6db891df (the acknowledgement case covers both localStorage and cookie). Validation used real SDK instances in jsdom; no real multi-tab browser or live ingestion result is claimed.
AI-assisted review.
| // event, the pre-write reconciliation and the cookie compare below. Only the | ||
| // cookie store, capped at 4 KB, still re-reads here; sessionStorage and memory | ||
| // have no siblings. | ||
| if (this._usesCookieMetaIdentifierRefresh()) { |
Contributor
There was a problem hiding this comment.
[P1] Reconcile an externally removed entry before rewriting localStorage
A sibling reset() can remove the main localStorage entry while its replacement save is deferred. If this instance captures before that replacement or the storage notification, this new cookie-only refresh gate lets it write its cached Meta identifier back. On the merge base the identifier stays removed; on this head it is restored.
The replacement _reconcileCrossTabFeatureFlagPropertiesBeforeWrite() merges Meta state only in the non-null branch (posthog-persistence.ts:567–568). Its null branch (:562–565) sets mainState.storageValue = null without reconciling Meta keys. The new storage-event null handling (:373–383) therefore fixes only the ordering where notification arrives before the write. It rereads current storage, so notification after resurrection can observe the restored value rather than the earlier deletion.
This is the remaining pre-write part of the earlier removal finding. Reproduced through automatic URL fbclid discovery followed by public SDK reset()/capture() calls:
import { uuidv7 } from '@posthog/browser-common/utils/uuidv7'
import { defaultPostHog } from './helpers/posthog-instance'
import { PERSISTENCE_FACEBOOK_CLICK_ID as FBC } from '../constants'
import type { PostHogConfig } from '../types'
const route = (click?: string) =>
window.history.replaceState({}, '', click ? `/?fbclid=${click}` : '/')
const init = (token: string, config: Partial<PostHogConfig> = {}) =>
defaultPostHog().init(token, {
persistence: 'localStorage',
person_profiles: 'always',
capture_pageview: false,
request_batching: false,
...config,
}, uuidv7())!
beforeEach(() => {
vi.useFakeTimers()
vi.setSystemTime(new Date('2026-10-26T12:00:00Z'))
route()
})
afterEach(() => {
route()
vi.clearAllTimers()
vi.useRealTimers()
vi.restoreAllMocks()
localStorage.clear()
sessionStorage.clear()
})
it('does not resurrect an automatically discovered identifier during sibling reset', () => {
const token = uuidv7()
const p = init(token)
route('first-click')
expect(p.capture('first automatic click')?.$set?.$fbc)
.toBe(`fb.1.${Date.now()}.first-click`)
p.persistence!.flush()
route()
const q = init(token, { persistence_save_debounce_ms: 250 })
q.persistence!.flush()
q.reset()
const key = `ph_${token}_posthog`
expect(localStorage.getItem(key)).toBeNull()
vi.setSystemTime(Date.now() + 1000)
const conversion = p.capture('conversion after reset on clean URL')
expect(JSON.parse(localStorage.getItem(key)!)).not.toHaveProperty(FBC)
expect(conversion?.$set?.$fbc).toBeUndefined()
})Executed with Node 24.13.0, Vitest 1.6.1 and jsdom using equivalent dependencies/configuration on merge base 64f8a4325bde1b2c00338a1c13ba6c9bbbfa9a4f and head 4e2cc29b40dc74b02883ec5445be3baf6db891df:
# Place alongside the existing browser source tests, then run from packages/browser:
pnpm exec vitest run src/__tests__/automatic-meta-review.test.ts --maxWorkers=1 --minWorkers=1Base: passes. Head: fails because the rewritten entry contains { value: 'fb.1.1793016000000.first-click', delivered: false }. A separate removal-before-notification test shows the same differential failure; delivering the keyed removal notification first passes on both revisions. The two named instances model independent caches; these exact orderings were not run in real multi-document browsers. The browser storage broadcast algorithm queues notification after mutation, so another SDK operation can run in this interval.
The observed impact is deleted attribution becoming durable again and potentially being reused; later sibling writes may correct it. This does not establish a consent bypass or a new identity-synchronization defect, and it does not generalize the absent-entry interval to cookie-wins reset configurations that synchronously publish a replacement.
Consider reconciling externally observed absence for Meta keys before writing, preserving pending-local priority and intentional local removal/migration behavior. Keep unrelated feature-flag retention semantics unchanged. Retain the public-reset, removal-before-notification and delivered-notification control cases as regression coverage.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Fixed in ddae297. The pre-write null branch now clears non-pending Meta keys when this tab had previously seen an entry (storageValue is a string), so a sibling reset() sticks even if our write lands before the storage event. Feature-flag handling in that branch is unchanged.
A first write can't take that path, since load seeds storageValue with null, so a cookie-only $fbc on first write is kept. Added your reset scenario plus that first-write case as a guard.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
The rewrite you reported was fixed in ddae297, but the event itself could still carry the cleared $fbc when this tab captured with a debounce on and the storage event hadn't arrived. Same cause as the cached A to B thread, fixed in 10cb54d, the capture now sees the removal before it's built.
Added a sibling reset() case for localStorage and localStorage+cookie.
…over sessionStorage frames and sibling reset
ioannisj
changed the title
dustinbyrne
reviewed
Oct 29, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The delayed-acknowledgement and sibling-reset reproductions now pass. The deferred hybrid capture case with an already-cached identifier still fails against the merge base; details are in the existing freshness thread. I also reproduced the batch-confirmation work concern already reported here, but treat that as optional because no latency regression was measured.
AI-assisted review.
dustinbyrne
reviewed
Nov 3, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The latest callback optimization addresses repeated confirmation work, but the cached-A→B deferred-capture freshness issue still applies on this head. The hybrid pre-capture path fills missing identifiers but does not reconcile an already-cached identifier before constructing the conversion.
Consider addressing that path and adding the cached-state regression before approval.
AI-assisted review.
Contributor
SDK performance: posthog-node1 change worth reviewing: 1 observed improvement. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate.
View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
dustinbyrne
approved these changes
Nov 5, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The cached-identifier localStorage/hybrid freshness issue is addressed, including capture before the storage event and sibling reset. I have one non-blocking compatibility note about same-namespace memory clients.
AI-assisted review.
|
|
||
| // The cookie store and sessionStorage (shared by same-origin frames) get no storage event. | ||
| private _usesMetaIdentifierSnapshotRefresh(): boolean { | ||
| return this._storage === cookieStore || this._storage === sessionStore |
Contributor
There was a problem hiding this comment.
M1. [P2] Include shared memory clients in the Meta refresh path
- Location:
packages/browser/src/posthog-persistence.ts:1402, RIGHT; related gates at lines 1395–1407 and 1473. - Severity / disposition: MINOR / OPTIONAL.
- Change relationship: Introduced by the full PR, already present at the previous reviewed head; newly identified in this follow-up, not introduced by the latest main merge.
- Evidence: Source-supported differential behavior; the regression sketch below was not executed.
Memory persistence can have another writer in the same JavaScript module: memoryStore uses a module-level map keyed by persistence name (storage.ts:582–611). Named init() is supported (posthog-core.ts:800–830), and the persistence name depends on the token or persistence_name, not the instance name (posthog-persistence.ts:99–110).
For two named clients using the same token and persistence: 'memory':
- Initialize both clients.
- P discovers click A, captures, and flushes persistence.
- Q discovers click B, captures, and flushes persistence.
- P captures a conversion on a clean URL before either request is confirmed.
The merge base refreshes P’s Meta state from the shared memory entry and constructs the conversion with B. On this head, memory is excluded from both the pre-capture classifier and the pre-write refresh gate (posthog-persistence.ts:1388–1407, :1473), so P retains and emits A.
The same exclusion can leave P repeating A after Q has confirmed the identical value and flushed its delivered state.
On base, identifier calculation calls refreshMetaIdentifiers() before reading cached state (posthog-core.ts:1673,1722). Memory’s object-valued _get() produces no string snapshot, so _refreshKeys() reads the current shared entry (posthog-persistence.ts:1269–1293). On head, those calculation-time calls are removed and memory does not qualify for their replacements. Loading copies the top-level props dictionary, and channel registration replaces the Meta object (posthog-persistence.ts:1162; posthog-core.ts:1607–1628), so Q’s update does not simply mutate P’s cache. P’s successful flush also clears its pending-local guard; an unconfirmed network request does not keep that guard set.
This is a narrow same-module/same-namespace configuration—not memory shared across tabs, and not a failure of ordinary single-instance memory persistence. The compatibility evidence is merge-base behavior; neither the public docs nor the inspected sdk-specs promise general cross-instance memory synchronization. The source-inferred impact is stale or redundant Meta person-property enrichment, not a dropped event, consent bypass, permanent attribution loss, or observed live Meta misattribution.
Consider retaining the existing memory refresh path and adding the regression controls below. Including the resolved memoryStore in the current classifier would preserve the pending-local guard and perform an object lookup, without JSON parsing or browser-storage I/O.
Test sketch (not executed): the test below predicts base success and head failure for memory A→B and sibling-confirmation cases. SessionStorage and pending-local controls should pass on both. Execution could not be completed: Docker is unavailable and the available sandbox mechanism aborts before establishing an execution environment.
Exact regression sketch — not executed
Place at packages/browser/src/__tests__/memory-meta-review.test.ts in isolated validation copies. Both clients initialize before attribution is discovered; capture assertions distinguish a missing identifier from a dropped event.
import { uuidv7 } from '@posthog/browser-common/utils/uuidv7'
import { defaultPostHog } from './helpers/posthog-instance'
import type { PostHog } from '../posthog-core'
import type { PostHogConfig } from '../types'
type Persistence = 'memory' | 'sessionStorage'
const originalUrl = window.location.href
const route = (click?: string) =>
window.history.replaceState(null, '', click ? `/?fbclid=${click}` : '/')
const captureFbc = (posthog: PostHog, event: string) => {
const result = posthog.capture(event)
expect(result?.event).toBe(event)
return result?.$set?.$fbc
}
const init = (token: string, persistence: Persistence) => {
const config: Partial<PostHogConfig> = {
persistence,
persistence_save_debounce_ms: 250,
person_profiles: 'always',
bootstrap: { distinctID: 'shared-review-visitor' },
capture_pageview: false,
autocapture: false,
disable_session_recording: true,
disable_surveys: true,
disable_conversations: true,
advanced_disable_flags: true,
request_batching: false,
rate_limiting: {
events_per_second: 1000,
events_burst_limit: 1000,
},
}
const posthog = defaultPostHog().init(token, config, uuidv7())
const responses: (() => void)[] = []
vi.spyOn(posthog, '_send_retriable_request').mockImplementation((options) => {
responses.push(() => options.callback?.({ statusCode: 200 }))
})
posthog.persistence!.flush()
return { posthog, responses }
}
const pair = (persistence: Persistence) => {
const token = uuidv7()
const p = init(token, persistence)
const q = init(token, persistence)
return { p, q }
}
beforeEach(() => {
vi.useFakeTimers()
vi.setSystemTime(new Date('2026-10-26T12:00:00Z'))
route()
document.cookie = '_fbc=; Max-Age=0; path=/'
document.cookie = '_fbp=; Max-Age=0; path=/'
})
afterEach(() => {
window.history.replaceState(null, '', originalUrl)
vi.clearAllTimers()
vi.useRealTimers()
vi.restoreAllMocks()
localStorage.clear()
sessionStorage.clear()
})
describe.each(['memory', 'sessionStorage'] as const)(
'shared named-instance Meta state: %s',
(persistence) => {
it('uses a sibling’s newer automatically discovered click', () => {
const { p, q } = pair(persistence)
route('click-a')
const a = captureFbc(p.posthog, 'landing A')
expect(a).toMatch(/\.click-a$/)
p.posthog.persistence!.flush()
vi.setSystemTime(Date.now() + 1000)
route('click-b')
const b = captureFbc(q.posthog, 'landing B')
expect(b).toMatch(/\.click-b$/)
q.posthog.persistence!.flush()
route()
expect(captureFbc(p.posthog, 'conversion')).toBe(b)
})
it('observes a sibling’s confirmation of the identical value', () => {
const { p, q } = pair(persistence)
route('click-a')
const a = captureFbc(p.posthog, 'landing A')
expect(a).toMatch(/\.click-a$/)
p.posthog.persistence!.flush()
// Keep the same URL and fixed clock. Q must actually send A.
expect(captureFbc(q.posthog, 'sibling delivery')).toBe(a)
expect(q.responses.length).toBeGreaterThan(0)
q.responses.forEach((respond) => respond())
q.posthog.persistence!.flush()
// P's own response remains withheld.
route()
expect(captureFbc(p.posthog, 'after confirmation')).toBeUndefined()
})
it('preserves a newer local change awaiting its write', () => {
const { p, q } = pair(persistence)
route('click-a')
expect(captureFbc(p.posthog, 'landing A')).toMatch(/\.click-a$/)
p.posthog.persistence!.flush()
vi.setSystemTime(Date.now() + 1000)
route('click-b')
expect(captureFbc(q.posthog, 'landing B')).toMatch(/\.click-b$/)
q.posthog.persistence!.flush()
vi.setSystemTime(Date.now() + 1000)
route('local-click')
const local = captureFbc(p.posthog, 'local landing')
expect(local).toMatch(/\.local-click$/)
// No flush of P's newer local change.
route()
expect(captureFbc(p.posthog, 'local conversion')).toBe(local)
})
}
)
it('stops enrichment after ordinary single-instance memory delivery', () => {
const p = init(uuidv7(), 'memory')
route('click-a')
expect(captureFbc(p.posthog, 'landing')).toMatch(/\.click-a$/)
expect(p.responses.length).toBeGreaterThan(0)
p.responses.forEach((respond) => respond())
p.posthog.persistence!.flush()
route()
expect(captureFbc(p.posthog, 'after delivery')).toBeUndefined()
})In equivalent disposable copies of base 6428f1664a82657b290647f661a2f39c44ff8d1f and head bc83272120b0eeb7f06553e67b54bd098e52f802, with matching audited dependencies and Node 24:
# Bootstrap workspace prerequisites at each repository root:
pnpm turbo run build --filter=posthog-js
# From packages/browser:
pnpm exec vitest run src/__tests__/memory-meta-review.test.ts \
--maxWorkers=1 --minWorkers=1| Case | Base — source-inferred | Head — source-inferred |
|---|---|---|
| Memory A→B | Conversion carries B | Conversion carries A |
| Memory sibling confirms A | Omits $fbc |
Repeats A |
| Memory pending-local priority | Keeps local value | Keeps local value |
| SessionStorage controls | Pass | Pass |
| Ordinary single memory client | Pass | Pass |
Executed results / exit codes: none. This sketch and the suggested correction were not executed. Docker is unavailable and sandbox-exec aborts with exit 134 even for /usr/bin/true, so no credential-isolated execution environment was established.
Contributor
Author
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Fixed in 62c156d. Memory now goes through the same refresh as cookie and sessionStorage before capture and before writes, so a sibling's $fbc (or its confirmation) is picked up. It's an object lookup on memoryStore, no parse, so the hot path matrix still shows 0 parses for memory.
With memory in, _tracksMetaIdentifierStorageSnapshot was true for every store, so I dropped it. Added your three cases (newer sibling click, sibling confirmation, pending local change) to capture-hot-path.test.ts.
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
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.
Problem
#5429 stopped parsing the persistence entry on every
capture()for the Meta$fbcand$fbpidentifiers, but the refresh is still on the capture path: threerefreshMetaIdentifiers()calls per event, each readingdocument.cookietwice and copying the whole localStorage value onlocalStorage+cookie. With a 310 KB entry (570 flags with details and payloads) that is 5 to 6 entry reads and 10 to 14 cookie reads per click.The entry only changes when a sibling writes it, and the browser already reports that through the
storageevent, while the write path already re-reads the entry once, gated on the text having changed.Changes
Replaces the three
refreshMetaIdentifiers()calls percapture()with one gated read before$setis built, so neither a debounced write nor astorageevent that hasn't arrived yet can hide a sibling's identifier. Where the main entry is already parsed (thestorageevent handler and the pre-write reconciliation), sibling updates go through a shared_mergeMetaIdentifierPropertieswith the same rulesrefreshKeyused: a pending local change wins, set when present and different, delete when absent, and a sibling removing the whole entry (reset(), opt-out) clears the keys.How each store picks up a sibling:
localStorageandlocalStorage+cookie: thestorageevent is queued behind the write, so a sibling's value is readable before this tab hears about it. Before capture, the raw entry text is compared against the last text seen, and the Meta keys are only re-read when it changed, so a newer value or a removal reaches the event being built.localStorage+cookiewithoutcookieWinsOnConflict: a cookie compare before capture and before each write parses the 4 KB cookie only when its text changed and fills a key this tab lacks, so a subdomain that writes only the cookie is still seen.localStorage+cookiewithcookieWinsOnConflict: the localStorage merge skips cookie-persisted keys and leaves them tosyncCookieProperties, so an older sibling value can't overwrite a newer subdomain cookie.cookieandsessionStorage: our listener only handles localStorage events, and same-origin frames share sessionStorage, so both keep the snapshot-gatedrefreshMetaIdentifiers(), which parses only when the raw text changed.memoryhas no siblings and refreshes nothing.The delivery callback refreshes from storage before marking an identifier delivered, so an older response can't confirm over a newer sibling value. On localStorage stores that refresh always parses, but only while the value is unconfirmed, so later responses in a batch skip it.
_rawTextis removed, nothing calls it any more.I benchmarked
mainagainst this branch in jsdom with the 310 KB entry. Percapture(), as parses / entry reads / cookie reads:With a 250 ms
persistence_save_debounce_msthis branch does 1 entry read per capture on every store. A sibling write costs one parse onlocalStorageand two onlocalStorage+cookie, because that store's_parsewrites the merged entry back, then parsing stops.The two cookie reads left in every mode are the
_fbcand_fbpreads in_readMetaCookie. Most of what remains per capture is the stringify andsetItemof the main entry for the session activity write, untouched here.Timings come from real browsers, since the jsdom medians moved too much between runs. On
localStorage+cookiewith a 269 KB entry and a 250 ms debounce,capture()went from 0.200 to 0.110 ms on Chromium 136, 0.400 to 0.200 ms on Firefox 137 and 0.250 to 0.167 ms on WebKit 18.4. PlainlocalStorageon Chromium barely moved. These are headless runs of unminified bundles, so read them as relative numbers.capture-hot-path.test.tskeeps the parse-count matrix from #5429 and covers each path above, including a debounced capture, a sibling write readable before itsstorageevent, a response racing a sibling write, two frames sharing sessionStorage, and a siblingreset(). The sibling orderings were also forced in real tabs on the same three browsers, and all 24 match the merge base. One difference showed up in a free-running tab loop: the merge base stops re-sending an in-flight value and this branch keeps sending it. It's the same value and I haven't traced why. No sdk-specs capability covers the Meta identifiers.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
DRI: @ioannisj
Autonomy: Human-driven (agent-assisted)