feat: add SIGQUIT/SIGTRAP handler for the CLI #5665
Conversation
cmd/coder/main.go
Outdated
|
|
||
| // Write to a well-known file. | ||
| dir, err := os.UserHomeDir() | ||
| if err != nil { |
There was a problem hiding this comment.
Why write to home? I'd rather just have these files in my temp dir.
There was a problem hiding this comment.
On darwin the temp dir is a very long random path (not in /tmp) so it's difficult to find the file afterwards, especially if you can't read stderr to see the filename (which is the case a lot of the time).
It seems weird to have different behavior on different platforms, and this is a rare debug step so I think this is fine.
There was a problem hiding this comment.
This handler is now only applied to server and agent, and exits on SIGQUIT again
There was a problem hiding this comment.
It's unclear to me why we want to do this and what problem we're addressing. Especially the part where we continue running after SIGQUIT, that seems like a feature we wouldn't want in the coder binary, perhaps only in server and agent.
At least as a user I would want the process to exit on SIGQUIT.
Have we evaluated other options, like setting export GOTRACEBACK=crash before running the command? This will both output to stderr and produce a core file.
One thing we lose by only outputting runtime.Stack is the register values that are usually produced when core is dumped. It seems this signal handling also interferes with GOTRACEBACK=crash as in it's no longer possible to get a proper core dump via SIGQUIT.
cmd/coder/main.go
Outdated
| if err != nil { | ||
| dir = os.TempDir() | ||
| } | ||
| fpath := filepath.Join(dir, fmt.Sprintf("coder-agent-%s.dump", time.Now().Format("2006-01-02T15:04:05.000Z"))) |
There was a problem hiding this comment.
I think these files are usually written to the current working directory, is there a reason we don't want to do that here?
There was a problem hiding this comment.
These files need to be in a well known location otherwise they're difficult to find when people are trying to send us debug logs. Since coder agent (which this is intended for) is a daemon, writing the working directory will mean the user will have to look in multiple places for the dump.
There was a problem hiding this comment.
I can understand the use-case for workspaces, I think we have more flexibility in dumping into home there. Should we at least move it to the agent cmd in that case?
The loss register values and supporting GOTRACEBACK is unfortunate, though. We could at least enable GOTRACEBACK by conditionally starting this routine only if it's unset.
There was a problem hiding this comment.
I disabled listening for SIGQUIT if GOTRACEBACK=crash, is that good?
|
I will change the behavior to listen for both |
Could we make it the other way around? |
|
err i meant SIGQUIT oops |
The default SIGQUIT handler in Golang dumps all goroutine stacktraces to stderr and then exits the program.
This changes the behavior so it dumps all goroutine stacktraces to stderr and a file (with the date) to $HOME, then exits. For the SIGTRAP handler, it does the same thing but doesn't exit.