fix(gitremote): kill git's helpers on timeout and retry a ls-remote that timed out (#27)
- commit
- e4c85c7db6af0460e5c3e1da9d3ce425ad80db7e
- author
- brice · · 2026-10-14 18:41 UTC
- committer
- bnema · · 2026-10-14 18:41 UTC
- parent
- ce48ee3
- signature
- SSH Unverified
SHA256:VOCA5lYARHcbKDZ7GhTV45VmcjWnJ6ZQIBazj17LivAThe key is unknown - refs
- tag: v0.10.1
5 files · +422 -16
M internal/adapters/gitremote/gitremote.go +50 -14
| @@ -20,7 +20,8 @@ import ( | |||
| 20 | 20 | ||
| 21 | 21 | // Defaults of the bounds. | |
| 22 | 22 | const ( | |
| 23 | - | // DefaultTimeout is how long one ls-remote may take. | |
| 23 | + | // DefaultTimeout is how long ListRefs may take in all, its attempts | |
| 24 | + | // included. It is below the 30 s the runner gives a call. | |
| 24 | 25 | DefaultTimeout = 20 * time.Second | |
| 25 | 26 | // DefaultMaxOutput is the most bytes of ls-remote output read: a repository | |
| 26 | 27 | // with 100,000 tags lists about 8 MB. More is refused as unavailable. | |
| @@ -125,18 +126,50 @@ func (w *limitWriter) Write(p []byte) (int, error) { | |||
| 125 | 126 | return w.buf.Write(p) | |
| 126 | 127 | } | |
| 127 | 128 | ||
| 129 | + | // attempts is how many times a ls-remote that timed out is run: a connection | |
| 130 | + | // that github.com never answered (a lost SYN) is often not repeated by a new | |
| 131 | + | // one, which may also reach another address. | |
| 132 | + | const attempts = 2 | |
| 133 | + | ||
| 128 | 134 | // ListRefs lists the branches and tags of owner/repo, with the peeled commit | |
| 129 | - | // of the annotated tags. | |
| 135 | + | // of the annotated tags. Timeout bounds the whole call, attempts and clean-up | |
| 136 | + | // included: the time to clean up after an attempt (kill, WaitDelay) is | |
| 137 | + | // reserved before it runs, and a ls-remote that timed out is run once more | |
| 138 | + | // with what is left, at most half. The cancellation of ctx is returned as it | |
| 139 | + | // is, not retried. | |
| 130 | 140 | func (r *Remote) ListRefs(ctx context.Context, owner, repo string) ([]ports.RemoteRef, error) { | |
| 131 | 141 | if !validSegment(owner) || !validSegment(repo) { | |
| 132 | 142 | return nil, domain.ErrNotFound | |
| 133 | 143 | } | |
| 134 | - | ctx, cancel := context.WithTimeout(ctx, r.cfg.Timeout) | |
| 144 | + | deadline := time.Now().Add(r.cfg.Timeout) | |
| 145 | + | cleanup := max(min(2*time.Second, r.cfg.Timeout/8), time.Millisecond) | |
| 146 | + | ran := 0 | |
| 147 | + | for ran < attempts { | |
| 148 | + | run := min(r.cfg.Timeout/attempts, time.Until(deadline)-cleanup) | |
| 149 | + | if run <= 0 { | |
| 150 | + | break | |
| 151 | + | } | |
| 152 | + | ran++ | |
| 153 | + | refs, timedOut, err := r.listOnce(ctx, owner, repo, run, cleanup) | |
| 154 | + | switch { | |
| 155 | + | case ctx.Err() != nil: | |
| 156 | + | return nil, ctx.Err() // the caller gave up | |
| 157 | + | case !timedOut: | |
| 158 | + | return refs, err | |
| 159 | + | } | |
| 160 | + | } | |
| 161 | + | return nil, fmt.Errorf("%w: git ls-remote %s/%s did not answer within %s (%d attempts)", domain.ErrUnavailable, owner, repo, r.cfg.Timeout, ran) | |
| 162 | + | } | |
| 163 | + | ||
| 164 | + | // listOnce runs one git ls-remote for at most d, then cleans up for at most | |
| 165 | + | // cleanup. timedOut tells that d ran out and parent did not. | |
| 166 | + | func (r *Remote) listOnce(parent context.Context, owner, repo string, d, cleanup time.Duration) (refs []ports.RemoteRef, timedOut bool, _ error) { | |
| 167 | + | ctx, cancel := context.WithTimeout(parent, d) | |
| 135 | 168 | defer cancel() | |
| 136 | 169 | out := &limitWriter{max: r.cfg.MaxOutput, cancel: cancel} | |
| 137 | 170 | var stderr bytes.Buffer | |
| 138 | 171 | url := strings.TrimSuffix(r.cfg.BaseURL, "/") + "/" + owner + "/" + repo | |
| 139 | - | cmd := exec.CommandContext(ctx, r.cfg.Bin, | |
| 172 | + | cmd := exec.Command(r.cfg.Bin, | |
| 140 | 173 | "-c", "credential.helper=", "-c", "core.askPass=", "-c", "http.followRedirects=false", | |
| 141 | 174 | "ls-remote", "--", url, "refs/heads/*", "refs/tags/*") | |
| 142 | 175 | cmd.Env = []string{ | |
| @@ -147,23 +180,26 @@ func (r *Remote) ListRefs(ctx context.Context, owner, repo string) ([]ports.Remo | |||
| 147 | 180 | cmd.Env = append(cmd.Env, r.cfg.Proxy...) | |
| 148 | 181 | cmd.Dir = r.cfg.Home | |
| 149 | 182 | cmd.Stdout, cmd.Stderr = out, &limitedBuffer{b: &stderr, max: 4096} | |
| 150 | - | cmd.WaitDelay = 2 * time.Second | |
| 151 | - | err := cmd.Run() | |
| 183 | + | // The backstop for a helper that escaped the kill and holds the pipes: the | |
| 184 | + | // time reserved for it by ListRefs. | |
| 185 | + | cmd.WaitDelay = cleanup | |
| 186 | + | err := run(ctx, cmd) | |
| 152 | 187 | switch { | |
| 153 | 188 | case out.exceeded: | |
| 154 | - | return nil, fmt.Errorf("%w: git ls-remote %s/%s listed more than %d bytes", domain.ErrUnavailable, owner, repo, r.cfg.MaxOutput) | |
| 155 | - | case errors.Is(ctx.Err(), context.DeadlineExceeded): | |
| 156 | - | return nil, fmt.Errorf("%w: git ls-remote %s/%s took more than %s", domain.ErrUnavailable, owner, repo, r.cfg.Timeout) | |
| 157 | - | case ctx.Err() != nil: | |
| 158 | - | return nil, ctx.Err() | |
| 189 | + | return nil, false, fmt.Errorf("%w: git ls-remote %s/%s listed more than %d bytes", domain.ErrUnavailable, owner, repo, r.cfg.MaxOutput) | |
| 190 | + | case err != nil && ctx.Err() != nil: | |
| 191 | + | return nil, true, nil | |
| 159 | 192 | case err != nil: | |
| 160 | 193 | msg := stderr.String() | |
| 161 | 194 | if notFound(msg) { | |
| 162 | - | return nil, domain.ErrNotFound | |
| 195 | + | return nil, false, domain.ErrNotFound | |
| 196 | + | } | |
| 197 | + | if firstLine(msg) == "" { | |
| 198 | + | msg = err.Error() | |
| 163 | 199 | } | |
| 164 | - | return nil, fmt.Errorf("%w: git ls-remote %s/%s: %v", domain.ErrUnavailable, owner, repo, firstLine(msg)) | |
| 200 | + | return nil, false, fmt.Errorf("%w: git ls-remote %s/%s: %v", domain.ErrUnavailable, owner, repo, firstLine(msg)) | |
| 165 | 201 | } | |
| 166 | - | return parse(out.buf.String()), nil | |
| 202 | + | return parse(out.buf.String()), false, nil | |
| 167 | 203 | } | |
| 168 | 204 | ||
| 169 | 205 | // notFound recognises what git says when github.com answers 404 or asks for | |
| @@ -20,7 +20,8 @@ import ( | |||||
| 20 | 20 | ||||
| 21 | // Defaults of the bounds. | 21 | // Defaults of the bounds. | ||
| 22 | const ( | 22 | const ( | ||
| 23 | - | // DefaultTimeout is how long one ls-remote may take. | 23 | + | // DefaultTimeout is how long ListRefs may take in all, its attempts |
| 24 | + | // included. It is below the 30 s the runner gives a call. | |||
| 24 | DefaultTimeout = 20 * time.Second | 25 | DefaultTimeout = 20 * time.Second | ||
| 25 | // DefaultMaxOutput is the most bytes of ls-remote output read: a repository | 26 | // DefaultMaxOutput is the most bytes of ls-remote output read: a repository | ||
| 26 | // with 100,000 tags lists about 8 MB. More is refused as unavailable. | 27 | // with 100,000 tags lists about 8 MB. More is refused as unavailable. | ||
| @@ -125,18 +126,50 @@ func (w *limitWriter) Write(p []byte) (int, error) { | |||||
| 125 | return w.buf.Write(p) | 126 | return w.buf.Write(p) | ||
| 126 | } | 127 | } | ||
| 127 | 128 | ||||
| 129 | + | // attempts is how many times a ls-remote that timed out is run: a connection | |||
| 130 | + | // that github.com never answered (a lost SYN) is often not repeated by a new | |||
| 131 | + | // one, which may also reach another address. | |||
| 132 | + | const attempts = 2 | |||
| 133 | + | ||||
| 128 | // ListRefs lists the branches and tags of owner/repo, with the peeled commit | 134 | // ListRefs lists the branches and tags of owner/repo, with the peeled commit | ||
| 129 | - | // of the annotated tags. | 135 | + | // of the annotated tags. Timeout bounds the whole call, attempts and clean-up |
| 136 | + | // included: the time to clean up after an attempt (kill, WaitDelay) is | |||
| 137 | + | // reserved before it runs, and a ls-remote that timed out is run once more | |||
| 138 | + | // with what is left, at most half. The cancellation of ctx is returned as it | |||
| 139 | + | // is, not retried. | |||
| 130 | func (r *Remote) ListRefs(ctx context.Context, owner, repo string) ([]ports.RemoteRef, error) { | 140 | func (r *Remote) ListRefs(ctx context.Context, owner, repo string) ([]ports.RemoteRef, error) { | ||
| 131 | if !validSegment(owner) || !validSegment(repo) { | 141 | if !validSegment(owner) || !validSegment(repo) { | ||
| 132 | return nil, domain.ErrNotFound | 142 | return nil, domain.ErrNotFound | ||
| 133 | } | 143 | } | ||
| 134 | - | ctx, cancel := context.WithTimeout(ctx, r.cfg.Timeout) | 144 | + | deadline := time.Now().Add(r.cfg.Timeout) |
| 145 | + | cleanup := max(min(2*time.Second, r.cfg.Timeout/8), time.Millisecond) | |||
| 146 | + | ran := 0 | |||
| 147 | + | for ran < attempts { | |||
| 148 | + | run := min(r.cfg.Timeout/attempts, time.Until(deadline)-cleanup) | |||
| 149 | + | if run <= 0 { | |||
| 150 | + | break | |||
| 151 | + | } | |||
| 152 | + | ran++ | |||
| 153 | + | refs, timedOut, err := r.listOnce(ctx, owner, repo, run, cleanup) | |||
| 154 | + | switch { | |||
| 155 | + | case ctx.Err() != nil: | |||
| 156 | + | return nil, ctx.Err() // the caller gave up | |||
| 157 | + | case !timedOut: | |||
| 158 | + | return refs, err | |||
| 159 | + | } | |||
| 160 | + | } | |||
| 161 | + | return nil, fmt.Errorf("%w: git ls-remote %s/%s did not answer within %s (%d attempts)", domain.ErrUnavailable, owner, repo, r.cfg.Timeout, ran) | |||
| 162 | + | } | |||
| 163 | + | ||||
| 164 | + | // listOnce runs one git ls-remote for at most d, then cleans up for at most | |||
| 165 | + | // cleanup. timedOut tells that d ran out and parent did not. | |||
| 166 | + | func (r *Remote) listOnce(parent context.Context, owner, repo string, d, cleanup time.Duration) (refs []ports.RemoteRef, timedOut bool, _ error) { | |||
| 167 | + | ctx, cancel := context.WithTimeout(parent, d) | |||
| 135 | defer cancel() | 168 | defer cancel() | ||
| 136 | out := &limitWriter{max: r.cfg.MaxOutput, cancel: cancel} | 169 | out := &limitWriter{max: r.cfg.MaxOutput, cancel: cancel} | ||
| 137 | var stderr bytes.Buffer | 170 | var stderr bytes.Buffer | ||
| 138 | url := strings.TrimSuffix(r.cfg.BaseURL, "/") + "/" + owner + "/" + repo | 171 | url := strings.TrimSuffix(r.cfg.BaseURL, "/") + "/" + owner + "/" + repo | ||
| 139 | - | cmd := exec.CommandContext(ctx, r.cfg.Bin, | 172 | + | cmd := exec.Command(r.cfg.Bin, |
| 140 | "-c", "credential.helper=", "-c", "core.askPass=", "-c", "http.followRedirects=false", | 173 | "-c", "credential.helper=", "-c", "core.askPass=", "-c", "http.followRedirects=false", | ||
| 141 | "ls-remote", "--", url, "refs/heads/*", "refs/tags/*") | 174 | "ls-remote", "--", url, "refs/heads/*", "refs/tags/*") | ||
| 142 | cmd.Env = []string{ | 175 | cmd.Env = []string{ | ||
| @@ -147,23 +180,26 @@ func (r *Remote) ListRefs(ctx context.Context, owner, repo string) ([]ports.Remo | |||||
| 147 | cmd.Env = append(cmd.Env, r.cfg.Proxy...) | 180 | cmd.Env = append(cmd.Env, r.cfg.Proxy...) | ||
| 148 | cmd.Dir = r.cfg.Home | 181 | cmd.Dir = r.cfg.Home | ||
| 149 | cmd.Stdout, cmd.Stderr = out, &limitedBuffer{b: &stderr, max: 4096} | 182 | cmd.Stdout, cmd.Stderr = out, &limitedBuffer{b: &stderr, max: 4096} | ||
| 150 | - | cmd.WaitDelay = 2 * time.Second | 183 | + | // The backstop for a helper that escaped the kill and holds the pipes: the |
| 151 | - | err := cmd.Run() | 184 | + | // time reserved for it by ListRefs. |
| 185 | + | cmd.WaitDelay = cleanup | |||
| 186 | + | err := run(ctx, cmd) | |||
| 152 | switch { | 187 | switch { | ||
| 153 | case out.exceeded: | 188 | case out.exceeded: | ||
| 154 | - | return nil, fmt.Errorf("%w: git ls-remote %s/%s listed more than %d bytes", domain.ErrUnavailable, owner, repo, r.cfg.MaxOutput) | 189 | + | return nil, false, fmt.Errorf("%w: git ls-remote %s/%s listed more than %d bytes", domain.ErrUnavailable, owner, repo, r.cfg.MaxOutput) |
| 155 | - | case errors.Is(ctx.Err(), context.DeadlineExceeded): | 190 | + | case err != nil && ctx.Err() != nil: |
| 156 | - | return nil, fmt.Errorf("%w: git ls-remote %s/%s took more than %s", domain.ErrUnavailable, owner, repo, r.cfg.Timeout) | 191 | + | return nil, true, nil |
| 157 | - | case ctx.Err() != nil: | |||
| 158 | - | return nil, ctx.Err() | |||
| 159 | case err != nil: | 192 | case err != nil: | ||
| 160 | msg := stderr.String() | 193 | msg := stderr.String() | ||
| 161 | if notFound(msg) { | 194 | if notFound(msg) { | ||
| 162 | - | return nil, domain.ErrNotFound | 195 | + | return nil, false, domain.ErrNotFound |
| 196 | + | } | |||
| 197 | + | if firstLine(msg) == "" { | |||
| 198 | + | msg = err.Error() | |||
| 163 | } | 199 | } | ||
| 164 | - | return nil, fmt.Errorf("%w: git ls-remote %s/%s: %v", domain.ErrUnavailable, owner, repo, firstLine(msg)) | 200 | + | return nil, false, fmt.Errorf("%w: git ls-remote %s/%s: %v", domain.ErrUnavailable, owner, repo, firstLine(msg)) |
| 165 | } | 201 | } | ||
| 166 | - | return parse(out.buf.String()), nil | 202 | + | return parse(out.buf.String()), false, nil |
| 167 | } | 203 | } | ||
| 168 | 204 | ||||
| 169 | // notFound recognises what git says when github.com answers 404 or asks for | 205 | // notFound recognises what git says when github.com answers 404 or asks for | ||
M internal/adapters/gitremote/gitremote_test.go +119 -2
| @@ -7,6 +7,7 @@ import ( | |||
| 7 | 7 | "os" | |
| 8 | 8 | "os/exec" | |
| 9 | 9 | "path/filepath" | |
| 10 | + | "strconv" | |
| 10 | 11 | "strings" | |
| 11 | 12 | "testing" | |
| 12 | 13 | "time" | |
| @@ -122,9 +123,125 @@ func TestListRefsTimesOut(t *testing.T) { | |||
| 122 | 123 | bin := filepath.Join(t.TempDir(), "git") | |
| 123 | 124 | require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\nsleep 30\n"), 0o755)) | |
| 124 | 125 | start := time.Now() | |
| 125 | - | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 200*time.Millisecond }).ListRefs(context.Background(), "o", "r") | |
| 126 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 400*time.Millisecond }).ListRefs(context.Background(), "o", "r") | |
| 126 | 127 | assert.ErrorIs(t, err, domain.ErrUnavailable) | |
| 127 | - | assert.Less(t, time.Since(start), 10*time.Second) | |
| 128 | + | // One overall deadline, both attempts and the clean-up inside it. | |
| 129 | + | assert.Less(t, time.Since(start), 1*time.Second) | |
| 130 | + | } | |
| 131 | + | ||
| 132 | + | // dead tells whether the process pid is gone or a zombie (not reaped yet, as | |
| 133 | + | // under a subreaper that does not wait): either way it runs no more. | |
| 134 | + | func dead(pid int) bool { | |
| 135 | + | raw, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") | |
| 136 | + | if err != nil { | |
| 137 | + | return true | |
| 138 | + | } | |
| 139 | + | // "pid (comm) S ...": comm may hold spaces and parentheses. | |
| 140 | + | s := string(raw) | |
| 141 | + | i := strings.LastIndexByte(s, ')') | |
| 142 | + | return i < 0 || len(s) < i+3 || s[i+2] == 'Z' || s[i+2] == 'X' | |
| 143 | + | } | |
| 144 | + | ||
| 145 | + | // git runs git-remote-https, the one that connects: when the time is up it is | |
| 146 | + | // killed too, and does not outlive the call. Each attempt starts one. | |
| 147 | + | func TestListRefsKillsTheHelpersOfGit(t *testing.T) { | |
| 148 | + | if _, err := os.Stat("/proc/self/stat"); err != nil { | |
| 149 | + | t.Skip("needs /proc") | |
| 150 | + | } | |
| 151 | + | dir := t.TempDir() | |
| 152 | + | pids := filepath.Join(dir, "pids") | |
| 153 | + | bin := filepath.Join(dir, "git") | |
| 154 | + | script := "#!/bin/sh\nsleep 30 &\necho $! >> '" + pids + "'\nwait\n" | |
| 155 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |
| 156 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 600*time.Millisecond }).ListRefs(context.Background(), "o", "r") | |
| 157 | + | assert.ErrorIs(t, err, domain.ErrUnavailable) | |
| 158 | + | raw, rerr := os.ReadFile(pids) | |
| 159 | + | require.NoError(t, rerr) | |
| 160 | + | fields := strings.Fields(string(raw)) | |
| 161 | + | require.Len(t, fields, 2, "one helper per attempt") | |
| 162 | + | for _, f := range fields { | |
| 163 | + | pid, aerr := strconv.Atoi(f) | |
| 164 | + | require.NoError(t, aerr) | |
| 165 | + | assert.Eventually(t, func() bool { return dead(pid) }, 2*time.Second, 20*time.Millisecond, "the helper %d of git is still running", pid) | |
| 166 | + | } | |
| 167 | + | } | |
| 168 | + | ||
| 169 | + | // A helper that left the group (setsid) and keeps the output of git open | |
| 170 | + | // cannot hold the call past its Timeout: the clean-up is inside the budget. | |
| 171 | + | func TestListRefsStaysInsideTimeoutWithAHelperHoldingTheOutput(t *testing.T) { | |
| 172 | + | setsid, err := exec.LookPath("setsid") | |
| 173 | + | if err != nil { | |
| 174 | + | t.Skip("needs setsid") | |
| 175 | + | } | |
| 176 | + | dir := t.TempDir() | |
| 177 | + | pids := filepath.Join(dir, "pids") | |
| 178 | + | bin := filepath.Join(dir, "git") | |
| 179 | + | script := "#!/bin/sh\n'" + setsid + "' sleep 30 &\necho $! >> '" + pids + "'\nwait\n" | |
| 180 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |
| 181 | + | t.Cleanup(func() { | |
| 182 | + | raw, _ := os.ReadFile(pids) | |
| 183 | + | for _, f := range strings.Fields(string(raw)) { | |
| 184 | + | if pid, err := strconv.Atoi(f); err == nil { | |
| 185 | + | if p, err := os.FindProcess(pid); err == nil { | |
| 186 | + | _ = p.Kill() | |
| 187 | + | } | |
| 188 | + | } | |
| 189 | + | } | |
| 190 | + | }) | |
| 191 | + | const timeout = 400 * time.Millisecond | |
| 192 | + | r := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, timeout }) | |
| 193 | + | start := time.Now() | |
| 194 | + | _, err = r.ListRefs(context.Background(), "o", "r") | |
| 195 | + | assert.ErrorIs(t, err, domain.ErrUnavailable) | |
| 196 | + | // Generous for a loaded CI, still far below a clean-up outside the budget | |
| 197 | + | // (a full WaitDelay of seconds). | |
| 198 | + | assert.LessOrEqual(t, time.Since(start), timeout+250*time.Millisecond) | |
| 199 | + | } | |
| 200 | + | ||
| 201 | + | // The helper of a git that exits by itself does not outlive it either. | |
| 202 | + | func TestListRefsKillsTheHelpersOfAGitThatExited(t *testing.T) { | |
| 203 | + | if _, err := os.Stat("/proc/self/stat"); err != nil { | |
| 204 | + | t.Skip("needs /proc") | |
| 205 | + | } | |
| 206 | + | dir := t.TempDir() | |
| 207 | + | pidFile := filepath.Join(dir, "pid") | |
| 208 | + | bin := filepath.Join(dir, "git") | |
| 209 | + | script := "#!/bin/sh\nsleep 30 >/dev/null 2>&1 &\necho $! > '" + pidFile + "'\nexit 0\n" | |
| 210 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |
| 211 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin = bin }).ListRefs(context.Background(), "o", "r") | |
| 212 | + | require.NoError(t, err) | |
| 213 | + | raw, rerr := os.ReadFile(pidFile) | |
| 214 | + | require.NoError(t, rerr) | |
| 215 | + | pid, aerr := strconv.Atoi(strings.TrimSpace(string(raw))) | |
| 216 | + | require.NoError(t, aerr) | |
| 217 | + | assert.Eventually(t, func() bool { return dead(pid) }, 2*time.Second, 20*time.Millisecond) | |
| 218 | + | } | |
| 219 | + | ||
| 220 | + | // A connection github.com never answers is run again within the same bound, | |
| 221 | + | // and the second one can succeed. | |
| 222 | + | func TestListRefsRunsAgainAfterATimeOut(t *testing.T) { | |
| 223 | + | dir := t.TempDir() | |
| 224 | + | bin := filepath.Join(dir, "git") | |
| 225 | + | id := strings.Repeat("a", domain.ObjectIDLen) | |
| 226 | + | script := "#!/bin/sh\nif [ ! -e '" + dir + "/seen' ]; then touch '" + dir + "/seen'; sleep 30; fi\nprintf '" + id + "\\trefs/heads/main\\n'\n" | |
| 227 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |
| 228 | + | start := time.Now() | |
| 229 | + | refs, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 1000*time.Millisecond }).ListRefs(context.Background(), "o", "r") | |
| 230 | + | require.NoError(t, err) | |
| 231 | + | require.Len(t, refs, 1) | |
| 232 | + | assert.Equal(t, "refs/heads/main", refs[0].Name) | |
| 233 | + | assert.Less(t, time.Since(start), 1500*time.Millisecond) | |
| 234 | + | } | |
| 235 | + | ||
| 236 | + | func TestListRefsStopsWhenTheCallerGivesUp(t *testing.T) { | |
| 237 | + | bin := filepath.Join(t.TempDir(), "git") | |
| 238 | + | require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\nsleep 30\n"), 0o755)) | |
| 239 | + | ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) | |
| 240 | + | defer cancel() | |
| 241 | + | start := time.Now() | |
| 242 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin = bin }).ListRefs(ctx, "o", "r") | |
| 243 | + | assert.ErrorIs(t, err, context.DeadlineExceeded) | |
| 244 | + | assert.Less(t, time.Since(start), 1500*time.Millisecond) | |
| 128 | 245 | } | |
| 129 | 246 | ||
| 130 | 247 | // git asks for no credentials and runs with no configuration of the machine: | |
| @@ -7,6 +7,7 @@ import ( | |||||
| 7 | "os" | 7 | "os" | ||
| 8 | "os/exec" | 8 | "os/exec" | ||
| 9 | "path/filepath" | 9 | "path/filepath" | ||
| 10 | + | "strconv" | |||
| 10 | "strings" | 11 | "strings" | ||
| 11 | "testing" | 12 | "testing" | ||
| 12 | "time" | 13 | "time" | ||
| @@ -122,9 +123,125 @@ func TestListRefsTimesOut(t *testing.T) { | |||||
| 122 | bin := filepath.Join(t.TempDir(), "git") | 123 | bin := filepath.Join(t.TempDir(), "git") | ||
| 123 | require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\nsleep 30\n"), 0o755)) | 124 | require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\nsleep 30\n"), 0o755)) | ||
| 124 | start := time.Now() | 125 | start := time.Now() | ||
| 125 | - | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 200*time.Millisecond }).ListRefs(context.Background(), "o", "r") | 126 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 400*time.Millisecond }).ListRefs(context.Background(), "o", "r") |
| 126 | assert.ErrorIs(t, err, domain.ErrUnavailable) | 127 | assert.ErrorIs(t, err, domain.ErrUnavailable) | ||
| 127 | - | assert.Less(t, time.Since(start), 10*time.Second) | 128 | + | // One overall deadline, both attempts and the clean-up inside it. |
| 129 | + | assert.Less(t, time.Since(start), 1*time.Second) | |||
| 130 | + | } | |||
| 131 | + | ||||
| 132 | + | // dead tells whether the process pid is gone or a zombie (not reaped yet, as | |||
| 133 | + | // under a subreaper that does not wait): either way it runs no more. | |||
| 134 | + | func dead(pid int) bool { | |||
| 135 | + | raw, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") | |||
| 136 | + | if err != nil { | |||
| 137 | + | return true | |||
| 138 | + | } | |||
| 139 | + | // "pid (comm) S ...": comm may hold spaces and parentheses. | |||
| 140 | + | s := string(raw) | |||
| 141 | + | i := strings.LastIndexByte(s, ')') | |||
| 142 | + | return i < 0 || len(s) < i+3 || s[i+2] == 'Z' || s[i+2] == 'X' | |||
| 143 | + | } | |||
| 144 | + | ||||
| 145 | + | // git runs git-remote-https, the one that connects: when the time is up it is | |||
| 146 | + | // killed too, and does not outlive the call. Each attempt starts one. | |||
| 147 | + | func TestListRefsKillsTheHelpersOfGit(t *testing.T) { | |||
| 148 | + | if _, err := os.Stat("/proc/self/stat"); err != nil { | |||
| 149 | + | t.Skip("needs /proc") | |||
| 150 | + | } | |||
| 151 | + | dir := t.TempDir() | |||
| 152 | + | pids := filepath.Join(dir, "pids") | |||
| 153 | + | bin := filepath.Join(dir, "git") | |||
| 154 | + | script := "#!/bin/sh\nsleep 30 &\necho $! >> '" + pids + "'\nwait\n" | |||
| 155 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |||
| 156 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 600*time.Millisecond }).ListRefs(context.Background(), "o", "r") | |||
| 157 | + | assert.ErrorIs(t, err, domain.ErrUnavailable) | |||
| 158 | + | raw, rerr := os.ReadFile(pids) | |||
| 159 | + | require.NoError(t, rerr) | |||
| 160 | + | fields := strings.Fields(string(raw)) | |||
| 161 | + | require.Len(t, fields, 2, "one helper per attempt") | |||
| 162 | + | for _, f := range fields { | |||
| 163 | + | pid, aerr := strconv.Atoi(f) | |||
| 164 | + | require.NoError(t, aerr) | |||
| 165 | + | assert.Eventually(t, func() bool { return dead(pid) }, 2*time.Second, 20*time.Millisecond, "the helper %d of git is still running", pid) | |||
| 166 | + | } | |||
| 167 | + | } | |||
| 168 | + | ||||
| 169 | + | // A helper that left the group (setsid) and keeps the output of git open | |||
| 170 | + | // cannot hold the call past its Timeout: the clean-up is inside the budget. | |||
| 171 | + | func TestListRefsStaysInsideTimeoutWithAHelperHoldingTheOutput(t *testing.T) { | |||
| 172 | + | setsid, err := exec.LookPath("setsid") | |||
| 173 | + | if err != nil { | |||
| 174 | + | t.Skip("needs setsid") | |||
| 175 | + | } | |||
| 176 | + | dir := t.TempDir() | |||
| 177 | + | pids := filepath.Join(dir, "pids") | |||
| 178 | + | bin := filepath.Join(dir, "git") | |||
| 179 | + | script := "#!/bin/sh\n'" + setsid + "' sleep 30 &\necho $! >> '" + pids + "'\nwait\n" | |||
| 180 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |||
| 181 | + | t.Cleanup(func() { | |||
| 182 | + | raw, _ := os.ReadFile(pids) | |||
| 183 | + | for _, f := range strings.Fields(string(raw)) { | |||
| 184 | + | if pid, err := strconv.Atoi(f); err == nil { | |||
| 185 | + | if p, err := os.FindProcess(pid); err == nil { | |||
| 186 | + | _ = p.Kill() | |||
| 187 | + | } | |||
| 188 | + | } | |||
| 189 | + | } | |||
| 190 | + | }) | |||
| 191 | + | const timeout = 400 * time.Millisecond | |||
| 192 | + | r := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, timeout }) | |||
| 193 | + | start := time.Now() | |||
| 194 | + | _, err = r.ListRefs(context.Background(), "o", "r") | |||
| 195 | + | assert.ErrorIs(t, err, domain.ErrUnavailable) | |||
| 196 | + | // Generous for a loaded CI, still far below a clean-up outside the budget | |||
| 197 | + | // (a full WaitDelay of seconds). | |||
| 198 | + | assert.LessOrEqual(t, time.Since(start), timeout+250*time.Millisecond) | |||
| 199 | + | } | |||
| 200 | + | ||||
| 201 | + | // The helper of a git that exits by itself does not outlive it either. | |||
| 202 | + | func TestListRefsKillsTheHelpersOfAGitThatExited(t *testing.T) { | |||
| 203 | + | if _, err := os.Stat("/proc/self/stat"); err != nil { | |||
| 204 | + | t.Skip("needs /proc") | |||
| 205 | + | } | |||
| 206 | + | dir := t.TempDir() | |||
| 207 | + | pidFile := filepath.Join(dir, "pid") | |||
| 208 | + | bin := filepath.Join(dir, "git") | |||
| 209 | + | script := "#!/bin/sh\nsleep 30 >/dev/null 2>&1 &\necho $! > '" + pidFile + "'\nexit 0\n" | |||
| 210 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |||
| 211 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin = bin }).ListRefs(context.Background(), "o", "r") | |||
| 212 | + | require.NoError(t, err) | |||
| 213 | + | raw, rerr := os.ReadFile(pidFile) | |||
| 214 | + | require.NoError(t, rerr) | |||
| 215 | + | pid, aerr := strconv.Atoi(strings.TrimSpace(string(raw))) | |||
| 216 | + | require.NoError(t, aerr) | |||
| 217 | + | assert.Eventually(t, func() bool { return dead(pid) }, 2*time.Second, 20*time.Millisecond) | |||
| 218 | + | } | |||
| 219 | + | ||||
| 220 | + | // A connection github.com never answers is run again within the same bound, | |||
| 221 | + | // and the second one can succeed. | |||
| 222 | + | func TestListRefsRunsAgainAfterATimeOut(t *testing.T) { | |||
| 223 | + | dir := t.TempDir() | |||
| 224 | + | bin := filepath.Join(dir, "git") | |||
| 225 | + | id := strings.Repeat("a", domain.ObjectIDLen) | |||
| 226 | + | script := "#!/bin/sh\nif [ ! -e '" + dir + "/seen' ]; then touch '" + dir + "/seen'; sleep 30; fi\nprintf '" + id + "\\trefs/heads/main\\n'\n" | |||
| 227 | + | require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) | |||
| 228 | + | start := time.Now() | |||
| 229 | + | refs, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin, c.Timeout = bin, 1000*time.Millisecond }).ListRefs(context.Background(), "o", "r") | |||
| 230 | + | require.NoError(t, err) | |||
| 231 | + | require.Len(t, refs, 1) | |||
| 232 | + | assert.Equal(t, "refs/heads/main", refs[0].Name) | |||
| 233 | + | assert.Less(t, time.Since(start), 1500*time.Millisecond) | |||
| 234 | + | } | |||
| 235 | + | ||||
| 236 | + | func TestListRefsStopsWhenTheCallerGivesUp(t *testing.T) { | |||
| 237 | + | bin := filepath.Join(t.TempDir(), "git") | |||
| 238 | + | require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\nsleep 30\n"), 0o755)) | |||
| 239 | + | ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) | |||
| 240 | + | defer cancel() | |||
| 241 | + | start := time.Now() | |||
| 242 | + | _, err := newRemote(t, "file:///x", func(c *gitremote.Config) { c.Bin = bin }).ListRefs(ctx, "o", "r") | |||
| 243 | + | assert.ErrorIs(t, err, context.DeadlineExceeded) | |||
| 244 | + | assert.Less(t, time.Since(start), 1500*time.Millisecond) | |||
| 128 | } | 245 | } | ||
| 129 | 246 | ||||
| 130 | // git asks for no credentials and runs with no configuration of the machine: | 247 | // git asks for no credentials and runs with no configuration of the machine: | ||
A internal/adapters/gitremote/run_linux.go +107 -0
| @@ -0,0 +1,107 @@ | |||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |
| 2 | + | ||
| 3 | + | //go:build linux | |
| 4 | + | ||
| 5 | + | package gitremote | |
| 6 | + | ||
| 7 | + | import ( | |
| 8 | + | "context" | |
| 9 | + | "errors" | |
| 10 | + | "fmt" | |
| 11 | + | "os/exec" | |
| 12 | + | "syscall" | |
| 13 | + | ||
| 14 | + | "golang.org/x/sys/unix" | |
| 15 | + | ) | |
| 16 | + | ||
| 17 | + | // Tests replace these two. | |
| 18 | + | var ( | |
| 19 | + | waitid = unix.Waitid | |
| 20 | + | killGroup = func(pgid int) error { return syscall.Kill(-pgid, syscall.SIGKILL) } | |
| 21 | + | ) | |
| 22 | + | ||
| 23 | + | // run starts cmd, waits for it, and kills what it leaves running. | |
| 24 | + | // | |
| 25 | + | // git runs git-remote-https, which is the process that connects and that holds | |
| 26 | + | // the pipes of git's output. Killing git alone leaves it running, and Wait | |
| 27 | + | // blocked on the pipes. So git leads a process group of its own, and the group | |
| 28 | + | // is killed when ctx is done and also when git has exited. | |
| 29 | + | // | |
| 30 | + | // The invariant that makes killing a group safe: the group is only signalled | |
| 31 | + | // while its leader is still not reaped. Linux does not reuse a PID as long as | |
| 32 | + | // the process is a zombie nobody waited for, nor while it is the group ID of a | |
| 33 | + | // live member, so -pid is still our group and cannot be another one. The | |
| 34 | + | // leader is therefore waited for with waitid(WNOWAIT), which observes its exit | |
| 35 | + | // without reaping it; the group is signalled; and only then cmd.Wait reaps. A | |
| 36 | + | // kill(-pid) after Wait, or from Cmd.Cancel which races with it, would not be | |
| 37 | + | // safe. When waitid fails, whatever the reason, nothing proves the leader is | |
| 38 | + | // unreaped: the group is not signalled, only the leader is killed (through | |
| 39 | + | // its pidfd, which cannot hit a reused PID), and the error is returned. | |
| 40 | + | // cmd.WaitDelay (set by the caller) stays the backstop for a descendant that | |
| 41 | + | // escaped the group and holds the pipes. | |
| 42 | + | func run(ctx context.Context, cmd *exec.Cmd) error { | |
| 43 | + | if err := ctx.Err(); err != nil { | |
| 44 | + | return err | |
| 45 | + | } | |
| 46 | + | cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} | |
| 47 | + | if err := cmd.Start(); err != nil { | |
| 48 | + | return err | |
| 49 | + | } | |
| 50 | + | pid := cmd.Process.Pid | |
| 51 | + | observed := make(chan error, 1) | |
| 52 | + | go func() { observed <- waitExit(pid) }() | |
| 53 | + | ||
| 54 | + | var werr error | |
| 55 | + | select { | |
| 56 | + | case err := <-observed: | |
| 57 | + | if err != nil { | |
| 58 | + | werr = err // the leader may have been reaped: no group signal | |
| 59 | + | } else { | |
| 60 | + | signalGroup(cmd, pid) // exited, not reaped: a zombie | |
| 61 | + | } | |
| 62 | + | case <-ctx.Done(): | |
| 63 | + | select { | |
| 64 | + | case err := <-observed: | |
| 65 | + | // waitid answered at the same time: trust it as above. | |
| 66 | + | if err != nil { | |
| 67 | + | werr = err | |
| 68 | + | } else { | |
| 69 | + | signalGroup(cmd, pid) | |
| 70 | + | } | |
| 71 | + | default: | |
| 72 | + | // No waitid error so far: the leader is alive or a zombie, unreaped | |
| 73 | + | // (only cmd.Wait below reaps it). | |
| 74 | + | signalGroup(cmd, pid) | |
| 75 | + | if err := <-observed; err != nil { | |
| 76 | + | werr = err // after the signals: reported, nothing more to do | |
| 77 | + | } | |
| 78 | + | } | |
| 79 | + | } | |
| 80 | + | if werr != nil { | |
| 81 | + | _ = cmd.Process.Kill() | |
| 82 | + | } | |
| 83 | + | err := cmd.Wait() | |
| 84 | + | if werr != nil { | |
| 85 | + | return fmt.Errorf("waiting for the process %d: %w", pid, werr) | |
| 86 | + | } | |
| 87 | + | return err | |
| 88 | + | } | |
| 89 | + | ||
| 90 | + | // signalGroup kills the group of the leader, and the leader alone should it | |
| 91 | + | // have left the group. The leader must not be reaped yet. | |
| 92 | + | func signalGroup(cmd *exec.Cmd, pid int) { | |
| 93 | + | _ = killGroup(pid) | |
| 94 | + | _ = cmd.Process.Kill() | |
| 95 | + | } | |
| 96 | + | ||
| 97 | + | // waitExit returns when the child pid has exited, without reaping it, or the | |
| 98 | + | // error of waitid. | |
| 99 | + | func waitExit(pid int) error { | |
| 100 | + | var info unix.Siginfo | |
| 101 | + | for { | |
| 102 | + | err := waitid(unix.P_PID, pid, &info, unix.WEXITED|unix.WNOWAIT, nil) | |
| 103 | + | if !errors.Is(err, unix.EINTR) { | |
| 104 | + | return err | |
| 105 | + | } | |
| 106 | + | } | |
| 107 | + | } | |
| @@ -0,0 +1,107 @@ | |||||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |||
| 2 | + | ||||
| 3 | + | //go:build linux | |||
| 4 | + | ||||
| 5 | + | package gitremote | |||
| 6 | + | ||||
| 7 | + | import ( | |||
| 8 | + | "context" | |||
| 9 | + | "errors" | |||
| 10 | + | "fmt" | |||
| 11 | + | "os/exec" | |||
| 12 | + | "syscall" | |||
| 13 | + | ||||
| 14 | + | "golang.org/x/sys/unix" | |||
| 15 | + | ) | |||
| 16 | + | ||||
| 17 | + | // Tests replace these two. | |||
| 18 | + | var ( | |||
| 19 | + | waitid = unix.Waitid | |||
| 20 | + | killGroup = func(pgid int) error { return syscall.Kill(-pgid, syscall.SIGKILL) } | |||
| 21 | + | ) | |||
| 22 | + | ||||
| 23 | + | // run starts cmd, waits for it, and kills what it leaves running. | |||
| 24 | + | // | |||
| 25 | + | // git runs git-remote-https, which is the process that connects and that holds | |||
| 26 | + | // the pipes of git's output. Killing git alone leaves it running, and Wait | |||
| 27 | + | // blocked on the pipes. So git leads a process group of its own, and the group | |||
| 28 | + | // is killed when ctx is done and also when git has exited. | |||
| 29 | + | // | |||
| 30 | + | // The invariant that makes killing a group safe: the group is only signalled | |||
| 31 | + | // while its leader is still not reaped. Linux does not reuse a PID as long as | |||
| 32 | + | // the process is a zombie nobody waited for, nor while it is the group ID of a | |||
| 33 | + | // live member, so -pid is still our group and cannot be another one. The | |||
| 34 | + | // leader is therefore waited for with waitid(WNOWAIT), which observes its exit | |||
| 35 | + | // without reaping it; the group is signalled; and only then cmd.Wait reaps. A | |||
| 36 | + | // kill(-pid) after Wait, or from Cmd.Cancel which races with it, would not be | |||
| 37 | + | // safe. When waitid fails, whatever the reason, nothing proves the leader is | |||
| 38 | + | // unreaped: the group is not signalled, only the leader is killed (through | |||
| 39 | + | // its pidfd, which cannot hit a reused PID), and the error is returned. | |||
| 40 | + | // cmd.WaitDelay (set by the caller) stays the backstop for a descendant that | |||
| 41 | + | // escaped the group and holds the pipes. | |||
| 42 | + | func run(ctx context.Context, cmd *exec.Cmd) error { | |||
| 43 | + | if err := ctx.Err(); err != nil { | |||
| 44 | + | return err | |||
| 45 | + | } | |||
| 46 | + | cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} | |||
| 47 | + | if err := cmd.Start(); err != nil { | |||
| 48 | + | return err | |||
| 49 | + | } | |||
| 50 | + | pid := cmd.Process.Pid | |||
| 51 | + | observed := make(chan error, 1) | |||
| 52 | + | go func() { observed <- waitExit(pid) }() | |||
| 53 | + | ||||
| 54 | + | var werr error | |||
| 55 | + | select { | |||
| 56 | + | case err := <-observed: | |||
| 57 | + | if err != nil { | |||
| 58 | + | werr = err // the leader may have been reaped: no group signal | |||
| 59 | + | } else { | |||
| 60 | + | signalGroup(cmd, pid) // exited, not reaped: a zombie | |||
| 61 | + | } | |||
| 62 | + | case <-ctx.Done(): | |||
| 63 | + | select { | |||
| 64 | + | case err := <-observed: | |||
| 65 | + | // waitid answered at the same time: trust it as above. | |||
| 66 | + | if err != nil { | |||
| 67 | + | werr = err | |||
| 68 | + | } else { | |||
| 69 | + | signalGroup(cmd, pid) | |||
| 70 | + | } | |||
| 71 | + | default: | |||
| 72 | + | // No waitid error so far: the leader is alive or a zombie, unreaped | |||
| 73 | + | // (only cmd.Wait below reaps it). | |||
| 74 | + | signalGroup(cmd, pid) | |||
| 75 | + | if err := <-observed; err != nil { | |||
| 76 | + | werr = err // after the signals: reported, nothing more to do | |||
| 77 | + | } | |||
| 78 | + | } | |||
| 79 | + | } | |||
| 80 | + | if werr != nil { | |||
| 81 | + | _ = cmd.Process.Kill() | |||
| 82 | + | } | |||
| 83 | + | err := cmd.Wait() | |||
| 84 | + | if werr != nil { | |||
| 85 | + | return fmt.Errorf("waiting for the process %d: %w", pid, werr) | |||
| 86 | + | } | |||
| 87 | + | return err | |||
| 88 | + | } | |||
| 89 | + | ||||
| 90 | + | // signalGroup kills the group of the leader, and the leader alone should it | |||
| 91 | + | // have left the group. The leader must not be reaped yet. | |||
| 92 | + | func signalGroup(cmd *exec.Cmd, pid int) { | |||
| 93 | + | _ = killGroup(pid) | |||
| 94 | + | _ = cmd.Process.Kill() | |||
| 95 | + | } | |||
| 96 | + | ||||
| 97 | + | // waitExit returns when the child pid has exited, without reaping it, or the | |||
| 98 | + | // error of waitid. | |||
| 99 | + | func waitExit(pid int) error { | |||
| 100 | + | var info unix.Siginfo | |||
| 101 | + | for { | |||
| 102 | + | err := waitid(unix.P_PID, pid, &info, unix.WEXITED|unix.WNOWAIT, nil) | |||
| 103 | + | if !errors.Is(err, unix.EINTR) { | |||
| 104 | + | return err | |||
| 105 | + | } | |||
| 106 | + | } | |||
| 107 | + | } | |||
A internal/adapters/gitremote/run_linux_test.go +113 -0
| @@ -0,0 +1,113 @@ | |||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |
| 2 | + | ||
| 3 | + | //go:build linux | |
| 4 | + | ||
| 5 | + | package gitremote | |
| 6 | + | ||
| 7 | + | import ( | |
| 8 | + | "context" | |
| 9 | + | "os" | |
| 10 | + | "os/exec" | |
| 11 | + | "path/filepath" | |
| 12 | + | "strconv" | |
| 13 | + | "strings" | |
| 14 | + | "sync" | |
| 15 | + | "syscall" | |
| 16 | + | "testing" | |
| 17 | + | "time" | |
| 18 | + | ||
| 19 | + | "github.com/stretchr/testify/assert" | |
| 20 | + | "github.com/stretchr/testify/require" | |
| 21 | + | "golang.org/x/sys/unix" | |
| 22 | + | ) | |
| 23 | + | ||
| 24 | + | // recordKills replaces killGroup with one that records the groups it is asked | |
| 25 | + | // to kill, and does not kill them. | |
| 26 | + | func recordKills(t *testing.T, really bool) func() []int { | |
| 27 | + | t.Helper() | |
| 28 | + | var mu sync.Mutex | |
| 29 | + | var got []int | |
| 30 | + | old := killGroup | |
| 31 | + | killGroup = func(pgid int) error { | |
| 32 | + | mu.Lock() | |
| 33 | + | got = append(got, pgid) | |
| 34 | + | mu.Unlock() | |
| 35 | + | if really { | |
| 36 | + | return old(pgid) | |
| 37 | + | } | |
| 38 | + | return nil | |
| 39 | + | } | |
| 40 | + | t.Cleanup(func() { killGroup = old }) | |
| 41 | + | return func() []int { | |
| 42 | + | mu.Lock() | |
| 43 | + | defer mu.Unlock() | |
| 44 | + | return append([]int(nil), got...) | |
| 45 | + | } | |
| 46 | + | } | |
| 47 | + | ||
| 48 | + | func helperScript(t *testing.T) (cmd *exec.Cmd, pidFile string) { | |
| 49 | + | t.Helper() | |
| 50 | + | dir := t.TempDir() | |
| 51 | + | pidFile = filepath.Join(dir, "pid") | |
| 52 | + | // A leader with a helper in its group, which keeps its stdout open. | |
| 53 | + | script := "sleep 30 &\necho $! > '" + pidFile + "'\nwait\n" | |
| 54 | + | return exec.Command("/bin/sh", "-c", script), pidFile | |
| 55 | + | } | |
| 56 | + | ||
| 57 | + | func killPidFile(t *testing.T, pidFile string) { | |
| 58 | + | t.Helper() | |
| 59 | + | raw, err := os.ReadFile(pidFile) | |
| 60 | + | if err != nil { | |
| 61 | + | return | |
| 62 | + | } | |
| 63 | + | if pid, err := strconv.Atoi(strings.TrimSpace(string(raw))); err == nil { | |
| 64 | + | _ = syscall.Kill(pid, syscall.SIGKILL) | |
| 65 | + | } | |
| 66 | + | } | |
| 67 | + | ||
| 68 | + | // When waitid fails nothing proves that the leader is not reaped: the group | |
| 69 | + | // is not signalled, the leader alone is killed, and the error is returned. | |
| 70 | + | func TestRunDoesNotSignalTheGroupWhenWaitidFails(t *testing.T) { | |
| 71 | + | calls := recordKills(t, true) | |
| 72 | + | old := waitid | |
| 73 | + | cmd, pidFile := helperScript(t) | |
| 74 | + | // Fails once the helper is started, so that the test can tell it lives. | |
| 75 | + | waitid = func(int, int, *unix.Siginfo, int, *unix.Rusage) error { | |
| 76 | + | for i := 0; i < 200; i++ { | |
| 77 | + | if _, err := os.Stat(pidFile); err == nil { | |
| 78 | + | break | |
| 79 | + | } | |
| 80 | + | time.Sleep(10 * time.Millisecond) | |
| 81 | + | } | |
| 82 | + | return unix.ECHILD | |
| 83 | + | } | |
| 84 | + | t.Cleanup(func() { waitid = old }) | |
| 85 | + | t.Cleanup(func() { killPidFile(t, pidFile) }) | |
| 86 | + | cmd.WaitDelay = 100 * time.Millisecond | |
| 87 | + | start := time.Now() | |
| 88 | + | err := run(context.Background(), cmd) | |
| 89 | + | require.ErrorIs(t, err, unix.ECHILD) | |
| 90 | + | assert.Empty(t, calls(), "the group was signalled") | |
| 91 | + | assert.Less(t, time.Since(start), 2*time.Second) | |
| 92 | + | // The helper of the group was not killed. | |
| 93 | + | require.Eventually(t, func() bool { _, err := os.Stat(pidFile); return err == nil }, time.Second, 10*time.Millisecond) | |
| 94 | + | raw, _ := os.ReadFile(pidFile) | |
| 95 | + | pid, _ := strconv.Atoi(strings.TrimSpace(string(raw))) | |
| 96 | + | require.NoError(t, syscall.Kill(pid, 0), "the helper should still be alive") | |
| 97 | + | } | |
| 98 | + | ||
| 99 | + | // Same when the context is done: the group is signalled once, while the | |
| 100 | + | // leader is not reaped, and the leader is gone after run. | |
| 101 | + | func TestRunSignalsTheGroupBeforeTheLeaderIsReaped(t *testing.T) { | |
| 102 | + | calls := recordKills(t, true) | |
| 103 | + | cmd, pidFile := helperScript(t) | |
| 104 | + | t.Cleanup(func() { killPidFile(t, pidFile) }) | |
| 105 | + | cmd.WaitDelay = time.Second | |
| 106 | + | ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) | |
| 107 | + | defer cancel() | |
| 108 | + | require.Error(t, run(ctx, cmd)) | |
| 109 | + | got := calls() | |
| 110 | + | require.Len(t, got, 1) | |
| 111 | + | assert.Equal(t, cmd.Process.Pid, got[0]) | |
| 112 | + | assert.True(t, cmd.ProcessState.Exited() || cmd.ProcessState.Sys() != nil) | |
| 113 | + | } | |
| @@ -0,0 +1,113 @@ | |||||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |||
| 2 | + | ||||
| 3 | + | //go:build linux | |||
| 4 | + | ||||
| 5 | + | package gitremote | |||
| 6 | + | ||||
| 7 | + | import ( | |||
| 8 | + | "context" | |||
| 9 | + | "os" | |||
| 10 | + | "os/exec" | |||
| 11 | + | "path/filepath" | |||
| 12 | + | "strconv" | |||
| 13 | + | "strings" | |||
| 14 | + | "sync" | |||
| 15 | + | "syscall" | |||
| 16 | + | "testing" | |||
| 17 | + | "time" | |||
| 18 | + | ||||
| 19 | + | "github.com/stretchr/testify/assert" | |||
| 20 | + | "github.com/stretchr/testify/require" | |||
| 21 | + | "golang.org/x/sys/unix" | |||
| 22 | + | ) | |||
| 23 | + | ||||
| 24 | + | // recordKills replaces killGroup with one that records the groups it is asked | |||
| 25 | + | // to kill, and does not kill them. | |||
| 26 | + | func recordKills(t *testing.T, really bool) func() []int { | |||
| 27 | + | t.Helper() | |||
| 28 | + | var mu sync.Mutex | |||
| 29 | + | var got []int | |||
| 30 | + | old := killGroup | |||
| 31 | + | killGroup = func(pgid int) error { | |||
| 32 | + | mu.Lock() | |||
| 33 | + | got = append(got, pgid) | |||
| 34 | + | mu.Unlock() | |||
| 35 | + | if really { | |||
| 36 | + | return old(pgid) | |||
| 37 | + | } | |||
| 38 | + | return nil | |||
| 39 | + | } | |||
| 40 | + | t.Cleanup(func() { killGroup = old }) | |||
| 41 | + | return func() []int { | |||
| 42 | + | mu.Lock() | |||
| 43 | + | defer mu.Unlock() | |||
| 44 | + | return append([]int(nil), got...) | |||
| 45 | + | } | |||
| 46 | + | } | |||
| 47 | + | ||||
| 48 | + | func helperScript(t *testing.T) (cmd *exec.Cmd, pidFile string) { | |||
| 49 | + | t.Helper() | |||
| 50 | + | dir := t.TempDir() | |||
| 51 | + | pidFile = filepath.Join(dir, "pid") | |||
| 52 | + | // A leader with a helper in its group, which keeps its stdout open. | |||
| 53 | + | script := "sleep 30 &\necho $! > '" + pidFile + "'\nwait\n" | |||
| 54 | + | return exec.Command("/bin/sh", "-c", script), pidFile | |||
| 55 | + | } | |||
| 56 | + | ||||
| 57 | + | func killPidFile(t *testing.T, pidFile string) { | |||
| 58 | + | t.Helper() | |||
| 59 | + | raw, err := os.ReadFile(pidFile) | |||
| 60 | + | if err != nil { | |||
| 61 | + | return | |||
| 62 | + | } | |||
| 63 | + | if pid, err := strconv.Atoi(strings.TrimSpace(string(raw))); err == nil { | |||
| 64 | + | _ = syscall.Kill(pid, syscall.SIGKILL) | |||
| 65 | + | } | |||
| 66 | + | } | |||
| 67 | + | ||||
| 68 | + | // When waitid fails nothing proves that the leader is not reaped: the group | |||
| 69 | + | // is not signalled, the leader alone is killed, and the error is returned. | |||
| 70 | + | func TestRunDoesNotSignalTheGroupWhenWaitidFails(t *testing.T) { | |||
| 71 | + | calls := recordKills(t, true) | |||
| 72 | + | old := waitid | |||
| 73 | + | cmd, pidFile := helperScript(t) | |||
| 74 | + | // Fails once the helper is started, so that the test can tell it lives. | |||
| 75 | + | waitid = func(int, int, *unix.Siginfo, int, *unix.Rusage) error { | |||
| 76 | + | for i := 0; i < 200; i++ { | |||
| 77 | + | if _, err := os.Stat(pidFile); err == nil { | |||
| 78 | + | break | |||
| 79 | + | } | |||
| 80 | + | time.Sleep(10 * time.Millisecond) | |||
| 81 | + | } | |||
| 82 | + | return unix.ECHILD | |||
| 83 | + | } | |||
| 84 | + | t.Cleanup(func() { waitid = old }) | |||
| 85 | + | t.Cleanup(func() { killPidFile(t, pidFile) }) | |||
| 86 | + | cmd.WaitDelay = 100 * time.Millisecond | |||
| 87 | + | start := time.Now() | |||
| 88 | + | err := run(context.Background(), cmd) | |||
| 89 | + | require.ErrorIs(t, err, unix.ECHILD) | |||
| 90 | + | assert.Empty(t, calls(), "the group was signalled") | |||
| 91 | + | assert.Less(t, time.Since(start), 2*time.Second) | |||
| 92 | + | // The helper of the group was not killed. | |||
| 93 | + | require.Eventually(t, func() bool { _, err := os.Stat(pidFile); return err == nil }, time.Second, 10*time.Millisecond) | |||
| 94 | + | raw, _ := os.ReadFile(pidFile) | |||
| 95 | + | pid, _ := strconv.Atoi(strings.TrimSpace(string(raw))) | |||
| 96 | + | require.NoError(t, syscall.Kill(pid, 0), "the helper should still be alive") | |||
| 97 | + | } | |||
| 98 | + | ||||
| 99 | + | // Same when the context is done: the group is signalled once, while the | |||
| 100 | + | // leader is not reaped, and the leader is gone after run. | |||
| 101 | + | func TestRunSignalsTheGroupBeforeTheLeaderIsReaped(t *testing.T) { | |||
| 102 | + | calls := recordKills(t, true) | |||
| 103 | + | cmd, pidFile := helperScript(t) | |||
| 104 | + | t.Cleanup(func() { killPidFile(t, pidFile) }) | |||
| 105 | + | cmd.WaitDelay = time.Second | |||
| 106 | + | ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) | |||
| 107 | + | defer cancel() | |||
| 108 | + | require.Error(t, run(ctx, cmd)) | |||
| 109 | + | got := calls() | |||
| 110 | + | require.Len(t, got, 1) | |||
| 111 | + | assert.Equal(t, cmd.Process.Pid, got[0]) | |||
| 112 | + | assert.True(t, cmd.ProcessState.Exited() || cmd.ProcessState.Sys() != nil) | |||
| 113 | + | } | |||
A internal/adapters/gitremote/run_other.go +33 -0
| @@ -0,0 +1,33 @@ | |||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |
| 2 | + | ||
| 3 | + | //go:build !linux | |
| 4 | + | ||
| 5 | + | package gitremote | |
| 6 | + | ||
| 7 | + | import ( | |
| 8 | + | "context" | |
| 9 | + | "os/exec" | |
| 10 | + | ) | |
| 11 | + | ||
| 12 | + | // run starts cmd and waits for it, killing the process (not its group: only | |
| 13 | + | // Linux has the guarantee that makes killing a group safe, see run_linux.go) | |
| 14 | + | // when ctx is done. cmd.WaitDelay stays the backstop for helpers that hold the | |
| 15 | + | // pipes. | |
| 16 | + | func run(ctx context.Context, cmd *exec.Cmd) error { | |
| 17 | + | if err := ctx.Err(); err != nil { | |
| 18 | + | return err | |
| 19 | + | } | |
| 20 | + | if err := cmd.Start(); err != nil { | |
| 21 | + | return err | |
| 22 | + | } | |
| 23 | + | done := make(chan struct{}) | |
| 24 | + | defer close(done) | |
| 25 | + | go func() { | |
| 26 | + | select { | |
| 27 | + | case <-ctx.Done(): | |
| 28 | + | _ = cmd.Process.Kill() | |
| 29 | + | case <-done: | |
| 30 | + | } | |
| 31 | + | }() | |
| 32 | + | return cmd.Wait() | |
| 33 | + | } | |
| @@ -0,0 +1,33 @@ | |||||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |||
| 2 | + | ||||
| 3 | + | //go:build !linux | |||
| 4 | + | ||||
| 5 | + | package gitremote | |||
| 6 | + | ||||
| 7 | + | import ( | |||
| 8 | + | "context" | |||
| 9 | + | "os/exec" | |||
| 10 | + | ) | |||
| 11 | + | ||||
| 12 | + | // run starts cmd and waits for it, killing the process (not its group: only | |||
| 13 | + | // Linux has the guarantee that makes killing a group safe, see run_linux.go) | |||
| 14 | + | // when ctx is done. cmd.WaitDelay stays the backstop for helpers that hold the | |||
| 15 | + | // pipes. | |||
| 16 | + | func run(ctx context.Context, cmd *exec.Cmd) error { | |||
| 17 | + | if err := ctx.Err(); err != nil { | |||
| 18 | + | return err | |||
| 19 | + | } | |||
| 20 | + | if err := cmd.Start(); err != nil { | |||
| 21 | + | return err | |||
| 22 | + | } | |||
| 23 | + | done := make(chan struct{}) | |||
| 24 | + | defer close(done) | |||
| 25 | + | go func() { | |||
| 26 | + | select { | |||
| 27 | + | case <-ctx.Done(): | |||
| 28 | + | _ = cmd.Process.Kill() | |||
| 29 | + | case <-done: | |||
| 30 | + | } | |||
| 31 | + | }() | |||
| 32 | + | return cmd.Wait() | |||
| 33 | + | } | |||