desktop: report whether the previous run ended in a crash - #1461
Conversation
The app cannot log its own segfault, so a bug report has no way to say whether the last session died or was quit — the journal shows a process that stopped either way. Each run creates a file named after its PID and holds an exclusive lock on it for its lifetime. The lock carries the signal, not the file: the kernel releases it when the process dies, however it dies. A marker that can be locked belonged to a run that is gone; one that cannot belongs to an instance still alive. PID liveness is deliberately not used, because PIDs are reused and a recycled one would make a crashed run look like a running second window. The file must also be stamped as running, under the lock, before it counts. A marker is briefly present and unlocked at both ends of a run — after creation before the lock is taken, and after the lock drops before the unlink — and a launch landing in either gap would otherwise report a deliberate quit as a crash. Claiming happens once the process is committed to starting the window, so the startup checks that end in os.Exit cannot strand a marker. Releasing happens after teardown returns, so a shutdown that hangs or is killed still reads as unclean. Self-update relaunch exits via os.Exit and skips the shutdown hook entirely, so the updater gained a pre-exit callback; that hook and the session handle are both guarded, since relaunch runs on its own goroutine and can overlap a window close. Markers are scoped per host: a home directory can be shared across machines, and a lock taken on one host says nothing about a process on another. Claude-Session: https://claude.ai/code/session_01KEyZgyPtfpXdbWqPPsTXZe
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 34bfa49. Configure here.
| app.Shutdown(a.srv) | ||
| // Last, so a teardown that hangs or is killed still reads as an unclean | ||
| // exit — that is precisely the failure worth knowing about. | ||
| markSessionEnd() |
There was a problem hiding this comment.
Clean quit reported as crash
High Severity
markSessionEnd runs only after app.Shutdown, but srv.Stop closes the HTTP listener and the existing server goroutine treats that as a fatal StartWithReady error and calls os.Exit(1). That exit wins the race with marker release, so a normal window close leaves a running file and the next launch logs a crash.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 34bfa49. Configure here.
There was a problem hiding this comment.
Confirmed, and it reproduces in a real log rather than just in theory. From the journal a user attached to #1360, on a normal window close:
14:31:08 Desktop app shutting down...
14:31:08 Shutting down...
14:31:08 Server error: accept tcp 127.0.0.1:41947: use of closed network connection
srv.Stop() closes the listener, http.Serve returns net.ErrClosed — which is not http.ErrServerClosed, so it is not filtered — and the server goroutine calls os.Exit(1). Every clean quit would race that exit and, often enough, leave a running marker behind and report a phantom crash on the next launch. That is the worst failure this feature can have, on the most common action a user takes.
Notably this was introduced by the previous fix, which moved the release after teardown so that a hung shutdown would still read as unclean. That traded a rare false negative for a frequent false positive.
Marking this PR draft rather than patching again. Full reasoning in the PR comment.
Separately, that log line is a pre-existing bug worth its own fix: radar-desktop exits with status 1 on every normal window close, and prints an alarming "Server error" in the process. It shows up in user bug reports today and looks like a fault when nothing is wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err != nil { | ||
| log.Printf("[desktop] could not lock session marker: %v", err) | ||
| } | ||
| file.Close() |
| // Only now, holding the lock, does the marker mean "a run is in progress". | ||
| if _, err := file.WriteString(runningMarker); err != nil { | ||
| log.Printf("[desktop] could not record session marker: %v", err) | ||
| file.Close() |
| if err := held.Truncate(0); err != nil { | ||
| log.Printf("[desktop] could not clear session marker: %v", err) | ||
| } | ||
| held.Close() // releases the lock |
| if err != nil || !locked { | ||
| // Held by a live instance, or the filesystem cannot lock. Saying | ||
| // nothing beats guessing at a crash that may not have happened. | ||
| file.Close() |
| if markedRunning(file) { | ||
| log.Printf("[desktop] previous run (pid %d) did not exit cleanly — it crashed or was force-quit", pid) | ||
| } | ||
| file.Close() |
|
Marking this draft. Recording why, so the next person does not rediscover it. The change is roughly a hundred lines whose entire output is one log line. It has now been through five review rounds, which between them found seven correctness bugs — every one in the same question, "did the previous run die", and every one producing a wrong signal rather than a crash or a hang:
The last one is the argument. A fix applied to remove a rare false negative introduced a frequent false positive, on the most common action a user takes, and it took another review round to catch. That is not a signal anyone should trust during triage. The value it was meant to add is also smaller than it first looked. In the report that motivated it, the journal already contained full Go stack traces with The webview library line that shipped alongside this in #1459 is the part that actually saved a round-trip with a reporter, and it drew no findings in any round. Leaving this open as a draft rather than closing it, because the underlying need is real. Anything that revives it should first fix the bug in (7) on its own merits: radar-desktop exits with status 1 on every normal window close, and prints |
2 participants
Why
The desktop app cannot log its own segfault. A bug report therefore has no way to say whether the last session died or the user simply quit — the journal shows a process that stopped, either way. Working that out for #1360 meant reading Go stack traces out of a 10MB journal export.
What
Each run creates a file named after its PID under
~/.radar/desktop-sessions/<host>/and holds an exclusive lock on it for its lifetime. On the next launch, a marker that can be locked belonged to a run that is gone:Passive. No behaviour change, no UI, nothing to opt into.
Why it is shaped this way
Nearly every design decision here is a correction of something that produced a wrong signal in review. Recording them because they are the reason the code is not simpler:
os.Exit, reporting a phantom crash on the next launch.Relaunchends inos.Exit(0)and skips the Wails shutdown hook entirely, so a routine self-update would have reported a crash every time.Relaunchruns on its own goroutine and the HTTP server accepts requests before registration, so closing the window mid-relaunch reaches both paths.Honest assessment
This is roughly a hundred lines whose entire output is one log line, and it has now been through five review rounds that found seven correctness bugs between them — every one in the question "is the previous run dead", which is harder than it looks.
It is worth knowing whether that trade is one this codebase wants. The alternative is that a crash report keeps arriving without the answer, and someone reads stack traces by hand. Declining this is a reasonable call.
Testing
19 tests: the unclean exit, reported once and not forever; a live instance ignored and its marker preserved; a crash under a reused PID still reported; both unlocked-marker windows; the stamp written under the lock; concurrent claim and release; per-host scoping; clean shutdown leaving nothing; malformed entries; a missing home directory; directory permissions. All green under
-raceon macOS and Linux. Builds clean for linux, windows and darwin.The limit worth stating: these drive the helpers directly. Nobody has segfaulted a real Wails build and watched the message appear on the next launch, and no test covers a filesystem where locking misbehaves — on one that cannot lock, the code stays silent rather than guessing.
https://claude.ai/code/session_01KEyZgyPtfpXdbWqPPsTXZe
Note
Low Risk
Local diagnostic logging only under
~/.radar; no auth or cluster paths touched, with mutex-guarded release and conservative behavior when locking fails.Overview
Adds passive crash detection for the desktop app: on launch it may log that the previous session did not exit cleanly, with no UI or behavior change beyond that line.
Each run creates a PID-named marker under
~/.radar/desktop-sessions/<host>/, holds an exclusive file lock for the process lifetime, and writesrunningonly while locked. The next launch treats a lockable marker still stampedrunningas an unclean exit (lock release on death, not PID liveness). Claim runs only after startup config checks pass; release runs last in Wails shutdown (after server teardown) and via a newupdater.OnBeforeExithook invoked fromrunBeforeExit()beforeos.Exiton self-update relaunch (darwin/linux/windows), so updates and early exits do not false-positive.Platform
tryLockFilehelpers useflock(Unix) andLockFileEx(Windows). Extensive tests cover one-shot reporting, live second instances, reused PIDs, release races, and host-scoped dirs.Reviewed by Cursor Bugbot for commit 34bfa49. Bugbot is set up for automated code reviews on this repo. Configure here.