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.
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(100 * time.Millisecond): |
There was a problem hiding this comment.
Magic number for log flush timeout
Low Severity
The 100 * time.Millisecond log flush timeout is a magic number used inline. In contrast, the related PR changes in gitparse.go define similar timeout values as named constants with descriptive comments (e.g., defaultWaitDelay = 5 * time.Second). For consistency and maintainability, this timeout value would benefit from being a named constant with documentation explaining why 100ms was chosen.
| results = cli.Flag("results", "Specifies which type(s) of results to output: verified (confirmed valid by API), unknown (verification failed due to error), unverified (detected but not verified), filtered_unverified (unverified but would have been filtered out). Defaults to verified,unverified,unknown.").String() | ||
| noColor = cli.Flag("no-color", "Disable colorized output").Bool() | ||
| noColour = cli.Flag("no-colour", "Alias for --no-color").Hidden().Bool() | ||
| logSync func() error //Package-level variable for sync function |
There was a problem hiding this comment.
Burying this var amongst all the cli.Flag vars that surround it is going to make it far more difficult to find than it needs to be. Can you put it either at the beginning or the end? I think it would be ok to take it out of the grouped declaration entirely (declaring it using its own var keyword) if gofmt lets you do that.
(That being said, I don't think you need to use a package var for this - read my other comments for details.)
| 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.
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)?