fix: keep the dedup hash for its full window, independent of flush timing - #375
fix: keep the dedup hash for its full window, independent of flush timing#375yosriady wants to merge 14 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa4b15d944
ℹ️ 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".
| // The hash folds in the timestamp truncated to the minute, so two identical | ||
| // events can only collide within one minute; holding a hash any longer than | ||
| // that cannot suppress anything. | ||
| const DEDUP_WINDOW_MS = 1_000 * 60; // 1 MINUTE |
There was a problem hiding this comment.
When two otherwise identical events are created on opposite sides of a UTC minute boundary—for example at 12:34:59.999 and 12:35:00.001—generateMessageId() gives them different hashes because it includes the minute-truncated timestamp. The map therefore accepts the second event even though the first entry's 60-second expiry remains open, so the new rolling deduplication window still misses double-fires near every minute boundary. Use a timestamp-independent fingerprint or account for adjacent minute buckets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in a6c6085. Duplicates are now judged by a sha256 of the event without its timestamp (generateDedupKey), and message_id keeps its minute-truncated timestamp because it is the wire identity and must never collide for events a minute apart. Pinning test: "catches a double-fire that straddles a UTC minute boundary" (:59.999 and :00.001, same content, different message ids, second dropped), plus "keeps message ids distinct across minutes while deduping by content".
| this.payloadHashes.forEach((expiresAt, id) => { | ||
| if (expiresAt <= now) this.payloadHashes.delete(id); | ||
| }); |
There was a problem hiding this comment.
On pages producing many distinct events, every enqueue traverses all hashes accepted during the preceding minute, so accepting n events performs roughly 1 + … + n expiry checks even though flushes no longer remove entries. High-frequency autocapture can therefore introduce quadratic main-thread work; expiry should be pruned with an ordered queue/cursor or another structure that visits each expired entry only once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in a6c6085. Entries are in insertion order and each expires a fixed interval after insertion, so pruneExpired now walks from the front and stops at the first live entry: each expired entry is visited once in its life, not once per enqueue. 9841643 and 1ae87f2 make the ordering assumption hold under wall-clock steps (performance.now() where available, clamped per instance on the Date.now() fallback). Pinning test: "prunes only from the front and stops at the first live entry".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
fa4b15d to
0916447
Compare
yosriady
changed the title
…the front Review follow-ups on #375. The dedup key was the message id, which folds in the minute-truncated timestamp because it is the event's identity on the wire. A double-fire straddling a UTC minute boundary (:59.999 and :00.001) therefore got two ids and both went out. Duplicates are now judged by a sha256 of the event without its timestamp; message_id is unchanged. Pruning walked the whole map on every enqueue. Entries are in insertion order and each expires a fixed interval after insertion, so the prune now stops at the first live entry: each expired entry is visited once in its life. Manual iterator because for..of over a Map does not compile under the ES5 target and Map.forEach cannot stop early. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6c6085c2b
ℹ️ 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".
| for (let step = entries.next(); !step.done; step = entries.next()) { | ||
| const key = step.value[0]; | ||
| const expiresAt = step.value[1]; | ||
| if (expiresAt > now) break; |
There was a problem hiding this comment.
When the system clock moves backward between two accepted events, insertion order no longer matches expiry order. After the later event's 60-second window expires, this break can stop at an older pre-adjustment entry whose expiry is still in the future, leaving the expired key in the map; isDuplicate() then drops legitimate repetitions until the clock reaches that older expiry, potentially extending suppression by the full clock adjustment. Use a monotonic clock or an expiry-ordered structure that remains valid across wall-clock corrections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 9841643 and b22bf49. The window is timed by elapsedNow(): the larger of monotonic elapsed (performance.now) and wall elapsed, clamped per instance to never decrease. A backward wall step is absorbed (the other source and the clamp hold), so insertion order stays expiry order and the front-prune assumption holds. Pinning test: "counts a forward wall-clock step toward expiry, and ignores a backward one".
| // Decrement the running byte total by exactly what left the queue. The | ||
| // dedup hashes stay: they expire on their own clock, not on flush. |
There was a problem hiding this comment.
Because fingerprints now survive every flush, they also survive requests that exhaust the built-in retries or receive a non-retryable response. If the per-event callback or application retries the identical event within 60 seconds, enqueue() classifies it as a duplicate and silently drops it, converting a recoverable delivery failure into guaranteed event loss; previously the hash was removed when the item left the queue. Remove or invalidate fingerprints for items whose batch fails while retaining them for successfully delivered items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in b22bf49, f3fd1a8 and 5ffd6ca. A batch whose send fails (retries exhausted or non-retryable) now releases its items' fingerprints so an app retry after the error callback is accepted; delivered batches keep theirs. Release is by a unique per-acceptance token, so a send that outlives the window through backoff (or is cut short by clear()) can never release a newer acceptance of the same key. Batches abandoned on a mid-flush consent withdrawal are released the same way. Pinning tests: "releases the fingerprint of an event whose send failed", "keeps the fingerprint of an event whose send succeeded", "does not release a newer fingerprint when an old send fails after the window", "does not release a same-instant re-acceptance after clear()", "releases the fingerprints of batches abandoned when consent is withdrawn mid-flush".
…notonically Review follow-ups on #375. enqueue() re-checked close() after its hash awaits but not consent, so a withdrawal in that gap could still buffer the event (the flush gate would then drop it, but the contract of this path is to never buffer after withdrawal). The consent gate is now repeated after the awaits. The dedup window used Date.now(). A wall-clock step could expire entries early or, stepping backwards, file a new entry behind older ones with later expiries, out of reach of the front-prune. The window now uses performance.now() where available. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 984164368b
ℹ️ 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".
| */ | ||
| const elapsedNow = (): number => | ||
| typeof performance !== "undefined" && typeof performance.now === "function" | ||
| ? performance.now() |
There was a problem hiding this comment.
On platforms where performance.now() does not advance while the OS is suspended, an event accepted before a laptop or mobile device sleeps remains in the deduplication window after wake even if hours of real time passed. An identical legitimate event is then dropped until the remaining 60 seconds of active monotonic time elapses, so suppression can last arbitrarily longer than the documented minute; use or reconcile with a clock that accounts for suspension while still handling wall-clock corrections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in b22bf49. elapsedNow() takes the larger of monotonic elapsed and wall elapsed (clamped non-decreasing), so suspension counts toward expiry: a device that sleeps past the window wakes with the entry expired. Forward wall steps expire early, which is the safe direction for a duplicate guard. Pinning test: "counts a forward wall-clock step toward expiry, and ignores a backward one" (forward step with no monotonic time elapsed expires the entry).
…the dedup window Review follow-ups on #375. Fingerprints now outlive the flush, so they also outlived a send whose retries were exhausted or whose response was not retryable. An app that sent the same event again after the error callback had that retry dropped as a double-fire. A failed batch now releases its fingerprints; a delivered one keeps them. performance.now() stops during OS suspension on some platforms, so a device that slept mid-window woke still inside it. The window clock now takes the larger of monotonic elapsed and wall elapsed, clamped to never decrease: forward wall steps and suspension expire entries on time or early, backward steps are absorbed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b22bf49330
ℹ️ 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".
| // retryable. Release its fingerprints so the app can send the same | ||
| // event again after the error callback, instead of having that | ||
| // retry classified as a double-fire and dropped for a minute. | ||
| batch.items.forEach(({ dedupKey }) => this.payloadHashes.delete(dedupKey)); |
There was a problem hiding this comment.
The new failure-release code unconditionally deletes the current entry for this content, even when it belongs to a newer event. If a request remains pending for over 60 seconds (the fetch wrapper has no timeout), the same content can legitimately expire and be accepted again; when the older request later fails, this line removes the newer event's fresh suppression window, allowing an immediate duplicate through. Associate the stored expiry/generation with each queue item and delete only when the map still contains that generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in f3fd1a8 and 5ffd6ca. Each queue item records the token its acceptance was given and releaseFingerprints deletes only on a token match, so an old send that fails after the same content was accepted again leaves the newer entry alone. Pinning test: "does not release a newer fingerprint when an old send fails after the window".
| const now = this.elapsedNow(); | ||
| this.pruneExpired(now); |
There was a problem hiding this comment.
Expired fingerprints are pruned only when another event reaches isDuplicate(). If a high-volume page emits a burst and then becomes idle, every hash from that burst remains retained for the lifetime of this queue even though none can affect deduplication after 60 seconds; unlike the old flush-coupled set, a flush does not release them. Schedule cleanup for the earliest expiry (or prune from an existing timer/flush path) so an idle long-lived page does not permanently retain its peak one-minute event volume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined, with the numbers. Retention is bounded to one minute of accepted events: an entry is a 64-char hex key plus a two-field object, about 200 B, so a burst of 1000 events in a minute holds about 200 kB until the next enqueue, and it is released in full by clear() (consent withdrawal) and close() (teardown). Analytics pages do not sustain thousands of distinct events per minute. A dedicated expiry timer would buy back that bounded memory at the cost of a lifetime hazard: a pending timer keeps a Node process alive (the e2e harnesses run the SDK in Node), so it would need unref plus clearing on every teardown path, which is more surface than the retention justifies. Happy to add it if a real page shows the retention mattering.
…ndoned on opt-out Review follow-ups on #375. A failed batch released its fingerprints by key alone. A send can outlive the window through retry backoff; by then the same event may have expired, been accepted again and be in flight under the same key, and the old failure would release that newer entry. Each item now records the expiry it was accepted with and releases only on a match. Batches abandoned when consent is withdrawn mid-flush were never sent but kept their fingerprints, so an app retry inside the window after opt-in was dropped. They are released before the break. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3fd1a8dc3
ℹ️ 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".
| if (this.payloadHashes.get(item.dedupKey) === item.dedupExpiresAt) { | ||
| this.payloadHashes.delete(item.dedupKey); |
There was a problem hiding this comment.
The expiry value is not a unique generation: clear() is explicitly reusable and can run while a flush is in flight, so on a coarse or frozen clock, immediately re-enqueuing the same event records the same dedupExpiresAt as the old item. If the old request subsequently fails, this equality succeeds and deletes the newer fingerprint, allowing a third identical event through. Fresh evidence beyond the prior report is that this collision occurs through same-tick clear()/reuse without waiting for the original 60-second window to expire; use a unique generation token rather than the timestamp itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 5ffd6ca. The token is now a per-acceptance counter (acceptanceSeq), not the expiry, so a same-tick clear() and re-accept cannot collide with the in-flight item. Pinning test: "does not release a same-instant re-acceptance after clear() when the old send fails".
Review follow-up on #375. clear() and a re-accept of the same event can land in the same millisecond as the original acceptance, so an expiry is not a unique token: an old in-flight send failing afterwards could release the newer entry. Each acceptance now carries a counter, and a failed or abandoned item releases its entry only on a token match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
…ming The dedup hash left the queue with its event at flush. Since the first event of a page load is flushed immediately (#326), an identical track() a moment later, the double-fire the guard exists for, was accepted. Hashes now live in a Map with a 60s expiry independent of flush timing, pruned on enqueue so the map is bounded by one minute of accepted events. 60s matches the hash itself, which truncates the timestamp to the minute. The build target is ES5, so the prune uses Map.forEach. Fixes #372. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
…the front Review follow-ups on #375. The dedup key was the message id, which folds in the minute-truncated timestamp because it is the event's identity on the wire. A double-fire straddling a UTC minute boundary (:59.999 and :00.001) therefore got two ids and both went out. Duplicates are now judged by a sha256 of the event without its timestamp; message_id is unchanged. Pruning walked the whole map on every enqueue. Entries are in insertion order and each expires a fixed interval after insertion, so the prune now stops at the first live entry: each expired entry is visited once in its life. Manual iterator because for..of over a Map does not compile under the ES5 target and Map.forEach cannot stop early. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
…notonically Review follow-ups on #375. enqueue() re-checked close() after its hash awaits but not consent, so a withdrawal in that gap could still buffer the event (the flush gate would then drop it, but the contract of this path is to never buffer after withdrawal). The consent gate is now repeated after the awaits. The dedup window used Date.now(). A wall-clock step could expire entries early or, stepping backwards, file a new entry behind older ones with later expiries, out of reach of the front-prune. The window now uses performance.now() where available. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
Where performance.now() is unavailable the window fell back to Date.now(), and a backward wall-clock step could file a new entry behind older ones with later expiries, out of reach of the front-prune. The clock is now clamped per instance to its last value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
…the dedup window Review follow-ups on #375. Fingerprints now outlive the flush, so they also outlived a send whose retries were exhausted or whose response was not retryable. An app that sent the same event again after the error callback had that retry dropped as a double-fire. A failed batch now releases its fingerprints; a delivered one keeps them. performance.now() stops during OS suspension on some platforms, so a device that slept mid-window woke still inside it. The window clock now takes the larger of monotonic elapsed and wall elapsed, clamped to never decrease: forward wall steps and suspension expire entries on time or early, backward steps are absorbed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
…ndoned on opt-out Review follow-ups on #375. A failed batch released its fingerprints by key alone. A send can outlive the window through retry backoff; by then the same event may have expired, been accepted again and be in flight under the same key, and the old failure would release that newer entry. Each item now records the expiry it was accepted with and releases only on a match. Batches abandoned when consent is withdrawn mid-flush were never sent but kept their fingerprints, so an app retry inside the window after opt-in was dropped. They are released before the break. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
Review follow-up on #375. clear() and a re-accept of the same event can land in the same millisecond as the original acceptance, so an expiry is not a unique token: an old in-flight send failing afterwards could release the newer entry. Each acceptance now carries a counter, and a failed or abandoned item releases its entry only on a token match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
The dedup window (rolling expiry, timestamp-free fingerprint, front-only prune, token-checked release, step-proof clock) adds about 0.5 kB brotlied. CI measured 59.07 kB against 58.5 kB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69db1940de
ℹ️ 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".
| const raw = Math.max(wall, monotonic); | ||
| if (raw > this.lastElapsed) this.lastElapsed = raw; | ||
| return this.lastElapsed; |
There was a problem hiding this comment.
If elapsedNow() observes a forward wall-clock jump and the clock is subsequently corrected backward, this high-water clamp remains pinned to the jumped value until performance.now() or wall time catches up. Events accepted after the correction therefore receive expiries relative to the pinned value, so an identical event can remain suppressed for the duration of the jump (for example, nearly an hour) rather than 60 seconds. Fresh evidence beyond the prior backward-clock report is this forward-then-corrected sequence; rebase discontinuous wall-clock observations instead of permanently incorporating the transient maximum.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in a81c122. The wall source is now a sum of forward deltas rather than Date.now() - start under a high-water clamp: a forward step is added (expires early, the safe direction), a backward step adds nothing, and the sum resumes at real pace from the new reading either way. So a forward jump later corrected leaves the clock ahead but advancing, and an event accepted after the correction expires 60s of real time later. Both sources stay non-decreasing, so their max does and the prune's ordering assumption holds. Pinning test: "keeps real pace after a forward wall-clock jump that is later corrected".
69db194 to
227d9d4
Compare
… pin the dedup window Review follow-up on #375. A forward wall-clock step later corrected backwards left the high-water clamp pinned at the stepped value until real time caught up, so events accepted after the correction stayed suppressed for the length of the step. The wall source is now a sum of forward deltas: a step forward is added (expires early, the safe way), a step back adds nothing, and the sum resumes at real pace either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a81c1228f5
ℹ️ 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".
| const wallNow = Date.now(); | ||
| const delta = wallNow - this.lastWall; | ||
| this.lastWall = wallNow; | ||
| if (delta > 0) this.wallElapsed += delta; |
There was a problem hiding this comment.
On platforms where performance.now() pauses during OS suspension, a backward wall-clock adjustment made after the last enqueue but before sleep is never observed as its own delta. If the device wakes while Date.now() is still behind lastWall, this code discards the entire negative delta—including the real time spent asleep—while the monotonic source also shows no progress, so an event repeated after hours away can still be dropped as a duplicate. Fresh evidence beyond the earlier clock findings is this combined unobserved-backstep-then-suspension sequence; the elapsed-time calculation needs to preserve suspension time even when the post-wake wall reading has not caught up to the pre-adjustment value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined, with the bound. The sequence needs a backward wall step larger than the sleep, made after the last enqueue and never observed before suspension, on a platform whose performance.now() pauses in sleep. In that case neither source can see the time asleep: the wall clock reads behind its last sample and the monotonic clock did not run. No clock the browser exposes observes it either (there is no suspend-aware monotonic source in the web platform). The cost is bounded by the window itself: the entry expires after at most 60s of time the clock does observe after wake, which is the same worst case the guard had before this PR for any event. Given the preconditions and the bound, adding a third heuristic here is not worth its surface.
Review follow-up on #375. clear() wiped the whole fingerprint map, which under the rolling window also held entries for delivered and in-flight events. An opt-out while a batch was in flight, followed by an opt-in inside the window, then accepted a copy of an event that had reached the wire. clear() now releases only the buffered items it drops; close() is terminal and still forgets everything. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
… is back Review follow-up on #375. The post-await consent check sampled the predicate, so an opt-out and opt-in inside the hash awaits let the pre-withdrawal event through. clear() now bumps a generation counter that enqueue() captures before its awaits and compares after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
…e; flush re-checks after waiting Review follow-ups on #375. sendBatches sampled consent between split batches, so an opt-out and opt-in while one batch was in flight let the pre-withdrawal batches behind it go out; they had been spliced from the queue, out of clear()'s reach. The flush now captures the clear generation and abandons (and releases) the remaining batches when it changes. flush() also did not re-check state after waiting on a pending flush, so a clear() or close() in that wait left it splicing nothing and POSTing an empty batch. The consent and empty-queue gates are repeated after the wait. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
Review follow-up on #375. The corrected-jump test set the clock forward and back with no read in between, so the forward-delta branch went unexercised. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
… drainAll with a pending flush Coverage review on #375: these were the only branches new to this PR without a test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG
|
Closing for now. The exposure is narrow (only the first event of a page load, double-fired within a minute, before the page hit lands) and the complete fix grew a fair amount of clock and consent machinery to be airtight. The branch stays as a reference: CI green, 22 pinning tests, ten Codex passes with no majors remaining, e2e green against the examples. Reopen if duplicate custom-event counts show up in practice. |
1 participant
Fixes #372.
Why
The queue drops an event whose payload is identical to one accepted recently. That guard exists for double-fire-prone UIs: React strict/dev double effects, double click handlers, a
track()in a re-run effect. Until 1.35.0 the hash lived as long as its event sat in the queue, in practice until the batch timer (30s) or the batch threshold. #326 then started sending the very first event of a page load immediately, and the hash left with it. So the first event of every page load had no duplicate protection at all, and the suppression window for every other event depended on when the last flush happened to run.What changed (
src/queue/EventQueue.ts)payloadHashesis now aMap<fingerprint, expiresAt>. An accepted event suppresses identical ones for 60 seconds from acceptance; flush no longer removes entries.original_timestamp.message_idis unchanged: it keeps its minute-truncated timestamp because it is the event's identity on the wire and must never collide for events a minute apart. Judging by it meant a double-fire straddling a UTC minute boundary (:59.999 / :00.001) was sent twice (review finding).for...ofover aMapdoes not compile under the ES5 target andMap.forEachcannot stop early.performance.now()) and wall elapsed, clamped per instance to never decrease. Backward wall steps are absorbed, so insertion order stays expiry order for the prune; forward steps and OS suspension expire entries on time or early, the safe direction for a duplicate guard (review findings).clear(), can never release a newer acceptance of the same key (review findings).clear()re-checked across every gap.enqueue()re-checks consent after its hash awaits, andclear()bumps a generation counter thatenqueue()andflush()capture before their awaits: an opt-out/opt-in round trip inside a gap still drops the pre-withdrawal event, and abandons (and releases) the split batches behind an in-flight one.flush()also repeats its gates after waiting on a pending flush, so it never POSTs an empty batch (review findings).clear()forgets only what it abandons. Under a rolling window the map also holds delivered and in-flight events;clear()(recoverable, consent withdrawal) releases only the buffered items it drops, so a copy of a delivered event is still a duplicate after an opt-out/opt-in round trip.close()(terminal) forgets everything (review finding).Bundle budget
The change adds about 0.3 kB brotlied on top of #376: 59.04 kB against the 58.75 kB budget #376 set. Raised to 59.5 kB, following the steps of #364, #367, #368 and #376.
Behaviour note
An identical payload is now suppressed for 60s, always. Before, it was suppressed "until the next flush", which was timing dependent. Autocaptured wallet events are unaffected in practice: each carries a hash, batch id, or distinct call data. The one visible case is a batch retried with the exact same calls inside a minute, whose
transaction:startedrows (no id yet) are deduped. That already happened before this change whenever no flush ran in between; now it is deterministic. The examples e2e harness sent the same batch three times in one second and was updated for this: getformo/examples#297.Tests (
test/lib/queue/EventQueue.spec.ts)22 new cases covering: duplicate after the immediate first flush; after a later flush inside the window; accepted again after 60s with the same timestamp; duplicate of a still-queued event; double-fire across a UTC minute boundary; message ids distinct across minutes while content dedup holds; front-only prune (exact survivors, expired key accepted again); consent withdrawn while hashing (and withdrawn-then-restored) does not buffer; failed send releases its fingerprint, delivered send keeps it; an old send failing after the window does not release a newer acceptance; batches abandoned on a mid-flush opt-out, and on an opt-out/opt-in round trip, are released and never sent; a delivered event keeps suppressing across
clear();close()forgets all; no empty POST whenclear()empties the queue mid-wait; consent lost whileflush()waits drops the buffer;drainAlldoes not wait on a pending flush; forward wall step expires, backward step does not reopen, a corrected forward jump keeps real pace; the clock withoutperformance.now. Existing fixtures that relied on hashes vanishing at flush now use distinct events and assert the buffer length.Coverage of
src/queue/EventQueue.ts(c8): 95.7% statements, 89.9% branches. Every uncovered line is pre-existing (defensive outer catch, retry-status branches, page-leave listeners); all code new in this PR is covered.Review
clear()wiping delivered fingerprints (3ea98d9), opt-out/opt-in round trip insideenqueue()(437a913) and inside a split flush plus the empty-POST gap (3e29f15), a test that did not sample the jumped clock (d477dcb). Pass 10: no majors.Verification
pnpm test: 1206 passing (rebased on fix: harden eip1193Fallback against remounts, chain races, and stale wrap state #376).pnpm lint,tsc: clean.pnpm test:e2e15/15 andpnpm test:browser(anvil, headless Chrome): green.sweep.mjs13/13,behaviours.mjs35/35,browser/run.mjs: green.The signed wallet snapshot for #373 was prototyped on this branch and removed before merge; #373 is closed as deferred with the implementation referenced.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YEjK9qkwf3u1CpGkb5NvYG