fix: potential inconsistent ledger creation #699
Conversation
WalkthroughThe changes update the ledger creation process in the storage driver. The Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Client
participant Driver as CreateLedger
participant Store as SystemStore
participant Bucket as Bucket
Client->>Driver: CreateLedger(ctx, ledger)
Driver->>Store: Attempt ledger creation
alt Error occurs
Driver->>Driver: Check error type
alt Constraint violation
Driver->>Client: Return "LedgerAlreadyExists" error
else Other error
Driver->>Client: Return resolved error
end
else Creation successful
Driver->>Bucket: Check initialization status
alt Bucket initialized
Driver->>Bucket: Verify up-to-date status
alt Bucket outdated
Driver->>Client: Return "Bucket outdated" error
else Bucket up-to-date
Driver->>Bucket: Add ledger to bucket
Driver->>Client: Return success
end
else Bucket not initialized
Driver->>Bucket: Migrate bucket
Driver->>Bucket: Add ledger to bucket
Driver->>Client: Return success
end
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/driver/driver_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (2)
internal/storage/driver/driver.go (2)
42-47: LGTM! Improved error handling for ledger creation.The enhanced error handling now properly distinguishes between constraint violations (duplicate ledgers) and other database errors, helping prevent inconsistent ledger states.
64-66: LGTM! Added proper error handling for ledger addition to bucket.The error handling ensures that any failures during ledger addition are properly caught and reported, preventing potential inconsistencies.
| } else { | ||
| if err := b.Migrate( | ||
| ctx, | ||
| migrations.WithLockRetryInterval(d.migratorLockRetryInterval), | ||
| ); err != nil { | ||
| return nil, fmt.Errorf("migrating bucket: %w", err) | ||
| } |
There was a problem hiding this comment.
Add ledger to bucket after migration.
When the bucket is not initialized, the code performs migration but doesn't add the ledger to the bucket afterward. This could lead to an inconsistent state where the ledger exists in the system store but not in the bucket.
Apply this diff to fix the issue:
} else {
if err := b.Migrate(
ctx,
migrations.WithLockRetryInterval(d.migratorLockRetryInterval),
); err != nil {
return nil, fmt.Errorf("migrating bucket: %w", err)
}
+ if err := b.AddLedger(ctx, *l); err != nil {
+ return nil, fmt.Errorf("adding ledger to bucket after migration: %w", err)
+ }
}📝 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.
| } else { | |
| if err := b.Migrate( | |
| ctx, | |
| migrations.WithLockRetryInterval(d.migratorLockRetryInterval), | |
| ); err != nil { | |
| return nil, fmt.Errorf("migrating bucket: %w", err) | |
| } | |
| } else { | |
| if err := b.Migrate( | |
| ctx, | |
| migrations.WithLockRetryInterval(d.migratorLockRetryInterval), | |
| ); err != nil { | |
| return nil, fmt.Errorf("migrating bucket: %w", err) | |
| } | |
| if err := b.AddLedger(ctx, *l); err != nil { | |
| return nil, fmt.Errorf("adding ledger to bucket after migration: %w", err) | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
==========================================
- Coverage 81.63% 81.61% -0.03%
==========================================
Files 131 131
Lines 7085 7086 +1
==========================================
- Hits 5784 5783 -1
- Misses 1000 1002 +2
Partials 301 301 ☔ View full report in Codecov by Sentry. |
No description provided.