recalculate cost fixes - #5669
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBilling log retrieval separates list and pricing projections, hybrid stores hydrate missing pricing inputs, cost reconstruction preserves served metadata and modality details, and recalculation reports rows whose pricing inputs remain unavailable. ChangesBilling fidelity pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RecalculateCosts
participant LogStore
participant ObjectStore
participant PricingManager
RecalculateCosts->>LogStore: SearchLogsForBilling
RecalculateCosts->>LogStore: HydrateBillingChunk
LogStore->>ObjectStore: fetch missing pricing inputs
ObjectStore-->>LogStore: return hydrated payload
RecalculateCosts->>PricingManager: reconstruct and calculate cost
PricingManager-->>RecalculateCosts: return billing outcome
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/hybrid.go`:
- Around line 656-667: Preserve the inner store’s unpriceable rows in
HybridLogStore.SearchLogsForBilling by capturing the second return value from
h.inner.SearchLogsForBilling and initializing the local unpriceable slice from
it before hydration; retain the existing mutex and append behavior for rows
discovered locally.
In `@framework/logstore/migrations.go`:
- Around line 3849-3868: Update ensureBillingFidelityBackfill’s batch filter to
use pg_input_is_valid(token_usage, 'jsonb') before any token_usage::jsonb
expressions, while retaining the existing non-empty and object-shape conditions
and cached_write_tokens criteria. This must prevent malformed legacy JSON rows
from aborting the batch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f30bd0fa-d392-40aa-9060-d0f8ddf871a4
📒 Files selected for processing (16)
framework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
55f6b7c to
a49c418
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
transports/bifrost-http/handlers/logging.go (1)
2001-2017: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
omitemptymakesunpriceableinconsistent with the sibling counters.
Total,Processed,Updated, andSkippedare always emitted;Unpriceabledisappears when zero, so clients seeundefinedrather than0for the common case. Droppingomitemptykeeps the counter set uniform.♻️ Proposed change
- Unpriceable int `json:"unpriceable,omitempty"` + Unpriceable int `json:"unpriceable"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/logging.go` around lines 2001 - 2017, Update the Unpriceable field in the response struct to remove omitempty from its JSON tag, keeping unpriceable serialized as 0 alongside the other counter fields.plugins/logging/operations.go (1)
1506-1541: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRelease the chunk's payloads on the hydration-error path too.
return nil, fmt.Errorf("failed to hydrate pricing inputs: ...")leaves the partially hydrated chunk's payloads attached tobatch. Callers abandon the batch on error so GC reclaims it, but adefer-free early return makes the "at most one chunk resident" invariant depend on the caller rather than on this function.♻️ Optional tightening
unpriceable, err := p.store.HydrateBillingChunk(ctx, chunk) if err != nil { + logstore.ReleaseBillingPayloads(chunk) return nil, fmt.Errorf("failed to hydrate pricing inputs: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/operations.go` around lines 1506 - 1541, Update priceLogsInChunks so the chunk passed to HydrateBillingChunk is released with logstore.ReleaseBillingPayloads before returning on hydration error. Preserve the existing wrapped error and normal-path release, ensuring every hydrated chunk is released within this function.framework/logstore/hybrid.go (1)
714-757: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cache_debugwill trigger a fetch for nearly every offloaded row under the default config.The doc says consulting the exclusion set removes the "empty means offloaded or never written" ambiguity, but it only does so when the column is in the exclusion set. With
object_storage_exclude_fieldsunset (the default), a row with no semantic cache has an emptycache_debugand is therefore classified as needing hydration. In practicetoken_usageis offloaded in that same configuration so the fetch happens regardless, andbillingPayloadsHydratedprevents a repeat, so this is not a live regression — but the comment overstates the precision of the gate. Worth softening the wording so a future reader doesn't rely on a guarantee that only holds for excluded columns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/hybrid.go` around lines 714 - 757, Soften the explanatory comment in billingRowNeedsHydration and its missing helper to clarify that the exclusion set disambiguates empty values only for columns explicitly listed there; unlisted empty columns may still represent either absent data or offloaded data. Keep the existing hydration logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/logstore/hybrid.go`:
- Around line 714-757: Soften the explanatory comment in
billingRowNeedsHydration and its missing helper to clarify that the exclusion
set disambiguates empty values only for columns explicitly listed there;
unlisted empty columns may still represent either absent data or offloaded data.
Keep the existing hydration logic unchanged.
In `@plugins/logging/operations.go`:
- Around line 1506-1541: Update priceLogsInChunks so the chunk passed to
HydrateBillingChunk is released with logstore.ReleaseBillingPayloads before
returning on hydration error. Preserve the existing wrapped error and
normal-path release, ensuring every hydrated chunk is released within this
function.
In `@transports/bifrost-http/handlers/logging.go`:
- Around line 2001-2017: Update the Unpriceable field in the response struct to
remove omitempty from its JSON tag, keeping unpriceable serialized as 0
alongside the other counter fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 33fc80a9-532e-4a36-b328-a61f658078e6
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/logging/main.go
7d07f11 to
32db51f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
framework/logstore/billinghydrationgate_test.go (1)
24-32: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
getsis appended and read without synchronization.The hybrid store runs background upload workers, and the test goroutine reads
objStore.getswhile they are alive. No worker callsGettoday, so this doesn't race yet, but it is one hydration call from a goroutine away from a-racefailure — and the fix is three lines.🔒 Proposed fix
type countingObjectStore struct { *objectstore.InMemoryObjectStore - gets []string + mu sync.Mutex + gets []string } func (s *countingObjectStore) Get(ctx context.Context, key string) ([]byte, error) { - s.gets = append(s.gets, key) + s.mu.Lock() + s.gets = append(s.gets, key) + s.mu.Unlock() return s.InMemoryObjectStore.Get(ctx, key) } + +func (s *countingObjectStore) getKeys() []string { + s.mu.Lock() + defer s.mu.Unlock() + return slices.Clone(s.gets) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/billinghydrationgate_test.go` around lines 24 - 32, Protect countingObjectStore.gets with a mutex: add synchronization to the struct, lock around the append in Get, and use the same lock whenever the test reads gets. Keep the existing object-store delegation unchanged.framework/logstore/hybrid.go (1)
802-811: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
DeserializeFieldsruns twice per hydrated row.
MergePayloadFromJSONalready deserializes every merged field (and rebuildsContentSummary) beforepruneUnrequestedPayloadFieldsnarrows the set, then line 809 parses again. On an image-generation row that means the full base64 payload is unmarshalled intoImageDatabefore being discarded — the transient spike the chunk size exists to bound. Not a correctness problem, but pruning before the first parse would avoid it ifMergePayloadFromJSONgains a field-filtered variant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/hybrid.go` around lines 802 - 811, Avoid the duplicate deserialization in the hydration flow around MergePayloadFromJSON and DeserializeFields: add or use a field-filtered merge path that applies pruneUnrequestedPayloadFields before deserializing merged fields. Preserve ContentSummary and TokenUsageParsed hydration while ensuring unrequested fields such as ImageData are never unmarshalled before being discarded.framework/logstore/store.go (1)
43-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc refers to an
unpriceablereturn that no longer exists.The signature now returns
BillingHydrationResult; the prose still describes a bareunpriceableslice. Worth naming the field so implementers of this interface aren't looking for a second return value.📝 Proposed doc tweak
- // unpriceable carries the IDs whose inputs could not be recovered — a failed object - // fetch, or a content-hidden row whose payload is never fetched back by design. + // BillingHydrationResult.Unpriceable carries the IDs whose inputs could not be + // recovered — a failed object fetch, or a content-hidden row whose payload is + // never fetched back by design.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/store.go` around lines 43 - 55, Update the HydrateBillingChunk documentation to refer explicitly to the unpriceable field on BillingHydrationResult rather than implying a separate return value. Preserve the existing explanation of which IDs are included and how callers must handle them.plugins/logging/costrecalc_test.go (1)
410-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the recorded backfill stays empty.
backfilledis recorded but never checked, so the documented invariant at Line 57 ("should never be reached here") isn't enforced — a regression that backfills rows the store never hydrated would pass silently.♻️ Suggested addition
if total != rows { t.Fatalf("chunks covered %d rows, want every one of %d: %v", total, rows, store.hydrateChunkSizes) } + if len(store.backfilled) != 0 { + t.Fatalf("nothing was hydrated, so nothing may be written back; got backfill calls %v", store.backfilled) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/costrecalc_test.go` around lines 410 - 443, Update TestRunCostRecalcJob_HydratesInBoundedChunks to assert that the recorded backfilled collection remains empty after the job completes, failing the test if any rows were backfilled without hydration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/hybrid.go`:
- Around line 738-746: Update the hydration decision around vouched and
billingPayloadColumnFor so token-billed offloaded rows with denormalized usage
counters indicating nothing to price are treated as already resolved, including
rows with empty token_usage. Keep modality objects eligible for fetching by
applying this guard only when billingPayloadColumnFor(l.Object) is empty, and
preserve existing backfill behavior for rows whose counters indicate billable
usage.
In `@framework/logstore/hybridbilling_test.go`:
- Around line 123-126: Complete the BillingHydrationResult migration: in
framework/logstore/hybridbilling_test.go at lines 91-93, 123-126, and 149-153,
rename the result variable from unpriceable and assert against its Unpriceable
field; at lines 180-182 and 231-234, inspect the Unpriceable and Hydrated fields
respectively instead of asserting on the struct; update the
framework/logstore/store.go lines 43-55 documentation to reference
BillingHydrationResult.Unpriceable rather than a standalone unpriceable return
value.
In `@plugins/logging/operations.go`:
- Around line 1610-1621: Move the provably-zero direct-cache-hit handling ahead
of the IsUsageDegraded gate in the pricing flow, so degraded usage rows with a
known zero cost return successfully without being rejected. Preserve the
existing errPricingInputsUnavailable behavior for degraded rows that still
require pricing.
---
Nitpick comments:
In `@framework/logstore/billinghydrationgate_test.go`:
- Around line 24-32: Protect countingObjectStore.gets with a mutex: add
synchronization to the struct, lock around the append in Get, and use the same
lock whenever the test reads gets. Keep the existing object-store delegation
unchanged.
In `@framework/logstore/hybrid.go`:
- Around line 802-811: Avoid the duplicate deserialization in the hydration flow
around MergePayloadFromJSON and DeserializeFields: add or use a field-filtered
merge path that applies pruneUnrequestedPayloadFields before deserializing
merged fields. Preserve ContentSummary and TokenUsageParsed hydration while
ensuring unrequested fields such as ImageData are never unmarshalled before
being discarded.
In `@framework/logstore/store.go`:
- Around line 43-55: Update the HydrateBillingChunk documentation to refer
explicitly to the unpriceable field on BillingHydrationResult rather than
implying a separate return value. Preserve the existing explanation of which IDs
are included and how callers must handle them.
In `@plugins/logging/costrecalc_test.go`:
- Around line 410-443: Update TestRunCostRecalcJob_HydratesInBoundedChunks to
assert that the recorded backfilled collection remains empty after the job
completes, failing the test if any rows were backfilled without hydration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c9a07df-1600-479c-873e-e75d18e41976
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
There was a problem hiding this comment.
🧹 Nitpick comments (4)
framework/logstore/hybrid.go (1)
765-781: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive this from
billingPayloadColumnsrather than a parallel switch. The column set lives inrdb.go; adding a key there without touching this switch silently returns"", which makes the gate fetch that row on every pass forever. Amap[string]func(*Log) string(or a getter beside the existing map) keeps the two in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/hybrid.go` around lines 765 - 781, Update billingPayloadColumnValue to derive payload accessors from the existing billingPayloadColumns definition in rdb.go instead of maintaining a parallel switch. Add or reuse a getter map keyed by column name, including the current Log fields, and return the existing empty fallback only for unknown keys so newly added billing columns cannot silently remain unprocessed.plugins/logging/costrecalc_test.go (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the documented "never backfilled" invariant.
The comment states
BulkBackfillBillingPayloadsshould never be reached with this fake, andbackfilledrecords it, but no test checks it. Addingif len(store.backfilled) != 0 { ... }to the new test would pin that nothing is written back when nothing was hydrated.Also applies to: 429-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/costrecalc_test.go` around lines 57 - 62, Update the new test covering the fakeRecalcStore flow to assert that store.backfilled remains empty after execution. Use the existing backfilled tracking field and fail the test when its length is nonzero, preserving the documented invariant that BulkBackfillBillingPayloads is never reached.plugins/logging/operations.go (1)
1494-1568: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBackfill accumulated so far is dropped if a later chunk fails to hydrate.
priceLogsInChunksreturns early onHydrateBillingChunkerror, discarding thebackfillmap built from earlier chunks. Those fetches then get paid again on the next run. Flushing what you have before returning would keep the self-healing property.♻️ Optional: flush before propagating the error
hydration, err := p.store.HydrateBillingChunk(ctx, chunk) if err != nil { + p.flushBillingBackfill(ctx, backfill) return nil, fmt.Errorf("failed to hydrate pricing inputs: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/operations.go` around lines 1494 - 1568, Update priceLogsInChunks so a HydrateBillingChunk failure flushes any accumulated backfill entries through BulkBackfillBillingPayloads before returning the hydration error. Preserve the existing non-fatal backfill behavior and avoid attempting the write when backfill is empty.framework/logstore/billingprojection_test.go (1)
108-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubstring matching can misfire as the projection evolves.
strings.Contains(cols, unused)matches any column whose name embeds one of these tokens (e.g.responses_input_historyalso satisfies theinput_historyentry, and a futuremetadata_*column would trip themetadataentry). Consider reusing the tokenizedcontainsColumnhelper (plus anAS <name>check) so the assertion fails only on a genuine selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/billingprojection_test.go` around lines 108 - 126, Update TestBillingProjectionOmitsColumnsPricingNeverReads to use the token-aware containsColumn helper instead of strings.Contains, including its AS <name> validation, so each forbidden entry matches only an actual selected column rather than a substring of another column name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/logstore/billingprojection_test.go`:
- Around line 108-126: Update TestBillingProjectionOmitsColumnsPricingNeverReads
to use the token-aware containsColumn helper instead of strings.Contains,
including its AS <name> validation, so each forbidden entry matches only an
actual selected column rather than a substring of another column name.
In `@framework/logstore/hybrid.go`:
- Around line 765-781: Update billingPayloadColumnValue to derive payload
accessors from the existing billingPayloadColumns definition in rdb.go instead
of maintaining a parallel switch. Add or reuse a getter map keyed by column
name, including the current Log fields, and return the existing empty fallback
only for unknown keys so newly added billing columns cannot silently remain
unprocessed.
In `@plugins/logging/costrecalc_test.go`:
- Around line 57-62: Update the new test covering the fakeRecalcStore flow to
assert that store.backfilled remains empty after execution. Use the existing
backfilled tracking field and fail the test when its length is nonzero,
preserving the documented invariant that BulkBackfillBillingPayloads is never
reached.
In `@plugins/logging/operations.go`:
- Around line 1494-1568: Update priceLogsInChunks so a HydrateBillingChunk
failure flushes any accumulated backfill entries through
BulkBackfillBillingPayloads before returning the hydration error. Preserve the
existing non-fatal backfill behavior and avoid attempting the write when
backfill is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 890d0045-660b-4f99-bb98-6eed2e03f6b9
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
32db51f to
e27f1d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
framework/logstore/store.go (1)
43-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc refers to a return name that no longer exists.
HydrateBillingChunkreturnsBillingHydrationResult; the paragraph still talks about "unpriceable carries the IDs". Rename toresult.Unpriceableso implementers reading the contract aren't looking for a named result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/store.go` around lines 43 - 55, The HydrateBillingChunk contract documentation refers to a nonexistent named return. Update the unpriceable paragraph to identify the returned field as result.Unpriceable, preserving the existing description of which IDs it contains and how callers must handle them.framework/logstore/billinghydrationgate_test.go (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding
getswith a mutex.Today only the sequential
HydrateBillingChunkloop callsGet, so this is race-free. ButFindByID/SearchLogson the hybrid store also hydrate, and the list path can fan out — the first test that touches those under-racewill flag this append.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/billinghydrationgate_test.go` around lines 24 - 32, The countingObjectStore.Get method records keys in the shared gets slice without synchronization. Add a mutex to countingObjectStore and guard the gets append in Get, preserving the existing object-store retrieval behavior and ensuring concurrent hydration through FindByID or SearchLogs is race-safe.plugins/logging/costrecalc_test.go (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe stated invariant isn't asserted.
The comment says backfill "should never be reached here", but nothing checks it. Since
backfilledis already recorded, addingif len(store.backfilled) != 0to the chunk test would turn the comment into a guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/costrecalc_test.go` around lines 57 - 62, Add an assertion to the relevant chunk test that the fake store’s recorded backfill calls remain empty, using the backfilled field populated by fakeRecalcStore.BulkBackfillBillingPayloads. This should enforce the stated invariant that BulkBackfillBillingPayloads is never reached.plugins/logging/operations.go (1)
710-717: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRealtime tier capture is gated on usage being present.
applyServedTierToEntry(entry, result, bifrostUsage)sits insideif usage := result.ResponsesResponse.Usage; usage != nil, so a realtime turn that reports a served tier but no usage never recordsservice_tier/speed/inference_geo. The helper already tolerates a nil usage argument, so it can be hoisted out of the block.♻️ Suggested hoist
if usage := result.ResponsesResponse.Usage; usage != nil { bifrostUsage := usage.ToBifrostLLMUsage() entry.TokenUsageParsed = bifrostUsage entry.PromptTokens = bifrostUsage.PromptTokens entry.CompletionTokens = bifrostUsage.CompletionTokens entry.TotalTokens = bifrostUsage.TotalTokens - applyServedTierToEntry(entry, result, bifrostUsage) + applyServedTierToEntry(entry, result, bifrostUsage) + } else { + applyServedTierToEntry(entry, result, nil) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/operations.go` around lines 710 - 717, Move applyServedTierToEntry outside the usage != nil conditional so realtime served-tier metadata is captured even when ResponsesResponse.Usage is absent. Keep the existing usage-to-token-field assignments inside the conditional, and pass the available bifrostUsage value (nil when usage is unavailable) to the helper.framework/logstore/billingprojection_test.go (1)
108-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubstring matching here can false-fail on future column names.
strings.Contains(cols, "metadata")(and"speech_input", etc.) will also match any new column that embeds these names (e.g.metadata_hash,speech_input_ref), failing the test for a column pricing legitimately does not read. Reusing the token-levelcontainsColumn(plus anAS <name>check) would keep the intent without the substring coupling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/billingprojection_test.go` around lines 108 - 126, The test TestBillingProjectionOmitsColumnsPricingNeverReads should match complete selected column names rather than using strings.Contains, which can match unrelated names such as metadata_hash. Reuse the existing token-level containsColumn helper and include the AS <name> check when validating each unused column.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/hybrid.go`:
- Around line 802-811: Update the hydration flow around MergePayloadFromJSON and
pruneUnrequestedPayloadFields to avoid fully deserializing and summarizing
payload data before unrequested fields are removed. Prune or narrow the initial
merge so the billing path restores only the fields needed for billing, then run
log.DeserializeFields() once on the retained payload.
---
Nitpick comments:
In `@framework/logstore/billinghydrationgate_test.go`:
- Around line 24-32: The countingObjectStore.Get method records keys in the
shared gets slice without synchronization. Add a mutex to countingObjectStore
and guard the gets append in Get, preserving the existing object-store retrieval
behavior and ensuring concurrent hydration through FindByID or SearchLogs is
race-safe.
In `@framework/logstore/billingprojection_test.go`:
- Around line 108-126: The test
TestBillingProjectionOmitsColumnsPricingNeverReads should match complete
selected column names rather than using strings.Contains, which can match
unrelated names such as metadata_hash. Reuse the existing token-level
containsColumn helper and include the AS <name> check when validating each
unused column.
In `@framework/logstore/store.go`:
- Around line 43-55: The HydrateBillingChunk contract documentation refers to a
nonexistent named return. Update the unpriceable paragraph to identify the
returned field as result.Unpriceable, preserving the existing description of
which IDs it contains and how callers must handle them.
In `@plugins/logging/costrecalc_test.go`:
- Around line 57-62: Add an assertion to the relevant chunk test that the fake
store’s recorded backfill calls remain empty, using the backfilled field
populated by fakeRecalcStore.BulkBackfillBillingPayloads. This should enforce
the stated invariant that BulkBackfillBillingPayloads is never reached.
In `@plugins/logging/operations.go`:
- Around line 710-717: Move applyServedTierToEntry outside the usage != nil
conditional so realtime served-tier metadata is captured even when
ResponsesResponse.Usage is absent. Keep the existing usage-to-token-field
assignments inside the conditional, and pass the available bifrostUsage value
(nil when usage is unavailable) to the helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a6bb2d58-b5c2-4f50-b8d7-de8d4d22e900
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/logstore/hybridbilling_test.go
e27f1d6 to
ab0f06b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/logging/costrecalc_test.go (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
backfilledis recorded but never asserted, so the documented "never reached" invariant isn't pinned.Adding
if len(store.backfilled) != 0 { ... }toTestRunCostRecalcJob_HydratesInBoundedChunkswould make the comment enforceable — otherwise a regression that backfills rows nothing hydrated goes unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logging/costrecalc_test.go` around lines 57 - 62, Update TestRunCostRecalcJob_HydratesInBoundedChunks to assert that the fakeRecalcStore backfilled collection remains empty after the job runs. Use the existing backfilled field to enforce the documented invariant that BulkBackfillBillingPayloads is never invoked when nothing is hydrated.framework/logstore/rdb.go (1)
743-758: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSingle transaction wrapping N row updates.
Fine at current batch sizes, but the loop issues one
UPDATEper id inside one transaction, so the write lock footprint grows linearly with the backfill map. Consider chunking (or aCASE-based multi-row update) if batch sizes ever grow beyond the recalc constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/rdb.go` around lines 743 - 758, Update the transaction flow around the loop over updates so large backfill batches do not hold locks for all N individual updates at once. Chunk the updates using the existing recalc batch-size constant, or use an equivalent CASE-based multi-row update, while preserving both token_usage and cache_debug writes for every id and the current error propagation behavior.framework/logstore/billinghydrationgate_test.go (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
getsis appended without synchronization.Only the hydration path calls
Getin these tests, so today it is single-goroutine. If a future test exerciseshybrid.FindByID/SearchLogs(which hydrate) concurrently with the upload workers,-racewill flag this. Async.Mutexplus aLen()/snapshot accessor would make the fake safe by construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/billinghydrationgate_test.go` around lines 24 - 32, Make countingObjectStore.Get concurrency-safe by adding a sync.Mutex around accesses to gets, and provide Len or snapshot accessor methods for synchronized inspection. Preserve the existing delegation to InMemoryObjectStore.Get while ensuring concurrent hydration and upload-worker activity is race-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/hybrid.go`:
- Around line 818-822: Update the final empty-token_usage guard in the
hydration/pricing flow around log.TokenUsage so it only returns the “hydrated
payload has no token_usage” error for rows billed on tokens. Allow speech, OCR,
and video_generation modality rows to proceed when their modality-specific usage
payload is present, preserving the existing error behavior for token-priced
rows.
---
Nitpick comments:
In `@framework/logstore/billinghydrationgate_test.go`:
- Around line 24-32: Make countingObjectStore.Get concurrency-safe by adding a
sync.Mutex around accesses to gets, and provide Len or snapshot accessor methods
for synchronized inspection. Preserve the existing delegation to
InMemoryObjectStore.Get while ensuring concurrent hydration and upload-worker
activity is race-free.
In `@framework/logstore/rdb.go`:
- Around line 743-758: Update the transaction flow around the loop over updates
so large backfill batches do not hold locks for all N individual updates at
once. Chunk the updates using the existing recalc batch-size constant, or use an
equivalent CASE-based multi-row update, while preserving both token_usage and
cache_debug writes for every id and the current error propagation behavior.
In `@plugins/logging/costrecalc_test.go`:
- Around line 57-62: Update TestRunCostRecalcJob_HydratesInBoundedChunks to
assert that the fakeRecalcStore backfilled collection remains empty after the
job runs. Use the existing backfilled field to enforce the documented invariant
that BulkBackfillBillingPayloads is never invoked when nothing is hydrated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df9fc73d-5020-4b9c-9bb3-0968e3a08e46
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
89e564a to
4a10a92
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/payload.go`:
- Around line 124-126: Update the billing recomputation path so rows marked
ContentHidden && HasObject first attempt billing-only hydration from object
storage. Release the recovered payload after extracting billing inputs, and
classify the row as Unpriceable only when the object is unavailable or storage
is unconfigured; otherwise preserve normal pricing.
In `@plugins/logging/costrecalc.go`:
- Around line 17-26: The billing query currently materializes an entire 200-row
page, so payload-heavy modality data can exceed the intended memory bound before
chunk hydration. In plugins/logging/costrecalc.go lines 17-26, replace
costRecalcBatchSize with a payload-safe page size or split scalar paging from
per-chunk payload retrieval; apply the same bound to synchronous recalculation
in plugins/logging/operations.go lines 1342-1346. In
plugins/logging/operations.go lines 1494-1502, remove or revise any claim of a
flat memory bound unless DB-resident modality payloads are fetched per chunk.
In `@plugins/logging/operations.go`:
- Around line 410-415: Update the streamed OpenAI accumulation flow around
applyServedTierToEntry so the response envelope or accumulator carries the
served service_tier from the provider’s stream chunks. Pass that preserved tier
instead of nil when applying it to the usage entry, ensuring priority and flex
streams retain their metadata for repricing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cfdc3303-97f7-4df4-b5ea-f73b02c5d543
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/logstore/billinghydrationgate_test.go (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding
getswith a mutex.Today only the test goroutine triggers
Get, so this is safe — but it is one background hydration away from a-racefailure. Async.Mutexplus aKeys()-style accessor keeps the counter trustworthy if a future test exercises a concurrent read path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/billinghydrationgate_test.go` around lines 24 - 32, Make countingObjectStore.Get concurrency-safe by adding a sync.Mutex and protecting accesses to gets, then provide a Keys()-style accessor that returns a safe snapshot for assertions. Update tests to use the accessor rather than reading gets directly, while preserving the existing object-store delegation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/logstore/billinghydrationgate_test.go`:
- Around line 24-32: Make countingObjectStore.Get concurrency-safe by adding a
sync.Mutex and protecting accesses to gets, then provide a Keys()-style accessor
that returns a safe snapshot for assertions. Update tests to use the accessor
rather than reading gets directly, while preserving the existing object-store
delegation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3416523-5002-497f-b8ff-dcb96368ca9a
📒 Files selected for processing (17)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.gotransports/bifrost-http/handlers/logging.go
4a10a92 to
69305f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/streaming/accumulator.go (1)
49-60: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPooled
ChatStreamChunk.LogProbsis never reset — stale logprobs can leak across requests.
putChatStreamChunkresetsServiceTierhere but notLogProbs. Inchat.go,chunk.LogProbsis only assigned whenresult.ChatResponse.Choicesis non-empty with a delta choice; a usage-only/synthetic terminal final chunk (the exact scenario this PR's new tests exercise forServiceTier) never touchesLogProbs. SincegetChatStreamChunk()does no reset onGet(), a pooled chunk reused for such a chunk retains whateverLogProbsa previous, unrelated request left behind, and the unconditional merge loop inprocessAccumulatedChatStreamingChunks(if chunk.LogProbs != nil { mergedLogProbs.Content = append(...) }) will splice that stale data into the current request's logged/returned response.
putResponsesStreamChunksimilarly never resetsChunkIndex(same asputChatStreamChunk); worth a pass to confirm it can't be read stale before being overwritten (e.g. the error branch inresponses.gofeedschunk.ChunkIndexintoreserveTerminalChunkIndexbefore it's reassigned).🐛 Proposed fix
func (a *Accumulator) putChatStreamChunk(chunk *ChatStreamChunk) { chunk.Timestamp = time.Time{} chunk.Delta = nil chunk.Cost = nil chunk.SemanticCacheDebug = nil chunk.ErrorDetails = nil chunk.FinishReason = nil chunk.TokenUsage = nil chunk.ServiceTier = nil + chunk.LogProbs = nil chunk.RawResponse = nil a.chatStreamChunkPool.Put(chunk) }As per coding guidelines, "Pooled objects must have every field reset before returning to a pool."
Also applies to: 104-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/streaming/accumulator.go` around lines 49 - 60, Update putChatStreamChunk to reset LogProbs before returning the chunk to the pool, and update putResponsesStreamChunk to reset ChunkIndex as well. Verify the responses error path cannot consume stale ChunkIndex before reassignment, particularly where reserveTerminalChunkIndex uses it, preserving correct terminal indexing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/hybrid.go`:
- Around line 730-736: Remove the unconditional ContentHidden early return in
the hydration decision logic, allowing those rows to proceed through the
existing vouched/backfilled checks. Preserve fetching for legacy hidden rows
with blank pricing inputs while avoiding full-payload hydration for newly
written hidden rows whose TokenUsage and CacheDebug values are present; update
the affected test fixture to blank token_usage when it must represent a legacy
row.
---
Outside diff comments:
In `@framework/streaming/accumulator.go`:
- Around line 49-60: Update putChatStreamChunk to reset LogProbs before
returning the chunk to the pool, and update putResponsesStreamChunk to reset
ChunkIndex as well. Verify the responses error path cannot consume stale
ChunkIndex before reassignment, particularly where reserveTerminalChunkIndex
uses it, preserving correct terminal indexing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e79f0eb-d432-4891-963d-28dd49b217ca
📒 Files selected for processing (24)
framework/logstore/billinghydrationgate_test.goframework/logstore/billingprojection_test.goframework/logstore/contenthidden_test.goframework/logstore/hybrid.goframework/logstore/hybrid_test.goframework/logstore/hybridbilling_test.goframework/logstore/migrations.goframework/logstore/payload.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goframework/streaming/accumulator.goframework/streaming/accumulator_test.goframework/streaming/chat.goframework/streaming/responses.goframework/streaming/responses_test.goframework/streaming/types.goplugins/logging/costfidelity_test.goplugins/logging/costrecalc.goplugins/logging/costrecalc_test.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.gotransports/bifrost-http/handlers/logging.go
69305f7 to
78d8e96
Compare
Merge activity
|
…r to final chunk and log entry (#6236) ## Summary Anthropic reports `service_tier` on the `message_start` usage block during streaming. The per-event converter drops it, and `BifrostLLMUsage` has no `service_tier` field, so it had nowhere to travel. As a result, every streamed Anthropic request logged an empty `service_tier` and was repriced at standard rates instead of the actual served tier (priority/flex). This fix latches the tier across streaming events and stamps it onto the final chunk's response envelope, mirroring the existing pattern for `speed` and `inference_geo`. ## Changes - In the Anthropic chat completion and responses streaming loops, `service_tier` from `message_start` usage is now latched into a `servedServiceTier` variable and applied to the final chunk's response envelope, matching how `speed` and `inference_geo` are already handled. - `StreamAccumulatorResult` gains a `ServiceTier` field so the resolved tier survives the tracer boundary. Without this field, the tier was lost when the accumulator handed off to the tracer, causing streamed rows to reprice at standard rates. - `ProcessStreamingChunk` in the tracer now copies `ServiceTier` from the processed response into the accumulator result explicitly, since it lives on the response envelope rather than inside `BifrostLLMUsage`. - `convertToProcessedStreamResponse` in the logging plugin now forwards `ServiceTier` from `StreamAccumulatorResult` into the processed response so `applyStreamingOutputToEntry` can write it to the log entry. - Tests added to verify the final chunk carries the correct `service_tier` for both the chat completion and responses streaming paths, and that the tier survives the full accumulator-to-log-entry handoff. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/anthropic/... go test ./plugins/logging/... go test ./... ``` The new test `TestAnthropicChatStreamFinalChunkCarriesServedServiceTier` replays a synthetic Anthropic SSE stream where `service_tier: priority` appears on `message_start` and asserts the final chunk's `ServiceTier` equals `priority` alongside the existing `speed` and `inference_geo` assertions. `TestStreamingServiceTierSurvivesAccumulatorHandoff` verifies that a `StreamAccumulatorResult` carrying `priority` tier produces a log entry with `service_tier: priority` after the full conversion chain. ## Breaking changes - [ ] Yes - [x] No ## Related issues Related to the same class of mis-billing addressed in #5669 for non-streamed rows. ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…r to final chunk and log entry (#6236) ## Summary Anthropic reports `service_tier` on the `message_start` usage block during streaming. The per-event converter drops it, and `BifrostLLMUsage` has no `service_tier` field, so it had nowhere to travel. As a result, every streamed Anthropic request logged an empty `service_tier` and was repriced at standard rates instead of the actual served tier (priority/flex). This fix latches the tier across streaming events and stamps it onto the final chunk's response envelope, mirroring the existing pattern for `speed` and `inference_geo`. ## Changes - In the Anthropic chat completion and responses streaming loops, `service_tier` from `message_start` usage is now latched into a `servedServiceTier` variable and applied to the final chunk's response envelope, matching how `speed` and `inference_geo` are already handled. - `StreamAccumulatorResult` gains a `ServiceTier` field so the resolved tier survives the tracer boundary. Without this field, the tier was lost when the accumulator handed off to the tracer, causing streamed rows to reprice at standard rates. - `ProcessStreamingChunk` in the tracer now copies `ServiceTier` from the processed response into the accumulator result explicitly, since it lives on the response envelope rather than inside `BifrostLLMUsage`. - `convertToProcessedStreamResponse` in the logging plugin now forwards `ServiceTier` from `StreamAccumulatorResult` into the processed response so `applyStreamingOutputToEntry` can write it to the log entry. - Tests added to verify the final chunk carries the correct `service_tier` for both the chat completion and responses streaming paths, and that the tier survives the full accumulator-to-log-entry handoff. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/anthropic/... go test ./plugins/logging/... go test ./... ``` The new test `TestAnthropicChatStreamFinalChunkCarriesServedServiceTier` replays a synthetic Anthropic SSE stream where `service_tier: priority` appears on `message_start` and asserts the final chunk's `ServiceTier` equals `priority` alongside the existing `speed` and `inference_geo` assertions. `TestStreamingServiceTierSurvivesAccumulatorHandoff` verifies that a `StreamAccumulatorResult` carrying `priority` tier produces a log entry with `service_tier: priority` after the full conversion chain. ## Breaking changes - [ ] Yes - [x] No ## Related issues Related to the same class of mis-billing addressed in #5669 for non-streamed rows. ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
2 participants
Summary
Cost recomputation was systematically mis-pricing requests due to three compounding issues: (1) the
SearchLogsprojection used for billing omitted modality output payloads and, on hybrid object-storage-backed stores, returned rows whosetoken_usagewas blanked at write time — causingDeserializeFieldsto rebuild a lossy stub wherePromptTokensis inclusive of cache buckets, inflating cache-heavy requests by 2–4x; (2) theLogtable had no columns for the served billing tier (service_tier,speed,inference_geo), so every row repriced at standard rates regardless of what was actually served; and (3) several modality-specific pricing inputs (OCR pages, 1h cache-write split, server-side fallback model) were dropped during response reconstruction.A user observed approximately 3x overbilling in production on a cache-heavy Anthropic request.
Changes
New
SearchLogsForBillinginterface method onLogStorethat selects the full billing projection (including modality output payloads) and, onHybridLogStore, concurrently hydrates offloaded payloads from object storage. Rows whose payloads cannot be recovered (content-hidden, missing object) are returned as anunpriceableID list rather than silently handed back as stubs.billingSelectColumns/billingPayloadColumnsadded toRDBLogStore. The list projection (listSelectColumns) deliberately stays free of unbounded output blobs to avoid reintroducing the Cloud Run 32 MB body-limit failure. The billing projection extends it withspeech_output,transcription_output,image_generation_output,video_generation_output, andocr_output.IsUsageDegradedflag onLog.DeserializeFieldsmarks the stub it builds from denormalized columns as degraded;calculateCostForLogrefuses to price a degraded row and returnserrPricingInputsUnavailable. The flag clears when a realtoken_usagepayload is subsequently parsed (the hybrid hydration path callsDeserializeFieldstwice).New denormalized columns:
cached_write_tokens,service_tier,speed,inference_geoon theLogtable, added viamigrationAddBillingFidelityColumns.cached_write_tokensis backfilled from existingtoken_usageJSON viaensureBillingFidelityBackfill(deferred out of the migration to avoid blocking pod startup). The tier columns cannot be backfilled — pre-migration rows reprice at standard rates.applyServedTierToEntrycaptures the served tier at write time from both the response envelope (non-streaming) and the usage struct (streaming, where no envelope is accumulated). Called fromapplyNonStreamingOutputToEntry,applyStreamingOutputToEntry, andapplyRealtimeOutputToEntry.servedTierFromLog/buildResponseForRequestTyperestore the tier onto the reconstructedBifrostResponsesoCalculateCost'stierFromResponsesees the correct multipliers.OCR billing fix:
resp.OCRResponsenow hasPages,UsageInfo, andDocumentAnnotationpatched back in fromOCROutputParsed. Previously the reconstructed response reported zero pages and the entire request priced to nothing.1h cache-write split fix:
CachedWriteTokenDetailsis now forwarded throughbuildResponseForRequestTypefor the Responses path, preventing 1h cache writes from billing at the cheaper 5m rate.Server-side fallback model fix:
logEntry.ServerSideFallbackModelis now set onRoutingInfo.ServerSideFallbackModelsoresolvePricingprices at the model that actually ran.Alias-without-canonical fix:
ResolvedKeyAlias.ModelIDis now populated wheneverlogEntry.Aliasis set, independent of whether a canonical name exists, so per-deployment override pricing resolves correctly.Unpriceablecounter added toCostRecalcJobMeta,RecalculateCostResult, and the API status struct, distinguishing "nothing to charge" from "refused to write a number known to be wrong".costRecalcBatchSizereduced from 1000 to 200 to bound the object-storage fan-out (oneGetper offloaded row per batch).waitForOffloadtest helper waits for both the object upload and thehas_objectDB flag commit, fixing a race inTestHybrid_ContentHiddenStripsDBRowAndSkipsHydrationwhereprocessUploadwrites the flag after thePut.Type of change
Affected areas
How to test
go test ./framework/logstore/... ./plugins/logging/...Key test files:
framework/logstore/billingprojection_test.go— projection separation and end-to-end modality output deliveryframework/logstore/hybridbilling_test.go— hydration, content-hidden reporting, fetch-failure reporting, non-offloaded passthroughplugins/logging/costfidelity_test.go— cache breakdown parity, degraded-usage refusal, tier preservation, 1h cache-write pricing, server-side fallback model, unpriceable accountingBreaking changes
The
SearchLogsForBillingmethod is additive to theLogStoreinterface. ExistingfakeRecalcStoretest doubles require aSearchLogsForBillingimplementation (seecostrecalc_test.gofor the pattern: delegate toSearchLogsand return a nil unpriceable list).Security considerations
None. No auth, secrets, or PII surface changes. The new columns store provider-echoed tier strings (
"priority","flex","fast","us") that are already present in the response payloads.Checklist