Drain an upload even while the crawler is catching up - #135
Merged
Conversation
A list of 109,474 URLs was submitted at 09:21 today. Every entry staged into import_entries within 55 seconds, and then nothing happened at all: queued_count stayed 0, and the page the submitter was given said "0% of 0 of 109,474 feeds — working". It would have said that for ever. The cause was position. `drainImport` sat eleven lines below `if (catchupOnly) return`, and CRAWL_CATCHUP=1 is set on the poller. Nothing errored and nothing logged, because from the tick's point of view nothing went wrong — it simply never reached the queue somebody was waiting on. The only way to find it was to read this file. Catch-up mode is right about everything else it skips. Discovery finds feeds on its own schedule, cards and clusters decorate what is already indexed, and none of it has a person watching a page. An import is the opposite: somebody handed over a list and was given a URL to follow. A slice a tick is a small enough price that it never had to be deferred. `notifyFinishedSubmissions` moves with it, because the two are a pair — this file already said so, that the daemon draining the queue is the one that tells the submitter it drained. Draining in catch-up mode while leaving that below would finish an upload and never say so, which is a stranger failure than not draining. It is now guarded, too: down there a throw cost only the housekeeping that followed, up here it would cost the crawl. `notifyFinishedDiscoveries` deliberately stays below. Nobody is waiting on it. The test is a source-order assertion, which is a compromise worth naming: index.js connects to a database and starts timers at import, so `tick()` cannot be loaded by a test without a refactor far larger than the bug. Checking the order of three lines is crude and it does hold the one invariant whose violation is invisible from outside. Verified it fails three ways with the drain moved back below the return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
A list of 109,474 URLs was submitted at 09:21 today. Every entry staged into
import_entrieswithin 55 seconds (entries_ready_at09:22:16), and then nothing happened at all —queued_countstayed0, and the page the submitter was given said:It would have said that for ever.
The cause was position
CRAWL_CATCHUP=1is set on the poller service. Nothing errored and nothing logged, because from the tick's point of view nothing went wrong — it simply never reached the queue somebody was waiting on. The only way to find it was to read the source.Why the import is the exception
Catch-up mode is right about everything else it skips. Discovery finds feeds on its own schedule, cards and clusters decorate what is already indexed, and none of it has a person watching a page. An import is the opposite: somebody handed over a list and was given a URL to follow. One slice a tick is a small enough price that it never had to be deferred.
notifyFinishedSubmissionsmoves with it, because the two are a pair — this file already said so:Draining in catch-up mode while leaving that below the return would finish somebody's upload and never say so, which is a stranger failure than not draining at all. It is now
try-guarded as well: below the return a throw cost only the housekeeping that followed it; above it, it would cost the crawl.notifyFinishedDiscoveriesdeliberately stays below. Nobody is waiting on it.About the test
It is a source-order assertion, which is a compromise worth naming rather than hiding:
apps/poller/src/index.jsconnects to a database and starts timers at import, sotick()cannot be loaded by a test without a refactor far larger than the bug. Checking the order of three lines in the source is crude, and it does hold the one invariant whose violation is invisible from outside.Verified it fails — three assertions go red with the drain moved back below the return — and passes on the fix.
Not in this PR
The submission page still has no live progress; it is a static render, which is most of why this looked like a stalled crawler rather than a stalled queue. Worth its own change.
Testing
All 11 workspace suites green under Node 22, the version CI runs.
🤖 Generated with Claude Code