feat(observability): unify scheduler backpressure diagnostics - #2003
Conversation
| !authEnabled | ||
| || ( | ||
| !!requestToken | ||
| && validTokens.has(requestToken) |
There was a problem hiding this comment.
🔴 Bug: Diagnostics route bypasses the normal token validation path
What's wrong
The new admin diagnostics endpoint authorizes directly against the in-memory token set. That skips the daemon’s normal token reconciliation and signed-request checks, so revocation and rotation semantics are weaker for this sensitive endpoint than for the rest of the protected API.
Example
If an operator rotates ~/.dkg/auth.token, the old token can remain in the in-memory validTokens set until a guarded route calls the reconciliation path. A request to GET /api/diagnostics/backpressure with that old bearer token can pass this direct check and receive the node-wide snapshot even though the token was revoked on disk.
Suggested direction
Use verifyToken() or httpAuthGuard() instead of consulting validTokens directly, then keep the existing !agent.resolveAgentByToken(...) node-admin check after the token is known current.
For Agents
In packages/cli/src/daemon/routes/backpressure.ts, validate the bearer through the same auth path used by protected routes before applying the node-admin-vs-agent distinction. Preserve auth-disabled access and the 403 for valid agent-scoped tokens. Add a route test for a stale token that remains in validTokens but should be rejected after auth reconciliation.
There was a problem hiding this comment.
🟡 Issue: Extract the node-admin gate instead of copying it again
What's wrong
The PR adds another ad-hoc copy of a security-sensitive route policy. Even if the behavior is correct, duplicating the predicate makes the daemon routing layer harder to maintain and easier to drift.
Example
If the daemon later adds another node-admin token source or changes how agent-scoped tokens are resolved, maintainers must update each copied predicate. Missing this new route would create route-level policy drift.
Suggested direction
Move the node-admin predicate into a canonical route-auth helper and call that from this diagnostics route.
For Agents
Add a shared helper in the daemon route layer, for example isNodeAdminCaller(ctx) and optionally requireNodeAdmin(ctx, message). Use it in packages/cli/src/daemon/routes/backpressure.ts; migrating the existing duplicated routes in the same PR would be a good cleanup but the behavior should remain unchanged.
There was a problem hiding this comment.
🟡 Issue: Backpressure diagnostics auth lacks negative coverage for missing or invalid tokens
What's wrong
This endpoint exposes node-wide scheduler diagnostics and implements its own authorization decision. The current tests prove that an agent-scoped token is blocked, but they do not prove that a request with no token, or a bearer token that is not in the node token set, is blocked while daemon auth is enabled. That leaves an important access-control path unverified.
Example
A focused regression test could call GET /api/diagnostics/backpressure with authEnabled: true and no requestToken, and another with a token not present in validTokens; both should assert 403 and no scheduler snapshot body.
Suggested direction
Add negative route tests for auth-enabled requests without a token and with an invalid token so the new diagnostics endpoint’s access-control boundary is explicitly verified.
For Agents
Update packages/cli/test/backpressure-route.test.ts around the new route tests to cover auth-enabled requests with no bearer token and with an unrecognized bearer token. Preserve the existing admin, agent-token, and auth-disabled behavior, and assert the route does not return backpressureRegistry.capture() for unauthorized callers.
There was a problem hiding this comment.
🟡 Issue: Reuse a canonical node-admin authorization helper
What's wrong
The authorization rule is subtle and security-sensitive, even when reviewed only as code structure. Adding another inline copy makes the daemon harder to maintain because the rule now has to stay synchronized across route files by convention.
Example
A future route that needs node-admin-only access has to rediscover the exact auth-disabled behavior, token membership check, and agent-scoped-token exclusion. Any small divergence creates a different admin boundary in one route.
Suggested direction
Move the node-admin predicate into one daemon helper and have this route call it. The route should only decide path/method and response shape; token taxonomy should live in one canonical place.
For Agents
Look at packages/cli/src/daemon/routes/backpressure.ts and the existing node-admin checks in daemon routes. Preserve the current authorization behavior, but extract a shared isNodeAdminRequest(ctx) or requireNodeAdmin(ctx, routeName) helper and use it here. Existing route tests should still pass, with this route covering the helper through its 200/403 cases.
| * run. All observability calls are fail-open so instrumentation cannot change | ||
| * scheduler behaviour. | ||
| */ | ||
| export class SchedulerPressureTracker { |
There was a problem hiding this comment.
🟡 Issue: Split the new backpressure core module before it becomes a grab bag
What's wrong
The new core observability layer starts life as one large mixed-concern module. That makes the abstraction harder to scan and raises the cost of future changes because unrelated concepts now share one file and import boundary.
Example
A change to the diagnostics response shape, a change to monitor log formatting, and a change to tracker state classification all land in the same core file today, even though those are separate reasons to edit the code.
Suggested direction
Keep the public API stable, but move tracker/state classification, registry capture, monitor/log formatting, and metric projection into separate files with narrow imports.
For Agents
Split packages/core/src/backpressure-observability.ts into cohesive modules such as backpressure/types.ts, pressure-tracker.ts, registry.ts, monitor.ts, and metrics.ts. Preserve the existing exported API through packages/core/src/index.ts and run the core, agent, storage, and CLI backpressure tests after moving code.
There was a problem hiding this comment.
🟡 Issue: Split the backpressure observability core by ownership
What's wrong
This introduces a broad central module that owns too many reasons to change. The abstraction is useful, but the implementation is not yet modular enough for a core observability surface that will likely get extended by more schedulers and routes.
Example
Changing only log formatting or summary cadence currently requires editing the same module that owns lifecycle ticket transitions and public snapshot contracts. Adding the next scheduler-specific reporting concern is likely to grow this file into a core catch-all.
Suggested direction
Keep the public API stable, but decompose the implementation so tracker lifecycle, registry capture, metric recording, and monitor/log formatting can evolve independently.
For Agents
Split packages/core/src/backpressure-observability.ts into focused modules such as backpressure-types, scheduler-pressure-tracker, backpressure-registry, backpressure-metrics, and backpressure-monitor. Preserve the public barrel exports and existing tests while making ownership boundaries explicit.
There was a problem hiding this comment.
🟡 Issue: Split the new backpressure core before it becomes a catch-all module
What's wrong
The new module is below the 1k-line hard threshold, but it already combines several independently changing responsibilities. That makes the abstraction less crisp than it needs to be and gives future contributors an obvious place to add unrelated backpressure behavior instead of preserving clear ownership boundaries.
Example
SchedulerPressureTracker records lifecycle metrics directly, while BackpressureMonitor.sample() records snapshot metrics from the registry. Those are two different sink responsibilities living beside the state model and registry, so a reader changing the pressure state rules has to scan logging and telemetry plumbing too.
Suggested direction
A cleaner structure would separate the pure pressure model/tracker, state helpers, registry, metrics sink, and monitor/log formatter. That lets scheduler integrations depend on the smallest concept they need and keeps future observability additions from accumulating in one broad file.
For Agents
Split packages/core/src/backpressure-observability.ts into focused modules while keeping the exported API stable from packages/core/src/index.ts. Preserve snapshot shapes and metric names. Unit tests should continue to cover tracker state, registry failure isolation, and monitor logging behavior after the split.
| * queue ordering, admission, coalescing, and release semantics and call these | ||
| * protected lifecycle methods at their existing boundaries. | ||
| */ | ||
| export abstract class ObservableScheduler implements BackpressureSource { |
There was a problem hiding this comment.
🟡 Issue: Prefer composition over the thin ObservableScheduler base class
What's wrong
This abstraction adds inheritance and protected wrapper methods without deleting complexity. It also couples unrelated scheduler implementations to a core superclass, which makes future schedulers harder to adapt if they already have an inheritance model or want only a BackpressureSource adapter.
Example
The subclasses get no real scheduling simplification from inheritance: they still call pressureEnqueue, pressureStart, pressureRejectQueued, pressureCancelQueued, and pressureFinish themselves, while also becoming locked into a core base class.
Suggested direction
Use a composed tracker or source adapter instead of inheritance. The code should make scheduler policy ownership explicit without forcing every observable scheduler through a pass-through base class.
For Agents
Delete ObservableScheduler or reduce it to a composable helper/adaptor. Give schedulers a private SchedulerPressureTracker or a small pressure lifecycle object, and register BackpressureSource instances explicitly at the owning module boundary. Preserve the existing snapshots and monitor output.
There was a problem hiding this comment.
🟡 Issue: The observable base class adds indirection without earning it
What's wrong
The new abstraction is mostly an identity wrapper around SchedulerPressureTracker. It does not simplify queue policy or remove lifecycle complexity; it just moves tracker calls behind protected methods and makes observability part of every scheduler's inheritance tree.
Example
StorePriorityScheduler now extends a telemetry base class and carries pressureTicket through queue entries; PriorityAdmissionQueue repeats the pattern with a WeakMap and observe-pressure helper methods. A reader has to understand inheritance and protected forwarding before reaching the actual queue policy.
Suggested direction
Delete the base class and let schedulers compose a tracker or adapter. Composition would keep telemetry out of the scheduler's class hierarchy, avoid spending TypeScript's single inheritance slot, and remove the protected pass-through layer.
Confidence note
This is a structural maintainability concern rather than a behavior failure; the current implementation can work, but it establishes an avoidable inheritance pattern for future schedulers.
For Agents
Look at packages/core/src/backpressure-observability.ts, plus the two scheduler integrations. Preserve snapshots and lifecycle events, but replace ObservableScheduler with composition: a private SchedulerPressureTracker or small BackpressureSource adapter that can be registered directly. Tests should still cover store and sync snapshots after enqueue/start/reject/finish.
There was a problem hiding this comment.
🟡 Issue: ObservableScheduler is a thin inheritance wrapper that adds indirection without simplifying the scheduler code
What's wrong
The new base class creates an architectural dependency between scheduler implementations and observability, but it does not centralize the complex lifecycle bookkeeping. That leaves the code with both inheritance indirection and scattered manual ticket transitions, which is harder to reason about than a direct composed tracker.
Example
PriorityAdmissionQueue extends ObservableScheduler, then separately maintains pressureTickets and calls observePressureEnqueue, observePressureStart, observePressureReject, observePressureCancel, and observePressureFinish across the admission branches. The base class mostly hides that these are plain SchedulerPressureTracker calls.
Suggested direction
Use composition: keep SchedulerPressureTracker as the real abstraction, expose a small source adapter for registry registration, and let each scheduler own a private tracker/null-observer instead of subclassing a pass-through base.
For Agents
In packages/core/src/backpressure-observability.ts, consider deleting ObservableScheduler. Let schedulers compose an optional SchedulerPressureTracker directly and register a small BackpressureSource adapter. Preserve the current snapshots/metrics while removing the inheritance wrapper and protected pass-through methods.
| export class PriorityAdmissionQueue<Payload> extends ObservableScheduler { | ||
| private readonly queue: InternalEntry<Payload>[] = []; | ||
| private readonly handoffReservations = new Map<number, HandoffReservation>(); | ||
| private readonly pressureTickets = new WeakMap<PriorityAdmissionEntry<Payload>, SchedulerPressureTicket>(); |
There was a problem hiding this comment.
🟡 Issue: Collapse the priority queue observability side channel into the admission lifecycle
What's wrong
The observability integration is bolted onto several existing branches instead of becoming a single lifecycle boundary. That makes the queue harder to reason about and creates a maintenance trap where future control-flow changes can silently drift from the pressure model.
Example
Adding one new queued-exit path now requires remembering to remove the queue entry, clear timers/listeners, settle the promise, record the scheduler decision metric, and call the matching pressure method. The pressure capacity is also whatever the latest acquire call wrote, rather than a canonical queue snapshot.
Suggested direction
Make pressure tracking part of the same internal entry lifecycle that already owns timers, abort listeners, removal, and settlement. Avoid a WeakMap side channel and avoid updating global capacity opportunistically from the last acquire call.
For Agents
In packages/agent/src/sync/priority-admission-queue.ts, move pressure state onto InternalEntry or a small AdmissionLifecycle object and route all queued settlement through one helper. Replace per-acquire updatePressureCapacity with an explicit capacitySnapshot/capacity hook owned by the queue or by the sync adapter. Keep admission ordering, displacement, timeout, abort, and release behavior unchanged.
There was a problem hiding this comment.
🟡 Issue: Do not model queue capacity as a last-acquire side effect
What's wrong
This hides a global invariant that all observed acquisitions must share the same limits. The queue now has two capacity models: per-call admission options for real scheduling and a mutable tracker capacity for diagnostics. That is a brittle boundary and will be hard to reason about when this generic queue gets another caller or policy shape.
Example
Acquire A with queueLimit: 2, leave it queued, then acquire B with queueLimit: 20; the existing queued work is now reported against queue capacity 20 even though admission for A was bounded by 2.
Suggested direction
Make pressure capacity an explicit owner-level contract, such as capacity: () => SchedulerPressureCapacity, instead of mutating scheduler-wide diagnostics from each admission request.
For Agents
Look at packages/agent/src/sync/priority-admission-queue.ts and packages/agent/src/sync/backpressure.ts. Preserve admission behavior, but move observable capacity to a queue-level provider/constructor option or implement BackpressureSource at the sync-global owner that already knows the canonical policy. Add a focused test proving snapshots do not depend on the last acquire call.
There was a problem hiding this comment.
🟡 Issue: Avoid side-table observability bookkeeping in the scheduler
What's wrong
The change bolts a parallel lifecycle onto the admission queue instead of simplifying around the queue's existing lifecycle. The WeakMap exists because immediate and queued work use different object shapes, so pressure cleanup is now distributed across many branches rather than owned by the entry cleanup/start/reject paths.
Example
A future path like dropOwner(ownerKey) or queue shutdown would have to remember to remove the queue entry, reject the promise, clear timers/listeners, and call the matching pressure ticket method. Missing one step leaves the observability state inconsistent even though the scheduling code looks locally complete.
Suggested direction
Collapse pressure state into the queue's existing internal entry lifecycle, or move it behind a dedicated observer object so the scheduler is not carrying a second cleanup protocol.
For Agents
In packages/agent/src/sync/priority-admission-queue.ts, preserve all existing admission/release semantics. Either use one internal admission record for both immediate and queued work with an optional pressureTicket, or extract a null-object PriorityAdmissionPressureObserver that owns ticket bookkeeping and is invoked from canonical queue transitions.
There was a problem hiding this comment.
🟡 Issue: Pressure capacity should not be last-call mutable state
What's wrong
This adds a hidden global side effect to a generic queue: every acquire can overwrite the capacity reported by observability. The queue's behavioral limit is still carried on each admission option, while the diagnostic limit is stored separately on the tracker, so maintainers now have to keep two capacity models in their head.
Example
If one caller acquires with queueLimit: 10 and another later acquires with queueLimit: 2, the pressure snapshot's capacity now depends on the last acquire path rather than the queue's actual policy. That makes the diagnostics model harder to reason about even when the current sync-global caller happens to pass stable values.
Suggested direction
Keep scheduler capacity as a scheduler/policy-level boundary rather than mutating it during every acquire. That would remove a hidden side effect from the hot admission path and make the snapshot contract explicit.
For Agents
Look in packages/agent/src/sync/priority-admission-queue.ts. Preserve admission behavior, but move pressure capacity ownership out of acquireInternal: either configure it once in the queue/policy, expose an observability.capacity() hook, or derive it explicitly from queued entries if per-entry limits are truly part of the model. Add/adjust a unit test that proves snapshots are not last-call-wins when acquire options vary.
There was a problem hiding this comment.
🟡 Issue: Keep pressure tracking out of the generic admission queue's control flow
What's wrong
This turns a policy-focused admission queue into a queue plus observability adapter plus global registration participant. The feature is mechanically correct-looking, but it tangles cross-cutting instrumentation through the hottest and most stateful parts of the scheduler, making future scheduling changes harder to audit.
Example
To understand one queued item's lifecycle now, a reader has to follow the queue entry, the returned admission, handoff reservations, the WeakMap ticket, and five observePressure* helpers. Adding another terminal path would require remembering to update both queue semantics and the pressure ticket bookkeeping.
Suggested direction
Prefer a small lifecycle observer owned by the constructor, with no-op behavior when disabled. The queue should call semantic hooks like observer.enqueued(entry), observer.started(entry), observer.finished(entry) without knowing about registry registration, tickets, WeakMaps, or optional observability branches.
For Agents
Look at packages/agent/src/sync/priority-admission-queue.ts. Preserve admission ordering, handoff, abort, timeout, displacement, and release behavior, but move pressure tracking behind a composed observer/null-object instead of inheritance plus scattered conditionals. Tests should prove the existing sync backpressure scenarios still produce the same queue decisions and pressure snapshots.
| this.recordEvent(lane, 'rejected', normalizedReason); | ||
| } | ||
|
|
||
| private laneSnapshot(lane: string, now: number): BackpressureLaneSnapshot { |
There was a problem hiding this comment.
🟡 Issue: Store pressure records by lane instead of rebuilding lanes by repeated scans
What's wrong
The tracker’s internal model is global queues plus lane metadata, but the public API and monitor operate lane-first. That mismatch adds avoidable iteration and spreads the lane aggregation logic across several blocks.
Example
For the store scheduler's four lanes, one sample walks the queued and active maps once to discover lane names, then filters both maps again for each lane. Adding more lanes or summary fields repeats this pattern instead of reading lane-local state directly.
Suggested direction
Make the lane the primary storage boundary. This should delete the per-lane filter passes and make capacity, queue/active records, rejection counters, and snapshot classification live together.
Confidence note
This is a maintainability/design concern rather than a current scale blocker; the existing store and sync queues are bounded, but the data model is still working against the snapshots it needs to produce.
For Agents
Refactor SchedulerPressureTracker so LaneRuntime owns queued and active records for its lane, plus capacity and rejection counters. Then snapshot can map lane runtimes directly, totals can reduce those lane snapshots once, and operation summaries can be computed from lane-local records.
| .sort((a, b) => a.priority - b.priority || b.sequence - a.sequence)[0]; | ||
| if (!victim) { | ||
| this.recordDecision(base, 'rejected'); | ||
| this.observePressureReject( |
There was a problem hiding this comment.
🟡 Issue: Rejection and cancellation pressure wiring is not covered
What's wrong
The PR adds observability for rejected, displaced, timed-out, and aborted work, which is central to the pressure model, but the new tests do not exercise those adapter paths through the actual schedulers. That leaves the public status state and admin diagnostics able to silently miss short saturation events while tests remain green.
Example
A regression that removes observePressureReject(base, 'global_queue_full') would still leave existing sync busy-error tests passing, but /api/status and /api/diagnostics/backpressure would not show sync-global as recently saturated after a queue-full rejection.
Suggested direction
Cover at least one real scheduler rejection path per instrumented adapter, rather than only testing the tracker primitive and the happy queued-to-finished lifecycle.
For Agents
Add adapter-level tests in packages/agent/test/sync-backpressure.test.ts and/or packages/storage/test/store-priority-scheduler.test.ts that trigger queue-full, displacement or wait-timeout/abort paths, then assert the generic backpressure snapshot records state: 'saturated', increments rejectedByReason, and does not retain queued work after cancellation.
| }, | ||
| // Public status carries state only. Detailed lane timings and operation | ||
| // summaries stay behind the node-admin diagnostics route. | ||
| backpressure: { |
There was a problem hiding this comment.
🟡 Issue: Public status backpressure projection is unverified
What's wrong
The diff adds a new public API field that is supposed to expose only scheduler state while withholding detailed lane timings and operation summaries. No test verifies either the presence of this new status field or the absence of the detailed diagnostics data, so a future change could leak the full registry snapshot or drop the field without failing the current tests.
Example
A failing-test sketch: register a source whose snapshot includes lanes, totals, and operation summaries; call /api/status; assert body.backpressure.state and body.backpressure.schedulers[0].state are present, and assert serialized status does not contain queuedOperations, activeOperations, totals, or operation labels.
Suggested direction
Add a regression test for the public /api/status contract and privacy boundary, separate from the admin diagnostics route tests.
For Agents
Add a focused status-route test in packages/cli/test/status-route-rpc.test.ts or a small new route test that registers a temporary BackpressureSource, calls handleStatusRoutes for /api/status, and proves the response exposes aggregate state only while keeping detailed diagnostics behind /api/diagnostics/backpressure.
There was a problem hiding this comment.
🟡 Issue: Public status backpressure exposure is not verified against diagnostic leakage
What's wrong
The change intentionally splits public status from node-admin diagnostics, but only the admin diagnostics route has tests. Because /api/status is public, the privacy boundary should be covered by a regression test that fails if lane details, counts, ages, or operation labels are accidentally included later.
Example
Register a fake source whose snapshot contains queuedOperations: [{ operation: 'private-cg-peer-a', ... }], request GET /api/status, and assert the response includes only { state, schedulers: [{ scheduler, state }], diagnosticsAvailable } without lanes, operation names, queue ages, or counts.
Suggested direction
Add a status-route regression test that registers a diagnostic source with recognizable lane and operation details, then asserts /api/status returns only aggregate scheduler state and the diagnostics route hint.
For Agents
Add or extend a status route test in packages/cli/test/status-route-*.test.ts for the new backpressure response field. Use a registered fake BackpressureSource, exercise handleStatusRoutes or the daemon request path, and prove public status exposes aggregate state only while omitting detailed diagnostics.
There was a problem hiding this comment.
🟡 Issue: Public status backpressure exposure is not verified
What's wrong
The PR intentionally exposes only backpressure state on the public status route, but no test locks that contract down. That leaves a user-facing and privacy-relevant response shape unverified.
Example
A regression that changed /api/status to return backpressureRegistry.capture() directly would expose queuedOperations/activeOperations on the public status endpoint, while the current added tests would still pass because they only exercise the admin diagnostics route.
Suggested direction
Cover the public /api/status contract separately from the admin diagnostics route, especially the privacy boundary that keeps detailed work summaries out of public status.
For Agents
Add a status-route test around handleStatusRoutes with a registered source containing lane operation summaries. Assert /api/status returns only aggregate scheduler/state data plus diagnosticsAvailable, and does not include lane timings or operation summaries.
There was a problem hiding this comment.
🟡 Issue: Public backpressure status projection is untested
What's wrong
This PR intentionally exposes a reduced backpressure view on the public status endpoint, while detailed diagnostics stay behind the admin route. The added tests verify the admin route but do not verify that /api/status includes the new field or keeps detailed scheduler data out of the public response.
Example
A regression that returned backpressureRegistry.capture() directly from /api/status would expose lane totals and operation summaries publicly, while the current added tests would still pass because none inspect /api/status.backpressure.
Suggested direction
Cover the public status contract separately from the admin diagnostics route so the intended privacy boundary is enforced by tests.
For Agents
Add a status route test around handleStatusRoutes: register a fake backpressure source with lane/totals/operation details, call GET /api/status, and assert the response contains only aggregate state, scheduler names/states, and diagnosticsAvailable, with no lanes, totals, operation summaries, or failures.
There was a problem hiding this comment.
🟡 Issue: Public status backpressure projection lacks a regression test
What's wrong
This PR adds a public status field with an explicit safety boundary, but the added tests only cover the admin diagnostics route and lower-level label sanitization. The public projection could accidentally start returning detailed scheduler snapshots without a failing test.
Example
A status response backed by a registry snapshot containing queuedOperations: [{ operation: 'durable:urn:cg:private:peer-a' }] should expose only { state, schedulers: [{ scheduler, state }], diagnosticsAvailable } and should not include lanes, operation names, graph IDs, or peer IDs.
Suggested direction
Add a route-level assertion for the public status shape and its redaction boundary.
Confidence note
I found no CLI test matching diagnosticsAvailable or the new public status backpressure shape.
For Agents
Add a handleStatusRoutes or daemon HTTP test that registers a diagnostic source with lane/operation details, calls /api/status, and asserts the response includes the aggregate backpressure state but excludes lanes, queuedOperations, activeOperations, graph identifiers, and peer identifiers.
There was a problem hiding this comment.
🟡 Issue: The public status backpressure redaction contract lacks a regression test
What's wrong
The change introduces a new public status field and relies on mapping the registry snapshot down to state-only data, but the test coverage only validates the admin diagnostics route. A future change could accidentally expose detailed node-wide scheduler work on the public status endpoint without a focused test catching it.
Example
Register a BackpressureSource whose snapshot contains queuedOperations like private graph or peer-looking labels, call GET /api/status, and assert the JSON includes only backpressure.state, scheduler names, scheduler states, and diagnosticsAvailable, with no operation summaries or lane timings.
Suggested direction
Add a /api/status regression test for the new backpressure field that uses a detailed registered source and asserts sensitive diagnostics fields are absent from the public response.
For Agents
Extend an existing status-route test or add a focused one around packages/cli/src/daemon/routes/status.ts. Seed backpressureRegistry with a detailed snapshot, hit /api/status, and prove the public response preserves the state-only privacy boundary.
Review —
|
| t | state | normal lane |
|---|---|---|
| 1 s | healthy | oldestActiveAgeMs=1000 |
| 6 s | degraded | oldestQ=5000 |
| 31 s | stalled | oldestA=31000 |
| 300 s | stalled | active:[{operation:"api.query.scoped", count:2, oldestAgeMs:300000}], background: inflight=0/1 queued=31 |
An operator alerts on lanes[].oldestActiveAgeMs > 30000 and reads the culprit straight off activeOperations. Per-source attribution — the thing that would have named the 24 KB query — genuinely exists and is bounded-cardinality: /api/query passes the literal source: 'api.query', and agent lanes pass static labels. Starvation is distinguishable from normal busy (background inflight=0/1 with 31 queued). Shed-by-reason works too: rejectedTotal: 4, byReason: {"queue_full":1,"queue_wait_timeout":3}.
Also verified: numbers match ground truth (22/22 checks against scheduler.snapshot under saturation, shedding, drain, abort, timeout); no counter leak on any path (mutation-proven — deleting pressureCancelQueued goes red); hot-path cost is +2.3–3.9 µs per store operation, not the eager-telemetry shape that bit #1991; auth matches the repo's diagnostic-route convention across every branch (no token / forged / agent-scoped → 403, node-admin → 200); no graph URIs, UALs, peer ids or endpoints reach the payload; and every field in docs/use-dkg/backpressure-observability.md checks out against the code, including the state precedence, the 60 s rejection window, the 8-summary cap and all 9 metric names.
MEDIUM — a fully jammed scheduler reports healthy
backpressure-observability.ts:372-393 (lane) and :288-311 (totals) collect and export inflight/inflightLimit but never reference them in either classifier. Escalation is driven only by queue depth vs queueLimit, ages, and the rejection window. A lane at 100% of its concurrency ceiling with a full backlog is healthy until an age threshold trips:
[t=0 fully saturated, 23 waiting] overall=healthy
normal: state=healthy q=18/64 inflight=2/2 oldestQ=0
background: state=healthy q=5/64 inflight=0/1 oldestQ=0
[t=4.999s] overall=healthy
[t=5.001s] overall=degraded
Nothing can be admitted, 23 items are waiting, verdict healthy. routes/status.ts:768-774 exposes only state, so the first 5 s of a store cascade produce a green /api/status.
The literal fix is a trap, which is why I'm giving the shape rather than a patch: adding active.length >= inflightLimit to the degraded branch makes background (inflightLimit: 1) degraded whenever one background op runs with anything queued — the steady state. It needs the queued > 0 guard and an opt-out for hard-capped lanes, or the store's background limit re-modelled.
MEDIUM — the public status surface goes green exactly when the instrumentation dies
capture() is fail-open per source: a source whose getBackpressureSnapshot() throws is dropped from schedulers and recorded in failures. But the rollup at :515-521 seeds from 'healthy' over a possibly-empty array, and status.ts:685,767-776 projects only state and schedulers[].{scheduler,state} — failures is dropped entirely:
zero-sources: 200 {"state":"healthy","schedulers":[],"failures":[]}
throwing-source: 200 {"state":"healthy","schedulers":[],
"failures":[{"scheduler":"store","error":"boom /graph/secret"}]}
The docs point operators at /api/status backpressure.state, and that surface has no failures array and no source count. The doc's claim that a broken source "is reported in the registry's failures array without hiding healthy sources" is true for the admin route and false for the surface it recommends.
Fix: seed the rollup as degraded/unknown when schedulers.length === 0 && failures.length > 0, and add failures: n (a count) to the status block. Do not put raw failures[].error strings on public status — as the repro shows, that message is source-controlled free text and is the one field in the payload that isn't label-normalized.
MEDIUM — sync-global lanes never report limits, so lane saturation is dead code there
priority-admission-queue.ts:168-173 calls updatePressureCapacity({ queueLimit, inflightLimit }) and never sets lanes, so laneSnapshot reads null for every sync lane and the queued >= queueLimit / >= 0.75 branches can never fire:
lane changelog: state=degraded q=1 queueLimit=null inflight=0 inflightLimit=null
lane durable: state=degraded q=1 queueLimit=null inflight=1 inflightLimit=null
Also visible there: totals.queueLimit is last-writer-wins across callers (latent today because sync-global's policy is constant). Fix: merge lanes: { [entry.lane]: {…} } into existing capacity — merge, not replace, or each acquire wipes the other lanes' limits.
MEDIUM — the scheduler with its own incident history isn't instrumented
createSyncResponderLimiter() (sync/responder/sync-handler.ts:261) builds a PriorityAdmissionQueue with canRun/onStart only and no observability block, so it never registers. /api/diagnostics/backpressure returns exactly two schedulers, store and sync-global — and the sync responder is the queue behind the scan-peg incidents (#1127/#1136/#1221: unbound-?g scans, retry-without-cancel, no backoff). For a PR titled "unify scheduler backpressure diagnostics", that's the one I'd most want covered. I found this independently and it's confirmed: it is the only PriorityAdmissionQueue construction site without an observability hook.
MEDIUM — route registration and token validation are each unpinned
backpressure-route.test.ts imports handleBackpressureRoutes directly with a hand-built ctx, so deleting the registration at handle-request.ts:365 404s the route in production and leaves the test green — the implemented-but-unwired shape that has bitten this repo repeatedly. Separately, deleting && validTokens.has(requestToken) from routes/backpressure.ts:31 (so any non-empty bearer is accepted) also leaves the suite green: there is no negative case for an unknown token, only agent-scoped and auth-disabled. Two small tests close both.
LOW
normallaneinflightLimitreports the shared normal+background budget (store-priority-scheduler.ts:329-332, 2 at defaults), butcanStartcaps normal atnormalLimitWhileBackgroundQueued(=1) whenever background is queued — so the snapshot shows headroom admission won't grant. Reportednormal inflight 1/2while a queued normal item was not admitted.events.rejectedisn't a balanceable flow counter — pre-enqueuerejectand post-enqueuerejectQueuedboth funnel into the same counter with opposite semantics (one must be subtracted fromenqueued, one must not). OnlyrejectedByReasondisambiguates.totals.queueLimitis an unreachable denominator — it sums four independent per-lane limits (256 at defaults) while each lane caps at 64, so both aggregate rules are inert. Cosmetic, but misleading in the JSON.stalledis computed twice and neither path is individually pinned — disabling the lane branch alone passes, disabling the totals branch alone passes, only both together go red.- Unauthenticated saturation oracle.
/api/statusis inPUBLIC_GET_PATHSand the rate-limiter exempt list, so any remote unauthenticated caller now reads node-wide scheduler pressure — direct feedback on whether a load-based attack is landing. No identifiers leak (state plus two static names), which is why this is LOW and not higher, and it's documented as intentional. Worth a conscious decision rather than a default. backpressureRegistry.registerthrows on a duplicate id — a fail-closed path inside fail-open instrumentation, called at module scope instore-priority-scheduler.ts:578. Latent today (no test doesvi.resetModules()on these paths), but a second module instance against one cacheddkg-corewould throw at import.
Scope note
#1939 is not covered by this PR. The finalization inbox isn't a BackpressureSource and isn't registered, so the diagnostics route cannot show a non-draining inbox. That's fine and probably correct — #2002 adds its own dueEntries/oldestDueAgeMs — but the PR description's "node-wide scheduler coverage" framing reads wider than what ships. Worth one sentence in the docs so an operator doesn't assume this route would have caught the finalization incident.
Merge readiness
Approve with fixes. Nothing here is a blocker: admission is provably unchanged, the data is accurate against ground truth, the cost is negligible, auth is right, and the docs are accurate field-by-field — which is rare. The two I'd fix before merge are the healthy-while-jammed classifier and the green-status-when-instrumentation-dies rollup, because both cause the diagnostic to under-report in exactly the conditions it exists for. The unwired-route and unknown-token tests are cheap and worth taking with them.
Method: 3 lenses, schema-free, executing the real StorePriorityScheduler, SchedulerPressureTracker, PriorityAdmissionQueue and the compiled route behind harnesses; every claimed test mutation-tested with proven restores. Suites: core 5, storage 22, agent 15, cli 3 — all green, and the CLI test is confirmed collected by the config CI runs. I independently verified the scheduler's ObservableScheduler inheritance and registration lifecycle, the two registered scheduler ids, and the sync-responder coverage gap.
| agingThresholdMs: options.agingThresholdMs, | ||
| }; | ||
| if (this.hooks.observability) { | ||
| this.updatePressureCapacity({ |
There was a problem hiding this comment.
🟡 Issue: Global sync queue saturation is not reflected on the queued lane
What's wrong
The new sync-global pressure source has multiple logical lanes but only supplies aggregate capacity. Lane snapshots classify saturation using per-lane limits, so a full global queue can leave the lane holding queued work marked healthy. That weakens the new diagnostics route and monitor logs for the main operator question: which lane is under pressure?
Example
With syncGlobalMaxInflight=1 and syncGlobalQueueLimit=1, start one durable sync and queue one swm_recovery item. The scheduler snapshot becomes saturated at totals because queued=1 and queueLimit=1, but the swm_recovery lane itself reports queueLimit:null and state:healthy until the age threshold passes. The detailed endpoint therefore says the scheduler is saturated while the actual queued lane is healthy.
Suggested direction
Either project global queue pressure onto affected lanes for PriorityAdmissionQueue, or add a documented aggregate lane to snapshots so operators do not see a saturated scheduler with only healthy lanes.
Confidence note
The behavior follows from the diff and tracker code; whether the team considers a global queue's saturated state sufficient without per-lane saturation needs product confirmation.
For Agents
Look at PriorityAdmissionQueue observability integration and SchedulerPressureTracker lane classification. Preserve global queue semantics, but make the diagnostic lane state reflect global saturation for lanes with queued/rejected work, or pass explicit per-lane capacity when the queue is configured as a global limit. Add a test asserting the queued sync lane is non-healthy when the global queue is full.
| ? getCachedExternalStoreQuads(agent, Date.now()) | ||
| : peekCachedExternalStoreQuads() | ||
| : null; | ||
| const backpressure = backpressureRegistry.capture(); |
There was a problem hiding this comment.
💡 Suggestion: Consider a state-only backpressure summary API
Why it matters
This would keep the public status route decoupled from the private diagnostics payload and make the intended boundary explicit in core instead of relying on every route to remember which fields to strip.
Suggestion
Add something like backpressureRegistry.captureSummary() or captureState() that returns only aggregate state and scheduler states, and keep full lane/operation capture reserved for the diagnostics route.
| }, | ||
| ) => { | ||
| const attributes = { scheduler: snapshot.scheduler, lane }; | ||
| metrics.backpressureQueueDepth.record(values.queued, attributes); |
There was a problem hiding this comment.
🟡 Issue: Backpressure metric emission is not asserted
What's wrong
Metrics are one of the main observability surfaces added by the change and documented for operators, but the tests do not prove that the new instruments actually receive samples.
Example
Deleting the body of recordBackpressureSnapshotMetrics, or removing the per-lane loop at line 561, would leave diagnostics/log tests green while the documented dkg.backpressure.queue_depth, inflight, and age metrics disappear.
Suggested direction
Add a focused metric-emission test for recordBackpressureSnapshotMetrics or BackpressureMonitor.sample, rather than relying on snapshot/log tests to indirectly exercise the path.
For Agents
Add a core telemetry test using the existing in-memory meter pattern or a stubbed metrics facade. Feed a snapshot with totals and one lane, then assert queue depth/limits/inflight/age metrics are recorded for lane: "all" and the concrete lane with bounded scheduler/lane attributes.
Round 2 — re-review at
|
Note on
|
| this: DKGAgent, | ||
| contextGraphId: string, | ||
| options: { signal?: AbortSignal } = {}, | ||
| options: { signal?: AbortSignal; source?: string } = {}, |
There was a problem hiding this comment.
🟡 Issue: Centralize source labels instead of threading ad-hoc strings through domain APIs
What's wrong
The observability concern is spreading as raw string parameters through business logic and public-ish method options. That creates a loose, typo-prone contract and makes domain APIs carry telemetry vocabulary that is not part of their core responsibility.
Example
agent.vmReconcile.swmFingerprint.operations, agent.vmReconcile.swmFingerprint.data, and agent.vmReconcile.swmFingerprint.privateRoots are repeated as raw strings and then asserted in tests. A typo or rename silently creates a new operation class rather than failing at a typed boundary.
Suggested direction
Keep caller attribution, but make the boundary explicit: a typed StoreQuerySource catalog, scoped helper such as queryWithSource(...), or operation-context attribution would avoid leaking raw observability strings through domain methods like getContextGraphOnChainId.
Confidence note
There are pre-existing source labels in the codebase, but this PR materially expands the pattern and also adds a source option to a domain method, so the maintainability concern is introduced/worsened by the diff.
For Agents
Look across the changed source: additions in agent, publisher, and CLI files. Preserve the emitted label values, but centralize them in a typed label catalog or small helper APIs near the store/observability boundary. Add a focused test that validates the catalog values rather than requiring every business method to expose string plumbing.
There was a problem hiding this comment.
🟡 Issue: Store-level attribution leaks into a domain API
What's wrong
The method now exposes an observability implementation detail as part of its options shape. That invites more caller-specific telemetry flags to spread through otherwise domain-focused APIs, and the value is an unconstrained string with no ownership model.
Example
A caller resolving a context graph on-chain id for VM reconcile now needs to know and pass a store scheduler attribution label, even though the semantic operation is just resolving the id.
Suggested direction
Keep this method's public contract semantic. If attribution must vary by caller, carry it through an operation context or scoped store/query helper rather than exposing source?: string on context-graph lookup APIs.
For Agents
Refactor getContextGraphOnChainId and its VM reconcile caller. Preserve abort behavior and the existing default attribution, but avoid adding a free-form telemetry field to the domain method contract. Add a focused test proving VM reconcile still gets distinct attribution through the new boundary.
There was a problem hiding this comment.
🟡 Issue: Query attribution is leaking into domain APIs as ad-hoc strings
What's wrong
This makes the diagnostics taxonomy harder to maintain because labels are now untyped string literals spread through unrelated business methods. It also couples ordinary domain APIs to scheduler-observability concerns, so future callers must understand both the domain behavior and the profiling taxonomy to avoid losing or mislabeling pressure data.
Example
getContextGraphOnChainId('research') and getContextGraphOnChainId('research', { source: 'agent.vmReconcile.resolveOnChainId' }) execute the same domain lookup but expose a different diagnostic identity depending on whether the caller remembered to pass an ad-hoc string.
Suggested direction
Centralize query-source attribution at the store/operation boundary, preferably with typed constants or a scoped withQuerySource/OperationContext mechanism, instead of widening domain method signatures with source?: string.
For Agents
Look at packages/agent/src/dkg-agent-cg-registry.ts and the new source-label callsites across agent/publisher. Preserve the emitted bounded operation labels, but move attribution behind a typed helper, scoped store wrapper, or operation context so domain methods do not grow arbitrary telemetry-string parameters. Add a focused test proving caller attribution still reaches the store boundary.
| }; | ||
| }, | ||
| onDepthChange: (depth) => getMetrics().syncBackgroundQueueDepth.record(depth), | ||
| observability: { |
There was a problem hiding this comment.
🟡 Issue: Sync-global saturation is not verified for global-only capacity
What's wrong
The sync-global scheduler does not configure per-lane queue limits, so its saturated state depends on the generic tracker’s total-capacity branch rather than lane classification. The added tests verify sanitized operation labels and an aged/degraded queue, but not the full-queue state operators will see in /api/status, diagnostics, logs, and metrics.
Example
With syncGlobalMaxInflight: 1 and syncGlobalQueueLimit: 1, start one durable sync and queue a second. Before the queue-age threshold elapses, backpressureRegistry.capture() should report the sync-global scheduler as saturated with totals.queued === 1 and totals.queueLimit === 1; the current tests do not prove that.
Suggested direction
Add a regression test for the exact global sync admission scenario where capacity exists only at the scheduler totals level.
Confidence note
This is based on the diff and test search; I did not run the suite in this read-only review environment.
For Agents
Extend the sync backpressure test that already fills the queue, or add a focused PriorityAdmissionQueue test with no per-lane limits, to assert total-only saturation and the scheduler-level snapshot state. If monitor behavior is part of the contract, also assert the all scheduler sample logs when lane states remain healthy.
Round 4 — re-review at
|
| mutation | result |
|---|---|
| remove the entire second argument | RED (names :386) |
keep {}, strip only source: |
GREEN ← false pass |
That second row is the gap. The realistic way it bites is not someone writing {} deliberately — it's a future call site that passes an options object for some other reason (maxResponseBytes, a signal, a priority) and never gets a source. The test would bless it, and the operator-facing attribution this whole PR is built on would quietly regress at that site.
Fix: require the second argument to carry a source. The non-trapping form matters here — don't simply assert "is an ObjectLiteralExpression with a source property", because that will false-positive the moment someone legitimately threads a prepared options variable (the queryOptions: cleanupQueryOptions pattern used elsewhere in this repo). Reject only when the argument is an object literal and that literal has no source key; allow identifiers and spreads through. I checked the current state — every covered call site today passes an object literal, so the stricter check is safe to land right now with no false positives.
LOW — coverage is scoped to a fixed file list and a store-named receiver
TARGETS is six agent files plus the publisher equivalent, so an unattributed store.query anywhere else still passes. The receiver regex (?:^|\.)store\??$ matches store, this.store, x.store and store?, but not a differently-named handle. Both are reasonable scoping choices for a first pass — worth a comment in the test saying the list is deliberately the profiled hot paths, so the next person knows adding a file is expected rather than wondering why their new query wasn't caught.
Still open from ed7a282cb
None of these are touched by the last three pushes, so restating briefly: the state classifier ignores inflightLimit (a fully jammed lane reports healthy); the status rollup seeds from healthy over an empty array so the public surface goes green when every source fails, with failures dropped from that surface entirely; sync-global lanes never populate capacity.lanes so per-lane saturation is dead code there; the sync-responder limiter — the queue from the #1127/#1136/#1221 scan-peg incidents — is still uninstrumented; and the route registration plus validTokens membership are each unpinned.
Merge readiness
Unchanged: approve-with-fixes. This push adds real value and introduces no risk I can find. The arity-vs-attribution gap is a one-condition change and worth taking while the test is fresh.
Method: reviewed directly. Cardinality checked across all 62 new labels; labels-only confirmed by whitespace-insensitive word-diff; the coverage guard mutation-tested in both directions with restores proven by git diff returning 0 lines and a clean git status.
| && node.expression.name.text === 'query' | ||
| ) { | ||
| const receiver = node.expression.expression.getText(sourceFile); | ||
| if (/(?:^|\.)store\??$/.test(receiver) && node.arguments.length < 2) { |
There was a problem hiding this comment.
🔴 Bug: The attribution model is being enforced through scattered call-site literals
What's wrong
This makes observability bookkeeping a cross-cutting concern in busy domain code. Every future query edit now has to choose or copy a label, and the AST test locks that implementation detail in place while still missing indirect helpers and label quality. That is brittle coupling rather than a clean boundary.
Example
packages/agent/src/dkg-agent-publish.ts now carries labels like agent.assertionFinalize.existingSeal, agent.assertionFinalize.lifecycleScope, and agent.assertionFinalize.existingKaId inside the already-large assertion finalization flow. The test only checks that an options object exists; it does not model the intended operation taxonomy.
Suggested direction
Push query attribution to a canonical operation boundary instead of requiring every domain query to remember a string literal. A withStoreSource(...) wrapper, typed repository method, or operation context would delete most of these one-off options and keep labels consistent.
For Agents
Look at the source-label additions in agent/publisher and the coverage tests. Preserve attribution in scheduler diagnostics, but move the defaulting to an operation-scoped store wrapper, repository helper, or typed query helper so a cohesive operation sets its source once. Replace the AST rule with tests around that boundary.
Source coverage tests do not actually require a source label
What's wrong
These tests are intended to verify query attribution, but they only check that a second argument exists. That gives false confidence for the main observability behavior added by this PR: a query can be unlabelled and still satisfy the test.
Example
A regression like await this.store.query(sparql, { signal }) or await this.store.query(sparql, {}) would still pass these coverage tests, even though the query is no longer attributable by options.source.
Suggested direction
Assert source attribution, not just the presence of an options argument.
For Agents
Update the AST coverage tests in packages/agent/test/query-source-coverage.test.ts and packages/publisher/test/query-source-coverage.test.ts to verify the second argument contains a source property, or explicitly allow only known helper/option objects that include source. Add a failing fixture or assertion that {} / { signal } is reported as missing attribution.
| now: hooks.observability?.now, | ||
| }); | ||
| this.hooks = hooks; | ||
| if (hooks.observability?.register) backpressureRegistry.register(this); |
There was a problem hiding this comment.
🟡 Issue: Backpressure source registration has hidden constructor side effects
What's wrong
The new global registry is now coupled to object construction in one integration and to module import in another. That makes lifecycle, tests, and embedders harder to reason about, especially as more schedulers adopt this abstraction.
Example
Constructing a second PriorityAdmissionQueue with the same scheduler name and register: true throws from inside queue construction, and the unregister handle is discarded. The store scheduler has different semantics: new StorePriorityScheduler() is observable but not globally registered unless callers use the exported singleton.
Suggested direction
Make registry membership explicit and consistent. Prefer registering concrete sources in one bootstrap layer, or inject a registry/unregister owner, instead of letting one scheduler constructor mutate the process-global registry while another relies on module-level registration.
Confidence note
This is a structural concern rather than a proven runtime failure; the current two registered schedulers appear to work, but the lifecycle ownership is inconsistent enough to be worth fixing before more sources are added.
For Agents
Look at PriorityAdmissionQueue registration and externalStorePriorityScheduler registration. Preserve the global store and sync-global sources, but move registration ownership to a daemon/bootstrap registry setup or make constructors return/expose an explicit unregister lifecycle. Add a small test for duplicate construction/teardown behavior if the registry remains process-global.
| * Callers should still pass static operation names rather than graph, peer, or | ||
| * job identifiers. | ||
| */ | ||
| export function normalizeBackpressureLabel(value: string, fallback = 'unknown'): string { |
There was a problem hiding this comment.
🟡 Issue: Operation labels stay stringly where the boundary should enforce bounded classes
What's wrong
The docs and route comments rely on labels being bounded and free of graph/peer/request identifiers, but the type boundary does not enforce that invariant. Different schedulers now solve classification differently, which makes the observability layer harder to extend safely and consistently.
Example
A future caller can pass an operation like durable:did:dkg:context-graph:private:peer-a; the core normalizer will preserve most of that shape rather than reject or classify it. The safety boundary depends on every integration remembering to pre-collapse labels.
Suggested direction
Change the core API so integrations provide a bounded operation class, not arbitrary text that is normalized after the fact. Sanitization can remain a defense-in-depth step, but it should not be the primary contract.
For Agents
Review SchedulerPressureWork, normalizeBackpressureLabel, the sync operation classifier, and store scheduler integration. Preserve current public diagnostic strings, but introduce a typed/bounded operation-class boundary, such as scheduler-owned label enums or a registry of allowed operation classes with an explicit fallback bucket.
| } | ||
|
|
||
| export const externalStorePriorityScheduler = new StorePriorityScheduler(); | ||
| backpressureRegistry.register(externalStorePriorityScheduler); |
There was a problem hiding this comment.
🟡 Issue: Real store scheduler registration is not verified
What's wrong
The new diagnostics surface depends on the store scheduler being registered globally, but the tests only validate snapshot mechanics and a fake route source. That leaves the production wiring unverified.
Example
If backpressureRegistry.register(externalStorePriorityScheduler) is removed, the new /api/diagnostics/backpressure endpoint can omit store scheduler pressure while the current new tests still pass.
Suggested direction
Cover the registry integration for the production store scheduler, not only standalone snapshot generation.
For Agents
Add a focused test near packages/storage/test/store-priority-scheduler.test.ts or the CLI diagnostics tests that imports the production scheduler module and asserts backpressureRegistry.capture().schedulers contains a scheduler: 'store' snapshot. Preserve existing per-instance snapshot tests.
Round 5 —
|
|
|
||
| private observePressureEnqueue(entry: PriorityAdmissionEntry<Payload>): void { | ||
| if (!this.hooks.observability) return; | ||
| this.pressureTickets.set(entry, this.pressureEnqueue(this.pressureWork(entry))); |
There was a problem hiding this comment.
🟡 Issue: Observability label failures can break admission
What's wrong
The new observability path runs a caller-supplied operation callback synchronously during enqueue/reject handling. If that callback throws, the scheduler operation throws from instrumentation code instead of following the existing admission policy, which contradicts the fail-open observability contract and can drop otherwise valid work.
Example
Create a PriorityAdmissionQueue with observability.operation = () => { throw new Error('boom'); }. acquire() now throws 'boom' before admitting work, even if canRun() is true. Expected behavior for observability is that the work still admits/runs and diagnostics fall back or skip the label.
Suggested direction
Wrap operation-label derivation and pressure tracker calls in fail-open try/catch blocks, using a bounded fallback such as the lane or 'unknown' when labeling fails.
Confidence note
Current sync-global usage passes a simple string label, so this mainly affects the new observability hook/API if a labeler or future payload shape misbehaves.
For Agents
In packages/agent/src/sync/priority-admission-queue.ts, contain failures from hooks.observability.operation and the pressure lifecycle calls inside observePressure* helpers. Preserve existing admission/rejection errors and release behavior. Add a focused test with a throwing observability.operation proving acquire/run still follows scheduler policy.
| }; | ||
| server = createServer(async (req, res) => { | ||
| const url = new URL(req.url ?? '/', 'http://127.0.0.1'); | ||
| await handleBackpressureRoutes({ |
There was a problem hiding this comment.
🟡 Issue: The new diagnostics endpoint is not verified through the daemon dispatcher
What's wrong
The only added route test bypasses the production dispatcher, so it verifies handler logic but not the changed HTTP behavior that operators actually use.
Example
A regression that deletes the new handleRequest call to handleBackpressureRoutes would leave GET /api/diagnostics/backpressure returning 404 in the real daemon path, while this test suite still passes because it invokes the route handler directly.
Suggested direction
Add one integration-style test that exercises handleRequest or a started daemon, registers a test backpressure source, requests /api/diagnostics/backpressure with a node-admin token, and expects the snapshot response.
For Agents
Add a dispatcher-level or live-daemon test for GET /api/diagnostics/backpressure using the node-admin token. Keep the existing handler auth assertions, but also prove the route is reachable through packages/cli/src/daemon/handle-request.ts.
Once the idle GC has drained a finalized SWM copy, the immutable-snapshot fallback is the NORMAL path for a late receipt rather than an exception, so its cost has to be bounded and not merely correct. Discovery is a bounded metadata read already; the expensive half is per candidate — a full snapshot read plus digest plus Merkle root — and it ran once per discovered row, up to 16 times on one receipt. The digest filter does not save it: VerifiedGraphScopedFinalizationEvidence carries publicQuadsDigest as OPTIONAL, and with no digest only the triple count discriminates, so every candidate does full payload work. Cap the verifications at MAX_IMMUTABLE_SNAPSHOT_VERIFICATIONS and log truncation. The cap is inert when the digest is known, because candidates are then content-identical and the first match wins. Keep the receipt lane's default priority: receipts are latency-sensitive and demoting them to the background lane trades a CPU spike for receipt starvation under load. Instead make the work attributable — resolveKnowledgeAssetOperationPublicQuads accepted no query options at all, so its reads ran with no source and were invisible to the scheduler observability landed in #2003. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
3 participants
Summary
Introduce a scheduling-policy-neutral backpressure observability layer and
adopt it in the two shared in-memory admission points that currently matter
most for the mainnet symptoms:
StorePriorityScheduler(store);PriorityAdmissionQueue(sync-global).The layer exposes comparable state, bounded operation summaries,
transition/recovery logs, OpenTelemetry metrics, a public state-only
/api/statusprojection, and a node-admin-only detailed diagnostics endpoint.This PR does not claim to fix the upstream Blazegraph slowdown, the atomic
graph-replace 500/deadline, or every symptom in #1989/#1990. It makes the
blocking work and queue state observable so the next incident can distinguish
queued victims from operations actually occupying scheduler capacity. It also
does not change the separate durable finalization inbox introduced by #1939.
Related to #1990 and the closed symptom tracker #1989. Operationally adjacent
to #1939, but not a change to that SQLite inbox.
Why
The current error tells us which operation waited too long:
It does not tell us which older admitted operations occupied the slots, how
long they had been active, whether another lane was full, whether rejection was
a one-off spike or sustained saturation, or when the node recovered.
Scheduler-specific counters exist, but they do not provide one comparable model
across store and sync admission.
That ambiguity is central to #1990: the named
blazegraph.queryis the queuedvictim, not proof that the query itself was the work consuming capacity. The
same limitation applies when the visible symptom is
DROP/INSERTatomic graphreplacement or a sync retry.
Findings and fixes
blazegraph.querytimeout can be misdiagnosed as a slow query even when older replace/update work owns all slotsObservableScheduler,SchedulerPressureTracker,BackpressureSource, and one process-wide registrydkg.backpressure.*OTel instruments with bounded scheduler/lane labels/api/statusstate-only; require the node admin token for/api/diagnostics/backpressure; reject agent tokensdurable,changelog,shared-memory,swm-recovery, orsyncArchitecture
flowchart LR subgraph Producers["Work producers"] SWM["SWM, publish, query, reconcile"] Sync["Sync requester stages"] end subgraph Policy["Existing scheduling policies - unchanged"] Store["StorePriorityScheduler<br/>ack / health / normal / background"] Global["PriorityAdmissionQueue<br/>global sync lanes"] end subgraph Shared["Shared core observability"] Base["ObservableScheduler<br/>lifecycle hooks only"] Tracker["SchedulerPressureTracker<br/>counts, ages, operation summaries"] Registry["BackpressureRegistry"] Monitor["BackpressureMonitor<br/>5 s sample / 60 s sustained summary"] Metrics["OpenTelemetry metrics"] end Public["Public /api/status<br/>state only"] Admin["Node-admin /api/diagnostics/backpressure<br/>bounded details"] Log["daemon.log<br/>transition / summary / recovery"] SWM --> Store Sync --> Global Store --> Base Global --> Base Base --> Tracker Tracker --> Registry Tracker --> Metrics Registry --> Monitor Monitor --> Metrics Monitor --> Log Registry --> Public Registry --> AdminObservableSchedulerdoes not decide admission. Subclasses retain completeownership of priority, FIFO ordering, reservations, displacement, timeouts,
cancellation, handoff, and release. They call protected lifecycle methods only
at their existing boundaries.
Work lifecycle sequence
sequenceDiagram participant Caller participant Scheduler as Existing scheduler policy participant Tracker as Shared pressure tracker participant Monitor as 5-second monitor participant OTel participant Log as daemon.log Caller->>Scheduler: submit static operation class Scheduler->>Tracker: enqueue(lane, operation) Tracker->>OTel: enqueued event alt capacity becomes available Scheduler->>Tracker: start(ticket) Tracker->>OTel: queue-wait histogram + started event Scheduler->>Scheduler: execute existing work closure Scheduler->>Tracker: finish(ticket, outcome) Tracker->>OTel: active-duration histogram + outcome event else queue full or wait deadline expires Scheduler->>Tracker: reject(reason) Tracker->>OTel: rejected event Scheduler-->>Caller: existing busy/timeout error else caller aborts while queued Scheduler->>Tracker: cancel(ticket) Tracker->>OTel: cancelled event end Monitor->>Tracker: snapshot() Tracker-->>Monitor: queue/inflight counts, limits, ages, bounded operations Monitor->>OTel: current gauges opt state transition, sustained summary, or recovery Monitor->>Log: one structured [backpressure] record endWhat an atomic-replace incident now looks like
sequenceDiagram participant Replace as Older graph-replace/update work participant Store as Store scheduler participant Query as Later blazegraph.query participant Monitor participant Operator Replace->>Store: admitted in normal lane Note over Store: inflight slots occupied Query->>Store: queued in normal lane Monitor->>Store: sample pressure Store-->>Monitor: active operation classes + queued query + ages Monitor-->>Operator: degraded transition alt query exceeds queue-wait deadline Store-->>Query: queue_wait_timeout Monitor-->>Operator: saturated transition with rejection count end Replace-->>Store: completes or fails Store->>Store: admit remaining queued work Monitor->>Store: next sample Store-->>Monitor: zero queue / normal active age Monitor-->>Operator: recoveredThis sequence avoids the incorrect inference that the timed-out query was
necessarily the blocker. During the next occurrence, the active summary should
show whether the slots are held by replace/update, count scans, queries, or
another static operation class.
State model
State precedence is
stalled > saturated > degraded > healthy:healthy: no threshold crossed;degraded: queue age crossed the scheduler threshold or a bounded queue isat least 75% utilized;
saturated: a queue is full or a rejection occurred in the last 60 seconds;stalled: the oldest admitted operation crossed the scheduleractive-duration threshold.
A recent rejection remains visible for 60 seconds so a short full-queue event
is not lost between five-second samples. These are evidence states, not causal
classifications.
Operator surfaces
Public status
GET /api/statusadds only:{ "backpressure": { "state": "degraded", "schedulers": [ { "scheduler": "store", "state": "degraded" }, { "scheduler": "sync-global", "state": "healthy" } ], "diagnosticsAvailable": "/api/diagnostics/backpressure" } }Detailed node-admin diagnostics
The route returns queue/inflight counts and limits, oldest ages, cumulative
lifecycle/rejection counts, bounded queued/active operation summaries, and
isolated source-capture failures. Agent-scoped tokens receive
403.Structured logs
The daemon emits
[backpressure]JSON only for:There are no per-enqueue or per-start log lines.
Common metrics
dkg.backpressure.queue_depthdkg.backpressure.queue_limitdkg.backpressure.inflightdkg.backpressure.inflight_limitdkg.backpressure.oldest_queued_age_msdkg.backpressure.oldest_active_age_msdkg.backpressure.events_totaldkg.backpressure.queue_wait_msdkg.backpressure.active_duration_msCurrent gauges use only bounded
schedulerandlanedimensions. Operationnames are deliberately excluded from metrics and retained only in capped
log/diagnostic summaries.
Privacy and cardinality boundaries
lane;
tracker;
A regression test queues labels containing CG and peer fragments and proves
they do not appear in the registry snapshot.
Scheduling invariants
This PR intentionally does not change:
deadlines;
caps;
Local microbenchmark
Indicative developer-machine benchmark on Node
v25.2.1, with the defaultno-op OTel provider after warm-up:
enqueue -> start -> finishlifecycleThe full snapshot is sampled once every five seconds in the daemon, not on
each admission transition. This benchmark is an indicative regression check,
not a CI performance gate.
Validation
pnpm --filter @origintrail-official/dkg-core exec vitest run test/backpressure-observability.test.tspnpm --filter @origintrail-official/dkg-storage exec vitest run test/store-priority-scheduler.test.tspnpm --filter @origintrail-official/dkg-agent exec vitest run --config vitest.unit.config.ts test/sync-backpressure.test.tspnpm --filter @origintrail-official/dkg exec vitest run --config vitest.unit.config.ts test/backpressure-route.test.ts test/status-route-store-quads.test.ts test/metrics-presence.test.tsDKG_SKIP_EVM_BUILD=1 pnpm --filter @origintrail-official/dkg... buildtests, and package-root checks
git diff --cached --checkThe repository-supported EVM-build skip was used because this PR does not
touch the EVM module.
Review focus
degraded,saturated, andstalledthresholds useful as evidencewithout implying root cause?
enough?
transitions, especially sync displacement/handoff?
BackpressureSource, while keeping its retry policy outsideObservableScheduler?Caller/provider attribution
The first observability pass could still collapse otherwise unrelated store
work into
blazegraph.query. This update gives direct store callers bounded,static source labels and propagates caller intent across provider boundaries.
It remains diagnostics-only: it does not change admission, priority,
concurrency, deadlines, retry behavior, or SPARQL.
Findings from repeated devnet profiles
blazegraph.queryagent.query.privateGraphAccessPolicygetContextGraphOnChainIdto accept a caller source, useagent.contextGraph.onChainIdas its bounded fallback, and passagent.vmReconcile.resolveOnChainIdfrom VM reconcileGraphManager.listContextGraphs(options)and passagent.swmHostMode.listContextGraphsThe labels contain no graph, UAL, job, peer, request, or SPARQL payload. They
are fixed operation classes and therefore preserve the privacy/cardinality
boundary described above.
Attribution sequence
sequenceDiagram participant Producer as API, VM reconcile, promote worker participant Provider as Agent/provider helper participant Scheduler as Store scheduler participant Proxy as Delay proxy participant BG as Blazegraph participant Profiler as 100 ms sampler Producer->>Provider: invoke operation Provider->>Scheduler: query/update with static source Scheduler->>Scheduler: enqueue/start ticket with lane + source Scheduler->>Proxy: admitted SPARQL Proxy->>Proxy: inject 1,200 ms latency Proxy->>BG: forward request loop every 100 ms Profiler->>Scheduler: GET admin backpressure diagnostics Scheduler-->>Profiler: active operations grouped by lane + source Profiler->>Profiler: integrate active count × sample interval end BG-->>Scheduler: result Scheduler->>Scheduler: finish ticketRealistic 3 MiB Hardhat devnet profile
This section records the corrected current-head rerun after the expanded
caller/provider labels. The tested PR head was
8931c89.
The earlier run is retained below only as an attribution baseline. Its
flamegraph is pinned to the older commit and must not be read as the
current-head distribution.
Topology and exact workload
needed for finalization; node 3 was the measured lagging node.
to every Blazegraph request.
3,145,728 N-Quads bytes (3.000000 MiB).
3,148,321 bytes for the offline assets. Persisted StorageACKs reported
publicByteSize=3,145,891.
assets finalized through the other peers, and node 3 was restarted under
injected store latency for durable sync and VM reconciliation.
503 no_publisher_wallets. It was retained separately and is excluded from
every number below.
Measurement definition
The profiler sampled the node 3 admin backpressure endpoint every 100 ms.
Slot time is therefore sampled concurrent occupancy: approximately invocation
wall time summed across active invocations. It is not invocation count and
it is not CPU time. Parallel operations contribute multiple slot-seconds,
so total slot time can exceed profile wall time.
For this run:
(first at 148.992 s; last at 252.148 s)
Thus the store scheduler — 978.783 slot-seconds value is the subtotal of
all sampled active store slots. Its 58.95% share is
978.783 / 1,660.277, not the percentage of API calls.
Initial attribution baseline
The image above is pinned to commit
0b1d24a and represents the initial
pre-expansion run, not the corrected current-head rerun.
The workload and delay settings were preserved and total observed occupancy is
nearly identical. Runtime scheduling remains nondeterministic, so this comparison
does not claim that every removed generic second maps one-to-one to a newly
named bucket. It does show that the added provider labels materially split the
previously ambiguous work.
Current scheduler and lane distribution
Largest observed buckets:
The remaining generic blazegraph.query bucket is still significant. Source
attribution is improved, not complete.
Observed pressure
Queued slot time was:
Observed state time was 367.817 s healthy, 61.814 s degraded, and 59.939 s
saturated for the store scheduler. Sync-global spent 227.172 s healthy and
262.398 s degraded.
Node 3 emitted exactly two StorageACK declines with
CORE_TEMPORARILY_UNAVAILABLE after the ACK handler exceeded its 15,000 ms
deadline. At those points the normal lane had eight queued operations while
the ACK lane itself had no queued work. This is direct evidence that an ACK
handler can miss its deadline while waiting on work admitted through the
normal store lane; it is not evidence of an ACK-lane queue backlog.
Two transient context-graph list calls returned HTTP 500 with:
Both occurred during delayed-store boot/recovery.
Functional result and reconcile transport failure
Before node 3 was stopped it had 108 VM and 108 SWM triples. After two more
assets finalized while it was offline, node 1 had 216 VM triples.
The original synchronous POST /api/context-graph/reconcile request ran for
302.148 s and ended with fetch failed, so its immediate harness receipt was
success=false. This was a transport/request-lifetime failure, not a convergence
failure: daemon evidence showed all four ordinals processed.
After removing injected delay, an independent read found:
A follow-up reconcile returned HTTP 200 in 621 ms with status=current,
attempted=false, headOrdinal=4, watermarkBefore=4, watermarkAfter=4,
reconciledOrdinals=0, and unresolvedOrdinals=0.
The network therefore converged, but the long-lived synchronous API request
crossed an approximately five-minute transport/server boundary and reported
failure to its caller after the underlying recovery succeeded.
What should change next
These are evidence-driven follow-ups, not behavior changes contained in this
merged observability PR.
The highest-confidence functional fix exposed by this run is the asynchronous,
durable reconcile API. The next correctness-sensitive fix is ACK-path isolation.
Polling reduction is the largest obvious throughput opportunity in the named
store work, but should be validated with production cadence before changing
policy.
Proposed reconcile lifecycle
sequenceDiagram participant Client participant API as Reconcile API participant Queue as Durable reconcile job participant Worker participant Store participant Status as Job status Client->>API: POST reconcile(context graph) API->>Queue: enqueue or join idempotent job Queue-->>API: job token API-->>Client: 202 Accepted + token Worker->>Queue: claim bounded work loop ordinal by ordinal Worker->>Store: sync and materialize Worker->>Status: persist watermark and progress end Worker->>Status: current or retryable failure loop until terminal Client->>API: GET reconcile job status API->>Status: read durable progress Status-->>Client: queued, running, current, or failed endClient disconnects then stop being conflated with reconciliation failure, while
the durable worker can preserve fairness, retries, and progress independently
of HTTP timeouts.
Harness corrections
The rerun also established these harness requirements:
watermark verification after a request timeout.
from scheduler/API failures.
before the child JVM was durably owned, and stopping the wrapper could leave
the Java child alive.
Additional validation for caller labels