fix(storage): prevent concurrent chunk finalization - #8
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: appwrite#13505 Source head: a0a1547
⛔ Shipwright · BlockedRecommendation: do not merge PR #8 · Tier
Findings (7)
Fireworks usage: 12,483 input · 1,013 output · 13,496 total tokens · $0.0034 · 16s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| throw new LockContention('Upload lease lost before transfer: ' . $lock->token()); | ||
| } | ||
|
|
||
| $chunksUploaded = $deviceForFiles->upload( |
There was a problem hiding this comment.
Shipwright · CRITICAL
The lock is refreshed before upload but not before finalizeUpload.
Impact: The lock is refreshed before upload but not before finalizeUpload. After a long transfer, the lease can expire between the isHeld() check and finalizeUpload's document write, allowing two requests to finalize concurrently and corrupt chunk accounting. The isHeld() check is TOCTOU: it verifies ownership, then finalizeUpload performs multiple non-atomic DB operations without re-verifying or refreshing the lease.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
| fclose($sourceHandle); | ||
|
|
||
| if ($duplicate) { |
There was a problem hiding this comment.
Shipwright · CRITICAL
The duplicate-chunk test path sends the same chunk twice concurrently.
Impact: The duplicate-chunk test path sends the same chunk twice concurrently. If both requests pass prepareUpload before either finalizes, both will call deviceForFiles->upload() with the same chunk. Depending on the device implementation, this can double-count chunksUploaded or corrupt the multipart upload state. The lock serializes the critical section, but the second request re-reads the file document inside prepareU…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| // Restart the lease so the transfer gets the full window, | ||
| // regardless of how long preparation took. | ||
| if (!$lock->refresh()) { |
There was a problem hiding this comment.
Shipwright · CRITICAL
The lock token is interpolated into an exception message: 'throw new LockContention('Upload lease lost before transfer: ' .
Impact: The lock token is interpolated into an exception message: 'throw new LockContention('Upload lease lost before transfer: ' . $lock->token())'. If the token is sensitive (e.g., a Redis lock token that could be used to release or refresh the lock), leaking it in error responses or logs enables an attacker to hijack the lock. The exception is caught and converted to a generic rate-limit error, but the original messag…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| use HTTP; | ||
|
|
||
| /** | ||
| * Lease for the per-file upload lock, in seconds. Refreshed before the |
There was a problem hiding this comment.
Shipwright · HIGH
The LOCK_TTL constant is 600 seconds, but the lock timeout passed to $locks() is 120.0 seconds.
Impact: The LOCK_TTL constant is 600 seconds, but the lock timeout passed to $locks() is 120.0 seconds. A reader must understand the difference between TTL (lease duration) and timeout (wait time to acquire) to know why these differ. The comment explains the TTL but not the timeout, and the magic number 120.0 appears twice without explanation.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
|
|
||
| public function testCreateBucketFileParallelChunksLargeFile(): void | ||
| public static function parallelChunksProvider(): array |
There was a problem hiding this comment.
Shipwright · HIGH
The test provider name 'duplicate chunks' is misleading.
Impact: The test provider name 'duplicate chunks' is misleading. The test does not verify that duplicate chunks are handled correctly; it sends duplicate requests and asserts all return 200/201. A new hire reading this test would assume duplicate chunk handling is validated, but the assertions only check response codes and final state, not that the duplicate was deduplicated or rejected.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| $this->assertEquals($chunksTotal, $uploadedFile['body']['chunksTotal']); | ||
| $this->assertEquals($chunksTotal, $uploadedFile['body']['chunksUploaded']); | ||
|
|
||
| // A late retry must return the completed file without writing or finalizing again. |
There was a problem hiding this comment.
Shipwright · HIGH
The retry test reuses '$requests[0]['headers']' and '$requests[0]['chunkPath']' to construct a late retry.
Impact: The retry test reuses '$requests[0]['headers']' and '$requests[0]['chunkPath']' to construct a late retry. If the original request contained a one-time upload token or session-bound header, this test masks a real-world issue where retries with stale credentials would fail. More importantly, the test asserts the retry returns 200 with the same signature, but does not verify the retry did not re-trigger finalization s…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| $container->set('locks', fn (Group $pools) => fn (string $key, int $ttl, callable $callback, float $timeout = 0.0): mixed => $pools->get('lock')->use( | ||
| fn (\Redis $redis) => (new Distributed($redis, $key, ttl: $ttl))->withLock($callback, timeout: $timeout) | ||
| function (\Redis $redis) use ($key, $ttl, $callback, $timeout): mixed { |
There was a problem hiding this comment.
Shipwright · LOW
The lock callback now passes the Distributed lock object to the callback, but the callback signature in Create.php is typed as 'function (Distributed $lock)'.
Impact: The lock callback now passes the Distributed lock object to the callback, but the callback signature in Create.php is typed as 'function (Distributed $lock)'. The container closure in resources.php invokes '$callback($lock)', which is correct. However, the 'withLock' callback is 'fn () => $callback($lock)', and 'withLock' may invoke the callback with no arguments; this is fine. No defect here.
Suggested fix: Fix the review finding before release.
1 participant
What does this PR do?
Fixes the upload race behind
StorageCustomServerTest::testCreateBucketFileParallelChunksLargeFilein the linked CI job. This is a server bug, not a test timeout.Utopia\Storage\Device::upload()can finalize an upload itself. Previously Appwrite locked preparation and document completion separately but calledupload()outside either lock. Concurrent Local uploads could both enterjoinChunks(), then unlink parts another request was still reading. CI reportedFailed to open chunk ...part.3and HTTP 500; cleanup also failed afterward.Transfers to the storage device for the same file now serialize. Different files retain independent locks. This avoids an upstream storage API change; it can increase same-file contention. The existing 600-second lock lease is unchanged and is not automatically renewed.
Test Plan
Validated in an isolated Local-storage/PostgreSQL dedicated stack with the repository's locked Composer dependencies, PHP 8.5.9, and Swoole:
main'sCreate.phpinto the same stack and ran the enhanced regression: the duplicate-chunk dataset failed with HTTP 500 andFailed to open chunk ...part.2, reproducing the CI failure mechanism.vendor/bin/paratest --processes 4 --functional tests/e2e/Services/Storage --exclude-group abuseEnabled --exclude-group screenshots --exclude-group antivirus: 92 tests, 1,600 assertions, all passed.composer lintfor both changed files,php -lfor both files, andgit diff --check: passed.S3, shared-table mode, and antivirus integration were not exercised locally.
Related PRs and Issues
Checklist
Source merge-base:
bd66764e20aee7a8622bc9ee1de052bca3bf07ceSource head:
a0a154792cb4fe893ce05e9e5be937d792461550