fix: stop host teardown from deleting a workdir it never created (#1409)

Host jobs run in a per-job directory under `host.workdir_parent`, which teardown removes. Teardown also deleted `<container.workdir_parent>/<owner>/<repo>`, a path host jobs never create. With an empty parent and no repository that was the filesystem root, and any existing directory at that path was deleted although the job never used it.

This removes that delete instead of guarding it, so teardown only removes what the job created.

---------

Co-authored-by: silverwind <[email protected]>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1409
Reviewed-by: bircni <[email protected]>
Co-authored-by: clintlmartin <[email protected]>

act/container/host_environment.go

@@ -31,18 +31,15 @@ import (
 )
 
 type HostEnvironment struct {
-	Path      string
-	TmpDir    string
-	ToolCache string
-	Workdir   string
-	// CleanWorkdir means teardown owns Workdir and may delete it. Leave false
-	// when Workdir points at a caller-owned checkout (e.g. `act` local mode).
-	CleanWorkdir bool
-	ActPath      string
-	Root         string
-	TrackingID   string // RUNNER_TRACKING_ID of the job, a step with another value keeps its leftovers
-	StdOut       io.Writer
-	AllocatePTY  bool // allocate a pseudo-TTY for each step's process
+	Path        string
+	TmpDir      string
+	ToolCache   string
+	Workdir     string
+	ActPath     string
+	Root        string
+	TrackingID  string // RUNNER_TRACKING_ID of the job, a step with another value keeps its leftovers
+	StdOut      io.Writer
+	AllocatePTY bool // allocate a pseudo-TTY for each step's process
 
 	// procGroup owns every process the job's steps start. Atomic: Remove may read
 	// it while a step is still starting.
@@ -436,24 +433,14 @@ func (e *HostEnvironment) Remove() common.Executor {
 		rmCtx, rmCancel := context.WithTimeout(context.Background(), hostCleanupTimeout)
 		defer rmCancel()
 
-		var errs []error
-		if err := removePathWithRetry(rmCtx, cmp.Or(e.Root, e.Path)); err != nil {
+		err := removePathWithRetry(rmCtx, cmp.Or(e.Root, e.Path))
+		if err != nil {
 			logger.Warnf("failed to remove host misc state %s: %v", cmp.Or(e.Root, e.Path), err)
-			errs = append(errs, err)
 		}
-		if e.CleanWorkdir {
-			if err := removePathWithRetry(rmCtx, e.Workdir); err != nil {
-				logger.Warnf("failed to remove host workspace %s: %v", e.Workdir, err)
-				errs = append(errs, err)
-			}
+		if errors.Is(err, context.DeadlineExceeded) {
+			return nil // a stalled delete must not fail job completion
 		}
-		for _, err := range errs {
-			if !errors.Is(err, context.DeadlineExceeded) {
-				return errors.Join(errs...)
-			}
-		}
-		// Teardown timed out; warned above. Do not fail job completion over it.
-		return nil
+		return err
 	}
 }
 

act/container/host_environment_test.go

@@ -158,27 +158,12 @@ func TestHostEnvironmentAllocatePTY(t *testing.T) {
 }
 
 func TestHostEnvironmentRemoveWorkdir(t *testing.T) {
-	for name, owned := range map[string]bool{"preserves caller workdir": false, "cleans owned workdir": true} {
-		t.Run(name, func(t *testing.T) {
-			miscRoot, workdir := t.TempDir(), t.TempDir()
-			e := &HostEnvironment{
-				Path:         filepath.Join(miscRoot, "hostexecutor"),
-				Workdir:      workdir,
-				CleanWorkdir: owned,
-				Root:         miscRoot,
-			}
-			require.NoError(t, os.MkdirAll(e.Path, 0o700))
-			require.NoError(t, os.MkdirAll(filepath.Join(miscRoot, "tmp"), 0o700))
-			require.NoError(t, e.Remove()(t.Context()))
-			assert.NoDirExists(t, miscRoot)
-			_, err := os.Stat(workdir)
-			if owned {
-				assert.ErrorIs(t, err, os.ErrNotExist)
-			} else {
-				require.NoError(t, err)
-			}
-		})
-	}
+	miscRoot, workdir := t.TempDir(), t.TempDir()
+	e := &HostEnvironment{Path: filepath.Join(miscRoot, "hostexecutor"), Workdir: workdir, Root: miscRoot}
+	require.NoError(t, os.MkdirAll(e.Path, 0o700))
+	require.NoError(t, e.Remove()(t.Context()))
+	assert.NoDirExists(t, miscRoot)
+	assert.DirExists(t, workdir)
 }
 
 func TestRemoveAllWithContextDoesNotHangOnStuckDelete(t *testing.T) {

act/runner/run_context.go

@@ -468,16 +468,15 @@ func (rc *RunContext) startHostEnvironment() common.Executor {
 			return err
 		}
 		rc.JobContainer = &container.HostEnvironment{
-			Path:         path,
-			TmpDir:       runnerTmp,
-			ToolCache:    toolCache,
-			Workdir:      rc.Config.Workdir,
-			CleanWorkdir: rc.Config.CleanWorkdir,
-			ActPath:      actPath,
-			Root:         miscpath,
-			TrackingID:   rc.runnerTrackingID(),
-			StdOut:       logWriter,
-			AllocatePTY:  rc.Config.AllocatePTY,
+			Path:        path,
+			TmpDir:      runnerTmp,
+			ToolCache:   toolCache,
+			Workdir:     rc.Config.Workdir,
+			ActPath:     actPath,
+			Root:        miscpath,
+			TrackingID:  rc.runnerTrackingID(),
+			StdOut:      logWriter,
+			AllocatePTY: rc.Config.AllocatePTY,
 		}
 		rc.cleanUpJobContainer = rc.JobContainer.Remove()
 		rc.setRunnerContextEnv(ctx)

act/runner/runner.go

@@ -70,7 +70,6 @@ type Config struct {
 	EventJSON             string               // the content of JSON file to use for event.json in containers, overrides EventPath
 	ContainerNamePrefix   string               // the prefix of container name
 	ContainerMaxLifetime  time.Duration        // the max lifetime of job containers
-	CleanWorkdir          bool                 // remove host executor workdir on teardown
 	DefaultActionInstance string               // the default actions web site
 	// DefaultActionInstanceIsSelfHosted reports whether DefaultActionInstance is this
 	// self-hosted Gitea (DEFAULT_ACTIONS_URL=self). It gates token trust: only then may the