fix: drain commit batch when an action throws to avoid InnerCommit hang#120
Conversation
If a single action's Parse or HandleAction throws, the foreach exited before enqueueing callbacks for the remaining batch members. Their Ready callbacks never fired, hanging InnerCommit's wait loop. Now per-action failures are caught and recorded; every action gets a callback, and updateState is skipped if any action threw.
📝 WalkthroughWalkthroughThe commit handler now catches failures for individual actions, marks their callbacks as failed, continues processing the batch, and skips state updates when an action failed. State-update exceptions are propagated to callbacks. The changelog records this fix. ChangesCommit batch error handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
JsonFlatFileDataStore/CommitActionHandler.cs (1)
44-83: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winIsolate per-action exceptions and avoid aborting the entire batch on a single failure.
The current implementation shares a single
actionExceptionacross the entire batch and skipsupdateStateif any action fails. Because batches group independent, concurrent caller requests as an I/O optimization, this causes two critical issues:
- Exception leakage: The shared
actionExceptionis overwritten if multiple actions fail, and the final exception is passed to all callers in the batch.- Collateral damage: A single failing action (e.g., a malformed update) aborts the entire batch, causing unrelated concurrent operations to fail and lose their changes.
To fix this, track exceptions per-action using a parallel queue, and only skip the final
Readysuccess state if a batch-level I/O error occurs inupdateState.🐛 Proposed fix
- Exception actionException = null; + var actionExceptions = new Queue<Exception>(); foreach (var action in batch) { try { var (actionSuccess, updatedJson) = action.HandleAction(JObject.Parse(jsonText)); callbacks.Enqueue((action, actionSuccess)); + actionExceptions.Enqueue(null); if (actionSuccess) jsonText = updatedJson; } catch (Exception e) { - // Record the failure but keep draining the batch so every caller's Ready - // callback fires — otherwise InnerCommit's wait loop would hang forever. - actionException = e; callbacks.Enqueue((action, false)); + actionExceptions.Enqueue(e); } } var updateSuccess = false; + Exception batchException = null; - if (actionException == null) - { - try - { - updateSuccess = updateState(jsonText); - } - catch (Exception e) - { - actionException = e; - } - } + try + { + updateSuccess = updateState(jsonText); + } + catch (Exception e) + { + batchException = e; + } foreach (var (cbAction, cbSuccess) in callbacks) { - cbAction.Ready(updateSuccess ? cbSuccess : false, actionException); + var actionEx = actionExceptions.Dequeue(); + cbAction.Ready(batchException != null ? false : (updateSuccess ? cbSuccess : false), batchException ?? actionEx); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore/CommitActionHandler.cs` around lines 44 - 83, Update the batch processing around the action loop and final Ready callbacks to track each action’s exception alongside its callback result, rather than sharing actionException across the batch. Continue processing all actions and call updateState with successful updates even when an individual action fails; only a batch-level updateState exception should force every Ready callback to report failure. Pass each action’s own exception to its corresponding cbAction.Ready call, preserving unrelated actions’ results.
🧹 Nitpick comments (1)
JsonFlatFileDataStore/CommitActionHandler.cs (1)
50-50: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid redundant JSON parsing in the batch loop.
Currently,
HandleActionreturns a serialized JSON string, forcingJObject.Parse(jsonText)to be called up to 50 times per batch. For large JSON files, this O(N) redundant parsing degrades throughput.Consider refactoring the
DataStore.CommitAction.HandleActiondelegate to return the mutatedJObjectdirectly instead of a string, so the parsed object can be passed to subsequent actions in the batch without re-parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@JsonFlatFileDataStore/CommitActionHandler.cs` at line 50, Refactor the CommitAction.HandleAction delegate and its implementations to return the mutated JObject rather than a serialized string. In CommitActionHandler’s batch loop, parse jsonText once, pass the resulting object through successive HandleAction calls, and serialize only when the final result must be persisted, preserving the existing actionSuccess behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@JsonFlatFileDataStore/CommitActionHandler.cs`:
- Around line 44-83: Update the batch processing around the action loop and
final Ready callbacks to track each action’s exception alongside its callback
result, rather than sharing actionException across the batch. Continue
processing all actions and call updateState with successful updates even when an
individual action fails; only a batch-level updateState exception should force
every Ready callback to report failure. Pass each action’s own exception to its
corresponding cbAction.Ready call, preserving unrelated actions’ results.
---
Nitpick comments:
In `@JsonFlatFileDataStore/CommitActionHandler.cs`:
- Line 50: Refactor the CommitAction.HandleAction delegate and its
implementations to return the mutated JObject rather than a serialized string.
In CommitActionHandler’s batch loop, parse jsonText once, pass the resulting
object through successive HandleAction calls, and serialize only when the final
result must be persisted, preserving the existing actionSuccess behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ec963e3-f828-42ce-98b5-a2390258b1d9
📒 Files selected for processing (2)
CHANGELOG.mdJsonFlatFileDataStore/CommitActionHandler.cs
1 participant
If a single action's Parse or HandleAction throws, the foreach exited before enqueueing callbacks for the remaining batch members. Their Ready callbacks never fired, hanging InnerCommit's wait loop.
Now per-action failures are caught and recorded; every action gets a callback, and updateState is skipped if any action threw.
Summary by CodeRabbit