fix: make import not transactional #893
Conversation
- test: add import test with error
- feat: remove sql transaction on imports
WalkthroughThis set of changes introduces a new advisory locking mechanism for ledger operations, replacing previous table-level locks with PostgreSQL advisory locks. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Store
participant DB
Client->>Controller: LockLedger(ctx)
Controller->>Store: LockLedger(ctx)
alt Using bun.DB
Store->>DB: Acquire connection
Store->>DB: pg_advisory_lock(hash(ledgerID))
Store-->>Controller: (locked Store, DB, release func, error)
else Using bun.Tx
Store->>DB: pg_advisory_xact_lock(hash(ledgerID))
Store-->>Controller: (locked Store, Tx, no-op release, error)
end
Controller-->>Client: (Controller, DB, release func, error)
Note over Client: Use locked controller and DB, then call release func
Possibly related PRs
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
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/storage/ledger/store.go (1)
166-168: Use a 64-bit hash to minimize collision risk
hashtext(text)returns anint4; passing it to the single-arg version of
pg_advisory_lockcasts it tobigint, but the value space is still 32 bit.
With many ledgers the probability of collision grows.
Consider hashing to twoint4parts (hashtextext, hashtextext(…||'salt')) and
calling the two-arg lock, or usehashtextext→bigintvia
hashtext(...)::bigint << 32 | hashtext('…salt…')::bigint.test/e2e/api_ledgers_import_test.go (3)
418-426:pg_lockscount assertion may be flaky on busy CI databasesThe test expects exactly one advisory lock, but other tests running in
parallel (or the driver itself) might hold unrelated advisory locks, causing
sporadic failures. Narrow the query byclassid/objidor bypidto the
current connection instead of relying on a global count.- Where("locktype = 'advisory'"). + Where("locktype = 'advisory'"). + Where("objid = hashtext(?)", fmt.Sprintf("ledger:%d", ledgerID)).
447-459: Same flakiness for “two locks” checkFor the same reason as above, counting all advisory locks can produce >2 rows
even when the expected behaviour is correct. Filter by the ledger hash and use
granted/waitingcolumns to assert the precise state (1 granted, 1 waiting).
155-163: Assertion assumes 2 inserted logs, but hard-codes nothing about input orderIf the import algorithm ever changes to skip invalid entries instead of
aborting, the test will silently pass while the behaviour has changed. Consider
asserting that the IDs present are exactly[0,1]to document the contract
and improve test robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
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_contro 8000 ller_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_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/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 (7)
internal/api/bulking/mocks_ledger_controller_test.go (2)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerController(25-29)LedgerControllerMockRecorder(32-34)internal/controller/ledger/controller.go (1)
Controller(19-80)
internal/controller/ledger/store_generated_test.go (1)
internal/controller/ledger/store.go (1)
Store(31-66)
internal/api/v2/mocks_ledger_controller_test.go (2)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerController(25-29)LedgerControllerMockRecorder(32-34)internal/controller/ledger/controller.go (1)
Controller(19-80)
internal/api/v1/mocks_ledger_controller_test.go (2)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerController(25-29)LedgerControllerMockRecorder(32-34)internal/controller/ledger/controller.go (1)
Controller(19-80)
internal/api/common/mocks_ledger_controller_test.go (2)
internal/api/v2/mocks_ledger_controller_test.go (2)
LedgerController(25-29)LedgerControllerMockRecorder(32-34)internal/controller/ledger/controller.go (1)
Controller(19-80)
internal/storage/ledger/store.go (1)
internal/controller/ledger/store.go (1)
Store(31-66)
internal/controller/ledger/controller_default.go (2)
internal/controller/ledger/controller.go (1)
Controller(19-80)internal/controller/ledger/errors.go (1)
NewErrImport(33-37)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (18)
internal/controller/ledger/controller.go (1)
23-23: Looks good - API extension with proper cleanup mechanism.The addition of
LockLedgermethod to theControllerinterface with the return signature(Controller, bun.IDB, func() error, error)is a well-designed improvement. This pattern provides a clean way to acquire a ledger lock and necessary resources while ensuring proper cleanup through the returned function.internal/controller/ledger/store.go (1)
53-53: Improved locking mechanism with explicit resource management.The enhanced
LockLedgersignature aligns with the Controller interface change and provides a more robust approach to ledger locking by returning the locked store, database interface, cleanup function, and error status. This change supports the goal of making imports non-transactional while maintaining data integrity.internal/controller/ledger/store_generated_test.go (1)
224-231: Correctly updated mock to match the new interface signature.The mock implementation properly reflects the updated
LockLedgermethod signature, correctly extracting and returning the four expected values: Store, bun.IDB, cleanup function, and error. This ensures tests using this mock will work correctly with the new locking mechanism.test/e2e/app_lifecycle_test.go (3)
97-97: Comment updated to reflect the new advisory locking mechanism.The comment has been correctly updated to reference advisory locks instead of table locks, which aligns with the architectural change in the PR.
131-133: Test verification query updated to check advisory locks.The test has been appropriately modified to check
pg_lockstable with a filter for advisory locks instead of the previous approach that looked at non-idle queries related to account insertions.
139-140: Updated lock count expectations with clear rationale.The expectations and comments have been updated to account for the new advisory locking strategy. The explanation about having
countTransactions+1locks (with the +1 for logs sync hashing) is clear and helpful for future maintenance.internal/api/bulking/mocks_ledger_controller_test.go (1)
332-347: Well-implemented mock for the new LockLedger method.The implementation correctly mirrors the Controller interface's new LockLedger method that supports the new PostgreSQL advisory locking mechanism. The mock properly returns the expected values (controller, database interface, cleanup function, and error) consistent with the interface definition in
controller.go.internal/api/v1/mocks_ledger_controller_test.go (1)
332-347: Correctly implemented mock for the LockLedger method.The mock implementation appropriately matches the Controller interface's LockLedger method signature, returning the four required components: controller, database interface, cleanup function, and error. This supports the new advisory locking mechanism for non-transactional imports.
internal/controller/ledger/controller_generated_test.go (1)
331-347: Proper implementation of the MockController's LockLedger method.The mock method implementation correctly aligns with the Controller interface definition, properly handling type assertions for the return values. This will properly support testing the advisory locking mechanism that replaces the previous table-level locks.
internal/api/common/mocks_ledger_controller_test.go (1)
332-347: Well-structured mock implementation for LockLedger.The implementation correctly matches the Controller interface signature, properly handling the four return values required for the advisory locking mechanism. This mock will allow effective testing of the new non-transactional approach to imports.
internal/controller/ledger/controller_with_traces.go (3)
43-43: LGTM: New histogram field added for lock ledger metricsThe addition of the
lockLedgerHistogramfield to theControllerWithTracesstruct is consistent with the existing pattern for other operations in the controller.
141-144: LGTM: Proper initialization of the new histogramThe metric initialization follows the same pattern as other metrics in this file with proper error handling.
464-486: LGTM: Well-implemented method for tracing ledger locksThe
LockLedgerimplementation correctly traces the operation, records metrics, and propagates the return values from the underlying controller. It follows the same pattern as other traced methods in this controller wrapper.internal/storage/ledger/adapters.go (1)
39-47: LGTM: Proper adapter method implementationThe
LockLedgermethod correctly delegates to the underlying store and wraps the returned store in a new adapter instance. This maintains the adapter pattern consistently throughout the codebase.internal/api/v2/mocks_ledger_controller_test.go (1)
332-347: LGTM: Mock implementation added for new controller methodThe mock implementation of
LockLedgerfollows the standard gomock pattern used throughout the codebase. It correctly handles the return types (Controller, bun.IDB, func() error, error) and provides a method to record expected calls.internal/controller/ledger/controller_default.go (3)
82-95: LGTM: Clean implementation of ledger lockingThe
LockLedgerimplementation correctly copies the controller, delegates to the store's locking mechanism, and returns the appropriate values. The approach is consistent with other similar methods likeBeginTX.
203-203: Improvement: Better error handling with specialized error typeGood change to use
NewErrImportinstead of a generic error. This allows callers to check for specific error types and provides more context about the nature of the failure.
205-205: Bug fix: Correctly updating lastLogID trackingThis change fixes an important bug where the
lastLogIDwas not being properly tracked during import. Now it correctly updates to the current log's ID after validation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #893 +/- ##
==========================================
- Coverage 82.27% 81.97% -0.30%
==========================================
Files 141 141
Lines 7640 7746 +106
==========================================
+ Hits 6286 6350 +64
- Misses 1039 1070 +31
- Partials 315 326 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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: []
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/controller/system/state_tracker.go (1)
159-176: Well-implemented centralized lock handlingThe new
withLockhelper function centralizes ledger locking logic and properly:
- Acquires the lock via
LockLedger- Handles lock acquisition errors
- Ensures lock release even in error cases
- Logs and records telemetry for lock release failures
- Correctly passes the locked controller to the callback function
This correctly fixes the issue mentioned in a previous review comment about the locked controller being discarded.
🧹 Nitpick comments (2)
internal/controller/system/state_tracker.go (2)
40-40: Consider creating a technical debt issue for this TODOThis TODO comment indicates that some code should be removed in a later version. Consider creating a technical debt issue to ensure this doesn't get forgotten.
19-51: Consider removing transaction handling from handleState in the futureWhile the
Importmethod has been updated to be non-transactional, thehandleStatemethod still uses both transactions and the new locking mechanism. Consider whether you should fully migrate to the advisory locking approach for consistency, or document why the hybrid approach is needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/ledger/controller_with_events.go(1 hunks)internal/controller/ledger/controller_with_traces.go(3 hunks)internal/controller/system/state_tracker.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/ledger/controller_with_traces.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 (2)
internal/controller/system/state_tracker.go (2)
32-34: Updated lock handling approachThe refactoring now uses the new
withLockhelper function to handle ledger locking in a centralized way. This makes the code more maintainable and ensures consistent lock handling across operations.
133-147: Import now uses advisory locking instead of transactionsThe
Importmethod has been refactored to use advisory locking instead of SQL transactions, aligning with the PR objective to make imports non-transactional. The code now:
- Acquires a lock using the
withLockhelper- Reloads the ledger state after acquiring the lock
- Verifies the ledger is still in the initializing state
- Calls the underlying import functionality
This implementation correctly ensures ledger state consistency without requiring a single transaction for the entire import process.
4c1c0b6 to
43a45d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/api/v2/controllers_logs_import.go (2)
28-29: Close the request body as early as possible.
http.Serverwill eventually closer.Body, but adding an explicit
defer r.Body.Close()documents intent and releases the socket immediately
when the handler finishes.dec := json.NewDecoder(r.Body) + defer r.Body.Close()
40-45: Avoid continuing the loop afterio.EOFto spare one needless select round.Once the decoder hits
io.EOFthe channel has been closed and no further
message can be produced, so jumping straight to the select adds a tiny bit of
unnecessary work and complicates reasoning about the value ofl.- if errors.Is(err, io.EOF) { - close(stream) - stream = nil - } else { + if errors.Is(err, io.EOF) { + close(stream) + // Nothing left to decode, wait for import result. + // Jump directly to the select below. + } else { common.InternalServerError(w, r, fmt.Errorf("reading input stream: %w", err)) return }
📜 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 (20)
internal/api/bulking/mocks_ledger_controller_test.go(1 hunks)internal/api/common/middleware_resolver.go(2 hunks)internal/api/common/mocks_ledger_controller_test.go(1 hunks)internal/api/v1/mocks_ledger_controller_test.go(1 hunks)internal/api/v2/controllers_logs_import.go(3 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/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)
- internal/api/common/middleware_resolver.go
🚧 Files skipped from review as they are similar to previous changes (18)
- internal/controller/ledger/store.go
- test/e2e/app_lifecycle_test.go
- internal/api/v1/mocks_ledger_controller_test.go
- internal/controller/ledger/controller_with_too_many_client_handling.go
- internal/controller/ledger/controller_with_events.go
- internal/controller/ledger/controller_with_cache.go
- internal/controller/ledger/controller.go
- internal/controller/ledger/controller_with_traces.go
- internal/controller/ledger/controller_default.go
- test/e2e/api_ledgers_import_test.go
- internal/ 8000 api/v2/mocks_ledger_controller_test.go
- internal/controller/ledger/controller_generated_test.go
- internal/api/bulking/mocks_ledger_controller_test.go
- internal/storage/ledger/adapters.go
- internal/api/common/mocks_ledger_controller_test.go
- internal/controller/ledger/store_generated_test.go
- internal/controller/system/state_tracker.go
- internal/storage/ledger/store.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/api/v2/controllers_logs_import.go (1)
internal/api/common/errors.go (1)
InternalServerError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (1)
internal/api/v2/controllers_logs_import.go (1)
22-27: 🛠️ Refactor suggestionEnsure the goroutine cannot leak by closing
errChan.
errChanis created with a buffer of one, so the main loop will read at most a single value.
If, for any unforeseen reason, the main handler returns before reading from the
channel (e.g. an early-return path is added later), the goroutine will block
forever onerrChan <- err.
Closing the channel after the send protects against that class of leak.go func() { err := common.LedgerFromContext(r.Context()).Import(r.Context(), stream) if err != nil { err = fmt.Errorf("importing logs: %w", err) } - errChan <- err + errChan <- err + close(errChan) }()Likely an incorrect or invalid review comment.
| if err != nil { | ||
| handleError(err) | ||
| return | ||
| } | ||
| if stream != nil { | ||
| panic("got nil error while not at the end of the stream") | ||
| } |
There was a problem hiding this comment.
Replace server-crashing panic with graceful error handling.
A programming error here would currently bring down the entire server.
Return a 500 and log the inconsistency instead of panicking.
- if stream != nil {
- panic("got nil error while not at the end of the stream")
- }
+ if stream != nil {
+ common.InternalServerError(
+ w, r,
+ fmt.Errorf("no error from import while stream still open – possible logic bug"),
+ )
+ return
+ }📝 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.
| if err != nil { | |
| handleError(err) | |
| return | |
| } | |
| if stream != nil { | |
| panic("got nil error while not at the end of the stream") | |
| } | |
| if err != nil { | |
| handleError(err) | |
| return | |
| } | |
| if stream != nil { | |
| common.InternalServerError( | |
| w, r, | |
| fmt.Errorf("no error from import while stream still open – possible logic bug"), | |
| ) | |
| return | |
| } |
🤖 Prompt for AI Agents (early access)
In internal/api/v2/controllers_logs_import.go around lines 55 to 61, replace the panic call that triggers when stream is not nil but err is nil with graceful error handling. Instead of panicking, log the inconsistency and return a 500 Internal Server Error response to avoid crashing the server. Ensure the error is properly logged and the client receives an appropriate error status.