Skip to content

fix(events): one D1 round trip per upload; stop re-reading never-exported rows - #75

Merged
yaacovcorcos merged 3 commits into
mainfrom
claude/collector-ingest-20261115
Nov 14, 2026
Merged

yaacovcorcos merged 3 commits into
mainfrom
claude/collector-ingest-20261115

Conversation

@yaacovcorcos

Copy link
Copy Markdown
Contributor

What this changes

The events collector (workers/events) does less database work per desktop upload. That shortens the time before Scient gets its acknowledgement, which is part of the G6 analytics send timeouts.

  • One D1 round trip per upload. Each upload used to make 4 sequential D1 calls: a tombstone check, a token check, the write batch, then a re-read for authentication. The tombstone and token checks are now inside the single database.batch. Every write was already guarded by them in SQL. The batch's last statement returns whether this token owns a live, non-deleted installation, and an empty result is the same 403 as before. Deleted installations and wrong tokens still get 403 with nothing written; the existing real-SQL tests cover both.
  • Rows the export never sends are no longer queued. Desktop diagnostics, install.account_observed, install.profile_observed and device install.owner_observed stayed posthog_state='pending' forever, so every export re-read them. In production that is 21,471 rows, all of the pending rows (read-only query, 2026-11-15). They are now written as 'excluded'. The export's SQL and JS filters stay as a backstop.
  • Export runs from the cron only. It no longer runs through waitUntil after every upload. Each 5-minute cron run exports up to 10 batches of 100, inside the existing lease. PostHog now lags D1 by up to about 5 minutes, instead of seconds. D1 stays the primary store, and the 202 acknowledgement never waited on PostHog.
  • Safe stage logging. Each upload logs one structured line: status, events, error_class, read_ms, rate_limit_ms, d1_ms, total_ms. It has no identifiers, tokens, IPs or payloads. The timings exist so a later change can be based on measured server time.
  • The report (analytics-report.mjs) counts excluded as first-party-only. Reconciliation (analytics-reconciliation.mjs) leaves excluded out, where it would otherwise throw "Unknown delivery state".

D1 migration 0008_analytics_export_excluded.sql

SQLite cannot widen a CHECK constraint in place, so the migration rebuilds analytics_events:

  1. Create the new table, with the CHECK allowing 'pending', 'sent' and 'excluded'.
  2. Copy every row, setting never-exported pending rows to 'excluded' (the backfill).
  3. Drop the old table and rename the new one.
  4. Recreate all 6 indexes and all 3 triggers (analytics_no_deleted_event, analytics_daily_event_totals, analytics_daily_active_totals).

The triggers are recreated after the copy, so daily totals are not counted twice. I checked the column, index and trigger list against production's live sqlite_master (read-only). Production holds about 149k rows (DB size 171 MB), so the copy is small.

Deploy order (operator step; not done here)

  1. Apply the migration first: wrangler d1 migrations apply scientfactory-downloads --remote --config workers/events/wrangler.jsonc. The deployed worker (f7abeca) keeps working after it, because it writes the default 'pending'.
  2. Then deploy this worker. Deploying it before the migration would fail every upload that carries a diagnostic event: the CHECK rejects 'excluded', the upload returns 500, and clients retry.
  3. Optional cleanup: rows the old worker inserted between steps 1 and 2 stay 'pending'. This idempotent query moves them:
    UPDATE analytics_events SET posthog_state = 'excluded' WHERE posthog_state = 'pending' AND ((source = 'desktop' AND privacy_level = 'diagnostic') OR event_name IN ('install.account_observed', 'install.profile_observed') OR (event_name = 'install.owner_observed' AND json_valid(properties_json) AND json_extract(properties_json, '$.ownerSource') = 'device'));
  4. Verify: /health shows the new worker_version_tag, the Workers logs show Analytics ingestion lines, and SELECT posthog_state, count(*) FROM analytics_events GROUP BY 1 has no never-exported pending rows.

This PR is independent of the desktop PR ScientFactory/scient-desktop#931. Neither changes the contract (still revision 11), and the two can land and deploy in either order.

Quality Review

  • Correctness of the merged checks. All writes in the batch were already guarded by the token and the tombstone:

    • The identity upsert has WHERE NOT EXISTS tombstone and a token-match WHERE.
    • Event inserts require an identity row with this token.
    • Sightings check the token and the tombstone.
    • Account, profile and device observations require the exact stored event, which is only written under the token.

    A wrong-token or deleted request now runs no-op guarded writes instead of stopping early. The response is the same, and no row changes. The tombstone trigger cannot fire, because deletion erases the identity in the same transaction that writes the tombstone. I updated the comment in functions/_lib/analyticsPeople.ts to say this.

  • Export throughput. At 1,000 events per 5 minutes, the cron exceeds today's ingest by a wide margin (about 25k stable events a week). The loop stops at the first short batch.

  • Reports. I checked every query that reads posthog_state in the worker and scripts: export, report and reconciliation.

  • Not changed: retention scans (not part of the decision), the contract revision, and website-event ingestion (functions/_lib/events.ts still writes 'pending').

Tests

  • bun run check: format, typecheck, events and mirror typecheck, 185 tests passed (2 skipped, as on main), and both builds. Run on Node 24.19 with the live-app env unset.
  • New tests, all failing on main:
    • index.test.ts: one batch with 3 statements, no .first(), 'pending' bound, and no export waitUntil.
    • readiness.test.ts, three real-SQL tests:
      • A diagnostic is written 'excluded' and the session event 'pending', with no export task.
      • 250 pending rows export in one cron run with 3 PostHog requests.
      • The migration backfills each row kind correctly. Totals stay at 5 through the rebuild, then rise to 6 on a new insert. The tombstone trigger still aborts.
    • analytics-reconciliation.test.mjs: the query excludes 'excluded'.
  • Local D1 dry run: wrangler d1 migrations apply --local into a scratch directory, seeded before 0008. Rows were backfilled, all indexes and triggers came back, and totals were not recounted. No remote write, migration or deploy was run.

Preview

No user-visible website change; the Cloudflare page preview doesn't apply.

Documentation impact: Updated — README.md (event export states, cron-only export, ingestion logging)

🤖 Generated with Claude Code

yaacovcorcos and others added 2 commits November 15, 2026 00:54
…rted rows

- Fold the tombstone and token checks into the ingest batch; its final
  statement reports ownership and maps to the same 403.
- Write desktop diagnostics and identity observations as
  posthog_state='excluded' (migration 0008 rebuilds analytics_events to widen
  the CHECK and backfills existing rows), so the export queue holds only rows
  it can send.
- Export events from the five-minute cron only, up to ten batches per run,
  instead of after every upload.
- Log each upload's status, event count and stage durations without
  identifiers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…est batch

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Nov 14, 2026 •

Copy link
Copy Markdown

Deploying scientfactory-website with  Cloudflare Pages  Cloudflare Pages

Latest commit: 045aae5
Status: ✅  Deploy successful!
Preview URL: https://26dd0507.scientfactory-website.pages.dev
Branch Preview URL: https://claude-collector-ingest-2026.scientfactory-website.pages.dev

View logs

Every observation write now also requires the installation's token and no
tombstone, not only its stored event. Found in review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@yaacovcorcos
yaacovcorcos merged commit b90e6ca into main Nov 14, 2026
2 checks passed
@yaacovcorcos
yaacovcorcos deleted the claude/collector-ingest-20261115 branch November 14, 2026 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant