perf(cmd/ssh): add timeout for gpg-agent forwarding and track duration #205
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Pull request overview
This PR adds timeout support and duration tracking to GPG agent forwarding operations to prevent hangs and gather performance metrics. The changes improve observability by adding structured logging with timing information throughout the GPG setup workflow.
- Adds a new
RunWithTimeoutfunction to the SSH helper that enforces a timeout and sends SIGKILL on timeout vs SIGINT on context cancellation - Implements 5-second timeouts for GPG export operations (
GetHostPubKeyandGetHostOwnerTrust) with duration tracking - Replaces legacy log format strings with structured logging using
logrus.Fieldsthroughout the GPG agent setup process
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| pkg/ssh/helper.go | Adds RunWithTimeout function with timeout context handling and differentiated signal behavior (SIGKILL for timeout, SIGINT for cancellation) |
| pkg/gpg/gpg_forwarding.go | Adds 5-second timeouts to GPG export commands with duration tracking and debug logging for performance monitoring |
| cmd/ssh.go | Migrates to structured logging, adds duration tracking for GPG operations, and uses new RunWithTimeout with 15-second timeout for agent setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // track the amount of time it takes to setup gpg agent forwarding | ||
| // to determine whether the timeout duration is appropriate | ||
| start = time.Now() | ||
| err = devssh.RunWithTimeout(ctx, containerClient, command, nil, writer, writer, nil, 15*time.Second) |
There was a problem hiding this comment.
The timeout duration of 15 seconds appears as a magic number. Consider extracting this to a named constant (e.g., GPGAgentSetupTimeout) to improve maintainability and make it easier to adjust if needed based on the duration tracking being performed.
|
|
||
| func GetHostPubKey() ([]byte, error) { | ||
| return exec.Command("gpg", "--armor", "--export").Output() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
There was a problem hiding this comment.
The timeout duration of 5 seconds appears as a magic number. Consider extracting this to a named constant (e.g., GPGExportTimeout) to improve maintainability and make it clear that the same timeout is intentionally used for both export operations.
|
|
||
| func GetHostOwnerTrust() ([]byte, error) { | ||
| return exec.Command("gpg", "--export-ownertrust").Output() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
There was a problem hiding this comment.
The timeout duration of 5 seconds appears as a magic number. Consider extracting this to a named constant (e.g., GPGExportTimeout) to improve maintainability and make it clear that the same timeout is intentionally used for both export operations.
| log.WithFields(logrus.Fields{ | ||
| "error": err, | ||
| }).Error("failed to export GPG owner trust, skipping GPG forwarding") | ||
| return fmt.Errorf("could not export GPG owner trust") |
There was a problem hiding this comment.
The error is no longer wrapped with the original error using %w. This loses important debugging information about why the GPG owner trust export failed. Consider using fmt.Errorf with %w to preserve the error chain, for example: fmt.Errorf("could not export GPG owner trust: %w", err)
| return fmt.Errorf("could not export GPG owner trust") | |
| return fmt.Errorf("could not export GPG owner trust: %w", err) |
| err = sess.Run(command) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
The new RunWithTimeout function lacks documentation. Consider adding a function comment that explains its purpose, parameters (especially the timeout parameter), and behavior when the timeout is exceeded versus when the context is cancelled.
| // RunWithTimeout runs the given SSH command on client with the provided standard | |
| // streams and environment variables, enforcing the specified timeout. If the | |
| // timeout duration elapses, the command is terminated by sending SIGKILL to | |
| // the remote session to ensure any child processes are also stopped. If the | |
| // parent context is cancelled for any other reason before the timeout expires, | |
| // SIGINT is sent instead to allow a graceful shutdown. In both cases the SSH | |
| // session is closed and any error from sess.Run is returned. |
| log.WithFields(logrus.Fields{ | ||
| "gitGpgKey": gitGpgKey, |
There was a problem hiding this comment.
The gitGpgKey variable includes trailing newlines from the command output. When logging this value in the structured field at line 615, it may display unexpectedly with embedded newlines. Consider trimming the value before logging, or use the already-trimmed gitKey variable from line 639 for consistency.
| log.WithFields(logrus.Fields{ | |
| "gitGpgKey": gitGpgKey, | |
| gitGpgKeyStr := strings.TrimSpace(string(gitGpgKey)) | |
| log.WithFields(logrus.Fields{ | |
| "gitGpgKey": gitGpgKeyStr, |
| "error": err, | ||
| "duration": duration, | ||
| }).Error("failed to detect GPG socket path, skipping GPG forwarding") | ||
| return fmt.Errorf("could not detect GPG socket path") |
There was a problem hiding this comment.
The error is no longer wrapped with the original error using %w. This loses important debugging information about why the GPG socket path detection failed. Consider using fmt.Errorf with %w to preserve the error chain, for example: fmt.Errorf("could not detect GPG socket path: %w", err)
| return fmt.Errorf("could not detect GPG socket path") | |
| return fmt.Errorf("could not detect GPG socket path: %w", err) |
| "error": err, | ||
| "duration": duration, | ||
| }).Warn("failed to setup gpg agent forwarding") | ||
| return fmt.Errorf("GPG agent setup failed") |
There was a problem hiding this comment.
The error is no longer wrapped with the original error using %w. This loses important debugging information about why the GPG agent setup failed. Consider using fmt.Errorf with %w to preserve the error chain, for example: fmt.Errorf("GPG agent setup failed: %w", err)
| return fmt.Errorf("GPG agent setup failed") | |
| return fmt.Errorf("GPG agent setup failed: %w", err) |
Signed-off-by: Samuel K skevetter@pm.me