fix: remove LXC working directory when it completes (#1003)

The working directory was not cleaned up upon completion of a LXC job because rc.stopJobContainer() -> rc.cleanUpJobContainer() -> rc.JobContainer.Remove() was never called for LXC containers.

- stopContainer() and closeContainer() must not call
  rc.stopHostEnvironment(ctx) for LXC containers because
  - it will needlessly be called twice
  - it intercepts the call to
    - rc.stopJobContainer()
    - rc.JobContainer.Close()
- rc.stopHostEnvironment(ctx) must be called in rc.cleanUpJobContainer which is indirectly called by rc.stopJobContainer()
- since rc.JobContainer.Close() is a noop, not calling it for LXC containers had no consequence

Resolves forgejo/runner#442

<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- bug fixes
  - [PR](https://code.forgejo.org/forgejo/runner/pulls/1003): <!--number 1003 --><!--line 0 --><!--description Zml4OiByZW1vdmUgTFhDIHdvcmtpbmcgZGlyZWN0b3J5IHdoZW4gaXQgY29tcGxldGVz-->fix: remove LXC working directory when it completes<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/1003
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.code.forgejo.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>

act/container/host_environment.go

@@ -407,6 +407,7 @@ func (e *HostEnvironment) Remove() common.Executor {
 		if e.CleanUp != nil {
 			e.CleanUp()
 		}
+		common.Logger(ctx).Debugf("HostEnvironment.Remove %s", e.Path)
 		return os.RemoveAll(e.Path)
 	}
 }

act/runner/run_context.go

@@ -316,7 +316,15 @@ func (rc *RunContext) startHostEnvironment() common.Executor {
 			StdOut: logWriter,
 			LXC:    rc.IsLXCHostEnv(ctx),
 		}
-		rc.cleanUpJobContainer = rc.JobContainer.Remove()
+		rc.cleanUpJobContainer = func(ctx context.Context) error {
+			if err := rc.stopHostEnvironment(ctx); err != nil {
+				return err
+			}
+			if rc.JobContainer == nil {
+				return nil
+			}
+			return rc.JobContainer.Remove()(ctx)
+		}
 		for k, v := range rc.JobContainer.GetRunnerContext(ctx) {
 			if v, ok := v.(string); ok {
 				rc.Env[fmt.Sprintf("RUNNER_%s", strings.ToUpper(k))] = v
@@ -890,9 +898,6 @@ func (rc *RunContext) IsHostEnv(ctx context.Context) bool {
 
 func (rc *RunContext) stopContainer() common.Executor {
 	return func(ctx context.Context) error {
-		if rc.IsLXCHostEnv(ctx) {
-			return rc.stopHostEnvironment(ctx)
-		}
 		return rc.stopJobContainer()(ctx)
 	}
 }
@@ -900,9 +905,6 @@ func (rc *RunContext) stopContainer() common.Executor {
 func (rc *RunContext) closeContainer() common.Executor {
 	return func(ctx context.Context) error {
 		if rc.JobContainer != nil {
-			if rc.IsLXCHostEnv(ctx) {
-				return rc.stopHostEnvironment(ctx)
-			}
 			return rc.JobContainer.Close()(ctx)
 		}
 		return nil

internal/app/run/runner_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
 	"testing"
 	"time"
 
@@ -355,13 +356,14 @@ func TestRunnerLXC(t *testing.T) {
 	forgejoClient.On("UpdateTask", mock.Anything, mock.Anything).
 		Return(connect.NewResponse(&runnerv1.UpdateTaskResponse{}), nil)
 
+	workdirParent := t.TempDir()
 	runner := NewRunner(
 		&config.Config{
 			Log: config.Log{
 				JobLevel: "trace",
 			},
 			Host: config.Host{
-				WorkdirParent: t.TempDir(),
+				WorkdirParent: workdirParent,
 			},
 		},
 		&config.Registration{
@@ -388,6 +390,17 @@ func TestRunnerLXC(t *testing.T) {
 		err := runner.run(ctx, task, reporter)
 		reporter.Close(nil)
 		require.NoError(t, err, description)
+		// verify there are no leftovers
+		assertDirectoryEmpty := func(t *testing.T, dir string) {
+			f, err := os.Open(dir)
+			require.NoError(t, err)
+			defer f.Close()
+
+			names, err := f.Readdirnames(-1)
+			require.NoError(t, err)
+			assert.Empty(t, names)
+		}
+		assertDirectoryEmpty(t, workdirParent)
 	}
 
 	t.Run("OK", func(t *testing.T) {