feat: add shared KMP S3 client for Cells transfers [WPB-26729]

[MohamadJaara]

https://wearezeta.atlassian.net/browse/WPB-26729

Stack

• PR 1 of 2.
• Follow-up hardening: fix(cells): harden Ktor S3 transfers PR 2 #4348.

Intent

Replace the JVM/Android AWS SDK path and Apple no-ops with a shared internal Ktor S3-compatible client across Apple, JVM, and Android.

Changes

• shared internal KMP client for object download, regular upload, multipart create/parts/complete
• SigV4 signing and S3 URL canonicalization
• Ktor/Okio streaming, progress, retry classification, and HTTP timeout configuration
• shared commonMain platform wiring and pre-signed URL downloader
• removal of the AWS SDK dependency and helper classes
• focused signer, transfer, and timeout tests

Scope

Strict XML parsing, download-length validation, typed credential failures, and extended edge-case coverage are in #4348.

[github-actions]

Test Results

0 tests   - 6 107   0 ✅  - 5 964   0s ⏱️ - 4m 23s
0 suites  -   914   0 💤  -   143 
0 files    -   914   0 ❌ ±    0 

Results for commit 66c4376. ± Comparison against base commit 45e3bc2.

♻️ This comment has been updated with latest results.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 86.34361% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.55%. Comparing base (45e3bc2) to head (66c4376).

Files with missing lines	Patch %	Lines
...kotlin/com/wire/kalium/cells/data/CellsS3Client.kt	90.64%	15 Missing and 11 partials ⚠️
...otlin/com/wire/kalium/cells/data/CellsAwsClient.kt	0.00%	16 Missing ⚠️
...otlin/com/wire/kalium/cells/data/AwsSigV4Signer.kt	90.90%	7 Missing ⚠️
.../kotlin/com/wire/kalium/cells/data/KtorOkioCopy.kt	76.19%	3 Missing and 2 partials ⚠️
...tlin/com/wire/kalium/cells/data/S3UploadChannel.kt	44.44%	5 Missing ⚠️
.../kotlin/com/wire/kalium/cells/data/S3UrlBuilder.kt	91.89%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #4262      +/-   ##
=============================================
+ Coverage      61.22%   61.55%   +0.33%     
- Complexity      4223     4257      +34     
=============================================
  Files           2195     2197       +2     
  Lines          71257    71540     +283     
  Branches        7195     7215      +20     
=============================================
+ Hits           43624    44036     +412     
+ Misses         24793    24647     -146     
- Partials        2840     2857      +17     

Files with missing lines	Coverage Δ
.../com/wire/kalium/cells/data/CellsS3HttpTimeouts.kt	100.00% <100.00%> (ø)
...otlin/com/wire/kalium/cells/data/FileDownloader.kt	93.33% <100.00%> (+93.33%)	⬆️
.../kotlin/com/wire/kalium/cells/data/S3UrlBuilder.kt	91.89% <91.89%> (ø)
.../kotlin/com/wire/kalium/cells/data/KtorOkioCopy.kt	76.19% <76.19%> (ø)
...tlin/com/wire/kalium/cells/data/S3UploadChannel.kt	44.44% <44.44%> (ø)
...otlin/com/wire/kalium/cells/data/AwsSigV4Signer.kt	90.90% <90.90%> (ø)
...otlin/com/wire/kalium/cells/data/CellsAwsClient.kt	0.00% <0.00%> (ø)
...kotlin/com/wire/kalium/cells/data/CellsS3Client.kt	90.64% <90.64%> (ø)

... and 4 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45e3bc2...66c4376. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Bencher Report

Branch	${GITHUB_HEAD_REF}
Testbed	ubuntu-latest

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

• Latency (nanoseconds (ns))

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results

