fix(executor): a preparation broken by the step's cancellation or timeout ends the step cancelled, not failed
- commit
- e707c63986c05710da2b1601c67c83ef68ae9831
- author
- brice · · 2026-10-14 03:37 UTC
- parent
- 5f78aaa
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +162 -11
M internal/executor/actions_cancel_test.go +129 -0
| @@ -3,9 +3,13 @@ | |||
| 3 | 3 | package executor | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "context" | |
| 7 | + | "net/http" | |
| 8 | + | "net/http/httptest" | |
| 6 | 9 | "os" | |
| 7 | 10 | "path/filepath" | |
| 8 | 11 | "strings" | |
| 12 | + | "sync" | |
| 9 | 13 | "testing" | |
| 10 | 14 | "time" | |
| 11 | 15 | ) | |
| @@ -59,3 +63,128 @@ func TestLocalCompositeSetupIsCancellable(t *testing.T) { | |||
| 59 | 63 | t.Errorf("always() step %s", got) | |
| 60 | 64 | } | |
| 61 | 65 | } | |
| 66 | + | ||
| 67 | + | // GO-014: what the timeout or the cancellation of a step breaks while a local | |
| 68 | + | // composite action is prepared (a resolution, a download, an image pull and its | |
| 69 | + | // retry sleep) ends the step as the cancellation of the step does: the timeout | |
| 70 | + | // message and a failed step for timeout-minutes, a cancelled step for a user | |
| 71 | + | // cancel, and no stray "The operation was canceled.". | |
| 72 | + | func TestPreparationInterruptedByTheStepTimeout(t *testing.T) { | |
| 73 | + | skew := -(60*time.Second - 1500*time.Millisecond) | |
| 74 | + | leaf := files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: o/leaf@v1\n"} | |
| 75 | + | docker := files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: docker://alpine:3\n"} | |
| 76 | + | hang := make(chan struct{}) | |
| 77 | + | slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 78 | + | select { | |
| 79 | + | case <-r.Context().Done(): | |
| 80 | + | case <-hang: | |
| 81 | + | } | |
| 82 | + | })) | |
| 83 | + | defer slow.Close() | |
| 84 | + | defer close(hang) | |
| 85 | + | failDocker := filepath.Join(t.TempDir(), "docker") | |
| 86 | + | writeFiles(t, filepath.Dir(failDocker), files{"docker": "#!/bin/sh\necho pull failed\nexit 1\n"}) | |
| 87 | + | ||
| 88 | + | for name, tc := range map[string]struct { | |
| 89 | + | tree files | |
| 90 | + | tweak func(*Config) | |
| 91 | + | }{ | |
| 92 | + | "blocked resolver": {leaf, func(c *Config) { | |
| 93 | + | c.Actions.Resolver = func(ctx context.Context, _ []string) ([]ResolvedAction, error) { | |
| 94 | + | <-ctx.Done() | |
| 95 | + | return nil, ctx.Err() | |
| 96 | + | } | |
| 97 | + | }}, | |
| 98 | + | "blocked download": {leaf, func(c *Config) { | |
| 99 | + | c.Actions.Resolver = func(context.Context, []string) ([]ResolvedAction, error) { | |
| 100 | + | return []ResolvedAction{{Ref: "o/leaf@v1", NameWithOwner: "o/leaf", ResolvedSHA: strings.Repeat("a", 40), TarballURL: slow.URL + "/t"}}, nil | |
| 101 | + | } | |
| 102 | + | }}, | |
| 103 | + | "docker pull retry sleep": {docker, func(c *Config) { | |
| 104 | + | c.Actions.DockerPath = failDocker | |
| 105 | + | c.Actions.Backoff = func() time.Duration { return time.Hour } | |
| 106 | + | }}, | |
| 107 | + | } { | |
| 108 | + | t.Run(name, func(t *testing.T) { | |
| 109 | + | start := time.Now() | |
| 110 | + | rr, res := actionResults(t, tc.tree, " - name: comp\n timeout-minutes: 1\n uses: ./c\n - name: after\n if: always()\n run: echo after-ran\n", func(c *Config) { | |
| 111 | + | c.StepTimeoutSkew = skew | |
| 112 | + | tc.tweak(c) | |
| 113 | + | }) | |
| 114 | + | if time.Since(start) > 15*time.Second { | |
| 115 | + | t.Fatalf("the preparation was not interrupted: %v", time.Since(start)) | |
| 116 | + | } | |
| 117 | + | comp := rr.step(t, "comp") | |
| 118 | + | log := logOf(comp) | |
| 119 | + | if res.Conclusion != ConclusionFailure || comp.Conclusion != ConclusionFailure { | |
| 120 | + | t.Fatalf("job %s, step %s\n%s", res.Conclusion, comp.Conclusion, log) | |
| 121 | + | } | |
| 122 | + | if !strings.Contains(log, "##[error]The action 'comp' has timed out after 1 minutes.") { | |
| 123 | + | t.Errorf("no timeout message:\n%s", log) | |
| 124 | + | } | |
| 125 | + | if strings.Contains(log, "The operation was canceled.") { | |
| 126 | + | t.Errorf("a stray cancel text:\n%s", log) | |
| 127 | + | } | |
| 128 | + | if rr.step(t, "after").Conclusion != ConclusionSuccess { | |
| 129 | + | t.Error("the job is not cancelled by a step timeout") | |
| 130 | + | } | |
| 131 | + | }) | |
| 132 | + | } | |
| 133 | + | } | |
| 134 | + | ||
| 135 | + | func TestPreparationInterruptedByAUserCancel(t *testing.T) { | |
| 136 | + | needTools(t, "bash") | |
| 137 | + | entered := make(chan struct{}) | |
| 138 | + | var once sync.Once | |
| 139 | + | home := t.TempDir() | |
| 140 | + | layout := NewLayout(home, "hello", home+"/tc") | |
| 141 | + | writeFiles(t, layout.Workspace, files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: o/leaf@v1\n"}) | |
| 142 | + | c := NewCanceller() | |
| 143 | + | go func() { | |
| 144 | + | <-entered | |
| 145 | + | c.Cancel(CancelUser) | |
| 146 | + | }() | |
| 147 | + | rec := newRecorder() | |
| 148 | + | done := make(chan Result, 1) | |
| 149 | + | go func() { | |
| 150 | + | done <- Run(c, Config{ | |
| 151 | + | WorkflowYAML: []byte(wfHead + " steps:\n - name: comp\n uses: ./c\n - name: cleanup\n if: always()\n run: echo cleanup-ran\n"), | |
| 152 | + | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rec, | |
| 153 | + | BaseEnv: []string{"PATH=" + os.Getenv("PATH"), "HOME=" + home}, | |
| 154 | + | TermAfter: 200 * time.Millisecond, KillAfter: 200 * time.Millisecond, WaitDelay: 500 * time.Millisecond, | |
| 155 | + | Actions: ActionsConfig{Resolver: func(ctx context.Context, _ []string) ([]ResolvedAction, error) { | |
| 156 | + | once.Do(func() { close(entered) }) | |
| 157 | + | <-ctx.Done() | |
| 158 | + | return nil, ctx.Err() | |
| 159 | + | }}, | |
| 160 | + | }) | |
| 161 | + | }() | |
| 162 | + | select { | |
| 163 | + | case res := <-done: | |
| 164 | + | if res.Conclusion != ConclusionCancelled { | |
| 165 | + | t.Fatalf("%+v", res) | |
| 166 | + | } | |
| 167 | + | case <-time.After(20 * time.Second): | |
| 168 | + | t.Fatal("the resolution ignored the cancellation") | |
| 169 | + | } | |
| 170 | + | if got := rec.step(t, "comp").Conclusion; got != ConclusionCancelled { | |
| 171 | + | t.Errorf("step %s, want cancelled", got) | |
| 172 | + | } | |
| 173 | + | if got := rec.step(t, "cleanup").Conclusion; got != ConclusionSuccess { | |
| 174 | + | t.Errorf("always() step %s", got) | |
| 175 | + | } | |
| 176 | + | } | |
| 177 | + | ||
| 178 | + | // A genuine error of the preparation still fails the step. | |
| 179 | + | func TestPreparationErrorStillFailsTheStep(t *testing.T) { | |
| 180 | + | tree := files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: o/leaf@v1\n"} | |
| 181 | + | rr, res := actionResults(t, tree, " - name: comp\n uses: ./c\n", func(c *Config) { | |
| 182 | + | c.Actions.Resolver = func(context.Context, []string) ([]ResolvedAction, error) { | |
| 183 | + | return []ResolvedAction{{Ref: "o/leaf@v1", Error: "Unable to resolve action `o/leaf@v1`, repository not found: `o/leaf`"}}, nil | |
| 184 | + | } | |
| 185 | + | }) | |
| 186 | + | comp := rr.step(t, "comp") | |
| 187 | + | if res.Conclusion != ConclusionFailure || comp.Conclusion != ConclusionFailure || !strings.Contains(logOf(comp), "##[error]Unable to resolve action `o/leaf@v1`, repository not found: `o/leaf`") { | |
| 188 | + | t.Errorf("%s %s\n%s", res.Conclusion, comp.Conclusion, logOf(comp)) | |
| 189 | + | } | |
| 190 | + | } | |
| @@ -3,9 +3,13 @@ | |||||
| 3 | package executor | 3 | package executor | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "context" | |||
| 7 | + | "net/http" | |||
| 8 | + | "net/http/httptest" | |||
| 6 | "os" | 9 | "os" | ||
| 7 | "path/filepath" | 10 | "path/filepath" | ||
| 8 | "strings" | 11 | "strings" | ||
| 12 | + | "sync" | |||
| 9 | "testing" | 13 | "testing" | ||
| 10 | "time" | 14 | "time" | ||
| 11 | ) | 15 | ) | ||
| @@ -59,3 +63,128 @@ func TestLocalCompositeSetupIsCancellable(t *testing.T) { | |||||
| 59 | t.Errorf("always() step %s", got) | 63 | t.Errorf("always() step %s", got) | ||
| 60 | } | 64 | } | ||
| 61 | } | 65 | } | ||
| 66 | + | ||||
| 67 | + | // GO-014: what the timeout or the cancellation of a step breaks while a local | |||
| 68 | + | // composite action is prepared (a resolution, a download, an image pull and its | |||
| 69 | + | // retry sleep) ends the step as the cancellation of the step does: the timeout | |||
| 70 | + | // message and a failed step for timeout-minutes, a cancelled step for a user | |||
| 71 | + | // cancel, and no stray "The operation was canceled.". | |||
| 72 | + | func TestPreparationInterruptedByTheStepTimeout(t *testing.T) { | |||
| 73 | + | skew := -(60*time.Second - 1500*time.Millisecond) | |||
| 74 | + | leaf := files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: o/leaf@v1\n"} | |||
| 75 | + | docker := files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: docker://alpine:3\n"} | |||
| 76 | + | hang := make(chan struct{}) | |||
| 77 | + | slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 78 | + | select { | |||
| 79 | + | case <-r.Context().Done(): | |||
| 80 | + | case <-hang: | |||
| 81 | + | } | |||
| 82 | + | })) | |||
| 83 | + | defer slow.Close() | |||
| 84 | + | defer close(hang) | |||
| 85 | + | failDocker := filepath.Join(t.TempDir(), "docker") | |||
| 86 | + | writeFiles(t, filepath.Dir(failDocker), files{"docker": "#!/bin/sh\necho pull failed\nexit 1\n"}) | |||
| 87 | + | ||||
| 88 | + | for name, tc := range map[string]struct { | |||
| 89 | + | tree files | |||
| 90 | + | tweak func(*Config) | |||
| 91 | + | }{ | |||
| 92 | + | "blocked resolver": {leaf, func(c *Config) { | |||
| 93 | + | c.Actions.Resolver = func(ctx context.Context, _ []string) ([]ResolvedAction, error) { | |||
| 94 | + | <-ctx.Done() | |||
| 95 | + | return nil, ctx.Err() | |||
| 96 | + | } | |||
| 97 | + | }}, | |||
| 98 | + | "blocked download": {leaf, func(c *Config) { | |||
| 99 | + | c.Actions.Resolver = func(context.Context, []string) ([]ResolvedAction, error) { | |||
| 100 | + | return []ResolvedAction{{Ref: "o/leaf@v1", NameWithOwner: "o/leaf", ResolvedSHA: strings.Repeat("a", 40), TarballURL: slow.URL + "/t"}}, nil | |||
| 101 | + | } | |||
| 102 | + | }}, | |||
| 103 | + | "docker pull retry sleep": {docker, func(c *Config) { | |||
| 104 | + | c.Actions.DockerPath = failDocker | |||
| 105 | + | c.Actions.Backoff = func() time.Duration { return time.Hour } | |||
| 106 | + | }}, | |||
| 107 | + | } { | |||
| 108 | + | t.Run(name, func(t *testing.T) { | |||
| 109 | + | start := time.Now() | |||
| 110 | + | rr, res := actionResults(t, tc.tree, " - name: comp\n timeout-minutes: 1\n uses: ./c\n - name: after\n if: always()\n run: echo after-ran\n", func(c *Config) { | |||
| 111 | + | c.StepTimeoutSkew = skew | |||
| 112 | + | tc.tweak(c) | |||
| 113 | + | }) | |||
| 114 | + | if time.Since(start) > 15*time.Second { | |||
| 115 | + | t.Fatalf("the preparation was not interrupted: %v", time.Since(start)) | |||
| 116 | + | } | |||
| 117 | + | comp := rr.step(t, "comp") | |||
| 118 | + | log := logOf(comp) | |||
| 119 | + | if res.Conclusion != ConclusionFailure || comp.Conclusion != ConclusionFailure { | |||
| 120 | + | t.Fatalf("job %s, step %s\n%s", res.Conclusion, comp.Conclusion, log) | |||
| 121 | + | } | |||
| 122 | + | if !strings.Contains(log, "##[error]The action 'comp' has timed out after 1 minutes.") { | |||
| 123 | + | t.Errorf("no timeout message:\n%s", log) | |||
| 124 | + | } | |||
| 125 | + | if strings.Contains(log, "The operation was canceled.") { | |||
| 126 | + | t.Errorf("a stray cancel text:\n%s", log) | |||
| 127 | + | } | |||
| 128 | + | if rr.step(t, "after").Conclusion != ConclusionSuccess { | |||
| 129 | + | t.Error("the job is not cancelled by a step timeout") | |||
| 130 | + | } | |||
| 131 | + | }) | |||
| 132 | + | } | |||
| 133 | + | } | |||
| 134 | + | ||||
| 135 | + | func TestPreparationInterruptedByAUserCancel(t *testing.T) { | |||
| 136 | + | needTools(t, "bash") | |||
| 137 | + | entered := make(chan struct{}) | |||
| 138 | + | var once sync.Once | |||
| 139 | + | home := t.TempDir() | |||
| 140 | + | layout := NewLayout(home, "hello", home+"/tc") | |||
| 141 | + | writeFiles(t, layout.Workspace, files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: o/leaf@v1\n"}) | |||
| 142 | + | c := NewCanceller() | |||
| 143 | + | go func() { | |||
| 144 | + | <-entered | |||
| 145 | + | c.Cancel(CancelUser) | |||
| 146 | + | }() | |||
| 147 | + | rec := newRecorder() | |||
| 148 | + | done := make(chan Result, 1) | |||
| 149 | + | go func() { | |||
| 150 | + | done <- Run(c, Config{ | |||
| 151 | + | WorkflowYAML: []byte(wfHead + " steps:\n - name: comp\n uses: ./c\n - name: cleanup\n if: always()\n run: echo cleanup-ran\n"), | |||
| 152 | + | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rec, | |||
| 153 | + | BaseEnv: []string{"PATH=" + os.Getenv("PATH"), "HOME=" + home}, | |||
| 154 | + | TermAfter: 200 * time.Millisecond, KillAfter: 200 * time.Millisecond, WaitDelay: 500 * time.Millisecond, | |||
| 155 | + | Actions: ActionsConfig{Resolver: func(ctx context.Context, _ []string) ([]ResolvedAction, error) { | |||
| 156 | + | once.Do(func() { close(entered) }) | |||
| 157 | + | <-ctx.Done() | |||
| 158 | + | return nil, ctx.Err() | |||
| 159 | + | }}, | |||
| 160 | + | }) | |||
| 161 | + | }() | |||
| 162 | + | select { | |||
| 163 | + | case res := <-done: | |||
| 164 | + | if res.Conclusion != ConclusionCancelled { | |||
| 165 | + | t.Fatalf("%+v", res) | |||
| 166 | + | } | |||
| 167 | + | case <-time.After(20 * time.Second): | |||
| 168 | + | t.Fatal("the resolution ignored the cancellation") | |||
| 169 | + | } | |||
| 170 | + | if got := rec.step(t, "comp").Conclusion; got != ConclusionCancelled { | |||
| 171 | + | t.Errorf("step %s, want cancelled", got) | |||
| 172 | + | } | |||
| 173 | + | if got := rec.step(t, "cleanup").Conclusion; got != ConclusionSuccess { | |||
| 174 | + | t.Errorf("always() step %s", got) | |||
| 175 | + | } | |||
| 176 | + | } | |||
| 177 | + | ||||
| 178 | + | // A genuine error of the preparation still fails the step. | |||
| 179 | + | func TestPreparationErrorStillFailsTheStep(t *testing.T) { | |||
| 180 | + | tree := files{"c/action.yml": "name: c\nruns:\n using: composite\n steps:\n - uses: o/leaf@v1\n"} | |||
| 181 | + | rr, res := actionResults(t, tree, " - name: comp\n uses: ./c\n", func(c *Config) { | |||
| 182 | + | c.Actions.Resolver = func(context.Context, []string) ([]ResolvedAction, error) { | |||
| 183 | + | return []ResolvedAction{{Ref: "o/leaf@v1", Error: "Unable to resolve action `o/leaf@v1`, repository not found: `o/leaf`"}}, nil | |||
| 184 | + | } | |||
| 185 | + | }) | |||
| 186 | + | comp := rr.step(t, "comp") | |||
| 187 | + | if res.Conclusion != ConclusionFailure || comp.Conclusion != ConclusionFailure || !strings.Contains(logOf(comp), "##[error]Unable to resolve action `o/leaf@v1`, repository not found: `o/leaf`") { | |||
| 188 | + | t.Errorf("%s %s\n%s", res.Conclusion, comp.Conclusion, logOf(comp)) | |||
| 189 | + | } | |||
| 190 | + | } | |||
M internal/executor/actions_composite.go +5 -3
| @@ -59,9 +59,11 @@ func (j *job) prepareLocalComposite(s *stepRun, def *actionDef, cancel <-chan st | |||
| 59 | 59 | saved := *j.actions | |
| 60 | 60 | j.actions.pullOrder, j.actions.buildOrder = nil, nil | |
| 61 | 61 | j.actions.pullUsers, j.actions.buildUsers = map[string][]*node{}, map[string][]*node{} | |
| 62 | - | // The preparation stops with the step: its context ends when the step is | |
| 63 | - | // cancelled (and with the job, whose context it derives from). | |
| 64 | - | ctx, stop := context.WithCancel(j.ctx()) | |
| 62 | + | // The preparation stops with the step, and only with it: its context ends when | |
| 63 | + | // the cancel channel of the step closes (job cancellation for a step that does | |
| 64 | + | // not run on always(), timeout-minutes). A step that keeps running after the | |
| 65 | + | // job was cancelled keeps the means to prepare its composite. | |
| 66 | + | ctx, stop := context.WithCancel(context.Background()) | |
| 65 | 67 | defer stop() | |
| 66 | 68 | if cancel != nil { | |
| 67 | 69 | go func() { | |
| @@ -59,9 +59,11 @@ func (j *job) prepareLocalComposite(s *stepRun, def *actionDef, cancel <-chan st | |||||
| 59 | saved := *j.actions | 59 | saved := *j.actions | ||
| 60 | j.actions.pullOrder, j.actions.buildOrder = nil, nil | 60 | j.actions.pullOrder, j.actions.buildOrder = nil, nil | ||
| 61 | j.actions.pullUsers, j.actions.buildUsers = map[string][]*node{}, map[string][]*node{} | 61 | j.actions.pullUsers, j.actions.buildUsers = map[string][]*node{}, map[string][]*node{} | ||
| 62 | - | // The preparation stops with the step: its context ends when the step is | 62 | + | // The preparation stops with the step, and only with it: its context ends when |
| 63 | - | // cancelled (and with the job, whose context it derives from). | 63 | + | // the cancel channel of the step closes (job cancellation for a step that does |
| 64 | - | ctx, stop := context.WithCancel(j.ctx()) | 64 | + | // not run on always(), timeout-minutes). A step that keeps running after the |
| 65 | + | // job was cancelled keeps the means to prepare its composite. | |||
| 66 | + | ctx, stop := context.WithCancel(context.Background()) | |||
| 65 | defer stop() | 67 | defer stop() | ||
| 66 | if cancel != nil { | 68 | if cancel != nil { | ||
| 67 | go func() { | 69 | go func() { | ||
M internal/executor/actions_docker.go +16 -4
| @@ -136,7 +136,7 @@ func (j *job) runDocker(s *stepRun, def *actionDef, in *inputSet, env *ordered, | |||
| 136 | 136 | j.out(num, "##[endgroup]") | |
| 137 | 137 | if err != nil { | |
| 138 | 138 | if err == errCancelled { | |
| 139 | - | return j.cancelledStep(num) | |
| 139 | + | return j.cancelledStep(s) | |
| 140 | 140 | } | |
| 141 | 141 | return fail(err) | |
| 142 | 142 | } | |
| @@ -265,12 +265,24 @@ func (j *job) dockerArgs(r *action.Runs, in *inputSet, te template.Env) (line st | |||
| 265 | 265 | return raw, splitArgs(raw), nil | |
| 266 | 266 | } | |
| 267 | 267 | ||
| 268 | - | // cancelledStep logs the cancellation of a step that was running docker. | |
| 269 | - | func (j *job) cancelledStep(num int) stepResult { | |
| 270 | - | j.errorf(num, "The operation was canceled.") | |
| 268 | + | // cancelledStep logs the cancellation of a step that was running docker, unless | |
| 269 | + | // the step timed out: then StepsRunner logs the timeout message instead | |
| 270 | + | // (stepRun.logCanceled). | |
| 271 | + | func (j *job) cancelledStep(s *stepRun) stepResult { | |
| 272 | + | s.logCanceled() | |
| 271 | 273 | return resCanceled | |
| 272 | 274 | } | |
| 273 | 275 | ||
| 276 | + | // done reports whether a cancellation channel is closed (a nil one never is). | |
| 277 | + | func done(c <-chan struct{}) bool { | |
| 278 | + | select { | |
| 279 | + | case <-c: | |
| 280 | + | return true | |
| 281 | + | default: | |
| 282 | + | return false | |
| 283 | + | } | |
| 284 | + | } | |
| 285 | + | ||
| 274 | 286 | func shortID() string { return strings.ReplaceAll(newImageTag(""), ":", "")[:6] } | |
| 275 | 287 | ||
| 276 | 288 | // dockerProcessEnv is the environment of the docker CLI: the one of the runner | |
| @@ -136,7 +136,7 @@ func (j *job) runDocker(s *stepRun, def *actionDef, in *inputSet, env *ordered, | |||||
| 136 | j.out(num, "##[endgroup]") | 136 | j.out(num, "##[endgroup]") | ||
| 137 | if err != nil { | 137 | if err != nil { | ||
| 138 | if err == errCancelled { | 138 | if err == errCancelled { | ||
| 139 | - | return j.cancelledStep(num) | 139 | + | return j.cancelledStep(s) |
| 140 | } | 140 | } | ||
| 141 | return fail(err) | 141 | return fail(err) | ||
| 142 | } | 142 | } | ||
| @@ -265,12 +265,24 @@ func (j *job) dockerArgs(r *action.Runs, in *inputSet, te template.Env) (line st | |||||
| 265 | return raw, splitArgs(raw), nil | 265 | return raw, splitArgs(raw), nil | ||
| 266 | } | 266 | } | ||
| 267 | 267 | ||||
| 268 | - | // cancelledStep logs the cancellation of a step that was running docker. | 268 | + | // cancelledStep logs the cancellation of a step that was running docker, unless |
| 269 | - | func (j *job) cancelledStep(num int) stepResult { | 269 | + | // the step timed out: then StepsRunner logs the timeout message instead |
| 270 | - | j.errorf(num, "The operation was canceled.") | 270 | + | // (stepRun.logCanceled). |
| 271 | + | func (j *job) cancelledStep(s *stepRun) stepResult { | |||
| 272 | + | s.logCanceled() | |||
| 271 | return resCanceled | 273 | return resCanceled | ||
| 272 | } | 274 | } | ||
| 273 | 275 | ||||
| 276 | + | // done reports whether a cancellation channel is closed (a nil one never is). | |||
| 277 | + | func done(c <-chan struct{}) bool { | |||
| 278 | + | select { | |||
| 279 | + | case <-c: | |||
| 280 | + | return true | |||
| 281 | + | default: | |||
| 282 | + | return false | |||
| 283 | + | } | |||
| 284 | + | } | |||
| 285 | + | ||||
| 274 | func shortID() string { return strings.ReplaceAll(newImageTag(""), ":", "")[:6] } | 286 | func shortID() string { return strings.ReplaceAll(newImageTag(""), ":", "")[:6] } | ||
| 275 | 287 | ||||
| 276 | // dockerProcessEnv is the environment of the docker CLI: the one of the runner | 288 | // dockerProcessEnv is the environment of the docker CLI: the one of the runner | ||
M internal/executor/actions_run.go +8 -0
| @@ -157,6 +157,14 @@ func (j *job) runAction(s *stepRun, cancel <-chan struct{}) stepResult { | |||
| 157 | 157 | if err == errSetupCancelled { | |
| 158 | 158 | return resCanceled // logged by the command that was stopped | |
| 159 | 159 | } | |
| 160 | + | // What the cancellation or the timeout of the step broke (a resolution, | |
| 161 | + | // a download, an extraction) is not a failure of the action: the step is | |
| 162 | + | // cancelled, and StepsRunner turns it into the timeout message when its | |
| 163 | + | // own timer fired. A genuine error keeps failing the step. | |
| 164 | + | if done(cancel) { | |
| 165 | + | s.logCanceled() | |
| 166 | + | return resCanceled | |
| 167 | + | } | |
| 160 | 168 | return fail(err) | |
| 161 | 169 | } | |
| 162 | 170 | } | |
| @@ -157,6 +157,14 @@ func (j *job) runAction(s *stepRun, cancel <-chan struct{}) stepResult { | |||||
| 157 | if err == errSetupCancelled { | 157 | if err == errSetupCancelled { | ||
| 158 | return resCanceled // logged by the command that was stopped | 158 | return resCanceled // logged by the command that was stopped | ||
| 159 | } | 159 | } | ||
| 160 | + | // What the cancellation or the timeout of the step broke (a resolution, | |||
| 161 | + | // a download, an extraction) is not a failure of the action: the step is | |||
| 162 | + | // cancelled, and StepsRunner turns it into the timeout message when its | |||
| 163 | + | // own timer fired. A genuine error keeps failing the step. | |||
| 164 | + | if done(cancel) { | |||
| 165 | + | s.logCanceled() | |||
| 166 | + | return resCanceled | |||
| 167 | + | } | |||
| 160 | return fail(err) | 168 | return fail(err) | ||
| 161 | } | 169 | } | ||
| 162 | } | 170 | } | ||
M internal/executor/actions_setup.go +4 -4
| @@ -46,7 +46,7 @@ func (j *job) pullImage(s *stepRun, image string, users []*node, cancel <-chan s | |||
| 46 | 46 | code, err = j.runDockerCmd(s, c, dirOrCwd(j.state.GitHub("workspace")), nil, cancel) | |
| 47 | 47 | if err == errCancelled { | |
| 48 | 48 | j.out(num, "##[endgroup]") | |
| 49 | - | return j.cancelledStep(num) | |
| 49 | + | return j.cancelledStep(s) | |
| 50 | 50 | } | |
| 51 | 51 | if err != nil { | |
| 52 | 52 | j.out(num, "##[endgroup]") | |
| @@ -61,7 +61,7 @@ func (j *job) pullImage(s *stepRun, image string, users []*node, cancel <-chan s | |||
| 61 | 61 | j.tagged(num, "##[warning]", fmt.Sprintf("Docker pull failed with exit code %d, back off %v seconds before retry.", code, d.Seconds())) | |
| 62 | 62 | if !j.sleepOrCancel(d, cancel) { | |
| 63 | 63 | j.out(num, "##[endgroup]") | |
| 64 | - | return j.cancelledStep(num) | |
| 64 | + | return j.cancelledStep(s) | |
| 65 | 65 | } | |
| 66 | 66 | } | |
| 67 | 67 | } | |
| @@ -88,7 +88,7 @@ func (j *job) buildActionImage(s *stepRun, info buildInfo, users []*node, cancel | |||
| 88 | 88 | code, err = j.runDockerCmd(s, c, dirOrCwd(info.workDir), nil, cancel) | |
| 89 | 89 | if err == errCancelled { | |
| 90 | 90 | j.out(num, "##[endgroup]") | |
| 91 | - | return j.cancelledStep(num) | |
| 91 | + | return j.cancelledStep(s) | |
| 92 | 92 | } | |
| 93 | 93 | if err != nil { | |
| 94 | 94 | j.out(num, "##[endgroup]") | |
| @@ -103,7 +103,7 @@ func (j *job) buildActionImage(s *stepRun, info buildInfo, users []*node, cancel | |||
| 103 | 103 | j.tagged(num, "##[warning]", fmt.Sprintf("Docker build failed with exit code %d, back off %v seconds before retry.", code, d.Seconds())) | |
| 104 | 104 | if !j.sleepOrCancel(d, cancel) { | |
| 105 | 105 | j.out(num, "##[endgroup]") | |
| 106 | - | return j.cancelledStep(num) | |
| 106 | + | return j.cancelledStep(s) | |
| 107 | 107 | } | |
| 108 | 108 | } | |
| 109 | 109 | } | |
| @@ -46,7 +46,7 @@ func (j *job) pullImage(s *stepRun, image string, users []*node, cancel <-chan s | |||||
| 46 | code, err = j.runDockerCmd(s, c, dirOrCwd(j.state.GitHub("workspace")), nil, cancel) | 46 | code, err = j.runDockerCmd(s, c, dirOrCwd(j.state.GitHub("workspace")), nil, cancel) | ||
| 47 | if err == errCancelled { | 47 | if err == errCancelled { | ||
| 48 | j.out(num, "##[endgroup]") | 48 | j.out(num, "##[endgroup]") | ||
| 49 | - | return j.cancelledStep(num) | 49 | + | return j.cancelledStep(s) |
| 50 | } | 50 | } | ||
| 51 | if err != nil { | 51 | if err != nil { | ||
| 52 | j.out(num, "##[endgroup]") | 52 | j.out(num, "##[endgroup]") | ||
| @@ -61,7 +61,7 @@ func (j *job) pullImage(s *stepRun, image string, users []*node, cancel <-chan s | |||||
| 61 | j.tagged(num, "##[warning]", fmt.Sprintf("Docker pull failed with exit code %d, back off %v seconds before retry.", code, d.Seconds())) | 61 | j.tagged(num, "##[warning]", fmt.Sprintf("Docker pull failed with exit code %d, back off %v seconds before retry.", code, d.Seconds())) | ||
| 62 | if !j.sleepOrCancel(d, cancel) { | 62 | if !j.sleepOrCancel(d, cancel) { | ||
| 63 | j.out(num, "##[endgroup]") | 63 | j.out(num, "##[endgroup]") | ||
| 64 | - | return j.cancelledStep(num) | 64 | + | return j.cancelledStep(s) |
| 65 | } | 65 | } | ||
| 66 | } | 66 | } | ||
| 67 | } | 67 | } | ||
| @@ -88,7 +88,7 @@ func (j *job) buildActionImage(s *stepRun, info buildInfo, users []*node, cancel | |||||
| 88 | code, err = j.runDockerCmd(s, c, dirOrCwd(info.workDir), nil, cancel) | 88 | code, err = j.runDockerCmd(s, c, dirOrCwd(info.workDir), nil, cancel) | ||
| 89 | if err == errCancelled { | 89 | if err == errCancelled { | ||
| 90 | j.out(num, "##[endgroup]") | 90 | j.out(num, "##[endgroup]") | ||
| 91 | - | return j.cancelledStep(num) | 91 | + | return j.cancelledStep(s) |
| 92 | } | 92 | } | ||
| 93 | if err != nil { | 93 | if err != nil { | ||
| 94 | j.out(num, "##[endgroup]") | 94 | j.out(num, "##[endgroup]") | ||
| @@ -103,7 +103,7 @@ func (j *job) buildActionImage(s *stepRun, info buildInfo, users []*node, cancel | |||||
| 103 | j.tagged(num, "##[warning]", fmt.Sprintf("Docker build failed with exit code %d, back off %v seconds before retry.", code, d.Seconds())) | 103 | j.tagged(num, "##[warning]", fmt.Sprintf("Docker build failed with exit code %d, back off %v seconds before retry.", code, d.Seconds())) | ||
| 104 | if !j.sleepOrCancel(d, cancel) { | 104 | if !j.sleepOrCancel(d, cancel) { | ||
| 105 | j.out(num, "##[endgroup]") | 105 | j.out(num, "##[endgroup]") | ||
| 106 | - | return j.cancelledStep(num) | 106 | + | return j.cancelledStep(s) |
| 107 | } | 107 | } | ||
| 108 | } | 108 | } | ||
| 109 | } | 109 | } | ||