Atomic migrations #142
Atomic migrations #142
Conversation
🦋 Changeset detectedLatest commit: c5fac73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request introduces enhancements to the migration system for the MongoDB package, focusing on implementing atomic migrations and robust error handling. Key changes include the addition of a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant MigrationPlugin
participant MigrationAdapter
participant Database
Client->>MigrationPlugin: Initiate Migration
MigrationPlugin->>MigrationAdapter: Attempt Lock
MigrationAdapter->>Database: Check Existing Migrations
alt Lock Successful
MigrationAdapter->>Database: Apply Migration
Database-->>MigrationAdapter: Migration Completed
MigrationAdapter-->>MigrationPlugin: Migration Success
else Lock Failed
MigrationAdapter-->>MigrationPlugin: Migration Locked Error
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@zemble/auth-anonymous
@zemble/auth-api-token
@zemble/auth
@zemble/auth-apple
@zemble/auth-otp
@zemble/bull
@zemble/bun
@zemble/core
create-zemble-app
create-zemble-plugin
@zemble/debug
@zemble/email-resend
@zemble/email-sendgrid
eslint-config-kingstinct
@zemble/firebase-auth
@zemble/graphql
@zemble/kv
@zemble/migrations
@zemble/mongodb
@zemble/node
@zemble/pino
@zemble/push-apple
@zemble/push-expo
@zemble/routes
@zemble/react
@zemble/sms-twilio
@zemble/sms-46elks
@zemble/supabase
@zemble/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/mongodb/migration-adapter-with-transactions.ts (1)
Missing retry mechanism in migration implementation
After reviewing both the migration adapter implementations, there is no retry mechanism present to prevent stuck migrations. The code only implements:
- Lock acquisition through
acquireUpLockandacquireDownLock- Transaction handling in
migration-adapter-with-transactions.ts- Error recording in the migrations collection
The current implementation will:
- Throw a
MigrationLockErrorif a migration is already running- Record errors in the database if a migration fails
- Not automatically retry failed migrations
🔗 Analysis chain
Line range hint
28-55: Verify retry mechanism implementationThe PR objectives mention implementing a retry mechanism to prevent stuck migrations, but this implementation doesn't show retry logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for retry-related implementations rg -l 'retry|backoff|attempt' --type typescript # Search for potential retry configurations ast-grep --pattern 'const $CONFIG = { $$$ retry: $_, $$$ }'Length of output: 152
Script:
#!/bin/bash # Search for retry-related implementations with ripgrep, using ts extension rg -l 'retry|backoff|attempt' -g '*.ts' # Search for retry configurations in TypeScript files ast-grep --pattern 'interface $_ { $$$ retry?: $_ $$$ }' # Look for retry-related function implementations ast-grep --pattern 'function $_($$_) { $$$ retry $$$ }' # Check the specific migration adapter file content cat packages/mongodb/migration-adapter-with-transactions.tsLength of output: 3475
Script:
#!/bin/bash # Let's check the migration-adapter.ts file for any retry logic in the lock acquisition cat packages/mongodb/migration-adapter.ts # Also check if there are any retry-related configurations in the core migration files rg -g '*.ts' -A 5 'interface.*Migration' packages/core/ rg -g '*.ts' -A 5 'type.*Migration' packages/core/Length of output: 4206
🧹 Nitpick comments (15)
packages/mongodb/migration-adapter.ts (5)
16-16: Consider creating the index in the background.
Creating a unique index can briefly lock the collection if not done in the background. Consider specifying "background: true" (for MongoDB versions <4.2) or "createIndexes" with "background: true" to avoid blocking writes during index creation.
22-40: Robust concurrency handling with acquireUpLock, but consider explicit logging for all error branches.
While the logic handles duplicate key errors, it might be helpful to add logs for other unexpected errors to aid debugging.
42-63: acquireDownLock concurrency approach looks consistent, but watch for partial migrations.
If the "down" migration partially applies changes before the lock error occurs, ensure you have a recovery plan in place. Also, consider consistent instrumentation/logging for easier troubleshooting.
Line range hint
70-83: Error storage is helpful, but consider more structured error data.
Storing the error message as JSON is good, yet storing structured fields (error name, stack, etc.) could improve querying in the database.
115-116: Status retrieval is straightforward, but watch out for large data sets.
If the number of entries grows significantly over time, consider pagination or capping.packages/migrations/plugin.ts (3)
129-130: Remove or replace temporary console logging.
This violates the no-console lint rule and should be replaced with the configured logger.- console.log('HEREERERE') + opts?.logger?.debug?.('HEREERERE')🧰 Tools
🪛 eslint
[error] 129-129: Unexpected console statement.
(no-console)
146-154: Progress callback usage is a neat feature.
Reporting progress via adapter-specific logic helps track partial migration results. Make sure it’s tested for large data migrations or concurrency scenarios.
159-165: Catching MigrationLockError to skip the migration is a good approach.
Prevents repeated attempts, but ensure logs or alerts so you’re aware if migrations skip frequently.packages/migrations/plugin.test.ts (1)
22-28: Verify migration behavior during startupThe test verifies that migrateUp isn't called, but consider adding tests for:
- Migration behavior when runOnStart is true
- Handling of duplicate migrations during startup
test('beforeStart', async () => { const migrateUpMock = mock(migrateUp) const app = await createTestApp(plugin) await runBeforeServe(app) expect(migrateUpMock).toHaveBeenCalledTimes(0) + + // Test with runOnStart enabled + const appWithRunOnStart = await createTestApp({ + ...plugin, + config: { runOnStart: true } + }) + await runBeforeServe(appWithRunOnStart) + expect(migrateUpMock).toHaveBeenCalledTimes(1) })packages/mongodb/test-setup.ts (1)
26-28: Add error handling to collection cleanupThe tearDownAfterEach function should handle potential failures during collection cleanup.
export const tearDownAfterEach = async () => { - await emptyAllCollections() + try { + await emptyAllCollections() + } catch (error) { + console.error('Failed to clean up collections:', error) + throw error + } }packages/mongodb/migration.test.ts (2)
16-37: Add validation for migration record timestampsThe test should verify the chronological order and validity of timestamps.
expect(migrations?.[0]).toStrictEqual( { name: 'testing', startedAt: expect.any(Date), _id: expect.any(ObjectId), completedAt: expect.any(Date), error: null, }, ) + // Verify timestamp order + const { startedAt, completedAt } = migrations[0] + expect(startedAt.getTime()).toBeLessThan(completedAt.getTime()) + expect(startedAt.getTime()).toBeLessThan(Date.now())
1-93: Consider adding network failure scenariosThe test suite would benefit from testing network-related edge cases:
- MongoDB connection drops during migration
- Replica set primary changes
- Write concern timeout
Would you like me to provide example test cases for these scenarios?
packages/mongodb/migration-adapter-with-transactions.ts (3)
46-54: Consider standardizing error objects for better monitoringWhile the error handling is improved, consider structuring the error object to be more Sentry-friendly:
$set: { name, - error: e instanceof Error ? e.message : JSON.stringify(e), + error: { + message: e instanceof Error ? e.message : JSON.stringify(e), + stack: e instanceof Error ? e.stack : undefined, + type: e.constructor.name, + }, erroredAt: new Date(), },
87-88: Consider implementing pagination for status retrievalFor databases with many migrations, retrieving all records at once could be inefficient. Consider implementing pagination:
- const res = await collection.find().toArray() + const res = await collection.find() + .sort({ completedAt: -1 }) + .limit(1000) // or accept as parameter + .toArray()
93-94: Consider atomic progress updatesProgress updates should be atomic to prevent race conditions. Consider using
findOneAndUpdatewith optimistic locking:await collection.findOneAndUpdate({ name: migrationStatus.name, + erroredAt: { $exists: false } // Only update if not errored }, { $set: { name: migrationStatus.name, progress: migrationStatus.progress, }, }, { upsert: true })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/thin-coins-wink.md(1 hunks)packages/migrations/MigrationLockError.ts(1 hunks)packages/migrations/plugin.test.ts(1 hunks)packages/migrations/plugin.ts(3 hunks)packages/mongodb/migration-adapter-with-transactions.ts(3 hunks)packages/mongodb/migration-adapter.ts(4 hunks)packages/mongodb/migration.test.ts(1 hunks)packages/mongodb/test-setup.ts(1 hunks)
🧰 Additional context used
🪛 eslint
packages/migrations/plugin.ts
[error] 129-129: Unexpected console statement.
(no-console)
🔇 Additional comments (14)
packages/mongodb/migration-adapter.ts (5)
2-2: Import of MigrationLockError is appropriate.
This error class helps differentiate lock conflicts from other database errors, maintaining code clarity and enabling specific handling logic.
68-69: Neat usage of getCollection for each migration run.
Centralizing the collection retrieval fosters consistency and reduces duplication.
95-101: Clean approach to “down” migration completion.
Deleting the migration document upon successful down ensures a clean reset. Great for maintaining clarity of completed migrations.
103-103: Persisting error details in down migration is consistent.
Matches the approach in the up migration. This symmetrical handling is beneficial for debugging.
121-122: Storing progress updates is a valuable feature.
Great for tracking intermediate states. Ensure that partial migration states are thoroughly tested.
packages/migrations/plugin.ts (5)
5-5: Importing MigrationLockError is consistent with the new locking design.
This ensures the plugin can properly handle lock-related errors.
133-139: Graceful handling of cancelledBecauseOfLock.
By halting further migrations when a lock is encountered, the code avoids collisions in a multi-instance environment.
155-158: Returning success is straightforward.
The return object structure is clear for future expansions (e.g., storing details about migrations applied).
167-168: Re-throwing other errors ensures they surface properly.
This pattern avoids swallowing unexpected exceptions.
171-172: Enforcing the presence of an up function.
This quick fail is beneficial for preventing partial or undefined migrations.
.changeset/thin-coins-wink.md (1)
1-5: Descriptive changeset message.
Provides context for future reference when reviewing the version history of @zemble/mongodb.
packages/migrations/MigrationLockError.ts (1)
1-6: New MigrationLockError class is properly structured.
Extending Error with a named constructor clarifies its intent. Make sure to capture and log the stack when throwing.
packages/mongodb/migration-adapter-with-transactions.ts (2)
2-2: LGTM! Import changes support atomic migrations
The addition of lock-related imports aligns with the PR objectives for implementing atomic migrations and preventing concurrent execution.
28-30: LGTM! Atomic migration implementation with proper locking
The lock acquisition before starting the transaction ensures atomic migrations and prevents concurrent execution.
| const { cancelledBecauseOfLock } = await completer | ||
| if (cancelledBecauseOfLock) { | ||
| const hasMigrationsCompleted = async () => { | ||
| const migrations = await getMigrations(migrationsPathOfApp, await config?.createAdapter?.(app)) | ||
| const migrationsToProcess = migrations.filter((migration) => !migration.isMigrated) | ||
| if (migrationsToProcess.length === 0) { | ||
| return true | ||
| } | ||
| logger.info({ migrationsToProcess }, `Waiting for ${migrationsToProcess.length} migrations to complete...`) | ||
| return false | ||
| } | ||
|
|
||
| const waitForMigrationsToComplete = async () => { | ||
| const hasCompleted = await hasMigrationsCompleted() | ||
| if (!hasCompleted) { | ||
| await new Promise((resolve) => { setTimeout(resolve, 1000) }) | ||
| return waitForMigrationsToComplete() | ||
| } | ||
| return undefined | ||
| } | ||
|
|
||
| return waitForMigrationsToComplete() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Retry mechanism for migrations that are cancelled because of a lock.
The recursive polling approach is straightforward. Consider adding a maximum wait time or exponential backoff if migrations remain locked for extended periods.
| test('migrateUp', async () => { | ||
| await createTestApp(plugin) | ||
| const { count } = await migrateUp({ | ||
|
|
||
| }) | ||
| expect(count).toBe(0) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for migrateUp
The current test only verifies the count is 0 but doesn't test the actual migration functionality. Consider adding test cases for:
- Successful migration with actual changes
- Error handling scenarios
- Retry mechanism behavior
test('migrateUp', async () => {
await createTestApp(plugin)
- const { count } = await migrateUp({
-
- })
- expect(count).toBe(0)
+ // Test successful migration
+ const { count: successCount } = await migrateUp({
+ migrations: [{
+ name: 'test-migration',
+ up: async () => { /* test migration logic */ }
+ }]
+ })
+ expect(successCount).toBe(1)
+
+ // Test error handling
+ await expect(migrateUp({
+ migrations: [{
+ name: 'failing-migration',
+ up: async () => { throw new Error('Migration failed') }
+ }]
+ })).rejects.toThrow('Migration failed')
})Committable suggestion skipped: line range outside the PR's diff.
| export const setupBeforeAll = async () => { | ||
| const { privateKey, publicKey } = await generateKeys() | ||
|
|
||
| setupEnvOverride({ PUBLIC_KEY: publicKey, PRIVATE_KEY: privateKey }) | ||
|
|
||
| await startInMemoryInstanceAndConfigurePlugin({ | ||
| replicaSetOptions: {}, | ||
| }) | ||
|
|
||
| await createTestApp(plugin) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and connection validation to setup
The setup function should handle potential failures and verify the MongoDB connection is successful.
export const setupBeforeAll = async () => {
+ try {
const { privateKey, publicKey } = await generateKeys()
setupEnvOverride({ PUBLIC_KEY: publicKey, PRIVATE_KEY: privateKey })
await startInMemoryInstanceAndConfigurePlugin({
replicaSetOptions: {},
})
+ // Verify MongoDB connection
+ const app = await createTestApp(plugin)
+ const isConnected = await app.providers.mongodb?.client.db().command({ ping: 1 })
+ if (!isConnected) {
+ throw new Error('MongoDB connection failed')
+ }
+
await createTestApp(plugin)
+ } catch (error) {
+ console.error('Test setup failed:', error)
+ throw error
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| test('run concurrent migrations', async () => { | ||
| const mongodbPlugin = plugin | ||
| const adapter = MongoMigrationAdapterWithTransaction({ | ||
| providers: mongodbPlugin.providers, | ||
| collectionName: 'migrations', | ||
| }) | ||
| const migrationRunner = mock(async () => { | ||
| await new Promise((resolve) => { setTimeout(resolve, 100) }) | ||
| }) | ||
|
|
||
| const migration1 = adapter.up('testing', migrationRunner) | ||
|
|
||
| const migration2 = adapter.up('testing', migrationRunner) | ||
|
|
||
| const migration3 = adapter.up('testing', migrationRunner) | ||
|
|
||
| await Promise.allSettled([migration1, migration2, migration3]) | ||
|
|
||
| const migrations = await mongodbPlugin.providers.mongodb?.db.collection('migrations').find().toArray() | ||
|
|
||
| expect(migrations).toHaveLength(1) | ||
| expect(migrationRunner).toHaveBeenCalledTimes(1) | ||
| expect(migrations?.[0]).toStrictEqual( | ||
| { | ||
| name: 'testing', | ||
| startedAt: expect.any(Date), | ||
| _id: expect.any(ObjectId), | ||
| completedAt: expect.any(Date), | ||
| error: null, | ||
| }, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance concurrent migration testing
The concurrent migration test could be more robust by:
- Testing with different delays
- Verifying lock acquisition
- Testing timeout scenarios
test('run concurrent migrations', async () => {
const mongodbPlugin = plugin
const adapter = MongoMigrationAdapterWithTransaction({
providers: mongodbPlugin.providers,
collectionName: 'migrations',
})
- const migrationRunner = mock(async () => {
- await new Promise((resolve) => { setTimeout(resolve, 100) })
- })
+ const delays = [100, 50, 150] // Different delays to test various scenarios
+ const migrationRunners = delays.map(delay => mock(async () => {
+ await new Promise((resolve) => { setTimeout(resolve, delay) })
+ }))
- const migration1 = adapter.up('testing', migrationRunner)
- const migration2 = adapter.up('testing', migrationRunner)
- const migration3 = adapter.up('testing', migrationRunner)
+ const migrations = migrationRunners.map(runner =>
+ adapter.up('testing', runner)
+ )
- await Promise.allSettled([migration1, migration2, migration3])
+ const results = await Promise.allSettled(migrations)
+ // Verify only one succeeded and others were rejected with lock error
+ const succeeded = results.filter(r => r.status === 'fulfilled')
+ const failed = results.filter(r => r.status === 'rejected')
+ expect(succeeded).toHaveLength(1)
+ expect(failed).toHaveLength(2)
+ failed.forEach(result => {
+ expect(result.status === 'rejected' && result.reason.message)
+ .toContain('Migration lock')
+ })
const migrations = await mongodbPlugin.providers.mongodb?.db.collection('migrations').find().toArray()
expect(migrations).toHaveLength(1)
- expect(migrationRunner).toHaveBeenCalledTimes(1)
+ // Verify only one runner was called
+ const totalCalls = migrationRunners.reduce((sum, runner) =>
+ sum + runner.mock.calls.length, 0)
+ expect(totalCalls).toBe(1)
})
+test('migration timeout', async () => {
+ const adapter = MongoMigrationAdapterWithTransaction({
+ providers: plugin.providers,
+ collectionName: 'migrations',
+ lockTimeout: 100 // Short timeout for testing
+ })
+
+ const longRunningMigration = adapter.up('timeout-test', async () => {
+ await new Promise(resolve => setTimeout(resolve, 200))
+ })
+
+ await expect(longRunningMigration).rejects.toThrow('Migration timeout')
+})Committable suggestion skipped: line range outside the PR's diff.
| await collection.findOneAndUpdate({ | ||
| name, | ||
| }, { | ||
| $set: { | ||
| name, | ||
| error: JSON.stringify(e), | ||
| error: e instanceof Error ? e.message : JSON.stringify(e), | ||
| erroredAt: new Date(), | ||
| }, | ||
| }, { upsert: true }) | ||
| throw e |
There was a problem hiding this comment.
Critical: Error update should use the session
The error update operation is not using the session, which could lead to inconsistency if the transaction is rolled back:
await collection.findOneAndUpdate({
name,
}, {
$set: {
name,
error: e instanceof Error ? e.message : JSON.stringify(e),
erroredAt: new Date(),
},
- }, { upsert: true })
+ }, { upsert: true, session })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await collection.findOneAndUpdate({ | |
| name, | |
| }, { | |
| $set: { | |
| name, | |
| error: JSON.stringify(e), | |
| error: e instanceof Error ? e.message : JSON.stringify(e), | |
| erroredAt: new Date(), | |
| }, | |
| }, { upsert: true }) | |
| throw e | |
| await collection.findOneAndUpdate({ | |
| name, | |
| }, { | |
| $set: { | |
| name, | |
| error: e instanceof Error ? e.message : JSON.stringify(e), | |
| erroredAt: new Date(), | |
| }, | |
| }, { upsert: true, session }) | |
| throw e |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/fifty-poems-joke.md (1)
4-5: Enhance the changeset description to better reflect the changes.The current description "Add lock handling" could be more descriptive. Consider expanding it to better capture the scope and impact of the changes.
-Add lock handling +Add atomic migration handling with locks + +- Implement migration locking mechanism to prevent concurrent migrations +- Add MigrationLockError for better error handling +- Prevent migrations from getting stuck by implementing retry mechanism +- Improve error visibility for monitoring
To think about:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores