Skip to content

fix(core): unblock backpressured sitemap load on persistState#3863

Open
anxkhn wants to merge 1 commit into
apify:masterfrom
anxkhn:patch-16
Open

fix(core): unblock backpressured sitemap load on persistState#3863
anxkhn wants to merge 1 commit into
apify:masterfrom
anxkhn:patch-16

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What & why

SitemapRequestList can deadlock its background sitemap loader when
persistState() runs while the loader is parked on stream backpressure.

pushNextUrl() pushes a URL into this.urlQueueStream and, when the stream is
full, blocks by waiting for a custom readdata event on that stream object:

// packages/core/src/storages/sitemap_request_list.ts
if (!this.urlQueueStream.push(url)) {
    // This doesn't work with the 'drain' event (it's not emitted for some reason).
    this.urlQueueStream.once('readdata', () => {
        resolve();
    });
}

That readdata event is only ever emitted by readNextUrl(), on whatever
this.urlQueueStream currently is. persistState() drains the current stream,
creates a fresh one, transfers the drained URLs, and swaps it in:

const newStream = this.createNewStream(this.urlQueueStream.readableHighWaterMark);
for (const url of urlQueue) {
    newStream.push(url);
}
// ...
this.urlQueueStream = newStream;

If a pushNextUrl() call was backpressured on the previous stream when the swap
happens, its once('readdata') listener stays on the now-orphaned old stream.
Nothing ever emits readdata on that old stream again, so the promise never
resolves. The background load() task is stuck at await this.pushNextUrl(...),
inProgressSitemapUrl is never cleared, isSitemapFullyLoaded() stays false,
and isFinished() never becomes true, so the crawler hangs.

persistState() is timer-driven (registered on the PERSIST_STATE event), so
this can trigger on any long-running sitemap load, not only on manual calls.

Fix

Re-emit readdata on the previous stream right after the swap, releasing any
pending pushNextUrl() waiter. The URL that waiter pushed has already been
drained and transferred into the new stream, so resolving it loses nothing and
introduces no duplicate. emit('readdata') with no listener is a safe no-op.

const previousStream = this.urlQueueStream;
const newStream = this.createNewStream(previousStream.readableHighWaterMark);
// ... transfer urls, propagate writableEnded ...
this.urlQueueStream = newStream;

previousStream.emit('readdata');

This mirrors the existing unblock idiom already used in teardown(), which
emits readdata for the same reason (to unblock a waiting pushNextUrl()).

Testing

Added a regression test to test/core/sitemap_request_list.test.ts:
maxBufferSize: 1 forces the loader to block on backpressure right after the
first URL, persistState() is called at that moment, and the test asserts the
list drains completely, reaches isSitemapFullyLoaded() / isFinished(), and
yields every URL. It times out (deadlocks) on master and passes with the fix.

  • yarn vitest run test/core/sitemap_request_list.test.ts: 18/18 pass (the new
    test times out at 10s without the source change).
  • Broader run (sitemap + recoverable_state + request_list + storages): 132/132.
  • yarn eslint and yarn biome format on both changed files: clean.
  • tsc --noEmit for the core build config and tsc-check-tests: both pass.

Well-behaved loads are unaffected: readNextUrl() still emits readdata on the
current stream as before, and the extra emit on the previous stream only
resolves an already-satisfied waiter.

SitemapRequestList.pushNextUrl() blocks under backpressure by waiting for a
'readdata' event on the current urlQueueStream. persistState() drains that
stream and replaces it with a new one, but 'readdata' is only ever emitted by
readNextUrl() on the current stream. After the swap, the waiter parked on the
old stream is never notified, so the background load() stalls, the sitemap is
never marked fully loaded, and isFinished() never becomes true (the crawler
hangs).

Re-emit 'readdata' on the previous stream after swapping so any pending
pushNextUrl() waiter is released; the pending URL has already been transferred
to the new stream, so nothing is lost. Adds a regression test that persists
state while the loader is backpressured (maxBufferSize: 1) and asserts the list
finishes with all URLs.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants