From cd7182395f2b80bd2536077d0b45dced21a2c6b9 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 4 Nov 2026 22:51:34 +0100 Subject: [PATCH] fix: a crawl cancel waits for its job, and a stale crawl is ended Cancelling a running crawl records cancel_requested_at and leaves the row running until its job stops and writes cancelled, so no second crawl starts and the row cannot be deleted or pruned meanwhile. The job writes heartbeat_at every minute; a running crawl without a heartbeat for 10 minutes is ended by the next start or delete. --- README.md | 2 +- classes/crawl/actions.go | 7 +- classes/crawl/job.go | 124 +++++++++++++--- classes/crawl/store.go | 49 +++++++ controllers/api/crawls.go | 9 +- controllers/crawls.go | 6 +- crawl_lifecycle_test.go | 138 ++++++++++++++++++ lang/en/lang.yaml | 1 + lang/pl/lang.yaml | 1 + mcp.go | 2 +- models/crawl.go | 11 +- ...040002_add_golem15_seo_crawls_heartbeat.go | 31 ++++ 12 files changed, 351 insertions(+), 30 deletions(-) create mode 100644 updates/202611040002_add_golem15_seo_crawls_heartbeat.go diff --git a/README.md b/README.md index a3c0614..58c37aa 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. 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. +**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`) ends a queued crawl at once; a running crawl records the request (`cancel_requested` in the API and MCP answers, **Cancelling** in the list) and keeps running until its job stops between requests, then ends cancelled with what was fetched. Until then no other crawl starts (409 `crawls.already_running`) and the crawl cannot be deleted; 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). A running crawl's job writes `heartbeat_at` every minute (`crawl.HeartbeatEvery`); a running crawl without a heartbeat for 10 minutes (`crawl.StaleAfter`, its job is gone, for example after a worker restart) is ended by the next start or delete (`crawl.RecoverStale`): failed with "the crawl job stopped without finishing (no heartbeat since …)", or cancelled when its cancel was requested. 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/actions.go b/classes/crawl/actions.go index fa6e74d..e0e430b 100644 --- a/classes/crawl/actions.go +++ b/classes/crawl/actions.go @@ -71,11 +71,16 @@ func Unignore(tx *gorm.DB, issues []models.Issue) (int, error) { } // DeleteCrawls deletes crawls with their pages, links and issues; a -// queued or running one refuses the whole call with ErrDeleteRunning. +// queued or running one (also one whose cancel is requested) refuses the +// whole call with ErrDeleteRunning. A running crawl without a heartbeat is +// ended first (RecoverStale), so it can be deleted. func DeleteCrawls(tx *gorm.DB, ids []uint) (int, error) { if len(ids) == 0 { return 0, nil } + if _, err := RecoverStale(tx, time.Now()); err != nil { + return 0, err + } var active int64 if err := tx.Model(&models.Crawl{}).Where("id IN ? AND status IN ?", ids, []string{models.CrawlQueued, models.CrawlRunning}).Count(&active).Error; err != nil { return 0, err diff --git a/classes/crawl/job.go b/classes/crawl/job.go index b37da2a..510766e 100644 --- a/classes/crawl/job.go +++ b/classes/crawl/job.go @@ -45,12 +45,19 @@ 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. +// cancellation cannot reach; the job writes heartbeat_at every +// HeartbeatEvery, and a running crawl without a heartbeat for StaleAfter +// is ended by the next Start or delete (RecoverStale). const ( - JobTimeout = 6 * time.Hour - FinishTimeout = 10 * time.Minute + JobTimeout = 6 * time.Hour + FinishTimeout = 10 * time.Minute + HeartbeatEvery = time.Minute + StaleAfter = 10 * time.Minute ) +// heartbeatWrite bounds one heartbeat update. +const heartbeatWrite = 10 * time.Second + // The crawl limits used until the Site audit settings say otherwise. const ( DefaultMaxPages = 5000 @@ -78,9 +85,11 @@ func (e *AlreadyRunningError) Since() time.Time { // Start queues a crawl (D-14, P33-39) inside tx: the settings row is // locked so two starts cannot both pass the check that no crawl is queued -// or running, then the crawl row is written and the conga job dispatched -// in the same transaction. A missing site address refuses with -// ErrNoSiteURL, a running crawl with *AlreadyRunningError. +// or running, a running crawl whose job stopped writing its heartbeat is +// ended (RecoverStale), then the crawl row is written and the conga job +// dispatched in the same transaction. A missing site address refuses with +// ErrNoSiteURL, a queued or running crawl (also one whose cancel is +// requested but whose job has not stopped) with *AlreadyRunningError. func Start(ctx context.Context, svc *classes.Service, tx *gorm.DB, trigger, name string) (models.Crawl, error) { site := svc.SiteURL() if site == "" { @@ -92,6 +101,11 @@ func Start(ctx context.Context, svc *classes.Service, tx *gorm.DB, trigger, name if err := tx.Exec(`SELECT id FROM golem15_seo_settings WHERE id = 1 FOR UPDATE`).Error; err != nil { return models.Crawl{}, err } + stale, err := RecoverStale(tx, time.Now().UTC()) + if err != nil { + return models.Crawl{}, err + } + endStaleJobs(ctx, svc, tx, stale) active, err := ActiveCrawl(tx) if err != nil { return models.Crawl{}, err @@ -182,6 +196,7 @@ func LoadCrawl(db *gorm.DB, id uint) (models.Crawl, error) { // 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. +// While it runs, the job writes heartbeat_at every HeartbeatEvery. func Run(ctx context.Context, svc *classes.Service, id uint) error { gdb, err := svc.GormDB() if err != nil { @@ -197,19 +212,49 @@ func Run(ctx context.Context, svc *classes.Service, id uint) error { } now := time.Now().UTC() moved := db.Model(&models.Crawl{}).Where("id = ? AND status = ?", id, models.CrawlQueued). - Updates(map[string]any{"status": models.CrawlRunning, "started_at": now}) + Updates(map[string]any{"status": models.CrawlRunning, "started_at": now, "heartbeat_at": now}) if moved.Error != nil { return moved.Error } if moved.RowsAffected == 0 { return nil } + stop := heartbeat(ctx, gdb, id) + defer stop() res, runErr := run(ctx, svc, db, row) fctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), FinishTimeout) defer cancel() return finish(fctx, svc, gdb.WithContext(fctx), row, res, runErr, ctx.Err()) } +// heartbeat writes the crawl's heartbeat_at every HeartbeatEvery until the +// returned stop is called; each write runs on a context detached from the +// job's, bounded by heartbeatWrite. +func heartbeat(ctx context.Context, gdb *gorm.DB, id uint) (stop func()) { + done := make(chan struct{}) + finished := make(chan struct{}) + go func() { + defer close(finished) + t := time.NewTicker(HeartbeatEvery) + defer t.Stop() + for { + select { + case <-done: + return + case <-t.C: + hctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), heartbeatWrite) + _ = gdb.WithContext(hctx).Model(&models.Crawl{}).Where("id = ? AND status = ?", id, models.CrawlRunning). + Update("heartbeat_at", time.Now().UTC()).Error + cancel() + } + } + }() + return func() { + close(done) + <-finished + } +} + // run fetches and checks the site of one crawl. func run(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Crawl) (*Result, error) { f, err := NewFetcher(row.Site, svc.Config()) @@ -267,14 +312,17 @@ func run(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.Craw return Crawl(ctx, f, opt) } -// cancelRequested reports whether the crawl was cancelled: its status is -// cancelled. +// cancelRequested reports whether a cancel of the crawl was requested: its +// cancel_requested_at is set, or 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 { + var rows []struct { + Status string + Requested bool + } + if err := db.Raw(`SELECT status, cancel_requested_at IS NOT NULL AS requested FROM golem15_seo_crawls WHERE id = ?`, id).Scan(&rows).Error; err != nil || len(rows) == 0 { return false } - return status == models.CrawlCancelled + return rows[0].Requested || rows[0].Status == models.CrawlCancelled } // timeLimitError is the error of a crawl that reached JobTimeout. @@ -370,6 +418,31 @@ func finish(ctx context.Context, svc *classes.Service, db *gorm.DB, row models.C return nil } +// endStaleJobs fails or stops the conga jobs of crawls RecoverStale ended, +// once tx commits; best effort. +func endStaleJobs(ctx context.Context, svc *classes.Service, tx *gorm.DB, rows []models.Crawl) { + if len(rows) == 0 { + return + } + lagoon.AfterCommit(ctx, tx, func(ctx context.Context, _ *gorm.DB) { + m, err := conga.From(svc.App()) + if err != nil { + return + } + for _, r := range rows { + if r.JobID == nil { + continue + } + meta := map[string]any{"crawl_id": r.ID, "status": r.Status} + if r.Status == models.CrawlCancelled { + _ = m.StopJob(ctx, *r.JobID, meta) + } else { + _ = m.FailJob(ctx, *r.JobID, meta) + } + } + }) +} + // Lookup returns the CMS record behind a crawled address (cached per // address): the content type whose Match answers the path exactly, its // descriptor facts in the matched locale, its primary parent's address and @@ -455,20 +528,31 @@ func Keyphrases(db *gorm.DB) (map[string]string, error) { // running. var errCancelledCrawl = errors.New("golem15.seo: the crawl is not running") -// Cancel stops a queued or running crawl inside tx: the row is marked -// cancelled by name (the crawl keeps what it fetched) and its conga job -// cancelled. +// Cancel stops a queued or running crawl inside tx. A queued crawl ends +// cancelled at once (by name, finished_at set). A running crawl gets +// cancel_requested_at and cancelled_by and stays running: its job stops +// between requests and writes cancelled with the pages and issues fetched +// so far, and until then no other crawl starts. The conga job is cancelled +// in both cases. func Cancel(ctx context.Context, svc *classes.Service, tx *gorm.DB, row models.Crawl, name string) error { if !row.Active() { return errCancelledCrawl } now := time.Now().UTC() - updates := map[string]any{"status": models.CrawlCancelled, "cancelled_by": name} - if row.Status == models.CrawlQueued { - updates["finished_at"] = now + queued := tx.Model(&models.Crawl{}).Where("id = ? AND status = ?", row.ID, models.CrawlQueued). + Updates(map[string]any{"status": models.CrawlCancelled, "cancelled_by": name, "finished_at": now}) + if queued.Error != nil { + return queued.Error } - if err := tx.Model(&models.Crawl{}).Where("id = ?", row.ID).Updates(updates).Error; err != nil { - return err + if queued.RowsAffected == 0 { + running := tx.Model(&models.Crawl{}).Where("id = ? AND status = ?", row.ID, models.CrawlRunning). + Updates(map[string]any{"cancel_requested_at": now, "cancelled_by": name}) + if running.Error != nil { + return running.Error + } + if running.RowsAffected == 0 { + return errCancelledCrawl + } } if row.JobID != nil { if m, err := conga.From(svc.App()); err == nil { diff --git a/classes/crawl/store.go b/classes/crawl/store.go index da7e66a..54a0507 100644 --- a/classes/crawl/store.go +++ b/classes/crawl/store.go @@ -189,6 +189,55 @@ func ActiveCrawl(db *gorm.DB) (*models.Crawl, error) { return &rows[0], nil } +// RecoverStale ends the running crawls whose job stopped writing its +// heartbeat (P33-60): a running row whose COALESCE(heartbeat_at, +// started_at, created_at) is older than StaleAfter before now becomes +// cancelled when its cancel was requested, else failed with the error +// "the crawl job stopped without finishing (no heartbeat since