[PM-27605] Populate MaxStorageGbIncreased for storage increase from 1GB to 5GB.

[cyprain-okeke]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27605

📔 Objective

This PR implements the transition migration for PM-27605: Populate MaxStorageGbIncreased for storage increase from 1GB to 5GB.

This migration populates the MaxStorageGbIncreased column (added in PM-27603) by setting it to MaxStorageGb + 4 for all User and Organization records that have a storage quota defined.

Changes

• Added: util/Migrator/DbScripts/2025-11-14_00_PopulateMaxStorageGbIncreased.sql
  • Updates all User records where MaxStorageGb IS NOT NULL
  • Updates all Organization records where MaxStorageGb IS NOT NULL
  • Sets MaxStorageGbIncreased = MaxStorageGb + 4 to represent the 4GB increase

Dependencies

• Requires PM-27603 to be deployed first (creates the MaxStorageGbIncreased column)
• This migration will fail if the column doesn't exist

Database Impact

According to the ticket, as of October 28th, 2025:

• ~148,000 User records will be updated
• ~35,000 Organization records will be updated

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @cyprain-okeke's task in 1m 52s —— View job

---

Code Review Complete

I've reviewed this PR and identified several issues that should be addressed before merging.

---

Critical Issues

1. Missing Transaction Control

The migration script 2025-12-12_00_PopulateMaxStorageGbIncreased.sql lacks explicit transaction control with TRY-CATCH blocks. With ~183,000 records to update across two tables, if the migration fails partway through (timeout, service restart, resource constraints), the database will be in an inconsistent state.

While the idempotency check (MaxStorageGbIncreased IS NULL) provides partial safety, without proper transaction management, a failed migration leaves uncertain state requiring manual investigation.

Recommendation: Wrap the entire migration in proper transaction blocks with error handling, though this is complicated by the GO statements (see next issue).

---

2. GO Statements Break Transaction Scope

The migration file contains multiple GO statements (lines 23, 42, 67, 72, 91, 116, 121) which are batch separators that reset variable scope and prevent wrapping the migration in a single transaction. Each GO creates a new batch, making it impossible to rollback the entire operation if something fails.

Impact: If the Organization table update succeeds but the User table update fails, you'll have inconsistent data with no automatic rollback.

Recommendation: Remove unnecessary GO statements or restructure to allow for proper transactional control. The GO statements after index drops are necessary, but the ones between logical sections could potentially be removed.

---

3. Index Creation Without ONLINE Option

The temporary indexes are created without the WITH (ONLINE = ON) option (lines 38 and 87), which can block concurrent reads/writes during index creation on production tables.

Recommendation:

CREATE INDEX IX_TEMP_User_MaxStorageGb_MaxStorageGbIncreased
ON [User]([MaxStorageGb], [MaxStorageGbIncreased])
WITH (ONLINE = ON);

Note: This requires Enterprise Edition. Consider adding conditional logic or documenting the requirement.

---

Important Issues

4. Batch Delay May Be Insufficient

The 100ms delay between batches (lines 63, 112) may not provide sufficient throttling for Azure SQL Database with DTU limits. With 5000-record batches, this could trigger throttling or DTU exhaustion.

Recommendation: Increase delay to 250-500ms or implement dynamic throttling based on sys.dm_db_resource_stats.

---

5. Missing Progress Timestamps

The migration includes progress logging but no timestamps. With 183,000 records, the migration could take 6-7 minutes minimum. DBAs cannot estimate completion time or identify if the migration is stalled.

Recommendation: Add timestamp logging:

PRINT CONCAT('Batch completed at: ', GETDATE(), ' - Total: ', @TotalUpdated);

---

Positive Aspects

• ✅ Excellent dependency validation at the start of the migration
• ✅ Proper idempotency with MaxStorageGbIncreased IS NULL check
• ✅ Good use of temporary indexes with cleanup
• ✅ Appropriate batch sizing (5000 records)
• ✅ Consistent logic between Create and Update procedures
• ✅ Logic correctly differentiates migration backfill (+4) vs. new records (direct from pricing service)

---

Notes

The logic difference between migration and stored procedures is correct and intentional per the PR discussion:

• Migration adds +4 to existing records (1GB → 5GB increase)
• Stored procedures set new records to @MaxStorageGb directly (pricing service will provide 5GB)

This requires coordinated deployment with the pricing service updates.

---

[claude]

❌ Missing column existence check

This migration will fail with a cryptic error if the MaxStorageGbIncreased column doesn't exist (PM-27603 not deployed).

Recommendation: Add a check at the start of the migration:

-- Verify MaxStorageGbIncreased column exists before proceeding
IF NOT EXISTS (
    SELECT * FROM sys.columns 
    WHERE object_id = OBJECT_ID('[dbo].[User]') 
    AND name = 'MaxStorageGbIncreased'
)
BEGIN
    RAISERROR('MaxStorageGbIncreased column does not exist in User table. PM-27603 must be deployed first.', 16, 1);
    RETURN;
END;

IF NOT EXISTS (
    SELECT * FROM sys.columns 
    WHERE object_id = OBJECT_ID('[dbo].[Organization]') 
    AND name = 'MaxStorageGbIncreased'
)
BEGIN
    RAISERROR('MaxStorageGbIncreased column does not exist in Organization table. PM-27603 must be deployed first.', 16, 1);
    RETURN;
