Skip to content

feat(HNT-2116): add the Redis-guarded article process step - #29

Draft
mmiermans wants to merge 1 commit into
claude/hnt-2086-discovery-handlerfrom
claude/hnt-2086-process-article
Draft

feat(HNT-2116): add the Redis-guarded article process step#29
mmiermans wants to merge 1 commit into
claude/hnt-2086-discovery-handlerfrom
claude/hnt-2086-process-article

Conversation

@mmiermans

@mmiermans mmiermans commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Goal

HNT-2116

Add processArticle, the Redis-guarded step that turns a validated crawl-article job into at most one Zyte fetch and one publish to the articles topic. It skips a URL fetched within its refresh window, takes a per-article lock, re-checks freshness under the lock, and publishes only when the extracted content changed since the last fetch.

This builds on the article extraction handler and worker helper layer, composing them with the crawl Redis keys, the redis-state and pubsub clients, worker config, and the domain types, and it adds the pubsub workspace dependency to the worker. docs/ARCHITECTURE.md in #14 draws this flow.

Implementation decisions

Decision Approach Why
Claim the fetch marker before the Zyte call and the publish setTimestamp(fetchKey) runs inside the lock but ahead of extraction and publishing, where the tech spec writes it last A partial failure, whether a failed publish, an ack-deadline expiry, or a mid-handler crash, then redelivers into a skip instead of re-paying for a Zyte fetch and republishing. The rare cost is that one interval's update can be dropped, which self-heals on the next crawl. This trades a very occasional missed update for bounded Zyte spend under at-least-once delivery.
Re-check article:fetch after acquiring the lock The freshness check runs again inside the lock before extracting Concurrent duplicate jobs all pass the pre-lock check and then serialize on the lock. Without the second read each would re-extract, so the first claims the marker and the rest skip.
Exclude url and extracted_at from the content hash contentHash hashes only the meaningful content fields, not url or extracted_at url is constant per key and extracted_at changes on every fetch, so including either would make the hash never match and republish the article every time.
Publish before storing the content hash, and refresh the content key every fetch publishIfChanged publishes when the hash differs, then always rewrites the hash If the store failed after a publish, the message redelivers and republishes rather than dropping the event, and the tech spec's at-least-once design already dedups by latest-per-URL. Rewriting the key each fetch keeps its TTL in step with the fetch marker so an unchanged article does not expire and get republished.
Release the lock best-effort in a finally The release is awaited in finally and its rejection is caught and logged, not rethrown The lock self-expires on its TTL, so a release failure must not propagate out and turn a successful publish into a nack that redelivers the message.
Dedup live articles on their per-message refresh interval The window is message.refresh_interval_minutes ?? config.articleFetchTtlMinutes Live articles carry their own interval so they dedup on the agent's cadence rather than being fetched on every delivery, while discovered articles fall back to the default fetch TTL.

@mmiermans
mmiermans force-pushed the claude/hnt-2086-discovery-handler branch from 4b7a119 to 21f1545 Compare July 6, 2026 17:08
@mmiermans
mmiermans force-pushed the claude/hnt-2086-process-article branch from fca1ad9 to 90aca3b Compare July 6, 2026 17:08
@mmiermans mmiermans changed the title feat(HNT-2086): add the Redis-guarded article process step feat(HNT-2116): add the Redis-guarded article process step Aug 6, 2026
@mmiermans

Copy link
Copy Markdown
Collaborator Author

Marker semantics: claim before, confirm after

The discovered-article refresh window is changing from 60 minutes to 30 days: a discovered article should be extracted once and left alone, while curated articles stay fresh through the live path every 15 minutes. That change makes the marker's TTL the thing that matters, not the moment it is written.

