fix: address query scheduler follow-up review feedback - #1994
Conversation
Review —
|
| R1 shared-flight signal leak | Proven end-to-end against a real GraphSetIndexStore (round 2's test used a bare OxigraphStore and therefore could not have caught the bug). Reverting :894-899 to reads.listGraphsByPrefix → RED: promise rejected "first caller disconnected" instead of resolving. Genuine fail-before. |
| R1 completeness | Every store read classified. The three coalesced entry points are listGraphs, listGraphsByPrefix and — the one everyone missed in round 2 — hasGraph (graph-set-index-store.ts:337-338, ensureGraphSet when pendingFullRefresh). It is reached only via listGraphFamily:1508-1514, whose sole call site :1100 receives reads.shared (passed at :1071). Covered. I verified this one independently — I had it as a suspected HIGH and it turned out to be sound. |
| R2 eager classifier | inferQueryOperation now at sparql-http.ts:729, behind the gates at :720/:722/:723. |
R3 AbortSignal.any leak |
100k-op soak, --expose-gc: old 108.80 MB → new 0.00 MB. Retained abort listeners on the long-lived signal: 0. Reinstating the AbortSignal.any branch kills all three new tests. |
| R3 did it break cancellation? | 10/10 against a real node:http server (headers-then-hang): abort mid-SELECT-body, mid-readResponseTextBounded, mid-CONSTRUCT, close() mid-body, close() mid-UPDATE, two concurrent close(), and 200 abort/completion races — all settled correctly, none hung. dispose() never unlinks a live signal, also true structurally: store-priority-scheduler.ts:346-370 only rejects queued entries, so task cannot settle while work() still holds the signal. |
| R4 unterminated response | if (!res.writableEnded) res.end(); |
| R5 route test was vacuous | Now genuinely pins it. Mutating query.ts:678-680 (signal→undefined, priority→'normal') → RED; deleting the disconnect branch alone → also RED. Both arms independently covered. |
| LOW shed-as-failure | respondIfApiQueryStoreBusy hoisted above tracker.fail; a shed now records tracker.cancel. |
| LOW badge | badge-cancelled arm at :1685, the badge-warning→badge-warn typo fixed, .badge-cancelled added to 12-observability.css:139, phase details render for cancelled at :1377. |
| LOW boot lane | configureApiQueryPriority at lifecycle.ts:1175, before listen at :3700; warns on unrecognised values; route reads the boot-resolved getApiQueryPriority() at :382, not process.env per request. |
The ACL parser survived its third move. A 38-case differential matrix between c50b39235 and be2e59b13 produced no fail-open. Only two verdict flips, and the second is a fix: at c50b39235, collectGraphVariables (code-point) and callerGraphValuesAreAuthorized (code-unit) disagreed about astral variable names — ?g𝕏 read as ?g by one of them. Consolidation makes them literally the same function, so they can no longer disagree; adversarially probed with astral+FOREIGN, astral+mixed, and mismatched VALUES/GRAPH variable pairs, all of which inject. The #1989 positive path is still byte-identical, and the guard-order test I asked for is there and load-bearing — moving both asserts below the early return turns it red, and one of the two mutations leaked a row.
I also closed the one question the lens left open: nothing re-wraps StoreSchedulerBusyError between the store and the route. The query engine has four catch blocks total, its single throw new Error (:1262) is guarded on ExactGraphReadError/QUAD_COUNT_MISMATCH, and finalization-handler.ts explicitly rethrows it (:472, :510, :963). So the 503 mapping works on the real path.
LOW 1 — the lexer consolidation dropped an ungated IRIREF scan, and that was my advice's fault
I said "move the forked primitives into one module and import in both." Applied literally, one implementation of each pair had to win — and for findMatchingCloseBrace the gated copy won. The two callers have different correctness criteria: a brace matcher only needs to skip <…> opaquely, whereas the value-extracting skipSparqlIriRef legitimately validates the first character. Now a digit-initial IRIREF containing # or ' fails the gate, falls through to i++, and the # is treated as a line comment that swallows the closing brace:
<1#x> in WHERE old=INJECT new=THROW (WHERE block could not be located)
<1'x> in WHERE old=INJECT new=THROW
<2001/schema#a> pred. old=INJECT new=THROW
<1x> (no # or quote) old=INJECT new=INJECT ← unchanged
<ab#x> letter-start old=INJECT new=INJECT ← unchanged
The direction is fail-closed, verified for all three consumers of the -1 return: constrainGraphVariablesToAllowedSet throws; injectMinTrustFilter returns null → empty result; wrapWithGraph/wrapWithGraphUnion return the query unwrapped, which executed against the store yields [] (default-graph-only, no named-graph leak). So this is a usability regression on an exotic-but-legal IRI shape, not an exposure.
Fix: give findMatchingSparqlCloseBrace an ungated <…> opaque skip while leaving skipSparqlIriRef gated for the value-extracting callers. One module, two behaviours, because the callers genuinely need different things.
LOW 2 — the successRate denominator went wider than I meant, and hides the exact incident this PR family targets
My note was "a shed shouldn't count as a failure." The fix changed the denominator to success + error (db.ts:2552), which also drops in_progress. Executed against the verbatim SQL under node:sqlite with 5 success / 2 error / 3 cancelled / 4 in-progress:
NEW successRate = 0.714 OLD successRate = 0.357
EMPTY {"totalCount":0,...} -> rate 0 ALL-CANCELLED -> rate 0
On a #1989-style store wedge every hung /api/query sits at in_progress forever, so the dashboard health rate stops degrading precisely when the node is least healthy. Null-safety and divide-by-zero are correct, so this is presentation rather than a defect.
Don't apply the obvious fix literally — adding in_progress back to the denominator means that under steady load a window always contains in-flight rows and the rate can never reach 1.0, which is why the original formula was also wrong. Better: keep healthCount as-is and surface in_progress as its own tile.
LOW 3 — the new lifecycle test pins that dispose() unlinks, never when
Hoisting signalScope.dispose() out of task.finally to immediately after start() — unlinking a still-live signal, the exact regression worth fearing here — leaves abortable-store-work-lifecycle.test.ts fully green (3/3). The class is caught hard elsewhere (sparql-http.test.ts goes 29-failed under that mutation), so this is coverage hygiene, not exposure. One case fixes it: start work that never resolves, assert the generation signal still has exactly one abort listener while in flight.
Also worth a heads-up, not a finding: storeCancellationCompletedTotal now also counts aborts during body parsing (sparql-http.ts:246-266), which previously produced no metric. Single count, no double-increment — but anyone watching that series will see a step change.
CI
Currently red, and I don't believe it's this PR. The failure is hermes-adapter.part-08.test.ts › H-AC-31 — a pure filesystem test (mkdtempSync/writeFileSync/readdirSync around setupHermesProfile) in adapter-hermes, a package this delta does not touch; the delta is confined to query/storage/cli/node-ui. It timed out at 5029 ms against the 5 s default rather than asserting wrong, and the job log shows Hermes local-agent registration skipped: fetch failed. Separately, the storage suite's parallel-worker failures are pre-existing: 18 failed at be2e59b13 vs 16 at c50b39235, and in isolation both worktrees give 82 passed (82). A re-run should clear the gate.
Merge readiness
Approve. All three findings are LOW and none blocks. LOW 1 is the only one I'd want fixed reasonably soon, since it silently rejects a legal query shape.
One coordination note: #1994 contains #1991, so merging this lands both — #1991 should be closed rather than merged separately, or the order made explicit.
Method: 3 lenses over the delta, schema-free, executing real modules (AbortableStoreWorkLifecycle under --expose-gc, both engine versions side by side over a shared corpus, a real node:http server for cancellation, a real GraphSetIndexStore for R1, node:sqlite for the health SQL). Every claimed test was mutation-tested with a proven restore. I independently verified the hasGraph lane coverage, the StoreSchedulerBusyError propagation path, the boot-lane wiring, R4 and the badge fix against the worktree at be2e59b13.
| const sharedReads = 'shared' in reads ? reads.shared : reads; | ||
| const callerSignal = 'shared' in reads ? reads.signal : undefined; | ||
| const allGraphs = await raceAgainstCallerAbort( | ||
| sharedReads.listGraphsByPrefix(prefix), |
There was a problem hiding this comment.
🔴 Bug: Aborted API queries can still start uncancellable graph-discovery work
What's wrong
The cancellation race is applied after the discovery promise has already been created. Because the shared lane intentionally omits the caller signal, an already-aborted request can still enqueue or start graph-discovery reads that cannot be cancelled by that request.
Example
If an /api/query request disconnects before graph discovery begins, createApiQueryRequestLifecycle passes an already-aborted signal. discoverGraphsByPrefix still evaluates sharedReads.listGraphsByPrefix(prefix) first, admitting a store graph-list read with no abort signal, then raceAgainstCallerAbort rejects the HTTP query. The backend/scheduler work continues even though there is no caller left.
Suggested direction
Gate shared discovery on the caller signal before constructing the promise, or split shared/coalesced GraphSetIndexStore refreshes from ordinary store discovery so already-aborted callers do not admit backend work.
For Agents
Look at createQueryStoreReadContext, discoverGraphsByPrefix, and resolveScopedContentGraphAllowList. Preserve the intended GraphSetIndexStore shared-flight behavior, but check reads.signal?.aborted before starting shared discovery and avoid dropping cancellation for non-shared/direct discovery paths. Add a test where the query signal is already aborted before graph discovery and assert no store listGraphsByPrefix/listGraphs call is made.
| warn(message: string): void; | ||
| } | ||
|
|
||
| let effectiveApiQueryPriority: ApiQueryPriority = 'background'; |
There was a problem hiding this comment.
🟡 Issue: Avoid routing policy through a mutable singleton
What's wrong
The new module extracts parsing, but the actual policy is still stored as process-global mutable state. That makes the route behavior depend on an out-of-band startup call, forces tests to reset shared state manually, and makes future per-daemon/per-test configuration harder to reason about.
Example
A route-level test or embedded daemon harness that calls handleQueryRoutes without running runDaemonInner silently uses the module default. A separate test that calls configureApiQueryPriority('normal') must remember to restore global state or later requests inherit the wrong lane.
Suggested direction
Keep the env parsing helper pure and inject the resolved lane through RequestContext or the route handler factory. That deletes getApiQueryPriority, removes the route module's dependency on daemon startup side effects, and makes tests local instead of order-sensitive.
For Agents
Move the resolved API query priority into an explicit daemon/request boundary, likely a new RequestContext field populated from runDaemonInner/handle-request input. Keep resolve/logging at startup, preserve /api/query options { priority, source, signal }, and adjust route tests to build the context with the intended priority instead of mutating module state.
| return { bindings: all }; | ||
| } | ||
|
|
||
| private async discoverGraphsByPrefix( |
There was a problem hiding this comment.
🟡 Issue: Make graph-discovery signal ownership explicit instead of structural
What's wrong
The abstraction hides the most important invariant behind a structural runtime check: some graph discovery calls must not receive the caller signal, while execution reads must. Because both flows are represented by nearly identical query/listGraphs... wrappers, readers have to chase the object shape to understand cancellation ownership.
Example
A future helper that receives a StoreReadLane and calls discoverGraphsByPrefix cannot tell from the type whether it is using caller-scoped reads or the shared discovery lane. The behavior changes based on a runtime property check, not the method name or type boundary.
Suggested direction
Replace the QueryStoreReadContext | StoreReadLane union plus 'shared' in reads dispatch with named helpers or distinct types for caller-scoped execution reads and shared graph discovery. The code should communicate at the call site which lane is being used.
For Agents
In packages/query/src/dkg-query-engine.ts, split the read model into explicit execution and discovery responsibilities. For example, make QueryReadContext the only top-level query argument and provide a dedicated discoverSharedGraphsByPrefix(prefix, ctx) helper, while lower-level discovery methods accept an already-selected GraphDiscoveryLane. Preserve the invariant that execution reads carry caller signal and shared discovery flights omit it but are raced against the caller locally.
| COUNT(*) as totalCount, | ||
| SUM(CASE WHEN status = 'success' THEN 1 ELSE 0 END) as successCount, | ||
| SUM(CASE WHEN status = 'error' THEN 1 ELSE 0 END) as errorCount, | ||
| SUM(CASE WHEN status IN ('success', 'error') THEN 1 ELSE 0 END) as healthCount, |
There was a problem hiding this comment.
🟡 Issue: Centralize the health-rate denominator instead of duplicating status policy
What's wrong
The diff changes the meaning of successRate by sprinkling the same status policy across several queries and one JS mapper. That leaves an important analytics rule as repeated string/formula fragments rather than a named model.
Example
If another non-health terminal status is added later, this metric policy has to be updated in four places, and one path already expresses the same rule differently from the others.
Suggested direction
Give success/error health eligibility a single local abstraction and reuse it across summary, time-series, per-type, and by-type rate queries. Even a small SQL fragment plus rate helper would make the policy obvious and harder to drift.
For Agents
Introduce one named health denominator concept in packages/node-ui/src/db.ts, such as a SQL fragment/helper for terminal health statuses and a small successRate(success, error) helper for mapped rows. Preserve counts and existing public return shapes; add or adjust the existing stats tests to prove cancelled rows stay visible but out of the health denominator.
| <div style={{ display: 'flex', gap: 4, marginBottom: 3, flexWrap: 'wrap' }}> | ||
| {phases.map((p: any, i: number) => { | ||
| const color = p.status === 'error' ? '#ef4444' : PHASE_COLORS[p.phase] ?? PHASE_FALLBACK_COLOR; | ||
| const color = p.status === 'error' |
There was a problem hiding this comment.
🟡 Issue: Do not scatter another operation-status presentation branch through the large Operations page
What's wrong
The cancelled-state UI is implemented as repeated special cases inside an already large page. The behavior is simple, but the structure makes status presentation a cross-cutting concern that future changes will have to rediscover and update by hand.
Example
Cancelled phase presentation now requires coordinated edits across MiniGantt labels, MiniGantt bars, hover text, the detail timeline, the legend, detail rows, and StatusBadge. The page already has 1,731 lines, so adding another status as scattered conditionals will make the component harder to scan.
Suggested direction
Model status presentation once and let each view consume that model. This is a better fit than continuing to add one-off status === 'cancelled' branches inside each rendering path.
For Agents
Extract a small operation/phase status presentation helper near the existing phase palette, for example returning { badgeClass, color, label, terminalDetailsLabel }. Replace the repeated ternaries in MiniGantt, PhaseTimeline, OperationDetail, and StatusBadge while preserving the current colors and labels.
| any?: (signals: AbortSignal[]) => AbortSignal; | ||
| }).any; | ||
| if (AnyImpl) return AnyImpl([primary, secondary]); | ||
| ): AbortSignalScope { |
There was a problem hiding this comment.
🟡 Issue: Make composed-signal cleanup lexical instead of caller-managed
What's wrong
The new API fixes listener lifetime by adding a manual disposal protocol, but it exports that protocol to every caller. This trades one lifecycle problem for an easy-to-misuse abstraction: cleanup is now correct only if each call site remembers the same ceremony.
Example
The next caller can easily write const { signal } = composeAbortSignals(a, b); return fetch(url, { signal }); and leak listeners on long-lived signals because disposal is a convention rather than enforced by the API shape.
Suggested direction
Use a callback-based helper or another ownership pattern that pairs signal composition and disposal in one place. That removes duplicated try/finally blocks and prevents future call sites from accidentally forgetting cleanup.
For Agents
In packages/storage/src/abortable-store-work-lifecycle.ts, consider replacing the exported raw scope protocol with a lexical helper such as withComposedAbortSignal(primary, secondary, fn), or keep the scope internal and expose a safer wrapper. Preserve current abort reason forwarding and ensure the existing SparqlHttpStore query/update paths still keep the composed signal alive through body consumption.
3 participants
Context and scope
Follow-up to the review feedback on merged PR #1991. The original change prevents scoped API reads from starving higher-priority store work; this PR tightens the cancellation, shared-flight, telemetry, route, and UI invariants identified in two review rounds.
Related to #1989 and the SWM symptom evidence in #1990.
This PR does not claim that the separate mainnet Blazegraph atomic graph-replace HTTP 500/deadline failures have the same upstream query. The
urn:dkg:internal:atomic-graph-replace:*names are transactional staging graphs. Recovering the Blazegraph/Jetty exception and correlating JVM, disk, lock, and scheduler telemetry remain separate evidence requirements.What this fixes
The proven code-level failure was a cross-workflow store-pressure cascade:
/api/queryread could enter the normal store lane;The branch keeps API reads in the
backgroundlane by default, threads cancellation to interruptible stores, preserves shared-flight safety, and makes cancellation a first-class terminal outcome.Finding-by-finding resolution
First review round
signal,priority, orsourceQueryStoreReadContextat query entry and expose bound execution and shared-discovery lanessuccess / (success + error)StoreSchedulerBusyErrorclasssparql-utils.ts/api/querypassed the valueshandleQueryRoutesdirectly and assert signal, priority, source, disconnect, and 503 behaviorSecond review round
GraphSetIndexStorerefresh could capture the first caller's signalGraphSetIndexStoretwo-caller test: A aborts, B completes, one upstream scanAbortSignal.anycompositions retained source-signal graphsAbortSignal.anywith an explicit disposable scope; dispose on synchronous start failure, task settlement, adapter completion, and abort/api/queryresponse was never endedres.end()when the disconnect error reaches the route and the response is not already endedwritableEnded === truehandleQueryRoutescoverage for exact option handoff, disconnect cancellation, and typed 503 sheddingStoreSchedulerBusyErrorascancelledbefore returning retryable 503DKG_API_QUERY_PRIORITYhad no boot-time validation or effective-value logbackground, and log the effective laneVALUESwere missingGRAPHvalidationVALUESdoes not bypass either guardSequence: shared discovery is caller-safe
sequenceDiagram autonumber participant A as API caller A participant B as API caller B participant R as Query route participant Q as Query engine participant I as GraphSetIndexStore participant S as Store backend A->>R: POST /api/query R->>Q: signal A, background, api.query Q->>I: listGraphsByPrefix with shared lane Note over Q,I: priority and source retained; caller signal omitted I->>S: start one refresh B->>R: POST /api/query R->>Q: signal B, background, api.query Q->>I: join the same refresh A--xR: client disconnects R->>Q: abort signal A Q--xR: caller A local race rejects R->>R: tracker.cancel and response.end S-->>I: graph list I-->>Q: shared refresh resolves Q-->>R: caller B continues R-->>B: 200 responseSequence: cancellation ownership lasts through body cleanup
sequenceDiagram autonumber participant R as Route caller signal participant L as Store work lifecycle participant P as Priority scheduler participant H as SPARQL HTTP adapter participant B as Response body R->>L: run caller work L->>L: compose caller plus generation scope L->>P: admit tracked task P->>H: execute with lifecycle signal H->>H: compose lifecycle plus deadline scope H-->>B: fetch resolves headers Note over H,B: signal scope remains linked alt caller disconnects during body decode R->>L: abort caller L->>H: propagate abort H->>B: abort delayed json, text, or N-Quads read B--xH: body cleanup rejects else body completes normally B-->>H: parsed response end H->>H: dispose HTTP scope H-->>P: operation settles P-->>L: release admission L->>L: dispose lifecycle scope Note over P,L: retry can enter only after prior cleanup settlesSequence: normal reads skip full telemetry parsing
sequenceDiagram autonumber participant Q as Completed SPARQL query participant T as Slow-query telemetry participant C as SPARQL classifier participant E as Event sink Q->>T: sparql, source, start time T->>T: threshold enabled? alt below threshold or telemetry disabled T-->>Q: return without scanning query else threshold exceeded T->>T: sampling gate alt not sampled T-->>Q: return without scanning query else sampled T->>C: classify complete query C-->>T: operation form T->>T: hash and size query T->>E: emit bounded slow-query event end endBenchmarks
Environment: Node
v25.2.1, macOS arm64. These are isolated microbenchmarks for the reviewed hot paths, not end-to-end HTTP latency.Slow-query telemetry gate
Nine-run median. The eager column runs the full canonical SPARQL classifier, matching the old placement. The gated column runs the current below-threshold fast path.
The important result is asymptotic: ordinary queries no longer scan their full SPARQL text for telemetry. Classification still runs for sampled slow events.
Abort-signal composition retention
Forced-GC soak over 200,000 completed compositions sharing one long-lived generation signal:
AbortSignal.anydispose()On this runtime, explicit disposal reduced retained heap by more than 99.97% and composition time by about 70.9%. Absolute retained bytes vary by Node/V8 version; the invariant enforced by tests is balanced listener ownership after every settled operation.
End-to-end terminal behavior
successerrorRetry-After: 1cancelledcancelledValidation
@origintrail-official/dkg-storage@origintrail-official/dkg-query@origintrail-official/dkg-node-ui@origintrail-official/dkgCLIgit diff --checkBase proof
Current head:
be2e59b1366d9985f77639279b413d7a4416203f.At the time of this update:
main:b676567723d038649690681375a0fbf1142915dbtestnet-canary:736d6c28e3508998d60cf175ebab4eb890f3a7feReview focus