fix(executor): signal the shell like the official runner (m2), bound a line without end (m3), read ACTIONS_ALLOW_* from the step environment (G7)
- commit
- fd7cbe10715ddfdb89df5065168fa554d0a40239
- author
- brice · · 2026-10-12 19:26 UTC
- parent
- 8c00265
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +127 -9
M internal/executor/job.go +1 -1
| @@ -75,7 +75,7 @@ type Config struct { | |||
| 75 | 75 | Credential *syscall.Credential | |
| 76 | 76 | // Cancellation timings and test hooks, see procSpec. | |
| 77 | 77 | TermAfter, KillAfter, WaitDelay time.Duration | |
| 78 | - | Signal func(pgid int, sig syscall.Signal) | |
| 78 | + | Signal func(pid int, sig syscall.Signal) | |
| 79 | 79 | // Now returns the current time; nil means time.Now. | |
| 80 | 80 | Now func() time.Time | |
| 81 | 81 | } | |
| @@ -75,7 +75,7 @@ type Config struct { | |||||
| 75 | Credential *syscall.Credential | 75 | Credential *syscall.Credential | ||
| 76 | // Cancellation timings and test hooks, see procSpec. | 76 | // Cancellation timings and test hooks, see procSpec. | ||
| 77 | TermAfter, KillAfter, WaitDelay time.Duration | 77 | TermAfter, KillAfter, WaitDelay time.Duration | ||
| 78 | - | Signal func(pgid int, sig syscall.Signal) | 78 | + | Signal func(pid int, sig syscall.Signal) |
| 79 | // Now returns the current time; nil means time.Now. | 79 | // Now returns the current time; nil means time.Now. | ||
| 80 | Now func() time.Time | 80 | Now func() time.Time | ||
| 81 | } | 81 | } | ||
M internal/executor/job_test.go +79 -2
| @@ -351,10 +351,10 @@ func TestCancelEscalatesToTermAndKill(t *testing.T) { | |||
| 351 | 351 | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rep, | |
| 352 | 352 | BaseEnv: []string{"PATH=" + envPath(), "HOME=" + home}, | |
| 353 | 353 | TermAfter: 250 * time.Millisecond, KillAfter: 250 * time.Millisecond, | |
| 354 | - | Signal: func(pgid int, sig syscall.Signal) { | |
| 354 | + | Signal: func(pid int, sig syscall.Signal) { | |
| 355 | 355 | got = append(got, sig) | |
| 356 | 356 | times = append(times, time.Since(t0)) | |
| 357 | - | _ = syscall.Kill(-pgid, sig) | |
| 357 | + | _ = syscall.Kill(pid, sig) | |
| 358 | 358 | mu <- struct{}{} | |
| 359 | 359 | }, | |
| 360 | 360 | }) | |
| @@ -524,3 +524,80 @@ func TestEventJSONIsNeverWrittenThroughALink(t *testing.T) { | |||
| 524 | 524 | t.Fatalf("the second step does not see a fresh event.json:\n%s", got) | |
| 525 | 525 | } | |
| 526 | 526 | } | |
| 527 | + | ||
| 528 | + | func TestLineWriterBoundsALineWithoutEnd(t *testing.T) { | |
| 529 | + | var got []string | |
| 530 | + | w := &lineWriter{fn: func(l string) { got = append(got, l) }} | |
| 531 | + | // 2.5 MiB of two-byte characters without a line feed, in odd-sized writes | |
| 532 | + | // that cut characters in two. | |
| 533 | + | blob := []byte(strings.Repeat("é", (5<<20)/4)) | |
| 534 | + | for i := 0; i < len(blob); i += 7777 { | |
| 535 | + | _, _ = w.Write(blob[i:min(i+7777, len(blob))]) | |
| 536 | + | } | |
| 537 | + | w.Flush() | |
| 538 | + | total := 0 | |
| 539 | + | for _, l := range got { | |
| 540 | + | if len(l) > maxLineBytes { | |
| 541 | + | t.Fatalf("a line of %d bytes", len(l)) | |
| 542 | + | } | |
| 543 | + | if strings.ContainsRune(l, '\ufffd') { | |
| 544 | + | t.Fatal("a character was cut in two") | |
| 545 | + | } | |
| 546 | + | total += len(l) | |
| 547 | + | } | |
| 548 | + | if len(got) < 3 || total != len(blob) { | |
| 549 | + | t.Fatalf("%d lines, %d bytes of %d", len(got), total, len(blob)) | |
| 550 | + | } | |
| 551 | + | } | |
| 552 | + | ||
| 553 | + | func TestEnvFlagReadsTheStepEnvironmentNotTheExecutors(t *testing.T) { | |
| 554 | + | // ACTIONS_ALLOW_UNSECURE_COMMANDS is read from the environment the steps | |
| 555 | + | // get (BaseEnv), not from the process of the executor. | |
| 556 | + | t.Setenv("ACTIONS_ALLOW_UNSECURE_COMMANDS", "true") | |
| 557 | + | needTools(t, "bash") | |
| 558 | + | rec, res := runLocal(t, wfHead+" steps:\n - run: echo '::set-env name=X::1'\n", "j", nil) | |
| 559 | + | if res.Conclusion != ConclusionFailure || !strings.Contains(logOf(rec.steps[1]), "The `set-env` command is disabled.") { | |
| 560 | + | t.Fatalf("%+v\n%s", res, logOf(rec.steps[1])) | |
| 561 | + | } | |
| 562 | + | rec, res = runLocal(t, wfHead+" steps:\n - run: echo '::set-env name=X::1'\n - run: echo \"X=$X\"\n", "j", func(c *Config) { | |
| 563 | + | c.BaseEnv = append(c.BaseEnv, "ACTIONS_ALLOW_UNSECURE_COMMANDS=true") | |
| 564 | + | }) | |
| 565 | + | if res.Conclusion != ConclusionSuccess || !strings.HasSuffix(logOf(rec.steps[2]), "X=1") { | |
| 566 | + | t.Fatalf("%+v\n%s", res, logOf(rec.steps[1])) | |
| 567 | + | } | |
| 568 | + | } | |
| 569 | + | ||
| 570 | + | // SIGINT goes to the process the step started, like the official runner | |
| 571 | + | // (ProcessInvoker.SendSignal: kill(_proc.Id, SIGINT)), not to its group: bash | |
| 572 | + | // is interrupted but the foreground sleep it waits for is not signalled by it. | |
| 573 | + | func TestCancelSignalsTheShellNotTheGroup(t *testing.T) { | |
| 574 | + | needTools(t, "bash") | |
| 575 | + | c := NewCanceller() | |
| 576 | + | started := make(chan struct{}) | |
| 577 | + | rec := newRecorder() | |
| 578 | + | rep := &signalReporter{recorder: rec, started: started, match: "ready"} | |
| 579 | + | go func() { <-started; c.Cancel(CancelUser) }() | |
| 580 | + | home := t.TempDir() | |
| 581 | + | layout := NewLayout(home, "hello", home+"/tc") | |
| 582 | + | marker := filepath.Join(home, "child-survived") | |
| 583 | + | var sigs []syscall.Signal | |
| 584 | + | res := Run(c, Config{ | |
| 585 | + | WorkflowYAML: []byte(wfHead + " steps:\n - run: |\n trap 'echo got-int' INT\n echo ready\n sleep 1 && touch " + marker + "\n"), | |
| 586 | + | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rep, | |
| 587 | + | BaseEnv: []string{"PATH=" + envPath(), "HOME=" + home}, | |
| 588 | + | TermAfter: 5 * time.Second, KillAfter: 5 * time.Second, | |
| 589 | + | Signal: func(pid int, sig syscall.Signal) { | |
| 590 | + | sigs = append(sigs, sig) | |
| 591 | + | if err := syscall.Kill(pid, sig); err != nil { | |
| 592 | + | t.Error(err) | |
| 593 | + | } | |
| 594 | + | }, | |
| 595 | + | }) | |
| 596 | + | if res.Conclusion != ConclusionCancelled || len(sigs) != 1 || sigs[0] != syscall.SIGINT { | |
| 597 | + | t.Fatalf("%+v signals %v", res, sigs) | |
| 598 | + | } | |
| 599 | + | // bash runs its trap after the foreground child ends: the sleep was not killed. | |
| 600 | + | if _, err := os.Stat(marker); err != nil { | |
| 601 | + | t.Fatalf("the foreground child was signalled with the shell: %v", err) | |
| 602 | + | } | |
| 603 | + | } | |
| @@ -351,10 +351,10 @@ func TestCancelEscalatesToTermAndKill(t *testing.T) { | |||||
| 351 | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rep, | 351 | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rep, | ||
| 352 | BaseEnv: []string{"PATH=" + envPath(), "HOME=" + home}, | 352 | BaseEnv: []string{"PATH=" + envPath(), "HOME=" + home}, | ||
| 353 | TermAfter: 250 * time.Millisecond, KillAfter: 250 * time.Millisecond, | 353 | TermAfter: 250 * time.Millisecond, KillAfter: 250 * time.Millisecond, | ||
| 354 | - | Signal: func(pgid int, sig syscall.Signal) { | 354 | + | Signal: func(pid int, sig syscall.Signal) { |
| 355 | got = append(got, sig) | 355 | got = append(got, sig) | ||
| 356 | times = append(times, time.Since(t0)) | 356 | times = append(times, time.Since(t0)) | ||
| 357 | - | _ = syscall.Kill(-pgid, sig) | 357 | + | _ = syscall.Kill(pid, sig) |
| 358 | mu <- struct{}{} | 358 | mu <- struct{}{} | ||
| 359 | }, | 359 | }, | ||
| 360 | }) | 360 | }) | ||
| @@ -524,3 +524,80 @@ func TestEventJSONIsNeverWrittenThroughALink(t *testing.T) { | |||||
| 524 | t.Fatalf("the second step does not see a fresh event.json:\n%s", got) | 524 | t.Fatalf("the second step does not see a fresh event.json:\n%s", got) | ||
| 525 | } | 525 | } | ||
| 526 | } | 526 | } | ||
| 527 | + | ||||
| 528 | + | func TestLineWriterBoundsALineWithoutEnd(t *testing.T) { | |||
| 529 | + | var got []string | |||
| 530 | + | w := &lineWriter{fn: func(l string) { got = append(got, l) }} | |||
| 531 | + | // 2.5 MiB of two-byte characters without a line feed, in odd-sized writes | |||
| 532 | + | // that cut characters in two. | |||
| 533 | + | blob := []byte(strings.Repeat("é", (5<<20)/4)) | |||
| 534 | + | for i := 0; i < len(blob); i += 7777 { | |||
| 535 | + | _, _ = w.Write(blob[i:min(i+7777, len(blob))]) | |||
| 536 | + | } | |||
| 537 | + | w.Flush() | |||
| 538 | + | total := 0 | |||
| 539 | + | for _, l := range got { | |||
| 540 | + | if len(l) > maxLineBytes { | |||
| 541 | + | t.Fatalf("a line of %d bytes", len(l)) | |||
| 542 | + | } | |||
| 543 | + | if strings.ContainsRune(l, '\ufffd') { | |||
| 544 | + | t.Fatal("a character was cut in two") | |||
| 545 | + | } | |||
| 546 | + | total += len(l) | |||
| 547 | + | } | |||
| 548 | + | if len(got) < 3 || total != len(blob) { | |||
| 549 | + | t.Fatalf("%d lines, %d bytes of %d", len(got), total, len(blob)) | |||
| 550 | + | } | |||
| 551 | + | } | |||
| 552 | + | ||||
| 553 | + | func TestEnvFlagReadsTheStepEnvironmentNotTheExecutors(t *testing.T) { | |||
| 554 | + | // ACTIONS_ALLOW_UNSECURE_COMMANDS is read from the environment the steps | |||
| 555 | + | // get (BaseEnv), not from the process of the executor. | |||
| 556 | + | t.Setenv("ACTIONS_ALLOW_UNSECURE_COMMANDS", "true") | |||
| 557 | + | needTools(t, "bash") | |||
| 558 | + | rec, res := runLocal(t, wfHead+" steps:\n - run: echo '::set-env name=X::1'\n", "j", nil) | |||
| 559 | + | if res.Conclusion != ConclusionFailure || !strings.Contains(logOf(rec.steps[1]), "The `set-env` command is disabled.") { | |||
| 560 | + | t.Fatalf("%+v\n%s", res, logOf(rec.steps[1])) | |||
| 561 | + | } | |||
| 562 | + | rec, res = runLocal(t, wfHead+" steps:\n - run: echo '::set-env name=X::1'\n - run: echo \"X=$X\"\n", "j", func(c *Config) { | |||
| 563 | + | c.BaseEnv = append(c.BaseEnv, "ACTIONS_ALLOW_UNSECURE_COMMANDS=true") | |||
| 564 | + | }) | |||
| 565 | + | if res.Conclusion != ConclusionSuccess || !strings.HasSuffix(logOf(rec.steps[2]), "X=1") { | |||
| 566 | + | t.Fatalf("%+v\n%s", res, logOf(rec.steps[1])) | |||
| 567 | + | } | |||
| 568 | + | } | |||
| 569 | + | ||||
| 570 | + | // SIGINT goes to the process the step started, like the official runner | |||
| 571 | + | // (ProcessInvoker.SendSignal: kill(_proc.Id, SIGINT)), not to its group: bash | |||
| 572 | + | // is interrupted but the foreground sleep it waits for is not signalled by it. | |||
| 573 | + | func TestCancelSignalsTheShellNotTheGroup(t *testing.T) { | |||
| 574 | + | needTools(t, "bash") | |||
| 575 | + | c := NewCanceller() | |||
| 576 | + | started := make(chan struct{}) | |||
| 577 | + | rec := newRecorder() | |||
| 578 | + | rep := &signalReporter{recorder: rec, started: started, match: "ready"} | |||
| 579 | + | go func() { <-started; c.Cancel(CancelUser) }() | |||
| 580 | + | home := t.TempDir() | |||
| 581 | + | layout := NewLayout(home, "hello", home+"/tc") | |||
| 582 | + | marker := filepath.Join(home, "child-survived") | |||
| 583 | + | var sigs []syscall.Signal | |||
| 584 | + | res := Run(c, Config{ | |||
| 585 | + | WorkflowYAML: []byte(wfHead + " steps:\n - run: |\n trap 'echo got-int' INT\n echo ready\n sleep 1 && touch " + marker + "\n"), | |||
| 586 | + | JobKey: "j", ContextsJSON: testGitHubContext("j"), Layout: &layout, Reporter: rep, | |||
| 587 | + | BaseEnv: []string{"PATH=" + envPath(), "HOME=" + home}, | |||
| 588 | + | TermAfter: 5 * time.Second, KillAfter: 5 * time.Second, | |||
| 589 | + | Signal: func(pid int, sig syscall.Signal) { | |||
| 590 | + | sigs = append(sigs, sig) | |||
| 591 | + | if err := syscall.Kill(pid, sig); err != nil { | |||
| 592 | + | t.Error(err) | |||
| 593 | + | } | |||
| 594 | + | }, | |||
| 595 | + | }) | |||
| 596 | + | if res.Conclusion != ConclusionCancelled || len(sigs) != 1 || sigs[0] != syscall.SIGINT { | |||
| 597 | + | t.Fatalf("%+v signals %v", res, sigs) | |||
| 598 | + | } | |||
| 599 | + | // bash runs its trap after the foreground child ends: the sleep was not killed. | |||
| 600 | + | if _, err := os.Stat(marker); err != nil { | |||
| 601 | + | t.Fatalf("the foreground child was signalled with the shell: %v", err) | |||
| 602 | + | } | |||
| 603 | + | } | |||
M internal/executor/proc.go +45 -3
| @@ -39,6 +39,12 @@ type lineWriter struct { | |||
| 39 | 39 | pending bool // the last byte seen was "\r": a following "\n" is part of it | |
| 40 | 40 | } | |
| 41 | 41 | ||
| 42 | + | // maxLineBytes bounds what a line may hold before it is emitted unfinished: a | |
| 43 | + | // step that prints gigabytes without a line feed must not fill the memory of | |
| 44 | + | // the executor. The official runner reads lines with StreamReader.ReadLine and | |
| 45 | + | // has no such bound; a line cut here is sent as consecutive lines. | |
| 46 | + | const maxLineBytes = 1 << 20 | |
| 47 | + | ||
| 42 | 48 | func (w *lineWriter) Write(p []byte) (int, error) { | |
| 43 | 49 | n := len(p) | |
| 44 | 50 | for len(p) > 0 { | |
| @@ -51,6 +57,12 @@ func (w *lineWriter) Write(p []byte) (int, error) { | |||
| 51 | 57 | } | |
| 52 | 58 | i := bytes.IndexAny(p, "\r\n") | |
| 53 | 59 | if i < 0 { | |
| 60 | + | for len(w.buf)+len(p) > maxLineBytes { | |
| 61 | + | room := maxLineBytes - len(w.buf) | |
| 62 | + | w.buf = append(w.buf, p[:room]...) | |
| 63 | + | p = p[room:] | |
| 64 | + | w.emitCut() | |
| 65 | + | } | |
| 54 | 66 | w.buf = append(w.buf, p...) | |
| 55 | 67 | break | |
| 56 | 68 | } | |
| @@ -62,6 +74,24 @@ func (w *lineWriter) Write(p []byte) (int, error) { | |||
| 62 | 74 | return n, nil | |
| 63 | 75 | } | |
| 64 | 76 | ||
| 77 | + | // emitCut emits the buffer without a line end, cutting before an incomplete | |
| 78 | + | // UTF-8 character so that it is not replaced by U+FFFD. | |
| 79 | + | func (w *lineWriter) emitCut() { | |
| 80 | + | cut := len(w.buf) | |
| 81 | + | for back := 1; back <= utf8.UTFMax && back <= len(w.buf); back++ { | |
| 82 | + | if utf8.RuneStart(w.buf[len(w.buf)-back]) { | |
| 83 | + | if !utf8.FullRune(w.buf[len(w.buf)-back:]) { | |
| 84 | + | cut = len(w.buf) - back | |
| 85 | + | } | |
| 86 | + | break | |
| 87 | + | } | |
| 88 | + | } | |
| 89 | + | rest := append([]byte(nil), w.buf[cut:]...) | |
| 90 | + | w.buf = w.buf[:cut] | |
| 91 | + | w.emit() | |
| 92 | + | w.buf = append(w.buf, rest...) | |
| 93 | + | } | |
| 94 | + | ||
| 65 | 95 | func (w *lineWriter) emit() { | |
| 66 | 96 | b := w.buf | |
| 67 | 97 | if !utf8.Valid(b) { | |
| @@ -173,8 +203,9 @@ type procSpec struct { | |||
| 173 | 203 | Cancel <-chan struct{} | |
| 174 | 204 | ||
| 175 | 205 | TermAfter, KillAfter, WaitDelay time.Duration | |
| 176 | - | // Signal delivers a signal to a process group; tests replace it. | |
| 177 | - | Signal func(pgid int, sig syscall.Signal) | |
| 206 | + | // Signal delivers a signal to the process the step started (the shell); | |
| 207 | + | // tests replace it. | |
| 208 | + | Signal func(pid int, sig syscall.Signal) | |
| 178 | 209 | } | |
| 179 | 210 | ||
| 180 | 211 | // procResult is how a process ended. | |
| @@ -199,7 +230,18 @@ func runProcess(spec procSpec) (procResult, error) { | |||
| 199 | 230 | spec.WaitDelay = DefaultWaitDelay | |
| 200 | 231 | } | |
| 201 | 232 | if spec.Signal == nil { | |
| 202 | - | spec.Signal = func(pgid int, sig syscall.Signal) { _ = syscall.Kill(-pgid, sig) } | |
| 233 | + | // Like the official runner, which signals the process it started and | |
| 234 | + | // not its group: ProcessInvoker.SendSignal calls kill(_proc.Id, signal) | |
| 235 | + | // for SIGINT and SIGTERM (Runner.Sdk/ProcessInvoker.cs:829-833), and the | |
| 236 | + | // last resort is Process.Kill() of that process alone (:855-869, which | |
| 237 | + | // is named "KillProcessTree" but does not kill the tree). What the shell | |
| 238 | + | // started is left to the shell: bash forwards nothing to a foreground | |
| 239 | + | // child it waits for, and the children that outlive the step are | |
| 240 | + | // killed by "Complete job" (the orphan cleanup, JobExtension.cs:132). | |
| 241 | + | // The step is still started in its own process group, so that a signal | |
| 242 | + | // sent to the executor's group (init forwarding a SIGTERM) does not reach | |
| 243 | + | // it before the executor decides. | |
| 244 | + | spec.Signal = func(pid int, sig syscall.Signal) { _ = syscall.Kill(pid, sig) } | |
| 203 | 245 | } | |
| 204 | 246 | cmd := exec.Command(spec.Argv[0], spec.Argv[1:]...) | |
| 205 | 247 | cmd.Dir = spec.Dir | |
| @@ -39,6 +39,12 @@ type lineWriter struct { | |||||
| 39 | pending bool // the last byte seen was "\r": a following "\n" is part of it | 39 | pending bool // the last byte seen was "\r": a following "\n" is part of it | ||
| 40 | } | 40 | } | ||
| 41 | 41 | ||||
| 42 | + | // maxLineBytes bounds what a line may hold before it is emitted unfinished: a | |||
| 43 | + | // step that prints gigabytes without a line feed must not fill the memory of | |||
| 44 | + | // the executor. The official runner reads lines with StreamReader.ReadLine and | |||
| 45 | + | // has no such bound; a line cut here is sent as consecutive lines. | |||
| 46 | + | const maxLineBytes = 1 << 20 | |||
| 47 | + | ||||
| 42 | func (w *lineWriter) Write(p []byte) (int, error) { | 48 | func (w *lineWriter) Write(p []byte) (int, error) { | ||
| 43 | n := len(p) | 49 | n := len(p) | ||
| 44 | for len(p) > 0 { | 50 | for len(p) > 0 { | ||
| @@ -51,6 +57,12 @@ func (w *lineWriter) Write(p []byte) (int, error) { | |||||
| 51 | } | 57 | } | ||
| 52 | i := bytes.IndexAny(p, "\r\n") | 58 | i := bytes.IndexAny(p, "\r\n") | ||
| 53 | if i < 0 { | 59 | if i < 0 { | ||
| 60 | + | for len(w.buf)+len(p) > maxLineBytes { | |||
| 61 | + | room := maxLineBytes - len(w.buf) | |||
| 62 | + | w.buf = append(w.buf, p[:room]...) | |||
| 63 | + | p = p[room:] | |||
| 64 | + | w.emitCut() | |||
| 65 | + | } | |||
| 54 | w.buf = append(w.buf, p...) | 66 | w.buf = append(w.buf, p...) | ||
| 55 | break | 67 | break | ||
| 56 | } | 68 | } | ||
| @@ -62,6 +74,24 @@ func (w *lineWriter) Write(p []byte) (int, error) { | |||||
| 62 | return n, nil | 74 | return n, nil | ||
| 63 | } | 75 | } | ||
| 64 | 76 | ||||
| 77 | + | // emitCut emits the buffer without a line end, cutting before an incomplete | |||
| 78 | + | // UTF-8 character so that it is not replaced by U+FFFD. | |||
| 79 | + | func (w *lineWriter) emitCut() { | |||
| 80 | + | cut := len(w.buf) | |||
| 81 | + | for back := 1; back <= utf8.UTFMax && back <= len(w.buf); back++ { | |||
| 82 | + | if utf8.RuneStart(w.buf[len(w.buf)-back]) { | |||
| 83 | + | if !utf8.FullRune(w.buf[len(w.buf)-back:]) { | |||
| 84 | + | cut = len(w.buf) - back | |||
| 85 | + | } | |||
| 86 | + | break | |||
| 87 | + | } | |||
| 88 | + | } | |||
| 89 | + | rest := append([]byte(nil), w.buf[cut:]...) | |||
| 90 | + | w.buf = w.buf[:cut] | |||
| 91 | + | w.emit() | |||
| 92 | + | w.buf = append(w.buf, rest...) | |||
| 93 | + | } | |||
| 94 | + | ||||
| 65 | func (w *lineWriter) emit() { | 95 | func (w *lineWriter) emit() { | ||
| 66 | b := w.buf | 96 | b := w.buf | ||
| 67 | if !utf8.Valid(b) { | 97 | if !utf8.Valid(b) { | ||
| @@ -173,8 +203,9 @@ type procSpec struct { | |||||
| 173 | Cancel <-chan struct{} | 203 | Cancel <-chan struct{} | ||
| 174 | 204 | ||||
| 175 | TermAfter, KillAfter, WaitDelay time.Duration | 205 | TermAfter, KillAfter, WaitDelay time.Duration | ||
| 176 | - | // Signal delivers a signal to a process group; tests replace it. | 206 | + | // Signal delivers a signal to the process the step started (the shell); |
| 177 | - | Signal func(pgid int, sig syscall.Signal) | 207 | + | // tests replace it. |
| 208 | + | Signal func(pid int, sig syscall.Signal) | |||
| 178 | } | 209 | } | ||
| 179 | 210 | ||||
| 180 | // procResult is how a process ended. | 211 | // procResult is how a process ended. | ||
| @@ -199,7 +230,18 @@ func runProcess(spec procSpec) (procResult, error) { | |||||
| 199 | spec.WaitDelay = DefaultWaitDelay | 230 | spec.WaitDelay = DefaultWaitDelay | ||
| 200 | } | 231 | } | ||
| 201 | if spec.Signal == nil { | 232 | if spec.Signal == nil { | ||
| 202 | - | spec.Signal = func(pgid int, sig syscall.Signal) { _ = syscall.Kill(-pgid, sig) } | 233 | + | // Like the official runner, which signals the process it started and |
| 234 | + | // not its group: ProcessInvoker.SendSignal calls kill(_proc.Id, signal) | |||
| 235 | + | // for SIGINT and SIGTERM (Runner.Sdk/ProcessInvoker.cs:829-833), and the | |||
| 236 | + | // last resort is Process.Kill() of that process alone (:855-869, which | |||
| 237 | + | // is named "KillProcessTree" but does not kill the tree). What the shell | |||
| 238 | + | // started is left to the shell: bash forwards nothing to a foreground | |||
| 239 | + | // child it waits for, and the children that outlive the step are | |||
| 240 | + | // killed by "Complete job" (the orphan cleanup, JobExtension.cs:132). | |||
| 241 | + | // The step is still started in its own process group, so that a signal | |||
| 242 | + | // sent to the executor's group (init forwarding a SIGTERM) does not reach | |||
| 243 | + | // it before the executor decides. | |||
| 244 | + | spec.Signal = func(pid int, sig syscall.Signal) { _ = syscall.Kill(pid, sig) } | |||
| 203 | } | 245 | } | ||
| 204 | cmd := exec.Command(spec.Argv[0], spec.Argv[1:]...) | 246 | cmd := exec.Command(spec.Argv[0], spec.Argv[1:]...) | ||
| 205 | cmd.Dir = spec.Dir | 247 | cmd.Dir = spec.Dir | ||
M internal/executor/step.go +1 -2
| @@ -5,7 +5,6 @@ package executor | |||
| 5 | 5 | import ( | |
| 6 | 6 | "context" | |
| 7 | 7 | "fmt" | |
| 8 | - | "os" | |
| 9 | 8 | "strings" | |
| 10 | 9 | "sync" | |
| 11 | 10 | ||
| @@ -52,7 +51,7 @@ func (s *stepRun) AddPath(p string) { | |||
| 52 | 51 | // EnvFlag reads a boolean variable of the env context, then of the process | |
| 53 | 52 | // (ActionCommandManager.cs:236-246). | |
| 54 | 53 | func (s *stepRun) EnvFlag(name string) bool { | |
| 55 | - | v := strings.ToLower(os.Getenv(name)) | |
| 54 | + | v := strings.ToLower(envValue(s.j.baseEnv, name)) | |
| 56 | 55 | if v != "true" { | |
| 57 | 56 | if e, ok := s.env.Get(name); ok { | |
| 58 | 57 | v = strings.ToLower(e) | |
| @@ -5,7 +5,6 @@ package executor | |||||
| 5 | import ( | 5 | import ( | ||
| 6 | "context" | 6 | "context" | ||
| 7 | "fmt" | 7 | "fmt" | ||
| 8 | - | "os" | |||
| 9 | "strings" | 8 | "strings" | ||
| 10 | "sync" | 9 | "sync" | ||
| 11 | 10 | ||||
| @@ -52,7 +51,7 @@ func (s *stepRun) AddPath(p string) { | |||||
| 52 | // EnvFlag reads a boolean variable of the env context, then of the process | 51 | // EnvFlag reads a boolean variable of the env context, then of the process | ||
| 53 | // (ActionCommandManager.cs:236-246). | 52 | // (ActionCommandManager.cs:236-246). | ||
| 54 | func (s *stepRun) EnvFlag(name string) bool { | 53 | func (s *stepRun) EnvFlag(name string) bool { | ||
| 55 | - | v := strings.ToLower(os.Getenv(name)) | 54 | + | v := strings.ToLower(envValue(s.j.baseEnv, name)) |
| 56 | if v != "true" { | 55 | if v != "true" { | ||
| 57 | if e, ok := s.env.Get(name); ok { | 56 | if e, ok := s.env.Get(name); ok { | ||
| 58 | v = strings.ToLower(e) | 57 | v = strings.ToLower(e) | ||
M internal/guest/jobexec.go +1 -1
| @@ -184,7 +184,7 @@ func runJob(ctx context.Context, conn *execstream.Conn, cfg ExecConfig, spec *ex | |||
| 184 | 184 | RunnerVersion: spec.GetRunnerVersion(), | |
| 185 | 185 | Home: su.Home, ToolCache: os.Getenv("RUNNER_TOOL_CACHE"), BaseEnv: stepEnv(os.Environ(), su), Credential: su.Credential, | |
| 186 | 186 | Debug: spec.GetDebug(), Reporter: rep, | |
| 187 | - | TermAfter: cfg.TermAfter, KillAfter: cfg.KillAfter, WaitDelay: cfg.WaitDelay, Signal: cfg.signal, | |
| 187 | + | TermAfter: cfg.TermAfter, KillAfter: cfg.KillAfter, WaitDelay: cfg.WaitDelay, | |
| 188 | 188 | }) | |
| 189 | 189 | if res.Conclusion == executor.ConclusionSuccess { | |
| 190 | 190 | return 0 | |
| @@ -184,7 +184,7 @@ func runJob(ctx context.Context, conn *execstream.Conn, cfg ExecConfig, spec *ex | |||||
| 184 | RunnerVersion: spec.GetRunnerVersion(), | 184 | RunnerVersion: spec.GetRunnerVersion(), | ||
| 185 | Home: su.Home, ToolCache: os.Getenv("RUNNER_TOOL_CACHE"), BaseEnv: stepEnv(os.Environ(), su), Credential: su.Credential, | 185 | Home: su.Home, ToolCache: os.Getenv("RUNNER_TOOL_CACHE"), BaseEnv: stepEnv(os.Environ(), su), Credential: su.Credential, | ||
| 186 | Debug: spec.GetDebug(), Reporter: rep, | 186 | Debug: spec.GetDebug(), Reporter: rep, | ||
| 187 | - | TermAfter: cfg.TermAfter, KillAfter: cfg.KillAfter, WaitDelay: cfg.WaitDelay, Signal: cfg.signal, | 187 | + | TermAfter: cfg.TermAfter, KillAfter: cfg.KillAfter, WaitDelay: cfg.WaitDelay, |
| 188 | }) | 188 | }) | ||
| 189 | if res.Conclusion == executor.ConclusionSuccess { | 189 | if res.Conclusion == executor.ConclusionSuccess { | ||
| 190 | return 0 | 190 | return 0 | ||