Benchmark	Latency	microseconds (µs)
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles	📈 view plot ⚠️ NO THRESHOLD	885.43 µs
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory	📈 view plot ⚠️ NO THRESHOLD	333,674.03 µs
com.wire.kalium.benchmarks.persistence.MessageReadBenchmark.inboxPagingDeepPageBenchmark	📈 view plot ⚠️ NO THRESHOLD	132,105.38 µs
com.wire.kalium.benchmarks.persistence.MessageReadBenchmark.inboxPagingFirstPageBenchmark	📈 view plot ⚠️ NO THRESHOLD	127,172.94 µs
com.wire.kalium.benchmarks.persistence.MessageReadBenchmark.localMarkAsReadBenchmark	📈 view plot ⚠️ NO THRESHOLD	3,385.06 µs
com.wire.kalium.benchmarks.persistence.MessageReadBenchmark.messagePagingDeepPageBenchmark	📈 view plot ⚠️ NO THRESHOLD	27,702.47 µs
com.wire.kalium.benchmarks.persistence.MessageReadBenchmark.messagePagingFirstPageBenchmark	📈 view plot ⚠️ NO THRESHOLD	13,171.69 µs
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark	📈 view plot ⚠️ NO THRESHOLD	1,350,995.01 µs
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark	📈 view plot ⚠️ NO THRESHOLD	21,005.33 µs

🐰 View full continuous benchmarking report in Bencher

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[MohamadJaara]

@claude review

[github-actions]

Review: Shared KMP S3 client for Cells transfers

Reviewed the full diff plus the final state of the changed files. Overall this is a nicely scoped change: it removes the heavy AWS SDK Kotlin dependency (and its version catalog) from :domain:cells, replaces the Apple no-op stubs with a real implementation, and unifies Apple/JVM/Android onto one small SigV4 + S3 client. The SigV4 signer is validated against the official AWS test vector, and the retry-classification tests are thoughtful.

I confirmed the removed awssdk catalog is not referenced by any other module, so that removal is safe, and that getPreSignedUrl was only removed from the impl (it was never part of the CellsAwsClient interface), so nothing dangles.

Findings ordered by impact.

---

High

1. A full token refresh is triggered on every signed request (incl. each multipart part and each retry)

signedRequest() calls credentialsProvider() unconditionally on every request, and on all three platforms that lambda awaits the cells credentials and then calls sessionManager.updateToken(accessTokenAPI, ...) to produce the S3 access key from the fresh access token.

SessionManagerImpl.updateToken() always calls refreshTokenAndPersistSession(...) — a network round-trip to the token endpoint that rotates and persists a new refresh token every time. Consequences:

• A 1 GB multipart upload (~100 x 10 MB parts) forces ~102 token refreshes — one per part, plus create/complete — each a blocking network call. Big latency/throughput regression.
• Because the refresh token is rotated and persisted on each call, rapid back-to-back refreshes risk racing or invalidating each other.

The previous AWS-SDK path used TokenRefreshingCredentialsProvider, but the SDK cached resolved credentials until near expiry, so it did not refresh per request. Recommend caching the resolved S3Credentials and only refreshing when expired (or reactively on a 401), rather than on every signedRequest().

2. Retry backoff is far too short to be useful against throttling

S3_FIRST_RETRY_MAX_DELAY_MILLIS = 10 and S3_SECOND_RETRY_MAX_DELAY_MILLIS = 15. Retryable conditions include 429 TooManyRequests, 503 ServiceUnavailable, and S3 SlowDown/Throttling error codes — but the max backoff is 10 ms then 15 ms (randomized 0..max), with only 3 total attempts. Against throttling this is effectively an immediate re-hammer and will not let the server recover. Suggest exponential backoff in the hundreds-of-ms-to-seconds range with jitter, and make it (and S3_MAX_ATTEMPTS) configurable via CellsS3ClientConfig so tests can keep it fast.

---

Medium

3. No HTTP timeouts configured

Both actuals build a bare HttpClient() with no HttpTimeout plugin. As a result:

• HttpRequestTimeoutException can never be thrown, so that catch branch in performRequestAttempt is dead code.
• Uploads/downloads can hang indefinitely except for whatever default socket timeouts the underlying engine happens to apply (OkHttp has 10s read/write on JVM/Android; the Darwin engine defaults differ). Recommend installing the HttpTimeout plugin with request/connect/socket timeouts.

4. Failed multipart uploads are never aborted

If uploadPart or completeMultipartUpload fails terminally, the exception propagates but no AbortMultipartUpload is issued, leaving orphaned parts consuming storage on the backend indefinitely. Consider wrapping uploadMultipart so a failure after createMultipartUpload triggers an abort.

5. XML parsing is naive and inconsistent

xmlTagValue() matches only bare tags and won't handle namespace-prefixed or attributed tags (e.g. s3:UploadId or UploadId with an xmlns attribute), whereas the error/result detection regexes (S3_ERROR_ELEMENT, S3_COMPLETE_MULTIPART_RESULT_ELEMENT) explicitly do handle namespace prefixes — so a namespaced response could be detected as valid yet yield a null UploadId/Code. Also, createMultipartUpload does not check for an embedded error element in a 200 body (only completeMultipartUpload does), so an error-in-200 would surface as the less helpful "did not include an UploadId". Since the target is arbitrary S3-compatible backends (Cells/MinIO), tightening this is worthwhile.

---

Low / Tests / Nits

6. Test gaps

• The multipart test uses a 9-byte file with maxRegularUploadSize = 1, which produces a single part — the actual chunking loop (part numbering, offset math, readPart across multiple 10 MB chunks) is never exercised with more than one part. Worth a test that produces several parts and asserts sequential PartNumbers and correct reassembled bytes.
• AwsSigningDate.now() / GMTDate.toAwsSigningDate() (month +1 offset, zero-padding) has no test.
• escapeXml and namespaced/attributed xmlTagValue parsing are untested.

7. Download body is streamed outside the retry wrapper — a mid-stream connection drop during copyToSink is not retried (only connection setup is). Acceptable, but note it. There is also no verification that bytes received match Content-Length.

8. Nits

• download's override uses fully-qualified okio.Sink while other okio types are imported — minor inconsistency.
• awaitOrThrow() throws IllegalStateException via error(...); a domain/NetworkFailure-typed failure would integrate better with the surrounding Either flow.

---

The retry classification (retryable vs terminal, exception ordering with RetryableS3Exception before S3RequestException, UploadSourceException/RetryableTransportException handling) is correct and well-tested. The two High items — per-request token refresh and the ineffective backoff — are the ones I would want addressed before merge; the rest are improvements.

_{Automated review}

[MohamadJaara]

Review: Shared KMP S3 client for Cells transfers

Reviewed the full diff plus the final state of the changed files. Overall this is a nicely scoped change: it removes the heavy AWS SDK Kotlin dependency (and its version catalog) from :domain:cells, replaces the Apple no-op stubs with a real implementation, and unifies Apple/JVM/Android onto one small SigV4 + S3 client. The SigV4 signer is validated against the official AWS test vector, and the retry-classification tests are thoughtful.

I confirmed the removed awssdk catalog is not referenced by any other module, so that removal is safe, and that getPreSignedUrl was only removed from the impl (it was never part of the CellsAwsClient interface), so nothing dangles.

Findings ordered by impact.

High

1. A full token refresh is triggered on every signed request (incl. each multipart part and each retry)

signedRequest() calls credentialsProvider() unconditionally on every request, and on all three platforms that lambda awaits the cells credentials and then calls sessionManager.updateToken(accessTokenAPI, ...) to produce the S3 access key from the fresh access token.

SessionManagerImpl.updateToken() always calls refreshTokenAndPersistSession(...) — a network round-trip to the token endpoint that rotates and persists a new refresh token every time. Consequences:

* A 1 GB multipart upload (~100 x 10 MB parts) forces ~102 token refreshes — one per part, plus create/complete — each a blocking network call. Big latency/throughput regression.

* Because the refresh token is rotated and persisted on each call, rapid back-to-back refreshes risk racing or invalidating each other.

The previous AWS-SDK path used TokenRefreshingCredentialsProvider, but the SDK cached resolved credentials until near expiry, so it did not refresh per request. Recommend caching the resolved S3Credentials and only refreshing when expired (or reactively on a 401), rather than on every signedRequest().

2. Retry backoff is far too short to be useful against throttling

S3_FIRST_RETRY_MAX_DELAY_MILLIS = 10 and S3_SECOND_RETRY_MAX_DELAY_MILLIS = 15. Retryable conditions include 429 TooManyRequests, 503 ServiceUnavailable, and S3 SlowDown/Throttling error codes — but the max backoff is 10 ms then 15 ms (randomized 0..max), with only 3 total attempts. Against throttling this is effectively an immediate re-hammer and will not let the server recover. Suggest exponential backoff in the hundreds-of-ms-to-seconds range with jitter, and make it (and S3_MAX_ATTEMPTS) configurable via CellsS3ClientConfig so tests can keep it fast.

Medium

3. No HTTP timeouts configured

Both actuals build a bare HttpClient() with no HttpTimeout plugin. As a result:

* `HttpRequestTimeoutException` can never be thrown, so that `catch` branch in `performRequestAttempt` is dead code.

* Uploads/downloads can hang indefinitely except for whatever default socket timeouts the underlying engine happens to apply (OkHttp has 10s read/write on JVM/Android; the Darwin engine defaults differ). Recommend installing the `HttpTimeout` plugin with request/connect/socket timeouts.

4. Failed multipart uploads are never aborted

If uploadPart or completeMultipartUpload fails terminally, the exception propagates but no AbortMultipartUpload is issued, leaving orphaned parts consuming storage on the backend indefinitely. Consider wrapping uploadMultipart so a failure after createMultipartUpload triggers an abort.

5. XML parsing is naive and inconsistent

xmlTagValue() matches only bare tags and won't handle namespace-prefixed or attributed tags (e.g. s3:UploadId or UploadId with an xmlns attribute), whereas the error/result detection regexes (S3_ERROR_ELEMENT, S3_COMPLETE_MULTIPART_RESULT_ELEMENT) explicitly do handle namespace prefixes — so a namespaced response could be detected as valid yet yield a null UploadId/Code. Also, createMultipartUpload does not check for an embedded error element in a 200 body (only completeMultipartUpload does), so an error-in-200 would surface as the less helpful "did not include an UploadId". Since the target is arbitrary S3-compatible backends (Cells/MinIO), tightening this is worthwhile.

Low / Tests / Nits

6. Test gaps

* The multipart test uses a 9-byte file with `maxRegularUploadSize = 1`, which produces a single part — the actual chunking loop (part numbering, offset math, `readPart` across multiple 10 MB chunks) is never exercised with more than one part. Worth a test that produces several parts and asserts sequential PartNumbers and correct reassembled bytes.

* `AwsSigningDate.now()` / `GMTDate.toAwsSigningDate()` (month +1 offset, zero-padding) has no test.

* `escapeXml` and namespaced/attributed `xmlTagValue` parsing are untested.

7. Download body is streamed outside the retry wrapper — a mid-stream connection drop during copyToSink is not retried (only connection setup is). Acceptable, but note it. There is also no verification that bytes received match Content-Length.

8. Nits

* `download`'s override uses fully-qualified `okio.Sink` while other okio types are imported — minor inconsistency.

* `awaitOrThrow()` throws `IllegalStateException` via `error(...)`; a domain/`NetworkFailure`-typed failure would integrate better with the surrounding `Either` flow.

The retry classification (retryable vs terminal, exception ordering with RetryableS3Exception before S3RequestException, UploadSourceException/RetryableTransportException handling) is correct and well-tested. The two High items — per-request token refresh and the ineffective backoff — are the ones I would want addressed before merge; the rest are improvements.

Automated review

Follow-up on the AI review:
Fixed issues introduced by the Ktor refactor:
Added connection and socket timeouts.
Improved S3 XML parsing and error handling.
Added download size validation and safe streaming behavior.
Added multipart, signing-date, timeout, and XML tests.
Improved missing-credentials errors and minor cleanup.
Not changed because they already existed with the AWS SDK:
Short retry delays.
Multipart uploads are not aborted after failure.