fix: make import not transactional #892
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Store
participant DB
Client->>Controller: LockLedger(ctx)
Controller->>Store: LockLedger(ctx)
Store->>DB: Acquire advisory lock
Store-->>Controller: (locked Store, DB handle, release func, error)
Controller-->>Client: (locked Controller, DB handle, release func, error)
Note over Client,Controller: Client uses locked controller and DB, then calls release func
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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
Documentation and Community
|
gfyrag
force-pushed
the
hotfix/v2.2/import-not-transactional
branch
from
fb4367d to
3409f47
Compare
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: true
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: false
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
poem: true
labeling_instructions: []
path_filters:
- '!dist/**'
- '!**/*.app'
- '!**/*.bin'
- '!**/*.bz2'
- '!**/*.class'
- '!**/*.db'
- '!**/*.csv'
- '!**/*.tsv'
- '!**/*.dat'
- '!**/*.dll'
- '!**/*.dylib'
- '!**/*.egg'
- '!**/*.glif'
- '!**/*.gz'
- '!**/*.xz'
- '!**/*.zip'
- '!**/*.7z'
- '!**/*.rar'
- '!**/*.zst'
- '!**/*.ico'
- '!**/*.jar'
- '!**/*.tar'
- '!**/*.war'
- '!**/*.lo'
- '!**/*.log'
- '!**/*.mp3'
- '!**/*.wav'
- '!**/*.wma'
- '!**/*.mp4'
- '!**/*.avi'
- '!**/*.mkv'
- '!**/*.wmv'
- '!**/*.m4a'
- '!**/*.m4v'
- '!**/*.3gp'
- '!**/*.3g2'
- '!**/*.rm'
- '!**/*.mov'
- '!**/*.flv'
- '!**/*.iso'
- '!**/*.swf'
- '!**/*.flac'
- '!**/*.nar'
- '!**/*.o'
- '!**/*.ogg'
- '!**/*.otf'
- '!**/*.p'
- '!**/*.pdf'
- '!**/*.doc'
- '!**/*.docx'
- '!**/*.xls'
- '!**/*.xlsx'
- '!**/*.map'
- '!**/*.out'
- '!**/*.ppt'
- '!**/*.pptx'
- '!**/*.pkl'
- '!**/*.pickle'
- '!**/*.pyc'
- '!**/*.pyd'
- '!**/*.pyo'
- '!**/*.pub'
- '!**/*.pem'
- '!**/*.rkt'
- '!**/*.so'
- '!**/*.ss'
- '!**/*.eot'
- '!**/*.exe'
- '!**/*.pb.go'
- '!**/*.pb.gw.go'
- '!**/*.lock'
- '!**/*.ttf'
- '!**/*.yaml'
- '!**/*.yml'
- '!**/*.xml'
- '!**/*.cfg'
- '!**/*.toml'
- '!**/*.ini'
- '!**/*.mod'
- '!**/*.sum'
- '!**/*.work'
- '!**/*.json'
- '!**/*.mmd'
- '!**/*.svg'
- '!**/*.jpeg'
- '!**/*.jpg'
- '!**/*.png'
- '!**/*.gif'
- '!**/*.bmp'
- '!**/*.tiff'
- '!**/*.webm'
- '!**/*.woff'
- '!**/*.woff2'
- '!**/*.dot'
- '!**/*.md5sum'
- '!**/*.wasm'
- '!**/*.snap'
- '!**/*.parquet'
- '!**/gen/**'
- '!**/_gen/**'
- '!**/generated/**'
- '!**/@generated/**'
- '!**/vendor/**'
- '!**/*.min.js'
- '!**/*.min.js.map'
- '!**/*.min.js.css'
- '!**/*.tfstate'
- '!**/*.tfstate.backup'
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches:
- main
finishing_touches:
docstrings:
enabled: true
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
chat:
auto_reply: true
create_issues: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
code_generation:
docstrings:
language: en-US
path_instructions: []
|
gfyrag
force-pushed
the
hotfix/v2.2/import-not-transactional
branch
from
93dadd5 to
d99db44
Compare
![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
🧹 Nitpick comments (3)
internal/controller/ledger/controller.go (1)
21-21: Expanded controller interface for non-transactional importThe
LockLedgermethod addition to theControllerinterface mirrors the changes in theStoreinterface, providing a consistent approach to locking throughout the codebase. The method now returns the locked controller, database interface, cleanup function, and error.However, this method is missing documentation comments describing its behavior and usage.
Consider adding a documentation comment for the
LockLedgermethod describing its purpose, return values, and usage pattern, similar to other methods in this interface:+// LockLedger acquires an advisory lock on the ledger and returns: +// - a controller with the lock held +// - the database interface +// - a release function that must be called to release the lock +// - an error if the lock cannot be acquired LockLedger(ctx context.Context) (Controller, bun.IDB, func() error, error)internal/storage/ledger/store.go (1)
149-184: Comprehensive refactoring of LockLedger from table-based to advisory locksThe implementation now uses PostgreSQL advisory locks instead of table locks, providing better granularity and control. The method distinguishes between regular database connections (using session-level locks) and transactions (using transaction-scoped locks), with appropriate clean-up mechanisms for each case.
This approach provides several benefits:
- Better concurrency - advisory locks are more granular than table locks
- Explicit lifecycle management with the returned release function
- Proper handling of both connection and transaction contexts
The hashing of the ledger ID ensures a unique lock key per ledger, allowing concurrent operations on different ledgers.
internal/controller/system/state_tracker.go (1)
133-147: Import process is now non-transactional with proper lockingThe import process has been refactored to use
withLockwithout wrapping the entire operation in a transaction, making it non-transactional as intended in the PR title. The implementation correctly re-checks the ledger state after acquiring the lock to ensure consistency.This non-transactional approach for imports improves performance and reduces lock contention for large imports, which is especially important if they contain many transactions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.coderabbit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (18)
internal/api/bulking/mocks_ledger_controller_test.go(1 hunks)internal/api/common/mocks_ledger_controller_test.go(1 hunks)internal/api/v1/mocks_ledger_controller_test.go(1 hunks)internal/api/v2/mocks_ledger_controller_test.go(1 hunks)internal/controller/ledger/controller.go(1 hunks)internal/controller/ledger/controller_default.go(2 hunks)internal/controller/ledger/controller_generated_test.go(1 hunks)internal/controller/ledger/controller_with_cache.go(1 hunks)internal/controller/ledger/controller_with_events.go(1 hunks)internal/controller/ledger/controller_with_too_many_client_handling.go(1 hunks)internal/controller/ledger/controller_with_traces.go(3 hunks)internal/controller/ledger/store.go(1 hunks)internal/controller/ledger/store_generated_test.go(1 hunks)internal/controller/system/state_tracker.go(4 hunks)internal/storage/ledger/legacy/adapters.go(1 hunks)internal/storage/ledger/store.go(1 hunks)test/e2e/api_ledgers_import_test.go(3 hunks)test/e2e/app_lifecycle_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
internal/controller/ledger/controller_with_events.go (2)
internal/controller/ledger/controller.go (1)
Controller(17-78)internal/controller/system/controller.go (1)
Controller(22-33)
internal/api/bulking/mocks_ledger_controller_test.go (4)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerController(22-25)LedgerControllerMockRecorder(28-30)internal/api/v1/mocks_ledger_controller_test.go (2)
LedgerController(22-25)LedgerControllerMockRecorder(28-30)internal/api/v2/mocks_ledger_controller_test.go (2)
LedgerController(22-25)LedgerControllerMockRecorder(28-30)internal/controller/ledger/controller.go (1)
Controller(17-78)
internal/api/v1/mocks_ledger_controller_test.go (1)
internal/controller/ledger/controller.go (1)
Controller(17-78)
internal/storage/ledger/legacy/adapters.go (3)
internal/storage/ledger/store.go (1)
Store(23-45)internal/controller/ledger/store.go (1)
Store(30-65)internal/storage/ledger/legacy/store.go (1)
Store(9-14)
internal/controller/ledger/controller_generated_test.go (1)
internal/controller/ledger/controller.go (1)
Controller(17-78)
🔇 Additional comments (24)
internal/controller/ledger/store.go (1)
52-52: Implementation change aligned with making import non-transactionalThe
LockLedgermethod signature has been updated to return four values (Store,bun.IDB,func() error,error) instead of just an error. This change enables more fine-grained control over ledger locking by providing:
- A locked store
- A database interface (for direct DB operations)
- A cleanup function to release the lock
- An error value
This is consistent with the PR title "make import not transactional" as it separates lock acquisition from transaction boundaries, allowing for more flexible transaction management.
internal/controller/ledger/store_generated_test.go (1)
220-229: Mock updated to match new interface signatureThe mock implementation of
LockLedgerhas been correctly updated to match the new signature, properly extracting and returning all four values from the mock call results.internal/controller/ledger/controller_with_cache.go (1)
52-62: Proper implementation of new LockLedger method in cache wrapperThe implementation of
LockLedgerinControllerWithCachecorrectly:
- Delegates to the underlying controller's
LockLedgermethod- Returns early with errors if the underlying call fails
- Wraps the returned controller in a new
ControllerWithCachewith the same registry and ledger- Passes through the database interface and release function
This maintains the cache functionality while supporting the non-transactional lock pattern.
internal/api/v1/mocks_ledger_controller_test.go (1)
328-343: Implementation looks good!The mock implementation of the new
LockLedgermethod correctly follows the interface pattern. This method will now return a Controller, a database interface, a release function, and an error, supporting the non-transactional locking mechanism for imports.internal/controller/ledger/controller_with_too_many_client_handling.go (1)
135-145: Implementation follows existing patterns!The
LockLedgerimplementation correctly follows the same pattern as the existingBeginTXmethod, wrapping the returned controller while preserving the delay calculator and tracer. Error handling is properly implemented by returning early if the underlying controller'sLockLedgercall fails.internal/api/common/mocks_ledger_controller_test.go (1)
328-343: Implementation looks good!The mock implementation of the new
LockLedgermethod correctly follows the interface pattern. This method will now return a Controller, a database interface, a release function, and an error, supporting the non-transactional locking mechanism for imports.internal/controller/ledger/controller_with_events.go (1)
169-180: Implementation follows existing patterns!The
LockLedgerimplementation correctly follows the same pattern as the existingBeginTXmethod, wrapping the returned controller while preserving the ledger, listener, and parent reference. Error handling is properly implemented by returning early if the underlying controller'sLockLedgercall fails.One observation: unlike the
BeginTXmethod, this implementation doesn't sethasTxto true. This appears intentional since the lock operation doesn't create a transaction, which is consistent with the PR's purpose to make imports non-transactional.internal/controller/ledger/controller_with_traces.go (3)
42-42: Field added for tracing ledger locking operations.The new
lockLedgerHistogramfield adds metrics tracking for the lock ledger operation, consistent with the existing pattern for other operations in this controller.
140-143: Proper initialization for new lock ledger metrics.The initialization follows the established pattern used for other operation metrics in this controller.
463-485: Well-implemented LockLedger method with proper tracing.The new method wraps the underlying controller's
LockLedgermethod with tracing and metrics instrumentation, consistent with the implementation pattern used for other operations in this controller. The method properly propagates the controller, connection, release function, and error.internal/controller/ledger/controller_default.go (2)
77-90: LockLedger method implementation follows established controller patterns.The method properly creates a controller copy, delegates to the store's LockLedger, and returns the relevant components (controller, database connection, release function, and error). This implementation aligns with the PR objective to make imports non-transactional by using a dedicated locking mechanism.
194-194: Improved error handling with specialized error type.Using
NewErrImportinstead of the genericfmt.Errorfprovides better error categorization and handling capabilities for import-specific errors.internal/api/v2/mocks_ledger_controller_test.go (1)
328-343: Mock implementation for the new LockLedger method.The mock correctly implements the expected behavior for the new
LockLedgermethod, returning the appropriate four values: a controller, database connection, release function, and error. This addition maintains test compatibility with the interface changes.test/e2e/app_lifecycle_test.go (3)
77-77: Updated comment to reflect new advisory lock behavior.The comment now accurately explains that transactions will block earlier at the advisory lock acquisition stage rather than at the log insertion stage when the ledger is initializing.
111-113: Test updated to verify advisory locks instead of active insert queries.The query now correctly checks for advisory locks in
pg_locksinstead of active insert operations inpg_stat_activity, reflecting the system's new locking mechanism implementation.
118-120: Updated assertion for correct advisory lock count.The test now correctly expects
countTransactions+1advisory locks, accounting for an additional lock used for logs sync hashing. This aligns with the new implementation and ensures proper verification of the locking behavior.internal/storage/ledger/legacy/adapters.go (1)
299-309: Implementation of LockLedger aligns with interface changesThe new
LockLedgerimplementation properly propagates the locking mechanism through the adapter pattern. It correctly:
- Delegates to the underlying
newStore's locking mechanism- Creates a new adapter instance that wraps both the locked store and the legacy store with the updated database connection
- Returns the necessary components (store, database connection, release function, error) according to the interface contract
This change supports the PR's goal of making imports non-transactional by using explicit locking with proper cleanup.
internal/api/bulking/mocks_ledger_controller_test.go (1)
328-343: Mock implementation for LockLedger correctly matches the interfaceThe added mock method properly implements the
LockLedgermethod signature from theControllerinterface, returning the expected four values: a controller, a database interface, a release function, and an error. The mock recorder method is also correctly implemented to set up expectations for test cases.internal/controller/ledger/controller_generated_test.go (1)
327-342: Mock Controller implementation for LockLedger properly matches the interfaceThe generated mock implementation for
LockLedgercorrectly follows the method signature from theControllerinterface, returning the expected four return values (Controller, bun.IDB, func() error, error). The implementation properly handles type assertions and helper methods for test expectations.test/e2e/api_ledgers_import_test.go (3)
134-170: Comprehensive test for import error handling and recoveryThis new test case verifies important behavior for the non-transactional import process:
- It confirms that when importing data with errors (invalid transaction ID), the import correctly fails with the expected error code
- It verifies that despite the failure, valid logs before the error are successfully inserted
- It tests that the import process can be resumed from the point of failure with corrected logs
This test is essential for validating the PR's goal of making imports non-transactional, ensuring partial progress is maintained and can be resumed after addressing errors.
402-412: Improved test verification for advisory locksThis enhancement verifies that PostgreSQL advisory locks are being used as expected during concurrent ledger operations. The test now explicitly checks that:
- A transaction creation acquires exactly one advisory lock
- The test properly waits for the lock to be acquired before proceeding
This addition helps validate that the new locking mechanism is working correctly at the database level.
433-445: Thorough verification of advisory lock contentionThis test enhancement verifies the expected behavior when import attempts to run concurrently with transaction creation:
- It checks that two advisory locks exist: one granted and one waiting
- It confirms that import blocks on acquiring the advisory lock already held by transaction creation
- The test properly validates the non-transactional locking behavior
This verification is crucial for ensuring the new locking mechanism handles contention correctly and prevents concurrent modifications as expected.
internal/controller/system/state_tracker.go (2)
32-34: Improved ledger locking in handleStateThe implementation now uses the centralized
withLockhelper function for ledger locking, which properly manages the lock lifecycle.
159-175: Well-implemented withLock helper functionThe new
withLockhelper function centralizes the ledger locking logic with proper error handling and resource cleanup. It correctly logs errors from lock release but doesn't propagate them to avoid masking the original operation errors.The implementation includes:
- Proper acquisition of the lock via
LockLedger- Deferred release of the lock
- Error logging and telemetry for release failures
- Clean function signature that provides both the locked controller and connection to the callback
gfyrag
force-pushed
the
hotfix/v2.2/import-not-transactional
branch
from
b03d097 to
171a055
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/storage/ledger/store.go (1)
149-185: Advisory locks replace table locks with more explicit controlThe implementation now uses PostgreSQL advisory locks instead of table locks, with different locking strategies for different database types:
- For
*bun.DB: Gets a dedicated connection and usespg_advisory_lock, requiring explicit unlock and connection closure- For
bun.Tx: Usespg_advisory_xact_lockwhich is automatically released when the transaction ends- For other DB types: Panics with an error message
This approach provides more fine-grained control and aligns with the PR's goal to "make import not transactional".
Two suggestions:
- Consider adding comments explaining why advisory locks are preferred over table locks
- The panic for unknown DB types could be replaced with a proper error return for better caller experience
internal/controller/system/state_tracker.go (1)
158-175: Add withLock helper for centralized lock managementThis new helper function:
- Acquires a lock using the controller's LockLedger method
- Sets up proper release using defer to ensure locks are always released
- Logs and records errors during lock release
- Provides a consistent pattern for managing locks
This is a good architectural improvement that centralizes locking logic and ensures proper resource management.
One suggestion: Consider adding more context to the error messages, such as including the ledger ID in error logs to make debugging easier.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.coderabbit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (18)
internal/api/bulking/mocks_ledger_controller_test.go(1 hunks)internal/api/common/mocks_ledger_controller_test.go(1 hunks)internal/api/v1/mocks_ledger_controller_test.go(1 hunks)internal/api/v2/mocks_ledger_controller_test.go(1 hunks)internal/controller/ledger/controller.go(1 hunks)internal/controller/ledger/controller_default.go(2 hunks)internal/controller/ledger/controller_generated_test.go(1 hunks)internal/controller/ledger/controller_with_cache.go(1 hunks)internal/controller/ledger/controller_with_events.go(1 hunks)internal/controller/ledger/controller_with_too_many_client_handling.go(1 hunks)internal/controller/ledger/controller_with_traces.go(3 hunks)internal/controller/ledger/store.go(1 hunks)internal/controller/ledger/store_generated_test.go(1 hunks)internal/controller/system/state_tracker.go(4 hunks)internal/storage/ledger/legacy/adapters.go(1 hunks)internal/storage/ledger/store.go(1 hunks)test/e2e/api_ledgers_import_test.go(3 hunks)test/e2e/app_lifecycle_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/app_lifecycle_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/controller/ledger/controller.go
- internal/controller/ledger/controller_with_cache.go
- internal/api/v1/mocks_ledger_controller_test.go
- internal/controller/ledger/store_generated_test.go
- internal/controller/ledger/controller_with_too_many_client_handling.go
- internal/controller/ledger/controller_generated_test.go
- internal/storage/ledger/legacy/adapters.go
- internal/controller/ledger/controller_default.go
- internal/api/v2/mocks_ledger_controller_test.go
- internal/api/common/mocks_ledger_controller_test.go
- internal/api/bulking/mocks_ledger_controller_test.go
- internal/controller/ledger/controller_with_traces.go
- test/e2e/api_ledgers_import_test.go
- internal/controller/ledger/controller_with_events.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (4)
internal/controller/system/state_tracker.go (4)
8-9: Add new imports for OTLP error recording and Bun databaseThe imports have been updated to include OTLP for error recording and Bun for database operations. This aligns with the changes to use advisory locks and structured error handling.
Also applies to: 11-12
32-36: Centralize ledger locking through withLock helperThe handleState method now uses the new
withLockhelper function instead of directly managing transactions. This centralizes the locking logic and ensures consistent lock acquisition and release across different operations.
40-41: Maintain TODO commentKeep the TODO comment to remove the code in a later version. This suggests the ledger state persistence might be handled differently in the future.
133-147: Simplify Import method with withLock helperThe Import method has been significantly simplified by using the
withLockhelper. The previous implementation likely used explicit transaction management with SERIALIZABLE isolation, which has been replaced with advisory locks.This change aligns perfectly with the PR's goal to "make import not transactional" while still providing isolation through advisory locks.
| @@ -49,7 +49,7 @@ type Store interface { | |||
| DeleteAccountMetadata(ctx context.Context, address, key string) error | |||
| InsertLog(ctx context.Context, log *ledger.Log) error | |||
|
|
|||
| LockLedger(ctx context.Context) error | |||
| LockLedger(ctx context.Context) (Store, bun.IDB, func() error, error) | |||
There was a problem hiding this comment.
Significant interface change to LockLedger
The LockLedger method signature has been extended from returning just an error to returning a tuple of (Store, bun.IDB, func() error, error). This change suggests a shift to a more explicit resource management pattern where callers receive a locked controller, database connection, and release function.
This approach provides better control over lock acquisition and release but requires careful handling by callers to ensure the release function is always called.
#!/bin/bash
# Find all implementations of the LockLedger method to ensure they match the new signature
rg -A 3 'func.*LockLedger\(ctx context.Context\)' --type go🤖 Prompt for AI Agents (early access)
In internal/controller/ledger/store.go at line 52, the LockLedger method signature has changed to return (Store, bun.IDB, func() error, error) instead of just an error. Update all implementations of LockLedger to match this new signature by returning the locked Store instance, the database connection, a release function to unlock or clean up resources, and an error. Ensure callers of LockLedger are updated to handle the new return values properly, especially calling the release function to avoid resource leaks.
171a055 to
b72771b
Compare
gfyrag
force-pushed
the
hotfix/v2.2/import-not-transactional
branch
from
b72771b to
aa98ff9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/controller/system/state_tracker.go (1)
165-171: Good error handling for lock release.The error handling for the lock release is well-implemented, with proper logging and OTLP error recording. As discussed in the previous reviews, the lock will be automatically released if the underlying PostgreSQL connection is broken.
🧹 Nitpick comments (2)
internal/controller/system/state_tracker.go (2)
40-40: Consider adding more context to the TODO comment.The TODO comment lacks specifics about what needs to be removed in a later version and why.
- // todo: remove that in a later version + // TODO: Remove the explicit state update in a later version as it will be handled automatically by the locking mechanism
134-135: Add more context to the TODO comment.Similar to the previous TODO, this comment would benefit from additional context.
- // todo: remove that in a later version + // TODO: Remove the explicit ledger state check in a later version as it will be handled at a different layer
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/ledger/controller_default.go(2 hunks)internal/controller/system/controller.go(1 hunks)internal/controller/system/state_tracker.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/controller/system/controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/ledger/controller_default.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (4)
internal/controller/system/state_tracker.go (4)
8-11: Appropriate imports for the new locking mechanism.The added imports of
otlpandbunsupport the new locking mechanism, withotlpfor error recording andbunfor database connections.
32-34: Good refactoring to use the new centralized locking mechanism.Refactoring the
handleStatemethod to use the newwithLockhelper function centralizes the locking logic and improves code maintainability.
133-146: Improved import process to handle locking without transactions.The rewritten
Importmethod now uses the centralized locking mechanism without wrapping the entire import in a transaction, which aligns with the PR objective to make imports non-transactional. This change improves system robustness by allowing partial imports to succeed.
158-175: Well-implemented centralized locking mechanism.The new
withLockhelper function properly centralizes the ledger locking logic with good error handling:
- It acquires a lock via
LockLedger- Executes the provided function with the locked controller and DB connection
- Ensures lock release in a deferred function
- Properly logs and records any errors during release using OTLP
This approach improves code maintainability and ensures consistent lock management throughout the codebase.
gfyrag
force-pushed
the
hotfix/v2.2/import-not-transactional
branch
from
739f94d to
f61be4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/api/v2/controllers_logs_import.go (1)
44-46: HTTP status might be too harsh for malformed JSONA decoding error that is not
io.EOFcurrently returns a 500.
If the body contains invalid JSON, the client supplied bad data, so 400 (Bad Request) may be more appropriate:- common.InternalServerError(w, r, fmt.Errorf("reading input stream: %w", err)) + api.BadRequest(w, "IMPORT_DECODE", fmt.Errorf("invalid JSON input: %w", err))This distinguishes server failures from client mistakes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/api/common/middleware_resolver.go(2 hunks)internal/api/v2/controllers_logs_import.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/api/common/middleware_resolver.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/api/v2/controllers_logs_import.go (2)
pkg/testserver/api.go (1)
Import(225-228)internal/api/common/errors.go (1)
InternalServerError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (1)
internal/api/v2/controllers_logs_import.go (1)
22-27: 👍 Good defensive error wrappingWrapping the result of
Importwith a contextual message before pushing it ontoerrChanmakes downstream error handling clearer. No further action required.
| if err != nil { | ||
| handleError(err) | ||
| return | ||
| } | ||
| if stream != nil { | ||
| panic("got nil error while not at the end of the stream") | ||
| } | ||
|
|
||
| api.NoContent(w) | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invariant panic is useful but could be replaced with structured handling
The panic guards against receiving a nil error before the stream is drained,
which is great for catching programmer mistakes in tests, but it will crash the
server in production. Prefer returning a 500 and logging the state instead:
- if stream != nil {
- panic("got nil error while not at the end of the stream")
- }
+ if stream != nil {
+ err := errors.New("import routine finished prematurely while stream open")
+ common.InternalServerError(w, r, err)
+ return
+ }This preserves the invariant without taking the whole service down.
🤖 Prompt for AI Agents (early access)
In internal/api/v2/controllers_logs_import.go around lines 55 to 64, replace the panic that occurs when stream is not nil but error is nil with structured error handling. Instead of panicking, log the unexpected state and respond with a 500 Internal Server Error to the client. This change maintains the invariant check while preventing the server from crashing in production.
| close(stream) | ||
| break | ||
| stream = nil | ||
| } else { | ||
| common.InternalServerError(w, r, err) | ||
| common.InternalServerError(w, r, fmt.Errorf("reading input stream: %w", err)) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
💥 Potential panic & busy-loop after EOF
close(stream)is executed every timedec.Decodereturnsio.EOF.
On the second (and every subsequent) EOF,streamis alreadynil, so
close(stream)will panic with “close of nil channel”.- After
io.EOF, the loop continues and immediately callsdec.Decodeagain, which
will keep returningio.EOF, creating a tight busy-loop while you wait for
errChan.
A minimal fix is to guard the close and skip further decoding once EOF is seen:
if errors.Is(err, io.EOF) {
- close(stream)
- stream = nil
+ if stream != nil { // avoid double-close
+ close(stream)
+ stream = nil
+ }
+ continue // wait on select for errChan / ctx
}However, this still leaves the outer for to spin on continue.
Consider introducing a doneDecoding flag (or breaking the loop and having a
separate select) to eliminate the busy-loop entirely.
📝 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.
| close(stream) | |
| break | |
| stream = nil | |
| } else { | |
| common.InternalServerError(w, r, err) | |
| common.InternalServerError(w, r, fmt.Errorf("reading input stream: %w", err)) | |
| return | |
| } | |
| } | |
| if errors.Is(err, io.EOF) { | |
| if stream != nil { // avoid double-close | |
| close(stream) | |
| stream = nil | |
| } | |
| continue // skip to next select/loop iteration | |
| } else { | |
| common.InternalServerError(w, r, fmt.Errorf("reading input stream: %w", err)) | |
| return | |
| } |
🤖 Prompt for AI Agents (early access)
In internal/api/v2/controllers_logs_import.go around lines 41 to 47, the code closes the stream channel every time dec.Decode returns io.EOF, which causes a panic on subsequent EOFs because the channel is already closed or nil. Additionally, the loop continues decoding after EOF, causing a busy-loop. Fix this by adding a guard to only close the stream once and prevent further decoding after EOF by setting a doneDecoding flag or breaking the loop to avoid the busy-loop.
No description provided.