Skip to content

Fix pre-receive hook hangs and missing logs by flushing logs on signal and using CommandContext for git commands #4714

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.

Sign up for GitHub

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

Open
jordanTunstill wants to merge 5 commits into
base: main
Choose a base branch
from
+60 −6
Open

Fix pre-receive hook hangs and missing logs by flushing logs on signal and using CommandContext for git commands #4714

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f12e1c9
Fix pre-receive hook hangs and missing logs by flushing logs on signa…
jordanTunstill Jan 27, 2026
296302c
Make git command WaitDelay configurable
jordanTunstill Jan 28, 2026
3af2ec5
Merge branch 'main' into AddLogFlushing
jordanTunstill Jan 28, 2026
f8221bd
added a test for wait delay
jordanTunstill Jan 28, 2026
08e18b4
Merge branch 'main' into AddLogFlushing
jordanTunstill Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"strings"
"sync"
"syscall"

"time"
"github.com/alecthomas/kingpin/v2"
"github.com/fatih/color"
"github.com/felixge/fgprof"
Expand Down Expand Up @@ -61,6 +61,7 @@ var (
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
Copy link
Contributor

@rosecodym rosecodym Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


allowVerificationOverlap = cli.Flag("allow-verification-overlap", "Allow verification of similar credentials across detectors").Bool()
filterUnverified = cli.Flag("filter-unverified", "Only output first unverified result per chunk per detector if there are more than one results.").Bool()
Expand Down Expand Up @@ -354,6 +355,7 @@ func main() {
logFormat = log.WithJSONSink
}
logger, sync := log.New("trufflehog", logFormat(os.Stderr, log.WithGlobalRedaction()))
logSync = sync //
// make it the default logger for contexts
context.SetDefaultLogger(logger)

Expand Down Expand Up @@ -413,6 +415,21 @@ func run(state overseer.State) {
} else {
logger.Info("cleaned temporary artifacts")
}

// Flush logs with timeout to prevent hanging
Copy link
Contributor

@rosecodym rosecodym Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if logSync != nil {
done := make(chan struct{})
go func() {
_ = logSync()
close(done)
}()

select {
case <-done:
case <-time.After(100 * time.Millisecond):
Copy link

@cursor cursor bot Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

logger.Info("Log flush timed out, exiting")
}
}
os.Exit(0)
}()

Expand Down
9 changes: 7 additions & 2 deletions pkg/gitparse/gitparse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func (c *Parser) RepoPath(
args = append(args, "--", ".", ":(exclude)"+glob)
}

cmd := exec.Command("git", args...)
cmd := exec.CommandContext(ctx, "git", args...)
absPath, err := filepath.Abs(source)
if err == nil {
if !isBare {
Expand Down Expand Up @@ -281,7 +281,7 @@ func (c *Parser) Staged(ctx context.Context, source string) (chan *Diff, error)
// Provide the --cached flag to diff to get the diff of the staged changes.
args := []string{"-C", source, "diff", "-p", "--cached", "--full-history", "--diff-filter=AM", "--date=iso-strict"}

cmd := exec.Command("git", args...)
cmd := exec.CommandContext(ctx, "git", args...)

absPath, err := filepath.Abs(source)
if err == nil {
Expand All @@ -293,6 +293,8 @@ func (c *Parser) Staged(ctx context.Context, source string) (chan *Diff, error)

// executeCommand runs an exec.Cmd, reads stdout and stderr, and waits for the Cmd to complete.
func (c *Parser) executeCommand(ctx context.Context, cmd *exec.Cmd, isStaged bool) (chan *Diff, error) {
const waitDelay = 5 * time.Second // Give the command a chance to finish before the timeout

diffChan := make(chan *Diff, 64)

stdOut, err := cmd.StdoutPipe()
Expand All @@ -304,6 +306,9 @@ func (c *Parser) executeCommand(ctx context.Context, cmd *exec.Cmd, isStaged boo
return diffChan, err
}

// Set WaitDelay to give the command a grace period to finish before being killed
Copy link
Contributor

@rosecodym rosecodym Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

@jordanTunstill jordanTunstill Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good change. I'll set the default to 5 seconds (unless you have a more appropriate time for a default)!

cmd.WaitDelay = waitDelay

err = cmd.Start()
if err != nil {
return diffChan, err
Expand Down
Loading