Skip to content

fix(plugin): bound the url-walk probe with one gtlt range, ready for Harper 5.3; v0.102.1 - #247

Open
harper-joseph wants to merge 1 commit into
mainfrom
fix/harper-531-ready
Open

harper-joseph wants to merge 1 commit into
mainfrom
fix/harper-531-ready

Conversation

@harper-joseph

Copy link
Copy Markdown
Contributor

Prepares the plugin for Harper 5.3.1. One code change; the rest of the 5.3 behaviour changes were audited and need nothing.

The fix

walkUrlRange's end-of-chunk probe searched url > cursor AND url < endBound as two conditions. Harper 5.2 ordered them with fixed fractions, so > cursor always led. Harper 5.3 orders conditions by a storage-level size estimate (harper#2163, #2479). When < endBound estimates narrower it leads, and finding the single row past the cursor can then walk every row below the bound.

The probe is now one exclusive gtlt range, value: [cursor, endBound]. Both 5.2.14 and 5.3.1 read it as a single bounded scan (search.ts supports gtlt in both). The existing fallback still applies if a store refuses the shape.

Only discoveredPurge passes an endBound, so this affects discovery purges and nothing on the serve path.

renderSchedule.js: the keeper's subscription note now describes 5.3 delivery (harper#2767). A write is delivered only while the row's local entry still carries its version, so the not-owned deletes 5.2 sent are gone. This is a comment-only change: the keeper already ignores not-owned rows and re-checks entries before granting.

Audited, no change needed (5.3.0 + 5.3.1)

Change Why it doesn't affect the plugin
#2047: writes refused (499) after a premature client disconnect Only transaction() (REST) and the operations API bind a request context. The bot path is server.http middleware with no context, so its writes carry no signal. For job_result, a disconnect mid-response means the worker retries the same bytes. Long admin work already runs through runDetached.
#2767: catch-up sends current versions only All three subscriptions use omitCurrent: true.
#2484: allowFullScan / maxSortRows now enforced SQL engine only, and the plugin sends no SQL. The Table.search refusals are unchanged between versions.
#2187 searchByIndex, #2259 counters, #2097 array PUT, #2902 localOnly Not used.
#2692: analytics stop under-counting Analytics are read only for the console's charts, which will step up about 1.5x.
rocksdb-js 2.10 shared buffers live as long as their column family getSab stays harmless.

Verification

  • node --test in packages/plugin: 2021/2021. npm run lint and npm run format:check are clean. The new regression test pins the probe to a single gtlt condition.
  • Real Harper, both versions. I ran this branch against the published harperfast/harper-pro:5.2.14 and :5.3.1 images. The rehearsal used a production-shaped component config on an --internal Docker network with a fake origin. A dry-run discovery purge over a prefix with neighbouring rows on both sides examined exactly the 300 in-range rows on each version, with 0 errors.
  • Same rehearsal, current release (0.102.0). I upgraded 5.2.14 → 5.3.1 in place and ran a mixed 5.2.14/5.3.1 cluster. Row counts were identical across the upgrade, and cache hits, raw cache, 404, discovery, overrides, claim and job_result all worked. Records and blobs replicated in both directions, and a peer upgraded mid-run caught up on writes made while it was down. Both logs had 0 errors.

Reserves prerender-v0.102.1 (patch; the planned request-sampling feature keeps 0.103.0).

🤖 Generated with Claude Code

…Harper 5.3; v0.102.1

Harper 5.3 orders separate search conditions by a storage-level size estimate instead of 5.2's
fixed fractions. The walk's end-of-chunk probe asked for `url > cursor` AND `url < endBound` as two
conditions; on 5.3 the `< endBound` side can estimate narrower and lead, so finding the one row past
the cursor walks every row below the bound. The probe is now one exclusive `gtlt` range, which both
5.2 and 5.3 read as a single bounded scan. Only discovery purges pass an endBound.

Also brings the queue keeper's subscription note up to date with 5.3's delivery (harper#2767): a
write is delivered only while the row's local entry still carries its version, so the not-owned
deletes 5.2 sent are gone. The keeper already ignores not-owned rows and re-checks every entry
against its durable row before a claim grants it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the @harperfast/prerender plugin to version 0.102.1, refactoring the URL range walking logic to use a single gtlt comparator instead of separate greater_than and less_than conditions. This change prevents Harper 5.3's query planner from reordering conditions and causing performance issues. The tests and mocks have been updated to support this new comparator. The reviewer pointed out a valid issue in the test mock where the comparator 'greater_than_or_equal' is incorrectly checked instead of 'greater_than_equal', which should be corrected to ensure the mock behaves as expected.

const gt = conditions.find((c) => c.comparator === 'greater_than')?.value ?? '';
const range = conditions.find((c) => c.comparator === 'gtlt')?.value;
const gt = range?.[0] ?? conditions.find((c) => c.comparator === 'greater_than')?.value ?? '';
const ge = conditions.find((c) => c.comparator === 'greater_than_or_equal')?.value;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The mock search method looks for the comparator 'greater_than_or_equal', but the actual comparator used by walkUrlRange (in urlWalk.js line 120) is 'greater_than_equal'. This mismatch causes the ge variable to always resolve to undefined during tests, making the mock filter fall back to u > gt (which resolves to u > '' and matches all URLs).

We should correct this to 'greater_than_equal' to ensure the mock filter behaves as intended and remains robust against future test data changes.

Suggested change
const ge = conditions.find((c) => c.comparator === 'greater_than_or_equal')?.value;
const ge = conditions.find((c) => c.comparator === 'greater_than_equal')?.value;

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