fix(core): preserve default retry match when overriding retry options - #18295
fix(core): preserve default retry match when overriding retry options#18295spokodev wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Sequelize constructor now merges persisted retry options with the default retry policy. Tests verify default matching, partial backoff overrides, explicit ChangesRetry option merging
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
packages/core/test/unit/configuration.test.tsParsing error: ESLint was configured to run on
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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
packages/core/src/sequelize-typescript.tspackages/core/test/unit/configuration.test.ts
20a88dd to
7f6be4d
Compare
Pull Request Checklist
Description of Changes
Closes #18259.
Sequelize's default retry policy is deliberately conservative:
Constructor options are shallow-merged, so supplying any
retryblock replaces the whole object. A config that only tunes backoff therefore loses the defaultmatch:In
retry-as-promised, an empty/absentmatchmeans retry on every error(
options.match.length === 0 || options.match.some(...)). So tuning backoff silently flips thepolicy from "retry one transient error" to "retry everything", including permanent ones: a losing
findOrCreatethat throwsUniqueConstraintErroris then retriedmaxtimes with full exponentialbackoff (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
retryonto the default instead of replacing it, so a partial override keepsthe default
matchwhile still letting callers override any field, includingmatch: []to opt intoretry-all explicitly. The merge is placed after the
...persistedSequelizeOptionsspread in theconstructor'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 noretryis given; the defaultmatch(andmax) survive a partial override; an explicitmatchstill overrides. Failing before the change, passing after; the full
coreunit suite stays green(2263 passing).
List of Breaking Changes
None intended, but one observable difference is worth calling out: code that passed a partial
retryobject without
matchwas (likely unintentionally) getting retry-on-every-error; it now retains thedefault
match: ['SQLITE_BUSY: database is locked']. Callers who actually want to retry all errorsshould set
match: []explicitly.Summary by CodeRabbit