Skip to content

[v6] Nested savepoints: name collision (also in v7) + parent .name mutation + CLS not cleared on savepoint rollback #18207

@kessiler

Description

@kessiler

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

Three compounding bugs in the savepoint path break nested-transaction workflows that wrap per-unit work inside an outer (typically top-level) transaction. All three are confirmed on sequelize@6.37.8 + Postgres + cls-hooked. Bug 1 is still present on main / v7; Bug 2 is already fixed there (via the explicit savepointName option); Bug 3 does not apply to v7 because v7 dropped CLS.

I believe these are the root causes behind the symptom reported in #12762.

Bug 1 — Savepoint name collision across nesting levels

v6.37.8/src/transaction.js:30-36:

this.parent = this.options.transaction;
if (this.parent) {
  this.id = this.parent.id;                                     // <-
  this.parent.savepoints.push(this);
  this.name = `${this.id}-sp-${this.parent.savepoints.length}`; // <-
}

Every descendant of a top-level transaction inherits the same id, and the savepoints counter is per parent, so the first child at every depth gets …-sp-1:

global  : id=abc, savepoints=[wrapper]
  wrapper: id=abc, name=abc-sp-1, savepoints=[child]
    child: id=abc, name=abc-sp-1   ← collision

Postgres' ROLLBACK TO SAVEPOINT abc-sp-1 resolves to the most recently declared savepoint with that name, so a later rollback of the wrapper lands on the child instead (or fails outright, depending on order). Writes done at the wrapper level between the child's lifetime and the wrapper's rollback are silently committed when the top-level transaction eventually commits.

v7 main still carries the same pattern in packages/core/src/transaction.ts:

if (this.parent) {
  this.id = this.parent.id;
  this.#name = `${this.id}-sp-${this.parent.#savepoints.size}`;
  this.parent.#savepoints.set(this.#name, this);
}

Fix idea: either use a top-level-rooted counter (walk up to the root and use its total savepoint count), or include a depth/path segment in the name, or just generate a fresh id per savepoint instead of inheriting from the parent.

Bug 2 — Parent .name mutated on every child create/rollback (v6 only)

v6.37.8/src/dialects/abstract/query-interface.js:519-527 and :552-566:

async startTransaction(transaction, options) {
  ...
  options = { ...options, transaction: transaction.parent || transaction };
  options.transaction.name = transaction.parent ? transaction.name : void 0; // <-
  ...
}

options.transaction is an alias for the parent object, not a copy. The next line mutates parent.name to the child's name. Same pattern in rollbackTransaction.

Effect: creating a savepoint inside a wrapper permanently stomps the wrapper's .name. Any caller that cached the wrapper's original name (e.g. to later issue a manual ROLLBACK TO SAVEPOINT <cachedName>) ends up targeting the child instead.

This is already fixed in v7 — the name is threaded through an explicit option (savepointName / transactionName) rather than mutated onto the parent. Asking for a v6 backport.

Bug 3 — CLS not cleared on savepoint rollback (v6 only)

v6.37.8/src/transaction.js:117-124:

cleanup() {
  if (this.parent || this.connection.uuid === void 0) {
    return;                       // <- savepoints early-return
  }
  this._clearCls();
  this.sequelize.connectionManager.releaseConnection(this.connection);
  this.connection.uuid = void 0;
}

