Stop Android storage entries coming back empty after an abrupt shutdown - #5579
Open
shai-almog wants to merge 17 commits into
Open
Stop Android storage entries coming back empty after an abrupt shutdown#5579shai-almog wants to merge 17 commits into
shai-almog wants to merge 17 commits into
Conversation
A storage entry on Android was written by openFileOutput, which truncates the entry as it opens it, and Android does not flush a file on close. Every write therefore had two windows in which the entry on disk was not the entry the app had stored: while the bytes were being written it was empty or half written, and once it was written it stayed in the page cache for as long as the kernel felt like holding it. An abrupt end to the process or the device inside either window -- a low memory kill, a force stop, a battery pull, a panic -- lost the entry, and on a filesystem that journals the truncation ahead of the data it came back as a zero length file. How wide those windows are is a property of the filesystem and of how eagerly the vendor kills background processes, which is why this only showed up on some devices. The port has carried an fsync for exactly this since the beginning, in closingOutput, whose comment cites Android's own note on the subject. It has never run on this path: its only caller is BufferedOutputStream.close, and the Android storage path hands back the raw FileOutputStream. iOS wraps, so it does call it. An entry is now written to a scratch file that is synced and then renamed over the entry, so the entry changes in one step no filesystem can show half done and the bytes reach the device before that step is taken. Wrapping the existing stream in a BufferedOutputStream would have restored the fsync, but it leaves the entry truncated in place and so leaves the first window open. Three more ways the same data could go missing, found while reading the path around it: Storage.writeObject cached the object before writing it and, when the write failed, deleted the entry through the implementation, which skips the cache. The stale copy then answered every read for the rest of the session, so the failure only surfaced as missing data after a restart. Util.writeObject wrote a map's entry count and then walked the map. A change arriving from another thread in between produced a file whose count did not match its contents, which readObject cannot detect -- it reads exactly count entries off a stream that no longer lines up. The pairs are now collected before the count is written, so the header always describes the payload, and the key and value are copied out of each entry rather than the entry kept, since a Map may hand out one mutable entry for the whole iteration. Preferences.set mutated the map and then called save without holding the lock save takes, which is how a real application reached the case above. It holds the lock across both now, and fires listeners outside it. Preferences is a single file rewritten in full on every set, so this took out every preference at once rather than one. Tests cover the three core fixes; each fails without it. The map case shows the damage is not confined to the object being written: the misaligned stream corrupted the next object read after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b28377d39b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Storage.writeObject returned true before its finally block closed the stream. Closing is where an implementation that replaces the entry in one step does the writing, so a failed fsync or rename reached cleanup(), which logs and swallows, and the method reported a write that never landed while keeping the new object in its cache. The stream is closed on the success path now, where the failure can still change the answer. Scratch files lived beside the entries under a name suffix, and any key ending in that suffix followed by digits -- session.cn1tmp1 -- was taken for one: invisible to exists() and listEntries(), and swept away by the next process. No pattern over a flat namespace can rule that out, so they moved to a directory of their own, where nothing an application can name reaches them. Deleting an entry raced the rename that publishes one: a write already mid close could put back an entry another thread had just deleted. Unlinking the path used to make that impossible on its own, since the write was left holding a descriptor on an inode with no name. Deletion now cancels the open writes for that entry and does so under the lock the rename takes, so the two take turns and the delete stays the later word. StorageOutputStream is static; getContext() is, so it never needed the outer instance (SpotBugs SIC_INNER_SHOULD_BE_STATIC_NEEDS_THIS). Adds the GPLv2 + Classpath Exception header to the two test files that lacked one, for check-copyright-headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
The scratch file was created before the write was registered, so a deleteStorageFile arriving in between found nothing to cancel and the write went on to rename itself over the entry that had just been deleted. Same shape as the race the review caught, moved rather than closed: whether a deletion can see a write is what decides it, so creating the file and becoming visible to a deletion have to be one step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 731413d992
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
…tests Two CI failures on the previous commit. build-test (8): the local that takes the stream over from the finally block tripped PMD's CloseResource, which is on the forbidden list. Suppressed the same way the declaration above it already is; the report is back to zero violations. build-test (17): three ARSessionTest cases assume a batch of implementation events reaches the bridge before the EDT drains it, and nothing arranged that -- the EDT is live, so it can drain between two calls and split a coalesced update into two events. That is correct behaviour for the bridge, which coalesces refinements only while they are still pending, so a fast EDT is allowed to deliver both; the tests were asserting how busy the machine was. They park the EDT while the batch is posted now. Pre-existing, and not from this branch: the same three fail about one run in ten against master's core, which is how it reached this PR looking like a new failure. Verified 0 failures in 12 runs with the fix, and the full suite green on JDK 8 and 17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251bcf8f6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
… Errors Five findings from the second review pass, all of them real. An application may run more than one process, each with its own copy of this class, so the sweep that removed scratch files left by an earlier run was deleting writes another process had in flight. The failed publish then sent writeObject down its error path, which deletes the entry -- a new way to lose data, in the change meant to stop losing it. There is no shared state to coordinate through and hidepid means one process cannot ask whether another is alive, so the sweep goes on age: a day, when nothing legitimate holds a storage stream open for more than moments. clearStorage is inherited and works off listStorageEntries, so a write open against an entry that does not exist yet was invisible to it, survived the clear and published afterwards. Android cancels every open write instead. The scratch directory was itself a legal storage key. An app that already had an entry by that name would find the directory could not be created and every write failing from then on, and on a fresh install that key could no longer be stored at all. No name reserved inside a namespace where every name is legal can be kept clear of the application, so the directory moved out of the files dir to a sibling, where there is nothing to collide with. The scratch file was named after the entry, and an entry name is allowed to reach the filesystem's limit by itself, so appending anything to a long key -- a URL used as a cache key gets close -- pushed it past NAME_MAX and broke a write that used to work. It is named after the writer now, process id and counter, which is a fixed size. Nothing needs the entry name on disk since cancellation became explicit state. writeObject caught Exception, so an OutOfMemoryError partway through left the stream to the finally, which closes it, and closing is now what publishes -- a few bytes of header replacing a good entry. It catches Error too, abandons the write, and rethrows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4d9d9ff1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
openFileOutput refused any entry name holding a path separator, and publishing by rename does not. With name normalization turned off a key like ../shared_prefs/settings.xml reached the rename as written and File resolved it, so the write landed elsewhere in the application's private data and left an entry Storage itself could no longer read or delete. The name is resolved and checked once, when the stream opens. Cancelling a write was still process-local, so a component under its own android:process could delete an entry while another process had a write open on it and get the entry back a moment later. The fix is the property the in-place write used to have for free: deleting an entry now unlinks the scratch files being written for it, whatever process owns them, which leaves that writer holding a good descriptor on an inode with no name and nothing for its rename to find -- the same outcome deleting the open entry used to produce. Scratch files go first so a publish that slips between the two still leaves an entry for the delete to remove. Naming them after a digest of the entry is what makes them findable while staying a fixed width, which the filesystem's limit on names requires. The sweep set a flag once, so a scratch file that was merely too young when a process first wrote was never looked at again for the life of that process, however large it was. It records when the youngest file it kept comes of age instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3c5906ebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…esses Two findings, both real. failedWrite logged before it abandoned the write. Reporting an OutOfMemoryError means building a message and a stack trace, so a second failure there carried off the rest of the method, and the write left open was published by the finally that closes it -- the partial object landing on top of the good entry, which is the case that reordering was meant to prevent in the first place. The entry and the cached copy go first now, and the logging happens after. Cancelling a write across processes worked by unlinking its scratch file, which only reaches the writes that exist when the deletion looks for them. A second process could create its scratch file just after that scan and publish over the entry the deletion went on to remove; clearStorage had the same gap. Creating a scratch file, deleting an entry and publishing a write now all run under a lock the filesystem arbitrates, so they cannot interleave between processes. The system drops that lock when a process ends however it ends, so a crash cannot leave it held, and failing to take it does not fail the write -- a storage that stops writing would be worse than one exposed to a race only a multi-process app can reach. The lock is claimed under the existing monitor and counts its nesting, since a FileLock belongs to the whole VM and cannot be taken twice, and clearStorage claims it and then deletes every entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41a1dbc9d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
clearStorage deleted every file in the scratch directory, the lock among them, while holding that very lock; and the sweep would have aged it out after a day, since nothing ever writes to it. Linux allows a locked file to be unlinked and the lock belongs to the inode rather than the name, so either one would let the next process create the name afresh and take a lock on a different inode. Both processes would then hold "the" lock and neither would wait for the other, which is the whole of what it was there to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d2a356024
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…andle A storage stream that lost its scratch file was treated as cancelled and closed quietly, so writeObject reported success for a value the storage never took. Deletion from another process is the case that was meant for, and failing instead reaches the same end -- writeObject deletes the entry when a write fails -- while telling the caller the truth. Everything else that could remove the file now gets the same honest answer rather than the silent loss this whole change exists to stop. Cancelling within the process stays quiet, because there the outcome is already known: the caller either asked for the entry to go or is abandoning the write itself. The sweep also skips the writes this process has open. Age cannot tell them apart on its own, since lastModified is a wall clock reading and a clock that jumps forward makes a file being written this moment look like a day old; what this process is doing it knows exactly. The lock handle is kept before the lock is attempted rather than after it succeeds, so a lock that throws still leaves something to close. A filesystem that refuses to lock was leaking a descriptor per storage operation until unrelated files stopped opening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd1ef9443b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Both of the outstanding findings come from the same place: writeObject deleted the entry whenever a write failed. That was right when the write went into the entry, since a failure left half an object there and deleting was the only way to be rid of it. It is wrong now that the value is assembled elsewhere and put in place in one step, because the entry was never touched -- so running out of memory partway through a large object, or having a scratch file swept, answered a failed write by destroying the good value that was already stored. Worse than the failure it was reporting. Ports say which they are: abandonStorageWrite discards the pending write and reports that the entry was left alone, and the default still answers that the caller has to delete. The cached copy goes either way, since the object never reached the storage. The sweep no longer judges a scratch file by its age. Age was the only thing separate processes could agree on, but lastModified is a wall clock reading and a clock that jumps forward makes a file being written this moment look arbitrarily old -- which is how a second process came to delete writes that were still in progress. Each process now holds a lock on a file named for it for as long as it runs, and the sweep asks the filesystem whether the owner of a scratch file is still there. The system drops that lock however a process ends, so it cannot outlive what it stands for, and anything the sweep cannot determine counts as running. What remains of the interval is a rate limit on the monotonic clock, never a judgement about a file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e703d23d06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Clearing threw away every file in the scratch directory, the liveness markers among them. A process whose marker is taken from underneath it goes on holding the lock, so it never notices and never makes the name again, and from then on every other process reads it as gone and feels free to delete the writes it has in flight. Same shape as the lock file two changes ago, so the exclusion is now a question about markers rather than about one name: clearing throws away the writes and nothing else, and the sweep stays the only place a marker is removed, once its owner is known to be gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69b149099e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A failed write was abandoned by entry name, so when two threads wrote the same entry and one of them failed, the other was given up along with it: it skipped publication, closed without complaint, and its writeObject reported success for a value that had been discarded. The write is named by its stream now -- Storage keeps the one the implementation handed it -- so only the write that failed is given up. The sweep passes over anything carrying its own process id, on the grounds that a process knows its own work. Android hands a process id out again once its holder is gone, so after a crash or a reboot that assumption covered files an earlier incarnation had abandoned, and they would have sat there for good. Claiming liveness now clears whatever is already present under this process's id, which happens before its first write, when it owns nothing and anything there must belong to the incarnation before it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2680be8d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lues The atomic replacement was applied to createStorageOutputStream, which backs a public streaming API where a caller may hold the stream open and read back what it has flushed. The log writer does exactly that: it keeps the stream for CN1Log__$ open for the life of the application and only flushes, while sendLog reads that entry behind its back. An entry that appears only on close left the log unreadable, sendLog uploading the previous session or nothing at all, and every line written since the process started lost when it ended. That is a regression this change made, and a bad one, since the log is what a crash is diagnosed from. The two are separated now. createStorageOutputStream writes into the entry as it always did, and gains only the flush on close that Android does not do, which changes nothing about when what is written can be read. writeObject asks for the other form, where the whole value is assembled elsewhere and put in place as one step. That matches what each is for: a value written in one go is never wanted half written, and a stream held open is no use if nothing can read it. The lock helpers report through Android's log rather than ours. Ours writes through storage, so a failure to take the lock would have been reported by a path that comes back through the same code with the depth still at zero, fails again the same way, and does not stop until the stack does. Adds the test that the round of fixes for it never had: a write that fails leaves the value that was already stored where it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db1ac3ec89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Finding an owner gone and deleting its files are two steps, and Android hands a process id out again the moment its holder is gone. Between the liveness check and the delete, a process could be given the id just examined, say it was running and begin a write, and have the sweep unlink the write it had only just started -- or the very file it had said it was alive with, after which every later sweep would take it for gone and delete whatever it was writing. Creating a scratch file was already serialized between processes; the sweep and the liveness claim were not. Both take that lock now, so deciding an id is dead cannot land in the middle of another process claiming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63a28ba310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…l alone Closing the handle gives up the lock, but the field saying this process held it was left set. Every later claim then returned as though the lock were still held, while no lock existed -- so every other process read the .live file as unlocked, took this process for gone, and was free to delete the writes it had in flight. Both fields are cleared now. The failure that prompted it could only happen because clearing up after the previous holder of this process id ran inside the same try as the claim itself. It has its own now: the claim has already succeeded by that point and is not worth giving up because a leftover file would not delete. Those keep until a later sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87a517e6f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
setStorageInstance is there so an application can wrap the bytes, seamless encryption being the case its own documentation names, and writeObject has always gone through the subclass's createOutputStream. Asking the platform for the stream directly walked past that: the value went to the store unwrapped while reads went on expecting otherwise, so what came back could not be decoded. writeObject asks createOutputStreamForWrite now, which is overridable. Its default hands back the platform's replace-on-close stream only for Storage itself; a subclass keeps the stream it has always been given, and can override the new method to wrap that one and have both. The test writes through a Storage that inverts every byte and reads it back, so a write that skipped the wrapper fails to decode. Verified against the previous revision, where it fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42f5ee3119
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Clearing what the last holder of this process id left behind rested on the process owning nothing yet, which is true on the first write and not afterwards: a claim that fails is retried by the next write, and by then there can be writes open under the same id. The cleanup deleted their scratch files, so a write that had serialized perfectly well failed when it came to publish. It leaves the writes it knows about alone now, which it can do exactly rather than by inference. A write still goes ahead when the liveness claim fails, and the reason is written where the decision is. A claim can only fail where the filesystem will not lock, and refusing to write there would turn that into an application unable to store anything at all -- worse than the cost, which is that a process sweeping at that moment may take the write for abandoned and unlink it. That fails the write, honestly, and leaves what was already stored where it is, and the next write claims again. It is the same trade the cross process lock already makes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Reported through support: a banking app keeps a public key and some registration flags in
Storage, and the key goes bad after a while — on a few Android devices only.The reported bug
createStorageOutputStreamhanded back the raw stream fromopenFileOutput, which truncates the entry as it opens it, and Android does not flush a file on close. Every write had two windows in which what was on disk was not what the app had stored:An abrupt end to the process or the device inside either window — a low memory kill, a force stop, a battery pull, a panic — lost the entry, and on a filesystem that journals the truncation ahead of the data it came back as a zero length file. How wide those windows are is a property of the filesystem and of how eagerly the vendor kills background processes, which is why this only showed up on some devices.
The port has carried an fsync for exactly this since the beginning, in
closingOutput, whose comment cites Android's own note on the subject. It has never run on this path — its only caller isBufferedOutputStream.close, and this path never wrapped. iOS wraps, so it does call it (and its nativewriteToFileisNSAtomicWritebesides).An entry is now written to a scratch file that is synced and then renamed over the entry, so the entry changes in one step no filesystem can show half done, and the bytes reach the device before that step is taken. Wrapping the existing stream in a
BufferedOutputStreamwould have restored the fsync, but it leaves the entry truncated in place and so leaves the first window open.The rest of the Android change falls out of that: scratch files stay invisible to
listStorageEntries/storageFileExists/getStorageEntrySize,deleteStorageFilecancels a write still open against the entry (otherwiseStorage.writeObject's error path deletes the entry and then has the failed bytes renamed over it), and a sweep clears scratch files orphaned by a previous crash.Three more ways the same data could go missing
Found while reading the path around it. Each has a test that fails without the fix.
Storage.writeObjectkept a stale cache entry after a failed write. It cached the object before writing, then on failure deleted the entry through the implementation, which bypasses the cache. The stale copy answered every read for the rest of the session, so the failure only surfaced as missing data after a restart — which is what "corrupted after some time" looks like from the outside.Util.writeObjectcould write a map header that did not match its payload. It wrote the entry count and then walked the map; a change arriving from another thread in between produced a filereadObjectcannot detect as bad — it reads exactly count entries off a stream that no longer lines up. The pairs are collected before the count is written now. The key and value are copied out of each entry rather than the entry kept, since aMapmay hand out one mutable entry for the whole iteration.Preferences.setmutated the map and then calledsavewithout holding the locksavetakes — which is how a real application reaches the case above. It holds the lock across both now and fires listeners outside it.Preferencesis a single file rewritten in full on everyset, so this took out every preference at once rather than one.Testing
core-unittests: 5195 passandroidandcore-unittestscheck-cast-semantics.shclean, no baseline changeAndroidImplementation.javais CRLF; the diff is 229 lines, line endings byte-preservedThe Android durability fix itself is not reachable by a JVM test — it needs a device losing power mid-write. Repro is
adb shell 'echo c > /proc/sysrq-trigger'(oradb emu kill) while a write loop runs; a plainam force-stopwill not show it, since the kernel still flushes.🤖 Generated with Claude Code