Skip to content

[SHOT-187] fix: Make database migration execution timeout configurable - #8143

Open
mimartin12 wants to merge 2 commits into
mainfrom
shot/shot-187/configurable-migration-timeout
Open

[SHOT-187] fix: Make database migration execution timeout configurable#8143
mimartin12 wants to merge 2 commits into
mainfrom
shot/shot-187/configurable-migration-timeout

Conversation

@mimartin12

@mimartin12 mimartin12 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

SHOT-187 — follows PM-37891.

📔 Objective

DbMigrator hardcoded a 5 minute DbUp execution timeout. A migration that rebuilds an index on a large Event table can exceed it, and because the whole upgrade runs inside a single transaction, one slow script rolls the entire upgrade back. Recovery then requires running SQL by hand against the database.

This makes that timeout configurable so operators can complete a long migration without manual DB surgery.

How to set it

Path Mechanism
Admin container globalSettings__sqlServer__migrationExecutionTimeoutSeconds
bitwarden.sh / Setup same key, read from bwdata/env/global.override.env
MsSqlMigratorUtility -t / --timeout

EnvironmentFileBuilder now seeds the key with an empty value so it ships discoverable and with the casing both readers agree on (Helpers.GetValueFromEnvFile matches case-sensitively; the app binds case-insensitively).

Behaviour

  • Unset preserves today's behaviour exactly: 5 minutes, or 60 minutes on the no-transaction path. Both were magic numbers and are now named constants.
  • A positive value is used verbatim, in seconds.
  • 0 means no limit, matching sqlcmd -t 0, SqlCommand.CommandTimeout, and EF Core.
  • A negative value falls back to the default, since a negative CommandTimeout throws on assignment.

Verification

Runtime-verified against SQL Server 2022 using a probe migration containing WAITFOR DELAY '00:05:30':

Run Result
no flag failed at 302s with SQL error -2 — pins the 5 minute default
--timeout 0 succeeded at 332s — genuinely unlimited, ran past the default
--timeout 3 vs a 10s script failed with SQL error -2
--timeout 60 vs a 10s script succeeded

New test/Migrator.Test project covers ResolveExecutionTimeout (5 cases). util/Migrator had no test project before; this follows test/Setup.Test, reusing its exact package versions, so nothing is new to the dependency graph. Also registered the project in bitwarden-server.slnx — every other test project is there, and without it the new test file sits outside the dotnet format CI gate.

Reviewers: this spans three CODEOWNERS areas. util/Migrator/** and util/MsSqlMigratorUtility/** are @bitwarden/team-platform-dev, util/Setup/** is joint @bitwarden/dept-shot @bitwarden/team-platform-dev, and src/Core/Settings/GlobalSettings.cs has its own owners.

Migrations that rebuild an index on a large Event table can exceed the
hardcoded five minute DbUp timeout. The whole upgrade is wrapped in a
single transaction, so one slow script rolls the upgrade back and
recovery needs manual SQL run against the customer database.

The timeout can now be set through
globalSettings__sqlServer__migrationExecutionTimeoutSeconds or the
MsSqlMigratorUtility timeout option. Leaving it unset keeps the existing
five minute default, so upgrades behave exactly as before. Setting it to
zero applies no limit, matching sqlcmd and SqlCommand semantics.
@mimartin12 mimartin12 added the ai-review Request a Claude code review label Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed at e84b8a4, covering DbMigrator's new executionTimeoutSeconds parameter and all three construction sites (SqlServerDbMigrator, MsSqlMigratorUtility, util/Setup), the GlobalSettings.SqlSettings addition, the global.override.env seed key, and the new test/Migrator.Test project. ResolveExecutionTimeout preserves today's behaviour on both the transactional (5 min) and no-transaction (60 min) paths when unset, maps 0 to a CommandTimeout of zero for no limit, and guards negatives — which DbUp's WithExecutionTimeout would otherwise reject with an ArgumentException. The -t short option does not collide with the existing -r/-f/-d options or CommandDotNet's reserved -h.

The single 🎨 from the previous round — an unparseable timeout silently discarded on the Setup path — is addressed: PrepareAndMigrateDatabase now reports both the applied and the ignored value, and DbMigrator logs a warning when a negative value falls back to the default. No new findings.

Code Review Details

No findings at or above the reporting threshold.

Verified, not flagged:

  • The empty seed value for globalSettings__sqlServer__migrationExecutionTimeoutSeconds binds to int? as null — the configuration binder short-circuits empty strings for nullable types — and follows the existing adminSettings__admins precedent. LoadExistingValues preserves an operator-set value across bitwarden.sh update.
  • Helpers.GetValueFromEnvFile matches keys case-sensitively, and the seeded key uses the same casing the Setup reader expects.
  • No IGlobalSettings change is needed — SqlServer is exposed there as the concrete GlobalSettings.SqlSettings.
  • test/Migrator.Test is picked up by test/Bitwarden.Tests.proj's **\*.*proj traversal glob, so dotnet test ./test and the dotnet format gate both cover it. It is also registered in bitwarden-server.slnx.

Dependency Changes

Package Change Ecosystem
Microsoft.NET.Test.Sdk 18.7.0 NuGet
xunit.v3 3.2.1 NuGet
xunit.runner.visualstudio 3.1.5 NuGet

These are the three direct references of the new test/Migrator.Test project, pinned to the exact versions already used by test/Setup.Test. No net-new package enters the dependency graph, so the AppSec dependency approval process does not apply. packages.lock.json is committed as RestorePackagesWithLockFile requires.

Comment thread util/Setup/Program.cs Outdated
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.46%. Comparing base (08ca9e1) to head (e84b8a4).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##           main    #8143       +/-   ##
=========================================
+ Coverage      0   67.46%   +67.46%     
=========================================
  Files         0     2312     +2312     
  Lines         0   100301   +100301     
  Branches      0     9021     +9021     
=========================================
+ Hits          0    67670    +67670     
- Misses        0    30353    +30353     
- Partials      0     2278     +2278     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mimartin12 mimartin12 added the t:bugfix Change Type - Bugfix label Aug 4, 2026
@mimartin12
mimartin12 marked this pull request as ready for review August 5, 2026 14:19
@mimartin12
mimartin12 requested review from a team as code owners August 5, 2026 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants