-
Notifications
You must be signed in to change notification settings - Fork 0
Conditional sitemap fetching — skip the reconcile on a 304 (v0.73.0) #165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import { config } from '../config.js'; | ||
| import { epochMsOf } from './time.js'; | ||
|
|
||
| /** | ||
| * Whether one sitemap document can be fetched CONDITIONALLY, and with what validator. | ||
| * | ||
| * Lives here rather than in `resources/Sitemap.js` for the usual reason: that module subclasses a | ||
| * table at import time and cannot be loaded without a live Harper, so a decision left inside it is | ||
| * untestable. | ||
| * | ||
| * ── WHY `Last-Modified` AND NOT `ETag` ─────────────────────────────────────────────────────── | ||
| * | ||
| * Measured against a production edge: `If-Modified-Since` returned a clean 304 with no body, while | ||
| * `If-None-Match` — sent back with the exact ETag that same edge had just served — returned 200 and | ||
| * the full multi-megabyte document. An origin that advertises a validator is not promising to | ||
| * honour it, and an ETag-based conditional fetch fails in the worst possible way: it looks correct, | ||
| * returns 200 every time, and silently re-transfers the whole corpus on every pass. Hence the stored | ||
| * validator is the `Last-Modified` string, echoed back verbatim. | ||
| * | ||
| * ── WHAT A 304 SKIPS, AND WHY THAT IS THE POINT ────────────────────────────────────────────── | ||
| * | ||
| * Not just the download. A pass scans the `sitemapUrl` index once per child, and that prune scan | ||
| * holds a read cursor whose seconds scale linearly with refresh frequency — it is the cost that | ||
| * decides how often a corpus can afford to be refreshed. A 304 skips the body, the parse, the scan | ||
| * and every write, so an unchanged pass costs one request per document and no database work. | ||
| */ | ||
| export const conditionalValidatorFor = (stored, revalidate) => { | ||
| const { enabled, fullPassInterval } = config.sitemap.conditional; | ||
| if (!enabled || revalidate) return null; | ||
|
|
||
| // No validator: first walk of this document, or an origin that sends none. Conditional | ||
| // fetching degrades to unconditional rather than to broken. | ||
| if (!stored?.lastModified) return null; | ||
|
|
||
| // THE REPAIR NET. 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` means "entries last INGESTED" precisely so | ||
| // it can be measured against here; a 304 deliberately does not update it. | ||
| // | ||
| // Written as `!(elapsed < interval)` rather than `elapsed >= interval` so that a NaN — an | ||
| // unreadable or absent date — takes the unconditional branch instead of silently reading as | ||
| // "ingested at the epoch" or, worse, passing the comparison. | ||
| if (!(Date.now() - epochMsOf(stored.lastRefreshed) < fullPassInterval)) return null; | ||
|
|
||
| return stored.lastModified; | ||
| }; | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { test } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { applyOptions } from '../src/config.js'; | ||
| import { conditionalValidatorFor } from '../src/util/sitemapConditional.js'; | ||
|
|
||
| globalThis.logger ??= { debug() {}, info() {}, warn() {}, error() {} }; | ||
|
|
||
| const HOUR = 60 * 60 * 1000; | ||
| const LM = 'Wed, 16 Sep 2026 06:08:37 GMT'; | ||
|
|
||
| const setConditional = (overrides = {}) => | ||
| applyOptions({ sitemap: { conditional: { enabled: true, fullPassInterval: 24 * HOUR, ...overrides } } }); | ||
|
|
||
| const stored = (over = {}) => ({ lastModified: LM, lastRefreshed: new Date(Date.now() - HOUR), ...over }); | ||
|
|
||
| test('a recently-ingested document with a validator is fetched conditionally', () => { | ||
| setConditional(); | ||
| assert.equal(conditionalValidatorFor(stored(), false), LM); | ||
| }); | ||
|
|
||
| // The repair net: a 304 skips the reconcile, and the reconcile is also what re-creates targets | ||
| // lost to anything else. Past the interval the document is re-ingested whether it changed or not. | ||
| test('a document not ingested within fullPassInterval is fetched unconditionally', () => { | ||
| setConditional(); | ||
| assert.equal(conditionalValidatorFor(stored({ lastRefreshed: new Date(Date.now() - 25 * HOUR) }), false), null); | ||
| }); | ||
|
|
||
| test('no stored validator means unconditional — degrade to full, never to broken', () => { | ||
| setConditional(); | ||
| assert.equal(conditionalValidatorFor(stored({ lastModified: null }), false), null); | ||
| assert.equal(conditionalValidatorFor(stored({ lastModified: '' }), false), null); | ||
| }); | ||
|
|
||
| test('a document never seen before is fetched unconditionally', () => { | ||
| setConditional(); | ||
| assert.equal(conditionalValidatorFor(null, false), null); | ||
| assert.equal(conditionalValidatorFor(undefined, false), null); | ||
| }); | ||
|
|
||
| // An unreadable or absent date must not read as "ingested at epoch 0" and certainly not as | ||
| // "recently ingested": NaN fails the comparison, which falls through to a full fetch. | ||
| 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); | ||
| }); | ||
|
|
||
| test('revalidate always fetches unconditionally — the operator asked for a re-ingest', () => { | ||
| setConditional(); | ||
| assert.equal(conditionalValidatorFor(stored(), true), null); | ||
| }); | ||
|
|
||
| test('disabling the feature restores unconditional fetching everywhere', () => { | ||
| setConditional({ enabled: false }); | ||
| assert.equal(conditionalValidatorFor(stored(), false), null); | ||
| }); | ||
|
|
||
| test('fullPassInterval 0 makes every fetch unconditional', () => { | ||
| setConditional({ fullPassInterval: 0 }); | ||
| assert.equal(conditionalValidatorFor(stored({ lastRefreshed: new Date() }), false), null); | ||
| }); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always perform a truthiness check on date values before parsing or coercing them. In JavaScript, passing
nulltonew Date()(which can happen insideepochMsOfifstored.lastRefreshedis null) evaluates to0(epoch 0) instead ofNaN. This can corrupt calculations and cause a document with a missing or nulllastRefreshedvalue to be incorrectly treated as recently ingested (especially iffullPassIntervalis large or during testing), bypassing the unconditional repair net. Adding an explicit truthiness check ensures we safely fall back to unconditional fetching.References
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declining this one — the premise does not hold in this codebase.
epochMsOfalready rejects null explicitly rather than handing it tonew Date():so
epochMsOf(null)isNaN, not0. That helper exists for precisely the reason this comment gives — its own doc comment callsNumber(null)being0"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 beDate.now() - 0, which is far larger thanfullPassInterval, so the comparison fails and the function returnsnull— an unconditional fetch. That is the repair net firing, not being bypassed.The guard is deliberately written as
!(elapsed < interval)rather thanelapsed >= intervalso that aNaNtakes the same unconditional branch, and the comment above it says so.test/sitemapConditional.test.jspins both shapes: