Conversation
The mode/kind migration (1786136603_warm_omega_flight.sql) left a large slice of global_model_stats / global_source_stats as used_mode and org_kind 'unknown', so the admin Global Stats page cannot make Credits + BYOK add up to All and shows almost no organization-kind attribution. Rebuilding from the source data fixes it exactly, because data retention never deletes log rows — cleanupExpiredLogData only nulls the verbose payload columns (messages, content, raw/upstream request and response, tools, customHeaders, userAgent, responsesApiData). Every column the aggregator reads survives, so the rebuild is lossless and needs no proration or scaling. Each day goes through the aggregator's own recomputeDayFully(), which deletes that day and re-aggregates its 24 hours, so the script is idempotent and resumable. Newest day first, so the recent days people actually look at are correct within minutes rather than at the end of the run, and the table is never fully empty. Today and yesterday are skipped: the incremental walker owns today and the safety net owns yesterday, and racing either risks interleaving a delete with an insert. Dry run by default; --commit to apply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe worker now includes a CLI script and npm command to rebuild global model and source statistics from retained log rows. The script supports date planning, dry runs, limits, ordering, progress reporting, and committed recomputation. ChangesGlobal statistics rebuild
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant rebuild_global_stats
participant RetainedLogRows
participant recomputeDayFully
CLI->>rebuild_global_stats: Provide rebuild flags
rebuild_global_stats->>RetainedLogRows: Resolve dates and attribution counts
rebuild_global_stats->>recomputeDayFully: Recompute selected days
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/worker/src/scripts/rebuild-global-stats.ts (1)
138-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the dry-run counts for the planned range.
The dry-run query counts unattributed rows across the whole table. The planned range can be a subset, for example when
--from,--to, or--limitis set. The printed numbers then do not describe the work the run would perform.Add a
day_timestampfilter for the planned range.🤖 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 `@apps/worker/src/scripts/rebuild-global-stats.ts` around lines 138 - 149, Update the dry-run query in the !commit branch of the rebuild flow to apply the same planned-range day_timestamp bounds used by the rebuild, including --from, --to, and any --limit-derived endpoint. Keep the existing unattributed-row conditions and report counts only for rows that the run would process.
🤖 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 `@apps/worker/src/scripts/rebuild-global-stats.ts`:
- Line 93: Validate every CLI date and numeric flag before using it: in the
limit parsing around apps/worker/src/scripts/rebuild-global-stats.ts lines
93-93, parse --limit once and reject values that are not positive integers, then
update the day selection around line 132 to use the undefined check so a zero
limit does not select all days. In the --from/--to handling at lines 102-118,
reject Invalid Date results before clamp and range validation proceed.
- Around line 153-158: Register SIGINT and SIGTERM handlers in the standalone
script before the loop that invokes recomputeDayFully, using the existing
stop-request mechanism so signals set the requested-stop state instead of
terminating immediately. Preserve the loop’s current halted-after-completion
behavior and cleanup any handlers when the script finishes.
- Around line 78-88: Update the least(...) start-boundary query in the
rebuild-global-stats flow to interpret the timestamp without time zone values as
UTC before node-postgres converts them to Date objects. Apply AT TIME ZONE 'UTC'
to the SQL expression (or parse an explicitly Z-suffixed text value), then
preserve the existing floorToUTCDay handling and null behavior.
---
Nitpick comments:
In `@apps/worker/src/scripts/rebuild-global-stats.ts`:
- Around line 138-149: Update the dry-run query in the !commit branch of the
rebuild flow to apply the same planned-range day_timestamp bounds used by the
rebuild, including --from, --to, and any --limit-derived endpoint. Keep the
existing unattributed-row conditions and report counts only for rows that the
run would process.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3af539e7-c876-47a8-9364-5eeb2c6f6e50
📒 Files selected for processing (3)
apps/worker/package.jsonapps/worker/src/scripts/rebuild-global-stats.tsapps/worker/src/services/global-stats-aggregator.ts
| const result = await db.execute(sql` | ||
| select least( | ||
| (select created_at from log order by created_at limit 1), | ||
| (select min(day_timestamp) from global_model_stats), | ||
| (select min(day_timestamp) from global_source_stats) | ||
| ) as start | ||
| `); | ||
| const start = (result.rows[0] as { start: Date | string | null } | undefined) | ||
| ?.start; | ||
| return start ? floorToUTCDay(new Date(start)) : null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the column types for log.created_at, global_model_stats.day_timestamp, and global_source_stats.day_timestamp.
fd -e ts -e sql . --exec rg -n -C3 'day_timestamp|dayTimestamp' {} \; | head -60
echo "---- schema definitions ----"
ast-grep run --pattern 'timestamp($$$)' --lang typescript 2>/dev/null | head -40Repository: theopenco/llmgateway
Length of output: 4960
🏁 Script executed:
#!/bin/bash
set -e
echo "---- rebuild script ----"
sed -n '1,130p' apps/worker/src/scripts/rebuild-global-stats.ts
echo "---- schema declarations ----"
rg -n -C4 'createdAt|dayTimestamp|globalModelStats|globalSourceStats' packages/db/src/schema.ts
echo "---- migrations ----"
rg -n -i -C3 'global_model_stats|global_source_stats|created_at.*timestamp|day_timestamp.*timestamp' packages/db drizzle migrations 2>/dev/null | head -240
echo "---- timestamp configuration and PostgreSQL clients ----"
rg -n -C3 'timestamp\(|withTimezone|parseInputDatesAsUTC|pg-types|node-postgres|timezone|TZ' packages apps package.json pnpm-lock.yaml 2>/dev/null | head -240Repository: theopenco/llmgateway
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
echo "---- exact global-stats migrations ----"
rg -l -i 'global_model_stats|global_source_stats' packages/db/migrations | sort | while read -r f; do
echo "### $f"
rg -n -i -C5 'global_model_stats|global_source_stats' "$f"
done
echo "---- log table creation ----"
rg -l -n 'CREATE TABLE "log"|created_at.*timestamp' packages/db/migrations | while read -r f; do
if rg -q 'CREATE TABLE "log"' "$f"; then
echo "### $f"
rg -n -C8 'CREATE TABLE "log"|created_at.*timestamp' "$f"
fi
done
echo "---- database client and timestamp options ----"
fd -i 'package.json|*.ts' packages/db apps/worker --exec rg -n -C5 'drizzle\(|new Pool|Pool\(|parseInputDatesAsUTC|pg-types|timestamp|DATABASE_URL' {} \; | head -240Repository: theopenco/llmgateway
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
echo "---- exact global-stats migrations ----"
rg -l -i 'global_model_stats|global_source_stats' packages/db/migrations | sort | while read -r f; do
echo "### $f"
rg -n -i -C5 'global_model_stats|global_source_stats' "$f"
done
echo "---- log table creation ----"
rg -l 'CREATE TABLE "log"' packages/db/migrations | while read -r f; do
echo "### $f"
rg -n -C8 'CREATE TABLE "log"|created_at.*timestamp' "$f"
done
echo "---- database client and timestamp options ----"
fd -i 'package.json|*.ts' packages/db apps/worker --exec rg -n -C5 'drizzle\(|new Pool|Pool\(|parseInputDatesAsUTC|pg-types|timestamp|DATABASE_URL' {} \; | head -240Repository: theopenco/llmgateway
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
echo "---- migration files containing table creation ----"
rg -l --glob '*.sql' 'CREATE TABLE "global_model_stats"|CREATE TABLE "global_source_stats"|CREATE TABLE "log"' packages/db/migrations | sort
echo "---- exact CREATE TABLE statements ----"
rg -n --glob '*.sql' -C12 'CREATE TABLE "global_model_stats"|CREATE TABLE "global_source_stats"|CREATE TABLE "log"' packages/db/migrations
echo "---- snapshot type records for the three columns ----"
python3 - <<'PY'
import json
from pathlib import Path
targets = {
("log", "created_at"),
("global_model_stats", "day_timestamp"),
("global_source_stats", "day_timestamp"),
}
for path in sorted(Path("packages/db/migrations/meta").glob("*_snapshot.json")):
try:
data = json.loads(path.read_text())
except Exception:
continue
for item in data.get("tables", {}).values():
if not isinstance(item, dict):
continue
table = item.get("name")
if table not in {t[0] for t in targets}:
continue
for name, col in item.get("columns", {}).items():
if (table, name) in targets:
print(path.name, table, name, col.get("dataType"), col.get("columnType"), col.get("type"))
PY
echo "---- client configuration and dependency versions ----"
rg -n -C6 --glob '*.ts' --glob 'package.json' --glob 'pnpm-lock.yaml' \
'drizzle\(|new Pool|Pool\(|parseInputDatesAsUTC|pg-types|node-postgres|DATABASE_URL' \
packages/db apps/worker package.json pnpm-lock.yamlRepository: theopenco/llmgateway
Length of output: 45016
🌐 Web query:
node-postgres timestamp without time zone parses local timezone Date parser
💡 Result:
By default, node-postgres parses PostgreSQL TIMESTAMP WITHOUT TIME ZONE columns as JavaScript Date objects based on the local time zone of the Node.js process [1][2]. The driver uses the postgres-date library to handle this conversion [3]. Because TIMESTAMP WITHOUT TIME ZONE lacks timezone information, the parser treats the input string as a local time when constructing the Date object [4][3]. This can lead to unexpected behavior if your application expects UTC or if your Node.js server and database server operate in different time zones [1][5]. To change this behavior, you can override the default type parser provided by the pg library [1][6]. Common approaches include: 1. Returning the raw string: If you want to avoid automatic Date conversion entirely, you can disable the parser [7][5]. const { types } = require('pg'); const TIMESTAMP_OID = 1114; types.setTypeParser(TIMESTAMP_OID, (timestamp) => timestamp); 2. Forcing UTC interpretation: If you need a Date object but want it interpreted as UTC, you can append a Z (UTC indicator) to the string before passing it to the Date constructor [8]. const { types } = require('pg'); const TIMESTAMP_OID = 1114; types.setTypeParser(TIMESTAMP_OID, (stringValue) => { return new Date(${stringValue.replace(' ', 'T')}Z); }); Note: If you are experiencing issues with time offsets, it is often recommended to use the TIMESTAMPTZ (timestamp with time zone) data type in PostgreSQL, which stores the value in UTC and avoids these ambiguity issues [1][2].
Citations:
- 1: https://node-postgres.com/features/types
- 2: https://github.com/brianc/node-postgres/blob/master/docs/pages/features/types.mdx
- 3: Date params conversion brianc/node-postgres#1172
- 4: Wrong timestamp to JavaScript date conversion brianc/node-postgres#993
- 5: Do not return DATE fields as Javascript Date brianc/node-pg-types#50
- 6: https://github.com/brianc/node-pg-types/
- 7: https://stackoverflow.com/questions/20712291/use-node-postgres-to-get-postgres-timestamp-without-timezone-in-utc
- 8: https://stackoverflow.com/questions/78725773/how-to-configure-javascript-pg-library-to-interpret-timestamps-without-timezone
🏁 Script executed:
#!/bin/bash
set -e
node - <<'JS'
const value = "2025-05-20 00:00:00";
for (const tz of ["UTC", "America/Los_Angeles", "Asia/Tokyo"]) {
const parsed = require("child_process")
.execFileSync(process.execPath, ["-e", `
const d = new Date(${JSON.stringify(value)});
const floored = new Date(Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate()));
console.log(JSON.stringify({
tz: process.env.TZ,
parsed: d.toISOString(),
floored: floored.toISOString()
}));
`], { env: { ...process.env, TZ: tz } })
.toString()
.trim();
console.log(parsed);
}
JSRepository: theopenco/llmgateway
Length of output: 439
Interpret the timestamp boundary as UTC. The three columns are timestamp without time zone values, and node-postgres parses them as local-time Date objects. Under TZ=Asia/Tokyo, floorToUTCDay can select the previous day. Convert the least(...) expression with AT TIME ZONE 'UTC', or parse text with an explicit Z; a bare text cast is insufficient.
🤖 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 `@apps/worker/src/scripts/rebuild-global-stats.ts` around lines 78 - 88, Update
the least(...) start-boundary query in the rebuild-global-stats flow to
interpret the timestamp without time zone values as UTC before node-postgres
converts them to Date objects. Apply AT TIME ZONE 'UTC' to the SQL expression
(or parse an explicitly Z-suffixed text value), then preserve the existing
floorToUTCDay handling and null behavior.
| async function main(): Promise<void> { | ||
| const commit = hasFlag("commit"); | ||
| const oldestFirst = hasFlag("oldest-first"); | ||
| const limit = parseFlag("limit") ? Number(parseFlag("limit")) : undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate all CLI flags before use. The script converts process.argv values into numbers and dates without validation. Malformed input then propagates as NaN or Invalid Date, which either silently rebuilds nothing, rebuilds everything, or throws RangeError during logging.
apps/worker/src/scripts/rebuild-global-stats.ts#L93-L93: parse--limitonce and reject values that are not positive integers; also change line 132 tolimit === undefined ? days : days.slice(0, limit)so--limit=0does not select every day.apps/worker/src/scripts/rebuild-global-stats.ts#L102-L118: reject--fromand--tovalues that produce anInvalid Datebefore the clamp and range checks run.
📍 Affects 1 file
apps/worker/src/scripts/rebuild-global-stats.ts#L93-L93(this comment)apps/worker/src/scripts/rebuild-global-stats.ts#L102-L118
🤖 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 `@apps/worker/src/scripts/rebuild-global-stats.ts` at line 93, Validate every
CLI date and numeric flag before using it: in the limit parsing around
apps/worker/src/scripts/rebuild-global-stats.ts lines 93-93, parse --limit once
and reject values that are not positive integers, then update the day selection
around line 132 to use the undefined check so a zero limit does not select all
days. In the --from/--to handling at lines 102-118, reject Invalid Date results
before clamp and range validation proceed.
| for (const day of planned) { | ||
| const completed = await recomputeDayFully(day); | ||
| if (!completed) { | ||
| console.log(`Stop requested — halted after ${done} day(s).`); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find where the stop flag used by isStopRequested is set.
rg -nP --type=ts -C5 '\b(isStopRequested|requestStop|stopRequested)\b'Repository: theopenco/llmgateway
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate file ---'
wc -l apps/worker/src/scripts/rebuild-global-stats.ts
sed -n '1,220p' apps/worker/src/scripts/rebuild-global-stats.ts
printf '%s\n' '--- stop-related definitions and usages ---'
rg -n -C4 'isStopRequested|requestStop|stopRequested|SIGINT|SIGTERM|shutdown|graceful' . \
-g '!node_modules' -g '!dist' -g '!build'Repository: theopenco/llmgateway
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
service_file="$(fd -t f 'global-stats-aggregator' . | head -n 1)"
printf 'service_file=%s\n' "$service_file"
wc -l "$service_file"
ast-grep outline "$service_file"
printf '%s\n' '--- recomputeDayFully and direct stop references ---'
rg -n -C12 'recomputeDayFully|isStopRequested|requestStop|stopRequested' "$service_file"
printf '%s\n' '--- all exact symbol references, excluding generated/dependency files ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' -g '!pnpm-lock.yaml' '\bisStopRequested\b|\brequestStop\b|\bstopRequested\b' .Repository: theopenco/llmgateway
Length of output: 486
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate service files ---'
fd -t f 'global-stats-aggregator' apps/worker
printf '%s\n' '--- implementation references ---'
rg -n -C16 'recomputeDayFully|isStopRequested|requestStop|stopRequested' apps/worker/src \
-g '*.ts' -g '!*.spec.ts'Repository: theopenco/llmgateway
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- shutdown state and signal registration ---'
cat -n apps/worker/src/shutdown.ts | sed -n '1,140p'
printf '%s\n' '--- process signal handlers in worker sources ---'
rg -n -C12 "process\.on\(['\"](SIGINT|SIGTERM)|requestStop\(" apps/worker/src \
-g '*.ts' -g '!*.spec.ts'
printf '%s\n' '--- worker entrypoint imports and startup/shutdown section ---'
rg -n -C10 'from "./shutdown\.js"|from "`@/shutdown`\.js"|process\.on|startWorker|main\(' apps/worker/src/worker.ts apps/worker/src/index.ts apps/worker/src/main.ts 2>/dev/null || trueRepository: theopenco/llmgateway
Length of output: 11347
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
script = Path("apps/worker/src/scripts/rebuild-global-stats.ts").read_text()
aggregator = Path("apps/worker/src/services/global-stats-aggregator.ts").read_text()
shutdown = Path("apps/worker/src/shutdown.ts").read_text()
worker_entry = Path("apps/worker/src/index.ts").read_text()
checks = {
"script imports recomputeDayFully": "recomputeDayFully" in script,
"aggregator imports isStopRequested": 'import { isStopRequested } from "`@/shutdown.js`";' in aggregator,
"recomputeDayFully checks isStopRequested": bool(
re.search(r"export async function recomputeDayFully[\s\S]*?isStopRequested\(\)", aggregator)
),
"shutdown initializes shouldStop=false": "let shouldStop = false;" in shutdown,
"standalone script has SIGINT handler": bool(
re.search(r"process\.on\(\s*[\"']SIGINT[\"']", script)
),
"standalone script has SIGTERM handler": bool(
re.search(r"process\.on\(\s*[\"']SIGTERM[\"']", script)
),
"worker entrypoint has SIGINT handler": bool(
re.search(r"process\.on\(\s*[\"']SIGINT[\"']", worker_entry)
),
"worker entrypoint has SIGTERM handler": bool(
re.search(r"process\.on\(\s*[\"']SIGTERM[\"']", worker_entry)
),
}
for name, result in checks.items():
print(f"{name}: {result}")
PYRepository: theopenco/llmgateway
Length of output: 505
Register signal handlers for the standalone script.
recomputeDayFully() checks isStopRequested(), but this script does not register SIGINT or SIGTERM handlers. A signal can terminate the process during a day rebuild and leave that day partially rebuilt.
🤖 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 `@apps/worker/src/scripts/rebuild-global-stats.ts` around lines 153 - 158,
Register SIGINT and SIGTERM handlers in the standalone script before the loop
that invokes recomputeDayFully, using the existing stop-request mechanism so
signals set the requested-stop state instead of terminating immediately.
Preserve the loop’s current halted-after-completion behavior and cleanup any
handlers when the script finishes.
|
Superseded: going with a plain truncate + watermark reset in production and letting the incremental walker rebuild, rather than adding a script for it. |
Why this is possible
The mode/kind migration (
1786136603_warm_omega_flight.sql) left a large slice ofglobal_model_stats/global_source_statswithused_modeandorg_kindset tounknown, so the admin Global Stats page can't make Credits + BYOK add up to All and shows almost no organization-kind attribution.The migration's note says recovering that means re-reading raw logs, which retention has pruned. That turns out to be wrong.
cleanupExpiredLogData(apps/worker/src/worker.ts:740) is an UPDATE, not a DELETE — after 30 days it nulls the verbose payload columns and setsdata_retention_cleaned_up:No production code path deletes
logrows at all — not the retention job, not account deletion. And not one column the aggregator reads is in that list:used_model,used_provider,used_mode,source,organization_id,created_at, every token column, every cost column,has_error,cached,streamed,unified_finish_reason.Confirmed against production: the earliest
logrow falls inside the firstglobal_model_statsday bucket, so there is no stats history predating the logs. A rebuild is lossless.That makes re-deriving from source strictly better than a redistribution backfill — it yields an exact
used_modeandorg_kindfor every row, with no proration and no scaling.Design
Each day goes through the aggregator's own
recomputeDayFully(), which already deletes that day's rows and re-aggregates its 24 hours. Reusing it means no new aggregation logic to keep in sync — the only production change is making that functionexported.global_aggregation_statewithout also raisingGLOBAL_STATS_INITIAL_LOOKBACK_DAYS(default 30) would silently rebuild only one month.--tois clamped accordingly.--from/--to.Usage
Dry run by default — reports the plan and how many rows are currently unattributed.
Verification
Driven end to end against a local stack seeded with real log rows in the exact shape the migration collapsed — 100 logs on one model and one UTC day, 60 credits/PAYG and 40 BYOK/DevPass, plus a bogus pre-existing
unknown/unknownaggregate row.After the rebuild:
Exact, not approximated. Also verified:
--commitrun produces identical rows.global_source_statsrebuilt with the same attribution, including a source value containing spaces and a slash.--toclamping — a future--tois clamped to today-2 with an explanatory message.--limit— restricts to the N newest days.apps/workersuite:global-stats-aggregator.spec.tspasses 5/5. Foursync-models.spec.tstests time out at 30s in my environment; they passed earlier in the same session at 5–11s each and are unaffected by this change (which adds anexportand a new file), so I've treated them as environment flake rather than a regression.pnpm formatandpnpm buildclean.Operational notes
GROUP BYscans overlog. The model query rideslog_created_at_used_model_used_provider_idx; the source query groups onsource, which isn't in that index. Run it off-peak.logactually says.Relationship to the other PRs
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements