Skip to content

feat(copilot): read per-request input/cache from session-store.db - #946

Open
kelchm wants to merge 1 commit into
getagentseal:mainfrom
kelchm:feat/copilot-session-store
Open

feat(copilot): read per-request input/cache from session-store.db#946
kelchm wants to merge 1 commit into
getagentseal:mainfrom
kelchm:feat/copilot-session-store

Conversation

@kelchm

@kelchm kelchm commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Rebased onto main after #945 merged (3536a1d); this PR is now a single commit.

Round 5: precedence machinery refactored to serve-time-only per the review discussion — the parse-time suppression (coverage probe, shutdownCovered tags, parse-time re-check, busy-deferral of session-state files) is deleted; the serve set is now the one coherent snapshot. Net source diff shrank by ~100 lines. Totals are A/B-identical to the previous head.

Why

The Copilot CLI and the GitHub Copilot desktop app both write ~/.copilot/session-store.db unconditionally. Its assistant_usage_events table records one row per API request, as it happens, with real timestamps.

Until now, input/cache tokens for these surfaces came only from session.shutdown rollups in events.jsonl. Validating against two machines' real stores showed that rollup to be lossy in three independent ways:

  1. Clean-shutdown only. A crashed or killed session loses its whole leg's input/cache accounting. Reproduced live: SIGKILL mid-turn → DB row present, no rollup.
  2. Counters reset at in-session compaction. Traced on a clean, single-process, 107-request CLI 1.0.78 session whose sole rollup matched its five post-compaction requests exactly, per model, to the token — omitting the prior 102 requests. Any long session that compacts is silently truncated, crash or not.
  3. One per-model total stamped at shutdown time. A session that ran on Aug 6 but closed the next morning put all its input/cache on Aug 7.

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-store source: one source per DB file (mirrors the OTel pattern — single lazy openDatabase, iterate everything, close in finally). 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 by TEXT NOT NULL) is priced as unknown rather 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 the events.jsonl per-turn calls.

input_tokens is cache-INCLUSIVE (input + cache_read + cache_write), same convention as the rollups; the parser emits the uncached remainder. Verified three ways:

  • each row's own token_details_json (tokenType:"input" entry is exactly the remainder)
  • footer reconciliation on machine 1
  • 18-comparison rollup reconciliation on machine 2 (16/18 exact; both divergences were the rollup gaps above, always DB > rollup)

Zero cache-inclusive violations across 1,380+ real rows.

Precedence — serve-time only

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 — 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.jsonl sources — 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:

  • a session ending between probe and parse (rows commit before the shutdown line is appended) double-counted under the durable merge — patched with a parse-time re-check, which then needed its own failure taxonomy;
  • an atomic store replacement between probe and parse could strand a session with neither representation;
  • a deleted store left orphaned cached rows suppressing rollups that had become the only live record.

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).

seenKeys ordering tricks still cannot substitute: the two paths' dedup keys share no shape, the n in copilot:<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 -wal without its -shm reads fine; reachable failures are corruption-class (SQLITE_CORRUPT / NOTADB / CANTOPEN).

Condition Behavior
True absence — stat() says ENOENT/ENOTDIR, no sqlite driver, or no such table/column (CLI builds predating the store, or a future migration) No source; rollups rule
Anything else — stat() EACCES/EIO, locked, corrupt, mid-replace, or the store failing at open or mid-parse after a validated probe The source is emitted anyway. Its parse raises the busy shape parseProviderSources already 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 failure

Session-state files no longer wait on the store for anything: a locked or corrupt store cannot stall or defer events.jsonl parsing, 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's token_details_json prices input/cache/output only), and per-turn calls already bill the full output.

cachedCallToApiCall previously re-added reasoningTokens to 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

Piece Detail
Dedup keys copilot-store:<sessionId>:<rowId>id is AUTOINCREMENT (verified in real DDL with sqlite_sequence present); keys are stable and never reused; a growing store appends only new keys under the durable merge
Timestamps From created_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: row created_at → previous row's timestamp (ids are insertion-ordered) → the sessions table's created_at, newly part of the shared SELECT
Project attribution Store rows take the project of 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); sessions.cwd → repository basename → sessionId only for sessions with no jsonl
WAL fingerprinting No change — .db paths already route through the -wal fold from #913

