Repository navigation
fix: say why an Antigravity conversation was lost, and stop caching the failure - #151
Merged
chattymin merged 6 commits intoAug 8, 2026
Merged
Conversation
`conversationEntries` was right to discard a scan that did not end on `SQLITE_DONE` — half a conversation must not pass for the whole of it — but it returned the same `[]` as a conversation with no usage, and said nothing. A store that could not be read stayed invisible for as long as it stayed that way, which on a per-provider tab now means the tab itself disappears rather than a number being slightly low. The read is now a value with four cases, because three of them produce no rows and only one of those three is a fact about the user's spend: the scan finished, the scan stopped early, the store could not be opened or queried, or the file has no `gen_metadata` at all. That last one is deliberately *not* a loss — this directory may hold databases that are not conversation stores, and a missing table is a permanent property of the file, so naming it would print the same line every two minutes forever. `SQLITE_ERROR` from `prepare` is that case; a BUSY or an I/O error there is a real loss. `AppLog.write` returns early outside the bundled app, so a test that watched the log file would cover nothing — the same reason the limit-alert decision was split out of its own side effect. `lossLog` is therefore a pure function from the reads to the lines, tested directly, and the one `AppLog.write` sits at the scan's caller. It also bounds itself: five stores are named per scan and the rest are counted. A store that cannot be read is re-read on every refresh — 720 times a day at the default interval — so an unbounded line per store per scan could rotate the 2 MB log several times a day on a machine whose conversation directory went bad, and take the crash history in that same file with it. The regression test walks the branch rather than approximating it: a fixture spanning several 512-byte pages with the last page's b-tree type byte damaged steps rows and then returns `SQLITE_CORRUPT`. It asserts the trigger condition first — that rows really do arrive and the scan really does not reach `DONE` — so it cannot keep passing if the failure quietly moves to the open. Page 1 is left intact for the same reason: corrupting the header would fail the open and cover a different branch. A lock cannot be used here; `immutable=1` disables locking, so the fallback reads straight through any lock a test could hold.
`tokenCount` clamped anything above the ceiling to 0, which is the right value — a `uint64` sentinel widened into `Int` traps the process on the next addition, and it traps again on every refresh because the file never changes — but it left the record looking like a turn that genuinely used no input. It now returns `nil` for a value that cannot be a count, which the absent case does not: `cache_write_tokens` is declared and never written by this CLI, so "the writer set nothing" is a legitimate zero and must not be reported. The two cases were indistinguishable before, and reporting them together would have printed every record of every scan. Counting happens per row and reporting per store. `tokenCount` runs four times a record and the live store here holds 3,934 of them, so a line where the discard happens is up to 15,000 lines from a single scan — enough to rotate the 2 MB log twice over and take the crash history with it. `discardLog` totals them per conversation and shares the loss lines' naming limit. `testSentinelTokenCountIsDiscardedRatherThanTrapping` already covered the clamp; it now also asserts the diagnostic, so the branch cannot go quiet again without a test failing. `testAbsentCounterIsNotADiscard` pins the other half — the distinction between a missing field and a bad one is the whole reason this can be reported at all.
…n a clock `LocalAntigravityUsageCache` expired after 30 seconds while `LocalUsageCache` keys its per-file blobs on `(path, mtime, size)`. A timer has both of a timer's failings here: for up to half a minute it served a store that had just changed from a parse taken before the change, and the moment it lapsed it reopened all 124 conversation stores to discover that none of them had moved. The scan now keeps one blob per store, reuses any whose signature still holds, and reopens only the rest. Both provider calls — daily and enrichment — re-stat every store and share the parse, which is where the sharing the expiry used to provide now comes from. The signature is the newest of the `.db` and its `-wal`, and their two lengths added. `-shm` is excluded on purpose, and that is the one subtle part: it holds no committed data, and a read-only WAL connection writes read marks into it, so a key that included it would be invalidated by this reader's own read, on every scan, forever — a cache that never hits, which is worse than the expiry it replaces. For the same reason it adds nothing to the window test: a `-shm` newer than both of the others means somebody read the store, not that anything was written to it. Three things had to move for a blob to be sound: - The window filter left the row loop. A cutoff baked into a cached unit is wrong as soon as the window moves; the rows are cached unfiltered and narrowed at assembly, which is also what lets one blob serve both windows. - A read that did not finish no longer writes a blob. Under an expiry, "drop the partial read and let the next refresh read it entire" was true by default; under a cache it is only true if the failure is not filed under the current signature. The previous rows are carried forward under their *old* signature instead, so the next scan retries. A store with nothing to carry forward gets no blob at all rather than an empty one, which would have read as no usage for as long as the store sat still. - A database with no `gen_metadata` does cache its empty, because that is a permanent property of the file and reopening it every two minutes for the life of the install is the failure mode in the other direction. The `monthKey` guard went with the expiry. `enrichmentScanStart` is a min of three non-decreasing functions of `now`, so the scan bound only ever advances and an older blob set is always a superset of what a later call needs. The guard was in fact keyed on the wrong quantity: at 00:30 on the 1st the month key says April while the scan window starts in March, so it threw away precisely the blobs the new week total needed, at the worst possible moment. The prune went too. Rebuilding the map from the stores the sweep visited drops what left the window and what left the disk, exactly, rather than on a 40-day approximation.
`sqlite3_close` fails and leaves the handle open if any statement is still live. Nothing leaks today, because the `finalize` is registered after the close and `defer` runs last-in-first-out — but that is a fact about where two lines sit relative to each other, not about the call, and this function has since grown early returns between the two. `sqlite3_close_v2` defers the close until the last statement goes, so the guarantee comes from the call. The error path in `openReadOnly` is the site where `v1` could already have been wrong: there is no statement to order against there at all. The same call appears once more in `LocalAdditionalUsageProvider.swift:622`, under the same reasoning. It is left alone here to keep this branch about Antigravity; the sibling readers have a few of these in common and are better swept together.
`chattymin#145` gave the JSON readers `LocalUsageReader.maxParsedTokenValue` while this reader has had `tokenCeiling` since it was written, and the two differ on both axes — the ceiling itself, and whether a value above it is clamped or dropped. Neither divergence is accidental, but nothing said so, and `defect-log.md` now asks for one defence point per class. Written down here rather than reconciled, because which way they should converge is a call for the maintainer.
…assumption Three classes came out of these four notes and none of them was in the ledger. The first is the one that made note 3 depend on note 1: a guard that discards a partial read is only a guard while a retry is actually coming, and a `(path, mtime, size)` key removes the expiry that used to guarantee one. The second is why `-shm` is not in that key — a read-only WAL connection writes read marks into it, so a scan would invalidate the blob it had just written and the cache would never hit at all. The third is the shape the review notes were mostly about: a reader that folds failure into `[]` makes "could not read" and "was not used" the same value, which on a per-provider tab is a tab that vanishes rather than a number that is slightly low. Each carries the regression tests that walk it, and the log-volume arithmetic, since a source that cannot be read is re-read on every refresh and one line per source per scan rotates the 2 MB log several times a day.
Owner
|
Thank you so much for your contribution! You implemented exactly what we had discussed earlier, so everything looks perfect without any additional back-and-forth. We really appreciate your interest and effort, and we look forward to seeing more contributions from you in the future! |
Open
10 tasks
7 of 10 tasks
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.
Summary
The four review notes from #141, as agreed there. Each is its own commit.
1. A lost conversation says which one it was.
conversationEntrieswas right to discard a scan that did not end onSQLITE_DONE, but it returned the same[]as a conversation with no usage and said nothing, so a store that could not be read stayed invisible for as long as it stayed that way. The read is now a value with four cases, because three of them produce no rows and only one of those three is a fact about the user's spend. A file with nogen_metadatais deliberately not a loss — this directory can hold databases that are not conversation stores, and a missing table is permanent, so naming it would print the same line every two minutes forever.SQLITE_ERRORfromprepareis that case; BUSY or I/O there is a real loss.The regression test walks the branch rather than approximating it. A lock cannot be used —
immutable=1disables locking, so the fallback reads straight through anything a test could hold — so the fixture spans several 512-byte pages and has the last page's b-tree type byte damaged, which steps rows and then returnsSQLITE_CORRUPT. It asserts the trigger condition first, that rows really do arrive and the scan really does not reachDONE, so it cannot keep passing if the failure moves to the open.2. Discarded token counters are reported.
tokenCountclamped anything above the ceiling to 0, which is the right value, but left the record looking like a turn that used no input. It now returnsnilfor a value that cannot be a count, which the absent case does not —cache_write_tokensis declared and never written, so "the writer set nothing" is a legitimate zero. Counting is per row and reporting per store:tokenCountruns four times a record and the live store here holds 5,572 of them.3. The cache keys on each store's
(mtime, size). It expired after 30 seconds whileLocalUsageCachekeys per-file blobs, which has both of a timer's failings — for up to half a minute it served a store that had just changed from a parse taken before the change, and the moment it lapsed it reopened all 160 stores to find that none had moved. Measured on the live store: cold scan 0.185s, second scan 0.013s with all 160 blobs reused.The signature is the newest of the
.dband its-walplus their two lengths.-shmis excluded on purpose and it is the subtle part: a read-only WAL connection writes read marks into it, so a key that included it would be invalidated by this reader's own read, on every scan — a cache that never hits, which is worse than the expiry it replaces.Three things had to move for a blob to be sound: the window filter left the row loop (a cutoff baked into a cached unit is wrong as soon as the window moves); a read that did not finish no longer writes a blob, carrying the previous rows forward under their old signature so the next scan retries; and a database with no
gen_metadatadoes cache its empty, because that is permanent. ThemonthKeyguard went with the expiry —enrichmentScanStartonly ever advances, and the guard was keyed on the wrong quantity anyway: at 00:30 on the 1st the month key says April while the scan window starts in March, so it threw away exactly the blobs the new week total needed.4.
sqlite3_close_v2. Nothing leaks today because thefinalizeis registered after the close anddeferruns last-in-first-out, but that is a fact about where two lines sit, not about the call — and the function has since grown early returns between them. The error path inopenReadOnlyis the site wherev1could already have been wrong: there is no statement to order against there at all.Plus the two follow-ups the protocol asks for.
docs/reference/defect-log.mdgains the three classes these notes produced — a blob cache removing the retry that a discarded partial read depends on, why the file our own read touches cannot be in the invalidation key, and failure folded into[]being indistinguishable from no usage. AndAntigravityProto.tokenCeilingnow cross-referencesLocalUsageReader.maxParsedTokenValue.One question, on that last point. #145 landed while #141 was open, and the two ceilings differ on both axes: 1e9 vs 1e15, and this one discards the counter where that one clamps to the ceiling. Neither divergence is accidental — nothing here adds two parsed values before
Entry.total, so a tighter ceiling costs nothing, and clamping keeps a number that then dominates today's total, the burn tier and the companion, where discarding loses one counter and leaves the record intact. Butdefect-log.mdasks for one defence point per class, so I have written the difference down rather than reconciled it. Happy to converge them either way if you would rather — it is a small change in either direction.Type of change
Checklist
swift buildandswift testpass locally — 577 tests,scripts/test-gate.shreports 89.52%Sources/PokeTokenBar/UI/changed, so the UI section is omitted