Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Track ALT reservations committor/src/transaction_preparator/cleanup_resources.rs, table-mania/src/manager.rs |
CleanupResources stores reserved ALT keys. Reservation paths record keys reserved during table reuse, creation, extension, and reconciliation. |
Capture resources during preparation committor/src/tasks/*, committor/src/transaction_preparator/*, committor/src/intent_executor/test_utils.rs, committor/src/intent_engine/intent_execution_engine.rs |
Preparation records buffer cleanup tasks, uniqueness nonces, and ALT reservations. Buffer delivery variants no longer include a preparation flag. |
Update recovery and execution cleanup flow committor/src/intent_executor/strategy_executor/*, committor/src/tasks/task_strategist.rs |
Recovery and strategy helpers update strategies or schedule callbacks without returning cleanup-only strategies. The executor registers cleanup records before preparation. |
Release recorded resources committor/src/intent_executor/*, committor/src/transaction_preparator/* |
The report stores cleanup records for preparation attempts. The cleanup handle deduplicates buffer identities and calls the preparator, which releases ALT reservations and conditionally closes buffers. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor
Sequence Diagram(s)
sequenceDiagram
participant TransactionExecutor
participant TransactionPreparator
participant IntentExecutionReport
participant CleanupHandle
TransactionExecutor->>IntentExecutionReport: register CleanupResources
TransactionExecutor->>TransactionPreparator: prepare with CleanupResources
TransactionPreparator->>IntentExecutionReport: record resources acquired during preparation
IntentExecutionReport->>CleanupHandle: pass cleanup records
CleanupHandle->>TransactionPreparator: cleanup_resources
Possibly related issues
magicblock-labs/magicblock-validator#1839— The changes track ALT reservations and release them during cleanup while making buffer closure conditional.
Possibly related PRs
- magicblock-labs/magicblock-validator#1843: This change updates the shared execution flow to pass cleanup records to the cleanup handle.
- magicblock-labs/magicblock-validator#1769: This change modifies the transaction recovery and cleanup flow introduced by that PR.
- magicblock-labs/magicblock-validator#1847: This change updates the recovery and transaction interfaces introduced by that PR.
Suggested labels: techdebt
Suggested reviewers: bmuddha, bzawisto
Merge Risk: 
b4f9c
-
No actionable issue was established in the cleanup-resource change; it is ready to merge after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 32.81% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold. Linked Issues check Issue #1842objectives are mostly implemented.CleanupResources(committor/src/transaction_preparator/cleanup_resources.rs) holds buffer tasks, ALT reservations, and the uniqueness nonce per prepara…Provide the following to decide this objective:
1. Confirm whether the intended rule still requires closing only buffers that completed preparation. The basepreparedgate is the current rule. If the author intends to close partial writes…-
Check name Status Explanation Title check The title clearly and concisely describes the main change: tracking cleanup resources separately from transaction strategies. Description check The description is directly related to the changes. It explains separate cleanup resources, reservation tracking, recovery changes, buffer handling, and retry behavior. Out of Scope Changes check The changed files support #1842. The production changes cover the cleanup record type, cleanup handle, delivery and transaction preparators, recovery and utility helpers,TableManiareservation trac…Full details: Out of Scope Changes check
-
Explanation
The changed files support
#1842. The production changes cover the cleanup record type, cleanup handle, delivery and transaction preparators, recovery and utility helpers,TableManiareservation tracking, thecommit_stage_tasktobuffer_taskrename, and the removal ofreset_commit_idanddummy_revaluate_alts. The test changes adapt cleanup and reservation assertions to the compact records. No unrelated functional change was found. The buffer-closure behavior question is handled under the linked-issue assessment, not as out-of-scope work.
-
Full details: Linked Issues check
-
Explanation
Issue
#1842objectives are mostly implemented.CleanupResources(committor/src/transaction_preparator/cleanup_resources.rs) holds buffer tasks, ALT reservations, and the uniqueness nonce per preparation attempt.IntentExecutionReportkeeps acleanupvector, anddispose/junkare removed.prepare_for_deliveryrecords resources before preparation.CleanupHandle(committor/src/intent_executor/cleanup_handle.rs) deduplicates buffers by(pubkey, commit_id)and keeps every ALT reservation, so buffers close once and reservations release separately.commit_stage_taskis renamed tobuffer_task. Recovery no longer returns dummy strategies. Reservation tracking inTableMania::reserve_pubkeysrecords keys per attempt.One acceptance criterion is not established. The base
CleanupTask::from_commit_finalize(commit_stage_task.rs) closed only deliveries withprepared: true. The head version (buffer_task.rs) closes everyStateInBufferandDiffInBuffertask. Cleanup records are taken before preparation runs, so a failed or partially written attempt can add buffers that were never initialized. These buffers stay inreport.cleanupand are closed on a later successful attempt withclose_buffers = true. The issue requires retaining the existing buffer-closure rules. The PR description says buffer closure follows the current success/failure rules. This change alters the success-path rule. The code and tests do not show whether closing an uninitialized buffer fails, and no test in the reviewed evidence covers success after a failed preparation attempt.Resolution
Provide the following to decide this objective:
- Confirm whether the intended rule still requires closing only buffers that completed preparation. The base
preparedgate is the current rule. If the author intends to close partial writes and failed-attempt buffers on success, state that change against the issue text. - Confirm the behavior of the buffer close instruction on a buffer PDA that was never initialized. State whether it returns an error that affects the intent result.
- Point to a test that runs a failed preparation attempt followed by a successful retry, and asserts which buffers are closed.
- Confirm whether the intended rule still requires closing only buffers that completed preparation. The base
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
cf236e6 to
c9f4e1e
Compare
c9f4e1e to
dfd29ac
Compare
47f5d6a to
b4f9c04
Compare
b4f9c04 to
0e487cb
Compare
Replace retained transaction strategies with compact cleanup records for each preparation attempt. Records contain buffer identities, acquired ALT reservations, and the uniqueness nonce used by that attempt. Keep them in the execution report before entering cancellable preparation futures. Release executable tasks and their account payloads after use. Recovery helpers now update remaining work without cloning tasks or returning dummy strategies for cleanup. Rename commit_stage_task to buffer_task to reflect its buffer preparation and closure role. Record ALT acquisitions inside TableMania as they occur, including partial failure and cancellation. Separate attempts retain separate reservations even when their keys match, so cleanup cannot under-release repeated reservations or release a key that an attempt never acquired. Close each recorded buffer at most once after successful intent execution, using its recorded nonce. Failed execution still releases ALT reservations without closing buffers that another attempt could reuse. Cleanup no longer rescans executable tasks to recover resource identities. Adapt the existing lifecycle, nonce-recovery, and cleanup tests. Extend the recovery regression to exercise real TableMania reference counts across repeated reservations, partial failure, and cancellation while preserving another intent's reservation. Check buffer closure and nonce association through the cleanup handle. Validation: make fmt; make ci-fmt; cargo check for the committor and TableMania packages with --tests; the recovery_preserves_progress_and_resources test; cargo clippy --workspace --all-targets -- -D warnings; cargo test --workspace (nextest is unavailable locally); git diff --check. Closes #1842.
Remove the buffer prepared flags now that cleanup records retain resource identities before preparation begins. Delete PreparationTask::done, its mutable-reference lifetime, and the commit-ID reset wrapper; recovery can update the commit ID directly. Make preparation borrow transaction strategies and tasks immutably while continuing to record cleanup resources separately. Reuse the validated v1 message after delivery preparation instead of assembling it a second time. Remove the redundant explicit drop before the runner replaces its current strategy or returns. Adapt the existing mocks and buffer fixtures to immutable preparation. Preserve real reservation-count, partial-failure, cancellation, buffer-deduplication, and callback recovery coverage. Validation: cargo fmt --check; cargo check -p magicblock-committor-service --tests; the focused recovery_preserves_progress_and_resources test; cargo clippy --workspace --all-targets -- -D warnings; cargo test --workspace (527 passed, none failed or ignored); git diff --check.
0e487cb to
89595b7
Compare
What changed
Cleanup keeps resource identities instead of whole transaction strategies, so finished tasks and their account data can be dropped. Recovery helpers no longer build dummy strategies;
commit_stage_taskbecomesbuffer_task.Closes #1842.
Impact
TableMania records each acquired reservation, including partial failures and cancellation. Cleanup releases repeated reservations separately and closes each buffer once after success. Failed executions keep buffers available for retry.