Cache healing

  • copilot's PROVIDER_PARSE_VERSIONS entry 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_DB stays in the env fingerprint (uniquely among the copilot overrides — repointing it changes which store's rows the serve set holds)
  • DAILY_CACHE_VERSION stays 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 days

Re-derivation rides the v14 carry-forward semantics.

Accepted residual cases

Documented in code:

  • A CLI upgraded mid-session leaves leg 1 only in the rollup and leg 2 only in the DB; the DB wins for the whole session and leg 1's input/cache goes uncounted.
  • Absence epochs over-count, bounded. If the store is deleted or reset while its rows are durably cached, those orphaned rows stop suppressing (the rollups are the only live record then), so the overlap legs count from both representations for up to 90 days until the orphan ages out. This replaces the previous design's failure mode — orphaned rows suppressing live rollups would under-count indefinitely — with a bounded, converging over-count. Regression test (o) pins the exact totals. One sub-case (post-refactor adversarial pass): if the daily cache finalizes an affected day during the epoch — i.e., the store was deleted the same day as the usage — that day's inflated total persists in daily history after live totals converge. This is the hydration-fence exposure class (finding 4 / Session-cumulative providers (hermes) lose post-finalization usage from the daily history #916) and rides that fix.
  • Cached store calls keep their first-served project label. The durable merge appends and never updates, so a session whose jsonl-derived label changes after its rows were cached keeps the old label on those rows (same staleness class as the pre-refactor sessions.cwd labels; workspace.yaml is written at session start, before any row, so a live mid-session flip is required to hit it).
  • Repointing CODEBURN_COPILOT_SESSION_STORE_DB sheds 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.
  • A permanently unreadable store stalls its own updates indefinitely (every parse defers; previously cached rows keep serving and keep suppressing, so rows written after the corruption never land while it persists). Transient corruption — the reachable kind, e.g. mid-replace — heals on the next successful read; the permanent limit is accepted because the alternatives are worse: reading corruption as absence double-counts every covered session, and the previous design stalled all session-state parsing behind it.

Out of scope

Follow-ups:

Validation

Version coverage across both machines: store-writing CLI builds 1.0.70, 1.0.78, 1.0.78-2, 1.0.79 all reconcile (the sole divergence is the 1.0.78 compaction reset above). The store first appears between CLI 1.0.67 and 1.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-state copy):

  • serve-time head and parse-time head, fresh caches: identical totals ($0.565 / 537,154 tokens) — the refactor preserves accounting exactly
  • pre-store main on the same snapshot: $0.503 / 512,423 (the rollup-gap loss)
  • serve-time head re-run over the cache warmed by pre-store main: heals to $0.565 / 537,154 — cached rollups drop at serve time, no double-count

Machine 1 (dev machine, CLI 1.0.78/1.0.79)

  • A/B on snapshots vs the pre-change branch: byte-identical totals where rollups were complete; only Calls grows with per-request granularity
  • Live crash test (SIGKILL mid-turn): pre-change build loses the leg; this branch recovers it exactly (+2 input / +24,729 cache-write, output untouched)
  • Live resumes: each warm-cache refresh delta matched the new DB rows to the token; cumulative leg rollups suppressed; overview running concurrently with a streaming CLI session read cleanly
  • Upgrade healing: a cache warmed by the pre-change build heals in place — a stale-cache double-count would have doubled input; it didn't
  • Error-shape lab against the real driver captured the actual codes behind the failure classification
  • Unit tests: both-representations parse contract, locked/corrupt/unstat-able stores still surface their source and defer only their own parse, mid-run schema migration defers the store parse, schema-mismatch reads as absent, cache-inclusive decomposition, multi-model sessions, dedup stability, UTC + fallback-chain timestamps, jsonl-preferred project attribution in both the NULL-cwd and differing-cwd shapes (fixture DBs built in-code, describe.skipIf(!isSqliteAvailable()))
  • Serve-level integration tests: growing-store durable merge (i), rollup-day reattribution (j), stale-cache healing (k), age-out exemption for still-discovered stores (l), the rows-then-shutdown race counted once (m — the round-2 repro, now raceless by construction), no-billable-rows-never-suppress covering both the atomic-replacement shape and the zero-usage-row predicate (n), absence-epoch behavior (o), and the reverse race — suppression never outruns the served store rows, so a row committing after the store read leaves the rollup counting instead of zeroing the refresh (p)

Machine 2 (independent, 4,826 sessions)

  • Identical schema (schema_version 6)
  • 1,360/1,360 ISO-Z timestamps
  • 0 cache-inclusive violations
  • 16/18 exact rollup reconciliations; both divergences root-caused (crashed sessions with no rollup; the compaction-reset rollup)
  • A/B delta reconciles exactly to those gaps with output delta 0
  • Upgrade-healing output byte-identical to a fresh-cache run at 4,800-session scale
  • One headline difference (4,826 → 4,825 sessions) traced to completion: lifetime (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 live

Process

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 --noEmit clean; npx vitest run green apart from known environmental failures unrelated to this change.

Copilot AI lite review requested due to automatic review settings August 7, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-store that reads assistant_usage_events rows 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.

Comment thread src/providers/copilot.ts Outdated
Comment thread tests/providers/copilot.test.ts
@kelchm
kelchm marked this pull request as draft August 7, 2026 21:11
@kelchm
kelchm force-pushed the feat/copilot-session-store branch 4 times, most recently from ae80de7 to 1dc69f3 Compare August 7, 2026 23:48
@kelchm

kelchm commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Validation update, worth reviewer attention because it reframes the impact: tracing a reconciliation outlier on a second machine showed the session.shutdown rollup resets its counters at in-session compaction. A clean, single-process, 107-request CLI 1.0.78 session's sole rollup matched its five post-compaction requests exactly (per model, to the token) and omitted the prior 102 requests. So rollup-only accounting truncates any session that compacts — not just crashed ones — and on that machine the store recovered ~35% of real Copilot spend overall. The delta-with-reset-detection logic from #944 remains correct for multi-rollup files; the reset trigger is now confirmed and documented at the code site. Full details in the updated description.

@kelchm
kelchm marked this pull request as ready for review August 8, 2026 00:11

@ozymandiashh ozymandiashh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking changes required on this exact head:

  1. This stack inherits both blockers from #945, and commit 1dc69f3 adds 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.

  2. 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 appends session.shutdown between 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.

  3. 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.

@kelchm
kelchm force-pushed the feat/copilot-session-store branch 2 times, most recently from efdaa9b to a55ad07 Compare August 9, 2026 01:32

@ozymandiashh ozymandiashh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kelchm
kelchm force-pushed the feat/copilot-session-store branch 2 times, most recently from 4b19d07 to bcab2f0 Compare August 9, 2026 02:03
@kelchm
kelchm marked this pull request as draft August 9, 2026 02:13

@ozymandiashh ozymandiashh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking findings on exact head bcab2f0aec88fecf33e336ecbeccc8c1c746a218:

  1. 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-2125 is supplementary accounting, but the downstream paths at src/parser.ts:2493-2501,1683,1717,1729 give 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.

  2. A store row with NULL cwd/repository falls back to session_id as 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.

  3. 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.

  4. 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.

@ozymandiashh

Copy link
Copy Markdown
Collaborator

One important refinement to blocker 4: rebasing onto #916 is necessary but not sufficient for the current storeProbeBusy sentinel. #916 marks hydration incomplete when discovery reports a typed retryable failure or when a changed source parser throws. This branch currently converts busy store discovery to sessionStore === 'busy', tags JSONL sources storeProbeBusy, and throws only when their parser runs. On a warm cache where those JSONLs are unchanged, createSessionParser is never invoked, so the retryable failure still never reaches the hydration fence. Please add a production-path regression for: busy store + unchanged cached JSONL => hydration incomplete and daily watermark held, and propagate the busy discovery result directly into the fence (or an equivalent source-independent signal).

@kelchm
kelchm force-pushed the feat/copilot-session-store branch 2 times, most recently from cc2340b to 31f1f02 Compare August 9, 2026 02:55
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).
@kelchm
kelchm force-pushed the feat/copilot-session-store branch from 31f1f02 to ec4ed4f Compare August 9, 2026 03:02
@kelchm

kelchm commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@ozymandiashh All four findings on bcab2f0 are addressed or sequenced. Head: ec4ed4f. Description rewritten (Precedence / Failure semantics / Validation) for the design change below.

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; sessions.cwd → repository → sessionId applies only to sessions with no jsonl. Regression covers your verbatim NULL-cwd/repository shape plus the differing-cwd shape.

