fix(db): move decryptPendingMessages prepares inside the writer lock

[OtavioStasiak]

Proposed changes

decryptPendingMessagesdecrypted each pending e2e message and calledprepareUpdateoutsidedb.write, committing the batch in a separate write later. A concurrent writer touching the same cached record during that window — a new message arriving in the room, for instance — left the prepared records stale, so the commit threw Cannot update a record with pending changes` (reaching Bugsnag) and the message stayed encrypted.

Decryption now happens first, outside the lock, and the prepareUpdate calls plus the db.batch run inside a single db.write callback. Records whose prepareUpdate throws are now filtered out instead of being batched as null.

Adds one regression test: a concurrent writer races the same record and the batch must commit without a "pending changes" throw. It fails on the current code and passes with the fix. Signature and both callers (Encryption.initialize, createOrUpdateSubscription) unchanged.

Issue(s)

https://rocketchat.atlassian.net/browse/NATIVE-1464

How to test or reproduce

• TZ=UTC pnpm test app/lib/encryption/encryption.test.ts — all pass; revert encryption.ts and the new test fails with
  Cannot update a record with pending changes
  • Open an E2EE room with pending encrypted messages (e.g. kill the app before decryption finishes, or enter the E2EE
    password after opening the room) — they all decrypt, none stay encrypted
  • Do the above while new messages arrive in that room, so decryption overlaps with incoming writes
  • Enter an E2EE room from the rooms list — createOrUpdateSubscription triggers a per-room decrypt
  • Cold start with several E2EE rooms having pending messages — Encryption.initialize decrypts them all

Screenshots

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• Improvement (non-breaking change which improves a current function)
• New feature (non-breaking change which adds functionality)
• Documentation update (if none of the other choices apply)

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• I have added necessary documentation (if applicable)
• Any dependent changes have been merged and published in downstream modules

Further comments

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability when decrypting pending messages, including situations where multiple updates affect the same message.
  • Prevented failed or incomplete decryption updates from being committed, reducing database errors and ensuring only valid changes are saved.
  • Ensured successful message updates continue to be processed even when another update fails.

• Tests

  • Added coverage for concurrent message updates, partial failures, and successful batch processing of decrypted messages.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fbc96ee7-5a1b-4fbe-bd79-e9c484a940e1

📥 Commits

Reviewing files that changed from the base of the PR and between 3206899 and ef21493.

📒 Files selected for processing (2)

• app/lib/encryption/encryption.test.ts
• app/lib/encryption/encryption.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• app/lib/encryption/encryption.test.ts

📜 Recent 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/encryption/encryption.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/encryption/encryption.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/encryption/encryption.ts

🧠 Learnings (1)

📚 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/encryption/encryption.ts

🔇 Additional comments (2)

app/lib/encryption/encryption.ts (2)

348-349: LGTM!

---

365-382: LGTM!

---

Walkthrough

decryptPendingMessages now prepares database updates inside the serialized write operation. Tests model concurrent database writes and verify that successful decrypted updates commit while failed updates are skipped.

Changes

Pending message decryption

Layer / File(s)	Summary
Serialized decryption writes app/lib/encryption/encryption.ts	The method decrypts pending records before preparing updates. It derives room IDs through optional chaining, prepares updates inside the database write operation, logs preparation failures, and batches the resulting values.
Concurrent write regression tests app/lib/encryption/encryption.test.ts	The database mock supports query results, serialized writers, and batch state clearing. Tests cover concurrent writes, successful commits, failed updates, and cleared pending state.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to ef214

This localized change moves message-update preparation and batching into one writer transaction and adds regression coverage for the concurrent-write failure; no actionable merge-blocking risk remains after normal checks and review.

Suggested labels: type: bug

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes moving decryptPendingMessages preparation inside the database writer lock.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

• NATIVE-1464: Request failed with status code 401

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

app/lib/encryption/encryption.test.ts (1)

175-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type the reusable test helpers.

makeMessageRecord and deferred infer their return shapes. makeMessageRecord also erases the updater contract with any. Define focused interfaces and explicit return types so an invalid prepareUpdate mock cannot satisfy this regression test.

Proposed change

+interface DeferredGate {
+	promise: Promise<void>;
+	resolve: () => void;
+}
+
+interface TestMessageRecord {
+	id: string;
+	t: string;
+	msg: string;
+	e2e?: string;
+	subscription: { id: string };
+	_preparedState: 'update' | null;
+	prepareUpdate(recordUpdater: (message: TestMessageRecord) => void): TestMessageRecord;
+}
+
-	const makeMessageRecord = (id: string) => {
-		const record: any = {
+	const makeMessageRecord = (id: string): TestMessageRecord => {
+		const record: TestMessageRecord = {
 			// ...
-			prepareUpdate(recordUpdater: (m: any) => void) {
+			prepareUpdate(recordUpdater: (message: TestMessageRecord) => void): TestMessageRecord {
 				// ...
 			}
 		};
 		return record;
 	};
 
-	const deferred = () => {
+	const deferred = (): DeferredGate => {
 		// ...
 	};
 
-				record.prepareUpdate((m: any) => {
-					m.msg = 'written by another writer';
+				record.prepareUpdate(message => {
+					message.msg = 'written by another writer';
 				})

As per coding guidelines, **/*.{ts,tsx} requires explicit annotations for function parameters and return types and prefers interfaces over type aliases for object shapes.

Also applies to: 226-229

🤖 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.test.ts` around lines 175 - 200, Define focused
interfaces for the message record and deferred helper, then update
makeMessageRecord and deferred with explicit parameter and return types. Replace
the any-based prepareUpdate updater with the interface’s typed updater contract,
while preserving the existing pending-state behavior and promise resolver API.

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.

