diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 530fb5ae..24ba47fc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -57,7 +57,7 @@ jobs: tags: | type=ref,event=branch type=ref,event=pr - type=sha,prefix={{branch}}-,format=short + type=sha,prefix={{branch}}-,format=short,enable={{is_default_branch}} type=raw,value=latest,enable={{is_default_branch}} - name: Build and push Docker image @@ -65,7 +65,7 @@ jobs: with: context: . platforms: linux/amd64 - push: true + push: ${{ github.event_name != 'pull_request' }} tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} cache-from: type=gha diff --git a/internal/services/crawler.go b/internal/services/crawler.go index a16f1c96..4a5e521a 100644 --- a/internal/services/crawler.go +++ b/internal/services/crawler.go @@ -68,12 +68,6 @@ func NewCrawlerService(r CrawlerServiceRepository, s CrawlerServicesContainer) * // running or if there's an error creating it. // Finally the previous crawl's data is removed and the crawl is returned. func (s *CrawlerService) StartCrawler(p models.Project, b models.BasicAuth) error { - previousCrawl := s.repository.GetLastCrawl(&p) - crawl, err := s.repository.SaveCrawl(p) - if err != nil { - return err - } - u, err := url.Parse(p.URL) if err != nil { return err @@ -83,11 +77,22 @@ func (s *CrawlerService) StartCrawler(p models.Project, b models.BasicAuth) erro u.Path = "/" } + // Acquire the in-memory lock before any DB writes so that a rejected + // duplicate trigger cannot leave an orphaned crawl record with a NULL + // end timestamp. c, err := s.addCrawler(u, &p, &b) if err != nil { return err } + previousCrawl := s.repository.GetLastCrawl(&p) + + crawl, err := s.repository.SaveCrawl(p) + if err != nil { + s.removeCrawler(&p) + return err + } + go func() { defer s.removeCrawler(&p) defer s.repository.DeleteCrawlData(&previousCrawl) diff --git a/internal/services/crawler_test.go b/internal/services/crawler_test.go new file mode 100644 index 00000000..7be341e8 --- /dev/null +++ b/internal/services/crawler_test.go @@ -0,0 +1,89 @@ +package services_test + +import ( + "testing" + + "github.com/stjudewashere/seonaut/internal/config" + "github.com/stjudewashere/seonaut/internal/models" + "github.com/stjudewashere/seonaut/internal/services" +) + +// crawlerTestRepository is a minimal mock that counts SaveCrawl calls. +type crawlerTestRepository struct { + saveCrawlCount int +} + +func (r *crawlerTestRepository) SaveCrawl(p models.Project) (*models.Crawl, error) { + r.saveCrawlCount++ + return &models.Crawl{Id: 1, ProjectId: p.Id, URL: p.URL}, nil +} + +func (r *crawlerTestRepository) GetLastCrawl(p *models.Project) models.Crawl { + return models.Crawl{} +} + +func (r *crawlerTestRepository) GetLastCrawls(p models.Project, limit int) []models.Crawl { + return []models.Crawl{} +} + +func (r *crawlerTestRepository) DeleteCrawlData(c *models.Crawl) {} + +func (r *crawlerTestRepository) CountIssuesByPriority(crawlId int64, priority int) int { + return 0 +} + +func (r *crawlerTestRepository) UpdateCrawl(c *models.Crawl) {} + +type crawlerHandlerTestRepository struct{} + +func (r *crawlerHandlerTestRepository) SavePageReport(pr *models.PageReport, crawlId int64) (*models.PageReport, error) { + return pr, nil +} + +type crawlerReportManagerTestRepository struct{} + +func (r *crawlerReportManagerTestRepository) SaveIssues(issues <-chan *models.Issue) { + for range issues { + } +} + +func newTestCrawlerService(repo *crawlerTestRepository) *services.CrawlerService { + broker := services.NewPubSubBroker() + reportManager := services.NewReportManager(&crawlerReportManagerTestRepository{}) + handler := services.NewCrawlerHandler(&crawlerHandlerTestRepository{}, broker, reportManager) + + return services.NewCrawlerService(repo, services.CrawlerServicesContainer{ + Broker: broker, + ReportManager: reportManager, + CrawlerHandler: handler, + ArchiveService: services.NewArchiveService(""), + Config: &config.CrawlerConfig{Agent: "testbot"}, + }) +} + +// TestStartCrawlerNoDuplicateDBRecord verifies that when StartCrawler is called +// while a crawl is already in progress, it returns an error and does not write +// a second crawl record to the DB — preventing the orphaned NULL-end-timestamp +// bug that permanently blocks future crawls. +func TestStartCrawlerNoDuplicateDBRecord(t *testing.T) { + repo := &crawlerTestRepository{} + svc := newTestCrawlerService(repo) + + // localhost:1 refuses connections immediately, so the background goroutine + // finishes quickly without making the test slow. + p := models.Project{Id: 1, URL: "http://localhost:1"} + + if err := svc.StartCrawler(p, models.BasicAuth{}); err != nil { + t.Fatalf("first StartCrawler: unexpected error: %v", err) + } + + // Second call while the first goroutine still holds the in-memory lock. + err := svc.StartCrawler(p, models.BasicAuth{}) + if err == nil { + t.Fatal("second StartCrawler: expected an error, got nil") + } + + if repo.saveCrawlCount != 1 { + t.Errorf("SaveCrawl called %d time(s), want exactly 1", repo.saveCrawlCount) + } +}