Repository navigation
fix(stats): keep reverted usage in session stats - #57928
lakshya-dhariwal wants to merge 20 commits into
Conversation
Added SQL usage tracking for session revert commits.
Prevent double counting of usage for reverted sessions.
Add tests for Session.revert functionality to ensure stats are correctly updated without double counting.
There was a problem hiding this comment.
This doesn't fix #57878 in a real run yet, and it adds new type errors.
How I checked: I ran the issue's reproduction (mock provider, opencode serve from source) on the base and on this branch. Both show 2 prompts · 2 steps · 2.2k tokens after the revert is committed, where 3.3k is expected. The local server's event bus doesn't save events (persist defaults to false; only workerd turns it on), so the event table stays empty and the new revert record is lost. The new test passes only because it builds the bus with persist: true. The issue's analysis warned about this (the reporter's event table was empty). The usage needs to be kept somewhere that always persists, for example a column or table written when the revert is committed, not a durable event.
I also forked a session and reverted it into the copied history: stats went from 2000 to 3000 input tokens and from $1.00 to $1.50, with no new model call (see inline). bun run typecheck in packages/core now fails; the base has no errors (see inline). The revert, stats and projector tests pass.
Public API: the durable session.usage.recorded event's source gains the value "revert"; the regenerated client types change to match. Producers aren't affected. Consumers that exhaustively match source ("title" | "compaction") will now see a new value.
|
|
||
| export const commit = Effect.fn("SessionRevert.commit")(function* (bus: Bus.Interface, session: SessionSchema.Info) { | ||
| if (!session.revert) return | ||
| const database = yield* Database.Service |
There was a problem hiding this comment.
commit now gets Database.Service from its environment, and that requirement leaks into its callers. bun run typecheck in packages/core fails with new errors in src/session.ts (475, 502), src/plugin/host.ts:570, src/config/plugin/command.ts:80, src/tool/plugin/subagent.ts:109 and test/session-shell.test.ts:198; the base has none. Passing the db in, or providing it where commit is called, would avoid widening the Session API types.
| FROM ${SessionMessageTable} | ||
| WHERE ${SessionMessageTable.session_id} = ${session.id} | ||
| AND ${SessionMessageTable.type} = 'assistant' | ||
| AND ${SessionMessageTable.seq} >= ${boundary.seq} |
There was a problem hiding this comment.
This adds up every assistant row from the boundary onward, including rows a fork copied from its parent. Stats leaves those out with session.fork_session_id IS NULL OR message.time_created >= session.time_created (#31032). I forked a session after two steps, then staged and committed a revert in the fork to the copied second prompt: stats went from 2000 to 3000 input tokens and from $1.00 to $1.50, with no new model call. The sum needs the same fork filter, and a test with a fork.
This only reaches stats if the bus persists durable events. The local CLI and server run with persist false (only packages/server/src/workerd.ts sets events.persist: true), so the event table stays empty and stats never see this record. With the issue's reproduction against opencode serve from this branch, stats still show 2.2k tokens after the commit instead of 3.3k. The test passes only because it builds the bus with Bus.configured({ persist: true }). Even with persistence on, the event's created time is the commit time, so reverted usage would land in the commit's day and range, not when the steps ran.
Added SessionRevertUsageTable to track reverted session messages and their usage statistics.
Removed 'revert' from the source literals in session event schema.
Removed 'revert' option from the source field in data.
Removed 'revert' option from the 'source' field in data.
There was a problem hiding this comment.
Both earlier points are fixed. With the issue's reproduction against opencode serve from this branch, stats now show 3 prompts · 3 steps · 3.3k tokens after the revert is committed. The reverted rows go into a table that the projector writes whether or not events are saved, they use the same fork filter as stats, and they keep their original times. bun run typecheck in packages/core is clean, and the revert, stats, projector and compaction tests pass, including the new fork test.
Public API: the event schema is no longer changed. The local database gains a session_revert_usage table through a new migration. The change is additive and backwards compatible; existing data is untouched.
One small thing: the branch is based on an older dev, and the 'restore upstream' commits copied newer files into it. GitHub's diff therefore shows unrelated-looking edits in projector.ts, revert.ts and session-event.ts that already match current dev. Rebasing on dev would remove them from the diff.
Merge upstream dev into patch-2
There was a problem hiding this comment.
Nothing new to raise. This push only merges dev, and the diff now shows only the stats change. The issue's reproduction against opencode serve from this branch still shows 3 prompts · 3 steps · 3.3k tokens after the revert is committed. The revert, stats, projector and compaction tests pass, and bun run typecheck in packages/core is clean.
Public API: unchanged. The local database gains a session_revert_usage table through a new migration; the change is additive and backwards compatible.
Issue for this PR
Closes #57878
Type of change
What does this PR do?
Committing a revert deletes the message rows from the boundary on, and
SessionStatssums usage from the rows that remain, so reverted work drops out ofopencode statseven though the session's stored counters still include it.On commit, the usage of the assistant rows about to be deleted is now recorded as a durable
UsageRecordedevent with a newsource: "revert", published beforeRevertEvent.Committed. Stats countsrevertevents the same way it already countscompactionones, so the totals keep the reverted work. The projector does not apply these events to the session counters: the steps already added their usage there when they ended, and applying it again would double count.Only token and cost totals are preserved; steps, prompts and per-model breakdowns still reflect the remaining messages, since
UsageRecordedcarries no step counts.How did you verify your code works?
Added a test in
session-revert.test.ts: run two steps with known usage, stage a revert to the second prompt, then prompt again to commit it. On currentdevstats lose the reverted step (1000 of 2000 input tokens); with the change stats keep the full 2000 tokens and $1.00 cost, and the stored session counters are unchanged. The test fails without the fix and passes with it. Thesession-stats,session-projectorandsession-compactionsuites all pass, and the client types were regenerated withbun run generate.Checklist
Added by the opencode bot (AI-generated).
Related: #46371, #48306
Public API: adds a
session_revert_usagetable to the local database through a new migration (additive, backwards compatible)