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 7 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds a rebuildable usage-rollup sidecar that folds aged 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 usage prefix
Rollup->>Rollup: append validated rows and commit
Rollup->>Rollup: write metadata atomically
Route->>Rollup: readRollupSnapshot()
Rollup-->>Route: validated aggregates and cutline
Route->>Log: readUsageSnapshotForManagement(fromOffset=cutline)
Log-->>Route: raw tail entries
Route->>Summary: summarizeUsage(entries, rollup)
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 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| 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 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.
…ade docs (4-round audited)
…ildable derived cache (010)
…read window (020)
…r usageRollupEnabled in key summaries Review-thread fixes for #1008: eligibleCutline yields between chunks and caps each fold segment (bounded-memory first fold over a large log, one commit per segment); complete malformed rows no longer stall the cutline; readRollupSnapshot revalidates the committed boundary so a truncated-then- regrown raw log cannot serve a stale sidecar; API-key summaries honor usageRollupEnabled; docs qualify exactness when the rollup is unavailable; test 2c exercises the rowCount check in isolation.
…cannot stall the cutline
…on; document the digest-window contract
b0d5417 to
8d1eec8
Compare
|
✅ Deterministic PR hygiene checks passed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto current Fixed in this push:
Deferred with rationale (kept unresolved on purpose): compaction/rewrite path analysis, partial-range day boundary, cross-model overflow dedup (redesign-scale); timezone-change rebuild (documented limitation); fold-failure observability (needs a contract, not a catch tweak); remaining minor test/devlog notes. Verification: full suite 9088 pass / 0 fail / 8 skip (580 files), typecheck clean. Audit rounds: 2×FAIL findings (last-segment-only validation, per-call validation cost) were fixed and re-verified to PASS. |
|
This pull request mentions @lidge-jun Please add a screenshot of the UI change to the description — drag and drop the image into the description editor, or paste a markdown image such as This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/000_research.md`:
- Around line 93-96: Update the crash-retry contract in the ordering description
to assign a fresh attemptId to every fold attempt. Replace the range-only
segment key and skip rule with commit validation using (seg, attemptId,
rowCount, payloadDigest), ensuring only a validated commit suppresses retries.
- Around line 100-104: Update the “Token aggregates only; cost stays
display-time” section to describe the rev3 contract: cost is stored at fold
time, and rollups are rebuilt when the priceFingerprint changes. Remove claims
that cost is recomputed at display time or that price-table fixes are
automatically retroactive, while preserving the grouping-key and long-context
behavior.
In `@devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md`:
- Around line 202-203: Update the boundary-digest test described near the rollup
rebuild cases to mutate the final 4 KiB of a committed segment, matching the
supported mutation boundary rather than an unspecified prefix edit. Add a
separate test covering the documented escape hatch: deleting both sidecar files
and rebuilding after a deep edit.
- Around line 206-212: Keep the exactness property generator within its declared
domain: generate only unique requestIds and valid non-combo inputs for the
summarize(foldPrefix ⊕ tail) equality property. Move duplicate IDs and
combo-overflow cases into the two dedicated documenting tests, or gate them
behind an explicit includeOutOfDomain option that the property test does not
enable.
- Around line 107-113: Update parseRollup() so each successfully validated
commit requires a strictly forward toOffset and verifies it is within the
current live raw-file size before adding or using the segment offset; treat
zero, backward, or oversized values as invalid and route them through the
existing rebuild/preflight handling instead of allowing validBySeg.has(offset)
to abort first. Add a rollback test covering a stale or corrupt toOffset.
In `@devlog/_plan/260804_usage_rollup_preservation/020_reader_merge.md`:
- Around line 54-65: Gate the rollup flow in GET /api/usage with
config.usageRollupEnabled: only call readRollupSnapshot(), pass its
cutlineOffset, include its contribution in summarizeUsage, and append its
revision when enabled. When disabled, use fromOffset 0, omit rollup
contributions, and retain the legacy cache revision; add a regression test
covering an existing valid sidecar.
In `@devlog/_plan/260804_usage_rollup_preservation/030_validation_docs_pr.md`:
- Around line 19-23: Update the documentation plan to explicitly enumerate the
ja, ko, ru, and zh-cn locale pages and require comparing each with the English
source for /api/usage, usageRollupEnabled, raw-tail fallback, and 7d/30d
semantics. Require every locale page to be updated or explicitly marked pending,
ensuring none contradicts the English documentation.
In `@src/usage/rollup.ts`:
- Around line 856-862: Update the fold flow around foldUsagePrefix and writeMeta
so lastFoldAttemptAt is recorded in a finally path even when folding throws,
while preserving derived-cache success semantics and allowing the original error
to propagate. Add a focused regression test near the crash-recovery cases in
usage-rollup.test.ts that forces foldUsagePrefix to throw and verifies a second
ensureRollupCurrent call within the throttle window does not re-enter the fold.
🪄 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: 39484cb9-587c-474e-8f72-8ef1f1d4046d
⛔ Files ignored due to path filters (1)
src/generated/jawcode-model-metadata.tsis excluded by!**/generated/**
📒 Files selected for processing (22)
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/server/management/oauth-account-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
| 3. **Ordering: fold → fsync rollup → advance meta (temp+fsync+rename).** A crash | ||
| between rollup append and meta advance re-folds the same range next run; to | ||
| make that idempotent each fold writes a `segment` record keyed by | ||
| `(lineageId, fromOffset)` and the folder skips ranges already recorded. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the crash-retry contract to use attemptId.
Lines [93-96] still describe a segment keyed only by (lineageId, fromOffset) and skipped when that range is recorded. devlog/_plan/260804_usage_rollup_preservation/002_audit_synthesis.md Lines [86-99] documents why that is unsafe: abandoned rows and retry rows can share the same segment. State that each attempt uses a fresh attemptId, and that only a commit validated by (seg, attemptId, rowCount, payloadDigest) suppresses a retry.
🤖 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/000_research.md` around lines
93 - 96, Update the crash-retry contract in the ordering description to assign a
fresh attemptId to every fold attempt. Replace the range-only segment key and
skip rule with commit validation using (seg, attemptId, rowCount,
payloadDigest), ensuring only a validated commit suppresses retries.
| 5. **Token aggregates only; cost stays display-time.** Cost is linear in tokens | ||
| for a fixed `(provider, model, tier, longContext)` price row, so grouping by | ||
| those keys preserves exact display-time recomputation and keeps price-table | ||
| fixes retroactive. Long-context is per-request non-linear, hence it is part of | ||
| the group key, evaluated at fold time with the same predicate used today. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the cost description with the rev3 implementation.
Lines [100-104] say that cost remains display-time and that price-table fixes stay retroactive. The current contract stores fold-time cost and rebuilds on a priceFingerprint mismatch, as documented in 001_roadmap.md Lines [84-92] and 010_rollup_core.md Lines [94-102]. Replace this wording so validation and future implementations do not assume display-time cost recomputation.
🤖 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/000_research.md` around lines
100 - 104, Update the “Token aggregates only; cost stays display-time” section
to describe the rev3 contract: cost is stored at fold time, and rollups are
rebuilt when the priceFingerprint changes. Remove claims that cost is recomputed
at display time or that price-table fixes are automatically retroactive, while
preserving the grouping-key and long-context behavior.
| recovery preflight: | ||
| read rollup file (complete JSONL lines only; a partial trailing line is dropped | ||
| by framing); validate each commit row by (attemptId, rowCount, payloadDigest) | ||
| against its attempt's group rows; committed cutline = max contiguous committed | ||
| toOffset; rows of uncommitted/failed attempts are permanently invisible garbage | ||
| verify previous committed segment's boundaryDigest against live raw bytes; | ||
| mismatch OR live size < cutline OR lineage/fingerprint mismatch → full rebuild from 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'function isCommitRow|isCommitRow|validBySeg|for \(let offset = 0' src/usage/rollup.tsRepository: lidge-jun/opencodex
Length of output: 3076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== File references for usage_rollup_preservation devlog ==\n'
git ls-files | rg 'devlog/_plan/260804_usage_rollup_preservation/010_rollup_core\.md' || true
printf '\n== Plan excerpt around recovery preflight ==\n'
sed -n '90,120p' devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md
printf '\n== Relevant rollup.ts excerpt ==\n'
sed -n '292,360p' src/usage/rollup.ts
printf '\n== Tests mentioning corrupt committed offset / toOffset / segment chain ==\n'
rg -n -C 4 'toOffset|corrupt|segment chain|validBySeg|cutline|boundaryDigest|full rebuild|recovery preflight' src tests devlog --glob '!**/^node_modules/**' || true
printf '\n== Behavioral check: parser loop semantics for bounded map and zero/backward offsets ==\n'
nl -ba src/usage/rollup.ts | sed -n '292,360p'Repository: lidge-jun/opencodex
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Rollup parser source excerpt ==\n'
sed -n '260,370p' src/usage/rollup.ts
printf '\n== Rollup commit row type (if nearby) ==\n'
sed -n '220,260p' src/usage/rollup.ts
printf '\n== Direct rollup source hits ==\n'
rg -n -C 3 'isCommitRow|validBySeg|for \(let offset = 0|toOffset > value\.seg|toOffset >.*readFileSync|live size|boundaryDigest|recovery preflight' src/usage/rollup.ts
printf '\n== Targeted test search only ==\n'
rg -n -C 3 'toOffset|rollup.*corrupt|corrupt.*rollup|committed offset|segment chain|rollup_core|recovery preflight' tests src devlog/_plan/260804_usage_rollup_preservation/010_rollup_core.md --glob '*.test.ts' --glob '*.ts' --glob '*.md' || true
printf '\n== JavaScript loop-skip semantics probe ==\n'
node - <<'JS'
const validBySeg = new Map([
[0, { commit: { toOffset: 0, seg: 0 } }],
[1, { commit: { toOffset: 1, seg: 1 } }],
]);
const segments = [];
for (let offset = 0; validBySeg.has(offset); ) {
const segment = validBySeg.get(offset);
segments.push(segment.commit.seg);
offset = segment.commit.toOffset;
}
console.log(JSON.stringify({ segments }));
JSRepository: lidge-jun/opencodex
Length of output: 21556
Enforce strict progress and live bounds on toOffset.
src/usage/rollup.ts:296 rejects invalid toOffset values that are <= seg, but parseRollup() still assigns offset = segment.commit.toOffset without checking the live raw-file size. A zero or backward toOffset can make validBySeg.has(offset) fail immediately, which stops the rebuild before the live-size/preflight checks can trigger. Require toOffset to be within the live raw file after successful commit validation, and add a rollback test for a stale/corrupt toOffset.
🤖 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 107 - 113, Update parseRollup() so each successfully validated commit
requires a strictly forward toOffset and verifies it is within the current live
raw-file size before adding or using the segment offset; treat zero, backward,
or oversized values as invalid and route them through the existing
rebuild/preflight handling instead of allowing validBySeg.has(offset) to abort
first. Add a rollback test covering a stale or corrupt toOffset.
| 4. Price-fingerprint mismatch → rebuild resets cutline to 0; boundary-digest | ||
| mismatch (prefix edited in place) → rebuild; live size < cutline → rebuild. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make the boundary-digest test target the supported mutation boundary.
Lines [145-147] state that deep edits are not auto-detected and require deleting both sidecar files. Lines [202-203] say “prefix edited in place” without specifying the boundary region. Make the test edit the last 4 KiB of a committed segment, and add a separate test for the documented delete-and-rebuild escape hatch.
🤖 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 202 - 203, Update the boundary-digest test described near the rollup
rebuild cases to mutate the final 4 KiB of a committed segment, matching the
supported mutation boundary rather than an unspecified prefix edit. Add a
separate test covering the documented escape hatch: deleting both sidecar files
and rebuilding after a deep edit.
| 7. Property test (in-domain by construction — the generator emits unique | ||
| requestIds; out-of-domain cases live in the two documenting tests): for a | ||
| randomized fixture, summarize(foldPrefix ⊕ tail) equals | ||
| summarize(allRaw) for range "all" — this lands in 020 when the merge exists, | ||
| but the fixture generator is written here. Generator must include combo | ||
| attempts, duplicate requestIds across days (out-of-domain documenting case), | ||
| out-of-order timestamps, and fold-boundary-day entries. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the exactness property fixture inside its declared domain.
Lines [206-212] describe an in-domain property generator with unique request IDs, then require that generator to include duplicate IDs and out-of-domain combo cases. Those inputs invalidate the equality oracle described in Lines [176-183]. Split the fixtures or add an includeOutOfDomain option, and reserve duplicate IDs and combo overflow for the two dedicated documenting tests.
🧰 Tools
🪛 LanguageTool
[grammar] ~211-~211: Ensure spelling is correct
Context: ...st include combo attempts, duplicate requestIds across days (out-of-domain documenting ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 206 - 212, Keep the exactness property generator within its declared
domain: generate only unique requestIds and valid non-combo inputs for the
summarize(foldPrefix ⊕ tail) equality property. Move duplicate IDs and
combo-overflow cases into the two dedicated documenting tests, or gate them
behind an explicit includeOutOfDomain option that the property test does not
enable.
| In `GET /api/usage`: | ||
|
|
||
| 1. `if (config.usageRollupEnabled !== false) void ensureRollupCurrent()` — | ||
| fire-and-forget, throttled internally; the fold never blocks the request. | ||
| 2. `const rollup = readRollupSnapshot()` — synchronously validated (R2-2: | ||
| version/lineage/fingerprint checked inside; null on any mismatch → raw-tail | ||
| legacy path with cutline 0, so stale costs are unreachable); pass | ||
| `fromOffset = rollup?.cutlineOffset ?? 0` into the snapshot read and | ||
| the contribution into `summarizeUsage`. | ||
| 3. Cache key/revision: append the rollup `cutlineOffset` (single commit | ||
| authority — a fold advance changes it) to the | ||
| revision key so a fold invalidates cached summaries. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Gate rollup reads and cache revisions with usageRollupEnabled.
Step 1 skips ensureRollupCurrent() when the flag is false, but Step 2 still calls readRollupSnapshot() unconditionally. With a valid existing sidecar, disabling the flag would still merge historical rows and change the tail offset. This violates the required legacy fallback.
Only load the snapshot when the flag is enabled. Use fromOffset = 0, omit rollup contributions, and use the legacy cache revision when the flag is false. Add a regression test with an existing valid sidecar.
🤖 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/020_reader_merge.md` around
lines 54 - 65, Gate the rollup flow in GET /api/usage with
config.usageRollupEnabled: only call readRollupSnapshot(), pass its
cutlineOffset, include its contribution in summarizeUsage, and append its
revision when enabled. When disabled, use fromOffset 0, omit rollup
contributions, and retain the legacy cache revision; add a regression test
covering an existing valid sidecar.
| Update the usage/monitoring page: what `usage-rollup.jsonl` / | ||
| `usage-rollup-meta.json` are, that history is preserved past the 64 MiB read | ||
| window, day-grain nuance for 7d/30d, `usageRollupEnabled` flag, and that | ||
| deleting the rollup files is safe (they rebuild). Check translated locales for | ||
| contradictions per repo policy (update English; note locale sync if present). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make locale verification mandatory and explicit.
Line [23] says to check translated locales “if present,” but the repository policy names ja, ko, ru, and zh-cn. Enumerate those locale pages and require each page to be updated or explicitly marked as pending after comparison with the English /api/usage, usageRollupEnabled, raw-tail fallback, and 7d/30d semantics.
As per path instructions, translated ja, ko, ru, and zh-cn pages must not contradict the English source.
🤖 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/030_validation_docs_pr.md`
around lines 19 - 23, Update the documentation plan to explicitly enumerate the
ja, ko, ru, and zh-cn locale pages and require comparing each with the English
source for /api/usage, usageRollupEnabled, raw-tail fallback, and 7d/30d
semantics. Require every locale page to be updated or explicitly marked pending,
ensuring none contradicts the English documentation.
Source: Path instructions
| writeMeta({ | ||
| version: 1, | ||
| lineageKey: currentLineage, | ||
| priceFingerprint: fingerprint, | ||
| lastFoldAttemptAt: now, | ||
| updatedAt: Date.now(), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A throwing fold never records lastFoldAttemptAt, so every request retries the full fold.
writeMeta runs only after the segment loop completes. If any step inside try throws — boundaryDigest at Line 838 when the raw log shrinks mid-fold, openSync at Line 843 on EMFILE, or fsyncSync at Line 850 on ENOSPC — control jumps to the finally at Line 863 and writeMeta is skipped.
ensureRollupCurrent() at Lines 871-881 swallows the error and clears rollupFlight. The throttle check at Line 877 reads meta.lastFoldAttemptAt, which was never advanced. The next /api/usage request therefore repeats the whole attempt: parseRollup re-reads the sidecar, eligibleCutline rescans the raw log from the cutline, and parseUsageRange re-materializes up to 64 MiB of entries. A persistent failure converts a ten-minute throttle into a per-request scan of the usage log.
Record the attempt even when the fold fails. Keep the derived-cache semantics unchanged: lastFoldAttemptAt marks an attempt, not a success.
🛠️ Proposed fix: write the attempt marker in a `finally`
let parsed = parseRollup(currentLineage);
let rawFd: number | undefined;
+ let committedFingerprint = fingerprint;
try {
...
- writeMeta({
- version: 1,
- lineageKey: currentLineage,
- priceFingerprint: fingerprint,
- lastFoldAttemptAt: now,
- updatedAt: Date.now(),
- });
} finally {
if (rawFd !== undefined) closeSync(rawFd);
+ // Record the ATTEMPT, not the success. A fold that throws must still burn
+ // the throttle window, otherwise every /api/usage request rescans the log.
+ try {
+ writeMeta({
+ version: 1,
+ lineageKey: currentLineage,
+ priceFingerprint: committedFingerprint,
+ lastFoldAttemptAt: now,
+ updatedAt: Date.now(),
+ });
+ } catch { /* meta is derived; a missing marker only costs an early retry */ }
}Add a regression test near the existing crash-recovery cases in tests/usage-rollup.test.ts: force foldUsagePrefix() to throw, then assert that a second ensureRollupCurrent() inside the throttle window does not re-enter the fold. As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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/usage/rollup.ts` around lines 856 - 862, Update the fold flow around
foldUsagePrefix and writeMeta so lastFoldAttemptAt is recorded in a finally path
even when folding throws, while preserving derived-cache success semantics and
allowing the original error to propagate. Add a focused regression test near the
crash-recovery cases in usage-rollup.test.ts that forces foldUsagePrefix to
throw and verifies a second ensureRollupCurrent call within the throttle window
does not re-enter the fold.
Source: Path instructions
…ads: fix-now vs defer)
…d, pushed unmerged
1 participant
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, to control usage-history preservation.Bug Fixes
Documentation
Tests