From 00f5bfca1cd52b91626810bc25e1af953aec5284 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 Aug 2025 14:20:44 +0000 Subject: [PATCH 1/4] Initial plan From c2577d8157ef5fde8883fd0cf2a97d058a641e3b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 Aug 2025 14:30:28 +0000 Subject: [PATCH 2/4] Fix concurrent ansible-galaxy collection install race condition Co-authored-by: fiftin <914224+fiftin@users.noreply.github.com> --- db_lib/AnsiblePlaybook.go | 3 +- db_lib/LocalApp_test.go | 78 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/db_lib/AnsiblePlaybook.go b/db_lib/AnsiblePlaybook.go index 88c12c590..abd44ce62 100644 --- a/db_lib/AnsiblePlaybook.go +++ b/db_lib/AnsiblePlaybook.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "os/exec" + "path" "strings" "github.com/creack/pty" @@ -27,7 +28,7 @@ func (p AnsiblePlaybook) makeCmd(command string, args []string, environmentVars cmd.Env = append(cmd.Env, "ANSIBLE_HOST_KEY_CHECKING=False") //cmd.Env = append(cmd.Env, "ANSIBLE_SSH_ARGS=-o UserKnownHostsFile=/dev/null") cmd.Env = append(cmd.Env, getEnvironmentVars()...) - cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", util.Config.GetProjectTmpDir(p.Repository.ProjectID))) + cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", path.Join(util.Config.GetProjectTmpDir(p.Repository.ProjectID), fmt.Sprintf("template_%d", p.TemplateID)))) cmd.Env = append(cmd.Env, fmt.Sprintf("PWD=%s", cmd.Dir)) cmd.Env = append(cmd.Env, environmentVars...) diff --git a/db_lib/LocalApp_test.go b/db_lib/LocalApp_test.go index b36507f84..a90b5e7b1 100644 --- a/db_lib/LocalApp_test.go +++ b/db_lib/LocalApp_test.go @@ -2,9 +2,13 @@ package db_lib import ( "os" + "os/exec" "strings" "testing" + "time" + "github.com/semaphoreui/semaphore/db" + "github.com/semaphoreui/semaphore/pkg/task_logger" "github.com/semaphoreui/semaphore/util" ) @@ -48,3 +52,77 @@ func TestGetEnvironmentVars(t *testing.T) { } } } + +func TestAnsiblePlaybookTemplateSpecificHome(t *testing.T) { + util.Config = &util.ConfigType{ + TmpPath: "/tmp", + Process: &util.ConfigProcess{}, // Empty process config to avoid nil pointer + } + + // Create two different playbooks with different template IDs + playbook1 := AnsiblePlaybook{ + TemplateID: 123, + Repository: db.Repository{ + ProjectID: 42, + }, + Logger: &mockLogger{}, + } + + playbook2 := AnsiblePlaybook{ + TemplateID: 456, + Repository: db.Repository{ + ProjectID: 42, // Same project but different template + }, + Logger: &mockLogger{}, + } + + // Test that both playbooks get different HOME directories + cmd1 := playbook1.makeCmd("test-command", []string{}, nil) + cmd2 := playbook2.makeCmd("test-command", []string{}, nil) + + // Extract HOME environment variables + var home1, home2 string + for _, env := range cmd1.Env { + if strings.HasPrefix(env, "HOME=") { + home1 = env[5:] // Remove "HOME=" prefix + break + } + } + for _, env := range cmd2.Env { + if strings.HasPrefix(env, "HOME=") { + home2 = env[5:] // Remove "HOME=" prefix + break + } + } + + // Verify HOME directories are different + if home1 == home2 { + t.Errorf("Expected different HOME directories for different templates, but got same: %s", home1) + } + + // Verify HOME directories are template-specific + expectedHome1 := "/tmp/project_42/template_123" + expectedHome2 := "/tmp/project_42/template_456" + + if home1 != expectedHome1 { + t.Errorf("Expected HOME for template 123 to be %s, got %s", expectedHome1, home1) + } + + if home2 != expectedHome2 { + t.Errorf("Expected HOME for template 456 to be %s, got %s", expectedHome2, home2) + } +} + +// mockLogger implements task_logger.Logger for testing +type mockLogger struct{} + +func (l *mockLogger) Log(msg string) {} +func (l *mockLogger) Logf(format string, a ...any) {} +func (l *mockLogger) LogWithTime(now time.Time, msg string) {} +func (l *mockLogger) LogfWithTime(now time.Time, format string, a ...any) {} +func (l *mockLogger) LogCmd(cmd *exec.Cmd) {} +func (l *mockLogger) SetStatus(status task_logger.TaskStatus) {} +func (l *mockLogger) AddStatusListener(l2 task_logger.StatusListener) {} +func (l *mockLogger) AddLogListener(l2 task_logger.LogListener) {} +func (l *mockLogger) SetCommit(hash, message string) {} +func (l *mockLogger) WaitLog() {} From c37dfad75dd08906ab44ba85e1dd838a04c3d387 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 Aug 2025 16:08:43 +0000 Subject: [PATCH 3/4] Refactor directory structure to template-based organization Co-authored-by: fiftin <914224+fiftin@users.noreply.github.com> --- db/Repository.go | 6 ++++-- db/Repository_test.go | 2 +- services/tasks/LocalJob_inventory.go | 10 +++++----- services/tasks/TaskRunner_test.go | 10 +++++----- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/db/Repository.go b/db/Repository.go index 4b1e1297e..8ee453995 100644 --- a/db/Repository.go +++ b/db/Repository.go @@ -33,7 +33,9 @@ type Repository struct { } func (r Repository) ClearCache() error { - return util.ClearDir(util.Config.GetProjectTmpDir(r.ProjectID), true, r.getDirNamePrefix()) + // In the new template-based structure, clear all template directories that might contain this repository + // This is a temporary solution - ideally we'd only clear templates that actually use this repository + return util.ClearDir(util.Config.GetProjectTmpDir(r.ProjectID), true, "template_") } func (r Repository) getDirNamePrefix() string { @@ -41,7 +43,7 @@ func (r Repository) getDirNamePrefix() string { } func (r Repository) GetDirName(templateID int) string { - return r.getDirNamePrefix() + "template_" + strconv.Itoa(templateID) + return path.Join("template_" + strconv.Itoa(templateID), "src") } func (r Repository) GetFullPath(templateID int) string { diff --git a/db/Repository_test.go b/db/Repository_test.go index cfe890570..5810c283b 100644 --- a/db/Repository_test.go +++ b/db/Repository_test.go @@ -21,7 +21,7 @@ func TestRepository_ClearCache(t *testing.T) { util.Config = &util.ConfigType{ TmpPath: path.Join(os.TempDir(), util.RandString(rand.Intn(10-4)+4)), } - repoDir := path.Join(util.Config.TmpPath, "project_0", "repository_123_55") + repoDir := path.Join(util.Config.TmpPath, "project_0", "template_55", "src") err := os.MkdirAll(repoDir, 0755) require.NoError(t, err) diff --git a/services/tasks/LocalJob_inventory.go b/services/tasks/LocalJob_inventory.go index b9f09101d..19c5cfe40 100644 --- a/services/tasks/LocalJob_inventory.go +++ b/services/tasks/LocalJob_inventory.go @@ -1,6 +1,7 @@ package tasks import ( + "fmt" "os" "path" "strconv" @@ -38,17 +39,16 @@ func (t *LocalJob) installInventory() (err error) { } func (t *LocalJob) tmpInventoryFilename() string { - if t.Inventory.Repository == nil { - return "inventory_" + strconv.Itoa(t.Inventory.ID) - } - return t.Inventory.Repository.GetDirName(t.Template.ID) + "_inventory_" + strconv.Itoa(t.Inventory.ID) + return "inventory_" + strconv.Itoa(t.Inventory.ID) } func (t *LocalJob) tmpInventoryFullPath() string { if t.Inventory.Repository != nil && t.Inventory.Repository.GetType() == db.RepositoryLocal { return t.Inventory.Repository.GetGitURL(true) } - pathname := path.Join(util.Config.GetProjectTmpDir(t.Template.ProjectID), t.tmpInventoryFilename()) + // Place inventory in template directory: /project_{projectID}/template_{templateID}/inventory_{inventoryID} + templateDir := path.Join(util.Config.GetProjectTmpDir(t.Template.ProjectID), fmt.Sprintf("template_%d", t.Template.ID)) + pathname := path.Join(templateDir, t.tmpInventoryFilename()) if t.Inventory.Type == db.InventoryStaticYaml { pathname += ".yml" } diff --git a/services/tasks/TaskRunner_test.go b/services/tasks/TaskRunner_test.go index 9f35b0c6c..0f221ed79 100644 --- a/services/tasks/TaskRunner_test.go +++ b/services/tasks/TaskRunner_test.go @@ -164,7 +164,7 @@ func TestGetRepoPath(t *testing.T) { } dir := tsk.job.(*LocalJob).App.(*db_lib.AnsibleApp).GetPlaybookDir() - if dir != "/tmp/project_0/repository_0_template_0/deploy" { + if dir != "/tmp/project_0/template_0/src/deploy" { t.Fatal("Invalid playbook dir: " + dir) } } @@ -210,7 +210,7 @@ func TestGetRepoPath_whenStartsWithSlash(t *testing.T) { } dir := tsk.job.(*LocalJob).App.(*db_lib.AnsibleApp).GetPlaybookDir() - if dir != "/tmp/project_0/repository_0_template_0/deploy" { + if dir != "/tmp/project_0/template_0/src/deploy" { t.Fatal("Invalid playbook dir: " + dir) } } @@ -567,7 +567,7 @@ func TestTaskGetPlaybookArgs(t *testing.T) { } res := strings.Join(args, " ") - if res != "-i /tmp/project_0/inventory_0 --extra-vars {\"semaphore_vars\":{\"task_details\":{\"id\":0,\"url\":null,\"username\":\"\"}}} test.yml" { + if res != "-i /tmp/project_0/template_0/inventory_0 --extra-vars {\"semaphore_vars\":{\"task_details\":{\"id\":0,\"url\":null,\"username\":\"\"}}} test.yml" { t.Fatal("incorrect result") } } @@ -623,7 +623,7 @@ func TestTaskGetPlaybookArgs2(t *testing.T) { } res := strings.Join(args, " ") - if res != "-i /tmp/project_0/inventory_0 --extra-vars {\"semaphore_vars\":{\"task_details\":{\"id\":0,\"url\":null,\"username\":\"\"}}} test.yml" { + if res != "-i /tmp/project_0/template_0/inventory_0 --extra-vars {\"semaphore_vars\":{\"task_details\":{\"id\":0,\"url\":null,\"username\":\"\"}}} test.yml" { t.Fatal("incorrect result") } } @@ -680,7 +680,7 @@ func TestTaskGetPlaybookArgs3(t *testing.T) { } res := strings.Join(args, " ") - if res != "-i /tmp/project_0/inventory_0 --extra-vars {\"semaphore_vars\":{\"task_details\":{\"id\":0,\"url\":null,\"username\":\"\"}}} test.yml" { + if res != "-i /tmp/project_0/template_0/inventory_0 --extra-vars {\"semaphore_vars\":{\"task_details\":{\"id\":0,\"url\":null,\"username\":\"\"}}} test.yml" { t.Fatal("incorrect result") } } From 915f5e67a506db0c219d608159ca920b962e80eb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 Aug 2025 16:54:36 +0000 Subject: [PATCH 4/4] Optimize Repository.ClearCache to only clear templates using the repository Co-authored-by: fiftin <914224+fiftin@users.noreply.github.com> --- api/projects/keys.go | 2 +- api/projects/repository.go | 4 +- db/Repository.go | 24 +++++++++-- db/Repository_test.go | 88 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 108 insertions(+), 10 deletions(-) diff --git a/api/projects/keys.go b/api/projects/keys.go index 4ab9511fd..baa5a231a 100644 --- a/api/projects/keys.go +++ b/api/projects/keys.go @@ -143,7 +143,7 @@ func (c *KeyController) UpdateKey(w http.ResponseWriter, r *http.Request) { if repo.SSHKeyID != key.ID { continue } - err = repo.ClearCache() + err = repo.ClearCache(helpers.Store(r)) if err != nil { helpers.WriteError(w, err) return diff --git a/api/projects/repository.go b/api/projects/repository.go index 7bf7b5729..ee50e128b 100644 --- a/api/projects/repository.go +++ b/api/projects/repository.go @@ -169,7 +169,7 @@ func UpdateRepository(w http.ResponseWriter, r *http.Request) { } if oldRepo.GitURL != repository.GitURL { - util.LogWarning(oldRepo.ClearCache()) + util.LogWarning(oldRepo.ClearCache(helpers.Store(r))) } helpers.EventLog(r, helpers.EventLogUpdate, helpers.EventLogItem{ @@ -201,7 +201,7 @@ func RemoveRepository(w http.ResponseWriter, r *http.Request) { return } - util.LogWarning(repository.ClearCache()) + util.LogWarning(repository.ClearCache(helpers.Store(r))) helpers.EventLog(r, helpers.EventLogDelete, helpers.EventLogItem{ UserID: helpers.UserFromContext(r).ID, diff --git a/db/Repository.go b/db/Repository.go index 8ee453995..e966c906d 100644 --- a/db/Repository.go +++ b/db/Repository.go @@ -2,6 +2,7 @@ package db import ( "fmt" + "os" "path" "regexp" "strconv" @@ -32,10 +33,25 @@ type Repository struct { SSHKey AccessKey `db:"-" json:"-" backup:"-"` } -func (r Repository) ClearCache() error { - // In the new template-based structure, clear all template directories that might contain this repository - // This is a temporary solution - ideally we'd only clear templates that actually use this repository - return util.ClearDir(util.Config.GetProjectTmpDir(r.ProjectID), true, "template_") +func (r Repository) ClearCache(templateManager TemplateManager) error { + // Find templates that use this repository and clear only their cache directories + templates, err := templateManager.GetTemplates(r.ProjectID, TemplateFilter{}, RetrieveQueryParams{}) + if err != nil { + return err + } + + // Clear cache only for templates that use this repository + for _, template := range templates { + if template.RepositoryID == r.ID { + templateDir := path.Join(util.Config.GetProjectTmpDir(r.ProjectID), "template_" + strconv.Itoa(template.ID)) + err := os.RemoveAll(templateDir) + if err != nil && !os.IsNotExist(err) { + return err + } + } + } + + return nil } func (r Repository) getDirNamePrefix() string { diff --git a/db/Repository_test.go b/db/Repository_test.go index 5810c283b..30ceaf57f 100644 --- a/db/Repository_test.go +++ b/db/Repository_test.go @@ -21,12 +21,19 @@ func TestRepository_ClearCache(t *testing.T) { util.Config = &util.ConfigType{ TmpPath: path.Join(os.TempDir(), util.RandString(rand.Intn(10-4)+4)), } - repoDir := path.Join(util.Config.TmpPath, "project_0", "template_55", "src") + repoDir := path.Join(util.Config.TmpPath, "project_0", "template_55") err := os.MkdirAll(repoDir, 0755) require.NoError(t, err) - repo := Repository{ID: 123} - err = repo.ClearCache() + // Create a mock template manager that returns a template using this repository + mockTemplateManager := &mockTemplateManager{ + templates: []Template{ + {ID: 55, ProjectID: 0, RepositoryID: 123}, + }, + } + + repo := Repository{ID: 123, ProjectID: 0} + err = repo.ClearCache(mockTemplateManager) require.NoError(t, err) _, err = os.Stat(repoDir) @@ -34,6 +41,81 @@ func TestRepository_ClearCache(t *testing.T) { assert.True(t, os.IsNotExist(err)) } +func TestRepository_ClearCache_NoTemplatesUsingRepo(t *testing.T) { + util.Config = &util.ConfigType{ + TmpPath: path.Join(os.TempDir(), util.RandString(rand.Intn(10-4)+4)), + } + repoDir := path.Join(util.Config.TmpPath, "project_0", "template_55") + err := os.MkdirAll(repoDir, 0755) + require.NoError(t, err) + + // Create a mock template manager with no templates using this repository + mockTemplateManager := &mockTemplateManager{ + templates: []Template{ + {ID: 55, ProjectID: 0, RepositoryID: 999}, // Different repository ID + }, + } + + repo := Repository{ID: 123, ProjectID: 0} + err = repo.ClearCache(mockTemplateManager) + require.NoError(t, err) + + // Directory should still exist since no templates use this repository + _, err = os.Stat(repoDir) + require.NoError(t, err, "repo directory should not be deleted") +} + +// Mock template manager for Repository tests +type mockTemplateManager struct { + templates []Template +} + +func (m *mockTemplateManager) GetTemplates(projectID int, filter TemplateFilter, params RetrieveQueryParams) ([]Template, error) { + var result []Template + for _, template := range m.templates { + if template.ProjectID == projectID { + result = append(result, template) + } + } + return result, nil +} + +func (m *mockTemplateManager) GetTemplateRefs(projectID int, templateID int) (ObjectReferrers, error) { + return ObjectReferrers{}, nil +} + +func (m *mockTemplateManager) CreateTemplate(template Template) (Template, error) { + return Template{}, nil +} + +func (m *mockTemplateManager) UpdateTemplate(template Template) error { + return nil +} + +func (m *mockTemplateManager) GetTemplate(projectID int, templateID int) (Template, error) { + return Template{}, nil +} + +func (m *mockTemplateManager) DeleteTemplate(projectID int, templateID int) error { + return nil +} + +func (m *mockTemplateManager) SetTemplateDescription(projectID int, templateID int, description string) error { + return nil +} + +func (m *mockTemplateManager) GetTemplateVaults(projectID int, templateID int) ([]TemplateVault, error) { + return nil, nil +} + +func (m *mockTemplateManager) CreateTemplateVault(vault TemplateVault) (TemplateVault, error) { + return TemplateVault{}, nil +} + +func (m *mockTemplateManager) UpdateTemplateVaults(projectID int, templateID int, vaults []TemplateVault) error { + return nil +} + func TestRepository_GetGitURL(t *testing.T) { for _, v := range []struct { Repository Repository