[themarolt]

Summary

Adds a hand-run script that removes the NULL-channel duplicates in activityRelations. Before #850, every re-send of an activity without a channel inserted a new row, because NULLs are distinct in the dedup index. The script keeps one copy per key and moves the others to a dated backup table. The copy it keeps is the one data-sink-worker already reads: newest by updatedAt, createdAt, activityId, the same order as findActivityRelationsByDedupKeys. It runs in short transactions that can be stopped and resumed.

Changes

• collect walks the dedup index in time windows. It records every key with more than one NULL-channel copy for the chosen platforms in a work table. Windows halve on a statement timeout, days without rows are skipped, and it resumes from the last finished window.
• clean takes whole keys, up to --batch-rows rows per transaction:
  • It locks the copies FOR UPDATE NOWAIT, so it never waits on app transactions.
  • It ranks only the copies it locked and keeps rank 1.
  • It copies parentId, conversationId and sourceParentId onto the kept copy when that copy has none.
  • It moves the kept copy's updatedAt at least 2 ms past every deleted copy's, so CDC and Tinybird see it as newest.
  • It deletes the other copies into the backup table, checks in the same transaction that every key kept exactly one locked copy and that every deleted row was backed up, and rolls back otherwise.
• Guards:
  • waits on Sequin replication-slot lag, and stops if a slot is missing;
  • won't start during the nightly meetings export window (00:30–05:30 UTC);
  • one run per tag (advisory lock);
  • --max-rows / --until required for a real run;
  • retries lock, deadlock and timeout errors, and splits a batch on timeout;
  • keys that keep failing are deferred and can be retried later with --retry-deferred.
• collect --rescan scans from the oldest row again after clean. It re-opens keys that got duplicates in the meantime, and a second clean handles them.
• Observability: JSON log events per batch and per window, periodic progress with ETA and backup size, a per-batch metrics table, and status for progress at any time. Exit codes tell a wrapper whether to restart (2 stopped, 4 connection lost) or stop and look (1).
• The script uses a raw pinned pg-promise session instead of queryExecutor, on purpose. The advisory run lock, the session temp table of locked ids and the set local timeouts all have to live on one connection for the whole run.
• Size: about 1900 lines, all in one new file. The script only makes sense whole, since collect without clean does nothing in prod. It reads best commit by commit: collect, clean, guards, then the review fixes.
• Tested locally against synthetic data with concurrent writers that imitate data-sink-worker, affiliation refresh, member merge with cascade delete, channel updates and new copies. It was also killed mid-run, interrupted with SIGINT/SIGTERM, and resumed. No wrong row was deleted, no key was left empty and no backup row was lost.
• Depends on fix: keep activity relation batch moves going when rows disappear concurrently #5258, the batch-loop fix in member/organization merge and affiliation refresh, which is already deployed.

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Performance improvement
• Chore / dependency update
• Documentation

Related issue

Refs linuxfoundation/lfx-cdp-insights-ops#852

[cursor]

PR Summary

High Risk
Bulk deletes/updates on activityRelations with backup moves; mis-runs could corrupt analytics or lose rows despite guards. Requires careful operational review and staging validation.

Overview
Adds a hand-run maintenance tool for legacy NULL-channel duplicates on activityRelations: collect walks time windows and fills per-tag work/progress/batch tables; clean runs bounded transactions that FOR UPDATE NOWAIT lock copies, keep the same row data-sink would (rank by updatedAt, createdAt, activityId), merge missing parent/thread fields onto the survivor, bump updatedAt for CDC/Tinybird, delete extras into a dated backup table with invariant checks, and supports retries, --max-rows/--until, Sequin slot lag, nightly window, and deferred keys; status reports progress.

Wires pnpm run cleanup-null-channel-duplicates in script_executor_worker to src/bin/cleanup-null-channel-duplicates.ts.

^{Reviewed by Cursor Bugbot for commit 385e57b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[greptile-apps]

Confidence Score: 5/5

[High impact] The PR appears safe to merge; no new actionable issues were found.

Summary

Adds a resumable script to collect and remove NULL-channel duplicates from activityRelations, with backups, limits, retries, and progress reports.

• Clean keeps one activity row and backs up the rest.
• Collect scans duplicate activity keys a window at a time.
• Operators can launch the cleanup with a tagged, bounded command.

_{Reviews (12) · Last reviewed commit: "fix: stop a null-channel collect whose p..." · Reviewed by Greptile}

[Copilot]

🔵 Needs a closer look

Cleanup retries and split batches can bypass stop, time, and replication-lag safeguards, while batch deletion can exceed the configured maximum row limit.

No open issues 🎉

What changed in this PR

Adds a hand-run maintenance script to remove NULL-channel duplicates from activityRelations, retaining the copy selected by data-sink-worker and backing up deleted rows. It builds on the concurrent batch-move fix in #5258.

Changes:

• Adds resumable key collection, transactional cleanup, and status reporting.
• Preserves relationship fields and adjusts survivor timestamps for downstream replication, with safety checks, retries, and operational guards.
• Registers a package command for running the script.

File	Description
services/​apps/​script_executor_worker/​src/​bin/​cleanup-null-channel-duplicates.ts	Implements collection, cleanup, backups, guards, and progress reporting.
services/​apps/​script_executor_worker/​package.json	Registers the cleanup script command.

_{Review effort: Balanced}

[cursor]

Stale Bugbot comment from a previous run.

[themarolt]

Re: Copilot review · summary findings

• Retries and split batches bypass stop, time and replication-lag guards
  Fixed in 7e81c3f. Every batch attempt, retry and split half now re-checks the stop conditions and waits on slot lag before it starts (details in the Greptile thread on processKeys).
• Deletion can exceed the configured maximum row limit
  Fixed in 7e81c3f. Batches are trimmed to the remaining --max-rows allowance. Only a single key can still go over, because keys are never split, and the help text says so.

[Copilot]

🔵 Needs a closer look

Unresolved checkpoint and rescan-resume defects, incorrect nightly-stop exit handling, and ineffective dry-run sample limits can block valid cleanup runs and exceed intended query bounds.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The cleanup can exceed its deletion limit, fail to resume interrupted rescans, and incorrectly reject collections completed in separate platform runs.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed because skipped keeper timestamp updates can let replicated DELETE before-images replace keeper metadata and propagate deleted-copy metadata into daily snapshots.

No open issues 🎉

_{Review effort: Balanced}

[themarolt]

Re: Copilot review · summary findings

• Skipped kept-row timestamp updates could let replicated DELETE before-images replace the kept row's data
  Not changed. The script only skips updating the kept row when the kept row is already strictly newer, so a DELETE before-image can't win in either Tinybird layer:
  • When the kept row is left alone. That only happens when kept.updatedAt >= date_trunc('ms', max(dropped.updatedAt)) + 2 ms, so it is strictly newer at the millisecond precision Tinybird stores.
  • Raw layer. A DELETE before-image comes in through $.record.* as a row with the dropped copy's own updatedAt. activityRelations is a ReplacingMergeTree on the dedup key with ENGINE_VER updatedAt, so the kept version wins.
  • Served layer. The MV snapshotId is the day of the row's updatedAt, plus one. A dropped before-image therefore never has a later snapshotId than the kept row. When both fall in the merge window, LIMIT 1 BY ... ORDER BY updatedAt DESC picks the kept row. When the kept row's day was merged earlier, the before-image's day is no later, so it is outside the window as well.
  • Kept rows the script updates get now() (3d18994), so they win everywhere.
  • Copies with a different createdAt year sit in other partitions, which ReplacingMergeTree doesn't merge. They are already separate rows in Tinybird today, and are a separate cleanup item on linuxfoundation/lfx-cdp-insights-ops#852.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because the published review omits a moderate-severity concern about platform-filtered status reporting.

No open issues 🎉

_{Review effort: Balanced}

[themarolt]

Re: Copilot review · summary findings

• Platform-filtered status reporting
  Fixed in bdc48cf. status accepted --platforms but ignored it: it always reported every platform of the tag, broken down per platform. The option is removed from status, so passing it now fails as an unknown option instead of suggesting a filter that didn't happen. The usage text says status covers every platform.

[Copilot]

🔵 Needs a closer look

Deleting roughly 100M rows with live writers requires DBA and data-platform validation of query plans, locking, CDC convergence, and recovery beyond the reported synthetic local tests.

No open issues 🎉

_{Review effort: Balanced}

[themarolt]

Re: Copilot review · summary findings

• A delete of this size with live writers needs DBA and data-platform validation beyond synthetic local tests
  Agreed. No code change; this is the rollout plan, tracked on linuxfoundation/lfx-cdp-insights-ops#852.
  • Production checks already done, through read-only sessions only:
    • plain EXPLAIN of the lock, clean and verify statements with real keys;
    • catalog, size and replication-slot checks;
    • collect --dry-run over sample days, to size the work (about 120–180M rows, collect about 24 h, clean 45–60 h of active time, WAL 80–120 GB).
  • Before production: a staging run of collect, clean, rescan and status. Then the first production runs are kept short with --max-rows, with data-sink-worker stopped, while the operator watches lock waits, Sequin slot lag, Kafka lag and Tinybird lag by hand.
  • Recovery: every deleted row is in the dated backup table with its kept activityId, so any batch can be restored. A manual VACUUM and the parentId repoint from the backup run after the final collect --rescan sweep.

[Copilot]

🔵 Needs a closer look

Date validation accepts nonexistent calendar dates and silently shifts collection boundaries instead of reporting a usage error for invalid --from or --to values.

No open issues 🎉

_{Review effort: Balanced}

[themarolt]

Re: Copilot review · summary findings

• Date validation accepts nonexistent calendar dates
  Fixed in the latest commit on this branch. JavaScript's Date rolled 2025-02-30 over to 2025-03-02, so a typo moved the collect range without any error. --from and --to now have to parse back to the same YYYY-MM-DD, or the run stops with a usage error (exit 64). Checked: 2025-02-30 is rejected, while 2024-02-29 and 0026-03-12 are accepted.

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate unexpectedly large dry-run workloads and shutdown behavior that can leave database sessions and advisory locks held while awaiting prompt input.

No open issues 🎉

_{Review effort: Balanced}

[themarolt]

Re: Copilot review · summary findings

• Sessions and advisory locks held while waiting for prompt input
  Fixed in 0265d35. The run lock is taken after the confirmation prompt, so a run waiting for an answer holds no lock. SIGTERM and SIGHUP now close the prompt, as Ctrl-C already did. Local tests: status showed no active run while the prompt was waiting, and a SIGTERM at the prompt exited in 0.2 s with "Aborted by the operator". A confirmed run still takes the lock, and a second run waits for it.
• Unexpectedly large dry-run workloads
  clean --dry-run is bounded by --sample-batches per platform, and every batch tried counts. collect --dry-run scans the same range as a real collect, because it reports exactly what a collect would find, which is hours on production. The help text now says so and points to --from/--to for a sample.

[Copilot]

🔵 Needs a closer look

Deleting ~100M live rows requires DBA/data-pipeline validation of concurrent-write locking and Sequin-to-Tinybird delete ordering that source review and synthetic tests cannot establish.

No open issues 🎉

_{Review effort: Balanced}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 0265d35. Configure here.}

[themarolt]

Re: Copilot review · summary findings

• Concurrent-write locking and Sequin-to-Tinybird delete ordering need DBA and data-pipeline validation
  No code change; both points are covered in earlier replies:
  • Locking is covered in the NOWAIT reply. App writes wait at most for one batch. FK waits are capped by lock_timeout and retried, and so are deadlocks.
  • Delete ordering in Tinybird is covered in the DELETE before-image reply. A deleted copy can't win over the kept row whatever order the events arrive in, because the result is decided by updatedAt (ReplacingMergeTree version, and ORDER BY updatedAt DESC in the merger), not by arrival order.
  • Validation on real infrastructure follows the rollout plan: a staging run, then short --max-rows production runs with data-sink-worker stopped while Sequin, Kafka and Tinybird lag are watched, with every deleted row kept in the backup table.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because an unpublished finding identifies orderly interruptions being reported as invalid CLI usage (exit 64) instead of the documented exit 2.

No open issues 🎉

_{Review effort: Balanced}