Make label_events writes idempotent on backfill (#25) - #26
Conversation
anderdc
left a comment
There was a problem hiding this comment.
Please remove packages/db/11_label_events_dedup.sql and packages/db/12_label_events_constraints.sql. Adjust the existing table definition in packages/db/07_label_events.sql instead — append the unique index next to the existing index:
CREATE UNIQUE INDEX IF NOT EXISTS uq_label_events_natural_key
ON label_events (repo_full_name, target_number, target_type,
label_name, action, timestamp)
NULLS NOT DISTINCT;
Any one-time migration SQL (e.g. the dedupe DELETE) should go in the PR description, not a dedicated .sql file.
TypeScript changes are fine as-is.
I'll fix it asap. |
|
Hello, @anderdc. |
…abel-events # Conflicts: # packages/das/src/webhook/github-fetcher.service.ts # packages/das/src/webhook/handlers/label.handler.ts
anderdc
left a comment
There was a problem hiding this comment.
Resolves all prior review items verbatim; runbook in body matches the prescribed shape.
|
Thanks, @anderdc . |
Summary
Make
label_eventswrites idempotent so backfill re-runs (and BullMQ retries) no longer duplicate rows.The webhook + backfill paths called
labelEventRepo.save()without a natural-key conflict path. BecauseLabelEvent.idis@PrimaryGeneratedColumn()andlabel_eventshad no UNIQUE constraint, every backfill INSERTed a fresh row for every label event already in the table. Thepr_labels_by_actor/issue_labels_by_actorviews collapse duplicates viaDISTINCT ON, so API output stayed correct while the table grew unbounded — eventually slowing the miners API as view scans degrade.Changes:
packages/db/07_label_events.sql—CREATE UNIQUE INDEX IF NOT EXISTS uq_label_events_natural_keyon(repo_full_name, target_number, target_type, label_name, action, timestamp) NULLS NOT DISTINCTappended next to the existing index.NULLS NOT DISTINCTdefends the rare case wheretarget_numberis NULL.packages/das/src/webhook/github-fetcher.service.ts—saveLabelTimelineEvents: per-noderepo.save()loop replaced with a single batchedinsert().values(rows).orIgnore().execute(). One round-trip per timeline instead of N, andON CONFLICT DO NOTHINGmakes re-runs no-ops.packages/das/src/webhook/handlers/label.handler.ts— samesave → insert().orIgnore()swap on the webhook write path. Defense-in-depth alongside the existingwebhook_deliveriesdelivery-id dedup.Deploy order for existing production databases:
orIgnore()is a behavioral no-op until the constraint exists).CONCURRENTLYon a running database to avoid locking the table):Applying the index (step 3) before deduping (step 2) will fail with a unique-violation error if duplicate rows exist — that is the intended safety check. Fresh installs skip steps 2–3 entirely; the index is part of the initial schema in
07_label_events.sql.Related Issues
Fixes #25
Type of Change
Testing
No test framework exists in this repo; verified manually.
Reproduction (matches issue #25 steps):
Build / lint:
npm run build— passes (NestJS compiler, full TS typecheck)npm run lint— cleannpm run format:check— cleanOut of scope (separate follow-ups):
new Date().toISOString()while backfill uses GraphQLLabeledEvent.createdAt. The same logical labeling action can be represented by two rows with slightly different timestamps, which won't collide on the new UNIQUE. Bounded leak (≈1 extra row per labeling action that occurs while the app is running, per backfill that follows). Proper fix is to drop the webhook'slabel_eventswrite entirely and enqueue a refresh job — architectural change deserving its own PR.DISTINCT ON (repo, target, label_name) ORDER BY timestamp DESCin the labels views still seq-scans after dedupe. A purpose-built index would help, but is a separate perf PR.Checklist