Release v2.87.1 — previews that work on old files and large ones - #2204
Merged
Merged
Conversation
* fix: a wallet with enough drives could not sync at all
The transaction-parse budget is shared out across the drives still to be
synced, and the sharing is integer division:
transactionParseBatchSize: 200 ~/ (drivesCount - drivesSynced)
Past 200 remaining drives that rounds to **zero**, and `BatchProcessor`
rejects a batch size of zero outright - `ArgumentError('Batch size cannot be
0')`. So the sync did not degrade at that scale, it stopped, and it stopped
with an argument error that names nothing about drives or batching.
Clamped to at least one. A batch of one is slow; a batch of zero is a crash.
The subtraction is guarded too: `drivesSynced` reaching `drivesCount` would
divide by zero, which fails the same way for the same reason.
**No wallet is known to hold that many drives**, so this is a floor under a
future scale rather than a fix for anyone's sync today - which is why it is
one expression on its own rather than part of the larger sync work in #2162,
where it has been sitting.
The policy moves onto `SyncRepository` as `transactionParseBatchSizeFor`, so
what the number means can be read and tested without standing up a sync. Its
test fails without the clamp.
* test: consume the batch stream, so the guard it checks actually runs
`BatchProcessor.batchProcess` is `async*`, so calling it runs none of its
body - including the `batchSize` guard the test existed to exercise. Asserting
`returnsNormally` on the call therefore asserted nothing: the test passed with
a batch size of zero, which is the single value that guard rejects. Verified
by forcing zero and watching it pass.
Now consumed with `expectLater(..., emitsDone)`, and the input list is mutable
because `batchProcess` clears it. Forcing zero now fails the test.
Raised by CodeRabbit.
* fix: a private AES-CTR file could never be previewed `cipherBufferImpl` knew only AES-GCM. CTR was there but commented out, with a note that the implementation "generates a 16 byte nonce by default" - so every private AES-CTR file threw `ArgumentError` from that lookup, one line before the `switch` in `decryptTransactionData` that handles CTR carefully. The preview swallowed it and said "this file can't be previewed here", which reads as an unsupported format rather than a decryption that never happened. Downloading the same file worked throughout, because downloads decrypt through `cipherStreamDecryptImpl`, which has always implemented CTR. The note was true of the wrong thing. `newNonce()` does return sixteen bytes where ArDrive's `Cipher-IV` is twelve - but that is nonce *generation*, and this function is only ever used to decrypt. `AesCtr` zero pads a short nonce into the counter block, so the twelve bytes a transaction carries already are the block its data was encrypted under. Checked rather than assumed: encrypting under a 12 byte nonce and under that nonce followed by four zero bytes produces identical ciphertext, so no counter block has to be assembled. Reachable by any file an older client wrote: 2.19.1 used CTR at 47 MiB, a size today's uploader writes as GCM. Also makes the failure audible. `_decodePrivateData` returned `null` in silence, and the only thing downstream is the same "can't be previewed" message - so a decryption failure was indistinguishable from an unsupported type, in the logs as much as on screen. That silence is why this took a gateway-timeout investigation to find. * fix: adding CTR to the buffered map must not enable it for writing Found reviewing my own change. `cipherBufferImpl` feeds the two buffered *encrypt* functions as well as the decrypt one, so putting AES-CTR in its map did more than make private CTR files previewable: it made `createEncryptedTransaction(cipher: aes256ctr)` succeed, where it used to throw. That is worse than the bug it replaced. `Cipher.encrypt` generates its own nonce when none is given, and `AesCtr`'s is 16 bytes where every ArDrive reader requires 12 - `AesCtrStream` throws on any other length. Such a call would have tagged `Cipher-IV` with sixteen bytes and written a file to Arweave that nothing could ever decrypt, silently and permanently. Not reachable today: every caller takes the GCM default and `data_bundler` hardcodes it. But it turned a loud `ArgumentError` into a latent trap, and this is exactly the hazard the original note on `cipherBufferImpl` was gesturing at - it just belongs on the encrypt path, where choosing the nonce is impossible, rather than as an absence on the decrypt path, where all it did was make private CTR files unpreviewable. Encryption now goes through `cipherBufferEncryptImpl`, which is GCM only and says why.
* fix: a preview timeout that measured duration, not silence
A 48 MiB file could not be previewed from any gateway. Both attempts died at
exactly the budget:
Gateway turbo-gateway.com failed for tx Y2qNfMZM...: TimeoutException
after 0:00:10.000000: Future not completed
Gateway permagate.io failed for tx Y2qNfMZM...: TimeoutException
after 0:00:10.000000: Future not completed
Nothing was cold and nothing was broken. `getSandboxedTx` buffers the whole
body behind one `Future`, and a `Future` timeout is a deadline on the
*transfer* - so ten seconds meant "this file must arrive at 5 MB/s". Previews
are capped at 100 MiB, so any large file was abandoned mid-body by every
gateway in turn while it was arriving perfectly well. The user saw
"unpreviewable", and a second refresh sometimes beat the clock, which is why
it looked intermittent.
The waterfall now reads the body as a stream, and a `Stream` timeout fires
only when no chunk has arrived for that long. The budget means silence, which
is what a timeout should mean: a slow body finishes, a dead gateway is still
dropped in the same ten seconds. `BrowserClient` reads through a
`ReadableStream`, so this is chunk-wise on the web too, not only on the VM.
No size thresholds and no larger per-read budget: those would only have moved
the cliff. The one constant that did change is the waterfall total, from 25s
to a 3 minute backstop - at 25s it was itself a deadline, needing 2 MB/s for
the same file.
Scoped to the preview waterfall. `_syncFetch` keeps the buffered read and its
own budget: sync's reads are a few hundred bytes each and there are hundreds
of them, so a wall clock is the right shape there.
Tests cover both halves - a body arriving in chunks over 500ms completes under
a 200ms budget, and a gateway that goes quiet for longer than its budget is
still dropped. The first fails if the timeout goes back to being a deadline.
* fix: hang up on a read the backstop has given up on
`Future.timeout` does not cancel what it times out. The waterfall's total
budget therefore only told the *caller* the read had failed - the read itself
carried on, buffering toward the 100 MiB preview cap and holding its
connection until the stream settled on its own.
That was nearly harmless while the total was 25s and every attempt was capped
at 10s of wall clock. It is not harmless now the total is a three minute
backstop and reads are allowed to run as long as bytes keep arriving.
The waterfall now borrows one client for its whole run, and the backstop
closes it - which aborts the request in flight, since `BrowserClient.close`
fires its `AbortController`. Reusing one client across the attempts is also
one connection rather than four.
Raised by CodeRabbit.
* fix: release the body of a gateway response nobody will read
Found reviewing my own change. `getSandboxedTx` consumed the body whatever the
status; streaming does not, and the non-2xx branch threw without touching
`response.stream` at all.
That matters more than it would have before, because the client is now shared
by every gateway in the waterfall: an abandoned error response held its reader
open for the rest of the run rather than until the end of its own attempt. Up
to four of them, and a gateway that answers an error with a large page holds
that much.
Cancelled rather than drained - there is no reason to download a body we have
already decided not to use.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
# Conflicts: # pubspec.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v2.87.1
dev→master. Version2.87.1(pubspec.yaml:6). Deployed and verified on staging atc6c83e75a.Three fixes, all of them things a user could hit. Two are previews: a large file, and a file uploaded by an old client. The third is a crash that no wallet is big enough to reach yet.
👀 A private file uploaded by an older client can be previewed again
cipherBufferImplknew only AES-GCM. AES-CTR was there but commented out, so every private CTR file threwArgumentErrorfrom that lookup — one line before theswitchthat handles CTR carefully. The preview swallowed it and said "This file can't be previewed here", which reads as an unsupported format rather than a decryption that never happened.Downloading the same file worked throughout, because downloads decrypt through a different implementation that always supported CTR. That asymmetry — downloads fine, previews not — is what located it.
Today's uploader writes GCM below 100 MiB and CTR above, but older clients wrote CTR at any size: the file that surfaced this was 47 MiB, written by ArDrive-App 2.19.1.
The note that kept CTR out said the implementation "generates a 16 byte nonce by default". True of generating one, and irrelevant to decrypting:
AesCtrzero pads a short nonce into the counter block, so the 12 byteCipher-IVon a transaction already is the block its data was encrypted under. Verified rather than assumed — encrypting under a 12 byte nonce and under that nonce plus four zero bytes gives identical ciphertext.Encryption is unchanged. Adding CTR to that map also, accidentally, let
createEncryptedTransaction(cipher: aes256ctr)succeed where it used to throw — which would have written a file whoseCipher-IVno reader accepts, permanently and silently. Buffered encryption now goes through a GCM-only path that says why. No caller could reach it, but the guard belongs there.⏱️ A large file is no longer abandoned while it is still arriving
A 48 MiB preview died at exactly ten seconds on every gateway in turn:
Nothing was slow or broken. The body was buffered behind a single
Future, and aFuturetimeout is a deadline on the transfer — so ten seconds meant "this file must arrive at 5 MB/s". Previews are capped at 100 MiB, so any large file was abandoned mid-body by every gateway while it was arriving perfectly well. A second refresh sometimes beat the clock, which is why it looked intermittent.The waterfall now reads the body as a stream, where the budget measures silence rather than duration: a slow body finishes, and a gateway that has actually stopped answering is still dropped in the same ten seconds. No size thresholds and no larger per-read budget — those only move the cliff.
Also here: the waterfall's own backstop now hangs up on a read it has given up on.
Future.timeoutdoes not cancel what it times out, so a read could carry on buffering toward the preview cap long after the caller was told it had failed.⚡ Sync at scale
The transaction-parse budget is shared across the drives still to sync, by integer division — which reaches zero past 200 remaining drives, and a zero batch size is rejected outright. Sync would not have slowed at that scale, it would have stopped, with an error naming nothing about drives or batching.
No wallet is known to hold that many drives, so this is a floor under a future scale rather than a fix for anyone's sync today.
Verification
flutter analyzeclean; 1441 passing / 4 skipped, plusardrive_crypto(46) andardrive_uploader(57). The CTR fix was confirmed against a real 47 MiB private video on a preview build.Included: #2201, #2202, #2203.