Skip to content

Persist the codex priority-turns memo across launches#1421

Closed
ProspectOre wants to merge 9 commits into
steipete:mainfrom
ProspectOre:perf/codex-priority-turns-persist
Closed

Persist the codex priority-turns memo across launches#1421
ProspectOre wants to merge 9 commits into
steipete:mainfrom
ProspectOre:perf/codex-priority-turns-persist

Conversation

@ProspectOre

Copy link
Copy Markdown
Contributor

Stacked on #1404 — this branch contains #1404's commits plus one new commit (e7f3e20e). Once #1404 lands I'll rebase so only the persistence change remains in the diff.

Problem

The priority-turns memo introduced in #1404 makes live refreshes incremental, but it is process-local. Every app launch and every CLI invocation pays one full logs_2.sqlite scan (tens of seconds on large trace databases) before incremental refreshes resume — the disclosed P1 limitation from #1404's review.

Change

Persist the memo to disk following the existing cost-usage cache conventions:

  • New artifact cost-usage/codex-priority-turns-v1.json with a producerKey embedding the parser hash (codex:pt:p<hash>), so a parser change falls back to one full rescan — same invalidation contract as the codex cost cache.
  • Atomic tmp-file + replaceItemAt writes; corrupted or version-mismatched artifacts are ignored.
  • Seeding happens once per process through the monotonic store from Resolve codex priority turns incrementally per refresh #1404, so persisted state can never regress a scan that already ran; the cursor/coverage/file-identity rescan checks all still apply to the seeded state (a replaced or truncated database invalidates exactly as before).
  • A dirty flag gates writes: the artifact is rewritten only after a scan actually advances the memo.
  • Load and persist are wired at the single codexPriorityTurns call site in the codex scan, which runs on the CostUsageScanExecutor serial queue (Run cost-usage corpus scans off the Swift cooperative thread pool #1402), so the synchronous file IO stays off the cooperative pool and the main actor.
  • Memo contents are metadata only (thread/turn IDs, model names, timestamps) — no prompt or response bodies, matching what the parsers already strip.

Runtime Proof (real database, redacted)

Two separate swift test processes against the live trace database — a true relaunch boundary. Cold first, then warm from the persisted artifact:

=== RUN 1: cold (no artifact) ===
PROOF[cold] db=logs_2.sqlite dbBytes=798629888
PROOF[cold] diskLoad=3.3ms scan=12313.6ms persist=2.7ms
PROOF[cold] priorityTurns=5 memoCursor=387421516 retainedCompletions=288 artifactBytes=28698

=== RUN 2: warm (fresh process, seeded from artifact) ===
PROOF[warm] db=logs_2.sqlite dbBytes=798629888
PROOF[warm] diskLoad=1.8ms scan=2.8ms persist=10.5ms
PROOF[warm] priorityTurns=5 memoCursor=387421705 retainedCompletions=288 artifactBytes=28698
  • Relaunch cost: 12,313.6 ms → 4.6 ms (1.8 ms artifact load + 2.8 ms incremental scan) on a 798 MB database.
  • The warm run's cursor advanced past the cold run's (387421705 > 387421516): rows appended between the two runs were picked up incrementally from the persisted cursor, with identical turn results.
  • Artifact size: 28 KB for a 798 MB database.

Validation

  • swift test --filter CostUsageScannerCodexPriority — 16 tests pass (13 from Resolve codex priority turns incrementally per refresh #1404 + 3 new: relaunch roundtrip with cursor continuity, producer-key/version mismatch discard, corrupted artifact ignored).
  • make check — 0 violations in 1043 files.
  • Scripts/regenerate-codex-parser-hash.sh check — hash committed (the new source file participates in the hash, so this release triggers the usual one-time rescan).

No CHANGELOG edit per current review guidance (release-owned).

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep open for maintainer review: the persistence change has strong live proof, but the current branch still changes the global Codex parser hash and can invalidate existing session-cost caches; merged work in #1430 also means maintainers should explicitly decide whether this stateful persistence path is still wanted.

Canonical path: Close this PR as superseded by #1430.

So I’m closing this here and keeping the remaining discussion on #1430.

Review details

Best possible solution:

Close this PR as superseded by #1430.

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

Yes for the merge blocker: source inspection shows Codex session cache keys are derived from CodexParserHash.value, and the PR changes that value without adding the current main key to the compatible set. The performance benefit itself is also supported by the PR body's two-process live output.

Is this the best way to solve the issue?

No as-is. Persisting the priority-turn memo is plausible, but the merge path should first preserve session-cache compatibility and get a maintainer decision on whether the merged file-stat approach already supersedes this stateful cache.

Security review:

Security review cleared: The diff adds local metadata cache serialization and tests, with no dependency, workflow, credential, permission, or downloaded-code changes.

AGENTS.md: found and applied where relevant.

What I checked:

  • linked superseding PR: perf: reduce Codex cost refresh metadata work #1430 (perf: reduce Codex cost refresh metadata work) is merged at 2026-06-11T10:36:19Z.
  • cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • steipete: Recent main history owns the merged file-stat performance replacement, compatible producer-key handling, and follow-up fixups on this PR branch. (role: recent area contributor and merger; confidence: high; commits: dd8cf8b06ebb, edc193fd0bef, 0874abaa41a6; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageCache.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CacheHelpers.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriorityPersistence.swift)
  • ProspectOre: Beyond opening this PR, this contributor authored the persistence commit and is credited in the merged scan-performance work that now shapes the remaining product decision. (role: recent feature contributor; confidence: high; commits: fc4941640c83, dd8cf8b06ebb, 4c964ccc91e0; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriorityPersistence.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriority.swift, Tests/CodexBarTests/CostUsageScannerCodexPriorityTests.swift)
  • iam-brain: The Codex priority-cost parsing, scanner, cache, pricing, and tests appear to date to this merged feature commit. (role: feature introducer; confidence: medium; commits: 2f6a31c23e2c; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriority.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Tests/CodexBarTests/CostUsageScannerCodexPriorityTests.swift)

Codex review notes: model internal, reasoning high; reviewed against dd8cf8b06ebb.

@ProspectOre

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 11, 2026
steipete and others added 9 commits June 11, 2026 11:37
Co-authored-by: pickaxe <54486432+ProspectOre@users.noreply.github.com>
The rowid-cursor memo keeps live refreshes incremental, but it is process-local:
every app launch and every CLI invocation pays one full trace-database scan
before incremental refreshes resume. Persist the memo to a versioned cost-usage
artifact (parser-hash producer key, atomic replace-on-write) and seed it once
per process through the monotonic store, so the first refresh after launch
resumes from the persisted cursor.
@steipete steipete force-pushed the perf/codex-priority-turns-persist branch from e7f3e20 to 0874aba Compare June 11, 2026 10:53
@steipete

Copy link
Copy Markdown
Owner

Rebased onto the reviewed #1404 stack and force-updated with an exact lease.

Maintainer fixups:

  • keep memo persistence portable on Linux by using the existing lock pattern instead of OSAllocatedUnfairLock
  • normalize persisted observation IDs across process launches
  • honor forceRescan by invalidating the selected trace-database memo before rebuilding it
  • add relaunch, stale/corrupt artifact, and forced-cursor-discard regressions

Proof:

  • swift test --filter 'CostUsageScanner(Priority|CodexPriority)Tests' (37 tests passed)
  • swift test --filter CostUsage (200 tests passed)
  • make check (parser hash current, SwiftFormat clean, SwiftLint 0 violations)
  • structured autoreview against the Resolve codex priority turns incrementally per refresh #1404 parent: clean after fixing the force-rescan finding

@ProspectOre

Copy link
Copy Markdown
Contributor Author

Linux CI failure on 0874abaa is the canImport(SQLite3) guard: invalidateCodexPriorityTurnsMemo is only defined inside the SQLite3 block, but the forceRescan call site in CostUsageScanner.swift:2197 compiles unconditionally — needs a no-op stub in the #else branch alongside the load/persist stubs.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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 Jun 11, 2026
@steipete

Copy link
Copy Markdown
Owner

Superseded by the smaller measured fix in #1430, landed as dd8cf8b.

Exact-head profiling on the same 60,607-file real archive showed the priority SQLite scan was about 4% of the expired-refresh cost; repeated Foundation metadata/resource-identifier reads and cached-path validation were dominant. The landed replacement removes that cost with +124/-24 lines, no persisted cursor/memo state, and measured a 6.74s warm expired refresh versus 20.22-37.43s across the stacked candidates.

Thanks @ProspectOre for the investigation, implementation work, and benchmark evidence. Contributor credit is retained in the landed commit and changelog.

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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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.

2 participants