Skip to content

fix(core): preserve default retry match when overriding retry options - #18295

Open
spokodev wants to merge 2 commits into
sequelize:mainfrom
spokodev:fix/retry-option-merge
Open

fix(core): preserve default retry match when overriding retry options#18295
spokodev wants to merge 2 commits into
sequelize:mainfrom
spokodev:fix/retry-option-merge

Conversation

@spokodev

@spokodev spokodev commented Jul 30, 2026

Copy link
Copy Markdown

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description of Changes

Closes #18259.

Sequelize's default retry policy is deliberately conservative:

retry: { max: 5, match: ['SQLITE_BUSY: database is locked'] }

Constructor options are shallow-merged, so supplying any retry block replaces the whole object. A config that only tunes backoff therefore loses the default match:

new Sequelize({ dialect, retry: { max: 5, backoffBase: 1000, backoffExponent: 1.5 } }).options.retry;
// before: { max: 5, backoffBase: 1000, backoffExponent: 1.5 }   // no `match`

In retry-as-promised, an empty/absent match means retry on every error
(options.match.length === 0 || options.match.some(...)). So tuning backoff silently flips the
policy from "retry one transient error" to "retry everything", including permanent ones: a losing
findOrCreate that throws UniqueConstraintError is then retried max times with full exponential
backoff (the issue reports ~8s of wasted waiting and a pinned pooled connection) before failing,
where it should fail on the first attempt.

This merges the user's retry onto the default instead of replacing it, so a partial override keeps
the default match while still letting callers override any field, including match: [] to opt into
retry-all explicitly. The merge is placed after the ...persistedSequelizeOptions spread in the
constructor's option normalization (packages/core/src/sequelize-typescript.ts).

Tests (packages/core/test/unit/configuration.test.ts, no DB): the default policy is kept when no
retry is given; the default match (and max) survive a partial override; an explicit match
still overrides. Failing before the change, passing after; the full core unit suite stays green
(2263 passing).

List of Breaking Changes

None intended, but one observable difference is worth calling out: code that passed a partial retry
object without match was (likely unintentionally) getting retry-on-every-error; it now retains the
default match: ['SQLITE_BUSY: database is locked']. Callers who actually want to retry all errors
should set match: [] explicitly.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed retry option merging so custom retry settings no longer overwrite default retry behavior unexpectedly.
    • Default retry limits and match rules are preserved when only backoff-related fields are customized.
    • Explicit retry match values (including an empty match) now correctly take precedence over defaults.
  • Tests
    • Added unit coverage to verify retry behavior across default, partial override, and explicit match scenarios.

@spokodev
spokodev requested a review from a team as a code owner July 30, 2026 19:30
@spokodev
spokodev requested review from ephys and sdepold July 30, 2026 19:30
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 5a40f19f-569e-4d15-aa38-381ed15a4bb6

📥 Commits

Reviewing files that changed from the base of the PR and between 20a88dd and 7f6be4d.

📒 Files selected for processing (2)
  • packages/core/src/sequelize-typescript.ts
  • packages/core/test/unit/configuration.test.ts

📝 Walkthrough

Walkthrough

The Sequelize constructor now merges persisted retry options with the default retry policy. Tests verify default matching, partial backoff overrides, explicit match replacement, and preservation of an explicit empty match.

Changes

Retry option merging

Layer / File(s) Summary
Merge retry options in the constructor
packages/core/src/sequelize-typescript.ts
Persisted retry fields are merged into the default retry configuration, preserving the default match unless explicitly overridden.
Validate retry option behavior
packages/core/test/unit/configuration.test.ts
Tests cover default retry values, partial backoff overrides, explicit match replacement, and explicit empty-match preservation.

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

Suggested reviewers: ephys, sdepold

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the main change: preserving the default retry match when retry options are partially overridden.
Linked Issues check ✅ Passed The code and tests implement the required merge behavior, preserving default retry matching while allowing explicit match overrides, including match: [].
Out of Scope Changes check ✅ Passed The PR stays focused on retry normalization and its tests, with no unrelated changes introduced.
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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/core/test/unit/configuration.test.ts

Parsing error: ESLint was configured to run on <tsconfigRootDir>/packages/core/test/unit/configuration.test.ts using parserOptions.project: /packages/core/tsconfig.json
However, that TSConfig does not include this file. Either:


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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/test/unit/configuration.test.ts`:
- Around line 196-204: Add a separate regression test alongside the existing
explicit-match test using createSequelizeInstance with retry.match set to an
empty array, and assert localSequelize.options.retry equals { max: 5, match: []
}, confirming the empty match override is preserved.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 937e63d4-04e1-4e33-8c9d-bc6f478c4ad2

📥 Commits

Reviewing files that changed from the base of the PR and between 7e1deec and 20a88dd.

📒 Files selected for processing (2)
  • packages/core/src/sequelize-typescript.ts
  • packages/core/test/unit/configuration.test.ts

Comment thread packages/core/test/unit/configuration.test.ts
@spokodev
spokodev force-pushed the fix/retry-option-merge branch from 20a88dd to 7f6be4d Compare July 30, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Overriding retry without match silently drops the default match and retries all errors (incl. permanent UniqueConstraintError)

1 participant