Repository navigation
Conversation
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
|
Review complete. 🟡 1 medium 📍 Findings outside the diff (1) — 🟡 1 medium — defects on lines GitHub can't attach comments to🟡 Medium — Failed // packages/lock/src/Distributed.php
129 try {
130 return $callback();
131 } finally {
132 $this->release();
133 }
The PR introduces lease-based distributed locking around S3 multipart uploads and file deletion, adds device-layer support for chunked staging and part accounting, and wires the new lock resource into the app bootstrap. E2E tests cover the S3 server and storage flows.
Reviewed commit: 37aebb6 |
🟢 Tier S · Ready to mergeMultipart uploads now separate preparation, transfer, and receipt recording so S3-backed parts can overlap while local uploads retain serialization. Native uploads use generation-specific paths and coordinate publication and deletion with S3 operations; distributed locks borrow Redis connections per command, and storage APIs expose explicit chunk upload without finalizing. Latest changes: The newest commits make lock release retry once while preserving callback errors, coordinate native document creation with S3 object publication, and retain finalized multipart staging on contention so receipts remain retryable.
📂 Walkthrough · 16✅ Fixed since the last review · 1
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Tier B · 1 blocking finding to address. Summary
This branch has not been deployed
1 participant
What does this PR do?
Native and S3-compatible multipart uploads currently hold a shared upload lock across every backend transfer. Eight concurrent SDK calls therefore still upload one part at a time. Split preparation, transfer and receipt recording so distinct S3-backed parts overlap, while same-part retries and final publication remain coordinated. Local storage retains its existing serialization.
Use unique native multipart generation paths and validate their path/upload ID before acknowledging progress. Coordinate native deletion with publication, and return retryable S3 SlowDown while a native file is incomplete. Native document creation now takes the same folder/name publication lock as S3 completion, only around document creation. Contention retains finalized staging bytes for safe retries when an older native writer bypasses that guard. Keep S3's isolated overwrite paths, selected-part ETags and pinned download streams intact. Existing upload() still auto-finalizes; uploadChunk() exposes the existing explicit non-finalizing phase.
Distributed locks can borrow Redis per command rather than retaining a pooled connection for the lease. This is necessary for nested per-part and upload-state guards without exhausting the connection pool; direct Redis construction is unchanged. Token-guarded release retries a transient Redis failure once; persistent cleanup failure preserves the original operation exception, and still propagates if the operation succeeded. No SDK or benchmark source changes are required.
Test Plan
Local isolated Appwrite + PostgreSQL + Redis + the CI-pinned Silo S3 backend, PHP 8.5.11, four HTTP workers:
Three verified 100 MiB samples/API/version with the same client, eight concurrent part calls and a controlled 250 ms backend part delay:
Native file-lock hold median: 374 → 3.72 ms; S3 upload-state lock hold median: 362 → 0.14 ms. These are controlled latency results, not production estimates or a Railway comparison. Initial exploratory single-worker memory exhaustion is not claimed fixed; both comparison sides use four workers.
Coverage limit: forced lease-loss and delete/recreate-in-flight checks are controlled manual drills, not automated CI E2E. The review-fix drill also remains manual. Current review-fix evidence is
/Users/loks0n/.codex/artifacts/storage-review-fixes-2026-10-21. The original performance source/trace/report bundle is/Users/loks0n/.codex/artifacts/storage-multipart-profile-2026-10-20; operational details are on the linked tracking issue.Deployments remain paused. No merge, release, staging/production rollout or Cloud dependency update is included.
Related PRs and Issues
Checklist