fix: prepare subscription updates inside the writer lock - #7544
fix: prepare subscription updates inside the writer lock#7544diegolmello wants to merge 1 commit into
Conversation
createOrUpdateSubscription and decryptPendingSubscriptions called prepareUpdate outside db.write and committed the batch later. A concurrent updateLastOpen took the writer lock in that gap, called update() on the same cached subscription record, and threw "Cannot update a record with pending changes". Both paths now prepare and batch inside one db.write, as room.ts already does.
GitHub Actions
Inactive
WalkthroughChangesThe database write paths now prepare subscription records and batch updates while holding the writer lock. Pending subscription decryption occurs before the lock. A concurrency test verifies that Subscription write concurrency
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant createOrUpdateSubscription
participant db.write
participant subscriptionRecord
participant db.batch
createOrUpdateSubscription->>db.write: prepare subscription and message changes
db.write->>subscriptionRecord: prepareUpdate
subscriptionRecord-->>db.write: prepared changes
db.write->>db.batch: commit prepared changes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
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 |
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/lib/methods/subscriptions/rooms.ts (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit return type to the newly exported function.
createOrUpdateSubscriptionis now part of the module's public surface. The coding guidelines require explicit return type annotations for TypeScript functions.♻️ Proposed change
-export const createOrUpdateSubscription = async (subscription: ISubscription, room: IServerRoom | IRoom) => { +export const createOrUpdateSubscription = async (subscription: ISubscription, room: IServerRoom | IRoom): Promise<void> => {🤖 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/rooms.ts` at line 49, Add an explicit Promise-based return type annotation to the exported createOrUpdateSubscription function, using the actual value it resolves to and preserving its existing behavior.Source: Coding guidelines
app/lib/encryption/encryption.ts (1)
398-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive name and explicit callback return types.
newSubdoes not identify the value as decrypted subscription data. Rename it todecryptedSubscription. Add explicit return types to the async mapper and thedb.writecallback.As per coding guidelines, use descriptive names and explicit TypeScript parameter and return types.
🤖 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/encryption/encryption.ts` around lines 398 - 403, Update the Promise.all mapper in decryptSubscription to rename newSub to decryptedSubscription and add an explicit callback return type. Also add an explicit parameter type and return type to the associated db.write callback, preserving the existing decrypted subscription behavior.Source: Coding guidelines
🤖 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.
Inline comments:
In `@app/lib/encryption/encryption.ts`:
- Around line 398-417: Update the subscription flow around decryptSubscription
and prepareUpdate to prevent stale decrypted snapshots from overwriting newer
lastMessage values: capture the source last-message identity or timestamp before
decryption, validate it against the current record inside prepareUpdate, and
when it differs re-read and decrypt the current subscription or skip it and
schedule a retry. Add a regression test covering a newer lastMessage arriving
while decryption is delayed.
In `@app/lib/methods/subscriptions/rooms.test.ts`:
- Around line 153-173: The concurrency test should force updateLastOpen to hold
the writer lock while createOrUpdateSubscription completes its preparation. In
the test around createOrUpdateSubscription and updateLastOpen, delay the
getSubscriptionByRoomId mock until the intended ordering is established, and
retain a positive assertion that mockDbBatch was called to verify the writer
path executed.
---
Nitpick comments:
In `@app/lib/encryption/encryption.ts`:
- Around line 398-403: Update the Promise.all mapper in decryptSubscription to
rename newSub to decryptedSubscription and add an explicit callback return type.
Also add an explicit parameter type and return type to the associated db.write
callback, preserving the existing decrypted subscription behavior.
In `@app/lib/methods/subscriptions/rooms.ts`:
- Line 49: Add an explicit Promise-based return type annotation to the exported
createOrUpdateSubscription function, using the actual value it resolves to and
preserving its existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7c1edd0-bd86-4ef2-8858-d0c57a345ac9
📒 Files selected for processing (3)
app/lib/encryption/encryption.tsapp/lib/methods/subscriptions/rooms.test.tsapp/lib/methods/subscriptions/rooms.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 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/rooms.test.tsapp/lib/encryption/encryption.tsapp/lib/methods/subscriptions/rooms.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
Files:
app/lib/methods/subscriptions/rooms.test.tsapp/lib/encryption/encryption.tsapp/lib/methods/subscriptions/rooms.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/lib/methods/subscriptions/rooms.test.tsapp/lib/encryption/encryption.tsapp/lib/methods/subscriptions/rooms.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/rooms.test.tsapp/lib/encryption/encryption.tsapp/lib/methods/subscriptions/rooms.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/rooms.test.ts
🔇 Additional comments (4)
app/lib/methods/subscriptions/rooms.ts (1)
154-225: LGTM!app/lib/methods/subscriptions/rooms.test.ts (2)
82-112: LGTM!
116-138: LGTM!app/lib/encryption/encryption.ts (1)
1-1: LGTM!
| const decryptedSubscriptions = await Promise.all( | ||
| subsEncryptedToDecrypt.map(async (sub: TSubscriptionModel) => ({ | ||
| sub, | ||
| newSub: await this.decryptSubscription(sub) | ||
| })) | ||
| ); | ||
|
|
||
| // Prepare and batch under the writer lock so a concurrent writer can't | ||
| // call prepareUpdate on a record with pending changes. | ||
| await db.write(async () => { | ||
| await db.batch(preparedSubscriptions.filter((record): record is Model => record !== null)); | ||
| const preparedSubscriptions = decryptedSubscriptions | ||
| .map(({ sub, newSub }) => { | ||
| try { | ||
| return sub.prepareUpdate( | ||
| protectedFunction((m: TSubscriptionModel) => { | ||
| if (newSub?.lastMessage) { | ||
| m.lastMessage = newSub.lastMessage; | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
mapfile -t files < <(
fd -t f -e ts -e tsx . app |
rg '/(encryption|room|rooms(\.test)?)\.(ts|tsx)$'
)
if ((${`#files`[@]} > 0)); then
rg -n -C 8 \
'decryptPendingSubscriptions|decryptSubscription|lastMessage|createOrUpdateSubscription|updateLastOpen|prepareUpdate|db\.write|db\.batch' \
"${files[@]}"
fiRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 224
🏁 Script executed:
#!/usr/bin/env/bash
set -euo pipefail
printf 'Files matching path:\n'
fd -t f -e ts -e tsx '^(encryption|room|rooms(\.test)?)$' app || true
printf '\nTracked files around encryption/room:\n'
git ls-files app | rg 'app/(lib/encryption/(encryption\.ts|.*test.*)|.*room.*\.ts|.*rooms.*\.ts)$' || true
printf '\nSearch relevant symbols:\n'
rg -n -C 6 \
'decryptPendingSubscriptions|decryptSubscription|lastMessage|createOrUpdateSubscription|updateLastOpen|prepareUpdate|db\.write|db\.batch' app || trueRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 50391
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'lock file locations:\n'
git ls-files | rg '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock)$' || true
printf '\nWatermelonDB version declarations:\n'
for f in $(git ls-files | rg '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock)$' | head -20); do
echo "--- $f"
rg -n -C 3 '"watermelondb"|watermelondb@|/watermelondb/' "$f" | head -80 || true
done
printf '\nencryption.ts relevant functions:\n'
sed -n '60,120p' app/lib/encryption/encryption.ts
sed -n '380,430p' app/lib/encryption/encryption.ts
printf '\nsubscription model and updates near lastMessage:\n'
git ls-files | rg 'app/.+_subscription.*\.ts$|app/.*subscription.*\.ts$|app/.*model.*\.ts$' | head -50
rg -n -C 8 'class .*Subscription|model.*subscription|lastMessage|updateLastOpen|createOrUpdateSubscription|prepareUpdate' app/lib app/sagas/rooms.js app/actions/room.ts app/actions/rooms.ts | head -220Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 18472
🌐 Web query:
WatermelonDB v0.28.1 db.write prepareUpdate concurrent writes lock writer callbacks
💡 Result:
In WatermelonDB, database writes are strictly serialized to ensure data integrity [1][2]. The library employs a writer-locking mechanism where only one writer (an action defined via database.write() or a @writer method) can execute at any given time [1][2]. Other incoming write requests are queued until the active writer completes [3]. Regarding prepareUpdate and concurrent writes, note the following critical technical constraints: 1. Writer Exclusivity: Because only one writer can be active at once, concurrent write conflicts are prevented at the library level [1][2]. You do not need to implement manual locks for database consistency, as the queueing system manages this automatically [3][2]. 2. Synchronous Batching Requirement: The method prepareUpdate (along with prepareCreate, prepareMarkAsDeleted, etc.) must be executed and passed to database.batch() synchronously within the same writer context [1][4][5]. Performing asynchronous operations (like await) between preparing an update and executing it in batch() can lead to errors such as: "record.prepareUpdate was called on... but wasn't sent to batch synchronously -- this is bad!" [4][6]. 3. Correct Pattern: All logically connected read and write operations should be wrapped together inside a single database.write() block or an @writer method [1][2]. Any necessary asynchronous data fetching or calculations should be completed before preparing the operations, or the batching must occur within the same asynchronous writer block where the preparations are made [4][5]. If you need to nest writer operations, you must use callWriter to ensure the nested operation runs within the existing writer's exclusive lock [1][3]. Nesting writers without callWriter will result in errors or deadlocks because the system expects only one active writer [1].
Citations:
- 1: https://watermelondb.dev/docs/Writers
- 2: [QUESTION] Ways of running queries Nozbe/WatermelonDB#750
- 3: Diagnostic error: callReader/callWriter call must call a reader/writer synchronously Nozbe/WatermelonDB#1344
- 4: https://stackoverflow.com/questions/73234694/error-record-prepareupdate-was-called-on-this-tablethis-id-but-wasnt-se
- 5: Create or update record (batch) Nozbe/WatermelonDB#252
- 6: Error:
record.prepareUpdate was called on ${this.table}#${this.id} but wasn't sent to batch() synchronously -- this is bad!Nozbe/WatermelonDB#1368
🌐 Web query:
WatermelonDB v0.28.1 Source Database/index.js writer lock prepareUpdate callback
💡 Result:
In WatermelonDB, the prepareUpdate method is used to prepare an update operation on a model record, but it does not execute the update immediately [1]. To ensure consistency and performance, WatermelonDB requires that these prepared operations are executed synchronously within a single database batch transaction [2][1]. If you receive an error stating that record.prepareUpdate was called but was not sent to batch synchronously, it indicates that the prepared update was created inside a writer but not passed to database.batch within the same execution context [2]. Key points regarding the writer lock and prepareUpdate: 1. Writer Locks: WatermelonDB enforces that all database modifications must occur within a Writer [3][4]. Only one Writer can execute at a time to ensure a consistent view of the database [3]. 2. Synchronous Requirement: When you use methods like prepareUpdate or prepareCreate, you are creating a "prepared" record [1]. These must be passed to database.batch or writer.batch synchronously [2][1]. 3. Correct Usage: You should perform your prepare calls inside the same writer block where you call the batch function [2]. Example of correct usage: await database.write(async => { const record = await collection.find('id'); const preparedUpdate = record.prepareUpdate(model => { model.field = 'new value'; }); await database.batch(preparedUpdate); }); If you perform asynchronous operations (e.g., fetching other data) between preparing an update and batching it, you risk breaking the synchronous requirement [2]. All necessary data should be prepared or fetched before the final batch execution [5].
Citations:
- 1: https://github.com/Nozbe/WatermelonDB/blob/master/src/Model/index.d.ts
- 2: https://stackoverflow.com/questions/73234694/error-record-prepareupdate-was-called-on-this-tablethis-id-but-wasnt-se
- 3: https://watermelondb.dev/docs/Writers
- 4: https://watermelondb.dev/docs/CRUD
- 5: bug: Performance issue in getUsersPresence due to unbatched WatermelonDB writes and race condition #7032
🌐 Web query:
@nozbe/watermelondb 0.28.1-0 GitHub source write writer lock pendingQueue changeRecords
💡 Result:
The version 0.28.1-0 of @nozbe/watermelondb was released on July 24, 2025 [1][2]. This release is primarily a version bump [1] and does not contain specific changes to the underlying architecture of writer locks, the pending queue, or changeRecords handling. In WatermelonDB, the core concurrency model remains unchanged in this version: 1. Writer Locks and PendingQueue: WatermelonDB enforces a strict concurrency model where only one writer can execute at a time [3]. When multiple write operations are initiated, they are placed in a pending queue [4]. If a writer is already active, subsequent writers are queued [4]. A common diagnostic warning occurs when many writers are queued, which can occur if operations are not batched efficiently [4][5]. 2. ChangeRecords: The concept of changeRecords is intrinsic to WatermelonDB's reactivity and synchronization system, where database mutations (create, update, delete) are tracked to propagate changes to observers and the synchronization engine [6]. 3. Development Recommendations: To avoid issues with writer locks and pending queues, the official documentation recommends batching database writes using database.batch rather than executing many individual writers [3][5]. For nested operations, the library requires the use of callWriter or callReader to manage context correctly within existing transactions [3][4]. For detailed implementation logic or migration guides, developers should refer to the standard documentation for the 0.28.x release series, as no breaking structural changes were introduced in 0.28.1-0 [7][8].
Citations:
- 1: Nozbe/WatermelonDB@f2a29e7
- 2: https://npmx.dev/package/@nozbe/watermelondb/v/0.28.1-0
- 3: https://watermelondb.dev/docs/Writers
- 4: Diagnostic error: callReader/callWriter call must call a reader/writer synchronously Nozbe/WatermelonDB#1344
- 5: Writing big query - more than 20 000 writers in queue at once Nozbe/WatermelonDB#1390
- 6: https://watermelondb.dev/docs/Implementation/SyncImpl
- 7: https://github.com/Nozbe/WatermelonDB/blob/HEAD/CHANGELOG.md
- 8: https://github.com/Nozbe/WatermelonDB/blob/master/CHANGELOG.md
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Find lock-creation/update paths targeting encrypted subscription lastMessage:\n'
rg -n -C 5 \
'lastMessage|lastMessageTs|updateLastOpen|createOrUpdateSubscription|prepareCreate|Object\.assign|subscription\.lastMessage|e2e_key_id|encrypted' \
app/lib app/sagas/rooms.js app/actions/room.ts app/actions/rooms.ts \
| rg -C 4 'lastMessage|updateLastOpen|createOrUpdateSubscription|Object\.assign|prepareCreate|e2e_key_id|encrypted' \
| head -280
printf '\nList subscription-related tracked files:\n'
git ls-files app | rg 'subscription|room|rooms|subscriptions' | head -120Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 16619
🌐 Web query:
"Nozbe/WatermelonDB" "v0.28.1-0" "src/Database/index.js" "writer" "pendingQueue"
💡 Result:
In WatermelonDB, including version 0.28.1-0, database operations are managed by a queuing system to ensure data consistency [1][2]. The code mentioned in src/Database/index.js interacts with an ActionQueue (or WorkQueue in later versions) which handles the scheduling of readers and writers [1][3]. The pending queue system functions as follows: 1. Consistency and Locking: WatermelonDB enforces that only one writer can execute at a time [2]. All write operations (create, update, delete) must occur within a writer block [1][2]. When a write operation is initiated, it is added to a pending queue if another action is currently running [1][2]. 2. Handling Queued Actions: If multiple write operations are triggered simultaneously, they are placed in a queue to be executed sequentially [4]. The library provides warnings if a large number of actions accumulate in this queue, as this may indicate that previous actions are stuck or that the application is inefficiently dispatching individual writes instead of using batching [5][4]. 3. Nesting: If you need to perform a write operation from within another writer, you must use the callWriter method [2]. Direct execution of a nested writer will fail because the database lock is already held by the parent writer [2]. 4. Internal Mechanics: Internally, the Database class maintains an instance of the queue (often referenced as _actionQueue or _workQueue in the source) [1][3]. Methods like unsafeResetDatabase specifically trigger an abort of all pending actions to ensure a clean state during reset operations [1]. If you are encountering issues related to a large number of writers in the queue, the recommended practice is to aggregate changes into a single batch operation using db.batch instead of executing multiple individual writes [4][2].
Citations:
- 1: https://github.com/Nozbe/WatermelonDB/blob/22188ee5b6e3af08e48e8af52d14e0d90db72925/src/Database/index.js
- 2: https://watermelondb.dev/docs/Writers
- 3: Readers&Writers Nozbe/WatermelonDB#1031
- 4: Writing big query - more than 20 000 writers in queue at once Nozbe/WatermelonDB#1390
- 5: Diagnostic error: callReader/callWriter call must call a reader/writer synchronously Nozbe/WatermelonDB#1344
Guard against stale decrypted subscription snapshots.
decryptSubscription(lastMessage) runs before db.write. If another writer changes the same subscription between decryption and prepareUpdate, this callback can overwrite the newer lastMessage with the older decrypted value.
Capture the source last-message identity or timestamp before decryption. Validate it against the current record inside prepareUpdate; if it changed, re-read and decrypt the current record or skip and schedule a retry. Add a regression test for a newer lastMessage arriving during delayed subscription decryption.
🤖 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/encryption/encryption.ts` around lines 398 - 417, Update the
subscription flow around decryptSubscription and prepareUpdate to prevent stale
decrypted snapshots from overwriting newer lastMessage values: capture the
source last-message identity or timestamp before decryption, validate it against
the current record inside prepareUpdate, and when it differs re-read and decrypt
the current subscription or skip it and schedule a retry. Add a regression test
covering a newer lastMessage arriving while decryption is delayed.
| it('does not leave a prepared subscription visible to a concurrent updateLastOpen', async () => { | ||
| const record = makeSubscriptionRecord(); | ||
| (getSubscriptionByRoomId as jest.Mock).mockResolvedValue(record); | ||
| // Slow message lookup keeps createOrUpdateSubscription busy after it fetched the subscription. | ||
| (getMessageById as jest.Mock).mockImplementation(() => new Promise(resolve => setTimeout(() => resolve(null), 10))); | ||
|
|
||
| const subscription = { | ||
| rid, | ||
| _id: rid, | ||
| lastMessage: { _id: 'msg-id', rid, msg: 'hi' } | ||
| } as any; | ||
|
|
||
| await Promise.all([ | ||
| createOrUpdateSubscription(subscription, undefined as any), | ||
| updateLastOpen(rid, [{ _updatedAt: '2026-01-01T12:00:00.000Z' }]) | ||
| ]); | ||
|
|
||
| const loggedPendingChanges = (log as jest.Mock).mock.calls.some(([error]) => /pending changes/.test(error?.message)); | ||
| expect(loggedPendingChanges).toBe(false); | ||
| expect(record.lastOpen).toEqual(new Date('2026-01-01T12:00:00.000Z')); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the new test fails on the pre-fix implementation of createOrUpdateSubscription.
set -euo pipefail
fd -t f 'rooms.ts' app/lib/methods/subscriptions --exec sh -c '
echo "=== $1 (current) ==="
rg -n "db.write|prepareUpdate|prepareCreate|db.batch|getSubscriptionByRoomId|getMessageById" "$1"
' _ {}
echo "=== previous revision of rooms.ts ==="
git log --oneline -3 -- app/lib/methods/subscriptions/rooms.ts
git show HEAD~1:app/lib/methods/subscriptions/rooms.ts 2>/dev/null \
| rg -n "db.write|prepareUpdate|prepareCreate|db.batch|getSubscriptionByRoomId|getMessageById" || echo "previous revision not available"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== current implementation ==="
sed -n '150,228p' app/lib/methods/subscriptions/rooms.ts
echo "=== previous implementation ==="
git show HEAD~1:app/lib/methods/subscriptions/rooms.ts | sed -n '150,228p'
echo "=== test ==="
sed -n '130,180p' app/lib/methods/subscriptions/rooms.test.ts
echo "=== mockDbBatch / db.write definitions in test file ==="
rg -n "mockDbBatch|db\.write|createOrUpdateSubscription|updateLastOpen" app/lib/methods/subscriptions/rooms.test.ts
echo "=== imports and setup ==="
sed -n '1,80p' app/lib/methods/subscriptions/rooms.test.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 8595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import asyncio
class MockDb:
def __init__(self):
self.lock = None
self.batch_calls = []
async def write(self, fn):
while self.lock is not None:
await asyncio.sleep(0)
self.lock = True
try:
return await fn()
finally:
self.lock = None
def batch(self, batch):
self.batch_calls.append(batch); return asyncio.sleep(0)
class SubscriptionRecord:
def __init__(self):
self._preparedState = 'pending'
self.lastOpen = 'old'
def prepareUpdate(self, update):
if self._preparedState is None:
raise Exception('RecordAlreadyPrepared: pending changes')
update(self)
return self
def update(self, data):
self.lastOpen = data.get('lastOpen', self.lastOpen)
async def update_last_open_old(db, rid, records):
sub = SubscriptionRecord()
await db.write(lambda: (
db.batch(sub.prepareUpdate(lambda s: s.update(records[0]))),
)[0])
return sub.lastOpen
async def create_or_update_old(db, rid, msg_delay=False):
sub = SubscriptionRecord()
batch = []
batch.append(sub.prepareUpdate(lambda s: s.update({'lastMessage': {'_id': 'msg-id'}})))
if msg_delay:
await asyncio.sleep(0.01)
await db.write(lambda: db.batch(batch))
async def main():
result = []
for name, cfunc in [('old', create_or_update_old), ('old', update_last_open_old)]:
pass
for name, cfunc in [('createOrUpdateSubscription_old', create_or_update_old), ('updateLastOpen_old', update_last_open_old)]:
print(name)
async def run():
db = MockDb()
try:
await asyncio.gather(cfunc(db, 'rid'), update_last_open_old(db, 'rid', [{'lastOpen': 'new'}]))
print('ok')
except Exception as e:
print(f'raised: {e}')
asyncio.run(run())
print('non-deterministic interleaving example: updateLastOpen acquires writer before createOrUpdateSubscription prepares')
async def run():
db = MockDb()
sub = SubscriptionRecord()
batch = []
batch.append(sub.prepareUpdate(lambda s: s.update({'lastMessage': {'_id': 'msg-id'}})))
await asyncio.sleep(0)
await db.write(lambda: db.batch(batch)).get() if hasattr((tmp := db.write(lambda: db.batch(batch))).get, '__await__') else None
PYRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== updateLastOpen implementation / mocks ==="
rg -n "function updateLastOpen|const updateLastOpen|updateLastOpen|db\.write|prepareUpdate|_preparedState" app/lib -g '*.ts' -g '*.tsx'
echo "=== focused updateLastOpen files ==="
fd -t f '^updateLastOpen\.(ts|tsx)$' app/lib
while IFS= read -r f; do
echo "=== $f ==="
sed -n '1,220p' "$f"
done < <(fd -t f '^updateLastOpen\.(ts|tsx)$' app/lib)Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 11238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== updateLastOpen implementation ==="
sed -n '1,80p' app/lib/methods/updateLastOpen.ts
echo "=== Subscription service implementation ==="
sed -n '1,180p' app/lib/database/services/Subscription.ts | sed -n '1,160p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2515
Make the concurrency ordering explicit.
updateLastOpen awaits getSubscriptionByRoomId(rid) before entering db.write, while this test resolves that lookup immediately. updateLastOpen can acquire and finish inside the writer lock before createOrUpdateSubscription performs any pre-write preparation, so the existing arrangement may pass even on the old implementation. Delay the subscription lookup so createOrUpdateSubscription prepares while updateLastOpen is inside its writer lock, and keep the positive mockDbBatch assertion.
🤖 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/rooms.test.ts` around lines 153 - 173, The
concurrency test should force updateLastOpen to hold the writer lock while
createOrUpdateSubscription completes its preparation. In the test around
createOrUpdateSubscription and updateLastOpen, delay the getSubscriptionByRoomId
mock until the intended ordering is established, and retain a positive assertion
that mockDbBatch was called to verify the writer path executed.
1 participant
Proposed changes
Fixes a Bugsnag crash: "Cannot update a record with pending changes". Two functions prepared WatermelonDB records outside the writer lock and committed the batch later. A concurrent writer on the same cached record made the commit throw, and the pending change was lost.
createOrUpdateSubscription(app/lib/methods/subscriptions/rooms.ts) now reads, prepares, and batches inside onedb.writecallback.decryptPendingSubscriptions(app/lib/encryption/encryption.ts) does the same; decryption still runs outside the lock.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1470
How to test or reproduce
TZ=UTC pnpm test app/lib/methods/subscriptions/rooms.test.ts app/lib/methods/subscriptions/room.test.ts.Screenshots
Types of changes
Checklist
Further comments
This is the first fix of a defect class tracked in https://rocketchat.atlassian.net/browse/NATIVE-1462: prepare calls outside
db.writewith the batch committed later. Seven more call sites follow the same pattern in later PRs.Summary by CodeRabbit
Bug Fixes
Tests