fix: race updating same record on concurrent message stream events#7493
Conversation
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesMessage update concurrency
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
diegolmello
temporarily deployed
to
android_build
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/methods/subscriptions/room.test.ts (1)
318-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly await concurrent
sub.updateMessagecalls rather than relying onsetImmediateflushing.The two
sub.updateMessagecalls are fired withoutawaitor.catch(), creating floating promises. While the mock setup does chain promises correctly (db.write → callback → db.batch → resolution), floating promises should still be handled explicitly. Replace thesetImmediateworkaround withawait Promise.all([...])to clearly express that both calls must complete before the test assertions run—this is cleaner and surfaces any rejections.♻️ Proposed refactor: await both concurrent calls with Promise.all
- // updateMessage's promise never resolves on the happy path, so fire both and flush the queues. - sub.updateMessage({ ...message }); - sub.updateMessage({ ...message }); - await Array.from({ length: 10 }).reduce<Promise<unknown>>( - chain => chain.then(() => new Promise(resolve => setImmediate(resolve))), - Promise.resolve() - ); + await Promise.all([ + sub.updateMessage({ ...message }), + sub.updateMessage({ ...message }), + ]);🤖 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 `@app/lib/methods/subscriptions/room.test.ts` around lines 318 - 324, In the test around the concurrent sub.updateMessage calls, replace the unawaited invocations and setImmediate queue-flushing reduce with Promise.all that awaits both updateMessage promises concurrently. Preserve the existing two-call behavior and ensure any rejection propagates before the assertions execute.Source: Learnings
🤖 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.
Nitpick comments:
In `@app/lib/methods/subscriptions/room.test.ts`:
- Around line 318-324: In the test around the concurrent sub.updateMessage
calls, replace the unawaited invocations and setImmediate queue-flushing reduce
with Promise.all that awaits both updateMessage promises concurrently. Preserve
the existing two-call behavior and ensure any rejection propagates before the
assertions execute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf9ddc42-0957-4993-94ee-ecba312158ce
📒 Files selected for processing (2)
app/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/subscriptions/room.tsapp/lib/methods/subscriptions/room.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript in strict mode, with imports resolved from theapp/baseUrl
Follow the repository's TypeScript/React Native code style enforced by Prettier: tabs, single quotes, 130-character width, no trailing commas, omit arrow-function parens when possible, and keep brackets on the same line
Files:
app/lib/methods/subscriptions/room.tsapp/lib/methods/subscriptions/room.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the project's ESLint setup (
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins) for code quality and style compliance
Files:
app/lib/methods/subscriptions/room.tsapp/lib/methods/subscriptions/room.test.ts
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/subscriptions/room.tsapp/lib/methods/subscriptions/room.test.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/subscriptions/room.test.ts
🔇 Additional comments (2)
app/lib/methods/subscriptions/room.ts (1)
268-284: LGTM!Also applies to: 395-397
app/lib/methods/subscriptions/room.test.ts (1)
4-6: LGTM!Also applies to: 92-92, 104-108
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109345 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTlj4UDTWEAiLHGN2pEz2aVNfDl8wwl-I8ueHdez6gE0PSvcnpClGjLkWk7tv9F6gplR4ORSg_DnAemUVxI |
|
iOS Build Available Rocket.Chat 4.75.0.109347 |
Concurrent stream-room-messages events for the same message id could call prepareUpdate on the same cached WatermelonDB record before either batch committed, throwing 'Cannot update a record with pending changes'. Move the reads, prepares and batch inside a single db.write so the writer lock serializes concurrent updateMessage calls. Also converts updateMessage to async, fixing a happy-path promise that never resolved.
diegolmello
force-pushed
the
knowledgeable-bagel
branch
from
8dcd72b to
cfa6b00
Compare
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
2 participants
Proposed changes
Concurrent
stream-room-messagesevents for the same message id could throwCannot update a record with pending changes (messages#…)/(threads#…).WatermelonDB caches a record instance per id and
prepareUpdatemarks it with a pending state that is only cleared when the batch commits.updateMessagebuilt its batch (callingprepareUpdate) outsidedb.write, andhandleMessageReceivedis a raw stream listener that is not serialized. When two events for the same id arrived close together, both grabbed the same cached record and the secondprepareUpdatethrew. The error was swallowed and logged as a warning, silently dropping that update.The fix moves the reads, prepares and batch inside a single
db.writeso the WatermelonDB writer lock serializes concurrentupdateMessagecalls: the second call waits until the first commits (clearing the pending state) before preparing. Decryption stays outside the lock.updateMessageis also converted to a plainasyncfunction, fixing a happy-path promise that never resolved (the previousnew Promisenever calledresolve()afterdb.write, so the awaiting caller hung and the follow-up read never fired).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1421
How to test or reproduce
Hard to reproduce by hand (a timing race between two stream events for the same message). Covered by an added regression test that fires two concurrent
updateMessagecalls for the same id and asserts no "pending changes" error is logged. The test'sdb.writemock serializes like the real writer lock, so it goes red on the old code and green on the fix.TZ=UTC pnpm test --testPathPattern='subscriptions/room.test'Screenshots
Types of changes
Checklist
Further comments
The same prepare-outside-writer pattern may exist in other
onStreamDatahandlers that callprepare*beforedb.write; not swept in this PR.Summary by CodeRabbit