diff --git a/CHANGELOG.md b/CHANGELOG.md index fcd0731..d18d84b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,19 @@ All notable changes to kage are recorded here. The format follows ### Fixed +- `--resume` picks an interrupted crawl back up instead of doing nothing ([#36](https://github.com/tamnd/kage/issues/36)). + `state.json` persisted only the visited set, and the frontier was rebuilt purely by re-rendering pages and following their links, which resume exists to avoid. + So a resumed run found its seed already visited, `enqueuePage` turned it down, nothing was queued, and the run printed `pages 0` and exited successfully with most of the site still missing. + Only sites with a `sitemap.xml` appeared to work, because those URLs are seeded independently on every run. + The unfinished frontier is now saved alongside the visited set, with each page's depth so `--max-depth` keeps its meaning across a restart, and re-queued at startup. + A run now also reports what it is leaving behind: `resume: 412 pages still to do, rerun to continue`. +- A page that failed is retried by the next run instead of being lost. + Failures were never recorded anywhere, so the only way to pick them up was `--refresh`, which re-renders the entire site. + This is the "memory of what failed" asked for in [#36](https://github.com/tamnd/kage/issues/36). + A page `robots.txt` disallows is not carried over, since a later run would only fetch `robots.txt` and skip it again. +- `--max-pages` no longer discards the pages it held back. + They stay in the frontier, so `kage clone example.com -p 20` to inspect a site and then `kage clone example.com` to finish it now works as a workflow. + - Saved pages keep their `` instead of rendering in quirks mode ([#16](https://github.com/tamnd/kage/issues/16)). kage serialises a rendered page as the outerHTML of ``, and a doctype is a sibling of `` rather than a child, so it was never in that string and every page kage has ever written came out without one. A document with no doctype is quirks mode in every browser: the box model reverts to the pre-CSS2 IE one and `line-height`, table cell inheritance and `vertical-align` all change, so the saved copy laid out differently from the original, and the `` declaration lost its authority, leaving a reader free to fall back to its locale encoding and mojibake every multibyte character. diff --git a/clone/cloner.go b/clone/cloner.go index 2940e77..f597ec4 100644 --- a/clone/cloner.go +++ b/clone/cloner.go @@ -132,7 +132,7 @@ func (c *Cloner) Run(ctx context.Context) (Result, error) { if err := c.front.load(c.statePth); err != nil { c.logf("resume: could not load state: %v", err) } else if n := c.front.visitedCount(); n > 0 { - c.logf("resume: %d pages already done", n) + c.logf("resume: %s already done", pagesPlural(n)) } } @@ -169,8 +169,15 @@ func (c *Cloner) Run(ctx context.Context) (Result, error) { }) } - // Seed. + // Seed. A resumed run also re-queues the frontier the previous run saved on + // the way out. Without it the seed is already visited, enqueuePage turns it + // down, and since the frontier is otherwise rebuilt only by re-rendering + // pages and following their links, the run ends having done nothing at all + // (issue #36). c.enqueuePage(ctx, c.seed, 0) + if n := c.requeueUnfinished(ctx); n > 0 { + c.logf("resume: picking up %s", pagesPlural(n)) + } if c.cfg.FollowSitemap { c.seedSitemaps(ctx) } @@ -186,6 +193,11 @@ func (c *Cloner) Run(ctx context.Context) (Result, error) { if c.cfg.Persist { if err := c.front.save(c.statePth); err != nil { c.logf("could not save resume state: %v", err) + } else if n := c.front.pendingCount(); n > 0 { + // Pages left in the frontier: interrupted, over the page budget, or + // failed. Saying so is the "memory of what failed" asked for in issue + // #36, and it stops a short run looking like a finished one. + c.logf("resume: %s still to do, rerun to continue", pagesPlural(n)) } } @@ -268,6 +280,7 @@ func (c *Cloner) processPage(ctx context.Context, j pageItem) { key := c.pageKey(j.u) if c.cfg.RespectRobots && !c.robots.Allowed(j.u.Path) { c.stats.skipped.Add(1) + c.front.markDone(key) return } if !c.waitForCrawlDelay(ctx) { @@ -436,6 +449,32 @@ func classifyError(err error) string { return err.Error() } +// pagesPlural renders a page count with the right noun, for log lines that are +// as likely to say 1 as 1,400. +func pagesPlural(n int) string { + if n == 1 { + return "1 page" + } + return fmt.Sprintf("%d pages", n) +} + +// requeueUnfinished puts the previous run's outstanding frontier back in the +// queue and reports how many pages it queued. It is a no-op on a fresh run. +func (c *Cloner) requeueUnfinished(ctx context.Context) int { + n := 0 + for _, p := range c.front.unfinished() { + u, err := url.Parse(p.URL) + if err != nil { + c.logf("resume: dropping unreadable state entry %q: %v", p.URL, err) + continue + } + if c.enqueuePage(ctx, u, p.Depth) { + n++ + } + } + return n +} + // enqueuePage offers a page URL to the frontier, honouring the visited set, the // depth cap, and the page budget. It reports whether the page was newly queued. func (c *Cloner) enqueuePage(ctx context.Context, u *url.URL, depth int) bool { @@ -446,7 +485,10 @@ func (c *Cloner) enqueuePage(ctx context.Context, u *url.URL, depth int) bool { if c.front.isVisited(key) { return false } - if !c.front.offer(key) { + // Recording the page as unfinished happens here, before the budget check + // below, so a page that was discovered but never started is still in the + // frontier the next run picks up (issue #36). + if !c.front.offer(key, pendingPage{URL: u.String(), Depth: depth}) { return false } c.mu.Lock() diff --git a/clone/cloner_test.go b/clone/cloner_test.go index 5ef038f..dcb5576 100644 --- a/clone/cloner_test.go +++ b/clone/cloner_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -423,3 +424,173 @@ func readAnyFile(t *testing.T, dir, name string) string { }) return out } + +// chainSite is a five-page site linked in a chain, home to /p1 to /p2 to /p3 to +// /p4. Nothing but following links finds the far end, so a resumed run that has +// lost the frontier cannot reach it. +func chainSite(t *testing.T) *httptest.Server { + t.Helper() + page := func(title, next string) string { + body := `` + title + `

` + title + `

` + if next != "" { + body += `next` + } + return body + `` + } + links := map[string]string{ + "/": "/p1", + "/p1": "/p2", + "/p2": "/p3", + "/p3": "/p4", + "/p4": "", + } + mux := http.NewServeMux() + mux.HandleFunc("/robots.txt", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("User-agent: *\nAllow: /\n")) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + next, ok := links[r.URL.Path] + if !ok { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write([]byte(page(r.URL.Path, next))) + }) + return httptest.NewServer(mux) +} + +// TestCloneResumeFinishesTheCrawl guards issue #36. Only the visited set used to +// be persisted, and the frontier was rebuilt purely by re-rendering pages and +// following their links, which resume guarantees never happens. So an +// interrupted crawl, restarted, found its seed already visited, queued nothing, +// and exited successfully having done no work at all. The README promised the +// opposite. +func TestCloneResumeFinishesTheCrawl(t *testing.T) { + if testing.Short() { + t.Skip("resume test drives Chrome; skipped under -short") + } + if _, ok := browser.LookChrome(); !ok { + t.Skip("no Chrome/Chromium found; skipping resume test") + } + + srv := chainSite(t) + defer srv.Close() + seed, _ := urlx.ParseSeed(srv.URL) + + out := t.TempDir() + cfg := DefaultConfig() + cfg.OutDir = out + cfg.Settle = 300 * time.Millisecond + + ctx, cancel := context.WithTimeout(context.Background(), 180*time.Second) + defer cancel() + + // Stop the first run partway through, the way Ctrl-C would. + first := cfg + first.MaxPages = 2 + res1, err := New(seed, first, t.Logf).Run(ctx) + if err != nil { + t.Fatalf("first run: %v", err) + } + if res1.Pages != 2 { + t.Fatalf("first run wrote %d pages, want 2", res1.Pages) + } + + // Resume with no cap: it must pick up the frontier the first run left and + // walk the rest of the chain. + res2, err := New(seed, cfg, t.Logf).Run(ctx) + if err != nil { + t.Fatalf("resume run: %v", err) + } + if res2.Pages != 3 { + t.Errorf("resume wrote %d pages, want the 3 that were left", res2.Pages) + } + + root := filepath.Join(out, seed.Hostname()) + for _, p := range []string{"index.html", "p1/index.html", "p2/index.html", "p3/index.html", "p4/index.html"} { + if !fileExists(filepath.Join(root, p)) { + t.Errorf("%s was never written", p) + } + } + + // Nothing is left over, so a third run has genuinely nothing to do. + res3, err := New(seed, cfg, t.Logf).Run(ctx) + if err != nil { + t.Fatalf("third run: %v", err) + } + if res3.Pages != 0 { + t.Errorf("third run wrote %d pages, want 0", res3.Pages) + } +} + +// TestCloneResumeRetriesFailures covers the other half of issue #36: the +// reporter asked for "a memory of what failed" so a later run could pick the +// missing pages up. A page that errored is not marked visited, so it stays in +// the frontier and a resumed run tries it again. +func TestCloneResumeRetriesFailures(t *testing.T) { + if testing.Short() { + t.Skip("resume test drives Chrome; skipped under -short") + } + if _, ok := browser.LookChrome(); !ok { + t.Skip("no Chrome/Chromium found; skipping resume test") + } + + var flaky atomic.Bool + mux := http.NewServeMux() + mux.HandleFunc("/robots.txt", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("User-agent: *\nAllow: /\n")) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/": + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write([]byte(`flaky`)) + case "/flaky": + // Down for the first run, back up for the second. The connection is + // dropped rather than answered with a 5xx because Chrome renders an + // error page happily; only a dead connection is a render failure. + if !flaky.Load() { + if hj, ok := w.(http.Hijacker); ok { + if conn, _, err := hj.Hijack(); err == nil { + _ = conn.Close() + return + } + } + http.Error(w, "down", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write([]byte(`

