Repository navigation
feat(#965): per-key migration timestamps and the stale-blob sweep - #972
Closed
diffwireauto wants to merge 2 commits into
Closed
diffwireauto wants to merge 2 commits into
diffwireauto wants to merge 2 commits into
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>
Member
Author
|
Agent: TealGlen (session c61ce804) |
This was referenced Oct 23, 2026
Member
Author
|
Landed on dev through #973, which carried both Epics after being retargeted to dev. Epic A's commits are on dev as One merge instead of two because this PR was stacked: merging A would have rebased its commits, changed B's head, and voided an approval given for B beforehand. Owner chose the single approval. Agent Mail: TealGlen |
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 A of #962: the engine. Per-key migration timestamps and the sweep that uses them. The manual action is #966.
Plan of record:
docs/initiatives/standalone/962-stale-blob-reclamation/PLAN.md.What it does
A migrated key's blob is a frozen snapshot taken at the migration moment, not a mirror: nothing writes it afterwards. 30 days later this deletes it, after asking the rows again, and returns the space.
__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. An install that migrated before this ships has no timestamp, so it is stamped on first sight and skipped on that pass. Updating the app can never delete a blob on the spot; every key gets the full 30 days measured from when we first knew about it.VACUUM, only when something was deleted.DELETEalone frees no disk space: measured, 74,350,592 bytes before deleting all 40 of the owner's migrated blobs and 74,350,592 after.The comparison rule lives in one place,
_absentFrom, used by both the migration gate and the sweep. The gate still decodes its own blob, because a secondcomputeover 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. ThecatchErrorsits at that fire-and-forget call site rather than insiderun, so #966's button still gets a failure it can report.scoping_inventorygained the new key, because the #853 gate caught it exactly as designed.Can't-tell branches
Four stop, one proceeds.
missingFromRowsreturns null for "could not tell" and empty for "verified nothing missing", and those are deliberately different values: collapsing them is how the only copy of an unreadable history gets deleted. The one that proceeds is a failedVACUUM, which affects file size only and follows deletes that were each individually verified.Evidence
18 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; a lossy converter refuses; an unstamped key is stamped not deleted; an unmigrated key is untouched;
contacts,discovered_contactsandpath_history_are asserted non-candidates by name with planted rows, so the prefix guard is the only thing protecting them; a DM key sweeps through the DM converter; and every kept blob is byte-identical across the sweep andVACUUMwith the row count unchanged.Full suite 1,472 pass, 1 skipped (was 1,454 on
dev).flutter analyzeclean,dart formatclean.A5, against a copy of the owner's real database, through the production sweep:
47 aged and 40 cleared because the other 7 migrated through the empty-blob path and never had a blob to reclaim.
Gemini adversarial gate
gemini-2.5-pro. Four findings across two rounds. All addressed; one was a real data-loss hole.BlobStore.reclaimVerifiedHistoryBlobholds the per-key lock across both the verify and the delete, which also serialises the sweep against a migration or a blob merge on the same key.missingFromRowstrusted a lossy converter. The serious one. 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 deleted. The migration gate has always guarded this with an independentcountJsonArrayElementsin a separatecompute; the verify path I added did not, leaving the two halves of the same rule unequal. Now cross-checked, with a test that a converter dropping its last row is refused withmissing == null.VACUUMblocks the database. Measured rather than redesigned. The review called it a UI freeze; it is not.drift_flutterruns SQLite in a spawned isolate (_isolateEntrypointtoDriftIsolate.inCurrent, verified in the package), so access queues behind it while the UI thread stays free. A stall on history-backed reads, not an ANR, which is the distinction that matters here. Close to once per install, since a cleared key has no blob to consider again. Duration is now logged: 0.6 s on the owner's 70.9 MiB database.Owner verification
Epic A is test-provable and needs no hardware. The hardware step is Epic C (#967). Verification mode for this plan's grant is
owner, so this PR needs your approval before it can merge.🤖 Generated with Claude Code