From 945dbb774342a69f21f32f0fdc22f35bc0d4600e Mon Sep 17 00:00:00 2001
From: Josh Deprez <jd@buildkite.com>
Date: Thu, 25 Jul 2024 17:55:44 +1000
Subject: [PATCH] Detach artifact phase context

---
 internal/job/executor.go | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/internal/job/executor.go b/internal/job/executor.go
index 5d08c6c3f8..0e2b7fef29 100644
--- a/internal/job/executor.go
+++ b/internal/job/executor.go
@@ -121,12 +121,6 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) {
 	defer stopper()
 	defer func() { span.FinishWithError(err) }()
 
-	// kind of a weird series of events - wrap a context in a cancellation (at the top of the func), then pull the cancellation
-	// out again, but some of the stuff we need to do in the executor (namely the teardown) needs to be able to "survive"
-	// a cancellation so we need to be able to pass a cancellable context to the non-teardown stuff, and an uncancellable
-	// context to the teardown stuff
-	nonCancelCtx := context.WithoutCancel(ctx)
-
 	// Listen for cancellation
 	go func() {
 		<-e.cancelCh
@@ -151,7 +145,11 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) {
 
 	// Tear down the environment (and fire pre-exit hook) before we exit
 	defer func() {
-		if err = e.tearDown(nonCancelCtx); err != nil {
+		// kind of a weird series of events - wrap a context in a cancellation (at the top of the func), then pull the cancellation
+		// out again, but some of the stuff we need to do in the executor (namely the teardown) needs to be able to "survive"
+		// a cancellation so we need to be able to pass a cancellable context to the non-teardown stuff, and an uncancellable
+		// context to the teardown stuff
+		if err := e.tearDown(context.WithoutCancel(ctx)); err != nil {
 			e.shell.Errorf("Error tearing down job executor: %v", err)
 
 			// this gets passed back via the named return
@@ -237,8 +235,10 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) {
 			span.RecordError(commandErr)
 		}
 
-		// Only upload artifacts as part of the command phase
-		if err = e.artifactPhase(ctx); err != nil {
+		// Only upload artifacts as part of the command phase.
+		// The artifacts might be relevant for debugging job timeouts, so
+		// artifact phase shouldn't be subject to cancellation.
+		if err = e.artifactPhase(context.WithoutCancel(ctx)); err != nil {
 			e.shell.Errorf("%v", err)
 
 			if commandErr != nil {