Re-parse cached entries when the shape of what we cache changes (#320) - #322
Open
cboos wants to merge 2 commits into
Open
Re-parse cached entries when the shape of what we cache changes (#320)#322cboos wants to merge 2 commits into
cboos wants to merge 2 commits into
Conversation
`messages.content` stores zlib(json(entry.model_dump())), so a cached blob is only as complete as the model that produced it. Freshness was keyed on source mtime, size and the sidecar fingerprint -- every one of them a property of the *file*, none of them a property of what we had written about it. Adding a field to a transcript model therefore left every transcript unchanged since serving a blob without that field, for as long as the file stayed untouched. Issue #320 is the first sighting. `imagePasteIds` shipped as a new field on user entries; on a cache predating it the field rehydrates as absent while the JSONL plainly carries it, so `[Image #N]` placeholders that do have a recorded association are reported as having none, rendered literally, and their images detached. Measured on one real archive: of 135 messages with placeholders, images and recorded ids, 85 held stale blobs -- 65 warned and refused to resolve, and 20 took the positional fallback (all 20 correct, since on contiguous numbering the recorded and positional readings coincide). No message resolved to a wrong image. `cached_files` gains a `content_version` column holding a digest derived from the entry models' declared field names, so a future field moves it without anyone having to remember to bump anything. A row whose version differs -- or is NULL, the state migration 014 leaves behind -- is stale and re-parses once. That NULL handling differs deliberately from migrations 007 and 011, which accept NULL as "no reason to think we missed anything". The column they added describes an input we might not have seen; this one describes the completeness of our own output, so unknown must mean incomplete.
`_schema_version` keys on the migration number alone, so on a database that already applied one of a colliding pair the other counts as applied and its DDL never runs -- silently, permanently, and only on the caches that upgraded through the first one. This was met rather than imagined. The migration added for #320 was numbered 013, as was one on another branch in flight; on a database carrying that one the `content_version` column was never added while the runner cheerfully reported the migration applied, and the freshness query then failed with "no such column". The #320 migration is 014 here for that reason. Neither branch can see the collision on its own -- the pair exists in one tree only once both merge, which is where this check now fires. A second test guards the shipped directory, so a future collision fails a test run instead of reaching a user's cache.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe SQLite cache now stores a digest of transcript model fields. Freshness checks reject mismatched or legacy rows. Save and append operations write the current digest. Migration discovery now rejects duplicate migration numbers. ChangesCache content schema versioning
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR improves cache correctness by re-parsing entries when their stored shape is outdated. A database interruption during the schema migration could temporarily prevent the cache from starting until it is repaired or rebuilt, so the change is mergeable with explicit owner awareness of this recovery edge case. Sequence Diagram(s)sequenceDiagram
participant CacheManager
participant content_schema_version
participant SQLite_cached_files
participant _cache_row_is_fresh
CacheManager->>content_schema_version: Compute transcript model digest
content_schema_version-->>CacheManager: Return 12-character version
CacheManager->>SQLite_cached_files: Save entries with content_version
CacheManager->>SQLite_cached_files: Read cached row and content_version
SQLite_cached_files-->>CacheManager: Return cached row
CacheManager->>_cache_row_is_fresh: Compare stored and current versions
_cache_row_is_fresh-->>CacheManager: Return fresh or stale
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies issue Full details: Out of Scope Changes checkExplanation The cache versioning and migration 014 changes support issue Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Fixes #320.
The bug
messages.contentstoreszlib(json(entry.model_dump())), but cacheinvalidation observed only
source_mtime,source_sizeandsubagents_fingerprint— every one of them a property of the file, none ofthem a property of what we had written about it. Adding a field to a
transcript model therefore left every transcript unchanged since then serving
a blob without that field, for as long as the file stayed untouched.
imagePasteIds(#305/#306) is the first sighting. On a cache predating it thefield rehydrates as absent while the JSONL plainly carries it, so
[Image #N]placeholders that do have a recorded association are reported as having
none, rendered literally, and their images detached — the exact pre-#306
symptom, reinstated by the cache.
The warning in the issue is therefore accurate about what it was handed, and
wrong about the transcript.
Blast radius
Measured on one real archive, over messages that have placeholders, images and
recorded ids:
The 20 are safe for the reason the fallback exists: on contiguous
1..knumbering the recorded and positional readings coincide.
In a 4000-blob sample,
imagePasteIdswas the only missing declared field —this is the hazard's first instance, not one of many.
The fix
cached_filesgains acontent_versioncolumn (migration 014) holding adigest derived from the transcript models' own declared field names. A row
whose version differs is stale and re-parses once.
It is derived, not remembered: a future field moves the digest without
anyone bumping a constant. It keys on field names only, deliberately — so a
Pydantic or Python upgrade cannot mass-invalidate a multi-gigabyte cache. A
manual salt is available for the one case names cannot see: a field whose
meaning or serialized representation changes while keeping its name.
A NULL — the state migration 014 leaves on an existing cache — counts as
stale, unlike the NULL handling in migrations 007 and 011. Those columns
describe an input we might not have seen, so "no reason to think we missed
anything" is right. This one describes the completeness of our own output,
so unknown must mean incomplete. The one-time re-parse is the point of the
migration.
Verification
End-to-end, both arms on identical database state (blobs stripped of the
field,
content_versionNULL, HTML cache cleared, source file untouched):[Image #N]in the page12 tests, three layers: the freshness rule, the digest's dependence on the
models (the pin — a new field must move it), and both freshness entry points
against the real schema. Each guard was neutralised in turn and reddens only
its own tests.
just cigreen from a rebuilt venv.Second commit: migrations claiming the same number
_schema_versionkeys on the migration number alone, so on a databasethat already applied one of a colliding pair the other counts as applied and
its DDL never runs — silently, and only on the caches that upgraded through
the first one. The failure surfaces far downstream, as
no such column.This is why the migration here is 014: 013 is claimed by
perf/watch-tick-latency. On a database carrying that one,content_versionwas never added while the runner reported the migration applied.
Neither branch can see the collision from its own tree — the pair exists in
one tree only once both merge, which is where the new check fires. A second
test guards the shipped directory.
Merge order: if
perf/watch-tick-latencylands after this, its migrationnumber is the one that needs to move.
Summary by CodeRabbit
Bug Fixes
Reliability