Stop a step's process that starts after the job was stopped
A cancel that landed between one step ending and the next one's process starting left that process running. CI's slower host hit this in the second-cancel test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0155wm8JcL2djVkP8kBaRGmo
- CI / Typecheck and bundlesuccess
- CI / Unit tests (1/2)success
- CI / Unit tests (2/2)success
- End-to-end / End-to-end (agent reviews, tasks, Today)success
- End-to-end / End-to-end (git, PRs, Actions)success
- End-to-end / End-to-end (large pushes)success
- End-to-end / End-to-end (passkeys, registry, email)success
- End-to-end / End-to-end (real histories)success
- End-to-end / End-to-end (runner Docker)success
- End-to-end / End-to-end (security)success
2 files changed+27−10
modifiedcontainers/runner/src/job.ts+9−8View file
| @@ -217,6 +217,13 @@ | ||
| 217 | 217 | for (const c of this.children) killTree(c); |
| 218 | 218 | } |
| 219 | 219 | |
| 220 | + /** A process the job started: stopped with the job, or at once when it started after the stop. */ | |
| 221 | + private track(c: ChildProcess): void { | |
| 222 | + this.children.add(c); | |
| 223 | + c.once("close", () => this.children.delete(c)); | |
| 224 | + if (this.halting) killTree(c); | |
| 225 | + } | |
| 226 | + | |
| 220 | 227 | /** Once the cancelled step has stopped: let the steps that still run after a cancel do their work. */ |
| 221 | 228 | private resumeAfterCancel(): void { |
| 222 | 229 | if (!this.halting || this.forced) return; |
| @@ -724,10 +731,7 @@ | ||
| 724 | 731 | env, |
| 725 | 732 | timeoutMs: o.timeoutMs, |
| 726 | 733 | onLine: (l) => proc.handle(l), |
| 727 | - onSpawn: (c) => { | |
| 728 | - this.children.add(c); | |
| 729 | - c.once("close", () => this.children.delete(c)); | |
| 730 | - }, | |
| 734 | + onSpawn: (c) => this.track(c), | |
| 731 | 735 | }); |
| 732 | 736 | await this.applyFileCommands(files, o.io); |
| 733 | 737 | if (res.signal && this.replacing && !this.stopping) { |
| @@ -809,10 +813,7 @@ | ||
| 809 | 813 | capture: o.capture, |
| 810 | 814 | input: o.input, |
| 811 | 815 | onLine: o.silent ? undefined : (l) => this.logger.line(o.io.logIndex, l), |
| 812 | - onSpawn: (c) => { | |
| 813 | - this.children.add(c); | |
| 814 | - c.once("close", () => this.children.delete(c)); | |
| 815 | - }, | |
| 816 | + onSpawn: (c) => this.track(c), | |
| 816 | 817 | }); |
| 817 | 818 | } |
| 818 | 819 |
modifiedcontainers/runner/test/job.test.ts+18−2View file
| @@ -521,16 +521,32 @@ | ||
| 521 | 521 | const t0 = Date.now(); |
| 522 | 522 | while (!api.logText(0).includes("started") && Date.now() - t0 < 10_000) await new Promise((r) => setTimeout(r, 50)); |
| 523 | 523 | runner!.cancel(); |
| 524 | - while (!api.logText(1).includes("cleanup-started") && Date.now() - t0 < 10_000) await new Promise((r) => setTimeout(r, 50)); | |
| 524 | + const t1 = Date.now(); | |
| 525 | + while (!api.logText(1).includes("cleanup-started") && Date.now() - t1 < 15_000) await new Promise((r) => setTimeout(r, 50)); | |
| 526 | + expect(api.logText(1)).toContain("cleanup-started"); | |
| 525 | 527 | runner!.cancel(); |
| 526 | 528 | const c = await p; |
| 527 | 529 | expect(c.conclusion).toBe("cancelled"); |
| 528 | 530 | expect(api.lastReport(1)?.conclusion).toBe("cancelled"); |
| 529 | 531 | expect(api.lastReport(2)?.conclusion).toBe("cancelled"); |
| 530 | 532 | expect(api.logText(2)).not.toContain("never"); |
| 531 | - expect(Date.now() - t0).toBeLessThan(15_000); | |
| 533 | + expect(Date.now() - t1).toBeLessThan(15_000); | |
| 532 | 534 | }); |
| 533 | 535 | |
| 536 | + it("a process a step starts after the job was stopped is stopped at once", async () => { | |
| 537 | + let runner: JobRunner | undefined; | |
| 538 | + const p = run(makeSpec(api, [{ run: "echo started; sleep 60" }, { if: "always()", run: "echo cleanup-started; sleep 60" }]), { onRunner: (r) => (runner = r) }); | |
| 539 | + const t0 = Date.now(); | |
| 540 | + while (!api.logText(0).includes("started") && Date.now() - t0 < 10_000) await new Promise((r) => setTimeout(r, 50)); | |
| 541 | + // Both cancels before the cleanup step's process starts: it must not run its 60 seconds. | |
| 542 | + runner!.cancel(); | |
| 543 | + runner!.cancel(); | |
| 544 | + const c = await p; | |
| 545 | + expect(c.conclusion).toBe("cancelled"); | |
| 546 | + expect(api.lastReport(1)?.conclusion).toBe("cancelled"); | |
| 547 | + expect(Date.now() - t0).toBeLessThan(20_000); | |
| 548 | + }); | |
| 549 | + | |
| 534 | 550 | it("stops the steps that run after a cancel once their grace runs out", async () => { |
| 535 | 551 | let runner: JobRunner | undefined; |
| 536 | 552 | const p = run(makeSpec(api, [{ run: "echo started; sleep 60" }, { if: "always()", run: "echo cleanup-started; sleep 60" }, { if: "always()", run: "echo never" }]), { onRunner: (r) => (runner = r) }); |