Skip to content

fix(db): erase revenue records with deleted analytics owners - #1116

Open
maskjelly wants to merge 4 commits into
databuddy-analytics:stagingfrom
maskjelly:fix/revenue-erasure
Open

maskjelly wants to merge 4 commits into
databuddy-analytics:stagingfrom
maskjelly:fix/revenue-erasure

Conversation

@maskjelly

@maskjelly maskjelly commented Oct 9, 2026 •

Copy link
Copy Markdown

Description

Website deletion previously left revenue analytics, including customer and profile identifiers and metadata, in ClickHouse. This adds analytics.revenue to the purge registry used by direct deletion and the deleted-owner sweep.

Slice

Changes

  • Delete rows tied to a deleted website and owner-level rows with a null or empty website ID. Preserve rows tied to transferred websites.
  • Use synced_at in retry scans so recently delivered historical payments count as recent.
  • Keep the existing asynchronous ClickHouse mutation lifecycle. A follow-up removed a broad replica wait that could exceed the shared 30-second client timeout.
  • Add unit tests and a ClickHouse integration test that polls for the eventual result. Wire the database package's integration suite into test:integration so the new test runs without a skip gate.

Validation

  • bun run lint: passed.
  • bun run check-types: 37/37 tasks passed.
  • bun test src/clickhouse/purge.test.ts: 5 passed.
  • The ClickHouse integration test was not run locally because no local ClickHouse service is available. CI and maintainer validation remain needed.

Merge notes

This is an independent change based on current staging. Mutation submission precedes the API response, while completion can be asynchronous; the existing retry sweep handles submission failures. CI runs the database integration command against its ClickHouse service. Maintainer review is still required.

Before merge

  • Maintainer applies the suggested labels and reviews the change.
  • Run the fork CI workflow (currently action_required with zero jobs) and confirm the ClickHouse integration suite passes.
  • Resolve the Vercel authorization check failures shown on the PR.

AI disclosure: implemented and tested with Codex. Source and local test results were reviewed during implementation; maintainer human review is required.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 4b23c3e6-2e14-459a-a7dc-227cd153c442

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@maskjelly
maskjelly marked this pull request as ready for review October 9, 2026 22:37
@vercel

vercel Bot commented Oct 9, 2026

Copy link
Copy Markdown

@maskjelly is attempting to deploy a commit to the Databuddy OSS Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps

greptile-apps Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Critical impact] Adds revenue record deletion to the data purge system.

The revenue deletion changes have no established blocking behavior defect, but the two explicit test-code requirements must be satisfied before merging.

Findings

  1. P2 New tests skip by default ▶
  2. P2 Mocks use unnecessary async ▶

Summary

Revenue now participates in website deletion and deleted-owner retry scans.

  • Deleting a website also removes its revenue records.
  • Purge batches wait for replicas before they are acknowledged.

Acknowledged tradeoff: maskjelly states that waiting for replicas can increase purge duration or fail when a replica is unavailable.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Website deletion or deleted-owner retry] --> B[Purge tenant rows including revenue]
  B --> C[Wait for replica completion]
  C -->|Success| D[Call batch callback]
  D --> E[Retry job records purge audit]
  C -->|Failure| F[Existing error and retry path]
Loading

Reviews (1) · Last reviewed commit: "fix(db): erase revenue records with dele..." · Reviewed by Greptile

Comment thread packages/db/src/clickhouse/purge.integration.test.ts Outdated
Comment thread packages/db/src/clickhouse/purge.test.ts Outdated

This branch has not been deployed

No deployments
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