[PB-6473]: feat/mail blob pointer table#74
Merged
Conversation
jzunigax2
force-pushed
the
feat/stalwart-events-webhook
branch
from
June 30, 2026 14:54
96cf7aa to
2a9c787
Compare
- Added `findBucketContextByProviderInternalId` method to `AddressRepository` for resolving user and bucket context. - Updated `AccountService` to delete provider links during account provisioning failure. - Implemented `createBucketEntry` method in `BridgeClient` for creating bucket entries in response to Stalwart events. - Enhanced `StalwartEventsService` to handle batch events and create bucket entries based on resolved account context. - Added unit tests for the new functionality in `StalwartEventsService` and `AddressRepository` to ensure proper behavior.
- Updated `deleteEmail` method in `EmailService` to handle quota entry release upon email deletion. - Introduced `releaseQuotaEntry` method to manage quota entries based on email deletion results. - Modified `deleteEmail` in `MailProvider` to return a result object containing the deleted entry key. - Enhanced unit tests for `EmailService` and `JmapMailProvider` to cover new deletion behavior and quota management scenarios. - Integrated `BridgeClient` for handling quota entry deletions in the bridge service.
- Introduced `decodeStalwartIdBig` function to handle Stalwart IDs exceeding JavaScript's safe integer range, allowing for proper decoding of email IDs. - Updated `decodeStalwartId` to utilize the new decoding function and throw an error for IDs exceeding safe integer limits. - Enhanced `StalwartEventsAuthGuard` to throw an `UnauthorizedException` for malformed authorization headers lacking a colon separator. - Added unit tests for the new decoding function and authentication guard to ensure robust error handling and functionality.
- Added migration for creating the `mail_bucket_entries` table to store email usage data. - Introduced `MailUsageModule` and `MailUsageService` for managing email bucket entries and tracking usage. - Updated `AddressRepository`, `EmailService`, and `StalwartEventsService` to integrate with the new mail usage functionality. - Refactored `BridgeClient` methods to accommodate changes in bucket entry management. - Enhanced models and repositories to support the new mail bucket entry structure.
- Modified `AddressRepository` to include `mailAccountId` in the returned results and updated account structure. - Refactored `EmailService` to replace `BridgeClient` with `MailUsageService` for managing email quota entries. - Enhanced unit tests across `AddressRepository`, `EmailService`, and `StalwartEventsService` to reflect changes in account ID handling and usage tracking. - Introduced new tests for `MailUsageService` and `MailBucketEntryRepository` to ensure proper functionality and error handling.
…nd repositories - Updated migration to change `mail_account_id` to `mail_address_id` in the `mail_bucket_entries` table. - Refactored `AddressRepository`, `EmailService`, and `StalwartEventsService` to use `mailAddressId` instead of `mailAccountId`. - Adjusted related models, repositories, and unit tests to reflect the new naming convention for better clarity and consistency. - Enhanced tests in `MailUsageService` and `MailBucketEntryRepository` to ensure proper functionality with the updated identifiers.
jzunigax2
force-pushed
the
feat/mail-blob-pointer-table
branch
from
July 1, 2026 23:40
efddac8 to
6692bb4
Compare
📝 WalkthroughWalkthroughThis change adds persisted mail bucket-entry tracking, updates BridgeClient entry operations, and routes Stalwart ingest and permanent email deletion through MailUsageService. Account bucket context now includes the mail address ID. ChangesMail usage tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StalwartEventsService
participant MailUsageService
participant BridgeClient
participant MailBucketEntryRepository
StalwartEventsService->>MailUsageService: trackStoredMessage(params)
MailUsageService->>MailBucketEntryRepository: findByEntryKey(entryKey)
MailUsageService->>BridgeClient: createBucketEntry(userUuid, bucketId, size)
MailUsageService->>MailBucketEntryRepository: create(mailAddressId, entryKey, bridgeEntryId, size)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
jzunigax2
marked this pull request as ready for review
July 13, 2026 22:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/modules/email/email.service.ts (1)
373-377: 🗄️ Data Integrity & Integration | 🔵 TrivialProvide a durable retry for failed quota release.
releaseStoredMessageruns after permanent deletion, while the surrounding catch suppresses failures. If usage release fails, the message is gone and no later deletion retries the quota update. Confirm that an outbox, reconciliation job, or equivalent retry path exists; otherwise quota can remain overstated.🤖 Prompt for 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. In `@src/modules/email/email.service.ts` around lines 373 - 377, Ensure the permanent-deletion flow around releaseStoredMessage has a durable retry mechanism for failed quota release, such as an existing outbox or reconciliation job. If no equivalent path exists, persist the release operation for later retry instead of allowing the surrounding suppressed failure to lose the quota update, while preserving the current deletion behavior.
🤖 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 `@src/modules/infrastructure/jmap/jmap-mail.provider.ts`:
- Around line 845-847: The entry-key construction must preserve complete
Stalwart IDs instead of truncating them with the 32-bit masks in the visible
account/email ID conversion logic. Update the identifiers used by the returned
key to retain full decoded values (or the original ID strings), ensuring IDs
differing by 2^32 produce distinct keys, and add a regression test covering that
case.
In `@src/modules/usage/mail-usage.service.ts`:
- Around line 43-60: Update the persistence error handling around entries.create
in the mail usage flow to roll back the minted bridge bucket entry for every
failure, while preserving the existing duplicate-entry debug behavior. Guard
bridge.deleteBucketEntry so rollback failures are caught and logged with enough
context for reconciliation, then rethrow the original persistence error. Update
mail-usage.service.spec.ts to expect rollback for generic persistence failures
and verify rollback errors are handled.
- Around line 68-93: The release flow in releaseStoredMessage must be idempotent
when BridgeClient.deleteBucketEntry reports a missing bridge entry. Handle the
bridge client's 404/not-found error as successful and continue deleting the
local entry, while still propagating other bridge deletion failures; preserve
the existing missing-local-entry early return and success logging.
---
Nitpick comments:
In `@src/modules/email/email.service.ts`:
- Around line 373-377: Ensure the permanent-deletion flow around
releaseStoredMessage has a durable retry mechanism for failed quota release,
such as an existing outbox or reconciliation job. If no equivalent path exists,
persist the release operation for later retry instead of allowing the
surrounding suppressed failure to lose the quota update, while preserving the
current deletion behavior.
🪄 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
Run ID: 54d91ed6-bec9-4e5b-bed7-3966d50ae1c2
📒 Files selected for processing (19)
migrations/20260623120000-create-mail-bucket-entries.jssrc/modules/account/repositories/address.repository.spec.tssrc/modules/account/repositories/address.repository.tssrc/modules/email/email.module.tssrc/modules/email/email.service.spec.tssrc/modules/email/email.service.tssrc/modules/infrastructure/bridge/bridge.service.spec.tssrc/modules/infrastructure/bridge/bridge.service.tssrc/modules/infrastructure/jmap/jmap-mail.provider.tssrc/modules/stalwart-events/stalwart-events.module.tssrc/modules/stalwart-events/stalwart-events.service.spec.tssrc/modules/stalwart-events/stalwart-events.service.tssrc/modules/usage/domain/mail-bucket-entry.domain.tssrc/modules/usage/mail-usage.module.tssrc/modules/usage/mail-usage.service.spec.tssrc/modules/usage/mail-usage.service.tssrc/modules/usage/models/mail-bucket-entry.model.tssrc/modules/usage/repositories/mail-bucket-entry.repository.spec.tssrc/modules/usage/repositories/mail-bucket-entry.repository.ts
Comment on lines
+845
to
847
| const numericAccountId = decodeStalwartIdBig(accountId) & 0xffffffffn; | ||
| const documentId = decodeStalwartIdBig(emailId) & 0xffffffffn; | ||
| return `${numericAccountId}:${documentId}`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'NODE'
const mask = (value) => value & 0xffffffffn;
const a = 1n;
const b = (1n << 32n) + 1n;
if (mask(a) !== mask(b)) process.exit(1);
console.log(`${mask(a)} === ${mask(b)}`);
NODERepository: internxt/mail-server
Length of output: 166
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n## provider outline\n'
ast-grep outline src/modules/infrastructure/jmap/jmap-mail.provider.ts --view expanded | sed -n '1,220p'
printf '\n## usage service outline\n'
ast-grep outline src/modules/usage/mail-usage.service.ts --view expanded | sed -n '1,240p'
printf '\n## entryKey references\n'
rg -n "entryKey|decodeStalwartIdBig|0xffffffffn|release lookup|duplicate" src/modules -SRepository: internxt/mail-server
Length of output: 10366
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n## provider slice around target lines\n'
sed -n '820,860p' src/modules/infrastructure/jmap/jmap-mail.provider.ts | cat -n
printf '\n## usage service slice around entryKey usage\n'
sed -n '1,260p' src/modules/usage/mail-usage.service.ts | cat -nRepository: internxt/mail-server
Length of output: 4734
Do not truncate Stalwart IDs to 32 bits. & 0xffffffffn makes distinct large IDs collide in entryKey, which this path uses for duplicate detection and release. That can skip tracking or release the wrong stored message. Keep the full decoded IDs, or use the original ID strings, and add a regression test for values that differ by 2^32.
🤖 Prompt for 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.
In `@src/modules/infrastructure/jmap/jmap-mail.provider.ts` around lines 845 -
847, The entry-key construction must preserve complete Stalwart IDs instead of
truncating them with the 32-bit masks in the visible account/email ID conversion
logic. Update the identifiers used by the returned key to retain full decoded
values (or the original ID strings), ensuring IDs differing by 2^32 produce
distinct keys, and add a regression test covering that case.
xabg2
reviewed
Jul 14, 2026
| const { statusCode, body } = await this.httpClient.request({ | ||
| method: 'DELETE', | ||
| path: `${this.basePath}/v2/gateway/users/${encodeURIComponent(userUuid)}/buckets/${encodeURIComponent(bucketId)}/entries/${encodeURIComponent(key)}`, | ||
| path: `${this.basePath}/v2/gateway/users/${encodeURIComponent(userUuid)}/buckets/${encodeURIComponent(bucketId)}/entries/${encodeURIComponent(entryId)}`, |
Contributor
There was a problem hiding this comment.
What about extracting the encodeURIComponent(userUuid) / encodeURIComponent(bucketId) / encodeURIComponent(entryId) to a constant?
Something like encodedUserUuid = ... and so on.
|
|
||
| describe('handleBatch', () => { | ||
| it('when an ingest event resolves to a bucket, then creates a bucket entry keyed by accountId:documentId', async () => { | ||
| it('when an ingest event resolves to a bucket, then tracks the stored message keyed by accountId:documentId', async () => { |
| }); | ||
|
|
||
| describe('trackStoredMessage', () => { | ||
| it('when the entry is new, then mints a bridge entry and persists the pointer', async () => { |
xabg2
approved these changes
Jul 17, 2026
…ess rollback on errors - Updated all test cases in the `bridge.service.spec.ts`, `stalwart-events.service.spec.ts`, `mail-usage.service.spec.ts`, and `mail-bucket-entry.repository.spec.ts` files to use 'test' instead of 'it' for improved readability and uniformity across the test suite. - Added error handling and rollback logic in `mail-usage.service.ts` to manage failures during bucket entry deletion, enhancing robustness.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
migrations/20260717233856-create-mail-bucket-entries.js (1)
7-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap migration operations in a transaction.
When a migration performs multiple operations sequentially (such as creating a table and then adding an index), it is highly recommended to wrap them in a transaction. This ensures that if a subsequent operation fails, the entire migration rolls back cleanly, avoiding a partially applied database state that could break future migration runs.
Note: DDL transactions are supported natively in PostgreSQL and SQLite, though not in MySQL.
♻️ Proposed refactor
- async up(queryInterface, Sequelize) { - await queryInterface.createTable(TABLE_NAME, { - id: { - type: Sequelize.UUID, - defaultValue: Sequelize.literal('gen_random_uuid()'), // using suggested fix - primaryKey: true, - allowNull: false, - }, - mail_address_id: { - type: Sequelize.UUID, - allowNull: false, - references: { model: 'mail_addresses', key: 'id' }, - onUpdate: 'CASCADE', - onDelete: 'CASCADE', - }, - entry_key: { - type: Sequelize.STRING(255), - allowNull: false, - unique: true, - }, - bridge_entry_id: { - type: Sequelize.STRING(24), - allowNull: false, - }, - size: { - type: Sequelize.BIGINT, - allowNull: false, - }, - created_at: { - type: Sequelize.DATE, - allowNull: false, - defaultValue: Sequelize.fn('now'), - }, - updated_at: { - type: Sequelize.DATE, - allowNull: false, - defaultValue: Sequelize.fn('now'), - }, - }); - - await queryInterface.addIndex(TABLE_NAME, ['mail_address_id']); - }, + async up(queryInterface, Sequelize) { + await queryInterface.sequelize.transaction(async (transaction) => { + await queryInterface.createTable(TABLE_NAME, { + id: { + type: Sequelize.UUID, + defaultValue: Sequelize.literal('gen_random_uuid()'), + primaryKey: true, + allowNull: false, + }, + mail_address_id: { + type: Sequelize.UUID, + allowNull: false, + references: { model: 'mail_addresses', key: 'id' }, + onUpdate: 'CASCADE', + onDelete: 'CASCADE', + }, + entry_key: { + type: Sequelize.STRING(255), + allowNull: false, + unique: true, + }, + bridge_entry_id: { + type: Sequelize.STRING(24), + allowNull: false, + }, + size: { + type: Sequelize.BIGINT, + allowNull: false, + }, + created_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn('now'), + }, + updated_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn('now'), + }, + }, { transaction }); + + await queryInterface.addIndex(TABLE_NAME, ['mail_address_id'], { transaction }); + }); + },🤖 Prompt for 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. In `@migrations/20260717233856-create-mail-bucket-entries.js` around lines 7 - 48, Wrap the sequential operations in the up migration with a transaction, passing the transaction option to both createTable and addIndex and committing only after both succeed. Roll back the transaction on failure while preserving the existing table schema and index definitions in up.
🤖 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.
Nitpick comments:
In `@migrations/20260717233856-create-mail-bucket-entries.js`:
- Around line 7-48: Wrap the sequential operations in the up migration with a
transaction, passing the transaction option to both createTable and addIndex and
committing only after both succeed. Roll back the transaction on failure while
preserving the existing table schema and index definitions in up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b364ea74-9273-4188-af86-16ee7a78fb83
📒 Files selected for processing (1)
migrations/20260717233856-create-mail-bucket-entries.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keep track of mail blobs using the new bucket entries table, on ingest event firing we store a pointer to the related network bucket entry so we can later on a delete event decrement size
Summary by CodeRabbit
New Features
Bug Fixes