fix(sync): TAM-6858: Fix patient_invoice_insurance_plan duplicate issue (HOTFIX 2.54) by chris-bes · Pull Request #10045 · beyondessential/tamanu

chris-bes · 2026-06-13T06:51:37Z

Changes

Add a brief description of the changes in this PR to help give the reviewer context.

Auto-Deploy

Deploy

Options

Tests

Run E2E tests
Run DAST scan

Review Hero

Run Review Hero
Auto-fix review suggestions Wait for Review Hero to finish, resolve any comments you disagree with or want to fix manually, then check this to auto-fix the rest.
Auto-fix CI failures Check this to auto-fix lint errors, test failures, and other CI issues.
Auto-merge upstream Check this to merge the base branch into this PR, with AI conflict resolution if needed.
Save suppressions Check this to capture 👎 reactions on Review Hero comments as suppression rules in .github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.

Remember to...

...write or update tests
...add UI screenshots and testing notes to the Linear issue
...add any manual upgrade steps to the Linear issue
...update the config reference, settings reference, or any relevant runbook(s)
...call out additions or changes to config files for the deployment team to take note of

review-hero · 2026-06-13T06:59:07Z

🦸 Review Hero Summary
9 agents reviewed this PR | 1 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 5 below threshold

Below consensus threshold (5 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
`packages/database/src/migrations/1781222400000-dedupePatientInvoiceInsurancePlans.ts:20`	Bugs & Correctness	`suggestion`	The cleanup DELETE from logs.changes (line 20) runs in the same transaction as the DELETE from patient_invoice_insurance_plans (line 11). CLAUDE.md states that Tamanu uses deferred constraint trigg...
`packages/database/src/migrations/1781222400001-makePatientInvoiceInsurancePlanIdDeterministic.ts:27`	Performance	`suggestion`	The UPDATE on logs.changes also filters by `table_name = 'patient_invoice_insurance_plans'` and runs on all servers. Facility servers lack the (table_name, record_id) index (central-only), so this ...
`packages/database/src/models/PatientInvoiceInsurancePlan.ts:22`	BES Requirements	`nitpick`	The set() comment says 'any sets of the convenience generated "id" field can be ignored', which describes what happens rather than why. The WHY is that PostgreSQL generated columns cannot be assign...
`packages/database/src/sync/saveIncomingChanges.ts:56`	Bugs & Correctness	`suggestion`	The new condition `(isCentralServer === false \|\| model.acceptsFacilityRestores)` means that on a facility server (isCentralServer === false), the restore path is taken for ALL models regardless of ...
`packages/facility-server/app/routes/apiv1/patient/utils/patientInsurancePlans.js:54`	Performance	`nitpick`	invoiceInsurancePlanIds is iterated twice with separate .filter() calls for toCreate and toRestore. A single loop building both arrays avoids the double pass: `const toCreate = [], toRestore = []; ...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

packages/database/src/migrations/1781222400001-makePatientInvoiceInsurancePlanIdDeterministic.ts:17: This migration mixes DDL (ALTER TABLE patient_invoice_insurance_plans) with DML (UPDATE logs.changes, UPDATE sync_lookup) in a single transaction, which violates the documented project rule in packages/database/CLAUDE.md. The project rule exists because PostgreSQL queues deferred constraint triggers at transaction commit — DML on any table creates pending trigger events that can prevent later ALTER TABLE statements. The author argues safety on the grounds that DDL is on patient_invoice_insurance_plans while DML is on logs.changes and sync_lookup. That argument holds only if logs.changes and sync_lookup have no deferred triggers that could queue events for patient_invoice_insurance_plans (e.g. via FK constraint triggers). Since the project explicitly uses deferred constraint triggers for audit logging, this should be split into three separate migrations (DDL-only, DML-only, DDL-only) as required by project convention, rather than relying on an implicit assumption about cross-table trigger isolation.

packages/database/src/migrations/1781222400000-dedupePatientInvoiceInsurancePlans.ts:29: The DELETE on sync_lookup filters by record_type alone: WHERE record_type = 'patient_invoice_insurance_plans' AND record_id NOT IN (...). The only index on sync_lookup is a unique constraint on (record_id, record_type) — record_id first — so this filter cannot use it efficiently and may do a full sequential scan of sync_lookup. On large central installations sync_lookup can contain millions of rows. Consider rewriting as NOT EXISTS with the primary key lookup: DELETE FROM sync_lookup sl WHERE sl.record_type = 'patient_invoice_insurance_plans' AND NOT EXISTS (SELECT 1 FROM patient_invoice_insurance_plans pip WHERE pip.id = sl.record_id) — the NOT EXISTS lets the planner use patient_invoice_insurance_plans PK for the inner lookup, though the outer scan of sync_lookup by record_type still needs a seq scan or a new index. Alternatively, adding a partial index on sync_lookup(record_type) before this migration would make it index-assisted.

chris-bes · 2026-06-15T01:14:02Z

+    // Restores only originate from the central server, except for models that opt in
+    // to facility-originated restores (see Model.acceptsFacilityRestores)
+    if (
+      (isCentralServer === false || model.acceptsFacilityRestores) &&


This PR is to migrate patient_invoice_insurance_plan.id to be deterministic, hence, instead of creating a new record when a previously deleted patient_invoice_insurance_plan is re added, the data is restored instead

However, the current sync model does not allow data restore originates from facility server (see original code). This was originally done by Biao which I'm not 100% sure the reason behind it.

My guess is I can understand it coming from the idea that restoring data is more of an admin task, but I don't see any problem with allowing it globally now since patient_invoice_insurance_plan is a legit case that we should allow restore coming from facility server.

However I'm also wary that any small change in sync can causes a lot of unknown side effects, so I'm playing it safe now and only allow it for patient_invoice_insurance_plan for now.

What do you reckon @edmofro ?

I think the reason is that our sync conflict handling is last-write-wins, but with a trump card that delete-always-wins.

Imagine that a record is on two facility servers

Facility A deletes it, then syncs that delete up to the central server

Before syncing down that delete, Facility B edits it, then syncs that edit up to the central server

This would create an unexpected restore.

One option is to change how we store whether an insurance plan is deleted, and use something like visibility_status, or removed_at instead, which avoids facility restores of soft deleted records.

Thoughts @chris-bes?

Yeah I did think of that approach, but did not have a strong preference since it would mean every time we want to allow restoring originates from facility, we would have to add an extra column there... But now after some more thinking, I don't think it's going to be a common case. Combined with the unexpected restore behaviour, I'm probably fine with your suggestion.

edmofro

I have comments to talk through

edmofro · 2026-06-19T04:48:01Z

+    // Restores only originate from the central server, except for models that opt in
+    // to facility-originated restores (see Model.acceptsFacilityRestores)
+    if (
+      (isCentralServer === false || model.acceptsFacilityRestores) &&


I think the reason is that our sync conflict handling is last-write-wins, but with a trump card that delete-always-wins.

Imagine that a record is on two facility servers

Facility A deletes it, then syncs that delete up to the central server

Before syncing down that delete, Facility B edits it, then syncs that edit up to the central server

This would create an unexpected restore.

One option is to change how we store whether an insurance plan is deleted, and use something like visibility_status, or removed_at instead, which avoids facility restores of soft deleted records.

Thoughts @chris-bes?

edmofro · 2026-06-19T04:50:47Z

+  // having facility-originated restores applied on central. Note there is no tick-based
+  // conflict resolution on deletion state, so a live copy pushed by a facility that
+  // hasn't yet pulled a deletion will resurrect the record — only opt in where
+  // re-add-wins is acceptable.


Oh yep this comment (presumably by claude) nailed the tradeoff

edmofro · 2026-06-19T04:53:36Z

+  // (patient_id, invoice_insurance_plan_id), so each pair must have exactly one row first.
+  // The released insert-new-on-re-add behaviour can leave a live row plus soft-deleted
+  // tombstones for the same pair. Keep the live row if one exists, otherwise the
+  // soft-deleted row with the highest id (deterministic across servers since ids are synced).


If they are in sync at the time of upgrade...?

edmofro · 2026-06-19T04:56:22Z

+    )
+    DELETE FROM logs.changes
+    WHERE record_id IN (SELECT id FROM deleted_duplicates)
+      AND table_name = 'patient_invoice_insurance_plans';


Ah ok so this has come up before, but we haven't solved it. We shouldn't delete records from logs.changes, it is an immutable and sacred audit log. Instead we need a way to represent that a record in logs.changes has had its source record deleted.

@julianam-w do you have any suggestions here?

Add a record in logs.changeswith a filled record_deleted_atand add a note in migration_context/ reason (I'm not sure which is for what)

edmofro · 2026-06-19T04:57:26Z

+// the same row (rather than inserting a new record), and two facilities adding the same
+// pair offline mint the same id and dedupe on sync instead of colliding.
+//
+// logs.changes and sync_lookup are remapped to the new ids (the preceding migration already


Again we are making changes to logs.changes. This is a lot less bad than removing records entirely, and we have done it before, but it's not entirely comfortable.

Thoughts @julianam-w @passcod?

Is that what the migration_context field is for? We create a new record in logs.changes with the change? This looks similar to updating the registration_status in patient_program_registrations

I think because we are migrating UUIDs to deterministic IDs which can be treated as deleting and recreating data, we can do both of these:

Insert new logs.changes with record_deleted_at that reflects the old patient_invoice_insurance_plans data (data that using UUIDs for IDs) have been deleted

Insert new logs.changes that reflects the latest patient_invoice_insurance_plans data that has got the IDs migrated to deterministic IDs (ie: patient_id;insurance_plan_id)

@julianam-w happy?

…dded

fixed patient_invoice_insurance_plan

41d47f8

chris-bes requested review from a team, dannash100, edmofro, passcod and rohan-bes as code owners June 13, 2026 06:51

review-hero Bot reviewed Jun 13, 2026

View reviewed changes

Comment thread ...ages/database/src/migrations/1781222400001-makePatientInvoiceInsurancePlanIdDeterministic.ts

Comment thread packages/database/src/migrations/1781222400000-dedupePatientInvoiceInsurancePlans.ts Outdated

chris-bes changed the title ~~fix(TAM-6858): Fix patient_invoice_insurance_plan duplicate issue~~ fix(sync): TAM-6858: Fix patient_invoice_insurance_plan duplicate issue Jun 14, 2026

chris-bes added 4 commits June 15, 2026 10:14

fixed migration down

e1c34bd

fix migration

def3416

fixed test

7f8bcf9

update test description

51735f0

chris-bes commented Jun 15, 2026

View reviewed changes

chris-bes changed the title ~~fix(sync): TAM-6858: Fix patient_invoice_insurance_plan duplicate issue~~ fix(sync): TAM-6858: Fix patient_invoice_insurance_plan duplicate issue (HOTFIX 2.54) Jun 15, 2026

edmofro reviewed Jun 19, 2026

View reviewed changes

chris-bes added 4 commits June 22, 2026 18:04

fix logs.changes DML

2765128

add visibility_status for patient_invoice_insurance_plans

6ccde9a

take care of logs.changes and sync_lookup when visibility_status is a…

f2cfd47

…dded

added dbt

60fff3e

chris-bes requested review from edmofro and julianam-w June 22, 2026 23:44

julianam-w approved these changes Jun 23, 2026

View reviewed changes

Uh oh!

Conversation

chris-bes commented Jun 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Auto-Deploy

Tests

Review Hero

Remember to...

Uh oh!

Uh oh!

Uh oh!

review-hero Bot commented Jun 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-bes Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmofro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianam-w Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chris-bes commented Jun 13, 2026 •

edited by github-actions Bot

Loading

chris-bes Jun 21, 2026 •

edited

Loading

julianam-w Jun 19, 2026 •

edited

Loading