Nitpick comments:
In `@app/lib/encryption/encryption.test.ts`:
- Around line 175-200: Define focused interfaces for the message record and
deferred helper, then update makeMessageRecord and deferred with explicit
parameter and return types. Replace the any-based prepareUpdate updater with the
interface’s typed updater contract, while preserving the existing pending-state
behavior and promise resolver API.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ab44418d-3a6d-4819-ade6-47f4caaff1df

📥 Commits

Reviewing files that changed from the base of the PR and between 89071f3 and cdf5483.

📒 Files selected for processing (2)

• app/lib/encryption/encryption.test.ts
• app/lib/encryption/encryption.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (1)

• 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/encryption/encryption.ts
• app/lib/encryption/encryption.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

Files:

• app/lib/encryption/encryption.ts
• app/lib/encryption/encryption.test.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/encryption/encryption.ts
• app/lib/encryption/encryption.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/encryption/encryption.ts
• app/lib/encryption/encryption.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/encryption/encryption.test.ts

🔇 Additional comments (6)

app/lib/encryption/encryption.ts (3)

338-343: LGTM!

---

378-378: 📐 Maintainability & Code Quality

No type change needed.

TThreadModel | TThreadMessageModel correctly describes the prepared message/thread records sent to db.batch; Model would be a broader type that loses the known message fields.

			> Likely an incorrect or invalid review comment.

---

344-379: 🗄️ Data Integrity & Integration

No change needed for deferred decrypt freshness.

A concurrent writer changes record.msg only before the decrypt result reaches db.batch, and the test still closes on the decrypted update; this is not stale-decrypt overwrite behavior.

app/lib/encryption/encryption.test.ts (3)

4-4: LGTM!

---

52-83: LGTM!

---

169-174: LGTM!

Also applies to: 202-212

[diegolmello]

The fix is right. prepareUpdate + db.batch inside one db.write, decryption outside it — that's the correct shape, and the regression test genuinely fails on the old code.

Requesting changes on one thing: the same bug is still in this file, 20 lines below the fix.

Blocker: the twin is still broken

decryptPendingSubscriptions (app/lib/encryption/encryption.ts:409-421) does exactly what you just fixed — sub.prepareUpdate inside Promise.all, outside the lock, with db.write wrapping only the db.batch at :423. Same crash, same Bugsnag report, different stack. Fixing one and shipping the other means a second PR for the same ticket.

There's a second reason beyond the concurrent-writer race: Model/index.js:122-131 asserts in dev that a prepared record reaches batch() synchronously. The subs path awaits decryptSubscription and then db.write after preparing, so it trips that invariant on its own, with no other writer involved.

Mirror the shape you just landed here. I wouldn't extract a shared helper — two call sites with different updaters, the abstraction costs more than it saves.

Follow-up ticket, not this PR

app/lib/methods/subscriptions/rooms.ts:158 has the same unlocked-prepare bug in a hotter path: prepares a subscription update, awaits getMessageById at :195, prepares a message update at :200, then finally opens db.write at :218 — and calls decryptPendingMessages(tmp.rid) at :225. That's the thing actually racing this decrypt path.

Two things I checked so nobody else flags them

The setImmediate ordering in the test is fine. Everything between entry and db.write is microtasks — resolved-promise fetch() calls and the mocked decryptMessage — and setImmediate is a macrotask, so it drains all of them. An extra await of a resolved promise can't turn it green; it'd take a real timer or IO in that path.

The stateful database mock backing the five older encryptMessage tests is fine too. It's the only honest way to reproduce the writer lock without native WatermelonDB.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.