fix(backup,ai): order backup jobs by started_at; count cached tokens as input - #3180
Open
ToddHebebrand wants to merge 3 commits into
Open
fix(backup,ai): order backup jobs by started_at; count cached tokens as input#3180ToddHebebrand wants to merge 3 commits into
ToddHebebrand wants to merge 3 commits into
Conversation
…as input
Two release-QA defects, both in "the number we show is derived from the
wrong field".
**A — backup dashboard picked the wrong "last job".**
`created_at` is a row-INSERT timestamp. The profile fan-out writes an
entire occurrence's jobs inside one transaction, so they share it to the
microsecond and `ORDER BY created_at DESC` is not even a total order. QA
hit that tie (two jobs at 06:32:11.829738), the planner returned the
OLDER run, and the device Backup tab hid its VSS Status panel — that
panel renders only when the chosen `lastJob` carries `vss_metadata`.
New `services/backupJobOrdering.ts` states the rule once, in two shapes:
- `latestBackupRunOrderBy` — `started_at DESC NULLS LAST, created_at
DESC, id DESC`, for "which run is most recent". NULLS LAST because a
`pending` job has not run: it has no snapshot, no VSS metadata and no
error log, so it must not displace a real run. It only demotes, so a
device whose every job is queued still reports one.
- `backupJobHistoryOrderBy` — `created_at DESC` primary, then the same
tiebreaks, for chronological feeds. Those filter on `created_at`
windows and a just-queued job has to stay at the top of the operator's
list; the tiebreaks only stop the fan-out reshuffling it per request.
Applied to `/backup/status/:deviceId` (the reported defect), the
attention-items `row_number()` window, the dashboard's latest-jobs feed
and the `/backup/jobs` history list.
The sweep also turned up the same defect mirrored in two AI tools:
`aiToolsBackup` used `desc(startedAt)`, and DESC in Postgres is NULLS
**FIRST**, so a single queued job floated above every real run and — under
`limit(1)` — made `get_backup_status` answer "latest backup: pending" for
a device that had just failed one.
**B — AI session input tokens excluded cached tokens.** Real bug, not
intent: no comment, column name or doc said otherwise, and the UI renders
the column as "{{input}} in". QA saw 8 turns report 17 input tokens
against 1029 output and $0.57 of spend, because prompt caching routes
almost the whole prompt through `cache_read_input_tokens` on every turn
after the first.
`sumInputTokens()` now feeds `ai_sessions.total_input_tokens`,
`ai_cost_usage.input_tokens`, the per-user `client_ai_usage` hook and the
client `done` event. The three components stay split in the COST path,
which was already cache-aware and correct — the sum is deliberately never
fed back into `calculateCostCents`, and a test pins that (a 1M cache-read
turn still prices at 30c, not 300c). Budget enforcement keys on
`total_cost_cents`, so it was never affected either way.
Historic rows are not backfilled: the split components were never
persisted, so there is nothing to recover them from.
Tests: 9 ordering-rule + 4 route cases for A (all four verified red
against the old code), 7 token-accounting cases for B.
Deploying breeze with
|
| Latest commit: |
f2706e9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5023f5d1.breeze-9te.pages.dev |
| Branch Preview URL: | https://fix-backup-lastjob-ordering.breeze-9te.pages.dev |
…cker mocks CI red on streamingSessionManager.clientLoop.test.ts: `recordExtraUsage` — "Number of calls: 0". Root cause is a test-double gap, not a product defect. That file mocks `./aiCostTracker` with an explicit factory listing only `recordUsageFromSdkResult`. Vitest does not return `undefined` for an export a factory omits — it THROWS on the access. So the new `sumInputTokens(usageData.usage)` call threw inside the `result` handler, the throw escaped to the processor's outer catch (visible in stderr as "[StreamingSessionManager] Query error: No 'sumInputTokens' export is defined on the './aiCostTracker' mock"), and everything after it was skipped — the per-user hook AND the `done` publish. Hence 0 calls rather than a wrong value. The assertion was NOT invalidated by the corrected accounting: that fixture carries no cache fields, so the correct answer is still inputTokens: 100. Assertions left exactly as they were. Fixed in all three streamingSessionManager suites that mock the module — droppedToolResult and deviceBoundAuth were green only because they never reach the result path, i.e. latent landmines on the same line. Also hardened `sumInputTokens` to accept a nullish usage object. It sits ahead of both the per-user hook and the `done` publish that returns the session to 'idle', so a throw there strands the turn and hangs the client — that line must not have a failure mode. This is the one real robustness point the failure exposed: the previous code did inline property reads, and hoisting a call there widened its blast radius. Ran the full requested surface this time (the miss that let this through): streamingSessionManager + aiCostTracker + routes/clientAi — 24 files, 248 tests, all passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two release-QA defects. Both are "the number we show is derived from the wrong field" — neither needs a migration.
Defect A — backup dashboard picked the wrong "last job"
created_atis a row-insert timestamp. The profile fan-out writes an entire occurrence's jobs inside one transaction, so they share it to the microsecond andORDER BY created_at DESCis not even a total order. QA hit exactly that tie (two jobs at06:32:11.829738), the planner returned the older run, and the device Backup tab hid its VSS Status panel — that panel renders only when the chosenlastJobcarriesvss_metadata.New
apps/api/src/services/backupJobOrdering.tsstates the rule once, in two shapes:latestBackupRunOrderBystarted_at DESC NULLS LAST, created_at DESC, id DESCbackupJobHistoryOrderBycreated_at DESC, started_at DESC NULLS LAST, id DESCOn NULLS LAST (the one judgement call, verified against the consumers before committing): a
pendingjob has not run — no snapshot, novss_metadata, noerror_log— so it must not displace a real run and blank the tab every time a backup is queued. It only demotes: a device whose every job is queued still reports one, andlastSuccess/lastFailurefilter to terminal statuses that always have astarted_at, so they are unaffected. The UI does not lose queued-job feedback either — "Run backup now" has its own banner and the job appears immediately in the jobs table.Why the feeds keep
created_atprimary:/backup/jobsfilters oncreated_atwindows (?from/?to/?date), and a just-queued job has to stay at the top of the operator's list rather than sink to the bottom. There the tiebreaks only stop the fan-out reshuffling the list between requests.Sweep
dashboard.ts/status/:deviceId— the reported defectcreated_at DESCdashboard.tsattention-itemsrow_number()windowcreated_at DESCdashboard.tslatest-jobs feed (limit 5)created_at DESCjobs.ts/backup/jobslistcreated_at DESCaiToolsBackup.tsget_backup_statusdesc(startedAt)aiToolsBackup.tslist_jobsdesc(startedAt)The last two are the same defect mirrored:
desc()in Postgres means NULLS FIRST, so a single queued job floated above every real run and — underlimit(1)— madeget_backup_statusanswer "latest backup: pending" for a device that had just failed one.Left alone deliberately:
readinessCalculatorandverificationServiceorder bycompleted_atand pre-filter to restorable statuses, sostarted_atis not the key they want.Defect B — AI session input tokens: real bug, not intent
Investigated before touching it. It is a real bug. Nothing in the code, columns or docs claims the counters track only uncached input:
total_input_tokens, notuncached_input_tokens, and the UI renders them as"{{input}} in / {{output}} out".streamingSessionManageralready accumulates all four SDK fields correctly and forwards them;aiCostTrackerthen destructured the cache fields, used them only for pricing, and added justinput_tokensto the column.aiCostTracker.test.ts— 1M cache-read → 30c) but never checked what landed in the token column. That turn incrementedtotal_input_tokensby 0.That exactly explains the QA observation: 8 turns, 17 input tokens, 1029 output, $0.57. Prompt caching routes almost the whole prompt through
cache_read_input_tokenson every turn after the first.Fix
sumInputTokens()(uncached + cache-read + cache-creation — three disjoint slices of one prompt, so summing cannot double-count) now feeds:ai_sessions.total_input_tokensai_cost_usage.input_tokens(daily/monthly org aggregates)client_ai_usagehook and the client-facingdoneeventCost is untouched and was correct throughout. The three components stay split in the pricing path — the sum is deliberately never fed back into
calculateCostCents, and a test pins that (a 1M cache-read turn still prices at 30c, not 300c). Budget enforcement keys ontotal_cost_cents, so it was never affected in either direction.Not in scope: no backfill. Historic rows carry the uncached slice only and read implausibly low; the split components were never persisted anywhere, so there is nothing to recover them from. Recorded in a comment on the schema column. Adding
cache_read/cache_creationcolumns to keep the breakdown would need a migration plus export-policy registration and is a bigger call than a release fix — happy to file it.Verification
tsc --noEmit -p apps/api/tsconfig.json→ exit 0eslinton all 11 changed files → clean"backup_jobs"."started_at" desc nulls last, "backup_jobs"."created_at" desc, "backup_jobs"."id" desc, and the window renders asrow_number() over (partition by "backup_jobs"."device_id" order by ...)src/routes/backup(20 files)src/services/backupJobOrdering.test.tssrc/services/aiCostTracker.test.ts+streamingSessionManager.usage.test.tssrc/routes/clientAi+clientAiUsage(19 files)All four new
/status/:deviceIdroute cases were verified red against the pre-fix implementation, not just green after.Not merged.
🤖 Generated with Claude Code