Skip to content

fix: guard FK counter reconciliation with REPLACE conflict check#6346

Closed
reverb256 wants to merge 2 commits intotursodatabase:mainfrom
reverb256:fix/fk-deferred-counter-double-decrement
Closed

fix: guard FK counter reconciliation with REPLACE conflict check#6346
reverb256 wants to merge 2 commits intotursodatabase:mainfrom
reverb256:fix/fk-deferred-counter-double-decrement

Conversation

@reverb256
Copy link
Copy Markdown

Summary

Fixes #6218 — deferred FK violation counter double-decremented during UPDATE on parent table.

The issue: emit_fk_parent_new_key_reconcile was called unconditionally for all UPDATE operations on parent tables with deferred FK children. emit_fk_update_parent_actions already handles the correct increment (OLD key) and decrement (NEW key) cycle. The unconditional second call produces a duplicate decrement loop in the bytecode, causing the FK counter to drift to zero and allowing COMMIT to succeed when it should fail.

The fix: guard the call with matches!(effective_rowid_alias_conflict, ResolveType::Replace) so it only runs during REPLACE conflict resolution, which is what the function's own documentation says it should do.

Before fix

PRAGMA foreign_keys = ON;
CREATE TABLE p (id INTEGER PRIMARY KEY);
CREATE TABLE c (id INTEGER PRIMARY KEY, pid INT REFERENCES p(id) DEFERRABLE INITIALLY DEFERRED);
INSERT INTO p VALUES (24);
INSERT INTO c VALUES (1, 24);
INSERT INTO c VALUES (2, 24);
BEGIN;
SAVEPOINT sp0;
UPDATE p SET id = 4 WHERE id = 24;
RELEASE sp0;  -- Turso: succeeds (BUG, counter is 0)
COMMIT;       -- Turso: succeeds (BUG, orphans c rows)

After fix

RELEASE sp0;  -- succeeds (deferred, correct)
COMMIT;       -- Error: deferred foreign key constraint failed (correct, matches SQLite)

Tests

Two new sqltest regression tests in testing/sqltests/tests/savepoint.sqltest:

  • deferred-fk-update-parent-pk-in-savepoint-rejects-commit — verifies COMMIT fails
  • deferred-fk-update-parent-pk-rollback-restores-state — verifies ROLLBACK TO restores state

All 23 savepoint tests pass.

Description of AI Usage

Used AI assistance for code exploration and understanding the bytecode generation pipeline. The fix itself is a one-line guard, as identified in the issue by @LeMikaelF.

emit_fk_parent_new_key_reconcile was called unconditionally for all
UPDATE operations on parent tables with deferred FK children. This
produced a double-decrement loop in the bytecode: once from
emit_fk_update_parent_actions (correct) and once from
emit_fk_parent_new_key_reconcile (duplicate). The counter drifted to
zero, allowing RELEASE SAVEPOINT and COMMIT to succeed when they
should fail with FOREIGN KEY constraint violation.

Guard the call with matches!(effective_rowid_alias_conflict,
ResolveType::Replace) so it only runs during REPLACE conflict
resolution, matching the function's own documentation.

Adds two sqltest regression tests:
- deferred-fk-update-parent-pk-in-savepoint-rejects-commit
- deferred-fk-update-parent-pk-rollback-restores-state

Fixes tursodatabase#6218
Copy link
Copy Markdown

@turso-bot turso-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review @pereman2

@github-actions github-actions bot added the core label Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Fossier: PR Auto-Closed

This PR was automatically closed because @reverb256 did not meet the trust threshold for this repository.

Score Breakdown

Total Score: 36.2/100 | Confidence: 100% | Outcome: DENY

Signal Value Score Weight
account_age 1105 1.00 0.07
public_repos 14 0.70 0.04
contribution_history 14 0.07 0.04
follower_ratio 0.67 0.33 0.04
bot_signals False 0.50 0.03
open_prs_elsewhere 10 0.00 0.07
closed_prs_elsewhere 9 0.10 0.08
merged_prs_elsewhere 11 1.00 0.06
prior_interaction 0 0.00 0.24
activity_velocity 3 0.50 0.06
pr_content ... 1.00 0.06
commit_email no_email 0.50 0.03
pr_description ... 0.55 0.04
repo_stars 18181 0.30 0.03
org_membership 0 0.20 0.02
commit_verification ... 0.30 0.03
contributor_stars 0 0.00 0.03

Appeal

If you believe this is a mistake, please open an issue to request manual review. A maintainer can vouch for you by adding your username to the VOUCHED.td file.

You can also reach the maintainers at: https://discord.gg/mNUkCHs2R to appeal this decision

@PThorpe92
Copy link
Copy Markdown
Collaborator

/fossier approve

@PThorpe92 PThorpe92 reopened this Apr 10, 2026
Copy link
Copy Markdown

@turso-bot turso-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review @pereman2

The previous guard only checked effective_rowid_alias_conflict which
is set for INTEGER PRIMARY KEY tables. Tables with UNIQUE ON CONFLICT
REPLACE (not PRIMARY KEY) also trigger REPLACE deletes in Phase 1 but
were excluded from reconciliation, causing deferred FK counter drift.

Now checks both rowid alias conflict AND any unique index with REPLACE.
@reverb256
Copy link
Copy Markdown
Author

Fix for CI failures

The two failing tests ( and ) use UNIQUE ON CONFLICT REPLACE rather than INTEGER PRIMARY KEY, so the rowid alias conflict clause is Abort (not Replace).

My previous guard matches!(effective_rowid_alias_conflict, ResolveType::Replace) only caught the PRIMARY KEY REPLACE case. The UNIQUE index REPLACE case was missed.

Root cause: When Phase 1 REPLACE deletes a parent row, emit_fk_delete_parent_existence_check_single increments the deferred FK counter. The BEFORE-check's NEW pass then tries to decrement but FkIfZero guards it (counter was 0 at that point). So the counter ends up at +1. The reconciliation is supposed to bring it back to 0, but our guard prevented it from running.

Fix: Check both the rowid alias conflict AND whether any unique index in the update has ON CONFLICT REPLACE:

let has_replace_conflict = matches!(effective_rowid_alias_conflict, ResolveType::Replace)
    || indexes_to_update.iter().any(|idx| idx.on_conflict == Some(ResolveType::Replace));

This correctly covers both REPLACE sources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deferred FK violation counter double-decremented during UPDATE on parent table

2 participants