Repository navigation
Fill full-text and autocomplete pages using Harper's existing paging - #3301
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
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) |
⊙ 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.
💡 Solution
Use Harper's existing
limitandoffsetfor 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-narrowverdict 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
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]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.offmissing 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 extraa1. 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-zerogate. 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:
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.
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_closeatmdb.c:10432, throughCursorWrap::closeatcursor.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 npmEALLOWSCRIPTS; 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 (EALLOWREMOTEfor the locked uWebSockets GitHub tarball, thenEALLOWSCRIPTSwith 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).
transaction.test.js:507: missing additional audit references.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