The early-return is correct for the connection-release half (savepoints share the parent's connection) but also skips _clearCls(). After savepoint.rollback(), CLS still points at the finished savepoint, so any subsequent CLS-routed query throws:

Error: rollback has been called on this transaction <id>-sp-1, you can no longer use it.

Suggested fix: reset CLS even for savepoints, only skip the connection release:

cleanup() {
  if (this.parent) {
    const clsNs = this.sequelize.constructor._cls;
    if (clsNs && clsNs.get('transaction') === this) {
      clsNs.set('transaction', this.parent);
    }
    return;
  }
  if (this.connection.uuid === void 0) return;
  this._clearCls();
  this.sequelize.connectionManager.releaseConnection(this.connection);
  this.connection.uuid = void 0;
}

This is N/A in v7 (no CLS), so only asking for the v6 fix.

Reproducible Example

Public gist: https://gist.github.com/kessiler/658a6dc64a4c2754c2b2a907ef17fec3

bug-repro.js — self-contained, ~120 lines, needs sequelize@6.37.8 + pg + cls-hooked + a reachable postgres
// Demonstrates three compounding bugs in sequelize@6.37.8 nested savepoints:
//   1. Savepoint name collision across nesting levels (transaction.js:30-36)
//   2. Parent .name mutation on child create/rollback (query-interface.js:523-524, 560-561)
//   3. CLS not cleared on savepoint rollback (transaction.js:117-124)

const cls = require('cls-hooked');
const { Sequelize } = require('sequelize');

const namespace = cls.createNamespace('sq-repro');
Sequelize.useCLS(namespace);

const sequelize = new Sequelize(
  process.env.DATABASE_URL || 'postgres://postgres:postgres@localhost:5432/postgres',
  { logging: false },
);

async function main() {
  // cls-hooked requires an active context before set() works
  namespace.enter(namespace.createContext());

  // ---------------------------------------------------------------
  // BUG 1 + BUG 2a: pure flow, no workarounds
  // ---------------------------------------------------------------
  console.log('===== PART 1: pure flow (no workarounds) =====\n');

  const globalA = await sequelize.transaction({ autocommit: false });
  const globalANameAtCreation = globalA.name;
  console.log('globalA.id   =', globalA.id);
  console.log('globalA.name at creation =', globalANameAtCreation);

  const wrapperA = await sequelize.transaction({
    autocommit: false,
    transaction: globalA,
  });
  const wrapperANameAtCreation = wrapperA.name;
  console.log('wrapperA.id   =', wrapperA.id);
  console.log('wrapperA.name at creation =', wrapperANameAtCreation);
  console.log();

  console.log('=== BUG 2a: creating wrapperA stomped globalA.name ===');
  console.log('globalA.name was :', globalANameAtCreation);
  console.log('globalA.name now :', globalA.name);
  console.log('mutated ?         ', globalA.name !== globalANameAtCreation);
  console.log();

  const childA = await sequelize.transaction({
    autocommit: false,
    transaction: wrapperA,
  });
  console.log('childA.id   =', childA.id);
  console.log('childA.name =', childA.name);
  console.log();

  console.log('=== BUG 1: savepoint name collision ===');
  console.log('wrapperA.name (at creation) :', wrapperANameAtCreation);
  console.log('childA.name                 :', childA.name);
  console.log('collision ?                   ', wrapperANameAtCreation === childA.name);
  console.log();

  // Clean up part 1
  try { await childA.rollback(); } catch (e) { /* noop */ }
  try { await wrapperA.rollback(); } catch (e) { /* noop */ }
  try { await globalA.rollback(); } catch (e) { /* noop */ }

  // ---------------------------------------------------------------
  // BUG 2b + BUG 3: workaround Bug 1 so the others are unambiguous
  // ---------------------------------------------------------------
  console.log('===== PART 2: Bug 2b + Bug 3 (with Bug 1 worked around) =====\n');

  const globalB = await sequelize.transaction({ autocommit: false });
  const wrapperB = await sequelize.transaction({
    autocommit: false,
    transaction: globalB,
  });

  // Force a unique wrapperB.id so child names don't collide with wrapperB.name
  wrapperB.id = `wrap-${Date.now()}`;
  const wrapperBNameAtCreation = wrapperB.name;

  const childB = await sequelize.transaction({
    autocommit: false,
    transaction: wrapperB,
  });

  console.log('=== BUG 2b: creating childB stomped wrapperB.name ===');
  console.log('wrapperB.name was :', wrapperBNameAtCreation);
  console.log('wrapperB.name now :', wrapperB.name);
  console.log('mutated ?          ', wrapperB.name !== wrapperBNameAtCreation);
  console.log();

  // Bug 3: put childB into CLS (what sequelize.transaction(callback) does
  // automatically, and what test frameworks like ours do explicitly), then
  // roll it back and inspect CLS.
  namespace.set('transaction', childB);
  console.log('cls tx before childB.rollback(): is childB ?',
    namespace.get('transaction') === childB);

  await childB.rollback();

  const clsTx = namespace.get('transaction');
  console.log('=== BUG 3: CLS not cleared on savepoint rollback ===');
  console.log('cls tx after childB.rollback():');
  console.log('  is childB           ?', clsTx === childB);
  console.log('  is wrapperB         ?', clsTx === wrapperB);
  console.log('  is null             ?', clsTx === null);
  console.log('  finished flag on it :', clsTx && clsTx.finished);
  console.log();

  try {
    await sequelize.query('SELECT 1', { transaction: namespace.get('transaction') });
    console.log('subsequent query via CLS tx: OK');
  } catch (e) {
    console.log('subsequent query via CLS tx threw:');
    console.log('  ', e.message);
  }
  console.log();

  try { await wrapperB.rollback(); } catch (e) { /* noop */ }
  try { await globalB.rollback(); } catch (e) { /* noop */ }
  await sequelize.close();
}

main().catch((e) => {
  console.error('unexpected error:', e);
  process.exit(1);
});

Run it:

mkdir sq-bug-repro && cd sq-bug-repro
npm init -y
npm i sequelize@6.37.8 pg cls-hooked
DATABASE_URL=postgres://user:pass@localhost:5432/postgres node bug-repro.js

What do you expect to happen?

  1. Each savepoint gets a unique name (at minimum, distinguishable from its parent).
  2. parent.name stays stable across child savepoint creation and rollback.
  3. cls.get('transaction') after savepoint.rollback() points at the parent transaction (or null), never at the rolled-back savepoint.

What is actually happening?

Actual output of the repro above against sequelize@6.37.8 + pg + Postgres 14:

===== PART 1: pure flow (no workarounds) =====

globalA.id   = 6459c59a-b454-427f-a26b-d4c0bba74989
globalA.name at creation = undefined
wrapperA.id   = 6459c59a-b454-427f-a26b-d4c0bba74989
wrapperA.name at creation = 6459c59a-b454-427f-a26b-d4c0bba74989-sp-1

=== BUG 2a: creating wrapperA stomped globalA.name ===
globalA.name was : undefined
globalA.name now : 6459c59a-b454-427f-a26b-d4c0bba74989-sp-1
mutated ?          true

childA.id   = 6459c59a-b454-427f-a26b-d4c0bba74989
childA.name = 6459c59a-b454-427f-a26b-d4c0bba74989-sp-1

=== BUG 1: savepoint name collision ===
wrapperA.name (at creation) : 6459c59a-b454-427f-a26b-d4c0bba74989-sp-1
childA.name                 : 6459c59a-b454-427f-a26b-d4c0bba74989-sp-1
collision ?                    true

===== PART 2: Bug 2b + Bug 3 (with Bug 1 worked around) =====

=== BUG 2b: creating childB stomped wrapperB.name ===
wrapperB.name was : 07b3b893-1250-4397-8f69-ff4615cf18e3-sp-1
wrapperB.name now : wrap-1775745107157-sp-1
mutated ?           true

cls tx before childB.rollback(): is childB ? true
=== BUG 3: CLS not cleared on savepoint rollback ===
cls tx after childB.rollback():
  is childB           ? true
  is wrapperB         ? false
  is null             ? false
  finished flag on it : rollback

subsequent query via CLS tx threw:
   rollback has been called on this transaction(wrap-1775745107157), you can no longer use it.

Environment

  • Sequelize version: 6.37.8
  • Node.js version: v24.14.0
  • If TypeScript related: n/a (plain JS repro)
  • Database & Version: PostgreSQL 14 (also reproduced on 16)
  • Connector library & Version: pg@8.x
  • CLS library: cls-hooked@4.2.2

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time but I will need guidance.

Happy to open PRs for each of the three once I have maintainer steer on:

  • Bug 1: preferred naming strategy (global counter vs. fresh id per savepoint vs. depth-aware template).
  • Bug 2: confirmation that backporting the v7 savepointName shape to v6's query-interface.js is the desired fix.
  • Bug 3: confirmation of the cleanup split above, or an alternative hook point maintainers prefer.

Metadata

Metadata

Assignees

No one assigned

    Labels

    pending-approvalBug reports that have not been verified yet, or feature requests that have not been accepted yet

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions