Fix pre-receive hook hangs and missing logs by flushing logs on signal and using CommandContext for git commands #4714
Conversation
…l and using CommandContext for git commands
pkg/gitparse/gitparse.go
Outdated
| return diffChan, err | ||
| } | ||
|
|
||
| // Set WaitDelay to give the command a grace period to finish before being killed |
There was a problem hiding this comment.
This comment doesn't quite match what I'm seeing in the docs:
The WaitDelay timer starts when either the associated Context is done or a call to Wait observes that the child process has exited, whichever occurs first. When the delay has elapsed, the command shuts down the child process and/or its I/O pipes.
That doesn't read like a grace period; it reads like an additional timeout.
An extra timeout looks appropriate, based on the way you've described the problem - but I'm concerned about hardcoding it in a function as generic as executeCommand. A caller might not realize that this function has a hard-coded five-second timeout (because Golang callers will probably assume that they can control any timeout using the passed-in context), and five seconds might not be the appropriate timeout for every command that a caller might use this function to invoke. Can we add the delay as a parameter instead of hard-coding it? (Or do we not know which command is hanging?)
There was a problem hiding this comment.
That sounds like a good change. I'll set the default to 5 seconds (unless you have a more appropriate time for a default)!
Replace hardcoded 5-second WaitDelay in executeCommand with a configurable field on the Parser struct. Add WithWaitDelay() option function to allow callers to customize the timeout when creating a Parser instance. Default is set to 5 seconds, open to changing if needed.
![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 and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
main.go
Outdated
| logger.Info("cleaned temporary artifacts") | ||
| } | ||
|
|
||
| // Flush logs with timeout to prevent hanging |
There was a problem hiding this comment.
I'm confused by this, because sync is already called: It's deferred on line 367 of this file. If that call is hanging, then the hang should be fixed there, not fixed by putting a timer on an os.Exit here. Or, that call should be removed entirely, so that this code is the only sync code. And if you do that, is there a way to pass the sync function through the overseer state, rather than relying on a package var? Mutable globals (like package vars) make the code more difficult to understand and maintain because it obscures entanglement.
There was a problem hiding this comment.
Looking at this, and with the advise of the robots, it seems that we'd meed a wrapper struct around overseer.State. Passing it as a parameter to Run still lets us get rid of the package level var; let me know if this works for you.
Currently, I have two syncs, one for a normal exit, where deferred sync runs, and another for signal-based exits, where the sync is explicit as deferred functions are bypassed by os.Exit().
| updateCfg := overseer.Config{ | ||
| Program: run, | ||
| Program: func(s overseer.State) { | ||
|
|
There was a problem hiding this comment.
unnecessary new line here
Description:
Fix missing log output on timeout, reduce pre-receive hook hangs
When TruffleHog times out in pre-receive hooks, it fails to output
diagnostic logs and can hang indefinitely when git commands block.
Changes:
logs are visible when process is killed
to ensure processes are killed when context is cancelled
This ensures diagnostic output is captured and prevents indefinite
hangs when git operations block in pre-receive hook environments.
Checklist:
make test-community)?make lintthis requires golangci-lint)?