feat(usage): preserve usage history past the management read window with a daily rollup sidecar - #1008
feat(usage): preserve usage history past the management read window with a daily rollup sidecar#1008lidge-jun wants to merge 4 commits into
Conversation
…ade docs (4-round audited)
…ildable derived cache (010)
…read window (020)
📝 WalkthroughWalkthroughThis PR adds a rebuildable usage-rollup sidecar ( ChangesUsage Rollup Preservation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Route as logs-usage-routes
participant Rollup as rollup.ts
participant Log as usage/log.ts
participant Summary as summary.ts
Route->>Rollup: ensureRollupCurrent()
Rollup->>Log: read eligible aged prefix
Rollup->>Rollup: foldUsagePrefix() append + fsync commit row
Rollup->>Rollup: write usage-rollup-meta.json atomically
Route->>Rollup: readRollupSnapshot()
Rollup-->>Route: RollupSnapshot (cutline, day/model/provider/key rows)
Route->>Log: readUsageSnapshotForManagement(fromOffset=cutline)
Log-->>Route: raw tail entries
Route->>Summary: summarizeUsage(entries, rollup=RollupContribution)
Summary-->>Route: merged usage summary
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0d5417d8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fromOffset = parsed.snapshot.cutlineOffset; | ||
| const toOffset = eligibleCutline(rawFd, size, fromOffset, now); | ||
| if (toOffset > fromOffset) { | ||
| const entries = await parseUsageRange(rawFd, fromOffset, toOffset); |
There was a problem hiding this comment.
When rebuilding from a large existing usage.jsonl, foldUsagePrefix first retains every normalized entry in the eligible prefix and only then calls accumulateEntries; a 157 MB log can therefore expand to several times that size in heap, and the append-only log makes the allocation unbounded for older installations. Because this derived sidecar is started from the management request path, an out-of-memory failure can terminate the proxy rather than fall back to the raw tail; parse and aggregate incrementally instead of materializing the prefix.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| for (let position = fromOffset; position < size;) { | ||
| const length = Math.min(READ_CHUNK_BYTES, size - position); | ||
| const chunk = readExactly(fd, length, position); |
There was a problem hiding this comment.
On the first GET /api/usage for a large existing log, the fire-and-forget call still executes synchronously until its first await, and eligibleCutline performs synchronous reads, JSON parsing, and date conversion over the entire old prefix before that point. Thus the advertised background fold blocks Bun's event loop and delays this and unrelated management requests; make the cutline scan cooperative or defer it before doing synchronous work.
Useful? React with 👍 / 👎.
| if (meta && currentLineage && meta.version === 1 && meta.lineageKey === currentLineage | ||
| && meta.priceFingerprint === fingerprint | ||
| && Date.now() - meta.lastFoldAttemptAt < ROLLUP_ATTEMPT_THROTTLE_MS) return; |
There was a problem hiding this comment.
If usage.jsonl is truncated or rewritten in place within ten minutes of a fold, its inode/birthtime and the metadata fingerprint still match, so this early return leaves readRollupSnapshot serving the stale cutline. A truncation below that offset makes /api/usage return read_failed until the throttle expires, while a same-sized rewrite merges stale aggregates; validate the committed size/boundary before honoring the throttle so the optional sidecar can immediately fall back to the raw-tail path.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| try { parsed = JSON.parse(line); } catch { return lineStart; } | ||
| if (!isObject(parsed) || !usableTimestamp(parsed.timestamp) | ||
| || localDateKey(parsed.timestamp) >= cutoff) return lineStart; |
There was a problem hiding this comment.
If an append-only log contains one complete malformed or timestamp-less JSONL row, eligibleCutline permanently returns that row's offset on every later fold. Although the existing raw reader deliberately skips malformed complete rows and continues, the sidecar can never fold any subsequent old history, so once the suffix exceeds the management byte window the preservation feature regresses to truncation; skip complete invalid rows while advancing and reserve the stop behavior for an incomplete trailing row or a valid young entry.
Useful? React with 👍 / 👎.
| const folded = readRollupSnapshot(); | ||
| const cutlineOffset = folded?.cutlineOffset ?? 0; |
There was a problem hiding this comment.
When an operator disables usageRollupEnabled after a sidecar has already been created, GET /api/keys still unconditionally loads that sidecar and reads the raw log from its cutline. This contradicts the flag's documented raw-tail-only behavior and means disabling a suspect rollup does not disable it for per-key totals; pass the flag through the API-key caller and use folded = null with offset zero when it is false.
Useful? React with 👍 / 👎.
| for (const entry of entries) { | ||
| if (!usableTimestamp(entry.timestamp)) continue; | ||
| oldestTimestampMs = oldestTimestampMs === null ? entry.timestamp : Math.min(oldestTimestampMs, entry.timestamp); | ||
| const date = localDateKey(entry.timestamp); |
There was a problem hiding this comment.
If the service folds history in one timezone and the host timezone or TZ later changes, the lineage and price fingerprint remain valid, so these persisted date keys continue using the old timezone while tail rows and range boundaries use the new one. Requests near midnight are then assigned to inconsistent days, and a 30-day query can include or exclude the wrong rolled-up boundary day; include the timezone in rollup validity or store timezone-neutral data that can be rebucketed when read.
Useful? React with 👍 / 👎.
| providers: buildProviders(filteredEntries, totals.totalTokens), | ||
| days: buildDayGrid( | ||
| range, since, now, filteredEntries, rollupDays, rollupModels, | ||
| range === "all" ? rollup?.oldestTimestampMs ?? null : null, |
There was a problem hiding this comment.
For range=all with a surface filter, this passes the rollup's global oldest timestamp even though rollupDays has already been filtered by surface. If older folded Claude traffic predates all Grok traffic, for example, the Grok day grid gains leading zero days back to the Claude timestamp, unlike the pre-rollup behavior based on filtered entries; retain an oldest timestamp per surface or derive it from the filtered rollup contribution.
Useful? React with 👍 / 👎.
| | `GET /api/debug/injection-logs` | Read bounded guidance-injection debug entries | — | | ||
| | `GET /api/claude/inbound-debug` | Read Claude inbound debug state and entries | — | | ||
| | `GET /api/usage` | Summarize usage by range and client surface | Returns an `error: "read_failed"` summary if storage cannot be read | | ||
| | `GET /api/usage` | Summarize usage by range and client surface. History older than the bounded read window is served from the daily rollup sidecar (`usage-rollup.jsonl`), so `all`-range summaries keep the full history; `7d` stays raw-exact, and `30d` includes rolled-up days at day granularity. Deleting the rollup files is safe — they rebuild in the background. | Returns an `error: "read_failed"` summary if storage cannot be read | |
There was a problem hiding this comment.
For a high-volume installation whose last nine days alone exceed managementUsageMaxReadBytes or the 200,000-entry cap, the rollup cannot cover the omitted recent prefix: 7d loses rows and even all reports historyTruncated rather than retaining full history. Qualify the all and 7d claims on the raw tail fitting those bounds so the public API documentation matches the implemented truncation metadata.
AGENTS.md reference: docs-site/AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 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 `@devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md`:
- Line 106: Add a language tag such as text to the opening pseudo-code fence at
the affected documentation section, keeping the fenced content unchanged so
markdownlint no longer reports MD040.
- Around line 162-167: The readRollupSnapshot() function in src/usage/rollup.ts
currently does not validate whether the live raw file is large enough to support
the committed cutlineOffset. Add a synchronous check within the validity gate
(around lines 424-432) that compares the currentRawSize against cutlineOffset
and returns null when the file is shorter than the cutline, preventing invalid
offsets from reaching downstream functions like
readUsageSnapshotForManagement(). Then extend the test file
tests/usage-rollup.test.ts (around lines 260-270) to call readRollupSnapshot()
after truncating the raw file below the cutline and verify it returns null
instead of proceeding with an invalid snapshot.
In `@docs-site/src/content/docs/reference/management-api.md`:
- Line 127: Update the GET /api/usage documentation row to qualify rollup
behavior: all-range summaries retain full history only when the rollup is valid
and caught up; 7d is raw-exact only when truncatedPrefixBytes is 0; and 30d may
include up to approximately 24 hours from the rolled-up boundary day. State that
disabled, invalid, rebuilding, or gapped rollups serve raw-tail-only data, and
deleting usage-rollup.jsonl can temporarily restore truncation until background
rebuilding completes.
In `@src/config.ts`:
- Line 951: Update the usageRollupEnabled schema definition to add a true
fallback for invalid values, while retaining the existing default for missing
values. Follow the established .catch(...) pattern used by nearby configuration
fields so hand-edited non-boolean values remain enabled and do not enter
loadConfig’s backup-and-defaults reset path.
In `@src/server/management/api-key-usage.ts`:
- Around line 160-173: Update the API-key usage aggregation around
readRollupSnapshot and readUsageSnapshotForManagement to accept and honor the
configured usageRollupEnabled flag: when disabled, use cutlineOffset 0 and pass
no folded keys or attributionSinceMs; when enabled, preserve the existing rollup
behavior. Include the flag in rollupCache.revisionKey or invalidate rollupCache
when it changes, and add a regression test covering a created rollup snapshot
followed by disabling the feature through the shared routing/configuration
layers.
In `@src/usage/rollup.ts`:
- Around line 171-178: Make the shared stableStringify implementation injective
by encoding undefined with an explicit sentinel before handling arrays and
objects, then update both src/usage/rollup.ts:171-178 and
scripts/generate-jawcode-metadata.ts:59-66 to reuse that exported helper rather
than maintaining duplicate copies. Preserve all existing canonicalization
behavior for other values; the resulting fingerprint change should trigger one
rollup rebuild. Bump ROLLUP_COST_SEMANTICS_VERSION in src/usage/cost.ts only if
making the rebuild explicit in metadata.
- Around line 479-508: The `parseUsageRange` function accumulates all entries
into the `entries` array with no cap, causing memory spikes on large log files.
Cap the `toOffset` parameter in `foldUsagePrefix` (the caller of
`parseUsageRange`) to limit the byte range processed per invocation, such as
`fromOffset + N` bytes or to a single day boundary. This allows the throttled
`ensureRollupCurrent` process to walk through the file in smaller segments,
which the existing segment chain in `parseRollup` already supports, without
requiring format changes.
- Line 613: Instead of building a composite key string at line 613 by joining
date, admissionKind, and apiKeyId with null-byte delimiters and then parsing it
back at line 650, store these identity fields directly on the accumulator value
object itself so they can be retrieved without re-deriving them from the key.
Apply this same pattern change to the model rows at lines 575/628 and the
provider rows at lines 586/640. Verify that mergeGroupRow at line 415 continues
to work correctly since it reconstructs keys from the row fields rather than
from the composite key string.
- Around line 807-809: Record rollup fold failures without changing the existing
raw-tail-only fallback: add module-level rollup statistics alongside the other
state, increment the failure counter in the catch around foldUsagePrefix, and
store only a safe error summary without raw usage-row content or credentials.
Keep the current retry and degradation behavior unchanged.
- Around line 310-320: Update parseRollup/readRollupSnapshot to cache the parsed
snapshot and avoid rereading and splitting the full rollup on every /api/usage
request. Add bounded growth by compacting the rollup atomically when its file
size or segment count crosses a threshold, ensuring foldUsagePrefix continues
appending safely and cached snapshots are invalidated or refreshed after
compaction.
- Around line 461-466: Update eligibleCutline to advance over complete
newline-terminated rows when JSON parsing fails or the parsed timestamp is
unusable, matching parseUsageRange and accumulateEntries behavior; stop only
when a valid timestamp is recent. Preserve lineStart for incomplete trailing
rows, and add regression tests covering malformed JSON and unusable timestamps.
- Around line 424-436: Update readRollupSnapshot() to parse the snapshot, obtain
currentUsageLogRevision().size, and return null when the revision is unavailable
or snapshot.cutlineOffset exceeds the current raw log size. Preserve existing
metadata validation and return the parsed snapshot only when the offset is
valid, then add a regression test covering in-place log truncation.
- Around line 772-782: Update the rollup append flow around repairAppendBoundary
to acquire a cross-process lock before repairing and writing, holding it through
fsyncSync and releasing it in the existing cleanup path. Open the file with
O_APPEND and change the writeSync calls to pass null for the position,
preserving the loop so the complete append is serialized across processes.
In `@src/usage/summary.ts`:
- Around line 164-170: Update rollupDateOverlapsRange and the surrounding
summary merge logic so a rollup day that begins before a partial since timestamp
is excluded from aggregates, leaving that boundary day to the exact
filteredEntries raw tail; continue including complete days wholly within the
range. Add a regression test with entries both before and after since on the
same local date and verify the merged result does not include the pre-since
usage.
- Around line 419-429: Update the rollup overflow handling around
dayModelSeedRequests and the “other” row construction to preserve request
identity across models, so a request attributed to multiple overflow models is
counted once like the live shared requests Set path. Persist or propagate enough
request-group information to deduplicate merged overflow requests rather than
summing per-model row.requests. Add coverage using more than
MAX_USAGE_MODEL_BREAKDOWN_ROWS models with one request attributed to multiple
overflow models, and verify rollup and unbounded results match.
In `@tests/helpers/usage-rollup-fixtures.ts`:
- Around line 35-48: Preserve the intended fold-boundary coverage by updating
test 1 in usage-rollup-merge.test.ts to retain the fixture-generated timestamps
for boundary rows instead of rewriting every generated entry. Because generated
is filtered, use fixture.foldBoundaryTimestamp for boundary assertions rather
than recomputing the original index modulus; keep the remaining synthetic
timestamp behavior unchanged.
- Line 86: Replace the random comparator in the entries shuffle with a seeded
Fisher–Yates loop, iterating from the final index down to 1 and selecting each
swap position with random() across the inclusive range. Preserve the seeded
random source and swap entries in place so permutation and random-call behavior
are deterministic across engines.
In `@tests/usage-rollup-merge.test.ts`:
- Around line 295-301: Add an explicit raw usage-log size precondition in the
phase-2 setup before starting the legacy server or asserting the response,
matching phase 1’s readFileSync(usageLogPath()).byteLength check and verifying
it exceeds the configured 300-byte managementUsageMaxReadBytes cap. Keep the
existing truncation assertions unchanged.
- Line 313: Remove the explicit apiKeyId: undefined from the entry fixture in
the usage-rollup merge test, leaving the optional property omitted while
preserving the loopback entry’s other fields and runtime behavior. Also update
the analogous usage fixture in the usage-rollup test to omit optional fields
currently assigned undefined, including usage and totalTokens.
- Around line 116-119: Update canonicalSummary’s day-row mapping to normalize
estimatedCostUsd consistently with the existing model and provider
normalization, if day rows expose that field, while preserving the day.models
sorting. Add an explicit toBeCloseTo assertion for the day-level cost in the
corresponding exact-equality test so the value remains covered rather than
discarded.
In `@tests/usage-rollup.test.ts`:
- Around line 185-194: The test fails to isolate the rowCount validation because
the digest restoration at line 190 inadvertently reapplies the tampered digest
instead of restoring the original. Save the original digest value from the
commit object immediately after line 186 before any tampering occurs, then use
that saved value to restore commit.payloadDigest at line 190 instead of
re-reading from commits() which returns the tampered version from disk. This
ensures that only the rowCount modification is active when line 193 validates,
properly isolating the rowCount check from the digest check.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 49484994-d35a-423c-b8ea-98d0015839e5
⛔ Files ignored due to path filters (1)
src/generated/jawcode-model-metadata.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
devlog/_plan/260804_usage_rollup_preservation/000_research.mddevlog/_plan/260804_usage_rollup_preservation/001_roadmap.mddevlog/_plan/260804_usage_rollup_preservation/002_audit_synthesis.mddevlog/_plan/260804_usage_rollup_preservation/010_rollup_core.mddevlog/_plan/260804_usage_rollup_preservation/020_reader_merge.mddevlog/_plan/260804_usage_rollup_preservation/030_validation_docs_pr.mddocs-site/src/content/docs/reference/configuration/server.mddocs-site/src/content/docs/reference/management-api.mdscripts/generate-jawcode-metadata.tssrc/config.tssrc/server/management/api-key-usage.tssrc/server/management/logs-usage-routes.tssrc/types.tssrc/usage/cost.tssrc/usage/log.tssrc/usage/rollup.tssrc/usage/summary.tstests/api-usage.test.tstests/helpers/usage-rollup-fixtures.tstests/usage-rollup-merge.test.tstests/usage-rollup.test.ts
|
|
||
| ## Fold algorithm (`foldUsagePrefix`) — rev2 | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the pseudo-code fence.
The opening fence on Line 106 has no language tag. Add text or another suitable language so markdownlint does not report MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md` at line
106, Add a language tag such as text to the opening pseudo-code fence at the
affected documentation section, keeping the fenced content unchanged so
markdownlint no longer reports MD040.
Source: Linters/SAST tools
| **Synchronous validity gate (R2-2):** `readRollupSnapshot()` itself validates | ||
| version, lineage (against the live raw file), and priceFingerprint BEFORE | ||
| returning; any mismatch or absent/corrupt meta returns null, and the caller | ||
| serves raw-tail-only (cutline 0, legacy behavior) while the background rebuild | ||
| proceeds. Stale-cost data is structurally unreachable regardless of the async | ||
| fold's timing. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(010_rollup_core\.md|.*rollup.*|.*usage.*)' | head -200
printf '%s\n' '--- target section ---'
target='devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md'
wc -l "$target"
sed -n '120,210p' "$target"
printf '%s\n' '--- related symbols and size checks ---'
rg -n -C 4 'readRollupSnapshot|cutlineOffset|currentRawSize|raw.*size|usage\.jsonl|fold preflight|lineage|priceFingerprint' . \
-g '!node_modules' -g '!dist' -g '!build' | head -500Repository: lidge-jun/opencodex
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- rollup implementation map ---'
ast-grep outline src/usage/rollup.ts
printf '%s\n' '--- rollup implementation ---'
sed -n '1,280p' src/usage/rollup.ts
printf '%s\n' '--- rollup tests map and relevant cases ---'
ast-grep outline tests/usage-rollup.test.ts
rg -n -C 5 'readRollupSnapshot|live size|cutline|truncate|lineage|fingerprint|ensureRollupCurrent' \
src/usage/rollup.ts tests/usage-rollup.test.ts src/server/management/logs-usage-routes.ts \
devlog/_plan/260804_usage_rollup_preservation/020_reader_merge.mdRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- synchronous reader gate ---'
sed -n '420,438p' src/usage/rollup.ts
printf '%s\n' '--- asynchronous truncation recovery ---'
sed -n '729,760p' src/usage/rollup.ts
printf '%s\n' '--- route merge path ---'
sed -n '180,225p' src/server/management/logs-usage-routes.ts
printf '%s\n' '--- raw snapshot reader ---'
ast-grep outline src/usage/summary.ts
rg -n -C 8 'readUsageSnapshotForManagement|fromOffset|truncatedPrefixBytes|readUsage' \
src/usage/summary.ts src/usage/log.ts src/server/management/logs-usage-routes.ts | head -260
printf '%s\n' '--- truncation regression coverage ---'
sed -n '248,273p' tests/usage-rollup.test.tsRepository: lidge-jun/opencodex
Length of output: 22530
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
rollup = Path("src/usage/rollup.ts").read_text()
reader = re.search(
r"export function readRollupSnapshot\(\): RollupSnapshot \| null \{(?P<body>.*?)\n\}",
rollup,
re.S,
)
if not reader:
raise SystemExit("readRollupSnapshot body not found")
body = reader.group("body")
print("readRollupSnapshot_has_fstat:", "fstatSync" in body)
print("readRollupSnapshot_has_size_cutline_comparison:",
bool(re.search(r"(rawSize|currentRawSize|size).*cutlineOffset|cutlineOffset.*(rawSize|currentRawSize|size)", body)))
print("readRollupSnapshot_returns_parsed_snapshot:",
"return parseRollup(currentLineage).snapshot;" in body)
fold = re.search(
r"export async function foldUsagePrefix\(\): Promise<void> \{(?P<body>.*?)\n\}",
rollup,
re.S,
)
if not fold:
raise SystemExit("foldUsagePrefix body not found")
fold_body = fold.group("body")
print("fold_has_async_size_cutline_check:",
"rawSize < parsed.snapshot.cutlineOffset" in fold_body)
tests = Path("tests/usage-rollup.test.ts").read_text()
case = re.search(r'test\("4c\. live raw size below.*?(?=\n\s*test\(|\Z)', tests, re.S)
print("truncation_test_present:", bool(case))
if case:
text = case.group(0)
print("truncation_test_calls_read_before_rebuild:",
bool(re.search(r"truncateSync.*?readRollupSnapshot\(\)", text, re.S)))
print("truncation_test_calls_rebuild_before_read:",
bool(re.search(r"truncateSync.*?ensureRollupCurrent\(\).*?readRollupSnapshot\(\)", text, re.S)))
PYRepository: lidge-jun/opencodex
Length of output: 462
Invalidate snapshots when the raw file is shorter than the committed cutline.
readRollupSnapshot() does not check the live raw-file size. After in-place truncation below cutlineOffset, /api/usage passes the invalid offset to readUsageSnapshotForManagement(), which throws and returns read_failed until the asynchronous rebuild completes.
Add a synchronous size check in src/usage/rollup.ts:424-432 and return null when currentRawSize < cutlineOffset. Extend tests/usage-rollup.test.ts:260-270 to call readRollupSnapshot() before rebuilding and expect null.
🤖 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 `@devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md` around
lines 162 - 167, The readRollupSnapshot() function in src/usage/rollup.ts
currently does not validate whether the live raw file is large enough to support
the committed cutlineOffset. Add a synchronous check within the validity gate
(around lines 424-432) that compares the currentRawSize against cutlineOffset
and returns null when the file is shorter than the cutline, preventing invalid
offsets from reaching downstream functions like
readUsageSnapshotForManagement(). Then extend the test file
tests/usage-rollup.test.ts (around lines 260-270) to call readRollupSnapshot()
after truncating the raw file below the cutline and verify it returns null
instead of proceeding with an invalid snapshot.
| | `GET /api/debug/injection-logs` | Read bounded guidance-injection debug entries | — | | ||
| | `GET /api/claude/inbound-debug` | Read Claude inbound debug state and entries | — | | ||
| | `GET /api/usage` | Summarize usage by range and client surface | Returns an `error: "read_failed"` summary if storage cannot be read | | ||
| | `GET /api/usage` | Summarize usage by range and client surface. History older than the bounded read window is served from the daily rollup sidecar (`usage-rollup.jsonl`), so `all`-range summaries keep the full history; `7d` stays raw-exact, and `30d` includes rolled-up days at day granularity. Deleting the rollup files is safe — they rebuild in the background. | Returns an `error: "read_failed"` summary if storage cannot be read | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the rollup availability and exactness conditions.
The route serves raw-tail-only data when rollup is disabled, invalid, rebuilding, or has a residual gap. Therefore, all is full-history only when the rollup is valid and caught up. 7d is raw-exact only when truncatedPrefixBytes === 0. For 30d, the whole rolled-up boundary day can add up to approximately 24 hours outside the exact window.
Update this row to state these conditions. Also state that deleting the sidecar can temporarily restore truncation until the rebuild completes.
🤖 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 `@docs-site/src/content/docs/reference/management-api.md` at line 127, Update
the GET /api/usage documentation row to qualify rollup behavior: all-range
summaries retain full history only when the rollup is valid and caught up; 7d is
raw-exact only when truncatedPrefixBytes is 0; and 30d may include up to
approximately 24 hours from the rolled-up boundary day. State that disabled,
invalid, rebuilding, or gapped rollups serve raw-tail-only data, and deleting
usage-rollup.jsonl can temporarily restore truncation until background
rebuilding completes.
Source: Path instructions
| const configSchema = z.object({ | ||
| port: z.number().int().min(0).max(65535).default(10100), | ||
| managementUsageMaxReadBytes: z.number().int().positive().default(64 * 1024 * 1024), | ||
| usageRollupEnabled: z.boolean().default(true), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add .catch(true) so a hand-edited bad value cannot trigger the backup-and-defaults reset.
usageRollupEnabled: z.boolean().default(true) fails the parse for any non-boolean value. Trace what a hand-edited "usageRollupEnabled": "true" (a string) does in loadConfig() at Lines 1617-1666:
- Line 1630
configSchema.safeParse(parsed)fails withinvalid_type. - Line 1644 builds
merged = { ...defaults, ...parsed }. The spread order puts the parsed bad value last, so the merge cannot repair a field that is present-but-invalid — it only repairs missing fields. - Line 1649 retry fails for the same reason.
- Line 1660 calls
warnAndBackupInvalidConfigand returnsgetDefaultConfig(), discardingproviders,codexAccounts, andapiKeys.
This is the exact outcome the surrounding comments say to avoid. Lines 985-1002 state that a hand-edited typo "must never trip the backup-and-defaults repair path below and wipe providers/pool accounts", and note that an emptied apiKeys array is worse than cosmetic because assertServerAuthConfig refuses to start without a data credential. streamMode, hostname, injectionModel, and syncCodexSubagentDefaults all carry .catch(...) for this reason.
Degrading is safe for this field. The consumer in src/server/management/logs-usage-routes.ts tests config.usageRollupEnabled !== false, so any degraded value keeps the rollup enabled — matching the documented default in src/types.ts:546-547.
🛡️ Proposed fix
- usageRollupEnabled: z.boolean().default(true),
+ // Degrade-don't-reject, same rationale as `streamMode` below: a hand-edited
+ // non-boolean must not trip the backup-and-defaults repair path and wipe
+ // providers/apiKeys for one optional derived-cache flag.
+ usageRollupEnabled: z.boolean().default(true).catch(true),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| usageRollupEnabled: z.boolean().default(true), | |
| // Degrade-don't-reject, same rationale as `streamMode` below: a hand-edited | |
| // non-boolean must not trip the backup-and-defaults repair path and wipe | |
| // providers/apiKeys for one optional derived-cache flag. | |
| usageRollupEnabled: z.boolean().default(true).catch(true), |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/config.ts` at line 951, Update the usageRollupEnabled schema definition
to add a true fallback for invalid values, while retaining the existing default
for missing values. Follow the established .catch(...) pattern used by nearby
configuration fields so hand-edited non-boolean values remain enabled and do not
enter loadConfig’s backup-and-defaults reset path.
| const folded = readRollupSnapshot(); | ||
| const cutlineOffset = folded?.cutlineOffset ?? 0; | ||
| const observedKey = `${usageLogRevisionKey(currentUsageLogRevision())}|${cutlineOffset}|${idsKey}`; | ||
| if (rollupCache?.revisionKey === observedKey && now < rollupCache.expiresAt) { | ||
| return rollupCache.snapshot; | ||
| } | ||
|
|
||
| const snapshot = await readUsageSnapshotForManagement(maxReadBytes); | ||
| const snapshot = await readUsageSnapshotForManagement(maxReadBytes, cutlineOffset); | ||
| const rolled = { | ||
| ...rollupApiKeyUsage(snapshot.entries, configuredIds, now), | ||
| ...rollupApiKeyUsage(snapshot.entries, configuredIds, now, folded?.keys, folded?.attributionSinceMs ?? null), | ||
| ...(snapshot.truncatedPrefixBytes > 0 || snapshot.entriesTruncated ? { historyTruncated: true as const } : {}), | ||
| }; | ||
| rollupCache = { | ||
| revisionKey: `${usageLogRevisionKey(snapshot.revision)}|${idsKey}`, | ||
| revisionKey: `${usageLogRevisionKey(snapshot.revision)}|${cutlineOffset}|${idsKey}`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Honor usageRollupEnabled in API-key usage aggregation.
This path always reads readRollupSnapshot(). If a user disables rollups after sidecar files exist, /api/usage ignores them but API-key usage continues to merge them and starts the raw read at their cutline.
Pass the configured flag into this path. When the flag is false, use cutlineOffset = 0 and omit folded.keys and folded.attributionSinceMs. Include the flag in the cache identity or clear the cache when it changes.
Add a regression test that creates a rollup snapshot and then disables the feature.
As per path instructions, runtime changes must not bypass the shared routing/config layers. <path_instructions>
🤖 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 `@src/server/management/api-key-usage.ts` around lines 160 - 173, Update the
API-key usage aggregation around readRollupSnapshot and
readUsageSnapshotForManagement to accept and honor the configured
usageRollupEnabled flag: when disabled, use cutlineOffset 0 and pass no folded
keys or attributionSinceMs; when enabled, preserve the existing rollup behavior.
Include the flag in rollupCache.revisionKey or invalidate rollupCache when it
changes, and add a regression test covering a created rollup snapshot followed
by disabling the feature through the shared routing/configuration layers.
Source: Path instructions
| const duplicateRequestId = `property-${seed}-duplicate`; | ||
| entries.push({ ...entries[0]!, requestId: duplicateRequestId, timestamp: now - 15 * dayMs }); | ||
| entries.push({ ...entries[1]!, requestId: duplicateRequestId, timestamp: now - 12 * dayMs }); | ||
| entries.sort(() => random() - 0.5); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ECMAScript Array.prototype.sort inconsistent comparator implementation-defined order JavaScriptCore
💡 Result:
In ECMAScript, the behavior of Array.prototype.sort is explicitly defined as implementation-defined if the provided comparator is not consistent [1][2]. A consistent comparator is required to satisfy specific mathematical properties, including reflexivity, symmetry of anti-symmetry, and transitivity [2][3]. If these conditions are not met, the resulting order of the array elements is not guaranteed by the ECMAScript specification [1][2]. For JavaScriptCore (the engine used by Safari and other WebKit-based browsers), the implementation of Array.prototype.sort has been updated over time to better align with the tightened ECMAScript specification [4]. While the specification allows for implementation-defined behavior when a comparator is inconsistent, major engines, including JavaScriptCore, generally aim to follow the formal requirements for well-behaved comparators [4]. If a non-well-formed comparator—one that violates anti-symmetry or transitivity—is used, the sort result can and often will differ across JavaScript engines (e.g., JavaScriptCore, V8, and SpiderMonkey) [5]. This occurs because different engines may employ different internal sorting algorithms (such as variations of Timsort or Quicksort) which handle inconsistent input in engine-specific ways [6][7]. Consequently, developers are strongly advised to ensure that their comparator functions are "well-formed" and consistent, as relying on engine-specific handling of inconsistent comparators can lead to unpredictable or non-portable code behavior [5].
Citations:
- 1: https://tc39.es/ecma262/2026/multipage/indexed-collections.html
- 2: https://tc39.es/ecma262/2025/multipage/indexed-collections.html
- 3: https://tc39.es/ecma262/2023/multipage/indexed-collections.html
- 4: WebKit/WebKit@73a3669
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort
- 6: https://v8.dev/blog/array-sort
- 7: https://v8.dev/features/stable-sort
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- fixture ---'
cat -n tests/helpers/usage-rollup-fixtures.ts | sed -n '1,115p'
printf '%s\n' '--- consumer references ---'
rg -n -C 8 'usage-rollup-fixtures|ageDays|foldBoundaryTimestamp|entries' tests/usage-rollup-merge.test.ts tests/helpersRepository: lidge-jun/opencodex
Length of output: 1924
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Read-only behavioral probe for the random comparator and seeded Fisher-Yates.
# This models Array#sort with a deliberately inconsistent comparator so that the
# PRNG draw count and resulting permutation are visible without executing repo code.
class Lcg:
def __init__(self, seed):
self.state = seed
self.draws = 0
def random(self):
self.state = (1664525 * self.state + 1013904223) & 0xffffffff
self.draws += 1
return self.state / 2**32
def random_comparator_sort(values, seed, algorithm):
rng = Lcg(seed)
a = list(values)
def cmp(x, y):
return -1 if rng.random() < 0.5 else 1
if algorithm == "insertion":
for i in range(1, len(a)):
value = a[i]
j = i
while j > 0 and cmp(a[j - 1], value) > 0:
a[j] = a[j - 1]
j -= 1
a[j] = value
elif algorithm == "merge":
def sort(xs):
if len(xs) < 2:
return xs
mid = len(xs) // 2
left, right = sort(xs[:mid]), sort(xs[mid:])
out = []
while left and right:
if cmp(left[0], right[0]) <= 0:
out.append(left.pop(0))
else:
out.append(right.pop(0))
return out + left + right
a = sort(a)
return a, rng.draws
def fisher_yates(values, seed):
rng = Lcg(seed)
a = list(values)
for i in range(len(a) - 1, 0, -1):
j = int(rng.random() * (i + 1))
a[i], a[j] = a[j], a[i]
return a, rng.draws
values = list(range(12))
for algorithm in ("insertion", "merge"):
print(algorithm, random_comparator_sort(values, 12345, algorithm))
print("fisher-yates", fisher_yates(values, 12345))
PYRepository: lidge-jun/opencodex
Length of output: 1924
Replace the random comparator with a seeded Fisher–Yates shuffle.
entries.sort(() => random() - 0.5) uses an inconsistent comparator. ECMAScript does not guarantee the resulting order in this case. The engine's comparison sequence controls both the permutation and the number of random() calls.
This permutation affects the age-based fold/tail split in tests/usage-rollup-merge.test.ts:157. Bun upgrades can therefore change fixture coverage without changing the seed.
Use a seeded Fisher–Yates shuffle at tests/helpers/usage-rollup-fixtures.ts:86:
for (let index = entries.length - 1; index > 0; index -= 1) {
const swap = Math.floor(random() * (index + 1));
[entries[index], entries[swap]] = [entries[swap]!, entries[index]!];
}🤖 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 `@tests/helpers/usage-rollup-fixtures.ts` at line 86, Replace the random
comparator in the entries shuffle with a seeded Fisher–Yates loop, iterating
from the final index down to 1 and selecting each swap position with random()
across the inclusive range. Preserve the seeded random source and swap entries
in place so permutation and random-call behavior are deterministic across
engines.
| const days = summary.days.map(day => ({ | ||
| ...day, | ||
| models: [...day.models].sort((a, b) => `${a.provider}/${a.model}`.localeCompare(`${b.provider}/${b.model}`)), | ||
| })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
canonicalSummary normalizes cost for models and providers but not for day rows, which can make line 188 float-fragile.
Lines 112 and 114 deliberately coerce estimatedCostUsd on model and provider rows, and test 1 then zeroes those fields at lines 184-187 before the exact toEqual at line 188. The day mapping at lines 116-119 does neither — it only re-sorts day.models.
The failure mode: per the PR objectives, the rollup stores fold-time cost sums, while the reference summarizeUsage(entries, "all", FIXED_NOW) at line 176 recomputes cost from raw entries. The two paths sum the same floating-point values in a different order, so they can differ in the last bits. Test 1 guards against exactly that for the summary (line 179, toBeCloseTo(..., 9)), for models (line 191), and for providers (line 195) — but line 188 still compares day rows with exact structural equality. If UsageSummary["days"][number] or its nested models carry estimatedCostUsd, this test can fail intermittently on a corpus reshuffle, and the failure would look like an unrelated rollup bug.
If day rows do carry a cost field, zero it the same way:
♻️ Proposed fix: normalize day-level cost too
const days = summary.days.map(day => ({
...day,
- models: [...day.models].sort((a, b) => `${a.provider}/${a.model}`.localeCompare(`${b.provider}/${b.model}`)),
+ ...(("estimatedCostUsd" in day) ? { estimatedCostUsd: 0 } : {}),
+ models: [...day.models]
+ .map(row => ({ ...row, ...(("estimatedCostUsd" in row) ? { estimatedCostUsd: 0 } : {}) }))
+ .sort((a, b) => `${a.provider}/${a.model}`.localeCompare(`${b.provider}/${b.model}`)),
}));Then add an explicit toBeCloseTo check for the day-level cost, mirroring lines 189-196, so the field stays covered rather than silently dropped.
Run this to confirm whether day rows and per-day model rows expose estimatedCostUsd:
#!/bin/bash
# Description: Determine whether UsageSummary day rows carry a cost field that line 188 compares exactly.
set -euo pipefail
echo "=== Locate the summary types ==="
ast-grep outline src/usage/summary.ts --items all
echo "=== Day/model row interfaces ==="
rg -nP --type=ts -A 25 '(interface|type)\s+(UsageSummary|UsageDay\w*|UsageModel\w*|UsageProvider\w*)\b' src/usage/summary.ts
echo "=== Where day rows receive estimatedCostUsd ==="
rg -nP --type=ts -C4 'estimatedCostUsd' src/usage/summary.ts🤖 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 `@tests/usage-rollup-merge.test.ts` around lines 116 - 119, Update
canonicalSummary’s day-row mapping to normalize estimatedCostUsd consistently
with the existing model and provider normalization, if day rows expose that
field, while preserving the day.models sorting. Add an explicit toBeCloseTo
assertion for the day-level cost in the corresponding exact-equality test so the
value remains covered rather than discarded.
| saveConfig(baseConfig({ managementUsageMaxReadBytes: 300, usageRollupEnabled: false })); | ||
| writeRaw([firstOld, entry({ requestId: "old-2", timestamp: FIXED_NOW - 12 * DAY_MS })]); | ||
| const legacyServer = startServer(0); | ||
| try { | ||
| const legacy = await managementFetch(new URL("/api/usage?range=all", legacyServer.url)).then(response => response.json()) as UsageRouteBody; | ||
| expect(legacy.historyTruncated).toBe(true); | ||
| expect(legacy.truncatedPrefixBytes).toBeGreaterThan(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Phase 2 asserts truncation without asserting the precondition that makes truncation possible.
Phase 1 pins its precondition explicitly at line 285:
expect(readFileSync(usageLogPath()).byteLength).toBeGreaterThan(300);Phase 2 sets the same managementUsageMaxReadBytes: 300 cap at line 295, writes two entries at line 296, then asserts historyTruncated === true at line 300 — with no equivalent size check.
The two entries currently serialize to roughly 420 bytes, so the test passes today. The margin is thin and undefended. If a field is later removed from the entry() helper at lines 52-58, or the requestId values shorten, the raw log can drop under 300 bytes. Then historyTruncated becomes false and line 300 fails with a bare expected true, received false — a message that points at the rollup flag logic rather than at the real cause, which is fixture size.
Add the same precondition assertion phase 1 already uses:
♻️ Proposed fix: pin the phase-2 precondition
saveConfig(baseConfig({ managementUsageMaxReadBytes: 300, usageRollupEnabled: false }));
writeRaw([firstOld, entry({ requestId: "old-2", timestamp: FIXED_NOW - 12 * DAY_MS })]);
+ // Truncation only happens above the cap; fail loudly if the fixture shrinks.
+ expect(readFileSync(usageLogPath()).byteLength).toBeGreaterThan(300);
const legacyServer = startServer(0);🤖 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 `@tests/usage-rollup-merge.test.ts` around lines 295 - 301, Add an explicit raw
usage-log size precondition in the phase-2 setup before starting the legacy
server or asserting the response, matching phase 1’s
readFileSync(usageLogPath()).byteLength check and verifying it exceeds the
configured 300-byte managementUsageMaxReadBytes cap. Keep the existing
truncation assertions unchanged.
| const entries = [ | ||
| entry({ requestId: "key-old", timestamp: FIXED_NOW - 15 * DAY_MS, admissionKind: "configured", apiKeyId: "key-a" }), | ||
| entry({ requestId: "key-recent", timestamp: FIXED_NOW - 2 * DAY_MS, admissionKind: "configured", apiKeyId: "key-a" }), | ||
| entry({ requestId: "loopback", timestamp: FIXED_NOW - DAY_MS, admissionKind: "loopback", apiKeyId: undefined }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
apiKeyId: undefined may not typecheck, and it is redundant.
Line 313 assigns undefined explicitly to apiKeyId, which src/usage/log.ts lines 46-89 declare as an optional property (apiKeyId?: string). If this repo enables exactOptionalPropertyTypes, assigning undefined to an optional property is a type error; the property must be omitted instead.
There is direct evidence in this PR that the flag is likely on. The new fixture helper tests/helpers/usage-rollup-fixtures.ts goes out of its way to avoid emitting undefined for the same optional fields, using conditional spreads at lines 69-70 and 74-76:
...(index % 3 === 0 ? { admissionKind: "configured" as const, apiKeyId: `key-${index % 5}` } : {}),That pattern only exists to satisfy exactOptionalPropertyTypes. The same concern applies to tests/usage-rollup.test.ts line 123 (usage: undefined, totalTokens: undefined).
Omit the key. The runtime behavior is identical, because rollupApiKeyUsage gates on !entry.apiKeyId (see src/server/management/api-key-usage.ts line 92).
♻️ Proposed fix
- entry({ requestId: "loopback", timestamp: FIXED_NOW - DAY_MS, admissionKind: "loopback", apiKeyId: undefined }),
+ entry({ requestId: "loopback", timestamp: FIXED_NOW - DAY_MS, admissionKind: "loopback" }),Run this to confirm whether exactOptionalPropertyTypes is enabled and whether a typecheck gate exists:
#!/bin/bash
# Description: Check exactOptionalPropertyTypes and any typecheck CI gate.
set -euo pipefail
echo "=== tsconfig files ==="
fd -H -t f 'tsconfig.*json' --exec sh -c 'echo "--- $1 ---"; cat "$1"' _ {}
echo "=== exactOptionalPropertyTypes / strict settings ==="
rg -nP 'exactOptionalPropertyTypes|"strict"|noUncheckedIndexedAccess' -g 'tsconfig*.json'
echo "=== typecheck scripts ==="
rg -nP '"(typecheck|check|lint|test)"\s*:' package.json🤖 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 `@tests/usage-rollup-merge.test.ts` at line 313, Remove the explicit apiKeyId:
undefined from the entry fixture in the usage-rollup merge test, leaving the
optional property omitted while preserving the loopback entry’s other fields and
runtime behavior. Also update the analogous usage fixture in the usage-rollup
test to omit optional fields currently assigned undefined, including usage and
totalTokens.
| const rows = parsedRollupLines(); | ||
| const commit = rows.at(-1)!; | ||
| commit.payloadDigest = "0".repeat(64); | ||
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | ||
| expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] }); | ||
| commit.payloadDigest = (commits()[0]?.payloadDigest ?? "0".repeat(64)); | ||
| commit.rowCount = Number(commit.rowCount) + 1; | ||
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | ||
| expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test 2c never exercises the rowCount check in isolation, because line 190 restores the tampered digest instead of the original one.
Trace the state of the file across the test:
- Line 185-186:
rowsis read from disk, andcommitis a reference into that array. ItspayloadDigestis the valid digest. - Line 187:
commit.payloadDigest = "0".repeat(64)overwrites the valid digest in memory. The original value is now lost — nothing saved it. - Line 188: the tampered
rowsare written to disk. - Line 189: the snapshot is rejected. Correct, and this is a real digest-mismatch assertion.
- Line 190:
commit.payloadDigest = (commits()[0]?.payloadDigest ?? "0".repeat(64)).commits()re-readsusage-rollup.jsonlfrom disk — which now holds the tampered commit from step 3. Socommits()[0].payloadDigestis"0".repeat(64). The row exists, so the??fallback never fires. The assignment is a no-op that re-applies the zeroed digest. - Line 191:
rowCountis incremented. - Line 193: the snapshot is rejected — but now two validators are violated at once: the digest and the row count.
The failure mode: if the rowCount validation were deleted from src/usage/rollup.ts, this test would still pass at line 193, because the zeroed payloadDigest alone forces rejection. The test title promises "row-count or payload-digest mismatch", and the PR objectives list rowCount as one of the three commit-row authorities alongside attemptId and payloadDigest. This is the only test that touches rowCount, so that validator currently has no effective coverage.
Capture the original digest before tampering, and restore it from the captured value.
🐛 Proposed fix: isolate each validator
const rows = parsedRollupLines();
const commit = rows.at(-1)!;
+ const validDigest = commit.payloadDigest;
+ const validRowCount = commit.rowCount;
commit.payloadDigest = "0".repeat(64);
writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`);
expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] });
- commit.payloadDigest = (commits()[0]?.payloadDigest ?? "0".repeat(64));
- commit.rowCount = Number(commit.rowCount) + 1;
+ // Restore the valid digest so only the row count is wrong on this pass.
+ commit.payloadDigest = validDigest;
+ commit.rowCount = Number(validRowCount) + 1;
writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`);
expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] });
+ // Sanity check: with both fields restored the segment is accepted again,
+ // which proves the two rejections above came from the tampering.
+ commit.rowCount = validRowCount;
+ writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`);
+ expect(readRollupSnapshot()?.days.length).toBeGreaterThan(0);The added final block is the part that makes the two negative assertions meaningful. Without a positive control, a validator that rejects unconditionally would also satisfy lines 189 and 193.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rows = parsedRollupLines(); | |
| const commit = rows.at(-1)!; | |
| commit.payloadDigest = "0".repeat(64); | |
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | |
| expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] }); | |
| commit.payloadDigest = (commits()[0]?.payloadDigest ?? "0".repeat(64)); | |
| commit.rowCount = Number(commit.rowCount) + 1; | |
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | |
| expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] }); | |
| }); | |
| const rows = parsedRollupLines(); | |
| const commit = rows.at(-1)!; | |
| const validDigest = commit.payloadDigest; | |
| const validRowCount = commit.rowCount; | |
| commit.payloadDigest = "0".repeat(64); | |
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | |
| expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] }); | |
| // Restore the valid digest so only the row count is wrong on this pass. | |
| commit.payloadDigest = validDigest; | |
| commit.rowCount = Number(validRowCount) + 1; | |
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | |
| expect(readRollupSnapshot()).toMatchObject({ cutlineOffset: 0, days: [] }); | |
| // Sanity check: with both fields restored the segment is accepted again, | |
| // which proves the two rejections above came from the tampering. | |
| commit.rowCount = validRowCount; | |
| writeFileSync(rollupPath(), `${rows.map(row => JSON.stringify(row)).join("\n")}\n`); | |
| expect(readRollupSnapshot()?.days.length).toBeGreaterThan(0); | |
| }); |
🤖 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 `@tests/usage-rollup.test.ts` around lines 185 - 194, The test fails to isolate
the rowCount validation because the digest restoration at line 190 inadvertently
reapplies the tampered digest instead of restoring the original. Save the
original digest value from the commit object immediately after line 186 before
any tampering occurs, then use that saved value to restore commit.payloadDigest
at line 190 instead of re-reading from commits() which returns the tampered
version from disk. This ensures that only the rowCount modification is active
when line 193 validates, properly isolating the rowCount check from the digest
check.
Problem
usage.jsonlis append-only and unbounded. The management reader caps reads atmanagementUsageMaxReadBytes(64 MiB default) plus a 200k-entry cap, so once the file outgrows the window the oldest days silently vanish from every/api/usageconsumer. This happened in production on 2026-08-04: a 157 MB / 380k-row log lost 11 of 30 days in the 30d view (historyTruncated: true,truncatedPrefixBytes: 97.5 MB). Raising the caps only defers the loss and slows every read.Design
Fold rows that leave the read window into a daily-aggregate rollup sidecar, and make the summary reader merge rollup (old days) + raw tail (recent days):
usage-rollup.jsonl(append-only aggregates) +usage-rollup-meta.json(lineage/fingerprint/throttle), both 0600 and ownership-registered. The raw log is never truncated — the rollup is a rebuildable derived cache.commitrow last (one fsync). A segment is visible only when its commit row validates byattemptId+rowCount+payloadDigest; the effective cutline and the raw-tail start both derive from the same committed segments, so there is no crash window where data double-counts. Partial appends are invisible garbage; the append boundary isftruncate-repaired before a retry.requests7din the API-key view) stays raw-exact. 30d includes rolled-up days at day granularity (the one boundary day is included whole, ≤ ~24h overcount — documented).readRollupSnapshot()verifies version/lineage/fingerprint before returning; any mismatch serves the legacy raw-tail path while the background rebuild proceeds.usageRollupEnabled: falserestores today's behavior entirely.Design provenance:
devlog/_plan/260804_usage_rollup_preservation/— a prior-art survey (Kafka segment compaction, SQLite WAL ordering, logrotate copytruncate pitfalls, TSDB downsampling, high-water-mark patterns; Tier-2 source-verified) plus a 4-round independent design audit whose blockers (crash protocol, distinct-request exactness domain,requests7downership, fingerprint completeness, same-lineage mutation) are each closed in the decade docs.Tests
tests/usage-rollup.test.ts(12): fold aggregates vs hand-computed fixtures, crash injection at every boundary (mid-append truncation, abandoned-attempt + retry collision, rowCount/digest mismatch, missing meta), lineage/fingerprint/size rebuild triggers, min-age watermark, mid-line cutline rejection.tests/usage-rollup-merge.test.ts(6): property testsummarize(fold(prefix) ⊕ tail) === summarize(full raw)for rangeall(totals, days, models, providers, cost to 1e-9), boundary-day additivity, surface predicates, 7d-never-touches-rollup + 30d day-grain, route cache invalidation on fold advance + flag-off legacy path, API-key totals across the fold boundary.Real-data validation
Against a copy of the 157 MB production log (381,096 rows): the merged all-range summary equals the unbounded full-raw parse — zero diffs across summary totals, all 39 day rows, all 90 model rows, and all providers (cost delta 2.5e-13 USD, float ordering).
truncatedPrefixBytes: 0with the rollup caught up. Fold of the 110 MB prefix: 1.7 s once; subsequent merged reads parse only the ~104k-row tail (378 ms vs full-file parses today).Docs
reference/configuration/server.mdgains theusageRollupEnabledrow;reference/management-api.mddocuments theGET /api/usagerollup semantics (locale files intentionally untouched pending translation sync).Follow-ups (out of scope)
Raw-file truncation/archival policy (needs writer coordination), GUI surfacing beyond existing truncation metadata.
Summary by CodeRabbit
New Features
usageRollupEnabledserver configuration (enabled by default) controls usage-history preservationBug Fixes
Documentation
Tests