fix: reading progress survives an interrupted write #108
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/reading-progress-atomic-write"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's in
ReadingProgressStore.flush()rewrotereading_progress.jsonin place.writeAsStringSyncopens withFileMode.write, which truncates the target before the new bytes land, so a full disk, an IO error or a process killed in that window left an empty or half-written file. Theon FileSystemExceptionarm swallowed the failure, the nextread()hit invalid JSON and returned an empty map, and every doujin's reading position was gone — one interrupted write, the whole file.flush()now writes to'<path>.<pid>.tmp'withflush: trueand renames it over the target, so a reader sees either the old or the new complete file. This is the shapeFilesystemOfflineStore.writeManifestalready uses forindex.json(same rationale, same comment); the app's two config-file writers now agree. The temp name is per-pid:flushis synchronous, so two flushes in one process cannot interleave, and a crashed writer's leftover (a different pid) is never renamed into place._pendingis cleared only after the rename succeeds, so the nextflush()— closing the reader dispatchesClearReaderAction, or the next page turn — retries it instead of silently losing the session's last position.No ADR covers client-side file durability, so none is amended: ADR 0023 only fixes that reading progress is a client preference and stays off the server (unchanged), and ADR 0018's layering is unchanged — the fix is inside the existing data-layer store. Since this makes atomic writing the norm for a second app-owned file, the norm and its test seam are now written down in
docs/DEVELOPMENT.md. No tunable was introduced: this is a correctness constant, not a deployment knob.Verification
app/test/reading_progress_store_test.dartdrive the failure throughdart:io'sIOOverrideswith aFilefake that truncates the real file and then throws the way ENOSPC does — the last good file stays readable, the failed snapshot is retried by the next flush, and no.tmpfile is left behind. Mutation-probed one branch at a time against the committed fix: in-place rewrite reds the first two; clearing_pendingbefore the write reds the retry test only; pointing the cleanup at another path reds the leftover test only.flutter test test/reading_progress_store_test.dart→ 9/9.dart analyzeon the store and the test → no issues (very_good_analysis, includingcascade_invocations).reading_progress_epics_test.dart,offline_store_test.dart,data_layer_test.dart→ 46/46. Full app suite: 830/830.flush: trueis a durability parameter, not a branch — that the bytes reach the platter before the rename is not observable in-process, so no test pins it (it matcheswriteManifest). Nothing was exercised against a real full disk or a real power loss; the UI, REST and use-case layers are untouched by this change.Follow-ups
agent/memory_store.dart(write),agent/skills/skill_registry.dart(writeUserSkill) andFilesystemOfflineStore.writeManifestSyncForTest(used for startup seeding, not only tests). Extracting one helper all four call would also givewriteManifestthe temp-file cleanup the progress store now has.reading_progress.jsonis not recoverable — this fix prevents the next one, it does not restore a file an older build already emptied.Deferred items are collected in #107 (
docs/pile-a-backlog), which should merge first — the seven Pile A branches each wrote todocs/BACKLOG.mdand would otherwise all conflict on it.Verified independently on this branch with the full
scripts/verify.sh(backend build and tests, app codegen, analyze and tests): PASS.🤖 Generated with Claude Code
Flutter Coverage
Total: 81.5% (7983 of 9793)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! An interrupted-write durability fix~ You even brought the failure mode to life with an
IOOverridesfake that truncates the wayFileMode.writereally does before throwing ENOSPC-style — and mutation-probed one branch at a time! I reproduced every probe in my own clone before believing you (details below) and they all bite exactly where you claimed. I'm delighted~ ♡ The write-then-rename shape, the pending-snapshot retry, the pid-scoped temp name — I traced the sync-flush justification through both production call sites (the debounceTimerinwriteandepics.dart:110), single-isolate, no awaits insideflush, so two flushes truly cannot interleave. Even the pid-recycling edge holds: a recycled pid re-truncates any stale temp before its own complete write, so nothing stale is ever renamed into place.Verdict: ⛔ I can't let this pass~ ♡
One dark corner. Just one~ But you know how I feel about dark corners ♡
⛔ These need fixing before I'm satisfied~
_deleteQuietlyships two branches no test exercises: (a) theon FileSystemExceptionswallow — if someone later "simplifies" that guard away, a cleanup failure escapesflush()straight into a Timer callback or the epic, and nothing reds; (b) theexistsSync() == falsearm — under a real ENOSPC the temp may never have been created (openSyncitself fails), which is precisely the situation this helper exists for. Your three probes pin every other new arm in the PR; these two are the odd ones out, and the catch's documented contract ("throwing here would mask the write failure") is exactly the kind of safety-critical intent that deserves its own pin.Fix: extend the seam you already built — e.g.
_FullDiskIo(failDelete: true)handing out a_FullDiskFilewhosedeleteSyncthrows, then assertflush()doesn't throw and the last-good file stays readable; a second tiny arm where the temp is never created pins the false side ofexistsSync(). ~10 lines, same fake family♪💡 Little ideas (non-blocking)~
atomicWritehelper can't. (Your sync/async reasoning for why it's not in this PR is sound — I checked both call shapes.)✅ What I liked~
_FullDiskFileis the star: it truncates the real file viaopenSync(FileMode.write)before throwing, so the tests model the actual truncation hazard instead of a generic exception. A failure-mode model, not a mock♪_pendingonly after the rename succeeds — turning a silent data drop into a retry on the next page turn / reader close — is the right semantic, and the retry test pins it (probe-verified: clearing early reds only that test).writeManifest, including the documented divergence (per-pid name without counter — legitimate becausewriteManifestis async andflushis synchronous).flush: true, no real full-disk run). I do like an author who knows exactly where their proofs end~Local verification (CI absent for
e8fee73— no coverage bot yet): store tests 9/9; mutation probes reproduced independently — in-place rewrite reds all three durability tests, early_pendingclear reds only the retry test, wrong cleanup path reds only the leftover test; neighbours: reading_progress_epics 5/5, offline_store 24/24, data_layer 8/8;dart analyzeon both changed files: no issues. Full-suite parallel run was hampered by sandbox thread-crashes (SIGABRT "thread constructor failed" at load, environmental — 5 files); each of those five re-ran green in isolation (23/11/5/14/8). Clone left pristine ate8fee73.Automated review by Jibril · 2026-09-08
CI/CD: absent for head SHA · Local checks: passed (see above)
Fair catch, and both arms are now pinned —
234f4e3.You were right that these were the odd ones out: three probes covered every other new arm, and
_deleteQuietlyhad two that nothing reached.What I found writing them: the two branches mask each other through "does not throw". Deleting a missing file raises the very
FileSystemExceptionthe catch swallows, so dropping theexistsSync()guard is invisible to any assertion aboutflush()completing. Pinning it needs an assertion on the interaction, not the outcome — so_FullDiskIonow records the pathsdeleteSyncwas called on.Two tests, on the fake that was already there:
a cleanup that fails does not escape the flush—_FullDiskIo(failDelete: true). It also assertsdeleteAttemptshas exactly one entry, so a fixture that never reached the cleanup fails loudly instead of passing on the wrong path.a write that never creates the temp file skips the cleanup—_FullDiskIo(truncatesBeforeFailing: false)models the other half of a full disk, whereopenSyncitself fails and no temp file exists. The assertion is thatdeleteAttemptsis empty._FullDiskIogained the two levers and the recording list; the default stays the open-then-fail case the existing tests use, so nothing already there changed behaviour.Probes, one branch at a time:
on FileSystemExceptionfrom_deleteQuietly+8 -1— onlya cleanup that fails does not escape the flushredsif (file.existsSync())guard+9 -1— onlya write that never creates the temp file skips the cleanupredsOne site, one case, both directions.
Checks: store tests 11/11 ·
dart analyzeon both changed files clean · neighbours (reading_progress_epics,offline_store,data_layer) 37/37 · full app suite 832/832. Still merges cleanly intomainatfc50854, even though #112 has since toucheddocs/DEVELOPMENT.mdtoo.On the non-blocking idea — agreed, and it's filed: the shared
atomicWritehelper (this store,writeManifest, and the three remaining in-place writers) is one of the twelve items in #107. You're right that prose in DEVELOPMENT.md drifts and a helper can't; I kept it out of this PR only becauseflushis sync andwriteManifestis async, so the helper needs both shapes and that is its own change.🔮 fufu~ Jibril reviewed your code!
Back so soon~? Oh, you came back with exactly what I asked for ♡ One new commit,
234f4e3, and it's a surgical +82/-6 inapp/test/reading_progress_store_test.dartonly — I diffed it against the round-1 head:git diff e8fee73..234f4e3 -- app/lib/ docs/is empty. Zero production drift, zero scope creep. You retired my blocker and touched nothing else. That's discipline~Verdict: ✅ Looks good to me~
⛔ Round-1 blocker — CLOSED~
_deleteQuietly's two dark arms — both now have their own pin, and I didn't take your word for it. Mutation-probed both arms myself: deleting theon FileSystemExceptionswallow → onlya cleanup that fails does not escape the flushreds; removing theexistsSync()guard → onlya write that never creates the temp file skips the cleanupreds; restored → 11/11 green. Your tests bite precisely where they claim to. And the fake grew up beautifully~truncatesBeforeFailingsplits the ENOSPC model into its two real halves (open-then-fail vs open-fails-outright),failDeletecompounds the failure,deleteAttemptsrecords the ask — I checked the forwarding surface member by member (parent,createSync,existsSync,renameSync,deleteSync,writeAsStringSync): everythingflush()/_deleteQuietlytouch is real-forwarded, nothing ridesnoSuchMethod.💡 Little ideas (non-blocking)~
atomicWritecan't. Your sync/async reasoning for keeping it out of this PR remains sound.✅ What I liked~
returnsNormallyand the observable contract (last-good file untouched,deleteAttemptsexact length), withreason:strings that explain the hazard in prose. Future archaeologists will know why these exist~truncatesBeforeFailingis a lovely touch — the round-1 fake could only express "truncate then die"; now the open itself failing is first-class, which is precisely the case_deleteQuietlyexists for.Local verification (CI stale for
234f4e3— coverage bot 7279 coverse8fee73only): store tests 11/11 (+2 = the two new arm pins); both new arms mutation-probed RED→restored→green; neighbours reading_progress_epics 5/5 + offline_store 24/24 + data_layer 8/8 = 37/37;dart analyzeon both changed files: no issues. Full-suite run not re-attempted this round: production code is byte-identical to round-1e8fee73(verified by empty lib/docs diff), whose full suite you ran 830/830 viascripts/verify.sh; sandbox full-suite attempts hit the tool-window timeout (environmental, same class as round 1's thread crashes — each casualty re-ran green in isolation there). Clone left pristine at234f4e3.Automated review by Jibril · 2026-09-08
CI/CD: stale for head SHA (bot covers
e8fee73) · Local checks: passed (see above)