fix(storage): bound the cold Codex log inspection and surface when it is skipped (#2605) - #2627
Conversation
Choose option (a): skip synchronous row aggregates once logs_2.sqlite exceeds 64 MiB, retaining file, schema, and capability inspection. The threshold matches the reporter's measured mitigation: a ~1 GB GROUP BY level took 17.3s, while bounded /api/storage returned in 628ms. A Worker (option b) would make both management endpoints asynchronous and add startup, admission, teardown, and Windows thread-exit cost to a management-only inspector; storage Workers are already serialized specifically around Windows teardown. Bounded queries (option c) cannot make count(*), GROUP BY, or sum() exact with LIMIT, and bun:sqlite provides no interruptible async statement on this request path. Expose metricsSkipped with the threshold so omitted aggregates are distinct from genuine zero values. Keep full metrics for small databases and cover both shapes with a falsified regression test.
… skipped Carries the size gate for #2605 and adds the GUI half. The server side skips the row aggregates above 64 MiB, which is what keeps a cold inspection off the proxy thread - the reporter measured GROUP BY level at 17.3s on a ~1 GB database, and the source already admitted the gap in its own comment. The GUI half was missing: with metrics null the row block simply disappeared, so a 1 GB database rendered as one with no rows. That is the exact confusion the server's null-vs-zero distinction exists to prevent, and it is most misleading precisely where the database is largest. The row now states the reason and the threshold, and the file sizes still render because only the aggregates were skipped, not the inspection. String added to all nine locales. Rendered in a real browser against the built stylesheet, not just asserted. Falsified: disabling the skipped-row branch reddens the new render test while the under-threshold case stays green.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe inspection now skips synchronous aggregate metrics for databases larger than 64 MiB and reports the reason and threshold. The storage workspace displays localized skip information, while backend and GUI tests cover large and normal databases. ChangesLog metrics guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
![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: a6a7f31f77
ℹ️ 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".
| ...common, | ||
| schema, | ||
| metrics: readMetrics(db, columns), | ||
| metrics: metricsSkipped === null ? readMetrics(db, columns) : null, |
There was a problem hiding this comment.
For every compatible database above 64 MiB, this sets the entire metrics object to null, even though page_size and freelist_count are cheap PRAGMAs rather than unbounded row aggregates. In the inspected Storage dashboard, reclaimAvailable requires metrics.reclaimableBytes > 0, so the Compact section now disappears for every large database—including one with substantial reclaimable space, which is precisely where compaction is most useful. Preserve the freelist metadata separately while skipping only the expensive row queries, or otherwise keep the action available.
Useful? React with 👍 / 👎.
| metrics: metricsSkipped === null ? readMetrics(db, columns) : null, | ||
| metricsSkipped, |
There was a problem hiding this comment.
When a database crosses this threshold, formatCodexLogGuardDoctor only prints its metrics line when report.metrics is present and never examines metricsSkipped. Consequently, ocx doctor silently loses the row, TRACE, and reclaimable-space diagnostics without explaining that the scan was deliberately skipped—the same ambiguity this change fixes in the dashboard. Add a doctor-formatting branch for metricsSkipped and focused coverage for the large-database report.
Useful? React with 👍 / 👎.
| inspectionOnly: 'Inspection only', | ||
| externalSqliteHome: 'External SQLite storage', | ||
| inspectionUnavailable: "Diagnostic log inspection is unavailable.", | ||
| metricsSkippedLarge: "Row metrics skipped: the database is above {threshold}, and scanning it would stall the proxy.", |
There was a problem hiding this comment.
This new visible string is added to the bespoke log-guard-labels.ts map rather than to src/i18n/en.ts and every locale module, and the component renders it through logGuardLabel instead of t(). That bypasses the repository's canonical TKey typing and automatic locale discovery, allowing this copy to drift outside the normal i18n validation path; add a standard locale key and render it through t("key").
AGENTS.md reference: gui/AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #2605. The first Storage inspection runs
count(*), twoGROUP BYqueries andsum(estimated_bytes)synchronously on the proxy thread. The reporter measuredGROUP BY levelat ~17.3s on a ~1 GBlogs_2.sqlite; the source documents a 49s case. Both storage endpoints call the inspection inline, so routing and/healthzstall behind it. Memoization does not help — an active Codex process changes the WAL and invalidates the entry, which the existing comment says outright: "This bounds the repeat cost, not the first one."Databases above 64 MiB now skip the row aggregates and keep file sizes, schema validation and capabilities.
metrics: nullplusmetricsSkipped: { reason, thresholdBytes }makes "not scanned" distinguishable from "genuinely zero".A Worker was considered and rejected: it turns both endpoints async and adds Worker admission/teardown cost on Windows, which is where this was reported. Query limiting was rejected because it cannot preserve an exact
count, grouping, or sum — a wrong number is worse than an absent one.The GUI half, added on review
The server change alone had a real gap: with
metricsnull the row block simply disappeared, so a 1 GB database rendered as one with no rows. That is exactly the confusion the null-vs-zero distinction exists to prevent, and it is most misleading precisely where the database is largest.The row now states the reason and the threshold. File sizes still render, because only the aggregates were skipped — the inspection itself succeeded.
Rendered in a real browser against the built stylesheet rather than trusted to a passing assertion:
String added to all nine locales.
Verification
Falsified both hunks: removing the server size gate makes the skipped-path test receive full metrics; disabling the GUI branch reddens the render test while the under-threshold case stays green.
One thing deliberately not done: a real-server test asserting
/healthzstays responsive during a cold inspection. A sparse large fixture's aggregate path completes too fast to reliably overlap the request, so the test would be timing-dependent and vacuous — it would pass whether or not the fix existed. The deterministic regression proves large databases never enterreadMetricsat all, which removes the known blocking operation rather than measuring around it. Saying this rather than shipping a green test that proves nothing.Checklist
devSummary by CodeRabbit
New Features
Bug Fixes