[AI] Fix batchMessages losing nested batch messages on outer failure#7225
[AI] Fix batchMessages losing nested batch messages on outer failure#7225dearlordylord wants to merge 4 commits intoactualbudget:masterfrom
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
dearlordylord
changed the title
📝 WalkthroughWalkthroughModifies batchMessages to always attempt sending accumulated batched messages even if the wrapped function throws, preserving original error semantics; adds regression tests that validate nested batch messages are not lost when outer batches fail or succeed; adds a release note entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant batchMessages
participant sendMessages
participant DB
Caller->>batchMessages: call with fn()
batchMessages->>batchMessages: collect messages from fn (inner may throw)
alt inner throws or returns
batchMessages->>sendMessages: send accumulated messages
sendMessages->>DB: apply messages (row-a, row-b)
sendMessages-->>batchMessages: send result / error
batchMessages-->>Caller: rethrow original error (if any) or throw send error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/loot-core/src/server/sync/batchMessages.test.ts (1)
52-52: Clarify the purpose ofstepForwardInTimein the test comment.
global.stepForwardInTime()only advancesDate.now()for uniqueTimestamp.send()values—it does not yield the event loop. The actual interleaving is achieved viayieldPromise. The current comment at line 45 ("Yield to event loop - B will run during this await") correctly describes the mechanism, but a brief comment on line 52 would clarify whystepForwardInTimeis needed.📝 Suggestion
+ // Advance clock so inner batch gets a distinct timestamp global.stepForwardInTime();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/sync/batchMessages.test.ts` at line 52, Add a brief inline test comment next to the call to global.stepForwardInTime() explaining that this helper only advances the mocked Date.now() so Timestamp.send() produces unique values and does not yield the event loop; make clear that actual concurrency/interleaving is achieved by yieldPromise (the earlier comment) and that stepForwardInTime is solely to bump timestamps for operations relying on Timestamp.send(). Reference global.stepForwardInTime(), Timestamp.send(), and yieldPromise in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/loot-core/src/server/sync/index.ts`:
- Around line 516-518: The code throws an unknown-typed variable `error` causing
an ESLint no-throw-literal violation; update the throw to use the same type
assertion used for the errorHandler call (e.g., replace `throw error;` with
`throw (error as Error);`) so the thrown value is explicitly an Error and
matches the `void errorHandler(error as Error);` usage.
---
Nitpick comments:
In `@packages/loot-core/src/server/sync/batchMessages.test.ts`:
- Line 52: Add a brief inline test comment next to the call to
global.stepForwardInTime() explaining that this helper only advances the mocked
Date.now() so Timestamp.send() produces unique values and does not yield the
event loop; make clear that actual concurrency/interleaving is achieved by
yieldPromise (the earlier comment) and that stepForwardInTime is solely to bump
timestamps for operations relying on Timestamp.send(). Reference
global.stepForwardInTime(), Timestamp.send(), and yieldPromise in the comment
for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ea945bc-cbb8-4671-9254-fa6f4a05bb6e
📒 Files selected for processing (2)
packages/loot-core/src/server/sync/batchMessages.test.tspackages/loot-core/src/server/sync/index.ts
|
@dearlordylord can you give us an example of a problem this solves in the app? |
It's only a defensive fix - without it, the code would bite whenever someone adds a throw path inside any outer batchMessages. So, no reproducible path in the current code |
3 participants
Description
batchMessagesuses module-level globalsIS_BATCHINGand_BATCHEDshared across async operations. When two async callers interleave (cooperative scheduling via await), a nested batch's messages get silently lost if the outer batch throws - the catch block ran before_sendMessages, discarding everything in_BATCHED.Fix moves
_sendMessagescall outside try/catch so accumulated messages are always sent regardless of whetherfunc()threw.Related issue(s)
None (found via code audit)
Testing
Added
batchMessages.test.tswith two tests:Both tests use promise-based yield control to deterministically reproduce the interleaving.
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/sync/index.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/sync/index.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged