From d7597bb1a5ee79c00a7da0886f83ba76c2cc8125 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 4 Nov 2026 22:50:22 +0100 Subject: [PATCH] fix: a cancelled or timed-out crawl stores what it fetched and ends The crawl job's context ends on a cancel or at the job timeout. Storing the pages, links and issues now runs on a context detached from it with its own 10-minute bound, a timeout ends the crawl failed with a time-limit error, a LoadLimits failure falls back to the defaults, and a fetch cut short by the cancellation is recorded as canceled. --- README.md | 2 +- classes/crawl/crawler.go | 4 + classes/crawl/job.go | 77 ++++++++++++++---- crawl_lifecycle_test.go | 165 +++++++++++++++++++++++++++++++++++++++ jobs.go | 3 +- 5 files changed, 234 insertions(+), 17 deletions(-) create mode 100644 crawl_lifecycle_test.go diff --git a/README.md b/README.md index 17b436b..a3c0614 100644 --- a/README.md +++ b/README.md @@ -247,7 +247,7 @@ A crawl is a conga job (`golem15.seo.crawl`, one attempt) started from **Start c **What it fetches.** `robots.txt`, the sitemap index at `/sitemap.xml` and its files (or a single urlset), then breadth-first from the home page and the sitemap addresses over same-host links, redirect targets, canonicals and hreflang alternates. Requests go through a `fetchguard` client limited to the site host and its `www` twin: HTTPS only, private and reserved addresses refused at dial time, redirects never followed (each `Location` is a new address checked again), the User-Agent `SummerCMS-SEO-Crawler/1 (+)`. Links to other hosts, credentials in a URL and plain http are never fetched. A staging or local site served over http or on a private address needs `golem15.seo.crawl_trusted: true` in config (or `SUMMER_GOLEM15__SEO__CRAWL_TRUSTED`), which no setting or MCP call can set; trusted mode still fetches only the site host and its twin. robots.txt rules are read for the report, not obeyed. -**Limits.** Two requests at a time with the settings' pause between requests (default 250 ms), a 10 s timeout and a 5 MiB body cap per request, at most 1000 links read per page, and the settings' page limit (default 5000; a crawl that stops there is marked "limit reached"). Cancelling (**Cancel crawl**, `seo_cancel_crawl`) stops between requests and keeps what was fetched; a failed crawl keeps the pages and issues found before the failure and records the error. The crawler writes only its own tables (`golem15_seo_crawls`, `golem15_seo_crawl_pages`, `golem15_seo_crawl_links`, `golem15_seo_issues`); crawls beyond the settings' "Crawls kept" are deleted with their pages, links and issues. +**Limits.** Two requests at a time with the settings' pause between requests (default 250 ms), a 10 s timeout and a 5 MiB body cap per request, at most 1000 links read per page, and the settings' page limit (default 5000; a crawl that stops there is marked "limit reached"). Cancelling (**Cancel crawl**, `seo_cancel_crawl`) stops between requests and keeps what was fetched; a failed crawl keeps the pages and issues found before the failure and records the error. A crawl that reaches the job's time limit (`crawl.JobTimeout`, 6 h) ends failed with "the crawl reached its time limit of 6h" and keeps what it fetched: the final writes run on a context the job's cancellation cannot reach, bounded by `crawl.FinishTimeout` (10 min). The crawler writes only its own tables (`golem15_seo_crawls`, `golem15_seo_crawl_pages`, `golem15_seo_crawl_links`, `golem15_seo_issues`); crawls beyond the settings' "Crawls kept" are deleted with their pages, links and issues. **Checks** (kind, severity): `page_status` (error), `sitemap_error` (error), `sitemap_noindex` (error), `sitemap_blocked` (error), `sitemap_missing` (notice), `broken_link` (error, with the linking pages), `redirected_link` (notice), `canonical_missing` (warning), `canonical_error` (error), `missing_title` and `duplicate_title` (warning), `missing_description` and `duplicate_description` (notice), `h1_missing` and `h1_multiple` (error), `heading_skipped` and `heading_empty` (warning), `h1_title_mismatch` (notice), `jsonld_invalid` (error), `jsonld_multiple` (warning), `jsonld_required` (error) and `jsonld_recommended` (notice) from the same rules as the structured data preview (`schema.Validate`; FAQPage and other types without Google rules find nothing), `hreflang_reciprocal` (warning), `hreflang_error` (error), `generic_anchor` (warning, the settings' generic link texts, whole text, any case), `image_alt` (warning: an `` without `alt`; `alt=""` is decorative), `image_size` and `featured_lazy` (notice), `content_js` (warning) and `orphan_page` (warning: an indexable sitemap page no crawled page links to, with up to 10 pages whose text mentions its title or focus keyphrase). The CMS-data checks run at the end of every crawl (and when the list is opened before any crawl) and are stored with the crawl's issues as source `cms`; a CMS issue the crawl found too is stored once. diff --git a/classes/crawl/crawler.go b/classes/crawl/crawler.go index 224954b..7c6d4bd 100644 --- a/classes/crawl/crawler.go +++ b/classes/crawl/crawler.go @@ -247,6 +247,10 @@ func Crawl(ctx context.Context, f *Fetcher, opt Options) (*Result, error) { return } p.Resp = f.Get(ctx, it.key) + if p.Resp.Err != "" && ctx.Err() != nil { + // The fetch was cut short by the cancellation. + p.Resp = &Response{Err: "canceled"} + } p.FetchedAt = time.Now().UTC() if p.Resp.Err == "" && p.Resp.Status == 200 && p.Resp.HTML() { base, _ := url.Parse(f.Absolute(it.key)) diff --git a/classes/crawl/job.go b/classes/crawl/job.go index e5f5969..b37da2a 100644 --- a/classes/crawl/job.go +++ b/classes/crawl/job.go @@ -3,6 +3,7 @@ package crawl import ( "context" "errors" + "strconv" "strings" "time" @@ -42,6 +43,14 @@ type CrawlArgs struct { // Kind is CrawlKind. func (CrawlArgs) Kind() string { return CrawlKind } +// The crawl job's time bounds (P33-60). JobTimeout is the job's deadline; +// FinishTimeout bounds storing what a crawl found, on a context the job's +// cancellation cannot reach. +const ( + JobTimeout = 6 * time.Hour + FinishTimeout = 10 * time.Minute +) + // The crawl limits used until the Site audit settings say otherwise. const ( DefaultMaxPages = 5000 @@ -168,6 +177,11 @@ func LoadCrawl(db *gorm.DB, id uint) (models.Crawl, error) { // the crawl finished, cancelled (pages and issues fetched so far are kept) // or failed (with the error; what was fetched before the failure is kept). // A crawl that is no longer queued is left alone. +// +// The job's context ends on a cancel (River cancels a running job's +// context) or at the job timeout; what was fetched is then stored on a +// context detached from it with its own FinishTimeout bound, so a +// cancelled or timed-out crawl keeps its results and never stays running. func Run(ctx context.Context, svc *classes.Service, id uint) error { gdb, err := svc.GormDB() if err != nil { @@ -191,7 +205,9 @@ func Run(ctx context.Context, svc *classes.Service, id uint) error { return nil } res, runErr := run(ctx, svc, db, row) - return finish(ctx, svc, db, row, res, runErr) + fctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), FinishTimeout) + defer cancel() + return finish(fctx, svc, gdb.WithContext(fctx), row, res, runErr, ctx.Err()) } // run fetches and checks the site of one crawl. @@ -228,8 +244,7 @@ func run(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Craw RenderErr: renderErr, RenderMax: RenderMax(svc.Config()), Canceled: func(ctx context.Context) bool { - var status string - if err := db.Raw(`SELECT status FROM golem15_seo_crawls WHERE id = ?`, row.ID).Scan(&status).Error; err == nil && status == models.CrawlCancelled { + if cancelRequested(db, row.ID) { return true } if m != nil && row.JobID != nil { @@ -252,21 +267,55 @@ func run(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Craw return Crawl(ctx, f, opt) } -// finish stores what a crawl found and writes its final state. -func finish(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Crawl, res *Result, runErr error) error { - status := models.CrawlFinished - errText := "" - if runErr != nil { - status, errText = models.CrawlFailed, runErr.Error() +// cancelRequested reports whether the crawl was cancelled: its status is +// cancelled. +func cancelRequested(db *gorm.DB, id uint) bool { + var status string + if err := db.Raw(`SELECT status FROM golem15_seo_crawls WHERE id = ?`, id).Scan(&status).Error; err != nil { + return false } - var current string - _ = db.Raw(`SELECT status FROM golem15_seo_crawls WHERE id = ?`, row.ID).Scan(¤t).Error - if current == models.CrawlCancelled || (res != nil && res.Canceled) { - status = models.CrawlCancelled + return status == models.CrawlCancelled +} + +// timeLimitError is the error of a crawl that reached JobTimeout. +func timeLimitError() string { + limit := JobTimeout.String() + if JobTimeout%time.Hour == 0 { + limit = strconv.Itoa(int(JobTimeout/time.Hour)) + "h" } + return "the crawl reached its time limit of " + limit +} + +// finalStatus decides a crawl's final status and error: a requested +// cancel, or a cancelled job context, ends cancelled; the job's deadline +// fails it with the time-limit error; another run error fails it with that +// error; else it finished (cancelled when the crawl stopped on a cancel). +func finalStatus(requested bool, res *Result, runErr, ctxErr error) (string, string) { + switch { + case requested: + return models.CrawlCancelled, "" + case errors.Is(ctxErr, context.DeadlineExceeded) || errors.Is(runErr, context.DeadlineExceeded): + return models.CrawlFailed, timeLimitError() + case errors.Is(ctxErr, context.Canceled) || errors.Is(runErr, context.Canceled): + return models.CrawlCancelled, "" + case runErr != nil: + return models.CrawlFailed, runErr.Error() + case res != nil && res.Canceled: + return models.CrawlCancelled, "" + } + return models.CrawlFinished, "" +} + +// finish stores what a crawl found and writes its final state. ctx is the +// detached, bounded context of Run, ctxErr the job context's error (nil +// while it was alive). A LoadLimits failure falls back to the default +// limits. +func finish(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Crawl, res *Result, runErr, ctxErr error) error { + status, errText := finalStatus(cancelRequested(db, row.ID), res, runErr, ctxErr) limits, err := LoadLimits(db) if err != nil { - return err + pluginLog.Warn("golem15.seo: crawl limits, using the defaults", "crawl", row.ID, "error", err) + limits = Limits{MaxPages: DefaultMaxPages, Delay: DefaultDelay, Keep: DefaultKeep} } store := func(ctx context.Context, tx *gorm.DB) error { if res == nil { diff --git a/crawl_lifecycle_test.go b/crawl_lifecycle_test.go new file mode 100644 index 0000000..c18d7d5 --- /dev/null +++ b/crawl_lifecycle_test.go @@ -0,0 +1,165 @@ +package seo_test + +import ( + "context" + "errors" + "net/http" + "strings" + "sync" + "testing" + "time" + + "git.golem15.com/golem15/sm-seo-plugin/classes/crawl" + "git.golem15.com/golem15/sm-seo-plugin/models" + "git.golem15.com/golem15/summercms/modules/fetchguard" +) + +// blockingSite is a fakeSite whose answer for one path waits until it is +// released or the request's context ends (then the request fails with the +// context's error, as a real transport does). reached is closed when that +// request arrives. +type blockingSite struct { + *fakeSite + path string + reached chan struct{} + release chan struct{} + once sync.Once + released sync.Once +} + +func newBlockingSite(path string) *blockingSite { + return &blockingSite{fakeSite: newFakeSite(), path: path, reached: make(chan struct{}), release: make(chan struct{})} +} + +func (s *blockingSite) RoundTrip(r *http.Request) (*http.Response, error) { + if r.URL.EscapedPath() == s.path { + s.once.Do(func() { close(s.reached) }) + select { + case <-s.release: + case <-r.Context().Done(): + return nil, r.Context().Err() + } + } + return s.fakeSite.RoundTrip(r) +} + +func (s *blockingSite) unblock() { s.released.Do(func() { close(s.release) }) } + +// lifecycleSite plants /, /a (no title: missing_title) and /b, listed in +// the sitemap, so the crawl fetches / and /a in its first batch and /b +// alone in the second. +func lifecycleSite(site *blockingSite) { + site.sitemap("/a", "/b") + site.plant( + doc{path: "/", title: "Home"}, + doc{path: "/a", noTitle: true}, + doc{path: "/b", title: "Page b"}, + ) +} + +// crawlRow reads a crawl row. +func (e *testEnv) crawlRow(t *testing.T, id uint) models.Crawl { + t.Helper() + row, err := crawl.LoadCrawl(e.db, id) + if err != nil { + t.Fatal(err) + } + return row +} + +// runCrawlAsync runs the crawl job of id on ctx in a goroutine; the +// returned channel answers the job's error. +func (e *testEnv) runCrawlAsync(t *testing.T, ctx context.Context, id uint) <-chan error { + t.Helper() + job := e.crawlJob(t) + done := make(chan error, 1) + go func() { done <- job.Work(ctx, crawl.CrawlArgs{CrawlID: id}) }() + return done +} + +// waitReached waits for the blocking request. +func waitReached(t *testing.T, site *blockingSite) { + t.Helper() + select { + case <-site.reached: + case <-time.After(20 * time.Second): + t.Fatal("the crawl never requested the blocking page") + } +} + +// waitJob waits for the crawl job to return. +func waitJob(t *testing.T, done <-chan error) error { + t.Helper() + select { + case err := <-done: + return err + case <-time.After(30 * time.Second): + t.Fatal("the crawl job did not return") + return nil + } +} + +// storedPages lists the stored pages of a crawl with their status. +func (e *testEnv) storedPages(t *testing.T, id uint) map[string]int { + t.Helper() + var rows []struct { + URL string + Status int + } + if err := e.db.Raw(`SELECT url, status FROM golem15_seo_crawl_pages WHERE crawl_id = ?`, id).Scan(&rows).Error; err != nil { + t.Fatal(err) + } + out := map[string]int{} + for _, r := range rows { + out[r.URL] = r.Status + } + return out +} + +// crawlIssueKinds lists the issue kinds per address of a crawl. +func (e *testEnv) crawlIssueKinds(t *testing.T, id uint) map[string][]string { + t.Helper() + var rows []struct { + URL string + Kind string + } + if err := e.db.Raw(`SELECT url, kind FROM golem15_seo_issues WHERE crawl_id = ?`, id).Scan(&rows).Error; err != nil { + t.Fatal(err) + } + out := map[string][]string{} + for _, r := range rows { + out[r.URL] = append(out[r.URL], r.Kind) + } + return out +} + +// TestCrawlTimeout pins L3 CR-01: a crawl whose job context reaches its +// deadline stores what it fetched and ends failed with the time-limit +// error and finished_at; a later Start succeeds. +func TestCrawlTimeout(t *testing.T) { + env := bootCrawlEnv(t) + token := env.admin(t, true) + if err := env.db.Exec(`UPDATE golem15_seo_settings SET crawl_delay = 100 WHERE id = 1`).Error; err != nil { + t.Fatal(err) + } + site := newBlockingSite("/b") + defer site.unblock() + lifecycleSite(site) + id := env.startCrawl(t, token) + ctx, cancel := context.WithTimeout(fetchguard.WithTransport(t.Context(), site), 3*time.Second) + defer cancel() + if err := waitJob(t, env.runCrawlAsync(t, ctx, id)); err != nil { + t.Fatalf("the timed-out job answered %v", err) + } + if !errors.Is(ctx.Err(), context.DeadlineExceeded) { + t.Fatal("the job returned before its deadline") + } + row := env.crawlRow(t, id) + if row.Status != models.CrawlFailed || !strings.Contains(row.Error, "time limit of 6h") || row.FinishedAt == nil { + t.Fatalf("row after the timeout = %+v", row) + } + if pages := env.storedPages(t, id); pages["/"] != 200 || pages["/a"] != 200 { + t.Fatalf("stored pages = %v, want / and /a", pages) + } + env.startCrawl(t, token) +} diff --git a/jobs.go b/jobs.go index d48fe60..47a69af 100644 --- a/jobs.go +++ b/jobs.go @@ -2,7 +2,6 @@ package seo import ( "context" - "time" "git.golem15.com/golem15/sm-seo-plugin/classes" "git.golem15.com/golem15/sm-seo-plugin/classes/crawl" @@ -16,7 +15,7 @@ import ( func (p *Plugin) Jobs() []pact.Job { return []pact.Job{ conga.Job(p.generateEntries, conga.MaxAttempts(3)), - conga.Job(p.crawl, conga.MaxAttempts(1), conga.Timeout(6*time.Hour)), + conga.Job(p.crawl, conga.MaxAttempts(1), conga.Timeout(crawl.JobTimeout)), } }