Conditional sitemap fetching — skip the reconcile on a 304 (v0.73.0)

[harper-joseph]

Stacked on #164, which is stacked on #163. Review in that order; GitHub retargets each as its parent merges. The commit here is the third in the diff.

What

sitemap.conditional sends If-Modified-Since from a stored Last-Modified and, on a 304, skips the body, the parse, the prune scan and every write.

Why this is worth more than the bytes

Every pass re-fetched every child in full and ran its prune scan whether the document had changed or not — on one production corpus, 31 documents and ~125 MB of XML per pass, four passes a day, to discover a change that happens once a night.

But the download is the smaller half. The per-child prune scan holds a read cursor, and cursor-seconds are the cost that scales linearly with refresh frequency (sitemap.refreshInterval's own docs say this is the reason not to set it very low). A 304 skips that too, so an unchanged pass costs one request per document and no database work at all.

That changes what is affordable. A deployment whose sitemaps rebuild once a night can now poll every few minutes and pay for the walk only on the pass that finds the rebuild — detecting it in minutes instead of waiting out a fixed grid slot, and without depending on the rebuild finishing before a fixed anchor.

Use Last-Modified, not ETag — measured, not assumed

request	result
If-Modified-Since: <last-modified>	304, zero bytes
If-None-Match: <the exact ETag that edge just served>	200 + full 7.9 MB body

An origin that advertises a validator is not promising to honour it, and an ETag-based conditional fetch fails in the worst way available: it looks correct, returns 200 every time, and silently re-transfers the whole corpus forever. sitemap_not_modified exists so that failure is visible — a steady zero where this is enabled means the origin is not honouring the validator and the frequency should come back down.

Two details

An index is still descended on a 304. That only says the child list is unchanged, not the children — they are separate documents with their own validators, and on a real corpus they rebuild on a different schedule from the index that lists them (measured: children nightly at ~06:08 UTC, their index at 17:40 UTC the same day). The stored entries are re-read and each child makes its own conditional decision. Watching only the root would miss the rebuild entirely.

fullPassInterval (24h) is the repair net, and it is why this is safe to leave on. A 304 skips the reconcile, and the reconcile is also what re-creates targets lost to anything else — a bad purge, a half-applied delete, a botched migration. Without a periodic unconditional pass a corpus could drift for as long as the origin left its sitemaps untouched, with nothing noticing. lastRefreshed now means "entries last ingested" precisely so it can be measured against; a 304 deliberately leaves it alone. 0 restores the pre-0.69.0 behaviour.

Shape

• schema.graphql: Sitemap.lastModified: String — additive. Stored as the raw header string and echoed back verbatim; parsing to a Date and reformatting risks handing the origin a value differing by a second and re-fetching everything.
• The 304 check sits before the res.ok guard — Response.ok is 200–299, so a not-modified would otherwise be thrown as a failed fetch.
• The stored-row read uses a narrow projection (url, isIndex, lastModified, lastRefreshed): entries on a product child is megabytes and this read happens for every document on every pass. Entries are read back only on the one path that needs them — a 304 on an index, whose row is small by construction.
• The decision lives in util/sitemapConditional.js so it is testable; resources/Sitemap.js subclasses a table at import time.
• New counter sitemap_not_modified, on the result, the progress row and the finish log.

Degradation

No stored validator (first walk, or an origin that sends none) → unconditional. Unreadable lastRefreshed → unconditional; the comparison is written !(elapsed < interval) so a NaN takes the full-fetch branch rather than silently reading as "recently ingested". revalidate: true → unconditional, since that is exactly a request to re-ingest.

Tests

npm test in packages/plugin: 1081 pass, 0 fail. Lint and format:check clean. 8 new tests cover the validator decision: recently-ingested-with-validator, past fullPassInterval, missing/empty validator, never-seen document, unreadable date, revalidate, disabled, and fullPassInterval: 0.

Follow-up (not in this PR)

Once this is deployed, sitemap.refreshInterval can come down to polling range on the consumer side. That is a config change on the deployment, worth making only after one day of sitemap_not_modified confirms the origin honours the validator in production the way it did in the probe.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces conditional sitemap fetching using the If-Modified-Since header to optimize performance and reduce database load during sitemap refreshes. It adds configuration options, GraphQL schema updates, and metrics tracking for unmodified sitemaps. Feedback on the changes highlights a potential bug in sitemapConditional.js where a null lastRefreshed value could evaluate to epoch 0 instead of NaN, potentially bypassing the unconditional repair net; a code suggestion is provided to add an explicit truthiness check.

[gemini-code-assist]

Always perform a truthiness check on date values before parsing or coercing them. In JavaScript, passing null to new Date() (which can happen inside epochMsOf if stored.lastRefreshed is null) evaluates to 0 (epoch 0) instead of NaN. This can corrupt calculations and cause a document with a missing or null lastRefreshed value to be incorrectly treated as recently ingested (especially if fullPassInterval is large or during testing), bypassing the unconditional repair net. Adding an explicit truthiness check ensures we safely fall back to unconditional fetching.

Suggested change

	if (!(Date.now() - epochMsOf(stored.lastRefreshed) < fullPassInterval)) return null;
	if (!stored.lastRefreshed) return null;
	if (!(Date.now() - epochMsOf(stored.lastRefreshed) < fullPassInterval)) return null;

References

1. When parsing or coercing date values in JavaScript, always perform a truthiness check first (e.g., ensuring the value is not null or undefined) before passing it to Date(). This avoids bugs where new Date(null).getTime() evaluates to 0 (epoch 0) instead of NaN.

[harper-joseph]

Declining this one — the premise does not hold in this codebase. epochMsOf already rejects null explicitly rather than handing it to new Date():

export const epochMsOf = (value) => (value || value === 0 ? new Date(value).getTime() : Number.NaN);

so epochMsOf(null) is NaN, not 0. That helper exists for precisely the reason this comment gives — its own doc comment calls Number(null) being 0 "the most plausible-looking wrong answer available" and explains that accepting a missing value as the epoch has real teeth on the claim-floor paths.

Worth noting the direction too: even if it did coerce to 0, the result would be Date.now() - 0, which is far larger than fullPassInterval, so the comparison fails and the function returns null — an unconditional fetch. That is the repair net firing, not being bypassed.

The guard is deliberately written as !(elapsed < interval) rather than elapsed >= interval so that a NaN takes the same unconditional branch, and the comment above it says so. test/sitemapConditional.test.js pins both shapes:

test('an unreadable lastRefreshed falls through to unconditional', () => {
	setConditional();
	assert.equal(conditionalValidatorFor(stored({ lastRefreshed: 'not a date' }), false), null);
	assert.equal(conditionalValidatorFor(stored({ lastRefreshed: null }), false), null);
});

[harper-joseph]

Reviewed as part of the combined train (#165 → #177 → #178 → #181 → #182) alongside kohls-pr#108/#111. Trial-merged all five; they compose and 1148 tests pass together.

First, the good news, because I went looking for the opposite. I expected the 304 short-circuit to break the post-walk sitemap-departure check. It does not: processDepartures(run) is called unconditionally and this PR doesn't touch that call site. And the case the departure check exists for — pagination shear across child boundaries — is structurally immune, because children are walked FIFO in index order, so a URL shifting to an earlier child is re-attributed before the losing child's prune scan runs. A 304 also means nothing departed from that child by construction. That's a genuinely nice property.

One narrow gap survives it.

1. Cross-child duplicate listings can produce a FALSE departure — MEDIUM

Both premises verified in the code:

• Duplicates are real and documented: sitemapRun.js:63 — "a URL listed in two children of the same index — 95 of them in one real 800k-URL corpus", handled by first-writer-wins / DUPLICATE.
• This PR's 304 path returns before reconcileSitemapEntries, so no REATTACH runs for that child.

The sequence:

1. Children A (earlier) and B (later) both list u. A claims it; B records DUPLICATE and writes nothing. u.sitemapUrl = A.
2. A drops u. A changed → 200 → A's prune nulls u.sitemapUrl and collects it as a departure candidate.
3. B still lists u, but B is unchanged → 304 → no entry loop → no REATTACH.
4. processDepartures sees sitemapUrl null → departed.

On main, step 3 is a 200 and the candidate is correctly counted reattached. With kohls-pr#111's departureAction: render armed, this hard-expires both device keys and files a due-now render for a product that never left the corpus.

Bounded (one wasted render + one expiry each, at ~95/800k scale), not a cache-wide event. But it's silent, and it acts rather than skips.

The same shape already exists on main for a child that fails its fetch — processDepartures never consults run.failed. This PR doesn't introduce that, but it turns "this child's reconcile didn't run" from a rare fault into the normal case, which is what makes it worth closing now.

Cheapest mitigation is on the consumer side (keep #111's dryRun: true), but a guard here would be better: on a 304, fold the stored entries for that child into a walk-scoped "declared this walk" set that decideDeparture consults.

2. parentUrl is no longer restamped on a 304 — MEDIUM, conditional

Unverified by me, but the mechanism looks right. Main rewrites row = {...latestSitemap, parentUrl} every pass; the 304 path writes nothing.

Precondition: a sitemap first ingested as a root (operator POST /sitemaps/<url> → parentUrl: null) and later listed by an index. The pass that would stamp parentUrl now 304s, so it stays null permanently, rootSitemapUrls() keeps returning it, and refreshAllSitemaps walks it twice per pass — the second time in its own Sitemap.refresh() call with its own visited set and its own processDepartures, where a candidate has no siblings available to re-attribute it. This self-corrected on main; here it wouldn't. kohls used manual POST /sitemaps/<url> before the scheduler was pinned, so the precondition is plausible there.

Fix looks small: read parentUrl in the narrow projection and patch it on the 304 path when it differs.

3. A truncated-but-parseable body now freezes for fullPassInterval — MEDIUM-HIGH if reachable

Reasoned, not empirically confirmed — I did not test fast-xml-parser's truncation behaviour.

parseSitemap runs with no XMLValidator call, so a truncated body that still yields a urlset parses to a shorter entry list without throwing. The codebase already treats that as real — it's the stated justification for departure.maxActions. On main the blast radius is one refreshInterval, because the next pass re-fetches in full. Here the truncated ingest also stores a lastModified, and the origin then correctly 304s against it, so the corpus stays truncated until fullPassInterval (24h) — after the mass prune has already fired.

Worth considering: only store the validator when the body length matches Content-Length, and/or refuse to prune more than X% of a child's targets in one pass.

4. Merge mechanics with #177 — please keep BOTH METRICS.md rows

You conflict with #177 in five files. All additive, but one resolution is a trap:

metrics.sitemapRun takes a free-form series string, so test/metrics.test.js cannot detect a dropped row — it only checks catalogued emitters. A resolution that takes one side's METRICS.md table wholesale silently ships a series nobody can read. That's the same DYNAMIC_SERIES_SLOT hole the console's guard has been bitten by before.

Also: keep both progressFields entries and both metrics.sitemapRun lines, and merge the finish-log into one template. And beware configSchema.js — a union resolve there drops the closing brace of this PR's conditional group and fails to parse ~500 lines later. I hit exactly that.

5. Minor

• fetchLatestSitemap sets notModified: false on the 200 return and refreshOneSitemap immediately deletes it. Not dead, but the pair is pointless — return the flag only on the 304 path.
• stored?.isIndex / storedRow?.entries ?? [] have unreachable null branches (a 304 requires ifModifiedSince, which requires a truthy stored.lastModified). Harmless.
• No console surface for sitemap_not_modified. The PR's rollout story depends on watching it, and today that means raw prerender_ops queries.
• Console sitemaps.js renders lastRefreshed as "… refreshed". Under this PR an unchanged sitemap reads hours old though it was checked minutes ago. "Ingested" would be the honest label.
• PrerenderAdmin.js:287 caches an entry page keyed by (url, lastRefreshed) on the comment "a refresh bumps lastRefreshed, which invalidates it naturally". Still correct under a 304 (unchanged entries), but the stated mechanism is now wrong.

Net: I think this is sound and the departure interaction is much narrower than I feared. Finding 1 is the one I'd want closed before kohls flips departure.dryRun: false.

🤖 Generated with Claude Code