feat: report disk capacity on crash telemetry events - #4382
Conversation
Adds last_known_disk_total_mb, last_known_disk_used_mb and last_known_disk_available_mb to app:crash_detected and renderer:crash_detected, so we can see whether crashes line up with a full disk. The performance monitor already saves a snapshot every 30s that the next launch attaches to both crash events, so this is one statfs call added to that snapshot. It measures the user data volume, not the apps folder. The same tick already reads and writes user-settings.json there, so the statfs adds no blocking the main thread did not already have; the apps folder is user-configurable and could be a network mount. For a default install they are the same volume. Used and available are both reported because they are not interchangeable: every platform holds some space back from ordinary writes, so total minus used overstates the room the user really had. Available is the one to threshold on. If the statfs fails the fields are omitted rather than reported as zero, and they are optional in the schema so older snapshots still parse. Measuring how much disk Dyad itself uses is out of scope, since that needs a recursive walk of every app directory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eddd8f6066
βΉοΈ About Codex in GitHub
Codex has been enabled to automatically 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 π.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
π Dyadbot Code Review SummaryVerdict: π€ NOT SURE - Potential issues Small, well-scoped telemetry addition. The core helper ( The Two things worth a reviewer's attention, neither blocking. I was not able to execute the test suite in this environment, so the notes below are from static reading of the diff and surrounding code. Issues Summary
π’ Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
Pins the three disk fields in the schemas round-trip test, which exists to check that the lastKnownPerformance shape survives the validation parse in writeSettings. Logs at debug when a reading is unavailable. The fields are dropped silently on a statfs failure, so without this a platform-wide breakage would make them vanish from telemetry with nothing to explain why. Asserts the disk fields are present in the e2e spec before comparing them, so a missing field reports itself instead of surfacing as an undefined comparison. Says plainly in the code comment why the user data volume is the one measured: it is the system volume, which is the disk we want a reading for, and the apps folder can sit on a different drive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
π Dyadbot Code Review SummaryVerdict: π€ NOT SURE - Potential issues Small, well-scoped telemetry addition: one Correctness looks sound: the fields are The one thing worth noting before this data gets analyzed is that the used/available distinction the PR is built on is a Unix property and does not hold uniformly on Windows. Issues Summary
π’ Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
wwwillchen
left a comment
There was a problem hiding this comment.
thanks! one minor comment, otherwise lgtm
getDiskUsageMB swallowed the error and returned null, so a failed statfs left nothing to diagnose it with. Logs at error level with the path, since a statfs that fails on the user data directory means something has gone badly wrong. Drops the debug line in the performance monitor that this replaces: it fired on the same condition and carried less detail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b48bcc5173
βΉοΈ About Codex in GitHub
Codex has been enabled to automatically 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 π.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
π Dyadbot Code Review SummaryVerdict: π€ NOT SURE - Potential issues Small, additive telemetry change: one
One MEDIUM issue, on the failure path. Nothing blocks merge. Note on confidence: the diff is complete (not truncated), but I could not execute the test suite in this environment, so the assessment is static analysis plus cross-referencing the surrounding code. Issues Summary
π’ Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
getDiskUsageMB runs on every 30s tick, and getSystemDebugInfo shows only the last 20 warn+ log lines, so a persistently failing statfs would fill that window in ten minutes and push out the crash warnings this telemetry exists to help read. The flag is deliberately not cleared on a later success: a volume that flaps would re-arm the log on every recovery, which is the case the guard is for. Both the once-only behaviour and the no-reset choice are pinned by tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
π Dyadbot Code Review SummaryVerdict: π€ NOT SURE - Potential issues Small, well-scoped, well-tested telemetry addition: one Things I checked and found correct:
Issues Summary
π’ Low Priority Notes (3 items)
Confidence note: the two unit tests that call Generated by Dyadbot persona-based code review |
The once-per-process guard hid more than it protected. getSystemDebugInfo returns the LAST 20 warn+ lines, so a single failure logged at startup is pushed out by any later warnings, and the disk problem is missing from exactly the window a bug report includes. A failure that repeats is the one thing always present in that view. A repeating error also carries information a single line cannot: whether the failure is persistent or a one-off, and whether it ever recovered. The other warnings are not lost either way, since the session debug bundle reads 5000 lines at every level. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
π Dyadbot Code Review SummaryVerdict: π€ NOT SURE - Potential issues The change is small, well-scoped, and correct in its core arithmetic. Two non-blocking concerns are below. Neither is a merge blocker. Issues Summary
π’ Low Priority Notes (3 items)
Generated by Dyadbot persona-based code review |
π Playwright Test Resultsβ Some tests failed
Summary: 898 passed, 1 failed, 6 flaky, 318 skipped Failed Testsπͺ Windows
|
Adds last_known_disk_total_mb, last_known_disk_used_mb and last_known_disk_available_mb to app:crash_detected and renderer:crash_detected, so we can see whether crashes line up with a full disk.
The performance monitor already saves a snapshot every 30s that the next launch attaches to both crash events, so this is one statfs call added to that snapshot.
It measures the user data volume, not the apps folder. The same tick already reads and writes user-settings.json there, so the statfs adds no blocking the main thread did not already have; the apps folder is user-configurable and could be a network mount. For a default install they are the same volume.
Used and available are both reported because they are not interchangeable: every platform holds some space back from ordinary writes, so total minus used overstates the room the user really had. Available is the one to threshold on.
If the statfs fails the fields are omitted rather than reported as zero, and they are optional in the schema so older snapshots still parse. Measuring how much disk Dyad itself uses is out of scope, since that needs a recursive walk of every app directory.