fix(sync): order changelog FK targets (users) first during pull

[dannash100]

Changes

Facility sync could dead-loop with:

insert or update on table "changes" violates foreign key constraint "changes_updated_by_user_id_fkey"
DETAIL: Key (updated_by_user_id)=(...) is not present in table "users".

Why it happens. Changelog rows are replayed during a pull attached to the records they describe and inserted by insertChangelogRecords during that model's batch. Each row's updated_by_user_id references users(id). But sortInDependencyOrder emits ready models alphabetically, so users (u) is saved after models like invoice_price_lists (i) — whose changelog rows then reference users that aren't in the table yet. That violates changes_updated_by_user_id_fkey and aborts the whole pull transaction, which recurs every sync because the cursor only advances on success.

The ChangeLog → User dependency is invisible to the topological sort because ChangeLog is DO_NOT_SYNC, so it never appears as a BelongsTo edge on a pulled model.

Fix. Derive the dependency from ChangeLog itself: read its BelongsTo targets (currently just User, via updated_by_user_id) and treat every other model as depending on those targets. The existing topological sort then orders them first, and it stays correct automatically if logs.changes ever gains another FK. A guard throws a clear error instead of looping forever if a dependency cycle is ever introduced.

Notes

Known limit: if a facility has more users than persistedCacheBatchSize, a changelog row in an earlier user batch could reference a user in a later user batch (user-edited-by-user across the batch boundary). This fixes the reported case (changelog on other tables referencing users) completely; that cross-user-batch edge is separate.

Review Hero

• Run Review Hero

🤖 Generated with Claude Code

[cursor]

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

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

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 90f8aff. Configure here.}

[review-hero]

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

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

Location	Agent	Severity	Comment
packages/database/src/migrations/1780500000000-makeChangelogUserFKDeferrable.ts:9	Bugs & Correctness	suggestion	The up function issues two separate DDL statements without explicit transaction wrapping. If DROP CONSTRAINT succeeds but ADD CONSTRAINT fails (e.g., an unrelated server crash or a FK violati...
packages/database/src/migrations/1780500000000-makeChangelogUserFKDeferrable.ts:21	Bugs & Correctness	suggestion	The down function recreates the FK without DEFERRABLE, but if any row in logs.changes has an updated_by_user_id that references a user not yet in users (exactly the scenario this PR is desi...

Local fix prompt (copy to your coding agent)

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

---

packages/database/src/sync/withDeferredSyncSafeguards.ts:46: The query returns unqualified constraint names (c.conname), but SET CONSTRAINTS resolves names using the current search_path, which defaults to public only. The new logs.changes_updated_by_user_id_fkey constraint lives in the logs schema, which is not in the default search path. When SET CONSTRAINTS "changes_updated_by_user_id_fkey" DEFERRED is executed, PostgreSQL will throw ERROR: constraint "changes_updated_by_user_id_fkey" does not exist, causing all sync operations to fail after this migration is applied.

Fix: qualify constraint names with their schema in the SELECT query:
sql SELECT quote_ident(n.nspname) || '.' || quote_ident(c.conname) AS conname
Then the constraint list becomes "public"."some_fk" , "logs"."changes_updated_by_user_id_fkey", which PostgreSQL resolves correctly regardless of search_path.

[review-hero]

🦸 Review Hero Summary
3 agents reviewed this PR | 0 critical | 0 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 2 below threshold

No issues found. Looks good!

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

Location	Agent	Severity	Comment
packages/database/src/utils/sortInDependencyOrder.ts:8	Bugs & Correctness	suggestion	The fix relies on a runtime assumption ('User has no BelongsTo dependencies of its own') but never enforces it. If the User model later gains a BelongsTo association (e.g. to a Role or Facility), i...
packages/database/src/utils/sortInDependencyOrder.ts:12	Bugs & Correctness	nitpick	The if (a !== b) guard is always true: Object.entries produces unique keys, so two distinct entries can never have the same key. The guard adds a misleading false impression that a === b is a...

[review-hero]

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

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

Location	Agent	Severity	Comment
packages/database/src/utils/sortInDependencyOrder.ts:17	Bugs & Correctness	suggestion	Object.values(models)[0]?.sequelize?.models?.ChangeLog reaches sequelize through the first model in the map. If any individual model doesn't have sequelize set (e.g., a partially-initialised ...
packages/database/src/utils/sortInDependencyOrder.ts:18	Bugs & Correctness	nitpick	changeLog.associations is passed directly to belongsToTargets without a null guard. Sequelize always initialises Model.associations to {}, but if something in the registry provides a Change...
packages/database/src/utils/sortInDependencyOrder.ts:23	Bugs & Correctness	suggestion	The injected changelogTargets are appended unconditionally to every non-target model's dependsOn, even when that model is not synced at the same time as changelog rows (e.g., it's only ever ins...

Local fix prompt (copy to your coding agent)

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

---

packages/database/src/utils/sortInDependencyOrder.ts:26: The filter(target => target !== name) only removes self-references, but doesn't prevent circular dependencies when ChangeLog has multiple BelongsTo targets. If a second BelongsTo is ever added to ChangeLog (e.g., a createdByUser association targeting Organization), then changelogTargets = ['User', 'Organization']. When processing User, it gains ['Organization'] as an implicit dep; when processing Organization, it gains ['User']. These create a mutual dependency and the new cycle-detection on line 34 throws, breaking sync entirely. The fix is to only inject changelog targets as implicit deps for models that are NOT themselves changelog targets: const isChangelogTarget = changelogTargets.includes(name); const dependsOn = [...belongsToTargets(model.associations), ...(isChangelogTarget ? [] : changelogTargets)];

[review-hero]

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

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

Location	Agent	Severity	Comment
packages/database/src/utils/sortInDependencyOrder.ts:15	Bugs & Correctness	suggestion	(Object.values(models)[0] as any)?.sequelize?.models?.ChangeLog reaches the shared sequelize instance through an arbitrary model's class property. If models is empty (valid input: empty sync ba...
packages/database/src/utils/sortInDependencyOrder.ts:16	Bugs & Correctness	suggestion	changeLog.associations is accessed without a null guard. If ChangeLog is present in sequelize.models but has not yet had its associations initialised (e.g. in certain test setups or early boo...

Local fix prompt (copy to your coding agent)

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

---

packages/database/src/utils/sortInDependencyOrder.ts:21: If changelogTargets ever contains more than one entry, those models deadlock each other. Every model in changelogTargets has all other changelog-target models injected as dependencies (line 23 filters only target !== name), so none of them can be emitted first — remaining never decreases and the cycle-detection error is thrown. For example, if ChangeLog had BelongsTo associations to both User and AuditLog, User would depend on AuditLog and AuditLog would depend on User, making both unresolvable. The fix is to sort the changelog-target group among themselves by their own real BelongsTo edges, rather than making them all mutually depend on each other. One practical approach: inject the changelogTargets as a dependency only for models that are NOT themselves changelog targets (i.e., change changelogTargets.filter(target => target !== name) to changelogTargets.filter(target => target !== name && !changelogTargets.includes(name)), which lets changelog-target models be resolved normally by their own associations before becoming available to others).