-
Notifications
You must be signed in to change notification settings - Fork 182
desktop: report whether the previous run ended in a crash #1461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
hisco
wants to merge
1
commit into
main
Choose a base branch
from
rad-304-desktop-crash-marker
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| //go:build !windows | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "errors" | ||
| "os" | ||
|
|
||
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| // tryLockFile takes an exclusive lock without blocking. The kernel drops it | ||
| // when the process exits, however it exits, which is what lets a later run | ||
| // tell a crashed session from a running one. | ||
| func tryLockFile(f *os.File) (bool, error) { | ||
| err := unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB) | ||
| switch { | ||
| case err == nil: | ||
| return true, nil | ||
| case errors.Is(err, unix.EWOULDBLOCK): | ||
| return false, nil | ||
| default: | ||
| return false, err | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "errors" | ||
| "os" | ||
|
|
||
| "golang.org/x/sys/windows" | ||
| ) | ||
|
|
||
| // tryLockFile takes an exclusive lock without blocking. Windows releases it | ||
| // when the handle closes, including on process death, which is what lets a | ||
| // later run tell a crashed session from a running one. | ||
| func tryLockFile(f *os.File) (bool, error) { | ||
| var overlapped windows.Overlapped | ||
| err := windows.LockFileEx( | ||
| windows.Handle(f.Fd()), | ||
| windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, | ||
| 0, 1, 0, &overlapped, | ||
| ) | ||
| switch { | ||
| case err == nil: | ||
| return true, nil | ||
| case errors.Is(err, windows.ERROR_LOCK_VIOLATION), errors.Is(err, windows.ERROR_IO_PENDING): | ||
| return false, nil | ||
| default: | ||
| return false, err | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "log" | ||
| "os" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| ) | ||
|
|
||
| // The desktop app cannot log its own segfault, so a bug report has no way to | ||
| // say whether the last run crashed or the user simply 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 whole lifetime. The lock is what carries the signal, not the file: | ||
| // the kernel releases it when the process dies, however it dies, so a marker | ||
| // we can lock belonged to a run that is gone. A marker we cannot lock belongs | ||
| // to an instance that is still alive. | ||
| // | ||
| // Liveness is deliberately not inferred from the PID. PIDs are reused, and a | ||
| // recycled one would make a crashed run look like a running second window — | ||
| // suppressing a real crash and reporting a window that does not exist. | ||
| // | ||
| // One file per PID rather than a single shared one: a second window must never | ||
| // adopt or delete the first one's marker, or quitting either would erase the | ||
| // other's crash evidence. | ||
| // | ||
| // 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 — a | ||
| // flat directory would let one machine report a session running happily | ||
| // elsewhere as a crash. | ||
| func sessionDir() string { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return filepath.Join(home, ".radar", "desktop-sessions", hostSlug()) | ||
| } | ||
|
|
||
| func hostSlug() string { | ||
| name, err := os.Hostname() | ||
| if err != nil || name == "" { | ||
| return "unknown-host" | ||
| } | ||
| // Keep it a single safe path element regardless of what the OS reports. | ||
| name = strings.Map(func(r rune) rune { | ||
| switch { | ||
| case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_', r == '.': | ||
| return r | ||
| default: | ||
| return '-' | ||
| } | ||
| }, name) | ||
| if name = strings.Trim(name, "."); name == "" { | ||
| return "unknown-host" | ||
| } | ||
| return name | ||
| } | ||
|
|
||
| // runningMarker is written under the lock once a run is fully recorded. A | ||
| // marker without it is either mid-creation or already released, and neither is | ||
| // a crash — the file existing is not enough, because it is briefly visible and | ||
| // unlocked at both ends of a run. | ||
| const runningMarker = "running" | ||
|
|
||
| // held keeps this run's marker open. Closing the file would drop the lock and | ||
| // advertise the process as gone while it is still running. | ||
| // | ||
| // Guarded because release is reachable from two goroutines: the Wails shutdown | ||
| // callback, and the self-update relaunch. Closing the window while a relaunch | ||
| // is pending runs both. | ||
| var ( | ||
| sessionMu sync.Mutex | ||
| held *os.File | ||
| ) | ||
|
|
||
| func markSessionStart() { claimSession(sessionDir()) } | ||
| func markSessionEnd() { releaseSession(sessionDir()) } | ||
|
|
||
| // claimSession reports any run that ended without cleaning up, then records | ||
| // this one. It must be called only once the process is committed to running: | ||
| // claiming before the startup checks would leave a marker behind on every | ||
| // os.Exit and report the next launch as a crash that never happened. | ||
| func claimSession(dir string) { | ||
| sessionMu.Lock() | ||
| defer sessionMu.Unlock() | ||
|
|
||
| if dir == "" || held != nil { | ||
| return | ||
| } | ||
|
|
||
| // Scan before claiming, so this run's own marker is never mistaken for an | ||
| // abandoned one. | ||
| reportAbandonedSessions(dir) | ||
|
|
||
| if err := os.MkdirAll(dir, 0o700); err != nil { | ||
| log.Printf("[desktop] could not record session marker: %v", err) | ||
| return | ||
| } | ||
|
|
||
| path := sessionFile(dir, os.Getpid()) | ||
| file, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) | ||
| if err != nil { | ||
| log.Printf("[desktop] could not record session marker: %v", err) | ||
| return | ||
| } | ||
| locked, err := tryLockFile(file) | ||
| if err != nil || !locked { | ||
| // Without the lock the marker would claim this run had already ended. | ||
| if err != nil { | ||
| log.Printf("[desktop] could not lock session marker: %v", err) | ||
| } | ||
| file.Close() | ||
|
|
||
| _ = os.Remove(path) | ||
| return | ||
| } | ||
|
|
||
| // 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() | ||
|
|
||
| _ = os.Remove(path) | ||
| return | ||
| } | ||
| held = file | ||
| } | ||
|
|
||
| // releaseSession drops this run's marker so a deliberate exit is not reported | ||
| // as a crash. Other instances' markers are left alone. | ||
| func releaseSession(dir string) { | ||
| sessionMu.Lock() | ||
| defer sessionMu.Unlock() | ||
|
|
||
| if dir == "" || held == nil { | ||
| return | ||
| } | ||
| // Clear the marker while the lock is still held. Unlinking first would | ||
| // leave the path briefly present and lockable, and a launch landing in | ||
| // that gap would report this deliberate quit as a crash. | ||
| if err := held.Truncate(0); err != nil { | ||
| log.Printf("[desktop] could not clear session marker: %v", err) | ||
| } | ||
| held.Close() // releases the lock | ||
|
|
||
| held = nil | ||
| if err := os.Remove(sessionFile(dir, os.Getpid())); err != nil && !os.IsNotExist(err) { | ||
| log.Printf("[desktop] could not clear session marker: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // reportAbandonedSessions logs every marker whose owner is gone and clears it, | ||
| // so one crash is reported once rather than on every launch afterwards. | ||
| func reportAbandonedSessions(dir string) { | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| for _, entry := range entries { | ||
| pid, err := strconv.Atoi(entry.Name()) | ||
| if err != nil || pid <= 0 { | ||
| continue | ||
| } | ||
|
|
||
| path := filepath.Join(dir, entry.Name()) | ||
| file, err := os.OpenFile(path, os.O_RDWR, 0o600) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| locked, err := tryLockFile(file) | ||
| 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() | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| // Lockable and still marked running: the owner died without clearing | ||
| // it. Anything else is a released or half-written marker, which is | ||
| // swept away without a claim in either direction. | ||
| if markedRunning(file) { | ||
| log.Printf("[desktop] previous run (pid %d) did not exit cleanly — it crashed or was force-quit", pid) | ||
| } | ||
| file.Close() | ||
|
|
||
| if err := os.Remove(path); err != nil && !os.IsNotExist(err) { | ||
| log.Printf("[desktop] could not clear stale session marker: %v", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func sessionFile(dir string, pid int) string { | ||
| return filepath.Join(dir, strconv.Itoa(pid)) | ||
| } | ||
|
|
||
| // markedRunning reports whether a marker was fully recorded by a run that then | ||
| // never released it. | ||
| func markedRunning(f *os.File) bool { | ||
| buf := make([]byte, len(runningMarker)) | ||
| n, err := f.ReadAt(buf, 0) | ||
| if err != nil && n != len(runningMarker) { | ||
| return false | ||
| } | ||
| return string(buf[:n]) == runningMarker | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean quit reported as crash
High Severity
markSessionEndruns only afterapp.Shutdown, butsrv.Stopcloses the HTTP listener and the existing server goroutine treats that as a fatalStartWithReadyerror and callsos.Exit(1). That exit wins the race with marker release, so a normal window close leaves arunningfile and the next launch logs a crash.Additional Locations (1)
cmd/desktop/main.go#L203-L209Reviewed by Cursor Bugbot for commit 34bfa49. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
srv.Stop()closes the listener,http.Servereturnsnet.ErrClosed— which is nothttp.ErrServerClosed, so it is not filtered — and the server goroutine callsos.Exit(1). Every clean quit would race that exit and, often enough, leave arunningmarker 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.