fix: harden analytics delivery - #581
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Unkey Deploy
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
Greptile SummaryThis PR hardens analytics and uptime delivery across ingestion, relay, querying, and browser retry paths.
Confidence Score: 4/5The concurrent pending-reservation path must be fixed before merging because it allows the same core analytics event to be durably delivered more than once. A second request observing an existing pending Redis token is admitted without reservation ownership, so concurrent retries can both publish the same stable event into tables that retain duplicate rows. Files Needing Attention: apps/basket/src/lib/security.ts, apps/basket/src/lib/event-service.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Tracker
participant B as Basket
participant R as Redis dedup
participant K as Redpanda
participant C as ClickHouse
T->>B: analytics event
B->>R: reserve stable event ID
R-->>B: acquired / pending / delivered
B->>K: acknowledged publish
alt Kafka unavailable
B->>C: acknowledged direct insert
end
B->>R: mark delivered
B-->>T: success or retryable 503
Reviews (1): Last reviewed commit: "fix(basket): durably admit core analytic..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // than falsely reporting success for an event that never reached storage. | ||
| return { | ||
| duplicate: false, | ||
| key, | ||
| token: result === "acquired" ? token : undefined, | ||
| ttl, | ||
| }; |
There was a problem hiding this comment.
When two requests submit the same client, event type, and event ID concurrently, the second request treats the first request's pending token as duplicate: false and proceeds without owning the reservation. Both requests can durably publish the same stable ID into MergeTree-backed analytics tables, producing duplicate rows and inflated analytics.
Knowledge Base Used: Basket Ingestion Flow
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 39 files
Confidence score: 2/5
- In
packages/tracker/src/plugins/pixel.ts, in-flight pixel requests can still complete after opt-out/clear, and deliveries can hang indefinitely on unresolved image events, which risks both privacy leakage and a stuck tracker queue; track/cancel activeImagerequests, add timeout/abort handling, and sanitizemaxRetriesto a finite non-negative integer. - In
apps/basket/src/lib/event-service.ts, releasing the retry guard after transport-level failures can replay events that were already accepted, inflating analytics totals in ClickHouse/Redpanda; keep the guard aligned to durable delivery semantics or add stronger idempotency on retries. - In
apps/basket/src/lib/security.ts, legacy dedup values ("1") are treated as someone else’s pending reservation, so valid retries fail for the full TTL and can drop expected reprocessing; treat the legacy delivered marker as delivered during the migration window. - In
apps/basket/src/index.ts, shutdown now leaves very little time after producer drain before hard termination, increasing the chance Redis/Postgres/log flushes are cut off; rebalance the shutdown timeout budget so serial drain and cleanup can complete reliably.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ai/src/query/builders/uptime.ts">
<violation number="1" location="packages/ai/src/query/builders/uptime.ts:21">
P3: This change adds the same 22-column `UPTIME_REPLAY_IDENTITY` and the same `uptimeEventSource()` helper in two separate packages (ai and rpc) with no shared source of truth or cross-reference comment. If a column is ever added to the table or the replay identity list is adjusted in one file, the two sites will silently deduplicate differently (query-time anti-replay behavior diverges), and the divergence won't be caught. Since the two packages can't easily share a module, at minimum add a cross-reference comment on each definition pointing at the other location (and at the schema file) so future edits prompt checking the paired copy.</violation>
<violation number="2" location="packages/ai/src/query/builders/uptime.ts:58">
P2: Dashboard queries sort the entire selected uptime range before deduplication, adding avoidable work to every uptime aggregate and window query. Since the identity covers every stored column, selection order cannot affect the retained row; remove this inner sort and let consumers that need presentation ordering keep their outer `ORDER BY`.</violation>
</file>
<file name="apps/basket/src/index.ts">
<violation number="1" location="apps/basket/src/index.ts:89">
P2: A full producer drain now leaves less than two seconds before the hard shutdown timer terminates Redis/Postgres and log flushing. Consider budgeting the process timeout for this serial drain plus cleanup, so a successful near-deadline delivery does not routinely skip the remaining shutdown work.</violation>
</file>
<file name="packages/rpc/src/routers/status-page.ts">
<violation number="1" location="packages/rpc/src/routers/status-page.ts:72">
P2: Public status-page refreshes now sort every selected check before deduplication, although each `LIMIT BY` key contains every physical `uptime_monitor` column. Exact duplicates are indistinguishable downstream, so remove this sort to avoid unnecessary sort and memory work for the 90-day query.</violation>
</file>
<file name="apps/basket/src/lib/security.ts">
<violation number="1" location="apps/basket/src/lib/security.ts:165">
P2: Retries matching dedup keys created before this rollout return retryable failures for the rest of their TTL, because prior delivered value `"1"` is classified as another request's pending reservation. Treat that legacy value as delivered during the transition.</violation>
</file>
<file name="packages/tracker/src/plugins/pixel.ts">
<violation number="1" location="packages/tracker/src/plugins/pixel.ts:67">
P2: A negative or non-finite runtime `maxRetries` can drop every pixel without even one delivery attempt. Normalize it to a finite non-negative integer before using it as the loop bound.</violation>
<violation number="2" location="packages/tracker/src/plugins/pixel.ts:116">
P2: Every delivery now blocks on the Image onload/onerror promise with no timeout or abort path, and a pixel whose load never settles leaves BaseTracker's queue permanently stuck. Because sendBeacon now returns false unconditionally, the unload/destroy and normal flush paths all go through this api.fetch image-load promise. If a request is aborted by navigation or the browser never fires onload/onerror for it, `await load()` never resolves, so `api.fetch` never returns. In `_flushQueue`, the `finally` block only clears `meta.flushing` when the fetch resolves — so this queue stays `flushing: true` forever and every subsequent event is queued but never sent. The previous sendBeacon path returned `true` immediately (clearing the queue) and fired the Image in the background, so this hang could not previously brick a queue. Consider wrapping `load()` in a timeout (e.g. racing against the existing MAX_PIXEL_RETRY_DELAY_MS) and treating a timeout as a failed attempt so the promise always settles.</violation>
<violation number="3" location="packages/tracker/src/plugins/pixel.ts:124">
P1: Opting out or clearing during an in-flight pixel load still lets that pixel request finish and transmit its event. Track active `Image` instances and cancel them when `cancelPendingRequests` runs; generation checks only prevent later retries.</violation>
</file>
<file name="apps/basket/src/lib/event-service.ts">
<violation number="1" location="apps/basket/src/lib/event-service.ts:225">
P1: Ambiguous delivery retries can inflate core analytics: this releases the guard after a rejection even if ClickHouse/Redpanda accepted the event before the response failed. Stable IDs alone do not deduplicate these `ReplicatedMergeTree` rows, so preserve an ambiguous reservation or add ID-level deduplication before permitting retry.</violation>
</file>
<file name="apps/uptime/src/worker.ts">
<violation number="1" location="apps/uptime/src/worker.ts:448">
P2: Corrupt or legacy persisted delivery jobs containing only these three fields pass validation and are sent to Redpanda as `UptimeData`. Validate every required `UptimeData` field (and `json_data` when present) before enqueue/replay so invalid data fails the job instead.
(Based on your team's feedback about runtime type validation.) [FEEDBACK_USED].</violation>
</file>
<file name="packages/ai/src/ai/agents/cache.test.ts">
<violation number="1" location="packages/ai/src/ai/agents/cache.test.ts:58">
P2: Spreading `...redisModule` into the mock re-exposes the real Redis-backed implementations that the old per-export stubs deliberately isolated. The internal redis functions (`cacheable`, the `invalidate*` helpers, and the `redis` proxy) call the module's own `getRedisCache`, which constructs a real `ioredis` client — they do not honor the `getRedisCache: () => mockRedisClient` override you added, because they import the internal function rather than the top-level export. `cache.ts` already binds itself to the real `cacheable` at import time. Today the specific assertions happen to avoid triggering those paths, but the isolation is gone: the first unit-under-test path that calls them will try to reach real infrastructure instead of `mockRedisClient`. Consider keeping explicit stubs for any redis/queue/bullmq-touching exports you don't want to run for real.</violation>
</file>
<file name="packages/ai/src/ai/mcp/conversation-store.test.ts">
<violation number="1" location="packages/ai/src/ai/mcp/conversation-store.test.ts:25">
P2: This mock now derives its whole export surface from a top-level `const redisModule = { ...actualRedis }` that is referenced inside the hoisted `vi.mock("@databuddy/redis")` factory. Vitest hoists `vi.mock` above all imports and runs the factory when the mocked module is first resolved — and this file now also imports `@databuddy/redis` statically at the top, so that resolution can occur before `redisModule` is initialized. That is the exact closure-over-top-level-binding case Vitest documents as causing "Cannot access ... before initialization" errors, and it will break as soon as module resolution timing shifts. Prefer `vi.hoisted` for the shared reference (or `vi.importActual` inside the factory) so the base module is captured safely, and consider restoring explicit stubs for the redis-touching exports so the test stays isolated.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Browser / Device
participant Basket as Basket API
participant Kafka as Redpanda (Kafka)
participant Vector as Vector (Infra)
participant ClickHouse as ClickHouse
participant Redis as Redis (Dedup + BullMQ)
participant BullMQ as BullMQ Queue (Uptime)
participant Uptime as Uptime Service
participant Worker as Uptime Delivery Worker
participant DB as PostgreSQL
Note over Client,ClickHouse: Analytics Event Ingestion (Basket)
Client->>Basket: POST /events, POST /batch, GET /px.jpg
Basket->>Redis: reserveDuplicate(deliveryId, eventType, sourceEventId)
alt Duplicate found
Redis-->>Basket: duplicate: true
Basket-->>Client: 200 (silent skip)
else Retryable (owned by another request)
Redis-->>Basket: retryable: true
Basket-->>Client: 503 + Retry-After
else New event or Redis unavailable
Redis-->>Basket: reservation token
Basket->>Kafka: send(topic, event) with stable event ID
alt Kafka succeeds
Kafka-->>Basket: acknowledged
Basket->>Redis: markDuplicateReservationDelivered(token)
else Kafka fails
Basket->>ClickHouse: direct fallback insert (clickhouseDirectFallbackInsert)
alt ClickHouse succeeds
ClickHouse-->>Basket: acknowledged
Basket->>Redis: markDuplicateReservationDelivered(token)
else ClickHouse fails
Basket->>Redis: releaseDuplicateReservation(token)
Basket-->>Client: 503 + Retry-After
end
end
end
Note over ClickHouse: Vector reads from Redpanda
Vector->>Kafka: consume analytics-* topics
Vector->>ClickHouse: batch insert with acknowledgements enabled
alt ClickHouse acknowledges
Vector->>Kafka: commit offset
else ClickHouse fails
Vector-->>Vector: retain offset / retry later
end
Note over Uptime,DB: Uptime Monitoring Flow
BullMQ->>Uptime: schedule trigger (uptime-check job)
Uptime->>DB: lookup schedule
DB-->>Uptime: schedule + website config
Uptime->>Uptime: runUptimeCheck() → produce UptimeData with event_id
Uptime->>Redis: checkpoint: update job data with probe result
alt Checkpoint fails
Redis-->>Uptime: error
Uptime-->>BullMQ: retry job (source level retry)
else Checkpoint succeeds
Uptime->>BullMQ: enqueueUptimeDelivery() → add to uptime-event-delivery queue
alt Enqueue fails
Redis-->>Uptime: error
Uptime-->>BullMQ: retry job (source level retry)
else Enqueue succeeds
BullMQ->>Worker: deliver uptime-event-delivery job
Worker->>Kafka: sendUptimeEvent() with acks=-1
alt Kafka acknowledges
Kafka-->>Worker: success
Worker->>Redis: job completed (idempotent via event_id)
else Kafka fails
Worker-->>BullMQ: retry (fixed delay, up to 1M attempts)
BullMQ->>Worker: retry delivery with same event data
end
Uptime->>Uptime: fireTransitionAlerts()
end
end
Note over ClickHouse,Worker: Query-Time Dedup for Replayed Uptime
Worker->>ClickHouse: SELECT ... LIMIT 1 BY [all check fields]
ClickHouse-->>Worker: deduplicated results (replayed payloads collapsed)
Note over Client,Basket: Pixel Retry by Tracker SDK
Client->>Basket: GET /px.jpg (pixel request)
alt Delivery fails (503)
Basket-->>Client: 503, no image body, Retry-After: 5
Client->>Client: retry after capped exponential backoff
Client->>Basket: same eventId, stable analytics ID
else Success
Basket-->>Client: 200, transparent pixel GIF
end
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| if (generation !== deliveryGeneration) { | ||
| return { attempts: retry, success: false }; | ||
| } | ||
| if (await load()) { |
There was a problem hiding this comment.
P1: Opting out or clearing during an in-flight pixel load still lets that pixel request finish and transmit its event. Track active Image instances and cancel them when cancelPendingRequests runs; generation checks only prevent later retries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tracker/src/plugins/pixel.ts, line 124:
<comment>Opting out or clearing during an in-flight pixel load still lets that pixel request finish and transmit its event. Track active `Image` instances and cancel them when `cancelPendingRequests` runs; generation checks only prevent later retries.</comment>
<file context>
@@ -76,36 +107,57 @@ export function initPixelTracking(tracker: BaseTracker) {
+ if (generation !== deliveryGeneration) {
+ return { attempts: retry, success: false };
+ }
+ if (await load()) {
+ return { attempts: retry + 1, success: true };
+ }
</file context>
|
|
||
| await runPromise(send("analytics-events", trackEvent)); | ||
| } catch (error) { | ||
| await releaseDuplicateReservation(reservation); |
There was a problem hiding this comment.
P1: Ambiguous delivery retries can inflate core analytics: this releases the guard after a rejection even if ClickHouse/Redpanda accepted the event before the response failed. Stable IDs alone do not deduplicate these ReplicatedMergeTree rows, so preserve an ambiguous reservation or add ID-level deduplication before permitting retry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/basket/src/lib/event-service.ts, line 225:
<comment>Ambiguous delivery retries can inflate core analytics: this releases the guard after a rejection even if ClickHouse/Redpanda accepted the event before the response failed. Stable IDs alone do not deduplicate these `ReplicatedMergeTree` rows, so preserve an ambiguous reservation or add ID-level deduplication before permitting retry.</comment>
<file context>
@@ -130,54 +169,64 @@ export function insertTrackEvent(
+
+ await runPromise(send("analytics-events", trackEvent));
+ } catch (error) {
+ await releaseDuplicateReservation(reservation);
+ throw deliveryUnavailable(error);
+ }
</file context>
| ORDER BY timestamp DESC | ||
| LIMIT 1 BY ${UPTIME_REPLAY_IDENTITY} |
There was a problem hiding this comment.
P2: Dashboard queries sort the entire selected uptime range before deduplication, adding avoidable work to every uptime aggregate and window query. Since the identity covers every stored column, selection order cannot affect the retained row; remove this inner sort and let consumers that need presentation ordering keep their outer ORDER BY.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/query/builders/uptime.ts, line 58:
<comment>Dashboard queries sort the entire selected uptime range before deduplication, adding avoidable work to every uptime aggregate and window query. Since the identity covers every stored column, selection order cannot affect the retained row; remove this inner sort and let consumers that need presentation ordering keep their outer `ORDER BY`.</comment>
<file context>
@@ -18,6 +18,47 @@ import type { SimpleQueryConfig } from "../types";
+ FROM ${UPTIME_TABLE}
+ WHERE
+ ${scope}
+ ORDER BY timestamp DESC
+ LIMIT 1 BY ${UPTIME_REPLAY_IDENTITY}
+ )`;
</file context>
| ORDER BY timestamp DESC | |
| LIMIT 1 BY ${UPTIME_REPLAY_IDENTITY} | |
| LIMIT 1 BY ${UPTIME_REPLAY_IDENTITY} |
| const { shutdownRedis } = await import("@databuddy/redis"); | ||
| // Wait for acknowledged delivery before tearing down its dependencies. | ||
| try { | ||
| await runPromise(disconnect); |
There was a problem hiding this comment.
P2: A full producer drain now leaves less than two seconds before the hard shutdown timer terminates Redis/Postgres and log flushing. Consider budgeting the process timeout for this serial drain plus cleanup, so a successful near-deadline delivery does not routinely skip the remaining shutdown work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/basket/src/index.ts, line 89:
<comment>A full producer drain now leaves less than two seconds before the hard shutdown timer terminates Redis/Postgres and log flushing. Consider budgeting the process timeout for this serial drain plus cleanup, so a successful near-deadline delivery does not routinely skip the remaining shutdown work.</comment>
<file context>
@@ -79,13 +84,35 @@ async function gracefulShutdown(signal: string, exitCode = 0) {
const { shutdownRedis } = await import("@databuddy/redis");
+ // Wait for acknowledged delivery before tearing down its dependencies.
+ try {
+ await runPromise(disconnect);
+ } catch (error) {
+ finalExitCode = 1;
</file context>
| FROM ${UPTIME_TABLE} | ||
| WHERE | ||
| ${scope} | ||
| ORDER BY timestamp DESC |
There was a problem hiding this comment.
P2: Public status-page refreshes now sort every selected check before deduplication, although each LIMIT BY key contains every physical uptime_monitor column. Exact duplicates are indistinguishable downstream, so remove this sort to avoid unnecessary sort and memory work for the 90-day query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rpc/src/routers/status-page.ts, line 72:
<comment>Public status-page refreshes now sort every selected check before deduplication, although each `LIMIT BY` key contains every physical `uptime_monitor` column. Exact duplicates are indistinguishable downstream, so remove this sort to avoid unnecessary sort and memory work for the 90-day query.</comment>
<file context>
@@ -33,6 +33,46 @@ import {
+ FROM ${UPTIME_TABLE}
+ WHERE
+ ${scope}
+ ORDER BY timestamp DESC
+ LIMIT 1 BY ${UPTIME_REPLAY_IDENTITY}
+ )`;
</file context>
| timestamp?: unknown; | ||
| }; | ||
| return ( | ||
| typeof event.event_id === "string" && |
There was a problem hiding this comment.
P2: Corrupt or legacy persisted delivery jobs containing only these three fields pass validation and are sent to Redpanda as UptimeData. Validate every required UptimeData field (and json_data when present) before enqueue/replay so invalid data fails the job instead.
(Based on your team's feedback about runtime type validation.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/uptime/src/worker.ts, line 448:
<comment>Corrupt or legacy persisted delivery jobs containing only these three fields pass validation and are sent to Redpanda as `UptimeData`. Validate every required `UptimeData` field (and `json_data` when present) before enqueue/replay so invalid data fails the job instead.
(Based on your team's feedback about runtime type validation.) .</comment>
<file context>
@@ -360,30 +416,156 @@ export async function processUptimeCheck(
+ timestamp?: unknown;
+ };
+ return (
+ typeof event.event_id === "string" &&
+ typeof event.site_id === "string" &&
+ typeof event.timestamp === "number"
</file context>
| getLinkCacheKey: vi.fn((slug: string) => `link:${slug}`), | ||
| getRateLimitHeaders: vi.fn(() => ({})), | ||
| getInsightsQueue: vi.fn(() => ({})), | ||
| ...redisModule, |
There was a problem hiding this comment.
P2: Spreading ...redisModule into the mock re-exposes the real Redis-backed implementations that the old per-export stubs deliberately isolated. The internal redis functions (cacheable, the invalidate* helpers, and the redis proxy) call the module's own getRedisCache, which constructs a real ioredis client — they do not honor the getRedisCache: () => mockRedisClient override you added, because they import the internal function rather than the top-level export. cache.ts already binds itself to the real cacheable at import time. Today the specific assertions happen to avoid triggering those paths, but the isolation is gone: the first unit-under-test path that calls them will try to reach real infrastructure instead of mockRedisClient. Consider keeping explicit stubs for any redis/queue/bullmq-touching exports you don't want to run for real.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/ai/agents/cache.test.ts, line 58:
<comment>Spreading `...redisModule` into the mock re-exposes the real Redis-backed implementations that the old per-export stubs deliberately isolated. The internal redis functions (`cacheable`, the `invalidate*` helpers, and the `redis` proxy) call the module's own `getRedisCache`, which constructs a real `ioredis` client — they do not honor the `getRedisCache: () => mockRedisClient` override you added, because they import the internal function rather than the top-level export. `cache.ts` already binds itself to the real `cacheable` at import time. Today the specific assertions happen to avoid triggering those paths, but the isolation is gone: the first unit-under-test path that calls them will try to reach real infrastructure instead of `mockRedisClient`. Consider keeping explicit stubs for any redis/queue/bullmq-touching exports you don't want to run for real.</comment>
<file context>
@@ -44,130 +45,18 @@ const mockEnrichAgentContext = vi.fn(
- getLinkCacheKey: vi.fn((slug: string) => `link:${slug}`),
- getRateLimitHeaders: vi.fn(() => ({})),
- getInsightsQueue: vi.fn(() => ({})),
+ ...redisModule,
getRedisCache: () => mockRedisClient,
- getUptimeQueue: vi.fn(() => ({})),
</file context>
| }), | ||
| }; | ||
|
|
||
| const redisModule = { ...actualRedis }; |
There was a problem hiding this comment.
P2: This mock now derives its whole export surface from a top-level const redisModule = { ...actualRedis } that is referenced inside the hoisted vi.mock("@databuddy/redis") factory. Vitest hoists vi.mock above all imports and runs the factory when the mocked module is first resolved — and this file now also imports @databuddy/redis statically at the top, so that resolution can occur before redisModule is initialized. That is the exact closure-over-top-level-binding case Vitest documents as causing "Cannot access ... before initialization" errors, and it will break as soon as module resolution timing shifts. Prefer vi.hoisted for the shared reference (or vi.importActual inside the factory) so the base module is captured safely, and consider restoring explicit stubs for the redis-touching exports so the test stays isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/ai/mcp/conversation-store.test.ts, line 25:
<comment>This mock now derives its whole export surface from a top-level `const redisModule = { ...actualRedis }` that is referenced inside the hoisted `vi.mock("@databuddy/redis")` factory. Vitest hoists `vi.mock` above all imports and runs the factory when the mocked module is first resolved — and this file now also imports `@databuddy/redis` statically at the top, so that resolution can occur before `redisModule` is initialized. That is the exact closure-over-top-level-binding case Vitest documents as causing "Cannot access ... before initialization" errors, and it will break as soon as module resolution timing shifts. Prefer `vi.hoisted` for the shared reference (or `vi.importActual` inside the factory) so the base module is captured safely, and consider restoring explicit stubs for the redis-touching exports so the test stays isolated.</comment>
<file context>
@@ -21,129 +22,16 @@ const mockRedisClient = {
}),
};
+const redisModule = { ...actualRedis };
+
vi.mock("@databuddy/redis", () => ({
</file context>
| const img = new Image(); | ||
| img.onload = () => resolve(true); | ||
| img.onerror = () => resolve(false); | ||
| img.src = url.toString(); |
There was a problem hiding this comment.
P2: Every delivery now blocks on the Image onload/onerror promise with no timeout or abort path, and a pixel whose load never settles leaves BaseTracker's queue permanently stuck. Because sendBeacon now returns false unconditionally, the unload/destroy and normal flush paths all go through this api.fetch image-load promise. If a request is aborted by navigation or the browser never fires onload/onerror for it, await load() never resolves, so api.fetch never returns. In _flushQueue, the finally block only clears meta.flushing when the fetch resolves — so this queue stays flushing: true forever and every subsequent event is queued but never sent. The previous sendBeacon path returned true immediately (clearing the queue) and fired the Image in the background, so this hang could not previously brick a queue. Consider wrapping load() in a timeout (e.g. racing against the existing MAX_PIXEL_RETRY_DELAY_MS) and treating a timeout as a failed attempt so the promise always settles.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tracker/src/plugins/pixel.ts, line 116:
<comment>Every delivery now blocks on the Image onload/onerror promise with no timeout or abort path, and a pixel whose load never settles leaves BaseTracker's queue permanently stuck. Because sendBeacon now returns false unconditionally, the unload/destroy and normal flush paths all go through this api.fetch image-load promise. If a request is aborted by navigation or the browser never fires onload/onerror for it, `await load()` never resolves, so `api.fetch` never returns. In `_flushQueue`, the `finally` block only clears `meta.flushing` when the fetch resolves — so this queue stays `flushing: true` forever and every subsequent event is queued but never sent. The previous sendBeacon path returned `true` immediately (clearing the queue) and fired the Image in the background, so this hang could not previously brick a queue. Consider wrapping `load()` in a timeout (e.g. racing against the existing MAX_PIXEL_RETRY_DELAY_MS) and treating a timeout as a failed attempt so the promise always settles.</comment>
<file context>
@@ -76,36 +107,57 @@ export function initPixelTracking(tracker: BaseTracker) {
+ const img = new Image();
+ img.onload = () => resolve(true);
+ img.onerror = () => resolve(false);
+ img.src = url.toString();
+ });
+
</file context>
| */ | ||
|
|
||
| const UPTIME_TABLE = "uptime.uptime_monitor"; | ||
| const UPTIME_REPLAY_IDENTITY = [ |
There was a problem hiding this comment.
P3: This change adds the same 22-column UPTIME_REPLAY_IDENTITY and the same uptimeEventSource() helper in two separate packages (ai and rpc) with no shared source of truth or cross-reference comment. If a column is ever added to the table or the replay identity list is adjusted in one file, the two sites will silently deduplicate differently (query-time anti-replay behavior diverges), and the divergence won't be caught. Since the two packages can't easily share a module, at minimum add a cross-reference comment on each definition pointing at the other location (and at the schema file) so future edits prompt checking the paired copy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/query/builders/uptime.ts, line 21:
<comment>This change adds the same 22-column `UPTIME_REPLAY_IDENTITY` and the same `uptimeEventSource()` helper in two separate packages (ai and rpc) with no shared source of truth or cross-reference comment. If a column is ever added to the table or the replay identity list is adjusted in one file, the two sites will silently deduplicate differently (query-time anti-replay behavior diverges), and the divergence won't be caught. Since the two packages can't easily share a module, at minimum add a cross-reference comment on each definition pointing at the other location (and at the schema file) so future edits prompt checking the paired copy.</comment>
<file context>
@@ -18,6 +18,47 @@ import type { SimpleQueryConfig } from "../types";
*/
const UPTIME_TABLE = "uptime.uptime_monitor";
+const UPTIME_REPLAY_IDENTITY = [
+ "site_id",
+ "url",
</file context>
|
Superseded by #585, rebuilt cleanly against current staging. |
1 participant
Summary
Constraints
Verification
Summary by cubic
Strengthened analytics and uptime delivery: Redpanda remains the durable source with ClickHouse‑acknowledged sinks, Basket drains on shutdown and prevents concurrent delivery, and uptime checks relay through BullMQ with validated payloads and long‑lived retries.
Refactors
effectruntime; clarified direct ClickHouse fallback behavior; stable IDs viastableAnalyticsEventIdwith batchsourceEventId.zodvalidation for job envelopes and payloads.TABLE_COLUMNSacross@databuddy/aiand@databuddy/rpc.Migration
infra/ingest/vector.yamlwith acknowledgements enabled.Written for commit 844f2ec. Summary will update on new commits.