Skip to content

fix: skip the Kiro rescan when the database has not changed (#178) - #179

Merged
chattymin merged 2 commits into
chattymin:mainfrom
Borisserz:fix/kiro-skip-unchanged-database
Aug 20, 2026
Merged

chattymin merged 2 commits into
chattymin:mainfrom
Borisserz:fix/kiro-skip-unchanged-database

Conversation

@Borisserz

@Borisserz Borisserz commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

LocalAdditionalUsageReader.kiroEntries re-reads and re-parses every stored Kiro conversation on every poll. Unlike Cursor and Copilot, Kiro rewrites a conversation's whole history JSON in place, so there is no monotonic row id. That was the right call for #170; the cost is this issue.

modifiedSince is applied inside kiroTurnEntries, after the row has been read out of SQLite and jsonObject has parsed it. Narrowing the window skips entry construction, not the read or the parse. Owner measured 209–360 ms on a 30×80 synthetic database, and a 37 ms floor on an empty window.

The cheap gate is the file's own signature. This reuses LocalAntigravityUsageReader.signature (main file + -wal, never -shm) rather than copying it — a WAL commit leaves the main file's mtime alone, and a read-only connection writes read marks into -shm, which would make the skip invalidate itself on every poll.

An unchanged database returns []. The .kiro cache merges existing + loaded, so a scan that returns nothing keeps the cached entries. A failed open (BUSY / corrupt) does not occupy the skip slot.

Skip state is not a process-lifetime map. Signatures live on LocalAdditionalUsageCache.Cached next to the entries they justify (option (b) from the #179 review — the same shape Antigravity uses). The actor passes previous?.kiroSignatures ?? [:]. A month-key drop sets previous to nil, so both existing and the skip die together. Without that, the first poll on the 1st returns [], dedupKeepMax([] + []) is [], and the week/block window that still reaches into last month reads 0 until Kiro next writes.

The [Entry] overload always passes [:], so a one-shot read (and the parse-stability test) actually re-reads.

Class recorded in docs/reference/defect-log.md: a filter applied after parsing bounds the output, not the work; a skip that outlives existing zeros the window the skip was supposed to keep.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • Other:

UI changes

No visible change. Totals stay the same; an unchanged Kiro database is no longer fully re-parsed every 120s. Month rollover no longer drops last-month Kiro turns from the week/block totals.

Checklist

  • swift build and swift test pass locally
  • PR title and description are written in English
  • UI changes are described above (before/after — images optional)
  • No copyrighted assets, secrets, or private tooling references are committed (see CONTRIBUTING)
  • Tests were added or updated for this change

Tests

KiroUsageTests — each branch alone:

  • testUnchangedDatabaseSkipsTheRescan — the The Kiro reader re-parses every conversation on every poll, and narrowing the window does not help #178 cost. Second scan with the first scan's signatures returns []; dedupKeepMax(existing + loaded) keeps the first scan's ids.
  • testWalCommitForcesARescan — a newer -wal sibling must invalidate the skip. Keying on data.sqlite3 alone would stay green here.
  • testShmChurnDoesNotForceARescan — -shm is a read mark, not a write. Including it would make hit rate 0.
  • testFailedOpenDoesNotOccupyTheSkipSlot — garbage bytes must not appear in the returned signatures map, or a later valid file with a lucky-matching mtime would be treated as empty forever.
  • testRescanningTheSameDatabaseProducesStableEntryIDs — still a parse-stability test. The [Entry] overload passes no known signatures, so it cannot pass by returning [].
  • testSkipDoesNotSurviveAnEmptyExistingSet — the fix: skip the Kiro rescan when the database has not changed (#178) #179 review reproduction. One Aug 31 turn; August scan sees it; September week window (modifiedSince Aug 30) with empty existing must still see it. Confirmed red against the process-lifetime cache at 4c27d3b.
  • testSkipStateIsPassedByTheCallerNotHeldByTheReader — source lock: no KiroDatabaseScanCache / clearKiroScanCache / recordedKiroSignature; the actor reads previous?.kiroSignatures.

swift test --filter KiroUsageTests: 29 tests, 0 failures.

Full suite: 682 pass, 9 skipped, 0 failures.

How to verify

DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer \
  swift test --filter KiroUsageTests

No UI steps. A machine with a large Kiro data.sqlite3 should see the second poll of an idle session return immediately instead of re-parsing every conversation. Crossing the 1st with an unchanged Kiro database must still show last week's turns in the week/block totals.

…n#178)

modifiedSince is applied after the JSON parse, so an unchanged store
still pays the full read. Gate on the existing Antigravity file
signature (db + wal, not shm) and return empty so the merge cache keeps
what it already has.
@chattymin

Copy link
Copy Markdown
Owner

Thanks @Borisserz — reusing LocalAntigravityUsageReader.signature rather than copying it is the right call, and getting the -shm exclusion right is the part that would have quietly made the cache never hit. Not recording a signature on a failed open is the same lesson #151 landed for Antigravity, and you applied it here without being asked.

Requesting one change before merge. That change is on me.

The spec in #178 was incomplete

#178 said a contentModificationDate compared against the previous scan was "enough to return early", and the only thing it asked you to confirm was that the first scan of a process isn't skipped. You did exactly that. But existing empties a second way, and the issue never mentioned it.

1. The skip survives a month rollover that empties existing — blocking

LocalAdditionalUsageCache.entries(for:) drops the cached entries when the month key changes:

// LocalAdditionalUsageProvider.swift:145
let previous = cached[source].flatMap { $0.monthKey == monthKey ? $0 : nil }
let existing = previous?.entries ?? []

kiroScanCache is a private static let, so it does not go with them. On the 1st, if Kiro hasn't been written to since the boundary, the skip still fires, kiroEntries returns [], and dedupKeepMax([] + []) is [].

That matters because the scan window reaches back across the boundary — enrichmentScanStart is min(startOfMonth, startOfWeek, now - 5h):

  • the 5-hour block window precedes the month start for the first five hours of every month, without exception
  • the week window precedes it in 10 of the next 12 months (2026-11 and 2027-08 are exempt, since the 1st is itself the week start), for up to 6 days

So the week total and the active block read 0 until the Kiro database is next written. Reproduced against 4c27d3b:

// One Aug 31 turn seeded.
let august = kiroEntries(modifiedSince: date("2026-08-01T00:00:00Z"), roots: [tmp])
XCTAssertFalse(august.isEmpty)                        // passes

// Month rolls over: existing == [] again, window is now the week start, Aug 30.
let september = kiroEntries(modifiedSince: date("2026-08-30T00:00:00Z"), roots: [tmp])
XCTAssertFalse(dedupKeepMax([] + september).isEmpty)  // FAILS

Inserting clearKiroScanCache() between the two calls — the behaviour before this PR — makes it pass, so the loss is the skip, not the window arithmetic.

This repo has hit this before. 476f94d removed the monthKey guard from the Antigravity path for exactly this reason:

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 Kiro path still has that guard, and this PR puts a skip behind it.

I gave you a one-line prescription last time and it is what produced this, so rather than another one, here is the invariant: the skip is only sound because the caller still holds the entries it isn't re-reading. When that stops being true, the skip has to stop too. Two shapes satisfy it:

  • (a) kiroEntries(modifiedSince:allowSkip:roots:), with the actor passing allowSkip: !existing.isEmpty. Smallest diff, keeps your tests as they are. Cost: someone with Kiro installed but unused keeps existing empty forever, so the skip never engages for them — cheap, given your 37 ms floor was on a 6.3 MB database, but worth being deliberate about.
  • (b) Move the signature into LocalAdditionalUsageCache.Cached, so it is invalidated alongside the entries that justify it. Larger, and it also retires the statics in 3 below. This is the shape Antigravity settled on.

Either works for me — your call.

Out of scope for this PR: removing the monthKey guard the way 476f94d did. That touches all five additional sources and belongs in its own change.

2. Does a 2.0.1+ store keep conversations_v2? — question

query returns nil both for a missing table and for a scan that stopped early (BUSY, damaged page), so guard v2Rows != nil || v1Rows != nil cannot tell them apart. If both tables exist on one file, a BUSY on one generation plus a success on the other reads as a completed scan and records the signature, freezing a partial result until the file next moves.

BUSY implies Kiro is writing and the mtime will move shortly, so this is largely self-healing and I am not asking for a change on its own — but if the migration leaves conversations_v2 in place, the guard's comment should say so. Shape (b) removes the hazard either way, since a partial result would no longer displace a complete cached one.

3. Note

clearKiroScanCache() and recordedKiroSignature(for:) are module-visible but exist only for the tests, which is new for this file. Shape (b) removes the need for both; no separate action if you take (a).

I will update #178 to describe the second path existing empties, so the issue stops pointing at the incomplete fix.

)

A process-lifetime skip map survived the month-key drop that empties
`existing`, so the first poll on the 1st returned [] and zeroed the
week/block window. Signatures now live on Cached next to the entries
they justify.
@Borisserz

Copy link
Copy Markdown
Contributor Author

Took (b).

The skip is now an argument, not a process-lifetime map. Cached holds kiroSignatures next to entries. The actor passes previous?.kiroSignatures ?? [:] — the same previous that supplies existing. A month-key drop sets previous to nil, so the next scan has both existing == [] and knownSignatures == [:] and re-reads. That is the Aug 31 / Sep week-window case from the review (testSkipDoesNotSurviveAnEmptyExistingSet). Confirmed red against 4c27d3b (static cache still skipped), green after the move.

clearKiroScanCache() and recordedKiroSignature(for:) are gone. The [Entry] overload always passes [:], so parse-stability tests re-read instead of passing on a skip []. Failed open still returns no signature.

On (2): query returning nil is now documented as a missing table or a scan that stopped early, and that a 2.0.1+ store may still keep conversations_v2. A later partial scan no longer freezes a process-wide skip against empty existing; if Cached still holds a complete set, dedupKeepMax(existing + loaded) keeps it.

monthKey guard left as-is (out of scope).

swift test --filter KiroUsageTests: 29 pass. Full suite: 682 pass, 9 skipped.

@chattymin
chattymin merged commit 8726908 into chattymin:main Aug 20, 2026
2 checks passed
@chattymin

Copy link
Copy Markdown
Owner

Thank you for your contribution!

lukemonaghan pushed a commit to lukemonaghan/PokeTokenBar that referenced this pull request Aug 24, 2026
…n#178) (chattymin#179)

* fix: skip the Kiro rescan when the database has not changed (chattymin#178)

modifiedSince is applied after the JSON parse, so an unchanged store
still pays the full read. Gate on the existing Antigravity file
signature (db + wal, not shm) and return empty so the merge cache keeps
what it already has.

* fix: keep the Kiro skip signature with the cached entries (chattymin#178)

A process-lifetime skip map survived the month-key drop that empties
`existing`, so the first poll on the 1st returned [] and zeroed the
week/block window. Signatures now live on Cached next to the entries
they justify.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants