Repository navigation
Fix crawl hanging when a sitemap request fails - #52
Merged
StJudeWasHere merged 1 commit intoOct 8, 2026
Merged
Conversation
wg.Add(1) was called for every sitemap, but wg.Done() was only reached on the success path. When sc.client.Get returned an error the goroutine returned early, leaving the WaitGroup counter above zero, so wg.Wait() blocked forever. ParseSitemaps is called from Crawler.Start, so a single failed sitemap request hung the whole crawl before any page was fetched. A transient error is enough to hang the crawl permanently. Sites whose sitemap index fans out to many children are more exposed, since the children are requested concurrently. Deferring wg.Done() releases the WaitGroup on every path. Unreachable sitemaps are skipped and the remaining ones are still parsed. Adds tests for ParseSitemaps, which had no test file: - returns when every sitemap request fails - one failing sitemap does not discard the working ones - the crawl limit is respected The tests run ParseSitemaps in a goroutine with a timeout so a regression fails the test instead of hanging the test run. They use a local httptest server for the sitemap index, since ParseSitemaps resolves the index over the network.
davidpelayo
force-pushed
the
fix/sitemap-waitgroup-deadlock
branch
from
July 31, 2026 13:47
dc1343d to
91f67d7
Compare
Open
Owner
|
Thanks for the fix and the detailed PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #51.
Problem
SitemapChecker.ParseSitemapscallswg.Add(1)for every sitemap, butwg.Done()was only reached on the success path. Whensc.client.Getreturned an error the goroutine returned early, the WaitGroup counter never reached zero, andwg.Wait()blocked forever.ParseSitemapsis called fromCrawler.Start, so a single failed sitemap request hung the entire crawl before a single page was fetched — the UI sits on "Crawling..." indefinitely with no page reports and no outbound requests.A transient error is enough to hang the crawl permanently. Sites whose sitemap index fans out to many children are more exposed, because the children are requested concurrently: the more parallel requests, the higher the chance that one fails.
Fix
Defer
wg.Done()so the WaitGroup is released on every path, including the early return. Unreachable sitemaps are skipped and the remaining ones are still parsed and crawled.go func(s string) { + defer wg.Done() + resp, err := sc.client.Get(s) if err != nil { return } ... - wg.Done() }(s)Tests
sitemap_checker.gohad no test file. This addsinternal/crawler/sitemap_checker_test.gocovering:TestParseSitemapsReturnsWhenRequestFails— every sitemap request fails;ParseSitemapsmust still return.TestParseSitemapsWithFailingAndWorkingSitemaps— one unreachable sitemap must not discard the working ones. This is the real-world case: an index with several children where one request fails.TestParseSitemapsRespectsLimit— the crawl limit is still honoured.Two details worth noting:
ParseSitemapsin a goroutine guarded by a timeout, so a regression fails the test rather than hanging the whole test run. Without that, a reintroduced bug would stall CI instead of reporting.httptestserver for the sitemap index, becauseParseSitemapsresolves the index over the network viacheckIndex. This keeps the tests hermetic and fast.Verified against the exact CI commands:
Confirmed the tests actually catch the bug — reverting
sitemap_checker.gotomainand rerunning:gofmt -landgo vet ./internal/crawler/are both clean.Notes
Beyond the scope of this PR, but noticed while debugging: a crawl that stalls has no timeout or watchdog, so any future hang in
Crawler.Startpresents the same way — an indefinite "Crawling..." with no diagnostics. A crawl-level timeout, or logging failed sitemap requests instead of silently swallowing the error, would make this class of problem much easier to spot. Happy to open a separate issue if that is of interest.