Fix every failed clone being reported as a timeout
The context was cancelled before its error was checked, so any failure looked like a timeout and private repositories never offered sign-in. Clone handling moves into cloneRepo, which checks for DeadlineExceeded specifically, with tests for success, failure with token masking, and timeout.
This commit is contained in:
@@ -195,6 +195,21 @@ func gitClone(ctx context.Context, repoURL, dest, token string) *exec.Cmd {
|
||||
return cmd
|
||||
}
|
||||
|
||||
// cloneRepo clones with a timeout and returns git's output with the token masked.
|
||||
func cloneRepo(repoURL, dest, token string) (string, error) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), cloneTimeout)
|
||||
defer cancel()
|
||||
output, err := gitClone(ctx, repoURL, dest, token).CombinedOutput()
|
||||
text := string(output)
|
||||
if errors.Is(ctx.Err(), context.DeadlineExceeded) {
|
||||
text = "timed out after " + cloneTimeout.String()
|
||||
}
|
||||
if token != "" {
|
||||
text = strings.ReplaceAll(text, token, "***")
|
||||
}
|
||||
return text, err
|
||||
}
|
||||
|
||||
// cloneFailure turns git output into a user-facing message and whether signing in (or
|
||||
// granting the app access) could fix it.
|
||||
func (g githubApp) cloneFailure(output string, signedIn bool) (message string, authRequired string) {
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -173,3 +174,29 @@ func TestCloneFailureMessages(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestCloneRepo(t *testing.T) {
|
||||
if _, err := exec.LookPath("git"); err != nil {
|
||||
t.Skip("git not installed")
|
||||
}
|
||||
source := t.TempDir()
|
||||
for _, args := range [][]string{{"init", "-q"}, {"-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "--allow-empty", "-m", "x"}} {
|
||||
if out, err := exec.Command("git", append([]string{"-C", source}, args...)...).CombinedOutput(); err != nil {
|
||||
t.Fatalf("git %v: %v %s", args, err, out)
|
||||
}
|
||||
}
|
||||
if out, err := cloneRepo(source, filepath.Join(t.TempDir(), "ok"), ""); err != nil {
|
||||
t.Fatalf("local clone failed: %v %s", err, out)
|
||||
}
|
||||
|
||||
out, err := cloneRepo(filepath.Join(source, "missing-ghu_secret"), filepath.Join(t.TempDir(), "x"), "ghu_secret")
|
||||
if err == nil || strings.Contains(out, "timed out") || strings.Contains(out, "ghu_secret") || !strings.Contains(out, "***") {
|
||||
t.Fatalf("failed clone = %v %q; want a masked, non-timeout error", err, out)
|
||||
}
|
||||
|
||||
defer func(old time.Duration) { cloneTimeout = old }(cloneTimeout)
|
||||
cloneTimeout = time.Nanosecond
|
||||
if out, err := cloneRepo(source, filepath.Join(t.TempDir(), "slow"), ""); err == nil || !strings.HasPrefix(out, "timed out") {
|
||||
t.Fatalf("expired clone = %v %q; want timeout", err, out)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,7 +3,6 @@ package main
|
||||
import (
|
||||
"bufio"
|
||||
"bytes"
|
||||
"context"
|
||||
"crypto/rand"
|
||||
"embed"
|
||||
"encoding/json"
|
||||
@@ -50,10 +49,13 @@ const (
|
||||
idleExpiry = time.Hour // snapshots unused and jobs untouched for this long are dropped
|
||||
maxSnapshots = 8 // ponytail: fixed caps sized for the 2GB container; make them configurable if memory changes
|
||||
maxIndexing = 2 // concurrent index runs; each already uses every CPU
|
||||
cloneTimeout = 10 * time.Minute
|
||||
)
|
||||
|
||||
var sseKeepAlive = 15 * time.Second // variable so tests can shorten it
|
||||
// Variables so tests can shorten them.
|
||||
var (
|
||||
sseKeepAlive = 15 * time.Second
|
||||
cloneTimeout = 10 * time.Minute
|
||||
)
|
||||
|
||||
type Server struct {
|
||||
jobsMu sync.Mutex
|
||||
@@ -188,17 +190,7 @@ func (s *Server) createJob(w http.ResponseWriter, r *http.Request) {
|
||||
token := s.githubAuthFor(owner).token
|
||||
go func() {
|
||||
checkout := filepath.Join(root, "repo")
|
||||
ctx, cancel := context.WithTimeout(context.Background(), cloneTimeout)
|
||||
output, err := gitClone(ctx, url, checkout, token).CombinedOutput()
|
||||
cancel()
|
||||
if err != nil {
|
||||
text := string(output)
|
||||
if ctx.Err() != nil {
|
||||
text = "timed out after " + cloneTimeout.String()
|
||||
}
|
||||
if token != "" {
|
||||
text = strings.ReplaceAll(text, token, "***")
|
||||
}
|
||||
if text, err := cloneRepo(url, checkout, token); err != nil {
|
||||
message, authRequired := s.github.cloneFailure(text, token != "")
|
||||
s.failJob(id, message, authRequired)
|
||||
s.removeWorkspace(root)
|
||||
|
||||
Reference in New Issue
Block a user