Skip to content

Fill full-text and autocomplete pages using Harper's existing paging - #3301

Merged
kriszyp merged 73 commits into
mainfrom
kyle/fulltext-snapshot-continuation
Oct 26, 2026
Merged

kriszyp merged 73 commits into
mainfrom
kyle/fulltext-snapshot-continuation

Conversation

@kylebernhardy

@kylebernhardy kylebernhardy commented Oct 19, 2026 •

Copy link
Copy Markdown
Member

⊙ Problem

Support full-text and autocomplete continuation in Table.search and REST exposed short pages when the first native result window was treated as the whole result set. Kris also identified that the separate continuation API added substantial machinery without providing consistency across Harper queries.

❓ Your call: Scope this issue to filling existing full-text and autocomplete pages, as Kyle requested after Kris's review. A general cross-query consistency API needs separate design and does not belong in this paging fix.

💡 Solution

Use Harper's existing limit and offset for full-text and autocomplete. Within each request, fetch enough eligible matches from one native reader to fill the requested page, reach actual exhaustion, or report a work-budget error. Separate page requests execute independently and may see changes between them.

The continuation functions, response headers, tokens, cross-worker owners, admission registry, retained leases, special return types, continuation-only tests and raw benchmark output have been removed. Public Resource/Table/REST paging stays on its existing contract. Compared with the prior pushed draft this removes over 5,000 lines; the current net change is 29 files, including native-adapter tests and configuration.

⚖️ Alternatives

Framing-Verdict: chosen-approach-sound (62f6ec1c30b6)

The fresh independent planning review cleared the existing-API approach. It follows adoption of the earlier smaller query-policy alternative: read only the three request leaves, while the native binding consumes process-wide runtime settings. The separate continuation API was removed in response to Kris and Kyle's direction. Planning clearance is separate from code-review convergence and CI status.

The first batch stays on reader.search. Released fulltext 0.6.0 classifies every traversal call as expensive, and selects its queue from that classification. Starting every query with traversal would route small ordinary queries through the global expensive-search permit. A bounded verified replay lets filled ordinary queries keep their existing queue isolation. Native rescoring remains library/Tantivy work in Reduce repeated matching work during deep ranked traversal.

A shared configuration-flattener repair would change aggregate handling for every camel-case section and the install/default merge path. This PR instead reads the existing registered leaves; the shared repair and required partial-install regression are tracked in Preserve camel-case aggregate values when flattening configuration. Capturing request policy once per index was not adopted: request policy is independent of native binding lifetime and tests change policy against an existing index; the measured three-read path does not justify another cache/invalidation mechanism.

The documentation review's option-set-too-narrow verdict 0364589df96e was resolved by adopting one restart-reference owner, aggregate-reservation wording and missing recovery/audit guidance. A vendored defaults checker and a product-wide error-code redesign remain outside this paging fix; copying a core snapshot would introduce another synchronized artifact without detecting later core drift automatically.

🔧 Changes

One request, existing paging

A bounded page returns eligible records up to its limit after the offset, or reports that its work budget was exhausted. One reader serves that request. No reader or query state is retained for the next request.

From query to page

Earlier matches are revisited for each offset request; filtering happens before the visible slice.

flowchart TD
    A[Existing limit and offset] --> B[Validate request limits]
    B --> C[Acquire one native reader]
    C --> D[Fetch ranked batch]
    D --> E[Check current source and filters]
    E --> F{Enough or exhausted?}
    F -->|No| G[Check budgets and traverse]
    G --> D
    F -->|Yes| H[Apply offset and limit]
    H --> I[Release reader]
Loading

Request defaults are 10,000 fetched hits, 10,000 retained offset-plus-limit records and 5,000 ms execution. Replay counts toward raw work. Raising raw work does not raise retained results. These are work/admission limits, not hard process-memory or response-time guarantees. HTTP request-setting changes require worker restart; native budgets require a full process restart. Many concurrent deep-offset queries can still contend for workers; this PR adds no per-caller concurrency scheduler.

✅ Verification

Main integration verified October 26

Merged current main, preserving its TypeScript conversion of manageThreads and its node: imports. The conflict resolution adds the existing full-text runtime import/provider to that converted module and retains the full-text validator beside main's updated imports. The two runtime-test imports now use the source-compatible alias. The paging algorithm, defaults and native dependency are unchanged by this merge.

Build, lint, typecheck, formatting and design-note checks pass. Focused schema/runtime/query/limit tests pass: 128 passing, two LMDB-only skips in RocksDB mode. Real-native lifecycle and Table.search tests pass: 32, no skips. Source TypeScript replacement-worker verification passes. Both deployed full-text suites pass independently under npm 11: 21 tests, no failures or skips.

The full resource gate reports 4,926 passing, 66 pending and one failing WebSocket test (socket.off missing in its fixture); the exact failure reproduces on unchanged main. The broad integration gate reports 2,499 passing, 65 failing, 172 cancelled, 18 skipped and two todo; all 21 full-text/example cases pass within it. Component-installation failures include npm 12's changed pack JSON shape, reproduced on unchanged main, but not every integration failure has been classified. The main unit gate finished with 7,927 passing, 222 pending and 89 failing under a finite 45-second test timeout, after the original run stalled in the process-tree test; that test also times out on main. The completed run includes npm 12 pack-format and EALLOWSCRIPTS failures, the MQTT cases below, and other failures that have not all been classified. No unrelated WebSocket, component-installation, certificate or locking fix is included, and no clean broad-gate result is claimed.

Fresh CI: the Windows job fails only the retained MQTT collection resume cases for QoS 1 and 2 at durableSessionResume.test.js:2244, returning [a1, a2, marker] instead of [a2, marker]. An earlier main Windows job independently shows the exact two assertions and extra a1. The exact-base main job passed, so the evidence is a recurring/intermittent main failure, not an always-failing baseline. The test and MQTT implementation are unchanged by this PR. The native full-text lifecycle step passed on Windows (25 tests). Other core CI jobs remain in progress; no retry or unrelated MQTT fix was made.

The required final Claude/Composer review and domain adjudication completed at this exact merge head (28 rounds / 15 full cumulatively). It did not identify a new worker/configuration conflict defect, but the canonical result remains non-converged and retains major labels for replay-budget headroom and per-query policy allocation, plus minor diagnostic/coverage and test-cost concerns. The pre-existing decisions remain explicit: fetched replay is work, the result ceiling does not promise that every admitted page fits the raw-work budget, and ordinary-first admission preserves the native queue distinction. Earlier three-read timing is narrow evidence, not a claim of full-query throughput qualification. Those findings are not represented as newly cleared by this merge; no additional full review is proposed just for SHA changes.

The final-artifact code is the reviewed merge commit. Scope remains the existing limit/offset fix, with no retained cross-request state. Both documentation companions are pushed and their exact combined production build passes. No PR merge, auto-merge, package publication or new reviewer request was performed.

Earlier verification and review history

End-to-end route: deployed Harper HTTP tests against real native fulltext 0.6.0. All 14 pass on Node 24.16 and the CI-pinned Bun 1.4.3, including independent result/raw configuration phases, completed worker restarts, and restore. The older local Bun 1.3.14 aborts in native async finalization during the new worker-rotation case; it does not reproduce on the pinned 1.4.3. This is a version-specific observation, not a proven root-cause claim. The Windows-only skip for this new rotation case follows Windows: investigate restart_service crash blocking component-load test paths.

Earlier combined query/native/runtime/limit suites: 101 passing. The latest cleanup rerun has 100 passing query/limits/runtime tests; build, lint, public types and changed-file formatting also pass. Build, lint, public types, design-doc checks and changed-file formatting pass. Real-native tests include a 10,025-record window regression and exact ordinary/traversal ordering parity at equal and unequal batch sizes. A Node 24.16 microbenchmark including all three populated env reads plus policy resolution measured 0.638 µs median across 9 × 100,000 calls (0.631–0.667 µs); this is not HTTP latency or throughput evidence.

The full gates run before the final narrow corrections were not green: main 7,344 passing / 220 pending / 41 failing; RocksDB resources 4,513 passing / 54 pending / 1 failing at subscriptionReplay.test.js:745; LMDB resources 3,428 passing / 743 pending / 0 failing; integration 2,262 passing / 70 failing / 196 cancelled / 18 skipped / 1 todo. Main/integration include npm-install policy, fixture and process-group failures; not all causes are classified. A fresh exact-base install was blocked by local npm policy, so these results do not establish baseline attribution. The audit-reference signature is now classified as shared baseline behavior by the independent main CI evidence below, tracked in Investigate missing audit references in the full resource suite; loader/SQL interference is tracked in Deployment activation and certification tests fail in full-suite runs while comparison cases pass in isolation, and subscription evidence remains in Subscription replay: startTime collection branch silently drops events committed after the audit cursor terminates. No historical red result is being labeled a consequence of this PR or a baseline failure without a matching control.

Fresh full implementation review and adjudication completed on the replay guard: 27 rounds / 14 full cumulatively. Claude and Cursor Composer supplied findings; Composer replaced a timed-out Grok leg. The canonical result remains not converged. The independent planning verdict above is cleared; it does not clear the code ledger or CI.

The new guard passed a fails-before/passes-after check: the prior implementation made two native calls in both impossible-budget cases; the corrected implementation rejects after one. The final query/configuration suites passed 100 tests; the native gate passed 32 tests, including eight directly exercising the published addon. A deliberate LMDB skip fails with the new --forbid-pending --fail-zero gate. That total includes the three additional lower-bound tests. They show why a total equal to the first batch cannot justify reserving a full requested page: that batch may be the entire result set, and returning its eligible rows is valid.

The strengthened native gate passes on Linux, macOS and Windows in the completed feature CI run. That run failed in Node 22 LMDB and Node 26 resource-core for the shared signatures detailed below. The completed cleanup run passed Node 22 (including LMDB) and all three native planes. Its Node 26 resource-core job failed at the same audit-reference assertion below. Its Node 24 LMDB job exited 139 after the GraphQL-declared elements-attribute heading, a different boundary from the earlier captured subscription crash; that exact failure remains unclassified. The final current-head CI run passes all three Node unit jobs, including resources and LMDB, and all three native full-text planes.

Remaining review concerns and their disposition:

Concern Disposition
Policy resolution/unused runtime validation on the query path Accepted measured overhead: the populated three-read plus resolver path measured 0.638 µs median as described above. The canonical ledger still marks the combined finding unmeasured/major; this author reconciliation does not rewrite its receipt. No cache is added.
Redundant per-hit source-version check Removed only the redundant non-string version guard from the source-entry helper; the helper still compares each hit version against the stored entry version, and its only caller rejects missing/non-string versions first. The existing missing-version regression and all 100 focused query/limits/runtime tests pass after the cleanup.
Invalid-policy 500 lacks a dedicated server log Accepted diagnostic limitation: the client receives the precise configuration error; no new per-request logging path is added.
Lower-bound total cannot predict future matches Preserve actual exhaustion semantics. The new guard uses only proven minimum work; an uninformative total can still require replay before a later budget error. The three added tests cover both outcomes.
Equal raw/result defaults Both are independent ceilings, not promises that every target is reachable. Replay consumes raw work; requests near the result ceiling may need a higher raw budget.
Temporary LMDB diagnostic changes Removed after capturing the failure. The final workflow diff contains only the full-text capability probe and native-test coverage gate. Shared LMDB/audit repairs remain outside this PR.
Duplicate deep native corpus and ordering contract Keep the real 10,025-record regression in both planes; published-addon tests verify equal/unequal collector prefixes and ordering. The duplicate CI cost is deliberate.
Highlight abort and concurrency qualification Deferred to the linked qualification issue below; no claim of prompt highlight/blob cancellation.
Logging-suppression state before a logger failure Pre-existing diagnostic behavior, unchanged here; no logging subsystem redesign in this paging change.

❓ Your call: Post-review delta: 61 additions / 21 deletions across four already-reviewed files, not re-reviewed: three lower-bound regression cases, their design clarification, removal of temporary LMDB crash instrumentation, and removal of duplicate source-version validation. The caller still rejects missing/non-string native versions before source access; the existing missing-version regression passes. No new API or query behavior is introduced by this cleanup. No clean canonical convergence is claimed.

The ordinary-first admission decision is supported by the exact published Rust references above. First ordinary batches are capped from maximum ID/version sizes and the advertised response envelope; an unexpected first-batch native oversize error remains an explicit 413. The early and late unbounded-limit errors name the effective minimum bound. Replayed hits count again because they are fetched again; excluding them would violate the raw-work contract.

The earlier whole-description factual review timed out without returning coverage. A fresh read-only Claude check verified the latest Node 24/26 failure wording, the 100 focused-test result, and the behavior-preserving duplicate-type-guard removal; its wording finding was corrected. Historical verification and control-run claims were outside that narrow reader’s coverage and remain supported by their cited evidence rather than a new whole-description review. The latest factual corrections are incorporated; previous pushed-head CI completed successfully.

Kris review closeout

Fresh verification on the unchanged core head: 119 focused schema, native runtime replacement-worker and query tests pass (two LMDB-only tests skipped in RocksDB mode), plus 21 deployed full-text and documented-example integration tests pass with no skips. The latter include rejecting removed paging functions on GET and DELETE before records can be changed.

The implementation addresses the five inline requests below. Each thread now has a response linking current code and executed verification, and all five are resolved. Resolving the threads does not claim a new human approval of the revised approach.

Review concern Final implementation and verification
Search return shape Removed continuation functions and the Promise/iterable split. Public Table/Resource/REST entry points match the PR base; deployed tests reject the removed functions before they can switch behavior.
Ordinary request work Defaults are 10,000 fetched hits and 5,000 ms; the execution deadline applies to ordinary and traversal paths. Unit tests exercise the configured deadline, and deployed tests separately exercise raw/result limits.
Duplicate serialization Removed continuation owner/requester code and its cross-worker page messages, including both serialization passes.
Raw benchmark output No continuation benchmark directory or raw measurement JSON remains in this PR. Qualification evidence is tracked in the linked qualification issue below.
Process-wide runtime capture Main-thread snapshot propagation and real-native replacement/nested-worker tests verify that changed native budgets do not reject replacement workers. Full process restart remains required to apply new budgets.

Author closeout classified all 30 retained canonical-ledger entries against the final head: removed-code/stale findings, final fixes, measured/explicit tradeoffs, and tracked qualification. The new full review above supplies independent coverage; the author closeout itself does not, and the canonical receipt is still not converged. Replay is charged as fetched work, the true 10,000-window fixture is retained despite duplicate CI cost, and highlight cancellation/concurrency qualification remains open rather than represented as fixed. Individual full-text configuration leaf names are supported for set_configuration; whole-block storage_fullText is not.

Node 22 LMDB investigation

Observed: the original Node 22 job exited 139 after the subscription replay empty-audit-log case. It matches Flaky: LMDB SIGSEGV in subscriptionReplay edge cases on Node 22, 24 and 26. Independent attempt 1 on the extended-iterable update branch shows the same test window and exit 139. The subscription test is byte-identical at this PR's base/head and inspected current main; this PR does not change the LMDB dependency. These are evidence of the known shared failure signature, not proof of its native cause.

Local Node 22.23.3 on macOS arm64: subscription replay alone passes all 43 tests. The full local LMDB gate passes: resources 3,431 passing / 743 pending, API 201 passing / 2 pending, and CLI 366 passing / 5 pending; zero failures. The targeted CI rerun failed again at the same empty-audit-log boundary, approximately eight seconds after the last passing test. This is a recurrent failure, not a cleared gate. Neither failure retained a native stack/core artifact. The instrumented Node 22 job failed again and captured the native stack: mdb_cursor_close at mdb.c:10432, through CursorWrap::close at cursor.cpp:55. A separate wrapper core propagates the child signal; it is not the native fault location. Current main CI independently crashes in the same subscription window. The stack narrows the fault to cursor cleanup; it does not yet prove the producer of the invalid lifetime. Temporary capture instrumentation was removed from the final PR scope. No speculative LMDB dependency change or test skip was added; native root cause remains unproven. These historical crash observations remain separate follow-up work; no merge is authorized.

The fresh local full-resource run reports 4,519 passing / 54 pending / 1 failing: transaction.test.js:507, missing additional audit references during complex resequencing. That audit-reference case passes alone (1 passing), so full-suite interaction remains a hypothesis. The fresh integration gate finished with 2,311 passing / 60 failing / 158 cancelled / 18 skipped / 1 todo; all 14 full-text HTTP cases pass. Several dependency-deployment failures explicitly report local npm EALLOWSCRIPTS; not all failures are classified, and the gate is not green. Main finished with 40 failing / 220 pending. Its reported 7,345 passing includes one process-group fixture that required external interruption; exclude that case from verified passes. Several failures report npm policy errors; other loader/runtime and process-group failures remain unclassified. Local exact-base installation was blocked by npm policy (EALLOWREMOTE for the locked uWebSockets GitHub tarball, then EALLOWSCRIPTS with npm 11); it supplies no executed local control. The audit-reference attribution below instead uses existing independent main CI, not this blocked setup. The main run required interruption of a process-group fixture after an approximately 11-minute stall; that case is not a pass.

Audit-reference attribution and remaining readiness gates

Conclusion: the audit-reference assertion is a pre-existing shared failure, not introduced by this full-text change (high confidence).

Independent evidence Observed result
Feature Node 26 job Node 26.12.1 / npm 11.20.0: 3,867 passing, 47 pending, one resource-core failure at transaction.test.js:507: missing additional audit references.
Main Node 26 job Node 26.12.0 / npm 11.20.0: the same test and assertion fail, before the latest feature push and without this PR.
Main Node 24 job The same assertion fails with the audit-write routine, test, transaction wrapper, encoder and Rocks transaction-log implementation matching this PR's base/head.
Exact-base CI Node 26 passed on the exact PR base. Together with the independent failures this is intermittent evidence, not proof of a new feature regression.

The main controls lock fulltext 0.5.0; this PR locks 0.6.0. Both lock RocksDB 2.12.0, extended-iterable 1.0.5 and LMDB 3.5.7. The primary record-write method is byte-identical between the PR base/head and the failing Node 24 main control; the test and the three other owning files have identical Git blobs. Node 26 main also includes a later source-apply duplicate guard; the failing test performs local patches, not source-apply replay. This establishes an independently existing failure; it does not quantify whether added tests change its occurrence rate or establish its root cause. Keep the repair in Investigate missing audit references in the full resource suite, without weakening the assertion or adding a skip here.

Readiness: current-head CI is green: 56 passing checks, six skipped, zero failures and zero pending. The completed unit run passes Node 22, 24 and 26, including all resource and LMDB stages, plus native full-text tests on Linux, macOS and Windows. Integration and packaging checks also pass. One canceled unexpanded consumer-install placeholder belongs to a superseded run; its replacement packaging run passes all four actual consumer-install jobs. Historical audit-reference and LMDB failures above remain disclosed and are not claimed fixed. The docs companion is green. Kris's five requests have implementation/evidence mappings above and verified closeout replies; all five threads are resolved, and no new approval is claimed. The canonical external code-review receipt remains non-converged with the explicit accepted/deferred dispositions above. Kyle authorized moving out of Draft after readiness checks clear; no merge or unrelated repair is authorized.

Deferred qualification remains in Qualify native full-text filtering and catalog-scale operation for v5.4: concurrency, deep-offset event-loop cost, memory/I/O and prompt abort during highlight/blob reads. Lifecycle work remains in Implement full-text generation lifecycle, rebuild, eviction, backup, and status. Native 64-term / 256-clause configuration remains in Make query term and clause limits configurable with bounded resource policy. Stop rolling-restart test probes after an assertion failure tracks a confirmed unrelated test cleanup defect. The old retained-search follow-ups Reuse full-text highlight tracing without an extra continuation reader and Evaluate fairness for full-text continuation admission are not prerequisites for this API-free paging fix.

Companion: Document full-text paging and configurable search limits.

Comment generated by kAIle (GPT-6)

Release target: v5.4 — bounded full-text paging using existing APIs.

Related PRs: #372 overlaps (Add integrated logger-status system with .status() API, health checks and hierarchical view, shared configuration schema), #3199 overlaps (Run source-compatible unit tests without a build, test workflow and package manifest), #3234 overlaps (Apply HTTP request queue limits to GET, HEAD and OPTIONS, shared config validation), 10 others independent

Complexity: complicated

Review-Coverage: authored=codex; ran=cursor-composer,claude,cursor-muse; adjudicated=domain; blocked=cursor-grok(failed); declined=gemini,cursor-kimi; rounds=28; full=15 @ 6538281

Review-Attention: deep ~70m (critical: fullTextNativeBinding.ts, fullTextQueryIndex.ts +3; sensitive: package-lock.json; hot: manageThreads.ts, socketRouter.ts; decisions: do-less-alternative, ordinary-first-admission, runtime-snapshot-boundary, exhaustion-semantics, replay-work-accounting, release-target-vs-evidence) @ 6538281

@kylebernhardy kylebernhardy added this to the v5.4 milestone Oct 19, 2026
@socket-security

socket-security Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​harperfast/​fulltext@​0.6.09410010096100

View full report

@kylebernhardy kylebernhardy changed the title Add full-text and autocomplete snapshot continuation Fill full-text and autocomplete pages using Harper's existing paging Oct 21, 2026
@kylebernhardy
kylebernhardy marked this pull request as ready for review October 21, 2026 21:28
@github-actions
github-actions Bot requested a review from kriszyp October 21, 2026 21:28
@kylebernhardy

Copy link
Copy Markdown
Member Author

Thanks, Kris. The revised implementation removes the separate continuation API, cross-worker owner/protocol and raw benchmark dump. It fills existing limit/offset pages with one native reader per request, bounded raw/result/time work, and no retained state or consistency promise between requests.

All five inline concerns now have current-code and test evidence and are resolved. Fresh verification on the unchanged core head passes 119 focused tests (two LMDB-only skips in RocksDB mode) and 21 deployed HTTP/example tests with no skips. This records the response to the earlier design concern; it does not claim a new approval of the revised approach.

Comment generated by kAIle (GPT-6)

@kriszyp
kriszyp merged commit 2c22cab into main Oct 26, 2026
63 of 68 checks passed
@kriszyp
kriszyp deleted the kyle/fulltext-snapshot-continuation branch October 26, 2026 19:38
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.

2 participants