Fix race condition in bulk upsert verification

[tponviji1005-oss]

Summary

• Re-fetch existing documents with FOR UPDATE inside the transaction.
• Detect concurrent modifications before bulk upsert.
• Prevent silent data loss caused by stale reads during concurrent bulk updates.

Fixes appwrite/appwrite#12960

Summary by CodeRabbit

• Bug Fixes
  • Added stronger safeguards against conflicting simultaneous document updates.
  • Changes are now validated against the latest stored version before applying, including checking the persisted last-updated time to prevent accidental overwrites when data has changed since it was read.

[coderabbitai]

Warning

Review limit reached

@tponviji1005-oss, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 54 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9faf27b7-867b-4b39-ba18-62ec8517c39a

📥 Commits

Reviewing files that changed from the base of the PR and between 7fd133e and 499f45d.

📒 Files selected for processing (1)

• src/Database/Database.php

📝 Walkthrough

Walkthrough

upsertDocumentsWithIncrease() now verifies locked row versions inside each batch transaction before executing the adapter upsert, throwing ConflictException when a document was deleted or changed since its initial read.

Changes

Bulk upsert concurrency validation

Layer / File(s)	Summary
Locked row version validation src/Database/Database.php	Existing documents are re-read with forUpdate: true under authorization skip, with tenant context reapplied when needed. Missing rows or changed updatedAt values raise ConflictException before the transactional adapter upsert.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• utopia-php/database#610: Modifies the same upsert path with updatedAt-based conflict detection.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The change detects stale rows, but it still relies on a pre-transaction merge base and turns concurrent disjoint upserts into conflicts instead of preserving both writes.	Move the merge base read and merge logic fully inside the write transaction, or make the bulk upsert patch only payload columns so concurrent disjoint updates are preserved.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: fixing a race condition in bulk upsert verification.
Out of Scope Changes check	✅ Passed	The changes stay focused on bulk upsert concurrency handling and related tenant scoping.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[greptile-apps]

Greptile Summary

Fixes one race in bulk upsert processing:

• Re-fetches existing documents inside the write transaction using locking reads.
• Rejects the batch if a document was deleted or its update timestamp changed before persistence.

Confidence Score: 3/5

The PR is not yet safe to merge because unsupported update locks and timestamp collisions still allow concurrent changes to be silently overwritten.

The deletion check now rejects a missing locking read result, but Redis and non-replica-set Mongo still lack the required exclusion between verification and upsert, while the sole update comparison uses a millisecond-precision timestamp that concurrent writes can share.

Files Needing Attention: src/Database/Database.php; src/Database/Adapter/Mongo.php; src/Database/Adapter/Redis.php

Important Files Changed

Filename	Overview
src/Database/Database.php	Adds transactional re-fetch and conflict checks before bulk upsert; the concurrent-deletion case is now rejected.

_{Reviews (4): Last reviewed commit: "Merge branch 'main' into main" | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@src/Database/Database.php`:
- Around line 7499-7501: Update the locking re-fetch in the document upsert flow
around getDocument to execute within withTenant($old->getTenant(), ...) when
tenant-per-document mode is enabled, matching the initial read’s tenant scoping;
preserve the existing authorization and silent wrappers and use the ambient
context only when tenant-per-document mode is disabled.
- Around line 7502-7504: The conflict check in upsertDocuments() must also
reject an originally existing row that is missing from the locked re-read.
Update the condition around $fresh and $old so it throws ConflictException when
$fresh is empty or its updated timestamp differs, preventing stale Change data
from recreating a concurrently deleted document.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: caa2ecea-6126-4f7b-b786-ec1463379fd8

📥 Commits

Reviewing files that changed from the base of the PR and between aa3fe32 and f946488.

📒 Files selected for processing (1)

• src/Database/Database.php