fix: use a unique name for each nested savepoint by MahinAnowar · Pull Request #18236 · sequelize/sequelize

MahinAnowar · 2026-06-08T13:21:10Z

Issue

Fixes #18207 (Bug 1 — the one still present on main/v7).

When nesting transactions with TransactionNestMode.savepoint, each savepoint's name was derived from a per-parent counter:

this.#name = `${this.id}-sp-${this.parent.#savepoints.size}`;

Because the counter resets for every parent, the first savepoint at every nesting level gets the same name (e.g. <id>-sp-0). Postgres (and others) resolve ROLLBACK TO SAVEPOINT <name> to the most recently declared savepoint of that name, so a rollback at one level could silently roll back to a deeper savepoint — committing writes that should have been undone. A maintainer confirmed the issue and favoured generating a fresh id per savepoint.

Fix

Give each savepoint a fresh unique id from the dialect's generateTransactionId() (the same generator already used for root transaction ids), instead of a per-parent counter. The now-unused #savepoints map (its only purpose was the counter) is removed.

if (this.parent) {
  this.id = this.parent.id;
  this.#name = generateTransactionId();
} else {
  const id = generateTransactionId();
  this.id = id;
  this.#name = id;
}

this.id still tracks the root transaction (used for the connection uuid); only the savepoint #name changes.

Tests

Added a unit test in packages/core/test/unit/transaction.test.ts (no DB — the existing queryRaw stub harness): it opens two levels of nested savepoint-mode transactions and asserts the two emitted SAVEPOINT statements use distinct names. Verified it fails on the previous code (expected 1 to equal 2 — both savepoints shared <id>-sp-0) and passes with the fix. ESLint + Prettier clean.

Scoped to Bug 1 only; Bugs 2 & 3 in the issue are v6-only and the maintainer indicated v6 won't be patched.

Summary by CodeRabbit

Bug Fixes
- Fixed nested savepoint transactions that could reuse or collide identifiers, ensuring each nesting level generates distinct savepoint names.
Tests
- Added a regression test covering savepoint-based nested transactions and verifying unique savepoint creation statements across nesting levels (skipping when unsupported).

coderabbitai · 2026-06-08T13:21:25Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 94055da7-a042-414c-9d4e-2c6679b7dcf6

📥 Commits

Reviewing files that changed from the base of the PR and between 20975e0 and b5899fe.

📒 Files selected for processing (1)

packages/core/test/unit/transaction.test.ts

💤 Files with no reviewable changes (1)

packages/core/test/unit/transaction.test.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

This PR changes nested savepoint naming to use a fresh dialect-generated transaction ID for each savepoint. It also adds a regression test that verifies nested savepoints receive distinct names.

Changes

Nested savepoint name collision fix

Layer / File(s)	Summary
Savepoint unique ID generation `packages/core/src/transaction.ts`	Removes the per-parent `#savepoints` map and generates a fresh dialect transaction ID for each nested savepoint.
Regression test for savepoint uniqueness `packages/core/test/unit/transaction.test.ts`	Imports `TransactionNestMode` and verifies that nested savepoint creation produces two distinct savepoint names.

Estimated code review effort: 2 (Simple) | ~12 minutes

Merge Risk: ⚪ Minimal · up to b5899

This localized change gives nested savepoints distinct names and adds regression coverage for the collision; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: sdepold, ephys

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR fixes unique savepoint naming for [`#18207`] but does not address its parent-name, CLS cleanup, shared-connection, or PostgreSQL requirements.	Implement the remaining [`#18207`] requirements or link this PR only to the specific unique-savepoint-name objective.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: unique names for nested savepoints.
Out of Scope Changes check	✅ Passed	The code and regression test directly support the nested savepoint naming fix and remain within the stated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Nested savepoint names were derived from a per-parent counter (`${id}-sp-${parent.savepoints.size}`), so the first savepoint at every nesting level collided on the same name (e.g. `<id>-sp-0`). `ROLLBACK TO SAVEPOINT` resolves to the most recently declared savepoint of that name, so a rollback could target the wrong savepoint. Give each savepoint a fresh unique id instead.

MahinAnowar · 2026-07-13T10:54:09Z

@WikiRik would you mind taking a look at this when you have some time? All checks are green and it's a fairly small diff.

SippieCup

LGTM

MahinAnowar · 2026-08-15T09:27:11Z

Just a heads up that this was sitting behind main, which I think is why it never went anywhere after the approval — GitHub was reporting it as out of date rather than mergeable. I've updated the branch, CI is green again and SippieCup's approval carried over, so it should be ready to go whenever someone has a minute.

SippieCup · 2026-08-15T17:17:38Z

@MahinAnowar It's a fairly straight forward commit, but I would still like to have @WikiRik give his blessing on it. just in case i am missing something.

WikiRik

Some comments that can be cleaned up. I think we can keep the one in transaction.ts.

Interesting that we didn't use #savepoints more, I'll do a sanity check before I approve what I can find back about that

MahinAnowar · 2026-08-16T09:28:44Z

Took all three, they're gone. Kept the one in transaction.ts since that's the one explaining the actual bug.

MahinAnowar · 2026-09-01T11:48:33Z

@WikiRik when you get a moment — @SippieCup wanted your read on this one before it goes in. Your three suggestions are applied, so the only comment left in the test is the one in transaction.ts that explains the actual bug. Everything's green.

SippieCup · 2026-09-02T03:31:07Z

this can be merged, Still will defer to @WikiRik :)

MahinAnowar requested a review from a team as a code owner June 8, 2026 13:21

MahinAnowar requested review from ephys and sdepold June 8, 2026 13:21

MahinAnowar force-pushed the fix/savepoint-name-collision branch from f470490 to 20975e0 Compare July 5, 2026 11:21

SippieCup approved these changes Jul 15, 2026

View reviewed changes

Merge branch 'main' into fix/savepoint-name-collision

7429533

WikiRik reviewed Aug 15, 2026

View reviewed changes

Comment thread packages/core/test/unit/transaction.test.ts Outdated

Comment thread packages/core/test/unit/transaction.test.ts Outdated

Comment thread packages/core/test/unit/transaction.test.ts Outdated

test: drop redundant comments in nested savepoint test

b5899fe

Merge branch 'main' into fix/savepoint-name-collision

5ec2fd1

WikiRik approved these changes Sep 3, 2026

View reviewed changes

WikiRik enabled auto-merge (squash) September 3, 2026 11:15

WikiRik merged commit 819dba6 into sequelize:main Sep 3, 2026
213 of 215 checks passed

Navigation Menu

Uh oh!

Uh oh!

fix: use a unique name for each nested savepoint - #18236

fix: use a unique name for each nested savepoint#18236
WikiRik merged 4 commits into
sequelize:mainfrom
MahinAnowar:fix/savepoint-name-collision

MahinAnowar commented Jun 8, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

MahinAnowar commented Jul 13, 2026

Uh oh!

SippieCup left a comment

Uh oh!

MahinAnowar commented Aug 15, 2026

Uh oh!

SippieCup commented Aug 15, 2026

Uh oh!

WikiRik left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MahinAnowar commented Aug 16, 2026

Uh oh!

MahinAnowar commented Sep 1, 2026

Uh oh!

SippieCup commented Sep 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

MahinAnowar commented Jun 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Fix

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

MahinAnowar commented Jul 13, 2026

Uh oh!

SippieCup left a comment

Choose a reason for hiding this comment

Uh oh!

MahinAnowar commented Aug 15, 2026

Uh oh!

SippieCup commented Aug 15, 2026

Uh oh!

WikiRik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MahinAnowar commented Aug 16, 2026

Uh oh!

MahinAnowar commented Sep 1, 2026

Uh oh!

SippieCup commented Sep 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MahinAnowar commented Jun 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading