Skip to content

fix(pricing): atomically replace cached catalogs on Linux - #3444

Merged
steipete merged 1 commit into
mainfrom
codex/fix-pricing-cache-atomic
Sep 6, 2026
Merged

fix(pricing): atomically replace cached catalogs on Linux#3444
steipete merged 1 commit into
mainfrom
codex/fix-pricing-cache-atomic

Conversation

@steipete

@steipete steipete commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Refreshing an existing models.dev pricing catalog on Linux can fail and remove the cache file. The old path atomically wrote a temporary file and then performed a second FileManager.replaceItemAt operation. This replaces that sequence with one atomic write to the destination, retaining the existing failure return and invalidating the in-memory memo only after a successful save.

This extracts the shared cache defect identified in #3412. Thanks @WeGoToMars for identifying and addressing it in that proposal. The Antigravity estimation feature remains separate and #3412 stays open. The generated parser fingerprint is updated through the existing generator; documentation and the Unreleased changelog are included. Production sources are net −7 lines (2 added, 9 removed, including the generated fingerprint).

Validation:

  • Linux x86_64 with the official Swift 6.3.3 container: the unchanged production cache code fails on its second save (saved=false, destination missing). The candidate passes 100 successive saves, each checked with a fresh disk decode and a memo read, with no leftover staging files.
  • Added a portable regression covering repeated saves, persisted contents, memo freshness, and staging-file cleanup.
  • make check passes. All 1,025 selections across 86 groups pass in make test; one unrelated RPC child-teardown timing failure passed on its group retry. All 42 focused pricing/cache tests pass, including the portable replacement regression.
  • CI passes all nine checks, including both macOS test shards, Linux x64, Linux arm64, and musl.

Extract the existing cache replacement defect identified in #3412 while leaving its Antigravity feature scope separate. Replace the redundant temporary-file swap with one atomic write and preserve successful-save memo invalidation.

Co-authored-by: WeGoToMars <profigor7@gmail.com>
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 6, 2026, 12:59 AM ET / 04:59 UTC.

ClawSweeper review

What this changes

Replace pricing-catalog saves with one atomic destination write, add repeated-save coverage, and update the parser fingerprint, documentation, and changelog.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 3 items remain

The cache repair remains useful, but the generated fingerprint change unnecessarily rebuilds existing usage databases. This upgrade defect was missed in the earlier review of the same head.

Priority: P2
Reviewed head: ff7d103c7916a93cdaa5002b1814c72a108de8ef

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused cache repair is sound, but its fingerprint update introduces an avoidable upgrade regression.
Proof confidence 🌊 off-meta tidepool Not applicable: The OWNER-authored PR is exempt from contributor proof gating; its captured body reports Linux production-cache before/after saves with disk and memo checks, but does not exercise SQLite predecessor adoption.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The OWNER-authored PR is exempt from contributor proof gating; its captured body reports Linux production-cache before/after saves with disk and memo checks, but does not exercise SQLite predecessor adoption.
Evidence reviewed 7 items Cache repair and production caller: The save path now writes atomically to the destination and invalidates the memo only after success. Catalog refresh still validates incoming prices and merges retained fallback prices before saving; serialization, cache location, and refresh policy are unchanged.
Introduced upgrade trigger: The introduced fingerprint changes d2e66225d0b33672 to 9547dc9d7b7675f6. SQLite derives its schema version from this fingerprint, but its compatible predecessor set omits d2e66225d0b33672. Opening that existing database therefore raises incompatibleSchema and enters the rebuild path.
Database deletion on incompatibility: The rebuild path removes the SQLite database and its WAL/SHM files before opening a new database. The verified test merge leaves this compatibility and rebuild code unchanged.
Findings 1 actionable finding [P1] Preserve databases using the released parser fingerprint
Security None None.

How this fits together

CodexBar caches public model prices for local usage-cost calculations. Catalog refreshes write that cache, while a generated source fingerprint also controls compatibility of the separate SQLite usage database.

flowchart TD
 A[Public pricing catalog] --> B[Catalog refresh]
 B --> C[Atomic cache write]
 C --> D[Disk catalog and memory cache]
 D --> E[Usage cost calculations]
 F[Generated source fingerprint] --> G[SQLite compatibility check]
 G --> H[Preserve or rebuild usage database]
Loading

Before merge

  • Preserve databases using the released parser fingerprint (P1) - Changing this fingerprint also changes CostUsageStore.schemaVersion. The v0.56.6 fingerprint d2e66225d0b33672 is absent from compatiblePredecessorParserHashes, so opening an existing database throws incompatibleSchema and deletes/rebuilds its SQLite files. This cache-write repair changes no stored row format; register the predecessor and extend the adoption test to preserve rows/checkpoints with zero rebuilds. Otherwise users incur a full rescan and cannot recover cached history whose source files are gone. This is a late finding: the previous review examined the same unchanged head.
  • Resolve merge risk (P1) - Existing v0.56.6 usage databases are deleted and rebuilt after the fingerprint change; rows whose original session files are unavailable cannot be reconstructed. The supplied catalog-save validation does not cover this database upgrade.
  • Complete next step (P2) - Add compatibility for the released database fingerprint and verify upgrade preservation without a rebuild.

Findings

  • [P1] Preserve databases using the released parser fingerprint — Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production −7 lines; tests +27 lines The cache simplification reduces production code and adds focused repeated-save coverage.

Merge-risk options

Maintainer options:

  1. Preserve the released database (recommended)
    Allow the released predecessor fingerprint through the existing adoption path and prove persisted rows and checkpoints survive without rebuilding.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Add d2e66225d0b33672 to the compatible predecessor parser hashes and extend the existing predecessor-adoption regression to verify preserved snapshots and zero rebuilds; retain the regenerated fingerprint and atomic catalog write.

Technical review

Best possible solution:

Keep the atomic catalog write and adopt the released predecessor fingerprint without rebuilding, with explicit upgrade preservation coverage.

Do we have a high-confidence way to reproduce the issue?

Yes: opening a database stamped with the v0.56.6 fingerprint under this head follows the source-proven incompatible-schema rebuild path. This read-only review did not execute it.

Is this the best way to solve the issue?

The atomic write is an appropriate narrow repair, but the accompanying fingerprint update needs the existing compatible-predecessor mechanism to avoid unnecessary database destruction.

Full review comments:

  • [P1] Preserve databases using the released parser fingerprint — Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4
    Changing this fingerprint also changes CostUsageStore.schemaVersion. The v0.56.6 fingerprint d2e66225d0b33672 is absent from compatiblePredecessorParserHashes, so opening an existing database throws incompatibleSchema and deletes/rebuilds its SQLite files. This cache-write repair changes no stored row format; register the predecessor and extend the adoption test to preserve rows/checkpoints with zero rebuilds. Otherwise users incur a full rescan and cannot recover cached history whose source files are gone. This is a late finding: the previous review examined the same unchanged head.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 211781be79d3.

Labels

Label justifications:

  • P2: This is a bounded Linux pricing-cache reliability repair.
  • merge-risk: 🚨 compatibility: The changed fingerprint makes released SQLite usage databases incompatible without registering their existing format for adoption.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The OWNER-authored PR is exempt from contributor proof gating; its captured body reports Linux production-cache before/after saves with disk and memo checks, but does not exercise SQLite predecessor adoption.

Evidence

Acceptance criteria:

  • [P1] swift test --filter CostUsageStoreTests.
  • [P1] swift test --filter ModelsDevCacheReplacementTests.
  • [P1] make check.
  • [P1] make test.

What I checked:

  • Cache repair and production caller: The save path now writes atomically to the destination and invalidates the memo only after success. Catalog refresh still validates incoming prices and merges retained fallback prices before saving; serialization, cache location, and refresh policy are unchanged. (Sources/CodexBarCore/Vendored/CostUsage/ModelsDevPricing.swift:574, ff7d103c7916)
  • Introduced upgrade trigger: The introduced fingerprint changes d2e66225d0b33672 to 9547dc9d7b7675f6. SQLite derives its schema version from this fingerprint, but its compatible predecessor set omits d2e66225d0b33672. Opening that existing database therefore raises incompatibleSchema and enters the rebuild path. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore.swift:609, ff7d103c7916)
  • Database deletion on incompatibility: The rebuild path removes the SQLite database and its WAL/SHM files before opening a new database. The verified test merge leaves this compatibility and rebuild code unchanged. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore.swift:663, ff7d103c7916)
  • Released predecessor: The latest supplied release, v0.56.6, uses d2e66225d0b33672, so the compatibility defect affects an actual released database producer. Both that release and pinned main retain the old two-stage catalog replacement. (Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4, 1696c7a71c94)
  • Existing upgrade regression seam: The compatible-predecessor test already seeds stored rows and scanner state, compares snapshots after adoption, and asserts rebuildCount is zero; it does not include the released predecessor fingerprint. (Tests/CodexBarTests/CostUsageStoreTests.swift:1027, ff7d103c7916)
  • Captured validation and related work: The complete supplied PR body reports production cache failure on the second save before the fix and 100 successful saves afterward in the official Swift 6.3.3 Linux container, plus disk decoding, memo reads, and staging-file checks. It explicitly extracts the cache repair from feat(antigravity): estimate local history costs from models.dev prices #3412 while leaving that open Antigravity feature separate. Snapshot sourceRevision: 2c94d6333b817d5d40603667fdd58dfda7f46cdb8491c0c7b6f5e8d9a22a3acb. No reviewer execution was performed. (ff7d103c7916)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Esteve Castells: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Preserve the v0.56.6 fingerprint through compatible predecessor adoption and verify existing snapshots and checkpoints survive with zero rebuilds.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-06T04:35:27.702Z sha ff7d103 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(pricing): atomically replace cached catalogs on Linux This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@steipete
steipete merged commit 559bfc8 into main Sep 6, 2026
9 checks passed
steipete added a commit that referenced this pull request Sep 6, 2026
Follow the pricing-cache repair in #3444 by replacing the remaining redundant file swap with one atomic write. Preserve complete scan, pricing, timezone and provider state across repeated saves, and retain the best-effort persistence contract.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant