feat(copilot): read per-request input/cache from session-store.db - #946
feat(copilot): read per-request input/cache from session-store.db#946kelchm wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for reading Copilot per-request input/cache usage from ~/.copilot/session-store.db (SQLite) and uses that as the authoritative source for input/cache/reasoning when available, suppressing redundant session.shutdown rollups to avoid double counting. This improves correctness for crash scenarios (no clean shutdown rollup) and improves timestamp/day attribution by using per-request timestamps.
Changes:
- Add discovery + parsing for a new Copilot source type
session-storethat readsassistant_usage_eventsrows and emits supplementary calls (input/cache/reasoning only; output excluded). - Tag JSONL session-state sources as
shutdownCovered(or defer parsing when the DB is locked) so shutdown rollups are suppressed only when the DB coverage is known and usable. - Bump Copilot parse version and daily-cache version to force re-derivation under the new per-request accounting; add/expand unit + integration tests and update changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/providers/copilot.ts |
Adds session-store discovery/parsing, coverage-based suppression/deferral for JSONL shutdown rollups, and wiring for new sourceTypes. |
src/session-cache.ts |
Fingerprints CODEBURN_COPILOT_SESSION_STORE_DB and bumps Copilot parse version to heal cached sessions under the new source. |
src/daily-cache.ts |
Bumps daily cache schema/version to re-derive day attribution with per-request timestamps. |
tests/providers/copilot.test.ts |
Adds hermetic env stubbing plus extensive unit coverage for session-store parsing/suppression/locking behavior. |
tests/parser.test.ts |
Adds end-to-end durable-merge integration tests for growing JSONL legs and growing session-store DB rows. |
CHANGELOG.md |
Documents the new session-store source behavior and associated cache/version bumps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ae80de7 to
1dc69f3
Compare
|
Validation update, worth reviewer attention because it reframes the impact: tracing a reconciliation outlier on a second machine showed the |
ozymandiashh
left a comment
There was a problem hiding this comment.
Blocking changes required on this exact head:
-
This stack inherits both blockers from #945, and commit
1dc69f3adds another prohibited Claude co-author trailer. #945 must be corrected and merged or closed first; then this PR needs an updated base and fresh review. -
Coverage is captured during discovery (
src/providers/copilot.ts:2281-2295), store rows are read later, and JSONL suppression uses only the stale coverage set (:2747-2753). If Copilot commits a DB row and appendssession.shutdownbetween those phases, both the DB call and JSONL rollup are emitted. A production-shaped fixture doubled input 100, cache-read 8,000, and cache-write 2,000, and durable union can persist the duplication. Coverage and emitted rows need one coherent snapshot/transaction or equivalent parser-side suppression derived from the same rows. -
The
stat()path catches every error as absence. EACCES/EIO and other unknown failures must defer rather than admit potentially duplicate JSONL rollups; distinguish expected absence/schema cases from transient or permission errors.
Targeted tests (110/110), typecheck, CLI build, and diff check passed locally, but this head has no remote checks and remains stacked on the unmergeable #945.
efdaa9b to
a55ad07
Compare
ozymandiashh
left a comment
There was a problem hiding this comment.
Blocking race remains on exact head a55ad07. The parse-time store recheck is enabled only when discovery saw an uncovered events.jsonl mtime within 60 seconds of the probe (src/providers/copilot.ts:2847-2849; consumed at 864-869). For an active file older than 60 seconds, Copilot can commit its DB row and append session.shutdown after discovery stat but before parsing. The store parser emits the row while JSONL emits the same rollup; durable union preserves both incompatible keys. A production-shaped repro sets mtime 10 minutes old, discovers it with no recheckStore, inserts the covered DB row, appends shutdown, then parses both sources: expected zero shutdown calls, actual one, duplicating input/cache. The existing fresh-file test always falls inside the 60-second heuristic and misses this window. Every uncovered session capable of shutdown emission needs a parse-time coverage recheck, or coverage/emission must share one coherent snapshot. Prior trailer and stat-error blockers are fixed; 205 targeted tests, typecheck, CLI build, and diff check pass, but this adversarial case fails. Rebase/update after #945 lands and request fresh review.
4b19d07 to
bcab2f0
Compare
ozymandiashh
left a comment
There was a problem hiding this comment.
Blocking findings on exact head bcab2f0aec88fecf33e336ecbeccc8c1c746a218:
-
A normal JSONL assistant call and its matching session-store row are both counted as API/model calls and separate turns. The DB call at
src/providers/copilot.ts:2105-2125is supplementary accounting, but the downstream paths atsrc/parser.ts:2493-2501,1683,1717,1729give it ordinary behavioral weight. An end-to-end fixture produces 2 calls/turns where one real request occurred. Store accounting calls need zero call/turn weight while retaining input/cache/reasoning usage. -
A store row with NULL cwd/repository falls back to
session_idas project (src/providers/copilot.ts:2073-2080), while the matching JSONL source uses workspace.yaml (:2242-2255). Because the session grouping key includes project (src/parser.ts:3126-3128), one real session splits into two projects/sessions. Resolve the store row to the same workspace/session project identity, with a NULL-cwd regression. -
The stale-mtime double-count race is fixed, but the opposite same-refresh race remains. Discovery/parse orders the store before JSONL (
:2835,:2870); the store snapshot reads at:2020-2038. If a row commits after that read but before JSONL reaches shutdown, the later live recheck (:858-868) sees coverage and suppresses the rollup, while the already-read store emitted no row. That refresh loses all input/cache for the request. Coverage and emitted rows need one coherent snapshot or a retry/defer contract that cannot suppress against rows absent from the emitted store snapshot. -
New SQLITE_BUSY/retryable deferrals are still reported as complete in write mode (
src/parser.ts:3008-3012,3837-3857), so daily hydration can seal a day missing both store and rollup usage (src/daily-cache.ts:756-790,806-827). The #916 branch fixes this global hydration fence, but both branches independently claim daily-cache v18. Merge #916 first; then rebase this PR and use a distinct cache version.
The exact-head targeted suite (189 tests), typecheck, CLI build, diff check, and all five remote workflows pass, but they do not cover these invariants. Three production-shaped adversarial fixtures reproduce findings 1-3. Please keep this draft until all four are addressed and request a fresh review.
|
One important refinement to blocker 4: rebasing onto #916 is necessary but not sufficient for the current |
cc2340b to
31f1f02
Compare
The Copilot CLI and the GitHub Copilot desktop app both write ~/.copilot/session-store.db unconditionally; its assistant_usage_events table holds one row per API request. Until now input/cache tokens for these surfaces came only from the session.shutdown rollups in events.jsonl, which are written only on clean shutdown (a crash loses the whole leg's input/cache accounting) and lump each session leg into one per-model total. The DB rows are per-request, crash-proof, and carry real timestamps, so sessions the store covers now take input, cache-read, cache-write and reasoning tokens from the DB. The rollup also RESETS its counters at in-session compaction (traced on a clean single-process 107-request session whose sole rollup covered exactly its five post-compaction requests), so even cleanly-closed long sessions were truncated; on a long-history machine the store recovered ~35% of real Copilot spend lost to crashes and compaction resets. The store's input_tokens is cache-INCLUSIVE (input + cache_read + cache_write), the same convention as the shutdown rollups — verified against each row's token_details_json and by reconciling per-session sums against the CLI's own footers and rollups across two machines (1,380+ rows, 8 models, CLI 1.0.78/1.0.79, schema_version 6): every divergence was a rollup gap — crashed sessions with no rollup, or a compaction-reset rollup covering only its final accounting epoch. Emitted calls mirror the shutdown-call contract exactly: input/cache/reasoning only, output 0 — per-turn output stays owned by the events.jsonl assistant.message calls. Copilot reasoning tokens are no longer double-billed at the report layer: they are a SUBSET of the output tokens the per-turn calls already price (the store's token_details_json prices exactly input/cache/output — no reasoning line), but cachedCallToApiCall re-added reasoningTokens to output when re-deriving cost, charging them twice; copilot now joins claude in the reasoning-inside-output case. Demonstrated against a real store row before/after the fix. Rollup-vs-store precedence is enforced at SERVE time, and only there. Both representations always parse and cache; parseProviderSources drops a session's copilot:<sid>:shutdown:* calls whenever the serve set holds copilot-store:<sid>:* calls from a still-discovered store. This is the review's "one coherent snapshot" taken literally — the serve set IS the snapshot. The parser-side alternative (a discovery probe snapshotting covered sessions and tagging jsonl sources) was built first and then broken by three independent review passes: a session ending between probe and parse (rows commit before the shutdown line is appended) double-counted under the durable merge, an atomic store replacement between probe and parse could strand a session with neither representation, and a deleted store left orphaned cached rows suppressing rollups that had become the only live record. Read-time precedence closes all three at once — nothing a writer does between discovery and a parse can change what one serve pass sees — and it is the only mechanism compatible with the never-delete durable union merge: it heals persisted duplication (stale coverage epochs, runtimes without node:sqlite, restored files) instead of preserving it forever. It also follows the repo's existing read-time-precedence pattern between overlapping representations (buildDurablePeriod). seenKeys tricks still cannot substitute — the two paths' dedup keys share no shape and the durable cache keeps stale rollup calls alive regardless of parse order. Discovery reduces to schema validation: stat the file, then prepare-validate the parser's exact query (LIMIT 1). ENOENT/ENOTDIR, a missing sqlite driver, or "no such table/column" (older or future CLI builds) read as ABSENT — no source, rollups rule. EVERY other failure (EACCES/EIO on stat, busy, corrupt, mid-replace) emits the source anyway: its parse raises the busy shape parseProviderSources skips-and-retries, the path stays discovered, and serve-time suppression keeps holding from previously cached rows instead of flapping covered sessions' rollups back in. The store open sits inside the same classify-and-defer boundary as its query, so an EACCES/CANTOPEN race after a validated probe defers rather than caching a failed marker. session-state files no longer wait on the store for anything: a locked or corrupt store cannot stall rollup parsing, and billable rows with an empty model still price as 'unknown' rather than vanish. Absence epochs are the one deliberate trade: cached store rows whose file is no longer discovered (store deleted, ~/.copilot reset) stop suppressing — the rollups are the only live record then — and the overlap legs may double-count for up to 90 days until the orphaned entry ages out. The 90-day age-out correspondingly exempts still-discovered paths, so a long-idle machine's live store keeps serving rows older than the cutoff instead of zeroing sessions whose rollups stay suppressed. The inverse gate would under-count live sessions indefinitely; this bounds the error and converges. Two attribution hardenings ride along: store rows take their project from the session's own session-state dir (workspace.yaml cwd — the same label its per-turn output calls carry, so one session never splits across two projects), falling back to sessions.cwd → repository basename → sessionId only for sessions with no jsonl; and no store call is ever emitted with an empty timestamp (row created_at → previous row's timestamp → the sessions table's created_at, newly part of the shared SELECT). The copilot session cache keeps the session-store-v1 parse-version bump (pre-store caches must re-parse so the DB rows land; their cached rollup calls are then dropped at serve time), the session-store path override stays in the env fingerprint, and the daily cache stays at v18: totals and per-day attribution both change against pre-store builds — crash-recovered tokens land, reasoning double-billing leaves, and a midnight-straddling session lands its input/cache on the days the requests actually happened. Verified by A/B on a snapshot of a real store: the serve-time build and the parse-time build produce identical totals from a fresh cache, and re-running the serve-time build over a cache warmed by the pre-store branch heals it to the same totals (rollup-only $0.503 / 512,423 tokens → store-corrected $0.565 / 537,154, no double-count). Earlier rounds' verification carries over unchanged: A/B on snapshots of two real stores, a live SIGKILL crash test (row present, no rollup, tokens recovered), live resumes whose warm-cache deltas matched new rows to the token, and upgrade-healing at 4,800-session scale. Known residual cases: a CLI upgraded mid-session leaves leg 1 only in the rollup and leg 2 only in the DB — the DB wins for the session and leg 1's input/cache goes uncounted; and the absence-epoch over-count above. Billing-grade cost from total_nano_aiu and throughput from the latency columns are follow-ups (getagentseal#890).
31f1f02 to
ec4ed4f
Compare
|
@ozymandiashh All four findings on 2 — fixed. Store rows now take the session's jsonl-derived project (workspace.yaml cwd — the same label its per-turn calls carry) via a sessionId→project map attached to the store source at discovery; 3 — fixed structurally. Precedence moved to serve time, taking "one coherent snapshot" literally: parsers always emit and cache both representations, and 1 — agreed; next head. Store accounting calls will carry zero call/turn weight with usage/cost/tokens retained. That threads a field through the cached-call schema and the stats/daily paths, so I'll land it on the post-#916 rebase rather than churn this head twice. One question: should the shutdown-rollup calls — same supplementary contract — go zero-weight too, or store rows only? 4 — agreed with your sequencing. After #916 lands: rebase, next distinct daily-cache version, finding 1 on that head, fresh review requested then. -- Refactor A/B on a real-store snapshot: the serve-time and parse-time heads produce identical fresh-cache totals; pre-store |
ozymandiashh
left a comment
There was a problem hiding this comment.
Blocking findings on exact head ec4ed4f2bc371ae7a8c76b90e22b5d2f0afa9c48:
-
Supplementary session-store rows still fabricate behavioral activity.
src/providers/copilot.ts:2111-2131emits each row as a normal call; calls withoutturnIdbecome separate turns atsrc/parser.ts:2493-2500, then increment turns/API/model calls at:1683, :1717, :1729. A production-path fixture with one normal JSONL assistant call plus its matching store row producedapiCalls=2, turns=2, modelCalls=2; expected1/1/1. Store rows need accounting-only/zero behavioral weight while retaining tokens and cost. -
A deferred changed-store read still reports hydration complete. The retryable catch at
src/parser.ts:3008-3012continues without recording incompleteness, and:3852-3872can therefore let daily history advance. A warm-cache fixture changed the discovered store into an unreadable file:isSessionHydrationComplete()wastrue, expectedfalse. -
The accepted absence epoch is not bounded once daily history seals. The committed test at
tests/parser.test.ts:1113-1144intentionally doubles live input from 600 to 1,200 while the store is absent. In an end-to-end daily-cache fixture, restoring the store made live usage converge to 600, but the finalized day remained permanently at 1,200. ENOENT is normal absence, so a retryable-only fence does not repair this. -
Same-path DB reset/replacement can reuse durable dedup keys and lose new usage. The key at
src/providers/copilot.ts:2080-2085is onlycopilot-store:<sessionId>:<rowId>; SQLite AUTOINCREMENT prevents reuse only within one database lifetime. Recreating the DB at the same path with the same session and a new rowid=1left the old 100-input call in the durable union instead of the new 200-input call (src/parser.ts:2980-2993rejects the reused key). -
Sequencing remains required: this head and the pending hydration/accounting fix both claim daily-cache v18. Land the hydration/accounting boundary first, then rebase this PR and use a distinct version so existing v18 data cannot be accepted under two meanings.
The Round-5 serve-time refactor does fix the prior project-alignment and two snapshot-order races, and all five remote workflows are green. Targeted session-store/serve tests, typecheck, builds, and diff-check also pass. The reproduced invariants above remain merge blockers; please add end-to-end regressions for each and request a fresh review.
|
Two additional exact-head blockers from the independent pass on
The same audit also confirmed that accepting #916's independently-defined v18 through this PR strips its |
|
On the call-weight question: do not make only store rows unconditionally zero-weight.
This likely needs an explicit count weight or a logical-request reconciliation rule, not just a source-wide boolean. Please pin at least: normal JSONL+matching row = 1 call/turn; JSONL+shutdown aggregate = JSONL call count only; store-only crashed request = 1 call; and partial store coverage + rollup preserves the uncovered tail exactly once. |
Why
The Copilot CLI and the GitHub Copilot desktop app both write
~/.copilot/session-store.dbunconditionally. Itsassistant_usage_eventstable records one row per API request, as it happens, with real timestamps.Until now, input/cache tokens for these surfaces came only from
session.shutdownrollups inevents.jsonl. Validating against two machines' real stores showed that rollup to be lossy in three independent ways:On a long-history validation machine (4,826 sessions, 1,360 store rows, 8 models), reading the store instead recovered ~35% of real Copilot spend ($165 → $256). The entire delta reconciles exactly to the rollup gaps above.
What this PR does
Adds a
session-storesource: one source per DB file (mirrors the OTel pattern — single lazyopenDatabase, iterate everything, close infinally). Env override:CODEBURN_COPILOT_SESSION_STORE_DB(default~/.copilot/session-store.db).Per-row calls
Each billable row becomes a supplementary call with input, cache-read, cache-write, and reasoning tokens, and
outputTokens: 0. Every billable row is emittable by construction — a row with an empty model (allowed byTEXT NOT NULL) is priced asunknownrather than dropped, since a served row suppresses its session's rollup on the promise that the rows replace it. Per-turn output, tools, and userMessage stay on theevents.jsonlper-turn calls.input_tokensis cache-INCLUSIVE (input + cache_read + cache_write), same convention as the rollups; the parser emits the uncached remainder. Verified three ways:token_details_json(tokenType:"input"entry is exactly the remainder)Zero cache-inclusive violations across 1,380+ real rows.
Precedence — serve-time only
Both representations always parse and cache.
parseProviderSourcesdrops a session'scopilot:<sid>:shutdown:*calls whenever the serve set holdscopilot-store:<sid>:*calls from a still-discovered store — applied identically in the live-source pass and the durable-orphan pass. That is the whole mechanism.This is the "one coherent snapshot" framing taken literally: the serve set is the snapshot. The parser-side alternative (a discovery probe snapshotting covered sessions and tagging
events.jsonlsources — what rounds 1–4 of this PR built and hardened) was ultimately broken by three independent review passes, each finding a way for the world to change between the probe and a parse:Read-time precedence closes the whole class at once: nothing a writer does between discovery and a parse can change what one serve pass sees. It is also the only mechanism compatible with the never-delete durable union merge — it heals persisted duplication (stale coverage epochs, runtimes without
node:sqlite, restored files, any historical double-cached state) instead of preserving it forever — and it follows the repo's existing pattern of read-time precedence between overlapping representations (buildDurablePeriod).The still-discovered gate is the absence-epoch fix: cached rows orphaned by a deleted/reset store must not shadow fresh rollups that are now the only live record. The 90-day age-out correspondingly exempts still-discovered paths, so a long-idle machine's live store keeps serving rows older than the cutoff instead of zeroing sessions whose rollups stay suppressed. In read-only serves the gate uses the genuinely-discovered source set (read-only runs add cached orphans to
allDiscoveredFiles, and an absent store must not suppress in read-only when it wouldn't in a refresh).seenKeysordering tricks still cannot substitute: the two paths' dedup keys share no shape, thenincopilot:<sid>:shutdown:<model>:<n>is unbounded, and the durable union-by-dedup-key cache keeps stale rollup calls alive regardless of parse order.Failure semantics
Discovery reduces to schema validation:
stat(), then prepare-validate the parser's exact query (LIMIT 1— shared verbatim with the parser so the two can never diverge on schema). Measured against the real driver (node:sqlite, WAL store): write locks never block readers, and a hot-walwithout its-shmreads fine; reachable failures are corruption-class (SQLITE_CORRUPT/NOTADB/CANTOPEN).stat()says ENOENT/ENOTDIR, no sqlite driver, orno such table/column(CLI builds predating the store, or a future migration)stat()EACCES/EIO, locked, corrupt, mid-replace, or the store failing at open or mid-parse after a validated probeparseProviderSourcesalready skips-and-retries; no cache write. The path stays discovered, so previously cached rows keep serving and serve-time suppression keeps holding — covered sessions' rollups don't flap back in during a transient failureSession-state files no longer wait on the store for anything: a locked or corrupt store cannot stall or defer
events.jsonlparsing, which removes the old synthetic busy-deferral path entirely.Reasoning tokens
Reasoning tokens are metadata, not a cost line — now true end to end. They are a subset of
output_tokens(the store'stoken_details_jsonprices input/cache/output only), and per-turn calls already bill the full output.cachedCallToApiCallpreviously re-addedreasoningTokensto output for non-claude providers, double-billing them (~$13 of phantom cost on machine 2's ~533K priced reasoning tokens; demonstrated on a real row, $0.00520 → $0.00379). Copilot now joins claude in the reasoning-inside-output case. The integration test pins the query-path cost rather than the parser-level value that gets discarded.Mechanics
copilot-store:<sessionId>:<rowId>—idisAUTOINCREMENT(verified in real DDL withsqlite_sequencepresent); keys are stable and never reused; a growing store appends only new keys under the durable mergecreated_at(every observed row is ISO-Z; zoneless SQL-default shapes normalized to UTC defensively). No call is ever emitted with an empty timestamp — fallback chain: rowcreated_at→ previous row's timestamp (ids are insertion-ordered) → the sessions table'screated_at, newly part of the shared SELECTsessions.cwd→ repository basename → sessionId only for sessions with no jsonl.dbpaths already route through the-walfold from #913Cache healing
PROVIDER_PARSE_VERSIONSentry keeps-session-store-v1(pre-store caches re-parse so the DB rows land; their cached rollup calls are then dropped at serve time)CODEBURN_COPILOT_SESSION_STORE_DBstays in the env fingerprint (uniquely among the copilot overrides — repointing it changes which store's rows the serve set holds)DAILY_CACHE_VERSIONstays at the 17→18 bump: totals change against pre-store builds (rollup-gap tokens land, reasoning double-billing leaves) and per-day attribution moves to real request daysRe-derivation rides the v14 carry-forward semantics.
Accepted residual cases
Documented in code:
sessions.cwdlabels; workspace.yaml is written at session start, before any row, so a live mid-session flip is required to hit it).CODEBURN_COPILOT_SESSION_STORE_DBsheds the old store's durable entries (the env-fingerprint reset carries forward only entries whose file is gone; a still-on-disk but no-longer-configured store is neither carried nor re-discovered). Deliberate since round 1 — the override changes which sessions' rollups were suppressed — and bounded: the affected sessions' rollups come back, so only crash-only rows (which have no rollup) lose their input/cache.Out of scope
Follow-ups:
total_nano_aiu/request_multiplier(upstream Cost modes: reconcile displayed cost with the source's own reported cost (auto/calculate/display) #890)Validation
Version coverage across both machines: store-writing CLI builds
1.0.70,1.0.78,1.0.78-2,1.0.79all reconcile (the sole divergence is the 1.0.78 compaction reset above). The store first appears between CLI1.0.67and1.0.70— sessions from older builds have no DB rows and keep the rollup path, exactly the absence case the design falls back on.Round-5 refactor A/B (this head vs the parse-time head)
On a fresh snapshot of a real store (
VACUUM INTO+session-statecopy):mainon the same snapshot: $0.503 / 512,423 (the rollup-gap loss)main: heals to $0.565 / 537,154 — cached rollups drop at serve time, no double-countMachine 1 (dev machine, CLI 1.0.78/1.0.79)
Callsgrows with per-request granularityoverviewrunning concurrently with a streaming CLI session read cleanlydescribe.skipIf(!isSqliteAvailable()))Machine 2 (independent, 4,826 sessions)
schema_version6)(sessionId, project)inventories are identical — one Aug-6 session with a next-morning shutdown simply leaves Aug 7's daily bucket, which is the per-day attribution correction the v18 bump exists for, observed liveProcess
The diff went through five independent adversarial review rounds (grok-4.5 ×2, Claude Opus, gpt-5.6-sol at max effort, plus a post-refactor pass) and GitHub Copilot's reviewer, alongside the maintainer review. Rounds 1–4 hardened the parse-time design; round 5's converging finding — three reviewers independently identifying probe-vs-parse coherence gaps (covered→uncovered transitions, atomic replacement, absence epochs) — motivated this refactor to serve-time-only precedence, which closes the class structurally rather than case by case. Maintainer findings from the review round (the coverage-snapshot race,
stat()failure classification, serve-set coherence) remain covered by verbatim-repro regression tests at the serve level.One known pre-existing gap is deliberately out of scope: a write-mode parse that busy-defers a source still reports session hydration as complete, so the daily cache can finalize a day derived without the deferred source's contribution. This predates the PR (every SQLite provider's busy-skip shares it; #856 fixed the read-only half); the store source's defer path has the same exposure, and a follow-up PR will make deferrals mark hydration incomplete. Every surviving finding was either fixed in this commit or refuted with evidence recorded above.
Integration tests exercise the production durable-merge path end to end.
npx tsc --noEmitclean;npx vitest rungreen apart from known environmental failures unrelated to this change.