Classify 507 Insufficient Storage, and stop the four places that swallowed it - #95
Merged
Conversation
The homeserver answers 507 Insufficient Storage once an account is over its quota — 1GB on the free tier — with the plain-text body "Disk space quota exceeded". Nothing in Loopky matched it, so it fell through toErrorReason()'s else branch to ErrorReason.Unknown and rendered as "Something went wrong. Please try again." A retry is the one action that cannot possibly work. Adds isQuotaExceeded() and ErrorReason.StorageFull, with copy on both platforms that names the two remedies that do work: delete a deck, or buy more room. Three details worth keeping: - The classifier runs ahead of isRateLimited/isNetworkFailure. Nothing they match collides with the 507 body today, but "quota" is the word a future bandwidth limit will also reach for, and reading a full disk as "the server is busy" would retry against it forever. - It matches 507 as a status code, not as a substring. Every failure message carries a pubky:// URL and ids are random alphanumerics, so a bare "507" in msg would send someone off to delete decks over a missing record. - withWriteRetry now returns on a quota error before the rate-limit branch can claim it. It already didn't retry 507 in practice; this makes that a property of the code rather than of the exact wording the homeserver happens to use. iOS ErrorMessages.swift ends both switches in `default:`, so the new variant would have compiled there and silently rendered the generic copy — the case is added in the same change, as that file's own warning asks. Refs #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…don't back off Both workers mapped every failure to Result.retry() so WorkManager's exponential backoff owned the pacing — deliberately, because a deck stopped by its chunk budget and one stopped by an outage both want another pass later. A 507 wants neither. It succeeds only after the user deletes something, so the retry chain never converges; it just wakes the process up forever to reach the same answer. Media re-hosting is the worse of the two, because it is itself a quota consumer: cloning a media-heavy deck pins its blobs to the source author, and this sweep later copies each one under your own pubky. So the mechanism that fills the quota is the same one that then retries against it. DeckMediaSweeper now ends the pass on the first 507 rather than trying every remaining blob into a full disk. It banks the progress it did make first — the commit still runs, the cursor still stops short of the unswept chunk — and then rethrows, because the outcome struct cannot express the difference the worker needs: `complete = false` means "come back later", and this means "stop". Compaction cannot fix this on its own either. A merge writes the landing chunk before emptying the source, so it temporarily grows usage — at a full quota the one job that would reclaim space is the one that cannot run. Refs #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ublish resolveDraftImage and resolveCoverImage both degraded a failed upload to null and carried on. So a 507 mid-publish produced a deck that looked successfully published, sat in the library, and was quietly missing the images the user had attached during triage — with nothing shown at any point. They now throw, and the publish aborts before the deck is written. Aborting orphans anything that already landed: media goes up before publish() writes the #49 marker manifest, so deckRepository.delete has no manifest to walk. The refs are tracked as they land and removed by hand instead, which matters most in exactly the case that triggers this — the disk being full is a poor moment to leak blobs. The chunk and manifest failures did surface, but as a raw err.message pushed into state.error: the FFI's diagnostic text ("HTTP transport error: error sending request for url (https://_pubky.rc3om…)") reaching the UI, which is the thing ErrorReason exists to prevent. state.error is now a PublishError. PublishError keeps the three failing steps apart instead of folding them into one ErrorReason, because the consequence differs and the copy has to say which: a failed publish leaves nothing, a failed cancel leaves the partial deck on the homeserver, and a failed undo leaves the deck published. Refs #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t take writes This was the worst of the four 507 swallow sites, and silent by construction. SrsRepository buffers graded reviews and writes them a chunk at a time. flush() is careful — on failure it puts the pending reviews back in the dirty set so the next flush retries. But that set is in memory only, and flushAsync threw the error away with a log line. At a full quota every flush 507s, every retry re-queues, and the whole set dies with the process. The user studies a full session, watches the counters move, restarts the app, and the progress is gone, having seen nothing go wrong at any point. Two halves, and both are needed — surfacing an error about work you have already lost is not much better than silence: The reviews now outlive the process. PendingReviewStore mirrors the unflushed buffer to SharedPreferences/NSUserDefaults, written on every grade rather than only on the failure path, since the window it closes is the process dying. It is restored before anything reads the cache, so a rebuilt queue doesn't re-show cards that were already graded, and cleared by the first flush that succeeds. It records each review's chunk rather than recomputing it, because the chunk is assigned when a state is first written and a restore that guessed differently would file the review into the wrong record. And the failure is now visible. flushAsync emits on SrsRepository.flushFailures, replayed so the study screen still learns about a flush that failed as the screen was going away. The synchronous path was swallowing too: the automatic every-20 flush comes back through review(), and onGrade logged and dropped it. The study screen shows a banner rather than an error state. The session is still worth finishing — the reviews are buffered and on disk — so blanking the card someone is mid-way through would cost them more than the warning is worth. It survives the next card, because emitCurrent rebuilds the state on every grade. Android's commit() and iOS's synchronize() are deliberate: the journal is written as a session ends, which is exactly when the process may not live long enough for a deferred write. Refs #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Architecture.md §8.5 collects what the homeserver actually does with a full quota and why each of the four handling decisions is the way it is — chiefly that 507 is the one terminal failure among a set of transient ones, and that the two background jobs which look like they would reclaim space (media re-hosting, chunk compaction) both consume it instead. Also writes down the three things still unmeasured, so the next person does not guess at them: how close 1GB really is (audio is uploaded uncompressed and dedupe is per-deck, so a single imported deck may be a meaningful fraction of the free tier), the separate bandwidth quota off the same signup token, and the 100MB request body cap. The first wants a real probe against a test homeserver and an admin-API read of storage_used — it cannot be answered from the client. Refs #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…an expiry Two things found by driving the journal on a device rather than trusting the tests, which pass either way. Recovered reviews were restored but not sent. Nothing would send them: a flush needs either a closing study screen or FLUSH_EVERY more grades, so a user who studied offline and never opened another session would keep a journal that was recoverable but never recovered. Building the due queue now sends what it recovered. restoreJournal reports whether it found anything rather than flushing itself, so the call from inside flush() cannot recurse. And the banner exposed a misclassification it did not cause. Offline, the FFI reports "Failed to import session: Request failed: HTTP transport error: error sending request for url (…/session)" — which contains both "session" and "import", so isSessionExpired matched it. On device that rendered as "Sign in with Pubky Ring again" to a user whose only problem was airplane mode; worse, requiresReauth would have signed them out over a dropped connection. A request that never reached the homeserver says nothing about the session, so a transport failure is now excluded before the substring match. Verified on device: 24 reviews graded offline survived a force-stop, were restored on relaunch, and reached the homeserver once the network was back. Refs #91 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
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.
Closes #91 (code); the measurement pass and the upstream ask are left open — see below.
The homeserver enforces a per-user storage quota and answers 507 Insufficient Storage on any write over it. Loopky had no classifier for it, no copy for it, and the four places most likely to hit it all swallowed the failure.
What changed
1. It has a name now.
isQuotaExceeded()+ErrorReason.StorageFull, with copy on both platforms that names the two remedies that actually work — delete a deck, or buy more room — instead of "please try again", which is the one action that cannot possibly succeed.Two details worth reviewing:
isRateLimited/isNetworkFailure. Nothing they match collides with the 507 body today, but "quota" is the word a future bandwidth limit will also reach for, and reading a full disk as "the server is busy" retries against it forever.507as a status code, not a substring. Every failure message carries apubky://URL and ids are random alphanumerics, so a bare"507" in msgwould send someone off to delete decks over a missing record. There is a test for exactly that.withWriteRetrynow returns on a quota error before the rate-limit branch can claim it. It already didn't retry 507 in practice; this makes that a property of the code rather than of the wording the homeserver happens to use.2. Study progress no longer dies silently. This was the worst of the four and silent by construction:
flushAsynclogged the failure and dropped it, the reviews went back into an in-memory dirty set, every retry hit the same 507, and the whole set died with the process. A user could study a full session, watch the counters move, restart, and find the progress gone having seen nothing go wrong.Both halves are needed — surfacing an error about work already lost is not much better than silence:
PendingReviewStoremirrors the unflushed buffer to SharedPreferences/NSUserDefaults, written on every grade rather than only on the failure path, since the window it closes is the process dying. Restored before anything reads the cache, sent immediately on recovery, cleared by the first flush that succeeds. It records each review's chunk rather than recomputing it, because a restore that guessed differently would file the review into the wrong record.SrsRepository.flushFailuressurfaces it, replayed so the study screen still learns about a flush that failed as the screen was going away. The synchronous path was swallowing too — the automatic every-20 flush comes back throughreview(), andonGradelogged and dropped it.3. The background jobs stop instead of backing off.
Result.failure(), notResult.retry()— WorkManager's exponential backoff against a permanent condition never converges.DeckMediaSweeperalso ends the pass on the first 507 rather than trying every remaining blob, banking its progress first. This compounds: re-hosting is a quota consumer, so the mechanism that fills the quota was the one retrying against it. And compaction can't dig you out either — a merge writes the landing chunk before emptying the source, so it temporarily grows usage.4. Publish no longer ships a deck missing the user's images.
resolveDraftImage/resolveCoverImagedegraded a failed upload tonulland carried on, producing a deck that looked successfully published and was quietly missing the images picked during triage. They now throw and the publish aborts. Anything already uploaded is swept by hand, because media goes up before the #49 marker manifest anddeletehas no manifest to walk — which matters most in exactly this case, since a full disk is a poor moment to leak blobs.state.erroris now aPublishErrorrather than a rawerr.messageputting FFI diagnostic text on screen.Found by driving it on a device
Two things the tests passed straight through:
"Failed to import session: … HTTP transport error", which contains both "session" and "import" — soisSessionExpiredmatched it. The new banner rendered "Sign in with Pubky Ring again" to a user whose only problem was airplane mode, andrequiresReauthwould have signed them out over a dropped connection. Pre-existing; the banner is just what made it visible.Verified on device (emulator, real homeserver): journal written live on each grade and cleared by a successful flush; 24 reviews graded offline survived a force-stop, were restored on relaunch, and reached the homeserver once the network was back; the banner renders over a live session without ending it.
Deliberately not done
storage_used— it cannot be answered from the client. What is known is written down inArchitecture.md §8.5so the next person doesn't guess: audio is uploaded uncompressed, dedupe is per-deck not per-user, and a clone re-hosts its own copy. Until there is a number, no copy should promise a deck count.🤖 Generated with Claude Code