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

@@ -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 (+<site>)`. 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 `<img>` 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.
 

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))

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 cancelRequested(db, row.ID) {
-			if err := db.Raw(`SELECT status FROM golem15_seo_crawls WHERE id = ?`, row.ID).Scan(&status).Error; err == nil && status == models.CrawlCancelled {
 				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.
+// cancelRequested reports whether the crawl was cancelled: its status is
-func finish(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Crawl, res *Result, runErr error) error {
+// cancelled.
-	status := models.CrawlFinished
+func cancelRequested(db *gorm.DB, id uint) bool {
-	errText := ""
+	var status string
-	if runErr != nil {
+	if err := db.Raw(`SELECT status FROM golem15_seo_crawls WHERE id = ?`, id).Scan(&status).Error; err != nil {
-		status, errText = models.CrawlFailed, runErr.Error()
+		return false
 	}
-	var current string
+	return status == models.CrawlCancelled
-	_ = db.Raw(`SELECT status FROM golem15_seo_crawls WHERE id = ?`, row.ID).Scan(&current).Error
+}
-	if current == models.CrawlCancelled || (res != nil && res.Canceled) {
+
-		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 {

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)
+}

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)),
 	}
 }