Keep writing the marker before the Zyte call, as processArticle does today with await setTimestamp(fetchKey) ahead of handleArticleExtraction, but with a short expiry of one page-crawl interval (ARTICLE_ATTEMPT_TTL_MINUTES, 20 minutes, added in #25), and rewrite it with the full retention once the work is genuinely done: after a successful extraction, and after a permanent failure (403, 404, 422, 451, using the Zyte client's existing isRetryable classification) so a dead URL is not re-extracted forever. A recoverable failure (429, 500, 503, 520, or the rate limiter's awaitZyteToken throwing) leaves only the short claim, so the URL returns on the first page crawl after it expires.

Rename the key article:fetch to article:extracted. The helper itself is articleFetchKey in packages/crawl-common/src/redis/keys.ts, which lands in #21, so the rename belongs there; this PR picks up the renamed helper, and its intervalMinutes fallback picks up the renamed DISCOVERED_ARTICLE_REFRESH_DAYS config from #25.

Why two TTLs rather than one: the long window must not apply to a failed attempt, or a single transient failure would suppress the article for 30 days; and the claim must not be dropped, or the enqueue gate turns failure into load. We considered a completion-only marker and rejected it, because processDiscovery gates enqueueing on the same key: with no marker written on failure, every 20-minute page crawl would mint a fresh job for a URL that keeps failing, so failure itself would generate load. Each such URL would permanently occupy roughly 1.5 of a pod's concurrency slots, and since blocked handlers burn no CPU while the HPA scales on CPU at a floor of one replica in prod, nothing would scale up to absorb it.

setTimestamp(key, ttlSeconds) already accepts a per-write TTL, so this needs no second key and no new concept.

Keep the re-check inside the lock. The existing tests that pin claim-before-call, including "claims the fetch marker before the Zyte call and publish" and "keeps the fetch claim when extraction throws, so a redelivery skips without re-fetching", still hold; only their TTL assertion changes.

@mmiermans

Copy link
Copy Markdown
Collaborator Author

Two corrections to the above.

The attempt claim TTL is 60 minutes, not 20. Reasons, in order of weight:

  • Measured Zyte download-error episodes last hours, not minutes. One well-resolved episode ran 49 hours with a sharp onset and end. A 20-minute retry cadence therefore spends attempts on a URL that will not be fetchable yet.
  • 60 minutes deterministically exceeds the 20-minute page-crawl interval. At 20 the claim expires at roughly the same moment the next crawl lands, so suppression is a race rather than a property.
  • It matches what the worker already defaults to today, since ARTICLE_FETCH_TTL_MINUTES is 60 and the marker is currently written before the Zyte call. A 20-minute claim would have made failure handling 3x more expensive than the status quo, which was an unintended regression.

Cost per persistently failing URL, for context: about 96 Zyte requests a day at 60 minutes, against about 288 at 20 minutes and about 1,440 with no claim at all. The ARTICLE_ATTEMPT_TTL_MINUTES default in #25 changes with it; the claim-before, confirm-after ordering itself is unchanged.

HTTP 521 stays retryable, so no change to RETRYABLE_STATUS_CODES. It belongs with 429, 500, 503 and 520 in the recoverable group above, not with the permanent codes. This was researched and settled:

  • 520 and 521 are both Zyte-generated, not passed through from the origin. Zyte defines 521 as /download/internal-error, where "permanent" means permanent until Zyte fixes their downloader profile for that site, a condition measured in hours to days rather than a property of the URL.
  • Zyte's own documentation advises treating 521 as 520 and retrying when it occurs only intermittently, and their Python client retries both by default.
  • 521 is 0.024% of requests over 90 days against 5.17% for 520, and 68% of 521s occur on domains already dominated by 520s.
  • The one cleanly isolated episode self-cleared in 49 hours with successful throughput unchanged, and the affected URLs fetch normally today.

So a terminal 520 or 521 must not get the long completion marker; it leaves only the hour-long claim. The implementation guide's Task 4.1 line listing 521 as a permanent error is the mistake, not the code.

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.

1 participant