From c2046d597c8a10fabe7c42284fe9f233de713347 Mon Sep 17 00:00:00 2001 From: clintlmartin <262727+clintlmartin@noreply.gitea.com> Date: Fri, 13 Nov 2026 06:50:44 +0000 Subject: [PATCH] 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 `//`, 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 Reviewed-on: https://gitea.com/gitea/runner/pulls/1409 Reviewed-by: bircni Co-authored-by: clintlmartin <262727+clintlmartin@noreply.gitea.com> --- act/container/host_environment.go | 41 +++++++++----------------- act/container/host_environment_test.go | 27 ++++------------- act/runner/run_context.go | 19 ++++++------ act/runner/runner.go | 1 - docs/post-task-script.md | 6 ++-- internal/app/run/runner.go | 1 - 6 files changed, 32 insertions(+), 63 deletions(-) diff --git a/act/container/host_environment.go b/act/container/host_environment.go index 8a88e473..8aa3f8ce 100644 --- a/act/container/host_environment.go +++ b/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 } } diff --git a/act/container/host_environment_test.go b/act/container/host_environment_test.go index 67bb8dc8..49ae0d46 100644 --- a/act/container/host_environment_test.go +++ b/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) { diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 8575b72c..daba5dcf 100644 --- a/act/runner/run_context.go +++ b/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) diff --git a/act/runner/runner.go b/act/runner/runner.go index 1a1cd261..6ede6459 100644 --- a/act/runner/runner.go +++ b/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 diff --git a/docs/post-task-script.md b/docs/post-task-script.md index cca3f4bb..deb80c8b 100644 --- a/docs/post-task-script.md +++ b/docs/post-task-script.md @@ -9,13 +9,13 @@ It is configured under `runner.post_task_script` in the runner YAML config (see For each task, execution order is: 1. Workflow runs (steps, actions, containers). -2. In-job cleanup (action `post:` steps, container stop/remove). +2. In-job cleanup (action `post:` steps, container stop/remove, host job directory removal). 3. Job outputs are reported to Gitea. 4. Bind-workdir workspace removal, when `container.bind_workdir` is enabled. 5. **Post-task script** (this hook). 6. Final task acknowledgement to Gitea (`reporter.Close()`). -The script is **additive**: it does not replace any built-in cleanup. When `container.bind_workdir` is enabled, the task workspace directory has usually already been deleted before the script starts. `GITEA_WORKSPACE` is still set to the path the job used, for reference. +The script is **additive**: it does not replace any built-in cleanup. The workspace of host jobs, and of container jobs with `container.bind_workdir`, is deleted before the script starts. ## Runner stays offline until the script finishes @@ -75,7 +75,7 @@ The script receives `runner.envs` / `runner.env_file` values plus: | `GITEA_TASK_ID` | Numeric task ID. | | `GITEA_RUN_ID` | Workflow run ID, when provided by the server. | | `GITEA_REPOSITORY` | Repository slug (`owner/name`). | -| `GITEA_WORKSPACE` | Workspace path used for the job (may already be deleted). | +| `GITEA_WORKSPACE` | Workspace path of the job. | | `GITEA_JOB_RESULT` | `success`, `failure`, `cancelled`, `skipped`, or `unknown`. | The script environment is **not** a full copy of the job container environment. System variables such as `PATH` are only present if you define them in `runner.envs` or `runner.env_file`. diff --git a/internal/app/run/runner.go b/internal/app/run/runner.go index a6ef3137..13c8f8cc 100644 --- a/internal/app/run/runner.go +++ b/internal/app/run/runner.go @@ -603,7 +603,6 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report. EventJSON: string(eventJSON), ContainerNamePrefix: fmt.Sprintf("GITEA-ACTIONS-TASK-%d", task.Id), ContainerMaxLifetime: maxLifetime, - CleanWorkdir: true, ContainerNetworkMode: docker_container.NetworkMode(r.cfg.Container.Network), CacheContainer: cacheContainer, ContainerNetworkCreateOptions: container.NewDockerNetworkCreateExecutorInput{