Repository navigation
fix(opencode-usage): merge migrated session usage per column, not per row - #22550
Conversation
A session that lived through OpenCode 2's V1 import has a row in both `session` and `session_v2`, and neither is complete. #22391 resolved the pair by ranking whole rows on one number — total token count, ties to `session_v2` — which let that number decide everything else on the row. Three consequences, each reproduced against that PR's own fixtures: - A recorded cost could be zeroed. `session_v2` wins on tokens while carrying `cost = 0`, and row parsing maps a zero cost to `null`, so a legacy row's $12.50 disappeared. Cost is re-derived by the same lossy reduce as the tokens, but only the tokens were guarded. - The token comparison decided metadata. A legacy row with more tokens supplied a stale pre-migration directory, and a legacy row with a NULL model erased the model `session_v2` had — 23 of 234 shared ids on a real migrated database have a model only on the v2 side. - Winner-takes-all is per row, so a legacy row holding the input tokens and a v2 row holding the cache reads reported one of them as zero. Metadata now comes from the generation OpenCode still writes, with older generations filling only its NULLs; usage columns take a per-column MAX. Both rows aggregate the same assistant messages, and the import can only drop messages, never invent them, so each column's MAX is a tighter lower bound on the truth than either row and can never exceed it. The relation stays exactly one row per id — the highest-priority generation holding it — every column stays `columnExists`-guarded with a SQL fallback, and a database with a single session table builds the same SQL it did before. Cache schema version 4 -> 5 so existing caches rescan.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSession queries now merge metadata and usage columns from rows across OpenCode generations. They select session ids by source priority and emit one row per session id. SQLite fixtures can represent a NULL model. The usage schema version increases from 4 to 5. Tests cover migrated sessions, v2-only sessions, and v1-only databases. Merge Risk: ⚪ Minimal · up to The reported query failure does not occur with supported OpenCode databases. No actionable merge-blocking issue remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the migration-merge rewrite in OpenCode usage scanning, verified against the full diff plus the affected test suite.
- Metadata from the live generation — the
session_v2select nowLEFT JOINssessionand readsCOALESCE(t.col, o1.col)forproject_id,directory,title,model, and the timestamps, so a frozen pre-migration twin no longer supplies identity. - Usage merged per column —
MAX(COALESCE(t.col,0), COALESCE(o1.col,0))forcostand all five token columns, replacing the row-level token tiebreak that could zero a recorded cost or erase a token column. - Row ownership by existence — each select claims only ids no higher-priority table holds (
NOT EXISTS), so the union stays one row per id. - Cache invalidation —
OPENCODE_USAGE_SCHEMA_VERSION4 → 5, required because usage caches key on file mtime/size which the query change does not touch. - Tests/fixture — 7 new tests (4 that fail pre-change, 3 invariance) plus a v2-only case; fixture gains
model?: string | null.
I read the generated SQL through every schema shape (v1-only, v2-only, migrated, migrated-without-token-columns). The existence-based exclusions are exclusive and exhaustive; MAX(...) is only emitted with two or more arguments so SQLite cannot resolve it as the aggregate; and every outer-joined column is COALESCE-guarded, so an id the lower generation never held contributes 0. The per-column lower-bound argument holds — each MAX(c_L, c_V) ≤ truth_c, and the summed total stays ≤ the true sum even when columns come from different rows.
One latent consequence, already noted in the PR body: metadata prefers the live row even when the live directory is '' (the shape the codebase uses for "no directory") while the legacy twin holds a real path, because COALESCE only fills NULLs. Since the import carries directory verbatim and it does not change post-migration, this is not reachable in practice and is not actionable.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…tablyai#22550) A session that lived through OpenCode 2's V1 import has a row in both `session` and `session_v2`, and neither is complete. stablyai#22391 resolved the pair by ranking whole rows on one number — total token count, ties to `session_v2` — which let that number decide everything else on the row. Three consequences, each reproduced against that PR's own fixtures: - A recorded cost could be zeroed. `session_v2` wins on tokens while carrying `cost = 0`, and row parsing maps a zero cost to `null`, so a legacy row's $12.50 disappeared. Cost is re-derived by the same lossy reduce as the tokens, but only the tokens were guarded. - The token comparison decided metadata. A legacy row with more tokens supplied a stale pre-migration directory, and a legacy row with a NULL model erased the model `session_v2` had — 23 of 234 shared ids on a real migrated database have a model only on the v2 side. - Winner-takes-all is per row, so a legacy row holding the input tokens and a v2 row holding the cache reads reported one of them as zero. Metadata now comes from the generation OpenCode still writes, with older generations filling only its NULLs; usage columns take a per-column MAX. Both rows aggregate the same assistant messages, and the import can only drop messages, never invent them, so each column's MAX is a tighter lower bound on the truth than either row and can never exceed it. The relation stays exactly one row per id — the highest-priority generation holding it — every column stays `columnExists`-guarded with a SQL fallback, and a database with a single session table builds the same SQL it did before. Cache schema version 4 -> 5 so existing caches rescan. (cherry picked from commit d438676)
ELI5
Orca counts OpenCode token usage by reading OpenCode's database. After an upgrade, each session exists twice — an old frozen copy and a live one — and neither is guaranteed complete. My previous fix picked whichever copy had more tokens and took everything from it, including the session's folder, model and recorded cost. That could report a stale folder, or silently zero a real dollar amount. Now each field is taken from wherever it is actually correct.
What Changed
The single row-level comparison from #22391 is split into two rules, both inside the same
UNION ALLrelation:session_v2select nowLEFT JOINssessionand reads metadata asCOALESCE(t.col, o1.col)forproject_id,directory,title,model,time_created,time_updated. The frozen legacy twin only fills NULLs.MAX(COALESCE(t.col, 0), COALESCE(o1.col, 0))forcostand all five token columns.Row ownership stays trivially exclusive and exhaustive — each select claims the ids the higher-priority generations don't hold via
NOT EXISTS— so there is exactly one row per id. Verified on the real database: 254 rows / 254 distinct ids.OPENCODE_USAGE_SCHEMA_VERSION4 → 5 so existing caches rescan.Why
Adversarial review of #22391 reproduced three wrong answers. The rationale there was right — the V1→V2 import genuinely recomputes, so a
session_v2row can legitimately hold less than its frozen twin — but one token comparison was made to decide far more than tokens.{tokensInput: 100, cost: 12.5}vs v2{tokensInput: 200, cost: 0}→estimatedCostUsd: null, becauseopencode-usage-row-parsing.ts:104maps cost 0 to null. $12.50 vanished. Cost is recomputed by the same lossy reduce as the tokens, but the tiebreak guarded only tokens.{tokensInput: 900, directory: '/old/pre-migration-path'}vs v2{tokensInput: 120, directory: '/new/current-path'}→ attributed to the stale path.{input: 1000, cacheRead: 0}vs v2{input: 0, cacheRead: 1200}→ reported input 0.Why per-column MAX cannot over-report. Let
Lbe the frozen legacy total,Vthe v2 total,Tthe truth.V= decodable pre-migration assistant messages + post-migration ones;L= all pre-migration assistant messages. Every term is non-negative and the import only ever drops messages, soL ≤ TandV ≤ Tper column, henceMAX(L, V) ≤ T. It is a strictly tighter lower bound than either row alone. Picking a whole row is the same kind of bound, just weaker.Why cost tracks MAX rather than the token winner. Cost comes from the same
assistants.reduce(v1-migration.bun.ts:461-466), is non-negative, and obeys the identical bound. Tying it to the token winner just recreates defect (a) mirrored — a cost comparison would then zero real tokens. The accepted tradeoff, noted in the code: cost and tokens may come from different rows, so implied $/token is not a row-consistent ratio. Neither row is correct anyway, and the alternative demonstrably zeroes $12.50.No SUM — these are two views of one session, not two sessions.
Source evidence (
v2.0.15):migration/20260804233008_loose_psylocke.tscopies nothing — on the V1 lineage it creates an emptysession_v2and never touchessession/message/part. Row population lives in a separate component,v1-migration.bun.ts. (On the pre-split V2 lineage it instead renamessession→session_v2, which is the "v2-only" shape.)v1-migration.bun.ts:593-604—directory,title,project_id,time_created,time_updatedare carried over verbatim.DROP TABLE sessionin either file; completion is recorded only as kvmigration.v1-v2 = {"phase":"completed"}(:215,670). Confirmed on the real database: that kv row coexists with 234 survivingsessionrows, so both tables coexisting is the settled steady state, not a mid-migration artifact.:461-466+:226-233— all six usage columns are recomputed by one reduce overassistants, after messages failingdecodeMessageare dropped. Cost is lossy exactly as the tokens are.:449-455—model: input.session.model ?? (derived from the last user message), so the v2 model is a superset: never NULL when legacy is non-NULL, sometimes non-NULL when legacy is NULL. That is the mechanism behind the 23-of-234 figure below.:637-640— the post-copyUPDATEtouches only the transformed session andtime_updated, so later metadata divergence is OpenCode 2 continuing to writesession_v2whilesessionstays frozen.Linked Issue
Refs #15841, and fixes three defects in my own merged #22391.
Visual Proof
N/A— and the measured reason is the interesting part.On the real local database the change is a zero delta, so a before/after screenshot of the usage panel would show two identical images and imply, falsely, that nothing was fixed.
That is the correct result. This database's 234 shared ids all round-trip losslessly (a lossless recompute over integers is bit-identical, so equality here does not imply a copy — five ids differ on
costby exactly 1, 1, 1, 2, 1 ULP, which only re-summation produces):directory,title,project_id,time_created,time_updatedare byte-identical on every one, all five token columns are equal on every one, andSUM(v2.cost − legacy.cost)across all 234 is1.75e-14— float noise. Because every token total ties, #22391's "ties go tosession_v2" already picked the v2 row everywhere, including the 23 ids where only v2 carries a model.So on this machine the three defects are latent, not active. The 23-of-234 is the blast radius — what gets lost the moment any legacy row wins on tokens — not a current mis-attribution. The fixtures are what reproduce the defects, which is why the proof below is tests rather than pixels.
The database copy used for measurement was taken first; the original was never opened for write.
Testing
Platforms: macOS; source verification against
v2.0.15.Reverted only
opencode-usage-row-queries.ts, keeping the tests and fixture:Restored: 6 files / 50 tests pass.
Three of the seven new tests cannot fail before the change, deliberately. I had asked for every new test to fail first; that was wrong for the invariance cases — "resolves a faithful migrated copy exactly as the v2 row alone", "leaves an OpenCode 1-only database untouched", and "emits exactly one row per session id" exist precisely to assert that behaviour is identical before and after. A version of those that failed on the old code would be asserting the wrong thing. The four defect tests are the ones that must fail, and do.
pnpm tc→ 0.pnpm run check:code-quality:changed→ 0 new findings.oxfmt --checkclean.AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
session_v2, take the unchanged path, and are covered by an invariance test. Every column stayscolumnExists-guarded with a SQL fallback.project_idis the one metadata column where v2 is not unambiguously better. The import reassignssession_v2.project_idtoProject.ID.globalwhen the legacy value is dangling (v1-migration.bun.ts:586-591), so preferring v2 points theprojectjoin at the global project rather than at nothing. It only matters whendirectoryis empty — anddirectoryisNOT NULLin both schemas whileextractCwdprefers it — so the practical impact is nil. Flagging it rather than hiding it; "use what OpenCode itself uses" seemed the right call.session_v2.costis 0 with no legacy twin to recover from — upstream not populating the column. Nothing this change can fix.LEFT JOINon a primary key plus per-columnMAX/COALESCE; still one pass per database on the existing worker thread.wslGated*helpers.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm testpass locally