3 — fixed structurally. Precedence moved to serve time, taking "one coherent snapshot" literally: parsers always emit and cache both representations, and parseProviderSources drops a session's shutdown calls only when the serve set holds that session's store rows from a still-discovered store. No probe snapshot and no live re-check remain, so suppression cannot outrun the emitted rows by construction. Your repro is pinned at serve level: test (p) — row commits after the store read → the rollup counts, nothing zeroes; next refresh swaps to the row, once. The rest of the transition family rides alongside: (m) rows-then-shutdown counted once, (k) stale-cache healing, (n) atomic replacement / zero-usage rows, (o) absence epochs. The parse-time machinery (coverage probe, shutdownCovered, re-check, session-state busy-deferral) is deleted — net −96 source lines — and a locked/corrupt store now defers only its own re-read while cached rows keep serving. Bounded residual documented in the description: orphaned rows of a deleted store stop suppressing, so overlap legs can double-count ≤90d until age-out, replacing the old indefinite under-count.

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 main reads $0.503 / 512,423 and this head heals that same cache in place to $0.565 / 537,154. tsc clean; 122 targeted tests; full suite green modulo known environmental failures.

@kelchm
kelchm marked this pull request as ready for review August 9, 2026 03:22

@ozymandiashh ozymandiashh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking findings on exact head ec4ed4f2bc371ae7a8c76b90e22b5d2f0afa9c48:

  1. Supplementary session-store rows still fabricate behavioral activity. src/providers/copilot.ts:2111-2131 emits each row as a normal call; calls without turnId become separate turns at src/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 produced apiCalls=2, turns=2, modelCalls=2; expected 1/1/1. Store rows need accounting-only/zero behavioral weight while retaining tokens and cost.

  2. A deferred changed-store read still reports hydration complete. The retryable catch at src/parser.ts:3008-3012 continues without recording incompleteness, and :3852-3872 can therefore let daily history advance. A warm-cache fixture changed the discovered store into an unreadable file: isSessionHydrationComplete() was true, expected false.

  3. The accepted absence epoch is not bounded once daily history seals. The committed test at tests/parser.test.ts:1113-1144 intentionally 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.

  4. Same-path DB reset/replacement can reuse durable dedup keys and lose new usage. The key at src/providers/copilot.ts:2080-2085 is only copilot-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 row id=1 left the old 100-input call in the durable union instead of the new 200-input call (src/parser.ts:2980-2993 rejects the reused key).

  5. 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.

@ozymandiashh

Copy link
Copy Markdown
Collaborator

Two additional exact-head blockers from the independent pass on ec4ed4f:

  • Partial store coverage drops the uncovered tail. Any served store row marks the whole session covered, and src/parser.ts:3073-3106 removes every shutdown rollup for that session. A fixture with DB request 1 plus a shutdown rollup for requests 1+2 served only request 1 while hydration remained complete. Coverage must be granular enough to preserve usage not represented by store rows, or the replacement contract must prove completeness.

  • A project learned after the store row was cached can split one session. The JSONL-derived project map is attached only during fresh discovery (src/providers/copilot.ts:2773-2784), while an unchanged store reuses its cached calls (src/parser.ts:2903-2912); project is part of the grouping key (:3141-3143). The regression fixture produced the same session under both testproj and actual-project. Cached store rows need project identity reconciliation/update when the session-state source appears.

The same audit also confirmed that accepting #916's independently-defined v18 through this PR strips its reasoningTokens and webSearchRequests fields while skipping re-derivation, strengthening the distinct-version sequencing requirement in the submitted review.

@ozymandiashh

Copy link
Copy Markdown
Collaborator

On the call-weight question: do not make only store rows unconditionally zero-weight.

  • A session.shutdown per-model rollup is always supplementary aggregate accounting, never one physical request. It should always carry zero call/turn/session/model-call/category weight while retaining its token/cost contribution.
  • A session-store row is one real request. It should carry behavioral weight only when that request has no served JSONL assistant/request representation; otherwise its token fields supplement the JSONL call and its behavioral weight is zero. Blanket weight 1 duplicates normal completed requests; blanket weight 0 hides crash/store-only requests.

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.

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.

3 participants