fix(daemon): avoid loading full gateway logs during diagnostics#99407
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 5:09 AM ET / 09:09 UTC. Summary PR surface: Source +52, Tests +97. Total +149 across 4 files. Reproducibility: yes. at source level: current main and v2026.6.11 fully read resolved gateway logs before selecting one diagnostic line, so a large log can force unnecessary buffering. I did not run tests because this review is read-only. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Land the bounded shared diagnostics reader after ordinary maintainer review if the 256 KiB recent-tail window is the desired operator diagnostic behavior. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main and v2026.6.11 fully read resolved gateway logs before selecting one diagnostic line, so a large log can force unnecessary buffering. I did not run tests because this review is read-only. Is this the best way to solve the issue? Yes. Putting the bound in the shared daemon diagnostics helper is narrower than per-caller guards, and reusing it from status-all avoids a second full-read implementation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1fef99962edf. Label changesLabel justifications:
Evidence reviewedPR surface: Source +52, Tests +97. Total +149 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
9636319 to
f17709c
Compare
4c1bd52 to
5f93789
Compare
|
Land-ready verification for exact head
No screenshot attached: the exact before/after artifact is terminal report size/content. Write-side log rotation remains separate. Thanks @sunlit-deng for the fix. |
|
Merged via squash.
|
…claw#99407) * fix(daemon): bound gateway diagnostic log reads * fix(daemon): share bounded gateway log tails * fix(daemon): complete bounded tail reads --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
What Problem This Solves
Fixes an issue where gateway diagnostics could load the full stdout/stderr log files just to surface one recent error line. On hosts with large gateway logs,
status, doctor, or onboarding failure diagnostics could allocate far more memory than needed while still reporting stale errors from the top of an old log.Related: #79422
Why This Change Was Made
Gateway diagnostics now read only the last 256 KiB of each resolved log file before scanning for known error patterns or the last non-empty line. This is a read-side bound only: it does not change launchd/systemd logging, log retention, or the error-pattern set from other diagnostics PRs.
User Impact
Operators still get the most recent actionable gateway diagnostic line, but diagnostic commands no longer need to buffer an entire accumulated gateway log.
Evidence
node scripts/test-projects.mjs src/daemon/diagnostics.test.ts:pnpm exec oxlint src/daemon/diagnostics.ts src/daemon/diagnostics.test.ts: exit 0.readLastGatewayErrorLinehelper with a 440066-byte gateway log whose stale matching error is at the start and current line is in the tail:Overlap checked before opening: #79892 handles launchd write-side log retention, #90828 changes macOS stderr routing, and #95902 adds ENOSPC error matching. Those PRs do not bound the diagnostics read path.
AI-assisted: built with Codex