back up

`)) + default: + http.NotFound(w, r) + } + }) + srv := httptest.NewServer(mux) + defer srv.Close() + seed, _ := urlx.ParseSeed(srv.URL) + + out := t.TempDir() + cfg := DefaultConfig() + cfg.OutDir = out + cfg.Settle = 300 * time.Millisecond + + ctx, cancel := context.WithTimeout(context.Background(), 180*time.Second) + defer cancel() + + if _, err := New(seed, cfg, t.Logf).Run(ctx); err != nil { + t.Fatalf("first run: %v", err) + } + flakyPath := filepath.Join(out, seed.Hostname(), "flaky", "index.html") + if fileExists(flakyPath) { + t.Fatal("the flaky page should not have been written while it was down") + } + + flaky.Store(true) + if _, err := New(seed, cfg, t.Logf).Run(ctx); err != nil { + t.Fatalf("resume run: %v", err) + } + if !fileExists(flakyPath) { + t.Error("resume should have retried the page that failed") + } +} diff --git a/clone/frontier.go b/clone/frontier.go index af01967..c1cf46c 100644 --- a/clone/frontier.go +++ b/clone/frontier.go @@ -8,29 +8,45 @@ import ( "sync" ) -// frontier is the deduped set of page URLs kage has already seen. It is small, -// concurrency-safe, and persists to disk so --resume can skip work already done. -// The actual queueing is handled by the cloner's channels; the frontier only -// answers "is this URL new?" and remembers the answer. +// frontier is the deduped set of page URLs kage has already seen, plus the work +// it has not finished. It is small, concurrency-safe, and persists to disk so +// --resume can skip what is done and pick up what is not. The actual queueing is +// handled by the cloner's channels; the frontier only answers "is this URL new?" +// and remembers the answer. type frontier struct { mu sync.Mutex - seen map[string]bool // queued or visited - visited map[string]bool // fully written + seen map[string]bool // queued or visited + visited map[string]bool // fully written + pending map[string]pendingPage // offered, not yet finished + resumed []pendingPage // unfinished work read from disk +} + +// pendingPage is a page kage meant to fetch and did not finish. The depth +// travels with it because --max-depth is measured from the seed, and a resumed +// run has no way to recompute it. +type pendingPage struct { + URL string `json:"url"` + Depth int `json:"depth"` } func newFrontier() *frontier { - return &frontier{seen: map[string]bool{}, visited: map[string]bool{}} + return &frontier{ + seen: map[string]bool{}, + visited: map[string]bool{}, + pending: map[string]pendingPage{}, + } } -// offer reports whether key is new (and records it as seen). A repeated key -// returns false so it is enqueued only once. -func (f *frontier) offer(key string) bool { +// offer reports whether key is new, recording it as seen and as unfinished work. +// A repeated key returns false so it is enqueued only once. +func (f *frontier) offer(key string, p pendingPage) bool { f.mu.Lock() defer f.mu.Unlock() if f.seen[key] { return false } f.seen[key] = true + f.pending[key] = p return true } @@ -38,9 +54,38 @@ func (f *frontier) offer(key string) bool { func (f *frontier) markVisited(key string) { f.mu.Lock() f.visited[key] = true + delete(f.pending, key) + f.mu.Unlock() +} + +// markDone records that a page needs no more work although nothing was written +// for it, so it is not carried into the next run. robots.txt disallowing a page +// is the case: a resumed run would only fetch robots.txt and skip it again. +// +// A page that failed is deliberately not marked done. It stays in the frontier +// and a resumed run retries it, which is the "memory of what failed" asked for +// in issue #36. +func (f *frontier) markDone(key string) { + f.mu.Lock() + delete(f.pending, key) f.mu.Unlock() } +// unfinished returns the work loaded from a previous run's state file, for the +// cloner to re-queue at startup. It is empty on a fresh run. +func (f *frontier) unfinished() []pendingPage { + f.mu.Lock() + defer f.mu.Unlock() + return f.resumed +} + +// pendingCount reports how many pages are queued or in flight but not finished. +func (f *frontier) pendingCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.pending) +} + // isVisited reports whether a page was already written in a previous run. func (f *frontier) isVisited(key string) bool { f.mu.Lock() @@ -54,12 +99,16 @@ func (f *frontier) visitedCount() int { return len(f.visited) } -// state is the JSON shape persisted for resume. +// state is the JSON shape persisted for resume. A file written before Pending +// existed still loads; it simply resumes with nothing left to do, which is what +// that run recorded. type state struct { - Visited []string `json:"visited"` + Visited []string `json:"visited"` + Pending []pendingPage `json:"pending,omitempty"` } -// load reads a previously saved visited set; a missing file is not an error. +// load reads a previously saved visited set and the frontier that was still +// outstanding; a missing file is not an error. func (f *frontier) load(path string) error { data, err := os.ReadFile(path) if err != nil { @@ -78,23 +127,35 @@ func (f *frontier) load(path string) error { f.visited[v] = true f.seen[v] = true } + // Unfinished pages are held for the cloner to re-queue. They are deliberately + // not marked seen and not put in pending here, because offer() is what does + // both, keyed the same way as every other entry. A page the cloner then turns + // down, which only happens when --max-depth is lower than it was, drops out of + // the frontier; that follows the narrower scope the user just asked for. + f.resumed = s.Pending return nil } -// save writes the visited set atomically (write temp, rename). +// save writes the visited set and the unfinished frontier atomically (write +// temp, rename). func (f *frontier) save(path string) error { f.mu.Lock() visited := make([]string, 0, len(f.visited)) for v := range f.visited { visited = append(visited, v) } + pending := make([]pendingPage, 0, len(f.pending)) + for _, p := range f.pending { + pending = append(pending, p) + } f.mu.Unlock() sort.Strings(visited) + sort.Slice(pending, func(i, j int) bool { return pending[i].URL < pending[j].URL }) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return err } - data, err := json.MarshalIndent(state{Visited: visited}, "", " ") + data, err := json.MarshalIndent(state{Visited: visited, Pending: pending}, "", " ") if err != nil { return err } diff --git a/clone/frontier_test.go b/clone/frontier_test.go index 534ff25..5ed3e89 100644 --- a/clone/frontier_test.go +++ b/clone/frontier_test.go @@ -1,19 +1,20 @@ package clone import ( + "os" "path/filepath" "testing" ) func TestFrontierOfferDedups(t *testing.T) { f := newFrontier() - if !f.offer("a") { + if !f.offer("a", pendingPage{URL: "https://example.com/a"}) { t.Fatal("first offer of a should be new") } - if f.offer("a") { + if f.offer("a", pendingPage{URL: "https://example.com/a"}) { t.Fatal("second offer of a should be a duplicate") } - if !f.offer("b") { + if !f.offer("b", pendingPage{URL: "https://example.com/b"}) { t.Fatal("first offer of b should be new") } } @@ -54,11 +55,85 @@ func TestFrontierSaveLoadRoundTrip(t *testing.T) { t.Fatal("about should be visited after load") } // A loaded visited URL is also seen, so it is not re-offered. - if g.offer("https://example.com/about") { + if g.offer("https://example.com/about", pendingPage{URL: "https://example.com/about"}) { t.Fatal("a loaded URL should not be offered again") } } +func TestFrontierPersistsUnfinishedWork(t *testing.T) { + // The bug in issue #36: only the visited set was saved, so a resumed run had + // no frontier to work from and did nothing at all. + path := filepath.Join(t.TempDir(), "state.json") + + f := newFrontier() + f.offer("/index.html", pendingPage{URL: "https://example.com/", Depth: 0}) + f.offer("/about/index.html", pendingPage{URL: "https://example.com/about", Depth: 1}) + f.offer("/deep/index.html", pendingPage{URL: "https://example.com/deep", Depth: 3}) + f.markVisited("/index.html") + if got := f.pendingCount(); got != 2 { + t.Fatalf("pendingCount = %d, want 2", got) + } + if err := f.save(path); err != nil { + t.Fatalf("save: %v", err) + } + + g := newFrontier() + if err := g.load(path); err != nil { + t.Fatalf("load: %v", err) + } + want := []pendingPage{ + {URL: "https://example.com/about", Depth: 1}, + {URL: "https://example.com/deep", Depth: 3}, + } + got := g.unfinished() + if len(got) != len(want) { + t.Fatalf("unfinished = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("unfinished[%d] = %v, want %v", i, got[i], want[i]) + } + } + // Unfinished pages must not come back marked seen, or re-queueing them would + // be refused and the resumed run would go back to doing nothing. + if !g.offer("/about/index.html", want[0]) { + t.Error("an unfinished page should be offerable again on resume") + } +} + +func TestFrontierMarkDoneDropsWork(t *testing.T) { + // A page robots.txt disallows is finished with, even though nothing was + // written for it, so it must not ride along in the state file forever. + f := newFrontier() + f.offer("/private/index.html", pendingPage{URL: "https://example.com/private"}) + f.markDone("/private/index.html") + if got := f.pendingCount(); got != 0 { + t.Fatalf("pendingCount = %d, want 0", got) + } + if f.isVisited("/private/index.html") { + t.Error("markDone should not claim the page was written") + } +} + +func TestFrontierLoadsStateWrittenBeforePending(t *testing.T) { + // State files from an older kage have no "pending" key. They must still load, + // with nothing outstanding, which is exactly what that run recorded. + path := filepath.Join(t.TempDir(), "state.json") + if err := os.WriteFile(path, []byte(`{"visited":["/index.html"]}`), 0o644); err != nil { + t.Fatal(err) + } + f := newFrontier() + if err := f.load(path); err != nil { + t.Fatalf("load: %v", err) + } + if !f.isVisited("/index.html") { + t.Error("visited set should still load") + } + if n := len(f.unfinished()); n != 0 { + t.Errorf("unfinished = %d, want 0", n) + } +} + func TestFrontierLoadMissingIsNotError(t *testing.T) { f := newFrontier() if err := f.load(filepath.Join(t.TempDir(), "nope.json")); err != nil { diff --git a/docs/content/guides/resuming-a-run.md b/docs/content/guides/resuming-a-run.md index 396e528..f58b76b 100644 --- a/docs/content/guides/resuming-a-run.md +++ b/docs/content/guides/resuming-a-run.md @@ -10,24 +10,46 @@ left off. ## How resume works -As it writes each page, kage records it in a small state file inside the mirror, -at `/_kage/state.json`. When a run ends, for any reason, that file holds -the set of pages already written. Resume is **on by default**: the next time you -run the same clone, kage loads the state and skips every page it already wrote, -re-crawling only what is left. +kage keeps a small state file inside the mirror, at `/_kage/state.json`. +When a run ends, for any reason, it holds two things: the pages already written, +and the frontier that was still outstanding. Resume is **on by default**: the +next time you run the same clone, kage skips every page it already wrote and +picks the frontier back up where it stopped. ```bash kage clone example.com # ... press Ctrl-C partway through ... +# resume: 412 pages still to do, rerun to continue # interrupted; resume state saved (rerun to continue) kage clone example.com # resume: 137 pages already done +# resume: picking up 412 pages ``` Ctrl-C is a clean stop: kage cancels in-flight renders, flushes the state file, and exits. You will not lose the pages already written. +## Pages that failed are tried again + +A page that errored, a render timeout, a host that was briefly down, is never +recorded as written, so it stays in the frontier and the next run tries it +again. Running the same clone a second time after a flaky network is enough to +fill in the gaps. + +A page that `robots.txt` disallows is not carried over, since a later run would +only fetch `robots.txt` and skip it again. + +## Raising a page budget + +`--max-pages` is a per-run budget, and the pages it held back stay in the +frontier. That makes it a way to look before you leap: + +```bash +kage clone example.com -p 20 # see what the first 20 pages look like +kage clone example.com # happy with it, crawl the rest +``` + ## Start fresh To ignore any previous run and rebuild the mirror from scratch, delete the diff --git a/docs/content/reference/configuration.md b/docs/content/reference/configuration.md index e271a89..4d90696 100644 --- a/docs/content/reference/configuration.md +++ b/docs/content/reference/configuration.md @@ -36,7 +36,7 @@ $HOME/data/kage/example.com/ │ │ ├── logo.png │ │ └── fonts/body.woff2 │ ├── cdn.example.com/ # assets from other hosts, by host -│ └── state.json # visited set, for resume +│ └── state.json # visited set + unfinished frontier, for resume └── ... ```