Repository navigation
Conversation
Traefik sends /v1/storage to the Rust API, alongside Users and Teams. The PHP module stays. The local-device suite passes through Traefik, and the benchmark compares both runtimes on the same stack. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review complete. 🟡 5 medium 💬 Inline comments (4)
📍 Findings outside the diff (1) — 🟡 1 medium — defects on lines GitHub can't attach comments to🟡 Medium — CI never verifies // .github/workflows/ci.yml
1119 - name: Verify /v1/users is served by Rust
1120 run: |
1121 docker compose exec -T appwrite curl -s -o /dev/null -w '%{http_code}\n' -H 'Host: appwrite.test' http://appwrite.test/v1/users
1122 docker compose exec -T appwrite-rust /usr/local/bin/appwrite-rust healthThe 🧹 Nitpicks (1) — 🟢 1 low
The PR introduces a new
Reviewed commit: 25d0e0a |
🟡 Tier B · Needs changes before merging
This PR ports the 14 Storage API routes to the Rust service, routes Latest changes: The newest commits refresh the Teams benchmark run, move endpoint median comparisons into per-load verdict badges with updated chart layout and copy, and remove a machine-specific Vite worktree launch configuration.
Fix with agent prompt### Issue 1
crates/appwrite/storage/src/device.rs:166-175
**Preserve the existing storage device configuration**
This factory only handles `_APP_STORAGE_DEVICE` values `s3` and `awss3`; otherwise it silently selects local storage. It ignores `_APP_CONNECTIONS_STORAGE` and the provider-specific device settings that `getDevice()` in `app/init/resources.php` supports, so installations using those configurations will no longer find existing files and may write new uploads to the wrong backend.
### Issue 2
crates/appwrite/storage/src/http/content.rs:284
**Avoid buffering the entire file for downloads**
`serve` reads the whole object into a `Vec` before handling ranges or returning the response, including plain files larger than the 20 MB transformation buffer and range requests that need only a small slice. PHP streams large files in bounded chunks or reads only the requested range, so concurrent downloads of valid large files can exhaust the Rust service's memory.
### Issue 3
crates/appwrite/server/src/main.rs:91
**Size concurrency against the request-body memory budget**
This admits 4,096 simultaneous requests while the server buffers each body up to the 12 MiB limit, so concurrent large uploads can require roughly 48 GiB just for request bodies and OOM the Rust process. Please size this cap against a bounded memory budget or add an aggregate body-memory limit.
### Issue 4
crates/utopia/http/src/server.rs:142
**Bound the time spent draining rejected request bodies**
This waits for EOF before returning 503, and `collect_body` keeps draining even after the size limit is exceeded; with no body-read timeout, clients can pause or trickle data indefinitely and keep arbitrarily many rejected connection tasks open beyond the concurrency cap. Please use a bounded drain/deadline so overload responses cannot be held open this way.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 13
⏳ Still open from earlier reviews · 4
Reviewed the commits since |
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR migrates the Storage service (buckets, files, previews, chunked uploads, antivirus scanning) to Rust, adding compose/Traefik routing for /v1/storage and CI coverage, with several medium-severity issues around encrypted-file compatibility, memory-buffered range serving, and infra verification.
Key findings
- 🟡 Range requests load and decrypt the whole file — content.rs:284
- 🟡 PHP-written encrypted files fail Rust decryption — content.rs:341
- 🟡 Rust storage silently ignores non-AWS S3 devices — docker-compose.override.yml:619
- 🟡 CI never verifies
/v1/storagereaches Rust — ci.yml:1119 - 🟡 Unbounded over-limit drain holds concurrency slot — server.rs:161
| let mut bytes = base::read_all(device, &path).await?; | ||
| if !file.string_or_empty("openSSLCipher").is_empty() { | ||
| bytes = decrypt(&ctx.state.config.openssl_key_v1, file, &bytes)?; | ||
| } | ||
| bytes = decompress(&file.string_or_empty("algorithm"), bytes)?; | ||
| let size = base::long(file, "sizeOriginal"); | ||
| let ranged = range_of(ctx, size, open_end)?; | ||
| let (status, body) = match ranged { | ||
| Some((start, end)) => (StatusCode::PARTIAL_CONTENT, slice(&bytes, start, end)), | ||
| None => (StatusCode::OK, bytes), | ||
| }; | ||
| let mut response = Response::with_body(status, content_type, body); |
There was a problem hiding this comment.
🟡 performance · medium
Range requests load and decrypt the whole file
serve reads the entire file via base::read_all and runs decrypt/decompress over the full buffer before range_of narrows the response to at most OUTPUT_CHUNK/READ_BUFFER bytes (crates/appwrite/storage/src/http/content.rs:284). For a multi-GB file, every ranged download/view request pays full I/O, decrypt, decompress and an extra Vec copy in slice just to emit ~10 MB. This regresses per-request memory and latency proportionally to file size on a hot path.
📋 Prompt for AI Agents
In crates/appwrite/storage/src/http/content.rs, restructure serve (around lines 270-310): call range_of first using the file's sizeOriginal, and when a range is present read only the requested span with device.read(&path, start, Some(end - start + 1)) instead of base::read_all, slicing after decrypt/decompress only when the file has a cipher/algorithm. Keep the full-file read only when no Range header is present.
| fn decrypt(key: &str, file: &Document, data: &[u8]) -> Result<Vec<u8>> { | ||
| let iv = hex::decode(file.string_or_empty("openSSLIV")).unwrap_or_default(); | ||
| let tag = hex::decode(file.string_or_empty("openSSLTag")).unwrap_or_default(); | ||
| appwrite_core::crypto::open(key, data, &iv, &tag) | ||
| .ok_or_else(|| Error::with_message(ErrorType::GeneralServerError, "Failed to decrypt file")) | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
PHP-written encrypted files fail Rust decryption
PHP encrypts stored files with OpenSSL::encrypt($data, CIPHER_AES_128_GCM, $key, 0, $iv, $tag) where options = 0 emits base64-encoded ciphertext, and decrypts the same way (src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.php:476). Rust seal/open work on raw ciphertext and a raw 16-byte GCM tag (crates/appwrite/core/src/crypto.rs:70-90), so any file written by the PHP runtime with bucket encryption enabled fails tag verification in Rust's decrypt and download/view/preview return 500 'Failed to decrypt file'. The reverse also holds for Rust-written files read by PHP.
📋 Prompt for AI Agents
In crates/appwrite/storage/src/http/content.rs lines 341-346 (decrypt), detect the PHP encoding (options=0 base64 ciphertext per src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.php:476) and base64-decode the data before calling appwrite_core::crypto::open, falling back to raw bytes when base64 decoding fails, so files encrypted by either runtime decrypt correctly. Verify the PHP serving decrypt options (Download/Get.php uses options=0) before choosing the default path.
| - _APP_STORAGE_DEVICE | ||
| - _APP_STORAGE_S3_ACCESS_KEY | ||
| - _APP_STORAGE_S3_SECRET | ||
| - _APP_STORAGE_S3_REGION | ||
| - _APP_STORAGE_S3_BUCKET | ||
| - _APP_STORAGE_S3_ENDPOINT |
There was a problem hiding this comment.
🟡 bug · medium
Rust storage silently ignores non-AWS S3 devices
The Traefik rule routes all /v1/storage traffic to the Rust container, but Rust's storage factory recognizes only _APP_STORAGE_DEVICE values s3/awss3 (plus the implicit local) and silently falls back to the local device otherwise (crates/appwrite/storage/src/device.rs:172-175). PHP supports do_spaces, backblaze, linode and wasabi devices, and their env vars are not passed to the Rust service either (docker-compose.override.yml:619-624). A deployment configured with one of those devices gets uploads written to the local volume by Rust while PHP uses the object store, producing divergent file placement depending on which runtime served the request.
📋 Prompt for AI Agents
In crates/appwrite/storage/src/device.rs for_root() (around lines 165-176), treat an unimplemented _APP_STORAGE_DEVICE value as a startup error (or map the additional S3-compatible provider names PHP supports onto the same S3 device with their env vars) instead of silently falling back to Stored::Local; also add the missing provider env vars to the appwrite-rust service environment in docker-compose.override.yml.
| async fn collect_body(mut body: Incoming, max: usize) -> Result<Bytes, ()> { | ||
| let mut collected = Vec::new(); | ||
| let mut over = false; | ||
| loop { | ||
| match body.frame().await { | ||
| Some(Ok(frame)) => { | ||
| let Ok(data) = frame.into_data() else { continue }; | ||
| if over || collected.len().saturating_add(data.len()) > max { | ||
| over = true; | ||
| continue; | ||
| } | ||
| collected.extend_from_slice(&data); | ||
| } | ||
| Some(Err(_)) => return Err(()), | ||
| None => break, | ||
| } | ||
| } | ||
| if over { Err(()) } else { Ok(Bytes::from(collected)) } |
There was a problem hiding this comment.
🟡 bug · medium
Unbounded over-limit drain holds concurrency slot
collect_body keeps reading frames after the limit is exceeded with no cap on drained bytes and no timeout (crates/utopia/http/src/server.rs:161-178), and the in-flight guard is taken before collection (crates/utopia/http/src/server.rs:135). header_read_timeout protects only header reads, so a client sending a huge Content-Length or endless chunked stream occupies its concurrency slot, the worker task and bandwidth for the entire upload; under concurrent abuse this is a cheap resource-exhaustion vector the previous Limited path did not have. The drain itself is deliberate (so Traefik forwards the 413), but it needs a bound.
📋 Prompt for AI Agents
In crates/utopia/http/src/server.rs, collect_body (lines 161-178): track drained bytes after over is set and stop reading once a bounded drain window (for example a few MB past max) or a short deadline is reached, returning Err(()) at that point so the connection closes instead of draining the client's stream to EOF.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
The service page, facts, tasks and changelog now show Storage served by Rust, with the recorded suite and the benchmark. Co-authored-by: Cursor <cursoragent@cursor.com>
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Tier B · 2 blocking findings to address. Summary
| let configured = utopia_system::env("_APP_STORAGE_DEVICE").unwrap_or_else(|| "local".to_owned()); | ||
| let key = utopia_system::env("_APP_STORAGE_S3_ACCESS_KEY").unwrap_or_default(); | ||
| let secret = utopia_system::env("_APP_STORAGE_S3_SECRET").unwrap_or_default(); | ||
| let region = utopia_system::env("_APP_STORAGE_S3_REGION").unwrap_or_default(); | ||
| let bucket = utopia_system::env("_APP_STORAGE_S3_BUCKET").unwrap_or_default(); | ||
| let endpoint = utopia_system::env("_APP_STORAGE_S3_ENDPOINT").unwrap_or_default(); | ||
| let s3 = | ||
| matches!(configured.as_str(), "s3" | "awss3") && !key.is_empty() && !secret.is_empty() && !bucket.is_empty(); | ||
| if !s3 { | ||
| return Ok(Stored::Local(Local::new(root))); |
There was a problem hiding this comment.
Preserve the existing storage device configuration
This factory only handles _APP_STORAGE_DEVICE values s3 and awss3; otherwise it silently selects local storage. It ignores _APP_CONNECTIONS_STORAGE and the provider-specific device settings that getDevice() in app/init/resources.php supports, so installations using those configurations will no longer find existing files and may write new uploads to the wrong backend.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/appwrite/storage/src/device.rs
Line: 166-175
Comment:
**Preserve the existing storage device configuration**
This factory only handles `_APP_STORAGE_DEVICE` values `s3` and `awss3`; otherwise it silently selects local storage. It ignores `_APP_CONNECTIONS_STORAGE` and the provider-specific device settings that `getDevice()` in `app/init/resources.php` supports, so installations using those configurations will no longer find existing files and may write new uploads to the wrong backend.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · bug · Reply if this doesn't apply.
| if !device.exists(&path).await? { | ||
| return Err(Error::new(ErrorType::StorageFileNotFound)); | ||
| } | ||
| let mut bytes = base::read_all(device, &path).await?; |
There was a problem hiding this comment.
Avoid buffering the entire file for downloads
serve reads the whole object into a Vec before handling ranges or returning the response, including plain files larger than the 20 MB transformation buffer and range requests that need only a small slice. PHP streams large files in bounded chunks or reads only the requested range, so concurrent downloads of valid large files can exhaust the Rust service's memory.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/appwrite/storage/src/http/content.rs
Line: 284
Comment:
**Avoid buffering the entire file for downloads**
`serve` reads the whole object into a `Vec` before handling ranges or returning the response, including plain files larger than the 20 MB transformation buffer and range requests that need only a small slice. PHP streams large files in bounded chunks or reads only the requested range, so concurrent downloads of valid large files can exhaust the Rust service's memory.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · performance · Reply if this doesn't apply.
A minute at 500 req/s held more than 1,000 requests. The server answered 503 before reading the body, so uploads failed as broken pipes. The Rust API now allows 4,096 in flight, reads the body before a 503, and reads upload bytes off the runtime threads. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Tier B · 2 blocking findings to address. Summary
| // (a busy machine, preview renders, chunked uploads) holds that many | ||
| // requests. The benchmark client opens up to 1,040 of them at 500 | ||
| // req/s, and each rejection was a 503. 4,096 still bounds a flood. | ||
| options.max_concurrency = appwrite_core::config::env_int("_APP_RUST_MAX_CONCURRENCY", 4_096).max(1) as usize; |
There was a problem hiding this comment.
Size concurrency against the request-body memory budget
This admits 4,096 simultaneous requests while the server buffers each body up to the 12 MiB limit, so concurrent large uploads can require roughly 48 GiB just for request bodies and OOM the Rust process. Please size this cap against a bounded memory budget or add an aggregate body-memory limit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/appwrite/server/src/main.rs
Line: 91
Comment:
**Size concurrency against the request-body memory budget**
This admits 4,096 simultaneous requests while the server buffers each body up to the 12 MiB limit, so concurrent large uploads can require roughly 48 GiB just for request bodies and OOM the Rust process. Please size this cap against a bounded memory budget or add an aggregate body-memory limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · performance · Reply if this doesn't apply.
There was a problem hiding this comment.
Bound the time spent draining rejected request bodies
This waits for EOF before returning 503, and collect_body keeps draining even after the size limit is exceeded; with no body-read timeout, clients can pause or trickle data indefinitely and keep arbitrarily many rejected connection tasks open beyond the concurrency cap. Please use a bounded drain/deadline so overload responses cannot be held open this way.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/utopia/http/src/server.rs
Line: 142
Comment:
**Bound the time spent draining rejected request bodies**
This waits for EOF before returning 503, and `collect_body` keeps draining even after the size limit is exceeded; with no body-read timeout, clients can pause or trickle data indefinitely and keep arbitrarily many rejected connection tasks open beyond the concurrency cap. Please use a bounded drain/deadline so overload responses cannot be held open this way.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · security · Reply if this doesn't apply.
Rust now holds 500 requests per second with no server errors. The service page, facts and changelog use that run. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the speedup labels under each latency curve with the same verdict badges used elsewhere, and publish the latest Teams run. Co-authored-by: Cursor <cursoragent@cursor.com>
This branch has not been deployed
1 participant
Summary
appwrite-rust. Traefik sends/v1/storagethere when therustcompose profile is on, the same way it already sends Users and Teams. The PHP module is still in the tree._APP_E2E_ENDPOINT=http://appwrite.test/v1) passed: 108 tests, 2883 assertions. CI runs that suite on the Rust E2E job. Antivirus, screenshots, abuse, queue retry, and usage outage stay excluded, matching the PHP job./v1/s3stays on PHP.Benchmark
ALLOW_BUSY=1 tests/benchmarks/api/compare.sh storage, 60s per step, results intests/benchmarks/results/storage.json. Other cargo jobs held the load between about 20 and 33 for the whole run, so these numbers are slower than a quiet machine. Both sides ran under that load.At 50 req/s both sides held the rate with no errors.
At 50 virtual users, Rust reached 487 req/s and PHP 209 req/s, with no errors. At 200 req/s Rust held the rate (p50 6.5 ms). PHP delivered 106 req/s and dropped 5746 iterations. At 500 req/s both sides crossed the 1% server-error budget.
Test plan
cargo fmt -p appwrite-storageandcargo clippy -p appwrite-storage --all-targets -- -D warningsparatest --functional tests/e2e/Services/Storagethrough Traefik, excludingabuseEnabled,screenshots,antivirus,queueRetry,usageOutagetests/benchmarks/api/compare.sh storageChecklist
Made with Cursor