mirror of
https://gitea.com/gitea/act_runner.git
synced 2026-06-07 14:06:35 +00:00
fix: matrix-job data races + outputs, leaner offline test suite (#994)
Running the full suite under `-race` (dropping `-short`) exposed pre-existing data races in parallel matrix-job execution, fixed by not sharing mutable state across combinations: - `containerDaemonSocket()`/`validVolumes()` derive per-job values instead of mutating shared `Config` - `getWorkflowSecrets` builds a fresh map, `rc.steps()` clones each step, and go-git workdir access is serialized - every write to a shared `Job`'s result/outputs runs under a per-`Job` lock, each combo interpolating outputs from a pristine snapshot (last wins, as on GitHub) ### Test suite - capability gates (docker / network / host-tools / Linux) replace the `-short` skips, and the suite runs offline via local fixtures (the artifact flow uses an in-process loopback server, only the docker-action force-pull needs the network) - drops redundant tests, adds a regression test for https://gitea.com/gitea/runner/issues/981 and a docker-in-docker harness (`make test-dind`) --- This PR was written with the help of Claude Opus 4.7 Reviewed-on: https://gitea.com/gitea/runner/pulls/994 Reviewed-by: Nicolas <bircni@icloud.com> Co-authored-by: silverwind <me@silverwind.io> Co-committed-by: silverwind <me@silverwind.io>
This commit is contained in:
@@ -66,8 +66,21 @@ func (e *Error) Commit() string {
|
||||
return e.commit
|
||||
}
|
||||
|
||||
// goGitMu serializes go-git repository access across the process. go-git is not safe for
|
||||
// concurrent use of the same repository (even read access decodes packfiles into shared
|
||||
// state), so parallel jobs inspecting the shared workdir repo race without this. The guarded
|
||||
// operations are fast local reads; gitea runs one job per process, so the lock is effectively
|
||||
// uncontended in production.
|
||||
var goGitMu sync.Mutex
|
||||
|
||||
// FindGitRevision get the current git revision
|
||||
func FindGitRevision(ctx context.Context, file string) (shortSha, sha string, err error) {
|
||||
goGitMu.Lock()
|
||||
defer goGitMu.Unlock()
|
||||
return findGitRevision(ctx, file)
|
||||
}
|
||||
|
||||
func findGitRevision(ctx context.Context, file string) (shortSha, sha string, err error) {
|
||||
logger := common.Logger(ctx)
|
||||
|
||||
gitDir, err := git.PlainOpenWithOptions(
|
||||
@@ -99,10 +112,13 @@ func FindGitRevision(ctx context.Context, file string) (shortSha, sha string, er
|
||||
|
||||
// FindGitRef get the current git ref
|
||||
func FindGitRef(ctx context.Context, file string) (string, error) {
|
||||
goGitMu.Lock()
|
||||
defer goGitMu.Unlock()
|
||||
|
||||
logger := common.Logger(ctx)
|
||||
|
||||
logger.Debugf("Loading revision from git directory")
|
||||
_, ref, err := FindGitRevision(ctx, file)
|
||||
_, ref, err := findGitRevision(ctx, file)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
@@ -174,6 +190,8 @@ func FindGitRef(ctx context.Context, file string) (string, error) {
|
||||
|
||||
// FindGithubRepo get the repo
|
||||
func FindGithubRepo(ctx context.Context, file, githubInstance, remoteName string) (string, error) {
|
||||
goGitMu.Lock()
|
||||
defer goGitMu.Unlock()
|
||||
if remoteName == "" {
|
||||
remoteName = "origin"
|
||||
}
|
||||
|
||||
@@ -16,7 +16,6 @@ import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
log "github.com/sirupsen/logrus"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
@@ -50,10 +49,6 @@ func TestFindGitSlug(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func testDir(t *testing.T) string {
|
||||
return t.TempDir()
|
||||
}
|
||||
|
||||
func cleanGitHooks(dir string) error {
|
||||
hooksDir := filepath.Join(dir, ".git", "hooks")
|
||||
files, err := os.ReadDir(hooksDir)
|
||||
@@ -78,8 +73,7 @@ func cleanGitHooks(dir string) error {
|
||||
func TestFindGitRemoteURL(t *testing.T) {
|
||||
assert := assert.New(t)
|
||||
|
||||
basedir := testDir(t)
|
||||
gitConfig()
|
||||
basedir := t.TempDir()
|
||||
err := gitCmd("init", basedir)
|
||||
assert.NoError(err) //nolint:testifylint // pre-existing issue from nektos/act
|
||||
err = cleanGitHooks(basedir)
|
||||
@@ -102,8 +96,7 @@ func TestFindGitRemoteURL(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestGitFindRef(t *testing.T) {
|
||||
basedir := testDir(t)
|
||||
gitConfig()
|
||||
basedir := t.TempDir()
|
||||
|
||||
for name, tt := range map[string]struct {
|
||||
Prepare func(t *testing.T, dir string)
|
||||
@@ -180,36 +173,55 @@ func TestGitFindRef(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestGitCloneExecutor(t *testing.T) {
|
||||
// Build a local bare "remote" so this runs offline and fast. The cases below mirror
|
||||
// the tag/branch/sha/short-sha ref paths the executor handles, formerly exercised by
|
||||
// cloning actions/checkout and anchore/scan-action over the network.
|
||||
remoteDir := t.TempDir()
|
||||
require.NoError(t, gitCmd("init", "--bare", "--initial-branch=main", remoteDir))
|
||||
|
||||
workDir := t.TempDir()
|
||||
require.NoError(t, gitCmd("clone", remoteDir, workDir))
|
||||
require.NoError(t, gitCmd("-C", workDir, "checkout", "-b", "main"))
|
||||
require.NoError(t, gitCmd("-C", workDir, "commit", "--allow-empty", "-m", "initial"))
|
||||
require.NoError(t, gitCmd("-C", workDir, "tag", "v2"))
|
||||
require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main"))
|
||||
require.NoError(t, gitCmd("-C", workDir, "push", "origin", "v2"))
|
||||
|
||||
// A branch with a dash in the name (mirrors the historical scan-action@act-fails case).
|
||||
require.NoError(t, gitCmd("-C", workDir, "checkout", "-b", "act-fails"))
|
||||
require.NoError(t, gitCmd("-C", workDir, "commit", "--allow-empty", "-m", "branch-commit"))
|
||||
require.NoError(t, gitCmd("-C", workDir, "push", "origin", "act-fails"))
|
||||
|
||||
out, err := exec.Command("git", "-C", workDir, "rev-parse", "main").Output()
|
||||
require.NoError(t, err)
|
||||
fullSha := strings.TrimSpace(string(out))
|
||||
|
||||
for name, tt := range map[string]struct {
|
||||
Err error
|
||||
URL, Ref string
|
||||
Err error
|
||||
Ref string
|
||||
}{
|
||||
"tag": {
|
||||
Err: nil,
|
||||
URL: "https://github.com/actions/checkout",
|
||||
Ref: "v2",
|
||||
},
|
||||
"branch": {
|
||||
Err: nil,
|
||||
URL: "https://github.com/anchore/scan-action",
|
||||
Ref: "act-fails",
|
||||
},
|
||||
"sha": {
|
||||
Err: nil,
|
||||
URL: "https://github.com/actions/checkout",
|
||||
Ref: "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f", // v2
|
||||
Ref: fullSha,
|
||||
},
|
||||
"short-sha": {
|
||||
Err: &Error{ErrShortRef, "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f"},
|
||||
URL: "https://github.com/actions/checkout",
|
||||
Ref: "5a4ac90", // v2
|
||||
Err: &Error{ErrShortRef, fullSha},
|
||||
Ref: fullSha[:7],
|
||||
},
|
||||
} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
clone := NewGitCloneExecutor(NewGitCloneExecutorInput{
|
||||
URL: tt.URL,
|
||||
URL: remoteDir,
|
||||
Ref: tt.Ref,
|
||||
Dir: testDir(t),
|
||||
Dir: t.TempDir(),
|
||||
})
|
||||
|
||||
err := clone(context.Background())
|
||||
@@ -228,8 +240,6 @@ func TestGitCloneExecutorNonFastForwardRef(t *testing.T) {
|
||||
// non-fast-forward between two fetches. Before the fix, the fetch used Force=false,
|
||||
// causing go-git to return ErrForceNeeded and short-circuit the checkout.
|
||||
|
||||
gitConfig()
|
||||
|
||||
// Create a bare "remote" repo with an initial commit on main and a feature branch.
|
||||
remoteDir := t.TempDir()
|
||||
require.NoError(t, gitCmd("init", "--bare", "--initial-branch=main", remoteDir))
|
||||
@@ -280,8 +290,6 @@ func TestGitCloneExecutorNonFastForwardRef(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestGitCloneExecutorOfflineMode(t *testing.T) {
|
||||
gitConfig()
|
||||
|
||||
// Build a local "remote" with a single commit on main.
|
||||
remoteDir := t.TempDir()
|
||||
require.NoError(t, gitCmd("init", "--bare", "--initial-branch=main", remoteDir))
|
||||
@@ -327,22 +335,21 @@ func TestGitCloneExecutorOfflineMode(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func gitConfig() {
|
||||
if os.Getenv("GITHUB_ACTIONS") == "true" {
|
||||
var err error
|
||||
if err = gitCmd("config", "--global", "user.email", "test@test.com"); err != nil {
|
||||
log.Error(err)
|
||||
}
|
||||
if err = gitCmd("config", "--global", "user.name", "Unit Test"); err != nil {
|
||||
log.Error(err)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func gitCmd(args ...string) error {
|
||||
cmd := exec.Command("git", args...)
|
||||
cmd.Stdout = os.Stdout
|
||||
cmd.Stderr = os.Stderr
|
||||
// Inject a deterministic identity and ignore the host's global/system config so commits
|
||||
// succeed regardless of the host having no user.name/user.email (e.g. CI, GITHUB_ACTIONS
|
||||
// unset) or a global commit.gpgsign, and without mutating the developer's ~/.gitconfig.
|
||||
cmd.Env = append(os.Environ(),
|
||||
"GIT_AUTHOR_NAME=Unit Test",
|
||||
"GIT_AUTHOR_EMAIL=test@test.com",
|
||||
"GIT_COMMITTER_NAME=Unit Test",
|
||||
"GIT_COMMITTER_EMAIL=test@test.com",
|
||||
"GIT_CONFIG_GLOBAL=/dev/null",
|
||||
"GIT_CONFIG_SYSTEM=/dev/null",
|
||||
)
|
||||
|
||||
err := cmd.Run()
|
||||
if exitError, ok := err.(*exec.ExitError); ok {
|
||||
|
||||
Reference in New Issue
Block a user