Repository navigation
feat(#966): Clear Stale History Now, with refusals reported - #973
Merged
Merged
Conversation
Epic A of #962. The engine only; the manual action is #966. A1 wrote the diagnosis as failing tests first, and they were red for the right reason: no sweep existed and six BlobStore methods did not. Also lands a control test asserting that DELETE alone does not shrink the database, which passes today and must keep passing, since it is the entire reason a VACUUM step exists. A2, the timestamps. A second blob, __msgrows_migrated_at, keyed by store key. Deliberately NOT a reshape of __msgrows_migrated: that marker is read on every save and every load through a cache, so changing its shape would put a migration of its own on the hottest storage path. Owner decision 2026-10-22. An install that migrated before this ships has no timestamp, so backfillMigratedAt stamps it on FIRST SIGHT and the sweep skips anything it stamped this pass: updating the app can never delete a blob on the spot, and every key gets the full 30 days measured from when we first knew about it. A3, the sweep. Eligibility is age plus a fresh verification, never the marker alone, which is the difference between "we recorded that this migrated" and "we just confirmed it". missingFromRows returns null for "could not tell" and an empty list for "verified nothing missing", and those are deliberately different values: collapsing them is how the only copy of an unreadable history gets deleted. A4, one VACUUM, only when something was deleted. A failure there is logged and tolerated: the deletes already happened and were each verified, so it costs space and not data, and the next sweep retries it. The comparison rule now lives in ONE place, _absentFrom, used by both the migration gate and the sweep. The gate still decodes its own blob, because a second compute over multiple megabytes on the migration path would cost more than the duplication saves, so the rule is shared even though the decode is not. Wired into main.dart's existing post-frame callback, beside the #906 exit-info collector, for the same reason: storage hygiene must never be on the path to a usable screen. The catchError sits at that fire-and-forget call site rather than inside run(), so #966's button still gets a failure it can report. scoping_inventory gained the new key, because the #853 gate caught it exactly as designed. Its verdict is globalOnPurpose for the same reason as the marker: it is keyed BY store keys that already carry the radio. 15 tests, the plan's whole acceptance bar: the 29-day and 30-day boundaries; one missing row refuses AND the same key clears once its rows are complete, which is what proves the gate discriminates rather than never deleting; an undecodable blob refuses; an unstamped key is stamped not deleted; an unmigrated key is untouched; contacts, discovered_contacts and path_history_ are asserted non-candidates BY NAME; a DM key sweeps through the DM converter; and every kept blob is byte-identical across the sweep and VACUUM with the row count unchanged. A5, verification against a copy of the owner's real database through the production sweep: blob keys: 470, migrated history keys aged: 47 cleared 40, refused 0, 17367040 bytes returned (16.56 MiB) kept keys missing: 0, byte-changed: 0 integrity_check: ok, rows 29949 -> 29949 47 aged and 40 cleared because the other 7 migrated through the empty-blob path and never had a blob to reclaim. Full suite 1,469 pass, 1 skipped, up from 1,454. analyze clean, format clean. Task: meshcore-open-5vt Agent Mail: TealGlen Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three findings, all addressed. One was a real data-loss hole. 1. CRITICAL, fixed. missingFromRows trusted the converter's own output as the measure of the converter. A callback that silently dropped a record, rather than throwing, would have had every record it DID produce found present, returned "nothing missing", and the blob holding the only copy of the dropped one would have been deleted. The migration gate has always guarded this with an independent countJsonArrayElements in a separate compute; the verify path I added did not, which left the two halves of the same rule unequal. It now does the same cross-check and refuses as unverifiable on a mismatch. New test: a converter that drops its last row is refused with missing == null, not missing == []. 2. Measured, not redesigned. VACUUM takes an exclusive lock, and the review called it a UI freeze. It is not: drift_flutter runs SQLite in a spawned isolate (_isolateEntrypoint -> DriftIsolate.inCurrent, verified in the package), so database access queues behind it while the UI thread stays free. That is a stall on history-backed reads, not an ANR, which is the distinction that matters for an app whose storage work began as an ANR investigation. It is also close to once per install, since a cleared key has no blob to consider again. The duration is now logged, because 0.6 s on the owner's 70.9 MiB database is a number worth watching in the field rather than assuming. 3. Fixed, and the review was right about why. The non-history-key test asserted only that the isHistoryKey helper returns false, which would still pass if run() stopped calling it: the loop visits keys that HAVE rows, and contacts and friends normally have none, so they were never reachable anyway. Those keys now get a planted row and a 99-day-old timestamp, so the prefix guard inside run() is the only thing standing between them and a delete. Also carried over from the previous round: the check-then-delete window is closed by BlobStore.reclaimVerifiedHistoryBlob, which holds the per-key lock across both the verify and the delete and so also serialises the sweep against a migration or a blob merge on the same key. Two tests cover it, including that a second call on a blobless key reports neither a delete nor a problem, which is what makes a repeat sweep silent. On the owner's database 7 of 47 aged keys are in that state, having migrated through the empty-blob path. 18 tests in the file, full suite 1,472 pass, 1 skipped. Re-verified against a copy of the owner's real database after the changes: blob keys: 477, migrated history keys aged: 47 cleared 40, refused 0, 17362944 bytes returned (16.56 MiB) kept keys missing: 0, byte-changed: 0 integrity_check: ok, rows 29950 -> 29950 Task: meshcore-open-5vt Agent Mail: TealGlen Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Epic B of #962. The manual trigger for Epic A's sweep, so a user does not wait 30 days. It passes olderThan: Duration.zero, which skips the WAIT and not the safety: the per-key verification inside reclaimVerifiedHistoryBlob is unchanged, proved by a test that Duration.zero still refuses a key whose rows cannot be verified. Its own widget rather than another card in app_settings_view.dart, which is already past a thousand lines, and so the tests can drive it without standing up the whole settings screen. Stateful against the repo's usual StatelessWidget-plus-Consumer habit, because the running flag and the last result are local to this card and belong to no service. The sweep is INJECTED, and that is a finding rather than a convenience. The first version of the widget test called the real sweep and hung past ten minutes instead of failing: the sweep uses compute, and an isolate deadlocks pumpAndSettle inside flutter_test's fake-async zone. A widget test should test the widget. So the card takes the sweep and the size read as parameters, the nine widget tests feed outcomes in directly, and the two new tests in stale_history_sweep_test.dart keep the injected semantic honest under plain test() where isolates work. Owner decision 2026-10-22, and the reason this Epic exists: the button REPORTS keys it refused, it does not only log them. "Cleared 1 old copies, 16.0 MB returned" and "Kept 1 that could not be verified" are both shown, including when a refusal is the reason nothing was cleared, which is the most important case to surface rather than the least. The automatic sweep stays log-only. Two Gemini findings, both fixed: - A fast double tap ran two sweeps. The disabled button is not enough on its own, because setState only SCHEDULES a rebuild, so two taps inside one frame both see a live onPressed. _clear now returns early on _running, and a test taps twice in one frame and asserts the sweep ran once. Two concurrent sweeps would have meant two concurrent VACUUMs. - A throw after real deletes was reported as a total failure. The honest fix was in the sweep, not the string: a successful sweep must not become an error because the closing size read threw, so that read now falls back to bytesBefore, which understates the reclaimed figure rather than inventing one. The failure string no longer claims nothing happened either. Strings are English-only, as everywhere else here: gen-l10n falls back to the template locale, and untranslated.json records the 9 keys across 17 locales, which is what that tracked file is for. 29 tests in the two files, full suite 1,483 pass, 1 skipped. analyze clean, format clean. Task: meshcore-open-gan Agent Mail: TealGlen Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Agent: TealGlen (session c61ce804) |
The owner asked whether the delete and compact are logged or silent. Checked rather than assumed, and the answer was bad on two counts: - A delete logged only a COUNT. "Cleared 40" names nothing, so there was no record of WHICH keys were reclaimed. Refusals were already logged properly, with the key and up to three missing records; deletions were not. - All of it goes through appLogger, which is gated on appDebugLogEnabled. That is off by default and off on his install, so on a normal install an irreversible delete left no trail at all. Both fixed. The summary now names the cleared and kept keys, and the sweep also writes a durable entry into the database itself, under __msgrows_reclaim_audit: timestamp, the cleared keys in full, the refused keys, and the byte counts either side. In the database because that survives restarts and the logging toggle, and can be read back without the app running, which is also how it gets verified on hardware. Key lists are stored in full. Truncating them would recreate the "Cleared 40" problem the entry exists to solve. Capped at the last 5 sweeps instead, which is generous given a key is cleared once and then has no blob to consider again, so this is close to a once-per-install operation. A failure to write the audit is logged and tolerated rather than failing the sweep: the deletes already happened and were each verified, so losing the record is worse than nothing but far better than reporting a correct sweep as broken. An unreadable audit likewise starts a fresh list and is logged, never silently replaced. scoping_inventory gained the key, globalOnPurpose: one sweep spans every radio and the entries name store keys that already carry the radio. Six tests: a sweep records which keys it deleted; the entry survives a fresh store object over the same database, so it is really on disk; refusals are recorded too; a sweep that did nothing writes no entry; the cap drops the oldest and keeps the newest; and an unreadable audit does not stop a sweep. Lands on Epic B's branch rather than Epic A's, where the sweep lives, to avoid restacking B over a changed A for a change that reaches dev through both either way. Origin is #965's engine. Full suite 1,489 pass, 1 skipped. analyze clean, format clean. Task: meshcore-open-gan Agent Mail: TealGlen Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ligibility bug it found The owner asked why there were no UI mock-ups to review. There was no good answer: this Epic's card was built and installed on both his machines without him ever seeing the design, and the plan had no step requiring otherwise. So the plan now has B0: mock-ups first, owner-approved before any of it is built, as a committed HTML file beside the plan in the format his firmware initiative already uses. epic-b-storage-mockups.html is that file: eleven numbered variants with real Material 3 tokens, a 360-wide pane, example values from his own install, and the behaviour to build as bullets under each. Owner-approved 2026-10-23. The confirmation (B1a, variants 8 to 10). It states how many copies, roughly how much space, how many messages are NOT affected, and that it cannot be undone, with the count on the action button so the scope is the last thing read. Cancel, a tap outside and back all decline. Nothing eligible opens no dialog at all, because a dialog that only says zero is not worth opening. That needed a preview pass: verify every eligible key and total it without deleting, so the numbers are real rather than estimated. Verification now runs twice for a confirmed clear, once to preview and once under the key lock. That is the cost of saying what will go before it goes. approximateBytes is named for what it is: the sum of blob lengths, not what VACUUM returns, because page rounding moves the real figure; the dialog says "about" and the result reports what actually changed. Writing the preview found a real bug, and it was about to bite him. The "skip anything stamped this pass" guard exists so installing an update never deletes a blob on sight, and it was being applied to the button as well. Every install that migrated before #962 has an empty __msgrows_migrated_at, so the first press stamped everything and cleared NOTHING. His install is exactly that: 47 migrated keys, none stamped. He would have pressed the button on a 70.9 MB database with 16.6 MB of reclaimable blobs and been told "nothing to clear". Fixed as eligibleByAge, one static rule both preview and run share, so the dialog cannot promise what the sweep will not do. An explicit press includes newly stamped keys; the automatic sweep still refuses them. Three tests on the rule directly, plus one proving the automatic path refuses while the press clears. Label stays "Clear Stale History Now" rather than "Compact", owner's call and the better argument: the name generalises to clearing stale history for other reasons later, such as #963's retention limit. 14 widget tests and 34 storage tests. Full suite 1,502 pass, 1 skipped. analyze clean, format clean. Task: meshcore-open-gan Agent Mail: TealGlen Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Oct 24, 2026
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.
Epic B of #962. The manual trigger for Epic A's sweep, so a user does not wait 30 days.
Stacked on #972 (Epic A). Base is
feat/965-stale-blob-sweep, so review this PR's own two commits rather than the diff againstdev.What it adds
A Storage card in App Settings with the current database size and a Clear Stale History Now button. It passes
olderThan: Duration.zero, which skips the wait and not the safety: the per-key verification insidereclaimVerifiedHistoryBlobis unchanged, and a test provesDuration.zerostill refuses a key whose rows cannot be verified.Its own widget rather than another card in
app_settings_view.dart, which is already past a thousand lines.Owner decision this Epic exists for
The button reports keys it refused, it does not only log them. Both halves are shown, including when a refusal is the reason nothing was cleared, which is the most important case to surface rather than the least:
The automatic sweep stays log-only, since nobody is watching it.
The injection is a finding, not a convenience
The first version of the widget test called the real sweep and hung past ten minutes instead of failing. The sweep uses
compute, and an isolate deadlockspumpAndSettleinsideflutter_test's fake-async zone. The same calls pass fine in thetest()-based storage suite.So the card takes the sweep and the size read as parameters, the nine widget tests feed outcomes in directly, and two new tests in
stale_history_sweep_test.dartkeep the injected semantic honest under plaintest(), where isolates work. A widget test should test the widget; the engine is Epic A's 20 tests.Gemini adversarial gate
Two findings, both fixed.
setStateonly schedules a rebuild, so two taps inside one frame both see a liveonPressed._clearnow returns early on_running, with a test that taps twice in one frame and asserts the sweep ran once. Two concurrent sweeps would have meant two concurrentVACUUMs.bytesBefore, which understates the reclaimed figure rather than inventing one. The failure string no longer claims nothing happened either.Evidence
Duration.zeromakes a key migrated seconds ago eligible while the default window does not, andDuration.zerostill refuses an unverifiable key.flutter analyzeclean,dart formatclean.Strings are English-only as everywhere else here:
gen-l10nfalls back to the template locale, anduntranslated.jsonrecords the 9 keys across 17 locales, which is what that tracked file is for.Owner verification
Epic B is test-provable. The hardware step is Epic C (#967), where you press this button on your own install and see a real reduction. This plan's grant uses
ownerverification, so this PR needs your approval before it can merge.🤖 Generated with Claude Code