END;
GO

This provides a clear deployment dependency error rather than "invalid column name" errors.

[claude]

💭 Transaction safety consideration

The GO statement here commits the User table update as a separate transaction from the Organization update. This means:

• ✅ Pro: Reduces lock duration, better for production with 148k User records
• ⚠️ Con: If Organization update fails, User table will have been updated but Organization table won't (partial completion)

Question: Is this intentional to minimize lock contention, or should both updates be wrapped in a single transaction for atomicity?

If atomicity is needed:

BEGIN TRANSACTION;
-- User update
-- Organization update  
COMMIT TRANSACTION;
GO

Please clarify the intended behavior for this migration.

[github-actions]

Checkmarx One – Scan Summary & Details – ffea5429-f801-4336-b735-ee66c97ad449

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	Path_Traversal	/src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 56	details Method at line 56 of /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs gets dynamic data from the model element. This ... ID: HwphbdG3YaZTdiMdREyc8GsPw%2Bw%3D Attack Vector

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/Controllers/CollectionsController.cs: 208

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.41%. Comparing base (982957a) to head (54a30bd).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6579      +/-   ##
==========================================
+ Coverage   54.60%   58.41%   +3.80%     
==========================================
  Files        1921     1921              
  Lines       85474    85474              
  Branches     7648     7648              
==========================================
+ Hits        46677    49933    +3256     
+ Misses      37020    33690    -3330     
- Partials     1777     1851      +74     

☔ View full report in Codecov by Sentry.
📢 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.

[amorask-bitwarden]

Change looks fine to me - I'm going to lean on @bitwarden/dept-dbops for the PopulateMaxStorageGbIncreased.sql file. Also, looks like you have some CI failures.

[mkincaid-bw]

How is the logic going to correlate with the actual enablement of the additional 4GB? From what I can tell, once this PR deploys, then any new Users/Orgs that get created, or Users/Orgs that get updated, will have the MaxStorageGbIncreased set to the same as MaxStorageGb. Is that logic correct? It seems like if we deploy this PR but don't enable the actual increase until later, then new Users/Orgs will get set to the old default, and then won't get updated since the migration will have already run. Am I missing something?

[mkincaid-bw]

This migration is going to take quite a while. There's no index on the MaxStorageGb column so the script as-is will force hundreds (or thousands) of table scans - one for each update statement in the batch.

I tested this alternative against a backup (creating an index first) and it ran in 3-4 minutes:

IF NOT EXISTS (
    SELECT 1
    FROM sys.indexes 
    WHERE object_id = OBJECT_ID('dbo.User') 
    AND name = 'IX_TEMP_User_MaxStorageGb_MaxStorageGbIncreased'
)
BEGIN
    CREATE INDEX IX_TEMP_User_MaxStorageGb_MaxStorageGbIncreased
    ON [User]([MaxStorageGb], [MaxStorageGbIncreased]);
END
GO

DECLARE @BatchSize INT = 5000;
DECLARE @RowsAffected INT = 1;
DECLARE @TotalUpdated INT = 0;

PRINT 'Starting User table update...';

WHILE @RowsAffected > 0
BEGIN
    UPDATE TOP (@BatchSize) [dbo].[User]
    SET [MaxStorageGbIncreased] = [MaxStorageGb] + 4
    WHERE [MaxStorageGb] IS NOT NULL
      AND [MaxStorageGbIncreased] IS NULL; -- Only update rows not yet processed

    SET @RowsAffected = @@ROWCOUNT;
    SET @TotalUpdated = @TotalUpdated + @RowsAffected;

    PRINT 'Users updated: ' + CAST(@TotalUpdated AS VARCHAR(10));

    WAITFOR DELAY '00:00:00.100'; -- 100ms delay to reduce contention
END

PRINT 'User table update complete. Total rows updated: ' + CAST(@TotalUpdated AS VARCHAR(10));
GO

DROP INDEX IF EXISTS [dbo].[User].[IX_TEMP_User_MaxStorageGb_MaxStorageGbIncreased];
GO

[cyprain-okeke]

The logic works correctly when both changes deploy together or in the correct sequence:

1. This migration script (2025-11-14_00_PopulateMaxStorageGbIncreased.sql):
   - Runs once during deployment
   - Updates existing records: MaxStorageGbIncreased = MaxStorageGb + 4
   - Migrates old 1GB quota → new 5GB quota
2. Stored procedures (e.g., User_Create, Organization_Create,User_Update,Organization_Update):
   - For new records created after this PR
   - Set MaxStorageGbIncreased = @MaxStorageGb (same value, not +4)
   - The @MaxStorageGb parameter comes from the API/Pricing Service
3. Pricing Service (separate deployment):
   - Updates plan definitions: 1GB → 5GB
   - API calls stored procedures with @MaxStorageGb = 5

[mkincaid-bw]

LGTM

[amorask-bitwarden]

@cyprain-okeke This looks good - however, it has to be merged for a specific release so leave the hold label on the PR. I'm going to change the subtask to a task and move it to On Hold with a note about the release and move the parent ticket to done.

[mkincaid-bw]

Discussed this with Conner. In Isolation, the PR is approved, but changes will need to be coordinated with @cd-bitwarden on PR 6482 to ensure changes are not overwritten.