fix: prevent scoped API queries from starving store work - #1991
Conversation
release: promote testnet-canary to main for 10.0.10
chore(release): bump version set to 10.0.10
| subGraphName, | ||
| callerAgentAddress, | ||
| signal: queryAbortController.signal, | ||
| priority: 'background', |
There was a problem hiding this comment.
🟡 Issue: API query priority and cancellation are not applied to graph discovery reads
What's wrong
The route now marks API queries as background work and wires disconnect cancellation, but the query engine only forwards those options to the final store query. Several planning/discovery reads still run before that final query and use the store defaults, so a cancelled or slow API query can continue occupying normal store capacity during graph enumeration/metadata discovery. That undercuts the isolation this change is meant to provide for promotion, reconciliation, validation, and catch-up work.
Example
A disconnected POST /api/query using includeContextGraphPartitions: true and GRAPH ?g still runs the allow-list discovery queries at the store's default normal priority, and those discovery reads do not observe queryAbortController.signal. Only the final rewritten SPARQL execution is background/cancellable.
Suggested direction
Propagate the new store options through all store reads that are part of query planning/discovery, not only the final execAndNormalize calls.
For Agents
Thread storeOptions(options) through the query-engine discovery helpers used during scoped/view routing, including discoverGraphsByPrefix, listGraphFamily, discoverRegisteredSubGraphNames, discoverRegisteredAssertionGraphs, discoverKnownChildContextGraphUris, and private-graph discovery. Preserve existing graph allow-list semantics, and prove with a recording store that every store call made by an API routed graph-variable query receives the same signal, priority, and source.
| remaining = newline === -1 ? '' : remaining.slice(newline + 1).trimStart(); | ||
| continue; | ||
| } | ||
| const prologue = remaining.match( |
There was a problem hiding this comment.
🟡 Issue: Reuse the canonical SPARQL operation classifier
What's wrong
The PR adds another bespoke SPARQL prologue parser for telemetry even though the repo already has a canonical operation classifier. That increases parser drift and makes future SPARQL lexical fixes land in multiple places.
Example
If the core classifier is hardened for another valid prologue shape, the query guard and this slow-query telemetry path can classify the same SPARQL differently because this adapter now owns a second parser.
Suggested direction
Delete the local scanner and map the canonical classifier's read form to the telemetry operation value.
For Agents
In packages/storage/src/adapters/sparql-http.ts, replace inferQueryOperation's local prologue parser with classifySparqlOperation from @origintrail-official/dkg-core. Preserve the existing event strings: select, ask, construct, describe, unknown. Keep the slow-query test that covers PREFIX-prefixed SELECT queries.
There was a problem hiding this comment.
🟡 Issue: Reuse the canonical SPARQL operation classifier instead of adding another parser
What's wrong
The new helper fixes telemetry classification by duplicating SPARQL prologue parsing inside storage. That makes future SPARQL grammar fixes split across two implementations and increases the chance that validation/classification and telemetry drift again.
Example
inferQueryOperation() now has its own loop for leading comments, PREFIX, and BASE, then maps SELECT/ASK/CONSTRUCT/DESCRIBE. That is the same conceptual classifier as core’s SPARQL operation helper, but with a separate regex and separate maintenance surface.
Suggested direction
Delete the local stripping loop/regex and call the shared operation classifier from @origintrail-official/dkg-core. This is a straightforward code-judo move: fewer parser rules, fewer comments, and one source of truth for SPARQL prologue handling.
For Agents
In packages/storage/src/adapters/sparql-http.ts, replace the local prologue parser in inferQueryOperation() with the canonical core classifier, mapping read forms to the slow-query event operation enum and returning unknown otherwise. Preserve the updated PREFIX-heavy slow-query attribution test.
| * UNDEF, nested groups, and malformed tokens return `null`, which makes the | ||
| * caller retain the full allow-list intersection rewrite. | ||
| */ | ||
| function readTopLevelStaticGraphValues( |
There was a problem hiding this comment.
🟡 Issue: Extract the new VALUES parser out of the query engine
What's wrong
This adds another chunk of hand-rolled SPARQL token scanning inside an already oversized query engine. The graph-routing policy is now coupled to low-level parsing details, and the file continues to sprawl instead of gaining a clearer boundary.
Example
The file now has one VALUES scanner for graph-variable allow-list elision and another for min-trust rewriting. A future fix for string, IRI, or tuple VALUES handling has to discover and update both scanners correctly.
Suggested direction
Create a small pure module such as sparql-graph-scope.ts or sparql-values.ts with a typed API, then let DKGQueryEngine call that instead of growing more parser code inline.
For Agents
Move the new static VALUES parsing and the graph-variable constraint policy into a focused SPARQL scope/VALUES utility, or consolidate it with the existing VALUES helper. Preserve the current behavior: skip full allow-list injection only when the caller's static VALUES for the graph variable are all in the allowed set; otherwise keep the intersection rewrite.
There was a problem hiding this comment.
🟡 Issue: The new VALUES parser deepens the query-engine monolith instead of isolating graph-scope rewriting
What's wrong
The feature is implemented by adding another 90+ lines of scanner/parser code into an already sprawling query engine. Even though it reuses some local token helpers, it keeps parser policy, graph-resolution orchestration, result normalization, metadata lookups, and query execution all in one file, which makes the next scoped-query change harder to reason about safely.
Example
Understanding one scoped query rewrite now spans constrainGraphVariablesToAllowedSet, readTopLevelStaticGraphValues, parseStaticGraphValues, collectPrefixDeclarations, readSparqlPrefixName, resolveSparqlPrefixedName, findMatchingCloseBrace, and several token-skipping helpers in the same giant module.
Suggested direction
Extract the graph-variable scoping machinery into a dedicated module, ideally moving both the new helper and its existing neighboring scanner helpers together. That would make the main engine read as orchestration and give the parser/rewrite policy a clear home.
Confidence note
The SPARQL scanner utilities already lived in this monolithic file before the PR, so the finding is scoped to the new readTopLevelStaticGraphValues / parseStaticGraphValues block materially worsening that structure rather than creating it from scratch.
For Agents
In packages/query/src/dkg-query-engine.ts, preserve the behavior that top-level static VALUES ?g inside the allowed graph set avoids full allow-list reinjection. Move the graph-scope rewrite/parser cluster into a focused module such as sparql-graph-scope.ts, with narrow exported functions and the existing tests kept against DKGQueryEngine behavior.
| if (this.closePromise) return this.closePromise; | ||
| const controller = this.closeController; | ||
| controller.abort(new Error('SparqlHttpStore closed')); | ||
| const draining = [...this.inFlight]; |
There was a problem hiding this comment.
🟡 Issue: Make the SPARQL HTTP close lifecycle explicit
What's wrong
The new close path spreads lifecycle state across a mutable controller, an in-flight set, a close promise, and a post-drain reset. That makes the adapter's state hard to reason about and weakens the claim that close() drains all queued/in-flight work.
Example
A caller can start close(), then another path can call query() before close() settles. That query is added after the draining snapshot, while close() later resets the controller. The lifecycle is neither clearly terminal nor clearly generational.
Suggested direction
Serialize work admission against closing, or model close generations explicitly so close() atomically owns the work it promises to drain.
Confidence note
This is a maintainability/lifecycle-clarity concern; the exact runtime impact depends on whether callers are allowed to start work while close() is in progress.
For Agents
In SparqlHttpStore, factor closeController/inFlight/closePromise into an explicit StoreWorkLifecycle helper or make close() terminal. Preserve the intended behavior of aborting and draining existing queued/in-flight work, and add a unit that starts work during close() to pin the intended contract.
The close test does not cover queued work despite the new queued-drain contract
What's wrong
The new shutdown contract says queued and in-flight HTTP work is aborted and drained before close() resolves. The current test only covers an in-flight fetch, so a regression that stops passing the close signal into the scheduler, or lets queued work start after close(), would not be caught.
Example
A test can saturate the external scheduler with background work, queue another SparqlHttpStore.query, call close(), and assert the queued query rejects with SparqlHttpStore closed without any additional fetch call being made.
Suggested direction
Add a queued-work close test in addition to the in-flight fetch cancellation test.
For Agents
Update packages/storage/test/sparql-http.test.ts around the new close test. Reuse the priority-scheduler saturation pattern from the existing SPARQL HTTP priority test, then close the store while one background request is still queued. The test should prove close aborts queued work before admission and resolves after the queued promise settles.
There was a problem hiding this comment.
🟡 Issue: This change pushes the SPARQL HTTP adapter past 1k lines instead of decomposing the new lifecycle concern
What's wrong
The PR crosses the explicit 1k-line smell threshold by adding another responsibility to an adapter that was already near the boundary. The new logic is not just local plumbing; it is a reusable lifecycle policy for aborting and draining scheduled store work, so embedding it in the adapter makes the file more coupled and harder to scan.
Example
The adapter now owns endpoint config, HTTP request construction, update atomicity, graph-list caching, slow-query telemetry, scheduler admission, and close/drain lifecycle in one 1k+ line class. A reader trying to understand shutdown has to reason through closeController, inFlight, closePromise, runStoreWork, postQuery, postUpdate, and close() inside the same large file.
Suggested direction
Move the newly introduced close/in-flight tracking out of the already-large adapter before merging. This PR has a clean boundary available: scheduler/lifecycle tracking is independent of query formatting and HTTP response parsing.
For Agents
In packages/storage/src/adapters/sparql-http.ts, preserve the new behavior that close aborts queued/in-flight work and drains it before resolving. Extract the request lifecycle/draining concern into a focused helper/class, for example AbortableStoreWorkTracker, and keep SparqlHttpStore responsible for SPARQL HTTP semantics. Add/keep the existing close-drain test to prove behavior is unchanged.
There was a problem hiding this comment.
🟡 Issue: Write-side store close cancellation is unverified
What's wrong
The close lifecycle now applies to scheduled store updates as well as queries, but the new tests only exercise query requests. A regression in the postUpdate wiring could leave writes running after close() while all added tests still pass.
Example
A failing-test sketch would start store.insert([...]) with fetch held open, call store.close(), and assert the update fetch receives an aborted signal and close() waits for the update promise to settle. If line 277 accidentally used only the caller signal, the current query-only tests would not catch it.
Suggested direction
Add a write-path close/drain regression test, because shutdown cancellation now affects both reads and updates.
For Agents
Look in packages/storage/test/sparql-http.test.ts. Add one close lifecycle test that drives a write API such as insert through postUpdate, using the existing fetch-stub pattern, and prove close aborts and drains that update before resolving.
| assertionName, | ||
| subGraphName, | ||
| callerAgentAddress, | ||
| signal: queryAbortController.signal, |
There was a problem hiding this comment.
🟡 Issue: The new /api/query cancellation lane is not verified at the route boundary
What's wrong
This PR’s user-facing behavior depends on the daemon route constructing and aborting the exact signal it forwards. A future regression that drops the listener, forgets priority: 'background', or passes a fresh/non-aborting signal would leave the lower-layer tests green while /api/query callers still create orphan normal-lane store work.
Example
A route-level test could stub agent.query to capture opts, keep it pending, emit req aborted or res close, and assert opts.signal.aborted === true, opts.priority === 'background', and opts.source === 'api.query'.
Suggested direction
Add a route-level regression test around the new abort/listener wiring and option forwarding.
For Agents
Look at packages/cli/src/daemon/routes/query.ts and the existing /api/query route tests. Add a focused route or live-daemon test proving all API queries are admitted on the background lane and the forwarded signal aborts when the client disconnects, while preserving normal successful query behavior.
There was a problem hiding this comment.
🔴 Bug: API query cancellation does not cover graph-discovery store work
What's wrong
The new route-level abort controller is meant to cancel orphaned API work and keep external reads in the background lane, but scoped queries perform graph discovery before the changed final query calls. Those discovery reads do not receive the signal or background priority, so disconnected dashboard/plugin requests can still consume normal store capacity and outlive the HTTP caller.
Example
A /api/query request with contextGraphId and includeSharedMemory disconnects while the engine is discovering VM/SWM graph partitions. The final SELECT would be aborted/backgrounded, but the preceding listGraphs/listGraphsByPrefix store work can still queue or run at normal priority and continue after the caller is gone.
Suggested direction
Forward the same cancellation and admission options to every store read performed while planning scoped queries, not only to the final store.query calls.
For Agents
Thread QueryOptions or a narrowed store-options object through discoverGraphsByPrefix, discoverContextGraphPerCgIdDataGraphs, discoverRegisteredSubGraphNames, and the helper listGraphsByPrefix, preserving existing graph filtering. Add a route/engine test with a recording store proving graph discovery receives signal, priority: 'background', and source, and aborts before the final query when the signal fires.
/api/query disconnect cancellation is not verified at the route boundary
What's wrong
The only code that turns an HTTP disconnect into an AbortSignal lives in the daemon route, but the added tests exercise only downstream option forwarding. That leaves the user-facing behavior introduced here unverified.
Example
A regression that removes signal: queryAbortController.signal or stops wiring res.once('close', ...) would still leave the agent/query/storage forwarding tests green, while a disconnected /api/query caller could keep store work alive.
Suggested direction
Add a route-level regression test for the new HTTP disconnect and background-lane contract.
For Agents
Add coverage around handleQueryRoutes in packages/cli/test/daemon/routes/query.test.ts or a focused route test: capture agent.query options, emit req.aborted or res.close while the query is pending, and assert priority === 'background', source === 'api.query', and the passed signal aborts; also assert the normal success path still returns 200.
There was a problem hiding this comment.
🟡 Issue: Route-level query admission behavior is not exercised
What's wrong
The user-facing behavior changed at the /api/query boundary, but the added tests validate the pieces in isolation. That leaves the most fragile part, the wiring between the HTTP route and agent.query, without a regression test.
Example
A regression that removes priority: queryLifecycle.priority or signal: queryLifecycle.signal from the agent.query(...) call would still leave query-route-lifecycle.test.ts and the agent/query-engine propagation tests green, because none of them execute this route handoff.
Suggested direction
Cover the route boundary itself, not only the helper functions and downstream query engine propagation.
For Agents
Add a focused /api/query route test around handleQueryRoutes or a small in-process HTTP server. Stub agent.query only to capture its received options and to throw a real StoreSchedulerBusyError; prove the route passes signal, priority: 'background', source: 'api.query', and returns 503 with Retry-After for that error.
| : wrapWithGraph(sparql, sharedMemoryGraph); | ||
| const dataResult = await this.store.query(dataSparql); | ||
| const smResult = await this.store.query(sharedMemorySparql); | ||
| const dataResult = await this.store.query(dataSparql, storeOptions(options)); |
There was a problem hiding this comment.
🟡 Issue: Multi-store query option propagation is only partially covered
What's wrong
Cancellation and background admission are most important on long or multi-part reads, but the added test only verifies a single final store query. That can give false confidence if one branch of a multi-query execution path stops forwarding the new store options.
Example
A regression that leaves dataResult = await this.store.query(dataSparql) without options but keeps options on smResult would still pass the current at(-1) assertion, while one of the store requests would ignore cancellation and priority.
Suggested direction
Extend the propagation test to cover a branch with multiple store calls and assert all captured options, not just the last one.
Confidence note
This is a verification gap rather than proof of a current runtime failure; the production diff does appear to thread options through these branches.
For Agents
In packages/query/test/query-engine.test.ts, add a case for a path that issues multiple store queries, such as includeSharedMemory, and assert every recorded store query receives the same signal, priority, and source. Preserve existing query results while proving no branch silently drops store options.
There was a problem hiding this comment.
🟡 Issue: Store-read options are scattered through the query engine instead of owned once
What's wrong
This change turns a cross-cutting store-admission concern into repeated manual plumbing across an already large query engine. It increases the amount of code a reader has to audit and makes future omissions easy because the type system does not force store reads to carry the derived options.
Example
A future helper added inside DKGQueryEngine can call this.store.query(...) or discoverGraphsByPrefix(...) and compile cleanly while silently dropping API cancellation, priority, and source attribution. The current implementation makes that invariant depend on every call site remembering the same option conversion.
Suggested direction
Compute the store read options once at the query boundary and pass a small execution context or use bound helper methods for store reads and graph discovery. That would remove the repeated storeOptions(options) calls and make cancellation/priority/source propagation a local invariant instead of ambient plumbing.
For Agents
In packages/query/src/dkg-query-engine.ts, preserve the current query behavior and option values, but introduce a per-query execution context or bound store-read helper computed once near query(...) entry. Route all store reads/discovery through that helper, with the special shared-discovery variant carried explicitly. Existing option-propagation tests should still pass and should cover at least one newly centralized path.
| }, | ||
| ); | ||
|
|
||
| expect(recordingStore.queryOptions.at(-1)).toMatchObject({ |
There was a problem hiding this comment.
🟡 Issue: Option-forwarding coverage misses graph-discovery reads
What's wrong
The change is meant to route API query work through cancellation and background scheduling, but this test only proves the final store query receives those options. Discovery reads are part of the same query path and can be the expensive queued work this change is trying to control.
Example
A test double that records listGraphs()/listGraphsByPrefix() during a contextGraphId query would catch discovery reads running without { signal, priority: 'background', source: 'api.query' }; the current test would not.
Suggested direction
Broaden the forwarding test so it proves all store reads in a scoped query, not just the terminal SPARQL query, inherit the new options.
For Agents
Extend the query-engine option-forwarding coverage in packages/query/test/query-engine.test.ts with a store that records graph-discovery read options, then run a scoped query that requires VM/SWM graph discovery and assert every store read gets the cancellation/lane/source options.
| expect(events[0]).not.toHaveProperty('sparql'); | ||
| }); | ||
|
|
||
| it('close aborts and drains queued/in-flight HTTP work before resolving', async () => { |
There was a problem hiding this comment.
🟡 Issue: SparqlHttpStore close test does not actually cover queued work
What's wrong
The new test gives coverage for an in-flight fetch observing close cancellation, but it never places a store operation in the scheduler queue. The queued half of the new close contract could regress or hang without this test failing.
Example
Saturate the background lanes with held HTTP requests, enqueue one more background query that has not reached fetch, call close(), and assert close() resolves promptly while the queued promise rejects with SparqlHttpStore closed. A regression that only aborts in-flight fetches would pass the current test.
Suggested direction
Add a queued-work case alongside the in-flight close test, or split the test title so the queue contract has its own assertion.
For Agents
Update packages/storage/test/sparql-http.test.ts near the new close test. Reuse the existing priority-saturation pattern in that file, then verify close aborts both an already-started fetch and a queued scheduler entry before returning.
Multi-lens review —
|
| scenario (real scheduler, prod defaults) | pre-PR (normal) |
post-PR (background) |
|---|---|---|
| 500 ms API read behind two 6 s background jobs | served @557 ms | REJECTED @10.05 s |
| node-UI 3-layer render, 6 s/layer | wm/swm/vm all ok | vm REJECTED @10.01 s |
| 20-read burst | 10 shed | 15 shed |
| 6 agent-bg jobs + 8 API reads | 5 ok / 3 shed | 0 ok / 8 shed |
Because normalFloor=1 is untakeable by background work, a short interactive read always got a slot before; now it queues behind whatever durability work owns the single background slot.
Two corrections that keep this at HIGH and not CRITICAL — both forced by skeptics, both verified:
- The 1-slot ceiling is pre-existing.
store-priority-scheduler.tsis not in this diff. What the PR introduces is admitting unbounded, user-triggered work into that FIFO. - "Before, everything completed" holds only at exactly one concurrent slow read. With two slow reads on base, the same evictions already occur. The PR lowers the trigger from 2 → 1. A real regression, but a threshold shift rather than a new failure mode. And
StoreSchedulerBusyError.retryable = truewith periodic sync rounds, so the result is a failed-and-retried round plus error noise — not permanently unpersisted peer data, and criterion the context graph explodes even after 4-5 rounds in the game #3 is not negated.
Equally important, the other direction is real: on base, three slow API reads starve promotion/gossip on the normal lane; post-PR all three complete. The PR buys genuine normal-lane relief. This is a trade, not a pure regression.
flowchart TB
subgraph BEFORE["Before — api.query on the normal lane"]
B1["POST /api/query"] --> B2{"normal lane<br/>floor 1, ceiling 2"}
B2 -->|"floor untakeable by background"| B3["admitted, ~1 ms"]
BGA["durable sync / SWM catch-up<br/>materialization / changelog"] --> B4{"background<br/>ceiling 1"}
end
subgraph AFTER["After — api.query on the background lane"]
A1["POST /api/query"] --> A2{"background lane<br/>ceiling 1, single FIFO"}
BGB["durable sync / SWM catch-up<br/>materialization / changelog"] --> A2
A2 -->|"slot held > 10 s"| A3["StoreSchedulerBusyError<br/>queue_wait_timeout"]
A3 --> A4["HTTP 500"]
A2 -->|"slot free"| A5["admitted"]
end
On the fix — I have to retract the obvious remedies, because I executed them and they fail:
-
"Add a dedicated— arithmetically impossible at defaults. Ordinary capacity is 2;apilane"normalFloorandbackgroundFlooralready claim one each, so a fifth lane clamps to ceiling 1. It also isn't a two-file change:storeWorkPriorityRankends in a catch-allreturn 3, the inflight counters end inelse … backgroundInflight, andcanStartbranches onpriority === 'background'— adding a member without editing all four silently counts API work in the background counters, with no type error. -
"Raise— the knob is inert.DKG_STORE_BACKGROUND_RESERVED_SLOTS"backgroundLimit = max(1, totalLimit − normalFloor)never reads the requested background floor. I ran it:defaults peak background concurrency = 1 backgroundReservedSlots=2 peak background concurrency = 1 backgroundReservedSlots=4 peak background concurrency = 1 backgroundReservedSlots=8 peak background concurrency = 1 maxConcurrent=5 peak background concurrency = 2 normalReservedSlots=0 peak background concurrency = 2Only
maxConcurrentmoves it — andstore-priority-scheduler.ts:94calls 4 "the incident-tested stable ceiling on an 8 GiB host", so widening re-opens the store pressure it was tuned to prevent.normalReservedSlots=0deletes the guarantee this PR exists to defend. -
"Keep— a cap ≥ 2 re-opens mainnet: rs.loop.tick-threw from Store scheduler queue wait timeout in blazegraph.query #1989; a cap of 1 reproduces today's serialization.normal+ per-source cap"
What I'd actually do: land P2 (below) in this PR so shedding is at least signalled as retryable, and treat the lane choice as an explicit, reversible decision — gate priority on an env var (DKG_API_QUERY_PRIORITY, default background) so the canary run can flip it without a redeploy. Right now the riskiest behavioural change in the PR has no runtime escape hatch: the literal is hard-coded and the only tuning knob that works is the one the code comment warns against.
P2 · MEDIUM — retryable overload reaches API callers as HTTP 500
StoreSchedulerBusyError carries code = 'STORE_SCHEDULER_BUSY' and retryable = true, but its message matches none of the 400 patterns at routes/query.ts:639-665, so it rethrows to respondWithDaemonError, which has no case for it and falls into the generic 500 (http-utils.ts:106-109). Reproduced end-to-end over a real socket with the compiled route, the real handler and a real scheduler error:
queue_wait_timeout → HTTP 500 {"error":"Store scheduler queue wait timeout (background: api.query)"}
queue_full → HTTP 500 {"error":"Store scheduler queue full (background: api.query)"}
The mapping is pre-existing — this is P1's blast radius, measured: with 6 saturating background workers, pre-PR normal lane shed 0/30, post-PR background lane shed 19/30. Routine admission shedding is now routinely reported as a server fault.
Direction: a respondIfStoreSchedulerBusy() helper keyed on code, alongside the existing respondIfChainRpcTransportError — 503 + Retry-After, { code, retryable: true }. It's safe to down-classify because the error is thrown only pre-start, so nothing is reclassified after a side effect. Two caveats from the skeptics: respondWithDaemonError is shared, so write routes that rethrow (knowledge-assets.ts:1454/1596, context-graph.ts:652, epcis.ts:463) also flip — retryable: true overpromises there for a partly-done multi-step write; and /api/genui/render (query.ts:736) hard-codes 500 for the same store call, so it needs the helper too. Don't also map disconnects to 499 — res.destroyed is already true and bytesWritten = 0, so nobody receives it.
P3 · MEDIUM — only the final read is background/cancellable, and the obvious fix is dangerous
storeOptions(options) reaches execAndNormalize and the two shared-memory calls, but the discovery reads the same request performs first are still options-less: dkg-query-engine.ts:763, 804, 966, 988, 1008. Measured on the dashboard shape:
[0..5] priority=undefined signal=NO :: listGraphs ×3, two _meta lookups, one GRAPH ?g scan
[6] priority=background signal=yes :: the user query
I initially had this as HIGH and that was wrong — five skeptics refuted the classification and I agree. At base, all store calls were options-less including the terminal one; head takes the dashboard path from 14/14 normal-lane + 0 cancellable to 11/14 + 3 cancellable. Normal-lane pressure and orphan work both strictly decrease. This is an incomplete fix of the PR's own goal, not a regression.
This is where I disagree with the review bot's first comment. Its remedy — thread storeOptions(options) through discoverRegisteredSubGraphNames / discoverRegisteredAssertionGraphs / discoverKnownChildContextGraphUris etc. — is not safe as written, and four skeptics reproduced why independently:
resolveScopedContentGraphAllowList memoises one in-flight promise keyed only by (cgId, subGraphName) (dkg-query-engine.ts:279/915-931). Threading the caller's AbortSignal into a shared flight means one caller's disconnect aborts every other concurrent caller on the same CG:
baseline (head): caller A disconnects → B, C succeed 0/2 collateral
with the fix: caller A disconnects → B, C fail with 2/2 collateral
"API query caller disconnected"
It also poisons signal-less internal reads that join the same map — an agent.durable-sync listGraphs joining an API-seeded flight dies, i.e. the exact starvation-victim class this PR is protecting. And priority contaminates the same way: an internal caller asking for normal gets run at background.
The repo already knows this hazard: sparql-http.ts:660-665 deliberately narrows a shared refresh to { source } only, dropping signal and priority.
Correct direction: thread priority and source only into shared/memoised flights; never the caller's signal. Per-caller signals belong only on reads that are not shared. Note the list is also incomplete — discoverGraphsByPrefix (:762), the view path's direct call (:560), hasGraph (:1338) and listGraphs are unlisted, and the helpers don't take an options parameter at all.
P4 · LOW — the close-drain test passes with the drain deleted
sparql-http.test.ts:464: expect(cancellationSettled).toBe(true) sits after await rejected. The mocked fetch sets that flag synchronously immediately before rejecting, so await rejected alone guarantees it. Three skeptics independently mutation-tested this — replacing await Promise.allSettled(draining) with void draining at sparql-http.ts:773 leaves 32/32 green. The behaviour backing acceptance criterion #4 ("no slow-query completion after Stopped.") is unguarded.
The production drain is correct — this is coverage only, hence LOW. Fix, verified in both directions (clean passes, mutant fails, 40/40 deterministic):
await closingStore.close();
expect(cancellationSettled).toBe(true); // move ahead of the await
await rejected; // keep, so the rejection is consumedP5 · LOW — every client disconnect is now recorded as a failed operation
The new abort wiring makes agent.query reject with Error('API query caller disconnected'), which takes the catch at routes/query.ts:636 → tracker.fail(ctx, err) → a failed-operation row in the node-UI Operations feed, then a 500 written into an already-destroyed socket. Pre-PR a disconnect let the query finish and be recorded as a success. Navigating away from the Explore tab, a plugin hitting its own timeout, or Ctrl-C on dkg query now all look like node failures — and this noise masks the genuine P1/P2 500s. Telemetry only (no throw, no unhandled rejection observed). Tag the abort with a typed code at the throw site (query.ts:590 currently throws a bare Error, detectable only by message-sniffing) and treat it as cancelled rather than failed.
P6 · nit — the comment on the security-critical branch says 249, measurement says 128
dkg-query-engine.ts:1554: "DKG added all 249 known VM graphs". 249 is the metadata-binding count of the preceding discovery query; the injected table is 128 (125 materialized + root + _meta + _shared_memory_meta) — which is what the PR body, the new test's comment and my measurement all say. This comment is the sole in-source rationale for skipping an authorization constraint, so the number should be right.
Also worth a decision (not a defect): only 1 of ~12 daemon agent.query call sites got the lane and signal — genui (query.ts:719/765), context-graph.ts:906, epcis.ts:446 and the metrics path stay on normal. Your Review-focus bullet 3 asks exactly this; my answer is that consistency matters less than P1, so I'd settle P1 first.
Claims that did not survive — please don't re-raise
"Empty— I filed this and it's dead. A skeptic ran the pinned mainnet image (VALUES ?g { }skips injection and could leak on Blazegraph"lyrasis/blazegraph:2.1.5@sha256:56be5edb…, the exact digest inci.yml:461) with a cross-CG secret loaded: baseline unconstrainedGRAPH ?greturns the secret (so the probe is sensitive), emptyVALUESreturns 0 rows on both Blazegraph and Oxigraph 0.5.5. And pre-PR the same function already emitted a bareVALUES ?g { }whenallowedGraphswas empty — the dependency predates the PR. No leak, no change."The still-injecting path (no caller VALUES) is an ×84 amplification this PR leaves in place"— executed base vs head on every such shape: byte-identical (9,020 B / 128 entries both). The PR can only shrink injection. Worse, the proposed remedies are security regressions:wrapWithProjectedGraphSubselectreturns the caller query verbatim whenhasGraphClause(always true here) = zero allow-list enforcement, and aSTRSTARTS(cg-root)prefix test over-admits 8 of 9 probes including_private. A transcript confirms removing the injection leaks aFOREIGN-SECRETbinding from another context graph. The injected table is the ACL boundary on that path."Add a cardinality cap to— an allow-list cannot be capped and remain an allow-list. Truncation silently drops the user's own graphs (and graph enumeration is nondeterministic, so different runs return different subsets); throwing makes any mature CG unqueryable viaconstrainGraphVariablesToAllowedSet"GRAPH ?g. Also,expect(executed).toBe(sparql)is already the tightest possible ceiling and has a genuine fail-before."The route change has zero test coverage / the PR body cites an unrelated file"— grep artefact. The literals are asserted inquery-engine.test.ts:117-147andquery-min-trust-alias.test.ts:57-73, both added by this commit; andcli/test/daemon/routes/query.test.tsis the route's own suite (real daemon, no mocks) whose 200 case traverses the new AbortController path. The residual — the:611constant isn't pinned at route level — is LOW, and a stub-agent route test would reinstate exactly the pattern68d5bb793deliberately removed."Blazegraph adapter parity is missing"— false.blazegraph.ts:181-194composesoptions?.signaland forwardsoptions?.priority. The lane change applies on mainnet too (which is part of why P1 matters)."Merging drags the 10.0.10 release commits into canary"— I raised this early and it's wrong: chore: sync main into testnet-canary after 10.0.10 #1986 already squash-applied that content, canary is 10.0.10, andgit merge-treeis clean.- CHANGELOG omission — not a finding; entries are authored in
chore(release)commits, not per fix PR.
Merge readiness
Not blocking. The parser is fail-closed, the incident shape is fixed byte-exactly, and the cancellation/drain work is sound. P4 and P6 are one-line changes. P3 is a real gap but strictly better than base, so it can be a follow-up — provided the fix threads priority/source only.
The one thing I would not merge silently is P1. It's a deliberate trade — normal-lane relief bought with external-read availability — and right now it ships hard-coded with no working knob and renders as HTTP 500. My recommendation: land P2's 503 mapping in this PR and put the priority behind an env-tunable default, then run the canary acceptance list. That turns an irreversible decision into a measurable one, which is what criteria 1–4 actually need.
Method: 5 lenses → 21 candidates → 10 verified by 3 adversarial skeptics each (30 verdicts). Findings were settled by executing the real modules — StorePriorityScheduler, DKGQueryEngine + OxigraphStore, the compiled route behind a real socket, and a pinned Blazegraph 2.1.5 container — rather than by reading. Every table and transcript above is a run, not an estimate; I re-executed the scheduler arithmetic, the inert-knob matrix and the 500 mapping myself before posting.
| error_message: string; | ||
| }): void { | ||
| this.stmt('cancelOp', ` | ||
| UPDATE operations SET status = 'cancelled', duration_ms = @duration_ms, |
There was a problem hiding this comment.
🟡 Issue: Cancelled operations are counted as unsuccessful in dashboard rates
What's wrong
The PR introduces cancelled as a non-failure state for API caller disconnects, but the existing dashboard aggregate logic still treats every operation row as part of the success-rate denominator. Because cancelled rows are neither counted as success nor error, normal caller disconnects will depress success rates and mislead operators about node health.
Example
If 9 operations succeed and 1 /api/query caller disconnects, cancelOperation records that row as cancelled; getOperationStats() reports totalCount = 10, successCount = 9, errorCount = 0, and successRate = 0.9. That makes caller disconnects look like degraded node success even though the change intentionally separates them from failures.
Suggested direction
Update dashboard aggregation semantics for the new cancelled terminal state so client disconnects do not silently reduce success rates without appearing as errors.
For Agents
Look in packages/node-ui/src/db.ts stats methods such as getOperationStats, getOperationStatsByType, getOperationTrends, and getSuccessRatesByType. Preserve cancelled visibility in the operations list, but either exclude cancelled rows from success/error health denominators or surface a separate cancelled count/rate. Add a DB-level test with success, error, and cancelled rows proving the intended denominator.
| } | ||
|
|
||
| /** Map retryable, pre-dispatch read shedding without changing write routes. */ | ||
| export function respondIfApiQueryStoreBusy(res: ServerResponse, err: unknown): boolean { |
There was a problem hiding this comment.
🟡 Issue: Scheduler-busy mapping depends on a stringly typed error shape
What's wrong
The route boundary now knows about storage overload errors through duplicated magic strings and ad-hoc object casts. That is a brittle abstraction boundary: the producer owns the contract, but the consumer has no typed link to it.
Example
If the storage layer changes the busy-error code or adds a new busy reason/priority shape, this route still compiles because it only checks a stringly typed object. The HTTP mapping then either stops firing or emits loosely shaped response fields without a type boundary catching the drift.
Suggested direction
Move the narrowing contract to the storage package or import the canonical error class/constant directly. The route should consume a typed predicate/result rather than reconstructing the storage error shape with local constants.
For Agents
In packages/cli/src/daemon/routes/query.ts, replace the local string/record duck typing with a canonical storage-layer predicate or class check, such as an exported isStoreSchedulerBusyError. Preserve the 503 status, Retry-After: 1, and response payload shape for scheduler shedding.
| return values; | ||
| } | ||
|
|
||
| function findMatchingCloseBrace(sparql: string, openIdx: number): number { |
There was a problem hiding this comment.
🟡 Issue: The new graph-scope helper duplicates the SPARQL scanner instead of sharing it
What's wrong
The extraction improves file locality for graph-values authorization, but it creates a second hand-rolled SPARQL scanner beside the one already used by the query engine. That makes parser maintenance harder and invites semantic drift in code whose correctness depends on small lexical details.
Example
A scanner fix for one SPARQL edge shape, such as IRI/comparison disambiguation or Unicode variable handling, now has to be applied in two places. The copies are already not identical: the existing query-engine variable reader walks code points, while the new module uses a regex over indexed string characters.
Suggested direction
Centralize the low-level SPARQL scanning functions and keep higher-level graph-scope logic in the new module. That keeps the useful extraction while avoiding two subtly different parsers in the same package.
For Agents
Extract the shared scanner primitives into packages/query/src/sparql-utils.ts or a dedicated sparql-scanner.ts, then import them from both dkg-query-engine.ts and sparql-graph-scope.ts. Preserve the current graph-values elision behavior and existing SPARQL rewrite behavior with the current tests.
| } | ||
| } | ||
|
|
||
| cancel(ctx: OperationContext, reason: unknown): void { |
There was a problem hiding this comment.
🔵 Nit: Cancellation tracking copy-pastes the failure terminal flow
Why it matters
The current implementation is understandable, but it duplicates a state-transition pattern that is likely to keep growing as operation statuses become more nuanced.
Suggestion
Factor the shared terminal-operation path into a small helper, for example finish(ctx, status, reason) in OperationTracker and a status-parametrized terminal update in DashboardDB. Keep the public fail and cancel methods as thin named entry points if that reads better.
Round 2 — re-review at
|
| Round 1 | Status at c50b39235 |
|---|---|
| P1 lane hard-coded, no escape hatch | resolveApiQueryPriority() + DKG_API_QUERY_PRIORITY, documented in README with the 503 contract |
| P2 shed → HTTP 500 | respondIfApiQueryStoreBusy → 503 + Retry-After, route-scoped so write routes are untouched, applied to genui too |
| P3 caller signal into shared flight | sharedDiscoveryStoreOptions strips signal, keeps priority/source; priority+source in the memo key (fixes the inversion half); raceAgainstCallerAbort per caller |
| P4 vacuous close-drain test | assertion moved before await rejected — and mutation-testing confirms it now kills the no-drain mutant |
| P5 disconnect → failed op + 500 | typed ApiQueryCallerDisconnectedError → tracker.cancel |
| P6 "249" comment | now "the complete 128-graph allow-list" |
The ACL parser survived extraction into sparql-graph-scope.ts. This was my main worry and it checks out: a 28-case adversarial fail-closed matrix is byte-identical to round 1 and all cases still fail closed; a 118-entry corpus covering the explicit-GRAPH-IRI path gave mismatches=0 against the round-1 engine materialised side by side; the #1989 positive path still executes byte-identically. The extracted predicate is also provably equivalent — !(v !== null && every(has)) ≡ v === null || some(!has). Mutation M1 (drop the allow-list check) turns query-engine.test.ts:1689 red with a real foreign binding, so the fail-closed test has a genuine fail-before. Suites: query 327/327, storage 34/34, agent 4/4, cli 4/4, node-ui 17/17, tsc --noEmit clean.
Verification caveat, stated up front: round 1 put every finding through three adversarial skeptics. In round 2 my verification workflow lost its final serialization step, so the findings below did not get that skeptic pass. I independently confirmed R1's mechanism by reading the code myself; R2 and R3 rest on benchmark/soak transcripts I did not re-run. Weight them accordingly — R1 I'd act on, R2/R3 I'd reproduce before changing anything.
R1 · HIGH — the caller signal now leaks into GraphSetIndexStore's shared refresh flight
This is the same bug class as P3, one layer further down. The engine-level shared flight is now correctly signal-stripped, but round 2 newly threads the full per-caller options — including the HTTP-disconnect signal — into listGraphsByPrefix / listGraphs / hasGraph from nine discovery sites (:355, 363, 490, 499, 513, 522, 601, 612, 639 → :840 → :1448-1464). Round 1 passed no options there, so those calls were uncancellable and therefore safe.
In production those land in GraphSetIndexStore.ensureGraphSet, served from a process-wide RefreshCoordinator. I read that coordinator myself: run(priority, start) looks up bestReusableFlight(priority) and returns the existing flight — keyed by priority alone. The flight's closure captured the first caller's options, so it scans with that caller's signal. Since every API read is on one lane by default, concurrent API callers all land in the same bucket, which makes this the common case rather than a corner one.
Reproduced end-to-end against the real engine + real GraphSetIndexStore, with a counterfactual in the same run:
E2E CALLER A -> rejected: API query caller disconnected
E2E CALLER B -> rejected: API query caller disconnected ← collateral
E2E listGraphs option signals -> [ 'HAS-SIGNAL' ]
R1-threading (no options, as at 1a3d86885):
CALLER A -> rejected: API query caller disconnected
CALLER B -> resolved ← survives
listGraphs option signals -> [ 'no-signal' ]
Warm-index variant (revalidateMs: 0): WARM CALLER B -> rejected
On the warm-revalidate path this also bypasses the graceful "keep the last known graph set" fallback, so B fails despite a healthy cached set. The window reopens every DEFAULT_GRAPH_SET_REVALIDATE_MS = 30 s.
Your new test can't catch it — keeps caller cancellation out of shared graph-discovery flights uses a bare OxigraphStore, which has no shared refresh flight at all.
flowchart LR
A["/api/query caller A<br/>signal A"] --> D["discoverGraphsByPrefix :840<br/>passes FULL options"]
B["/api/query caller B<br/>signal B"] --> D
D --> L["listGraphsByPrefix :1448"]
L --> R{"RefreshCoordinator.run<br/>keyed by PRIORITY only"}
R -->|"B joins A's flight"| S["shared scan<br/>runs with signal A"]
A -.->|"client hangs up"| X["abort"]
X --> S
S -->|"flight rejects"| Z["BOTH A and B fail"]
Fix — the pattern you already wrote one layer up: pass sharedDiscoveryStoreOptions(options) to listGraphsByPrefix/listGraphs/hasGraph in discoverGraphsByPrefix (:840) and listGraphFamily (:1463-1464), then wrap in raceAgainstCallerAbort(..., options?.signal) so the caller still cancels promptly without killing work others joined. Then extend the shared-flight test to run against a GraphSetIndexStore-wrapped store so the whole class is covered, not this instance.
R2 · HIGH — the canonical-classifier swap put a full-query scan on every read's hot path
This one is on me — I endorsed reusing classifySparqlOperation instead of the hand-rolled prologue regex. The reuse is right; doing it eagerly is the problem. sparql-http.ts:534 calls it unconditionally on every query(), and it runs stripSparqlLiteralsAndComments — a per-character loop with 1–2 RegExp.test() per char plus an Array(n)+join over the whole query — then a second full-string scan. The result is then discarded at :697-700 unless the query is both over the slow threshold and sampled.
Cost scales with query bytes, i.e. exactly the bytes this PR's own VALUES scoping adds:
| query bytes | old µs/op | new µs/op | slowdown |
|---|---|---|---|
| 2,733 | 3.52 | 99.3 | 28× |
| 13,283 | 11.90 | 291.6 | 25× |
| 26,533 | 25.45 | 866.5 | 34× |
| 53,033 | 46.54 | 1,429.9 | 31× |
A 26.5 KB scoped read — the shape of #1989's own fingerprint — now burns ~870 µs of synchronous event-loop time per store query to compute a label that is thrown away, and the engine issues many store reads per API request. That works against the CPU pressure this PR exists to relieve.
Fix: make it lazy. Drop const operation = inferQueryOperation(trimmed) at :534 and compute it inside maybeEmitSlowQuery after the threshold+sample gates, right next to hashQuery(), which is already gated exactly that way.
R3 · HIGH — AbortSignal.any against a process-lifetime signal retains ~176 B per store op
abortable-store-work-lifecycle.ts:85 composes each caller signal with generation.controller.signal and never unlinks. The generation controller is created once and replaced only by close(), so it lives for the process; on Node 22 this takes the AbortSignal.any branch, and any([shortLived, processLifetime]) permanently retains ~176 B that GC cannot reclaim. Round 2's own P3 fix multiplies this by threading caller signals into ~20 store call sites per API query.
Soak against the shipped module with --expose-gc and an explicit global.gc() between rounds: heapUsed grew monotonically 24.5 → 175.7 MB over 1 M ops, a linear 16.8 MB/100k, with dependants=0 and inFlight=0 (so it is not the dependant-signal set). Controls isolate it cleanly: plain AbortController 0 slope; any([fresh, fresh]) 0 slope; any([fresh, long-lived]) 176 B/op, not reclaimed even after aborting the long-lived signal. With callerSignal undefined (compose returns the generation signal directly) the slope is 0.
Base had no closeController at all, so this is introduced here. Fix: have composeAbortSignals return { signal, dispose } using the manual controller path rather than AbortSignal.any, and call dispose() from the void task.finally(...) already present in run() at :88-90. Measured to bring growth to exactly 0 B/op.
R4 · MEDIUM — the disconnect branch returns without ending the response
query.ts:710: if (err?.code === API_QUERY_CALLER_DISCONNECTED) { tracker.cancel(ctx, err); return; } leaves res.writableEnded === false. handle-request.ts:392 gates chain termination on exactly if (res.writableEnded) return;, so the abandoned request continues through the local-agents, epcis, pca, wallet, notification and plugin route handlers before a 404 is written to a dead socket. handlePluginRoutes loops over every operator-configured routePlugins and invokes plugin.handle() for a request whose client is gone. At round-1 head this error hit throw err → respondWithDaemonError → response ended → chain stopped.
[disconnect] tracker calls = ["start","startPhase","completePhase","startPhase","cancel"]
[disconnect] res.writableEnded = false headersSent = false status = 0
[busy] status = 503 headers = {"Content-Type":"application/json","Retry-After":"1"} ← busy path correctly ends
Bounded to MEDIUM because writeHead+end on an aborted response doesn't throw on v22.22. Fix: tracker.cancel(ctx, err); if (!res.writableEnded) res.end(); return;
R5 · MEDIUM — the route wiring can be reverted with the whole suite green
query-route-lifecycle.test.ts tests resolveApiQueryPriority / createApiQueryRequestLifecycle / respondIfApiQueryStoreBusy in isolation. Nothing asserts that handleQueryRoutes actually passes queryLifecycle.priority/.signal/.source into agent.query, or that the catch routes a disconnect to tracker.cancel. Mutation-proven: reverting the route only (signal → undefined, priority → 'normal', disconnect branch deleted) leaves 4/4 passing. A control mutation confirms the helper tests themselves are not vacuous.
Since P1/P3/P5 were all route-wiring bugs, the regression guard for the exact bugs being fixed is the one thing missing. And my round-1 caution about the retired stub pattern doesn't block you here: the new file uses no vi.mock/vi.fn, and as unknown as ServerResponse stubs already exist in 12 other packages/cli/test files — test/publisher-job-by-intent-route.test.ts is the precedent to copy.
LOW
cancellednever renders distinctly (Operations.tsx:27).STATUS_COLORShas exactly one occurrence repo-wide — its own declaration — so the added entry is dead. The live renderer isStatusBadge:1654, which has nocancelledarm and falls tobadge-warning, a class that doesn't exist (the defined one is.badge-warn). Phase details also render only whenstatus === 'error', so the stored disconnect reason is never shown. Net: P5's UX half isn't visible yet.- Shed recorded as a failed operation (
query.ts:712).tracker.failruns beforerespondIfApiQueryStoreBusy, so admission shedding is persisted asstatus='error'and counted bygetSuccessRatesByType. That contradicts the rationale for addingcancelledin the same commit. Hoist the busy check abovetracker.fail, or document the choice. DKG_API_QUERY_PRIORITYhas no boot validation or log (query.ts:358). Only the exact stringnormalflips the lane; it's read per request, never at boot. An operator who sets=noramlor=highmid-incident gets no error and no confirmation, and debugs the wrong layer while reads keep shedding. The safe-fallback direction is right and matches repo convention — the missing piece is resolving once at boot and logging the effective lane.sparql-graph-scope.tsforks eight lexer primitives thatdkg-query-engine.tsstill defines, andreadSparqlVariablehas already diverged (UTF-16-code-unit class vs the engine's code-point PN_CHARS_U grammar). Two lenses found this independently. No reachable fail-open — 400k random, 300k structured and 500k adversarial cases produced zero fail-open divergences, and the 399 permissiveness differences were all correct elisions round 1 missed — but this is two ACL-relevant parsers drifting inside one commit. Move them tosparql-utils.tsnext toskipSparqlStringLiteraland import in both.- The guard-order invariant is unpinned.
assertGraphVariablesAreTopLevelcorrectly runs before the elision decision, but moving both asserts belowif (variablesNeedingConstraint.length === 0) return sparql;leaves 327/327 green. Under that inversion a nestedGRAPH ?greturns rows instead of throwing. Two small tests — an authorized top-levelVALUESplus (a) a bare default-graph pattern and (b)GRAPH ?ginsideOPTIONAL— are red under the inversion and green today.
Also verified correct (so nobody re-raises)
db.tsis not a migration risk:statusis bareTEXT DEFAULT 'in_progress'with noCHECK, the delta has noCREATE/ALTER/user_versionlines, and the status filter isn't allow-listed server-side — existing databases are unaffected.jsonResponsemergesextraHeadersover Content-Type+CORS without clobbering; the 503 body carriescode/reason/priority/retryable.- The custom abort
codesurvives the whole chain — Node 22fetchrejects with the abort reason intact,composeAbortSignalspropagatesprimary.reason, and neitheragent.querynor the engine rewrap store errors. close()semantics are pinned: mutations dropping the abort, the drain, or the fresh-generation reinstall are all killed by the suite. Theclosingguard mutation survives but is provably not a defect (the scheduler throws synchronously on a pre-aborted signal, and the guard/start/inFlight.addare one synchronous turn).- The unbounded drain is bounded outside: shutdown races cleanup against
SHUTDOWN_HARD_TIMEOUT_MS = 15 s, so a stuck drain degrades a clean exit to a forced one rather than hanging. Correctly not filed. - Memo-key fragmentation is a non-issue: only one production producer of
sourcereaches the engine, and priority is a per-process constant, so the key yields ~2 buckets per(cg, subGraph). raceAgainstCallerAbortleaks nothing — shared flight rejecting with every caller gone produced zero unhandled rejections and the map entry is cleaned.
Merge readiness
Closer, but I'd fix R1 before merge. It's a live cross-caller failure on the default configuration, it's the same class as P3 rather than a new one, and the fix is the pattern already written twenty lines away. R4 is cheap and worth taking with it.
R2 and R3 are both real and both introduced here, but they're performance/heap rather than correctness, and I'd want them reproduced independently before you change code on my say-so — particularly R2, which exists because I recommended the classifier reuse in round 1.
R5 is the one I'd insist on as a condition of merge rather than a follow-up: three of the six round-1 findings were route-wiring bugs, and right now that wiring can be reverted wholesale with a green suite.
Method: 4 lenses over the round-2 delta (ACL-refactor re-proof, storage lifecycle, route + node-UI, engine regression sweep), each executing the real modules and mutation-testing the new tests with proven restores. Round-1's three-skeptic pass did not run this round — see the caveat above. I re-verified R1's mechanism and all six round-1 fixes myself against the worktree at c50b39235.
Addendum to the round-2 review — R2 and R3 mechanisms now confirmedMy round-2 comment carried a blanket caveat that R2 and R3 came from agent transcripts without an adversarial pass, and said to reproduce them before changing anything. That was honest at the time but it is now too weak, and I don't want three HIGHs deprioritised on the strength of a caveat I can partly retire. I have since walked both mechanisms myself against the worktree at R2 — confirmed. The chain is exactly as reported:
So on the large majority of reads the label is computed across the full query and then discarded. R3 — confirmed. Same three links:
One detail that corroborates the measurement rather than just the mechanism: What remains single-source. I re-derived the mechanisms, not the numbers. The 25–34× slowdown table for R2 and the 176 B/op figure for R3 are still from benchmark and soak transcripts I have not re-run, so treat the magnitudes as indicative. Neither suggested fix depends on the exact multiplier: R2 is "compute it after the gate, like R1 is unchanged — I confirmed that one's mechanism in the round-2 comment itself by reading |
3 participants
Summary
This fixes the managed-Oxigraph store-scheduler cascade investigated in #1989.
A caller that already constrained
GRAPH ?sourceGraphto five verified VM partitions was being rewritten with a secondVALUES ?sourceGraphcontaining the entire DKG allow-list. On the captured node, this expanded a 3,079-byte query into the stable approximately 24 KB query that occupied Oxigraph for roughly 300 seconds. Repeated refreshes then caused normal and background store queue timeouts across SWM catch-up, durable sync, gossip validation, and promotion work.The change:
VALUESconstraint when every graph is already inside the DKG allow-list;/api/querywork through the background lane withsource=api.query;SELECT/ASK/CONSTRUCT/DESCRIBEafter SPARQLPREFIXandBASEprologues.Related to #1989 and the SWM symptom evidence in #1990. This PR does not claim that the separate mainnet Blazegraph atomic-replace failures have the same upstream query.
Root cause and fixed path
sequenceDiagram autonumber participant B as Blackbox ruleset refresh participant API as DKG POST /api/query participant A as DKGAgent.query participant Q as Scoped DKG query engine participant S as Store priority scheduler participant O as Managed Oxigraph participant W as SWM and durable sync work participant D as Daemon shutdown rect rgb(255, 235, 235) Note over B,O: Before this PR - redundant graph scoping creates the planner stall B->>API: Metadata SELECT over the CG meta graph API->>A: contextGraphId plus prefixed SPARQL A->>Q: Query without store source, priority, or cancellation Q->>O: Metadata query O-->>Q: 249 assertionGraph and status bindings Q-->>B: 249 bindings B->>B: Keep 125 confirmed partitions and select a batch of 5 B->>API: Partition SELECT with VALUES sourceGraph containing 5 exact graph IRIs API->>A: Request enters with no disconnect signal A->>Q: Scoped query Q->>Q: Discover 125 materialized VM graphs Q->>Q: Build DKG allow-list with 128 entries Q->>Q: Inject a second VALUES sourceGraph containing all 128 entries Note over Q,O: Caller query 3,079 bytes becomes approximately 24,051 bytes Q->>S: Enqueue as normal sparql-http.query S->>O: Execute expanded query O--xS: Planner remains occupied until the 300 second client deadline B--xAPI: Plugin timeout or caller disconnects Note over API,O: Disconnect is not propagated, so backend work remains queued or in flight B->>API: Periodic refresh submits the same query family again API->>S: More normal query work accumulates W->>S: SWM catch-up, durable insert, gossip validation, and promotion request store work S--xW: Queue wait timeout in normal and background lanes Note over S,W: SWM catch-up job fails without counters or parity evidence D->>S: stop requested D->>D: Emits Stopped while SPARQL HTTP work still exists O-->>S: Slow-query completions appear after Stopped end rect rgb(235, 255, 235) Note over B,O: After this PR - bounded query, isolated admission, and lifecycle cancellation B->>API: Same 5-graph partition SELECT API->>API: Create AbortController and label source as api.query API->>A: Pass background priority, source, and disconnect signal A->>Q: Forward store work options Q->>Q: Parse top-level static VALUES sourceGraph Q->>Q: Verify every caller graph is inside the DKG allow-list alt Every graph is allowed Q->>Q: Keep the caller's 5-graph VALUES unchanged Note over Q,O: Query stays bounded instead of carrying the 128-entry list else Dynamic, unsupported, malformed, or foreign graph Q->>Q: Retain the full allow-list intersection Note over Q: Access control remains fail-closed end Q->>S: Enqueue as background api.query with cancellation S->>O: Execute bounded query O-->>S: Return result S-->>API: Complete without starving protected work W->>S: SWM, durable sync, gossip, promotion, health, and ACK work S-->>W: Protected lanes continue to make progress opt Caller disconnects API-xS: Abort queued or in-flight query S-xO: Abort HTTP fetch end opt Daemon stops D->>S: Abort store lifecycle controller S-xO: Cancel all queued and in-flight SPARQL HTTP work D->>D: Await drain, then emit Stopped end endFull incident flow
The 249-binding metadata query succeeds. The following five-partition content query is the request that DKG amplifies and that occupies Oxigraph.
flowchart TB subgraph BB["Blackbox ruleset refresh"] T["Periodic ruleset refresh starts"] Q1["_verified_partitions_sparql()"] META["Query exact context-graph /_meta graph"] R249["Return 249 assertionGraph/status bindings"] FILTER["Keep 125 confirmed materialized VM partitions"] BATCH["Sort and batch: 5 graph IRIs per query"] Q2["_partition_threats_sparql()"] CALLER["Caller supplies VALUES ?sourceGraph with 5 exact graphs"] T --> Q1 --> META --> R249 --> FILTER --> BATCH --> Q2 --> CALLER end NOTSLOW["The metadata query is discovery, not the 24,051-byte stalled query"] META -. clarification .-> NOTSLOW subgraph OLD["Behavior before this PR"] API["POST /api/query with contextGraphId"] AUTH["Compute existing DKG authorization scope"] DISCOVER["Discover 125 materialized VM graphs plus route entries"] DETECT["Detect GRAPH ?sourceGraph"] INJECT["Inject a second VALUES ?sourceGraph with the full 128-entry allow-list"] INTERSECT["Both VALUES tables intersect to the original 5 graphs"] LARGE["Backend query expands from 3,079 bytes to about 24,051 bytes"] UNKNOWN["Telemetry reports source=unknown and operation=unknown"] API --> AUTH --> DISCOVER --> DETECT --> INJECT --> INTERSECT --> LARGE API -. missing attribution .-> UNKNOWN end CALLER --> API subgraph OXI["Planner stall and timeout mismatch"] PLAN["Oxigraph planner remains occupied"] STORETIME["DKG SPARQL client deadline: about 300 seconds"] PLUGINTIME["Blackbox request timeout: about 120 seconds"] DISCONNECT["Blackbox disconnects"] ORPHAN["Backend request is not cancelled"] RETRY["Later refresh submits the same query family"] OVERLAP["Planner-stalled requests overlap"] LARGE --> PLAN PLAN --> STORETIME PLAN --> PLUGINTIME --> DISCONNECT --> ORPHAN --> OVERLAP RETRY --> OVERLAP OVERLAP -. periodic refresh .-> RETRY end subgraph SAT["Store scheduler saturation"] SLOTS["Store concurrency slots remain occupied"] QUEUE["Other store operations queue"] WAIT["Scheduler queue-wait deadline expires"] SLOTS --> QUEUE --> WAIT end OVERLAP --> SLOTS subgraph DAMAGE["Observed collateral failures"] PROMOTE["Promotion and reconciliation time out"] GOSSIP["Gossip validation times out"] SWM["SWM catch-up, cleanup, and materialization time out"] RS["Random-sampling loop reports store failures"] WRITES["Durable inserts and graph materialization lose store capacity"] WAIT --> PROMOTE WAIT --> GOSSIP WAIT --> SWM WAIT --> RS WAIT --> WRITES end subgraph STOP["Shutdown gap"] SHUTDOWN["Daemon shutdown begins"] SERVER["Managed Oxigraph server stops"] STOPPED["Daemon logs Stopped."] LATE["Orphan promises settle and emit slow-query records afterward"] SHUTDOWN --> SERVER --> STOPPED --> LATE end ORPHAN --> LATE subgraph FIX["Behavior with this PR"] SAFE["Recognize simple top-level static graph VALUES"] CHECK["Verify every caller graph is in the existing DKG allow-list"] SKIP["Skip only the redundant full-list injection"] SMALL["Keep the original five-graph query bounded"] BACKGROUND["Admit external API work as background api.query"] LABEL["Classify prefixed query as operation=select"] ABORT["Propagate disconnect and shutdown cancellation"] DRAIN["Drain outstanding SPARQL work before shutdown completes"] HEALTHY["Protected store work continues to make progress"] FALLBACK["For dynamic, malformed, or foreign values, retain the old intersection"] SAFE --> CHECK CHECK -->|"all graphs authorized"| SKIP --> SMALL --> BACKGROUND --> HEALTHY CHECK -->|"cannot prove safe"| FALLBACK BACKGROUND -. telemetry .-> LABEL BACKGROUND --> ABORT --> DRAIN end CALLER -. fixed path .-> SAFE BOUNDARY["Evidence boundary: this PR explains the managed-Oxigraph incident. It does not prove that the separate mainnet Blazegraph atomic-replace failures have the same Blackbox source."] WRITES -. related symptom class only .-> BOUNDARYThe existing DKG allow-list is an authorization boundary, not a new query limitation. An unrestricted graph variable still ranges over every graph authorized by the selected context graph and view. The optimization applies only when the caller already supplied a smaller static subset and DKG can prove that every graph in it is authorized.
Problematic backend query
The slow request is the Blackbox ruleset refresh's partition-content query, not its preceding metadata query. Blackbox submits this bounded form (representative captured batch shown):
Before this PR, DKG effectively inserted another table for the same graph variable at the start of the outer WHERE group:
The two tables are semantically intersected, so the result remains scoped to the original five authorized graphs. The failure is planner amplification: the redundant 128-entry table is combined with the UNION and OPTIONAL-heavy graph pattern, producing the captured 24,051-byte, approximately 300-second query family with hash 8ae067fa999106aa.
Implementation details
Fail-closed graph constraint optimization
constrainGraphVariablesToAllowedSetnow recognizes only the scalar, top-level static shape used by graph-batch callers:VALUES ?sourceGraph { <graph-1> <graph-2> ... }It skips the redundant second constraint only when every parsed IRI resolves inside the already-computed DKG allow-list. Tuple
VALUES, variables, literals,UNDEF, nested groups, malformed tokens, unknown prefixes, and any out-of-scope graph keep the prior intersection rewrite.Admission and cancellation
QueryOptionsnow carriessignal,priority, andsourcethroughDKGAgent.query,DKGQueryEngine, and the final store operation. The daemon query route marks external reads as background work and aborts when the request or response connection closes.SparqlHttpStorecomposes caller cancellation with a store-lifecycle controller.close()aborts and awaits the scheduler promises before returning, while preserving the adapter's existing reusable-close behavior for direct remote-endpoint callers.Telemetry
Slow-query operation inference now skips SPARQL comments plus
PREFIXandBASEprologues. The incident query will be reported as:rather than
source=unknown operation=unknown.Captured reproduction
10.0.10, base commitf61f335085ec2aa6509a768e85222988d67947dfsparql-http8ae067fa999106aaThe one-byte reconstruction variance is likely graph-inventory digit drift after the captured window; this PR does not claim a byte-for-byte hash reproduction. The source, query shape, cardinality, expanded size, and repeated log cadence all align.
Validation
pnpm exec vitest run test/query-engine.test.tsinpackages/query: 117/117pnpm exec vitest run test/sparql-http.test.tsinpackages/storage: 32/32pnpm exec vitest run --config vitest.unit.config.ts test/query-min-trust-alias.test.tsinpackages/agent: 4/4pnpm exec vitest run test/daemon/routes/query.test.tsinpackages/cli: 3/3 against a real daemonpnpm --filter @origintrail-official/dkg-storage buildpnpm --filter @origintrail-official/dkg-query buildpnpm --filter @origintrail-official/dkg-agent build, including type and package-root testspnpm --filter @origintrail-official/dkg buildgit diff --checkThe query regression mirrors the incident cardinality with 125 materialized VM graphs and a five-graph caller constraint. It asserts that the executed query remains exactly the caller query. A separate regression supplies a foreign graph and verifies that DKG retains the intersection and does not leak it.
Runtime verification still required
Deploy this branch to the same edge canary and rerun the Blackbox/SWM scenario. Acceptance criteria:
8ae067fa999106aaor the approximately 24 KB query family;api.queryoperation;Stopped..Review focus
VALUESparser and prefix resolution;SparqlHttpStoreinstances.Review feedback addressed
Commit
c50b39235addresses the review findings:DKG_API_QUERY_PRIORITY=normalwhile retainingbackgroundas the protective default;STORE_SCHEDULER_BUSYshedding to HTTP 503 withRetry-After: 1on read-only query paths;cancelled, not failed, in operation tracking;Additional validation on this head:
git diff --checkpasses.