publish() streamed the staged file into the SAF destination with copyTo and had no
answer for a copy that failed partway. The destination volume filling up is the obvious
way in; a provider giving out mid-write is the other. Either way the bytes it had
managed stayed at the name the user picked, while the UI said "Could not save the file".
The user was left holding a truncated file they had just been told was never written,
and nothing in the app would ever tidy it up -- staging cleanup reaches
<cacheDir>/conversions and nowhere else, by design.
So a failed copy now deletes the document. The interesting part is not the delete, it is
what stops it, because removing a file the user already had would be a far worse defect
than the one being fixed.
Only a document URI. DocumentsContract.deleteDocument is the only delete this code has
any right to attempt and it is defined on document URIs, so isDocumentUri() gates it.
That is not a formality: it asks the package manager whether anything answers
ACTION_DOCUMENTS_PROVIDER for the authority, so a file:// path, a MediaStore item or a
content URI from an ordinary provider all fall out here untouched.
Only a destination that was empty when we started. The size is read BEFORE the stream
is opened -- opening for write truncates, so afterwards the question can no longer be
asked -- and the delete runs only when the answer was positively zero. Every
destination that reaches publish() today comes from the SAF CreateDocument contract, so
in practice it is a document this app created seconds earlier; but publish() cannot
verify that from a Uri, and a provider that hands back an EXISTING document for a name
the user re-picked would otherwise have its file deleted rather than merely truncated.
Truncated is bad. Gone is worse, and it is the user's file either way.
That guard fails towards doing nothing. A provider that does not report _size, a query
that returns no row, a resolver call that throws -- all of them land in "not known to
be empty", so the fix is conservative rather than universal: it will not clean up
behind such a provider, and it will not delete anything of theirs either. The defect is
closed for providers that answer a size query; ExternalStorageProvider backs its
documents with real files, so a freshly created one reports 0, but that is reasoning
about it rather than a run against it. Stated here rather than implied, because
"fixed" would overclaim what was verified.
The original failure is what the caller sees. Cleanup runs in its own runCatching and a
throw from it is attached to the original exception as a suppressed one. deleteDocument
reports failure two different ways -- false, or a rethrown RuntimeException -- and
neither is worth failing the save over, because the save has already failed.
ConversionViewModel.save() reports e.message, and "could not delete the half-written
file" is not what to tell someone whose disk just filled up.
Two smaller decisions in the control flow. The whole `use` is guarded, not just copyTo:
a close() that throws while flushing IS the disk-full case and it arrives after copyTo
has returned successfully, so guarding only the copy would miss exactly the failure this
commit is about. The cost is that a file whose every byte reached the provider before a
failing flush is deleted too, which is the right way round -- a flush that failed means
the bytes are not durably there. And openOutputStream's own failure is deliberately
OUTSIDE the guard: nothing has been written at that point, so there is nothing of ours to
remove.
Nothing else in the class moves. hasSpaceFor, discardStaged and sweepStaging are
untouched, and so is save(): the staged file is still deliberately kept on a failed save,
for the reason its own comment gives -- it may be the only copy of an hour of
transcoding, and now the destination genuinely does not have it either.
Seven tests, JVM, Robolectric. The failure has to be injected, which is what shapes them:
Robolectric's ShadowContentResolver consults its registered-stream map before it reaches
any provider, so a test can hand out a stream that writes 512 bytes to a real file and
then throws "No space left on device" while a fake provider answers the size query and
the delete against that same file. The provider is registered twice under two authorities
and two component names, once with the ACTION_DOCUMENTS_PROVIDER intent filter and once
without, which is the only way to have a content URI that is not a document URI. The
delete is observed by the wire names DocumentsContract.deleteDocument actually sends --
"android:deleteDocument" and the "uri" extra -- because both constants are hidden from
the public SDK.
fails partway leaves nothing RED before the fix: expected the delete, got []
close that fails while flushing RED before: the file was still there
cleanup that fails RED before: suppressed was []
already held bytes is not deleted green before the fix -- see below
not a document is left alone green before the fix -- see below
cannot be opened at all green before, and must stay so
succeeds, every byte, no delete green before, and must stay so
The last four are green against the unfixed code for a reason worth writing down: the old
publish() never deleted anything, so every guard passes trivially. They pin the guards
rather than the defect, and pinning is only worth something if the pin is real, so both
were reverted on their own:
- dropping `if (destinationWasEmpty)` fails "a destination that already held bytes is
not deleted" with "a document this app did not create must survive"
- dropping the isDocumentUri() check fails "a destination that is not a document is
left alone" with "deleteDocument has no business on a URI that is not a document
expected:<[]> but was:<[content://...test.plain/document/holiday_plain.mp4]>"
Both were restored. 193 unit tests green.
UnopenableUriTest's unwritable-destination case (an authority with no provider behind it)
is unchanged and keeps passing by two independent routes: the size query on a dead
authority yields "not known to be empty", and the failure itself comes out of
openOutputStream, which sits outside the guard. It is an instrumented test and was
compile-verified here, not executed -- instrumented tests do not run on this host
(CLAUDE.md). Its JVM twin is in the list above.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JMR-devandClaude Opus 5
dbfc463c6d