fix(observability): classify shared-pool sync lanes on the queue they draw on - #2107
Conversation
… draw on `GET /api/diagnostics/backpressure` classifies a lane's depth against a per-lane queue limit. The sync admission queue publishes none — its lanes order one shared queue by priority rather than partitioning it — so both depth-based branches were dead for `sync-global` and a sync lane only left `healthy` after 15 s of queue age, 120 s of active age, or a rejection that had already happened. Lane `state` was a trailing indicator. A scheduler now declares how its lanes divide capacity. `partitioned` (the default, and the store scheduler) keeps today's behaviour exactly: the classifier evaluates the same expressions, and the added `queued.length > 0` guard is implied by the comparisons it guards. `shared` resolves a lane's ceilings from the scheduler-level limits and measures depth against the pool, charged only to lanes that hold queued work — so every lane waiting behind a full sync queue reports `saturated` at the moment it fills, and `degraded` at 75%, before anything is rejected. An idle lane stays `healthy`. The `/api/status` rollup cannot move: for an all-shared scheduler a lane's new predicate is the predicate the totals already evaluate, so `maxState` over lanes can only reproduce a state the rollup was going to report anyway. The same identity means a shared scheduler's lanes now match its rollup, which suppresses its `"lane":"all"` log record — shared lanes therefore carry `poolQueued` beside `queued` so the pool's depth stays on the line. Fixes #2075. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // its own backlog when the allocation is private, the whole pool when the | ||
| // ceiling is shared. Under `partitioned` this is `queued.length`, so the | ||
| // classifier below is unchanged for every scheduler that owns its lanes. | ||
| const boundedDepth = shared ? this.queued.size : queued.length; |
There was a problem hiding this comment.
🟡 Issue: Expose the pressure depth instead of mixing lane-local queued with pool capacity
What's wrong
The implementation fixes classification by adding an internal boundedDepth, but the public lane row still pairs a lane-local queued value with a pool-wide queueLimit. That overloaded shape is what forces the new log special case and is likely to spread more capacityModel === 'shared' branches into every consumer that wants to explain or visualize lane pressure.
Example
For a shared pool with four queued items and one item in changelog, the lane snapshot exposes queued: 1 and queueLimit: 4 but classifies using boundedDepth: 4. Any new consumer that computes lane utilization from the row sees 25%, while the state is saturated; the monitor already needs a bespoke poolQueued branch to explain that mismatch.
Suggested direction
Make the snapshot carry both concepts explicitly: lane-local attribution depth and the depth/capacity pair used for pressure classification. Then metrics, logs, diagnostics, and future consumers can read one canonical model instead of learning the shared-pool exception independently.
For Agents
In SchedulerPressureTracker.laneSnapshot and BackpressureMonitor.message, introduce an explicit lane pressure descriptor, for example pressureQueued/pressureQueueLimit or a nested capacity object containing the numerator used for state classification. Preserve lane-local queued for attribution and preserve existing state behavior; update the shared-pool tests to assert the explicit pressure depth and have logging read that field rather than reconstructing it from scheduler totals.
There was a problem hiding this comment.
🟡 Issue: Capacity-model policy is threaded through the lane classifier
What's wrong
laneSnapshot was already responsible for collecting queued/active work, computing ages, classifying state, and building the DTO. This change embeds the new capacity-model semantics directly into that busy path with several coordinated branches. The behavior is understandable now, but the abstraction boundary is weak: the classifier has to know how every capacity model maps limits and pressure depth, which makes the next model or rule change more error-prone and harder to review.
Example
Today the model check fans out through queueLimit, inflightLimit, boundedDepth, depthCeiling, and pressureQueued. Any future capacity model or change to shared-lane idle handling would require editing this classifier in multiple places.
Suggested direction
Move the model-specific logic behind a small helper, e.g. capacityForLane(lane, laneQueuedCount), or normalize SchedulerPressureCapacity in updateCapacity. Have it return capacityModel, queueLimit, inflightLimit, pressureQueued, and whether depth pressure applies, so laneSnapshot can remain a model-agnostic state classifier.
For Agents
Look in SchedulerPressureTracker.laneSnapshot. Preserve current shared-pool behavior and partitioned behavior, but extract or normalize lane capacity into one descriptor before classification. Existing shared/partitioned tests should continue to prove the same states and snapshot fields.
There was a problem hiding this comment.
Agreed — this was the weakest part of the design and the diagnosis is exact. Fixed in 49d0744.
The lane row now publishes both concepts:
| Field | Meaning |
|---|---|
queued |
this lane’s own backlog — the attribution signal |
pressureQueued |
the depth state was classified against, and the numerator that belongs with queueLimit |
pressureQueued equals queued for a private allocation and the pool’s depth for a shared one, so utilization is pressureQueued / queueLimit on every scheduler and nothing downstream learns a shared-pool exception. Your worked example now reports queued: 1, pressureQueued: 4, queueLimit: 4 alongside state: saturated — coherent on its own, with no cross-reference to totals.
The log special case is gone with it: BackpressureMonitor.message reads the published pressureQueued (falling back to queued for a source that predates it) instead of reaching into scheduler.totals.queued, and emits it only when it differs from queued — so the branch is now value-based rather than model-based, and every store record stays byte-identical. poolQueued never shipped outside the first commit of this PR.
Both new fields are optional, per the sibling capacityModel thread.
There was a problem hiding this comment.
🔴 Bug: Idle shared lanes publish pool pressure they were not classified against
What's wrong
The new field is documented as the depth used to classify the lane and as the utilization numerator to pair with queueLimit. For shared schedulers, empty but previously-known lanes are deliberately kept healthy even when the pool is full, but this return value still publishes the full pool depth. API consumers now see a healthy lane with full utilization and must special-case queued === 0, which contradicts the new contract and can create false diagnostics for lanes that are not actually waiting.
Example
With the new sync test shape: one swm_recovery item is active, durable and changelog each have one queued item, and queueLimit is 2. The swm_recovery lane correctly reports state: "healthy" and queued: 0, but this line also reports pressureQueued: 2 and queueLimit: 2, so pressureQueued / queueLimit says 100% utilization for a lane whose depth was not classified and has no waiting work.
Suggested direction
Derive a separate published pressure depth from the guarded classification input, for example falling back to the lane’s own queued.length when the lane has no queued work, instead of returning the pool depth unconditionally.
For Agents
In SchedulerPressureTracker.laneSnapshot, keep the empty-lane guard and make the published pressureQueued match the same depth actually used for lane depth classification. Add/adjust a shared-pool test where a known lane has queued: 0 while the pool is full, and assert it remains healthy with a non-misleading pressureQueued value, likely 0 or equal to queued.
Keep pressureQueued aligned with the depth that actually classifies the lane
What's wrong
The new snapshot contract says pressureQueued is the depth the lane state was classified against and the numerator paired with queueLimit, but the implementation returns the shared pool depth even when the classifier intentionally ignored that depth because the lane has nothing queued. That makes the exported row self-contradictory and reintroduces a hidden rule consumers must know: for shared lanes, pressureQueued only means pressure when queued > 0. This weakens the new abstraction exactly where it is supposed to remove consumer special-casing.
Example
In the new shared-pool scenario, swm_recovery can have state: 'healthy', queued: 0, queueLimit: 2, but still emit pressureQueued: 2 because the shared pool is full. A generic consumer following the new pressureQueued / queueLimit rule would display that healthy idle lane as 100% utilized.
Suggested direction
Make pressureQueued mean the effective classifier numerator, for example queued.length === 0 ? queued.length : shared ? this.queued.size : queued.length. If the raw shared pool depth is useful even for idle lanes, expose it under a separate name such as poolQueued instead of overloading pressureQueued.
For Agents
Update SchedulerPressureTracker.laneSnapshot so one effective queue-pressure value is computed after the idle-lane guard and reused for both classification and the snapshot. Preserve shared lanes with queued work being classified by pool depth and partitioned lanes being classified by their own depth. Add/adjust a test proving an idle shared lane emits pressureQueued as 0/queued or omits it while remaining healthy, and queued shared lanes still degrade/saturate from pool depth.
There was a problem hiding this comment.
Re the nested "Capacity-model policy is threaded through the lane classifier" — same timing race as the sibling thread: you posted it ~12 s before my reply, so I resolved over it. Reopening in substance. Fixed in 5d9e8b6, taking your suggested direction almost verbatim.
laneCapacityFor(lane, laneQueued) now owns the whole capacity decision and returns {capacityModel, queueLimit, inflightLimit, pressureQueued, depthCeiling}. laneSnapshot destructures that and is a model-agnostic classifier again — the five coordinated locals you counted (queueLimit, inflightLimit, boundedDepth, depthCeiling, pressureQueued) are gone from it, and shared is not referenced there at all. Adding a model means extending one descriptor rather than editing branches.
depthCeiling carries the "does depth pressure apply" bit you asked for, so the idle-lane rule also lives with the model rather than in the classifier.
Behaviour is unchanged and that is checked, not asserted: the same 12 tests pass, and all three mutants were re-run against the new structure — pool-denominator, idle-lane guard, and log-field — each still killed by exactly the tests that killed it before the refactor. Store lanes and the /api/status rollup are covered by the two dedicated partitioned tests.
There was a problem hiding this comment.
Two comments landed here since my last reply — taking them separately.
🔴 "Idle shared lanes publish full-pool pressureQueued" (07:03:17) — already fixed at the commit you reviewed.
This review is recorded against 52e107f8a, which is the commit that fixes it (pushed ~07:02, four minutes before the review). Running your exact scenario against that build — queueLimit: 2, one active swm_recovery, one queued each in durable/changelog:
{"lane":"changelog","state":"saturated","queued":1,"pressureQueued":2,"queueLimit":2}
{"lane":"durable","state":"saturated","queued":1,"pressureQueued":2,"queueLimit":2}
{"lane":"swm_recovery","state":"healthy","queued":0,"pressureQueued":0,"queueLimit":2}
The idle row reports pressureQueued: 0, not 2. Both suites assert it, and reintroducing the old expression is a mutant that kills those assertions and nothing else. Reads like the finding was re-derived from the full base...head diff rather than the tip — flagging it rather than re-fixing.
🟡 "Separate depth classification from capacity reporting" (07:07:50) — agreed, fixed in d1c11aa.
You are right that depthCeiling: number | null was carrying three meanings, and that the invariant lived in comments instead of the type. It is now:
depthPressure: { queued: number; limit: number } | nullEither depth applies — with both of its terms — or it does not. The reported ceilings (queueLimit/inflightLimit) stay separate, exactly as you suggested, and pressureQueued now falls out of the pair (depthPressure?.queued ?? queued.length) rather than being re-derived from a sentinel. Both classifier branches read the pair, so the "no usable limit" and "idle lane" cases are no longer distinguishable only by comment.
Behaviour is unchanged and checked: the same 12 tests pass, and all three mutants were re-run against the new structure — idle-lane pool depth, always-lane-local depth, and the log field — each still killed by exactly the tests that killed it before.
There was a problem hiding this comment.
🟡 Issue: Shared lane metrics pair lane backlog with the pool limit
What's wrong
For shared lanes, this change reports the pool limit on each lane while leaving the existing metrics exporter to record the lane's own backlog as queue depth. The JSON/log path gained pressureQueued, but metrics did not, so Prometheus/OpenTelemetry dashboards that combine dkg.backpressure.queue_depth and dkg.backpressure.queue_limit now see an internally inconsistent pair and can miss the pressure condition this PR is meant to surface.
Example
With a shared sync-global pool of 4 and three lanes holding one queued item each, diagnostics reports each active lane as degraded with pressureQueued: 3 and queueLimit: 4. The exported lane metrics still become queue_depth=1 and queue_limit=4, so a metric-only utilization alert reads 25% instead of the intended 75%. A full pool spread across four lanes would similarly read 25% while the lane state is saturated.
Suggested direction
Update the metric contract alongside the snapshot contract. Either emit a separate pressure-depth gauge, record pressureQueued for the existing pressure utilization metric while keeping attribution elsewhere, or include enough model/numerator data in metrics so queue_depth / queue_limit does not silently underreport shared-pool pressure.
For Agents
Look at packages/core/src/backpressure-observability.ts: the lane snapshot now exposes pressureQueued, but recordBackpressureSnapshotMetrics still emits only queued against the lane queueLimit. Preserve lane-local backlog attribution, but add/export the pressure numerator for shared lanes or otherwise avoid publishing an incompatible depth/limit pair. Add a metric-focused test for a shared pool spread across lanes proving metric consumers can observe the same degraded/saturated pressure as diagnostics.
There was a problem hiding this comment.
🟡 Issue: The new pressure depth model stops short of the metrics boundary
What's wrong
The PR’s model says consumers should use pressureQueued / queueLimit, but only snapshots and log messages get that value. Metrics still publish lane queued beside the shared pool queueLimit, so the observability abstraction is split across surfaces and future consumers have to relearn the shared-lane special case.
Example
For a shared pool with queueLimit=4 and four queued items spread across lanes, a lane can report queued: 1, pressureQueued: 4, queueLimit: 4, and state: saturated. The snapshot/log row is self-consistent, but the lane metric pair still looks like queue_depth=1 and queue_limit=4.
Suggested direction
Treat pressureQueued as a first-class normalized field at the observability boundary. Either add a dedicated pressure-depth gauge or centralize lane normalization so metrics, logs, and diagnostics do not each need their own interpretation of queued versus pressure depth.
For Agents
In packages/core/src/backpressure-observability.ts, route snapshot/log/metric consumers through one normalized lane shape. Preserve queued as attribution, but expose/record the pressure depth separately or make the metric contract explicitly use it. Add a metric-focused test for a shared lane where queued !== pressureQueued.
There was a problem hiding this comment.
Both metrics findings (09:04:19 and 09:04:20 — same defect) are correct, and it is one this PR introduced. Fixed in 47c6bc2.
Before this change a shared lane published queueLimit: null, so recordBackpressureSnapshotMetrics never recorded the limit gauge for it — there was no pair to be inconsistent. Publishing the pool ceiling per lane created one, and I extended the numerator to the snapshot and the log line but not to metrics. Your worked example is exact: queue_depth=1 against queue_limit=4 while the lane is degraded at 75%. That is the surface W1 alerting actually reads (tools/observability/w1/w1-rules.yaml), so it is the worst of the three to have left behind.
New instrument, dkg.backpressure.pressure_depth{scheduler,lane} — the numerator that belongs with queue_limit:
| gauge | meaning |
|---|---|
queue_depth |
this lane own backlog — attribution, unchanged |
pressure_depth |
the depth the lane state was classified against |
queue_limit |
the ceiling that depth was measured against |
So utilization is pressure_depth / queue_limit on every scheduler, and on a partitioned lane the two depths are equal — no model-specific branch for a metric consumer, which is the "one normalized lane shape" the second comment asks for. The rollup row (lane="all") reports its own depth as its pressure.
Test added as requested: a shared pool spread across three lanes, asserting queue_depth=1, pressure_depth=3, queue_limit=4 per lane and pressure_depth / queue_limit >= 0.75 alongside the degraded state, plus a partitioned lane where both depths are 1. Exporting the lane backlog as pressure_depth is a mutant that kills only that test.
One note on how it is tested, since it bit me: the test binds a stand-in meter provider rather than patching the instrument objects. With no global provider registered the OTel API hands back one shared no-op instrument, so stubbing backpressureQueueDepth.record stubs every gauge — my first attempt captured 24 records across all of them and asserted against the wrong thing. Binding a provider also pins the published metric names, which is the part a dashboard actually depends on.
There was a problem hiding this comment.
Both comments in this thread are addressed at abcbc663e, which landed after each was written. Recording where, rather than resolving — that's the author's call.
The original comment (2026-08-05) asks for "an explicit lane pressure descriptor, for example pressureQueued". It quotes boundedDepth and poolQueued, identifiers from a revision that no longer exists. pressureQueued is now a public field on BackpressureLaneSnapshot (backpressure-observability.ts:96-104), set at :500, and read by BackpressureMonitor.message at :873 rather than reconstructed from scheduler totals — which is what the comment asked for. queued is preserved as the lane-local attribution signal.
The newest comment (2026-08-06 09:04, against the previous head) is the metrics half, and it is right — I raised the same thing independently. It shipped fixed ~40 minutes later:
dkg.backpressure.pressure_depth(telemetry-api.ts:214,:410), recorded atbackpressure-observability.ts:703asvalues.pressureQueued ?? values.queued. This is the comment's first suggested direction — "emit a separate pressure-depth gauge" — withqueue_depthleft as the lane's own backlog for attribution, as it asked.- The same treatment for concurrency:
dkg.backpressure.pressure_inflight(telemetry-api.ts:224,:420), recorded at:705. - The metric-focused test the comment asks for exists at
packages/core/test/backpressure-observability.test.ts:617. It pins values and metric names, not just that an instrument fired: for a shared pool of 4 with three lanes holding one item each — the comment's own example — it assertsqueue_depth = 1alongsidepressure_depth = 3againstqueue_limit = 4, i.e. the 75% the comment says a metric-only alert should have seen instead of 25%. A partitioned row asserts the two gauges equal.
Verified by execution against the pinned head rather than by reading: at pool capacity the exported ratio is 1.00 where the lane state is saturated, and 0.75 where it is degraded; on the store scheduler pressure_depth == queue_depth on all five rows with zero exceptions. The ?? values.queued fallback keeps a hand-built BackpressureSource written against an older dkg-core working unchanged.
One residue worth carrying forward, raised in my round-2 review: pressure_inflight and the pressureInflight field have no test coverage anywhere in the repo — reverting :506 to active.length leaves both suites green. The depth half is pinned; the inflight half is not.
…city, log test Four review findings from otReviewAgent on #2107. 1. `capacityModel` was a required field on an exported snapshot type, so a hand-built `BackpressureSource` written against an older `dkg-core` stopped compiling for a scheduler that is still plainly partitioned. It is now optional, absent meaning `partitioned`; the tracker still always emits it. The monitor test's source is deliberately left in the pre-`capacityModel` shape so the old shape stays exercised. 2. `SchedulerPressureCapacity` allowed a shared pool to carry private `lanes`, which the implementation silently ignored. It is now a discriminated union where the shared arm forbids `lanes`, so a contradictory capacity cannot be published. Verified with tsc: the illegal shape is rejected and all three legal shapes still compile. 3. The lane row paired a lane-local `queued` with a pool-wide `queueLimit`, so a consumer computing utilization saw 25% next to `saturated`, and the log needed a bespoke shared-only branch. The row now also carries `pressureQueued` — the depth the state was classified against, equal to `queued` under `partitioned` and the pool's depth under `shared`. Utilization is `pressureQueued / queueLimit` everywhere, and the monitor reads the published field instead of reaching into scheduler totals, emitting it only where it differs from `queued`, which leaves every store record identical. `poolQueued` never shipped; it is `pressureQueued` now. 4. Nothing exercised the monitor's emission path, so a regression that dropped the field would have left every test green. A BackpressureMonitor test now asserts the emitted JSON for a shared and a private lane of the same scheduler, and that the `lane: all` record is absent once a lane matches the rollup. Verified by mutation: deleting the emission kills only that test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The capacity input called it `laneCapacity` while the snapshot reported it as `capacityModel`, so a reviewer reading the declaration and the payload met two names for one concept. Both are `capacityModel` now, as asked in review. Pure rename: no behaviour, no test expectation, and no emitted value changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lift the model out of the classifier Two findings that arrived as nested replies on existing threads. The 75% shared-pool band was only proven with the pool already full, where the pool depth and a lane's own depth agree often enough to hide a regression. The reviewer named the exact surviving mutant — saturation on pool depth, the degraded branch back on `queued.length` — and it did survive all 11 tests. `opens the 75% band on a shared pool no single lane is anywhere near filling` is the shape where the two denominators disagree: three lanes holding one entry each against a ceiling of four, so the pool is at 75% while every lane is at 25%. It fails against that mutant and passes without it. `laneSnapshot` had grown the model's semantics across five coordinated locals, so the next model would have to be threaded through the classifier again. `laneCapacityFor` now returns the whole capacity decision — model, both ceilings, the depth to classify against, and whether depth pressure applies — and `laneSnapshot` is model-agnostic once more. No behaviour change: the same 12 tests pass, and all three mutants were re-run against the new structure, each still killed by exactly the tests that killed it before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported as a 🔴 on #2107, and correct. `pressureQueued` is documented as the depth the lane's state was classified against and the numerator to pair with `queueLimit`, but a shared lane with an empty backlog published the pool's whole depth — while the classifier had deliberately ignored that depth and left the lane `healthy`. A consumer following the documented rule would render a lane with nothing waiting as fully utilized, and would have to special-case `queued === 0`, which is exactly the consumer-side branching the field exists to remove. The reviewer's example came from this PR's own test shape, where `swm_recovery` sits healthy and idle beside a full pool. The published value is now taken after the idle-lane guard, from the same `depthCeiling` that decides whether depth applies at all, so it is always the number the classifier used: the pool's depth for a shared lane with work waiting, its own backlog otherwise. Both the core and the sync test now assert `pressureQueued: 0` on the idle lane, and reintroducing the bug is a mutant that kills that assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // `oldestActiveAgeMs` are 0) and nothing is ever rejected, so queue depth is | ||
| // the only signal that can move a lane off `healthy`. Without that, a lane | ||
| // state assertion here would pass for the wrong reason. | ||
| it('classifies a shared-pool lane against the pool depth, not its own share of it', () => { |
There was a problem hiding this comment.
💡 Suggestion: Table-drive the capacity-model matrix instead of growing bespoke scenario tests
Why it matters
The new coverage is valuable, but the test file more than doubles and repeats the same setup/assertion structure. A table-driven shape would make the actual model differences easier to scan and reduce future edit points when lane snapshot fields change.
Suggestion
Extract small helpers like trackerWith(capacity), enqueueLanes(tracker, entries), and a table of {name, capacity, work, expected} cases for the shared/partitioned pressure matrix. Keep one dedicated test only where the setup truly differs, such as active idle-lane handling.
There was a problem hiding this comment.
Thanks — declining this one, with reasons, and taking the half of it that applies.
Where I agree: the monitor test had the repeated-setup problem you describe, and it already uses a laneRow(lane, queued, extra) helper for exactly that. Its scheduler shape also just changed in d1c11aa (two realistic sources instead of one hybrid), so the duplication there is down.
Where I would rather not go table-driven: these cases are not the same assertion over varying data — each one exists to kill a different way the fix could be wrong, and the reason is the payload:
| Test | The mutant it exists to kill |
|---|---|
| shared / spread | pool denominator → per-lane depth (a concentrated-shape test survives this, which is the whole reason the spread shape is there) |
| shared / concentrated | the issue as literally filed |
| distributed 75% | saturation on pool depth while the degraded branch regressed to per-lane — this one survived 11 tests until it was written |
| idle lane | the laneQueued > 0 guard, and the idle row publishing pool depth |
| partitioned, own allocation | shared semantics leaking into store |
| partitioned, rollup full / lanes not | the pool denominator leaking in via the totals path |
Folded into a {name, capacity, work, expected} table, those six rationales become a column of names, and the next person to edit a lane snapshot field sees a data change rather than "this row is the only thing standing between us and a silently inert fix". Each case also carries a comment naming the trap, which is what made the distributed-75% gap findable in the first place. The setup they share is three lines; the reasoning they do not share is the point.
I have kept the shared-setup reduction (laneRow, and constants rather than repeated literals) and left the case bodies explicit. If a seventh and eighth shared/partitioned case land, that calculus changes and a table is the right answer — happy to revisit then.
…l capacity model Two review findings on 52e107f, both about the shape of the boundary rather than behaviour. No state, no published value, and no test expectation changes. `depthCeiling: number | null` was doing three jobs — the reported ceiling, whether depth classification applies, and the idle-lane exemption — so a maintainer touching one had to preserve an invariant only the comments explained. It is now `depthPressure: { queued, limit } | null`: either depth applies, with both of its terms, or it does not. `pressureQueued` falls out of it (`depthPressure?.queued ?? queued.length`) rather than being derived from a sentinel, and both classifier branches read the pair. The capacity model is a scheduler invariant, but only the lane rows carried it, so a snapshot could describe one scheduler with lanes in different models — and the monitor test exercised exactly that impossible shape. `BackpressureSnapshot` now carries `capacityModel` as the authoritative value (optional, absent means partitioned, like the lane copy), the lane copy stays as derived data so a row still explains its own `queueLimit` when it travels alone in a log line, and the monitor test registers two realistic schedulers — one wholly shared, one wholly partitioned — instead of one hybrid. All three mutants re-run against the new structure and are still each killed by exactly the tests that killed them before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI note — the two red checks on this PR are pre-existing on the base branch, not from this change.
The same job fails identically on Same file, same test, same assertion, same 1/331/3 counts as this PR run. It also passes locally on this branch ( Every lane that covers this change is green: the core lane, the other nine agent shards, all four CLI shards, storage, and query. I have not touched it — it is the base branch's to fix, and I could not find an open issue tracking it. Happy to file one if it is not already known. |
|
Correction and follow-up to my previous CI note. I re-ran the two failed jobs on the same commit — they fail again, identically. It is not a flake. My earlier "looks shard- or environment-dependent rather than deterministic" was wrong; scratch that line. The rest of that comment stands, and the picture is now sharper. It is a deterministic breakage on
The window Why this PR is not involved, two independent ways:
Where it goes wrong ( That belongs to #2099/#2101, not here — I have not touched it. Happy to open an issue against it if one is not already tracked; say the word and I will. Every lane that covers this change is green: core, the other nine agent shards, all four CLI shards, storage and query. |
Merge-readiness review, three findings on d1c11aa. Two of them are the same defect, and it is one this PR introduced. Before this change a shared lane published `queueLimit: null`, so the limit gauge was never recorded for it and there was no pair to be inconsistent. Publishing the pool's ceiling per lane created one: `recordBackpressureSnapshotMetrics` still emitted `queue_depth` as the lane's own backlog, so a shared lane exported `queue_depth=1` against `queue_limit=4` and a metric-only utilization alert read 25% while the lane was classified `degraded` at 75% — underreporting exactly the pressure this PR exists to surface, on the surface W1 alerting actually reads. `dkg.backpressure.pressure_depth{scheduler,lane}` now carries the numerator that belongs with `queue_limit`. `queue_depth` is unchanged and remains the attribution signal; on a partitioned lane the two are equal, so no consumer needs to know the model. Snapshot, log line and metrics now expose the same normalized shape. The third finding is a comment that lied: `SchedulerPressureCapacity` told maintainers that partitioned lane allocations "add up to the scheduler's ceiling", while nothing validates that and this PR's own store test deliberately uses a scheduler ceiling below the sum. The scheduler-level limit is documented as an independent rollup ceiling now, with the sum language dropped. Test: metric emission is captured through a stand-in meter provider, which also pins the published instrument NAMES. Patching the instrument objects does not work — with no global provider the API returns one shared no-op instrument, so stubbing one gauge stubs them all, and the first version of this test silently captured 24 records across every gauge. Exporting the lane backlog as `pressure_depth` is a mutant that kills only this test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Verification reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
Jurij89
left a comment
Jurij89
left a comment
There was a problem hiding this comment.
Independent review at d1c11aad7
No blockers. The core fix is correct, and all four load-bearing invariants survive execution rather than argument — I built a differential harness that loads the base and head blobs byte-identically and ran them side by side under a frozen clock. Three things I would address before merge, all cheap, plus a set of claim/code mismatches that matter because the CHANGELOG bullet becomes release notes.
CI red is not attributable to this PR
Tornado: agent [3/10]— the single failure ispackages/agent/test/publish-finalized-agent-lane.test.ts:238,expected [] to have a length of 1 but got +0(331 passed / 1 failed / 3 skipped). The same file, test name, assertion and tally fail on the merge base82ddb03584fdwithout this PR (push run 31061793199, job 92491484234), and again on the newer base head9cc29ca8de22, where re-sharding moved the same failure to[7/10]. That is the #1786 author-selectable VM publish area; none of the six changed files is on its path.CI gate— job 92553933752 contains no test output at all. Its only errors aretornado-agent ended with failure/Process completed with exit code 1. It is the downstream aggregate, and it is red ontestnet-canaryat both base heads.
This PR's own lanes are green: Tornado: core + RDF + storage shows ✓ test/backpressure-observability.test.ts (12 tests) inside 106 passed / 1673 passed.
What holds — verified by execution, not reading
| Claim | Verdict | Evidence |
|---|---|---|
(1) /api/status rollup cannot change for an all-shared scheduler |
HOLDS | 0 differences in snapshot().state or totals across 172,800 exhaustive type-legal shared shapes, 288,000 including pathological thresholds, and 30,000 randomized op sequences with mid-run updateCapacity |
(3) sumLaneLimits can never sum a pool ceiling |
HOLDS | totals.queueLimit/inflightLimit were always exactly normalizeLimit(pool) or null — never a multiple — across all 288,000, including every mixed set/null combination |
(4) "lane":"all" is unreachable for a shared scheduler |
HOLDS | 0 reachable hits; the only hits (2,016/288,000) need an empty scheduler with a zeroed age threshold, and sync-global sets 15,000/120,000. Store [backpressure] records byte-identical in 500/500 randomized 40-step monitor runs |
(5) No shipped rule or dashboard reads queue_limit/inflight_limit |
HOLDS | repo-wide grep: the only dkg.backpressure.* consumer anywhere is oldest_queued_age_ms at w1-rules.yaml:101,202. packages/node-ui has no backpressure reference at all |
(6a) k ≤ 4 lanes reach sync-global |
HOLDS | SyncSchedulerLane has 6 members, but pre_authorization/responder go to the responder limiter, whose PriorityAdmissionQueue (sync-handler.ts:261) passes no observability block — so it never calls updatePressureCapacity and is never registered |
And on the question I most wanted settled — does this swap an always-healthy signal for an always-saturated one? It does not. At the real limits (queueLimit = 4, inflightLimit = 2) the executed ladder is: pool 0–2 healthy, 3 degraded, 4 saturated, and an idle lane correctly stays healthy however full the pool. Against #2006's measured distribution a lane holding work reads saturated ~55% (receiver 1) / ~39% (receiver 2) of a 2 h incident.
Three to address before merge
① The fix stops at the JSON and the log — metrics still mix the pair
recordBackpressureSnapshotMetrics is untouched (no hunk in :654-688). It calls record(lane.lane, lane) at :687, and the closure reads values.queued (:668) and values.queueLimit (:673). Because a shared lane's queueLimit is no longer null, the if (values.queueLimit !== null) gate at :672 now emits per-lane queue_limit (and :675 inflight_limit) series for sync-global where base emitted none — carrying the pool ceiling, repeated on every lane, including idle ones. pressureQueued reaches no metric and capacityModel is no attribute.
Executed against the pinned head with production capacity, pool full at 4 split changelog=3 / durable=1: the API reports both lanes saturated (pressureQueued 4/4), while the metric stream at the same instant emits queue_depth{lane="durable"}=1 against queue_limit{lane="durable"}=4 — 25% utilization on a lane the API calls saturated.
Three things genuinely reduce the severity here, and I checked each rather than assuming:
- The correct pair is already emitted. For a shared lane with work waiting,
pressureQueuedistotals.queuedand its ceiling istotals.queueLimit, soqueue_depth{lane="all"} / queue_limit{lane="all"}is exactly the new predicate — on base and head alike. The number is not missing; per-lane replication and a model label are. sum()was never safe on this metric.record('all', …)at:679shares the instrument name with the lane rows, sosum(dkg_backpressure_queue_limit{scheduler="store"})already double-counted on base. This PR widens an existing hazard rather than creating one.- No shipped consumer, per claim (5) above.
So this is not live breakage — it is a trap laid precisely for the W1 central-collector work that #2075 explicitly defers to, on a function with zero test coverage anywhere in the repo. Either record lane.pressureQueued ?? lane.queued as the lane depth, or gate the per-lane ceilings on capacityModel !== 'shared' and keep them on the all row — or scope it out explicitly in the description, which is the cheapest honest option.
② Depth-first classification masks the per-lane age signal
Precedence is saturated > degraded, and the depth branch is the else if immediately ahead of the age branch (:490-508). Under shared the depth term is the pool's depth, identical on every lane, so when the pool is full it fires everywhere at once and outranks oldestQueuedAgeMs >= degradedQueueAgeMs — the only per-lane term in the classifier.
Executed, pool 4/4, durable queued 90 s ago and the other three at 0 ms: base gives durable=degraded, others healthy (two states, the stuck lane visible); head gives all four saturated (one state). Against #2006's counters, rejections explain at most 5.8% / 1.7% of the saturated windows, so >90% of that saturation is depth-driven — this is the common case for roughly half an incident.
The description discloses that lane state replicates the pool's pressure and that queued/queuedOperations remain the attribution signal. That covers who is waiting; it does not cover how long, which is what the age branch carried. Bounded — oldestQueuedAgeMs stays on the row and is the only metric the shipped alert rules read, so alerting is unaffected — but the state column is where the triage docs currently point first.
Filed as #2109 with three options and an acceptance test; a stateReason: 'depth' | 'age' | 'rejection' | 'active_age' discriminator is the smallest fix and would also resolve the saturated-with-pressureQueued: 0 oddity below. Fine to take there rather than here — but it should be a decision, not an inheritance.
③ The inflight half of the same mismatch is left in place
laneCapacityFor resolves inflightLimit from the pool (:453-455) while laneSnapshot still publishes lane-local inflight: active.length (:517), and there is no pressureInflight. Pre-PR this was invisible for sync-global: lane inflightLimit was null, so the metric was skipped and the only record carrying an inflight pair was the lane:"all" rollup, which reported 2/2 correctly. Claim (4) removes that record.
Executed on the real PriorityAdmissionQueue, queue 4 / inflight 2, two admissions running and one queued per lane: a row now reads lane=swm_recovery state=saturated queued=1 pressureQueued=4 queueLimit=4 inflight=0 inflightLimit=2, and the same shape lands in daemon.log. An operator reads that as the concurrency pool is idle when it is 100% occupied and is the reason nothing drains. Pre-PR the same incident produced one correct record.
This is the same defect class the PR exists to fix, on a surface the PR newly populates. Minimum fix: report inflightLimit: null on shared lane rows so no consumer can form the ratio; better: extend the pressure pair to concurrency with the same omit-when-equal rule.
Claim / code mismatches
These are worth correcting because the ### Fixed bullet ships as release notes.
-
Claim (2)'s stated proof is false as written. "The added
laneQueued > 0guard is implied by the comparisons it guards" holds only fordegradedQueueUtilization > 0. The old degraded branch evaluatedqueued.length / queueLimit >= threshold, which is0 >= 0— true — on an idle lane at threshold 0 or negative, whiledepthApplies(:449) is false. Executed: frozen clock,{lanes: {interactive: {queueLimit: 4}}},degradedQueueUtilization: 0→ basedegraded, headhealthy; identical at −0.5. 3,761/30,000 randomized hits once that threshold entered the generator, and exactly 0/30,000 without it. No in-tree caller sets it, butSchedulerPressureThresholdsis exported public API. Either scope the guard (… && (!shared || laneQueued > 0)) or soften the claim to "unchanged for every positive utilization threshold", which is what was actually proven. -
Claim (6)'s log-volume arithmetic is wrong in both directions. "1 line/minute to k (≤ 4)" takes the 60 s summary cadence as the baseline, but per-lane records are emitted on
transitiontoo (observeSample :794-803) at the 5 s sample interval, and sync lanes were already leavinghealthyonoldestQueuedAgeMs >= 15 spre-PR — so the base was never 1/min. Two measurements against the shipped cadence: on a trace calibrated to #2006 receiver 1's distribution, 754 → 954 lines per 2 h (1.27×, base ≈ 6.3/min); on an adversarially oscillating pool across the 75%/100% band, 7 → 48 lines/min. The ceiling is lanes × samples, not lanes. The realistic number is more favourable than the claim — but the claim as stated is wrong in both the baseline and the bound. -
capacityModelreads"partitioned"until the firstacquire().PriorityAdmissionQueueregisters in its constructor (:125) while the onlyupdatePressureCapacitycall lives inacquireInternal(:177), so a freshly booted daemon advertises the wrong model on the field the docstring calls authoritative. It is permanent underDKG_SYNC_GLOBAL_MAX_INFLIGHT=0, wherewithGlobalSyncBackpressureshort-circuits atsync/backpressure.ts:293-303without ever acquiring. Zero code readers today, never wrong on a lane row (updatePressureCapacitystrictly precedes and is co-gated with every lane-creating call), and no effect on classification — so LOW. The one-line fix was verified inert: passcapacity: { capacityModel: 'shared' }in thesuper({…})call; post-acquire output is byte-identical becauseupdateCapacityreplaces wholesale. -
docs:85overstatespressureQueuedfor an unbounded pool. WithcapacityModel: 'shared'andqueueLimit: null,depthPressureisnull(:449), so each lane falls back to its own backlog at:480. Executed with 2 queued indurableand 1 inchangelog: rows reportpressureQueued 2and1while the pool holds 3. Harmless (the row'squeueLimitis null so no ratio is computable) but not what the table says. -
docs:125says5 × 7; the closed set has 8 members.SYNC_ADMISSION_SOURCES(policy.ts:45-54) has 8, and the code comment atsync/backpressure.ts:66already says5 × 8. Pre-existing, but this PR edits the same file three lines below. -
The description says
poolQueued; the code and docs saypressureQueued. Stale from an earlier revision — same vocabulary the still-open 🟡 thread quotes.
Tests
The clock is genuinely frozen in every state-asserting test (now: () => 1_000; the agent queue forwards hooks.now into the tracker at priority-admission-queue.ts:118-124), nothing is ever rejected, and the distributed-75% test genuinely and uniquely discriminates its mutant — queueLimit: 4, three lanes at one entry each, so every lane sits at 25% on its own while the pool is at exactly 0.75. That test earns its place, and I could not find another that kills the same mutation.
Two holes worth closing:
-
The snapshot-level
capacityModel— the field the doc comment calls authoritative — is asserted by no test built from a real tracker. All nine new tests assertcapacityModelonly inside alanes:array; the two top-level occurrences (test:441,:462) are hand-built input to a registry, not an assertion on tracker output. Deletesrc:380and every new and pre-existing test stays green, while the API would report{"capacityModel":"partitioned", …, "lanes":[{"capacityModel":"shared", …}]}— the authoritative field contradicting every row beneath it. Fix is two lines: add it as a sibling ofstate/totalsattest:119andtest:264. -
expect(line('all')).toBeUndefined()(test:494) is vacuous with respect to this PR. The suppression it exercises lives atsrc:761, which this PR does not touch, and the test feeds it hand-built sources whose equal states the test itself chose (:412,:439,:460). So load-bearing claim (4) has no evidence from a real shared tracker. Concretely: change the shared numerator at:460tothis.queued.size - laneQueued; core tests 1 and 3 and agent test A die, but the property this assertion claims to prove is now false — with pool 4/4 anddurableholding 2, that lane reads 2/4healthywhile the rollup issaturated, sosync-globalresumes emitting"lane":"all"records, the opposite of what is documented, and nothing observes it. Driving the monitor from a real tracker with the spread shape would make the identity, not the author's chosen states, the thing under test.
Smaller:
"fills the queue on its own"(test:140) has a kill set that is a strict subset of the spread test's. Withthis.queued.size === laneQueued === 4the pool-vs-lane distinction — the entire subject of the fix — is invisible to it, and it asserts neithercapacityModelnorpressureQueued. Mutate:460tolaneQueuedand it still passes whiletest:99and agent test A die; mutate:449tolaneQueued > 1and it survives while the spread test dies onchangelog. This contradicts "each mutant kills exactly one test". (The partitioned pair does not collapse the same way —test:274is the only test in the suite where the rollup strictly outranks every lane, so it uniquely guardstotals.queued >= totals.queueLimit.)- Claim (3) is exercised but never asserted.
test:169reachessumLaneLimits('inflightLimit', …)and does not asserttotals.inflightLimit. Relax the:547early return to skip nulls and sum the rest — a plausible future "improvement" — and all nine tests stay green while a shared scheduler publishing onlyinflightLimitwould report N× the pool. AddinginflightLimit: nullto that test'stotals, plus one case with{inflightLimit: 2, capacityModel: 'shared'}and noqueueLimit, pins it. pressureQueued's?? queued.lengthfallback is untested where it differs from?? 0. Only the empty-backlog case is covered. Mutate:480to?? 0and all nine pass, while an unbounded lane would reportpressureQueued: 0besidequeued: 7and the monitor's inequality at:847would stamp a fabricated"pressureQueued":0on every record.- Agent test B's lane scoping is decorative.
lanes.find(l => l.lane === 'durable')assertscapacityModel,queueLimitandinflightLimit, all three of which resolve from scheduler-level capacity undersharedand are therefore identical on every row — andthis.lanesis never pruned, so thefindcan match a stale row left by an earlier test against the module singleton. Change the lane default atsync/backpressure.ts:305from'durable'to'changelog'and this test still passes.lanes.every(l => l.capacityModel === 'shared')plus one lane-specific fact would be a real witness. recordBackpressureSnapshotMetricshas no test in the repo, and this PR changes what it emits.
One calibration note
The restored degraded band is one queue slot wide: queueLimit = limit * 2 = 4 and degradedQueueUtilization falls through to the 0.75 default, so the ladder is 0–2 healthy, exactly 3 degraded, 4 saturated. The sync thresholds block (sync/backpressure.ts:100-103) overrides degradedQueueAgeMs and stalledActiveAgeMs but not the utilization. Since reconcileSyncFromConnectedPeers (dkg-agent-lifecycle.ts:4238-4278) fires un-awaited across 7–14 peers, depth goes 0→4 faster than the 5 s monitor sample, so the warning band is frequently skipped — consistent with #2006's own counters showing only ~9% degraded samples against 55%/39% saturated. Setting degradedQueueUtilization: 0.5 explicitly for sync-global would put the warning at depth 2 and buy two slots of headroom instead of one. Worth a line of rationale in the docs either way.
Summary. The mechanism is right, the hard invariants are proven, store is genuinely untouched, and the additive fields break no consumer — I checked for strict-equality and snapshot assertions on a lane row across the repo and there are none. What I would want first: ① and ③ resolved or explicitly scoped out, the two test holes closed, and the overstated claims in the CHANGELOG corrected. ② is tracked at #2109.
|
Third CI note, and one more correction to my own reasoning. On the new head Correction: in my earlier note I argued this PR was not involved partly because "shard 3 runs 27 files / 335 tests on both base and this PR — identical". The shard plan is evidently dynamic — shard 7 here ran 38 files / 538 tests — so that argument does not hold as stated. Withdrawing it. What still holds, and is sufficient:
Still not mine to fix, and still blocking the gate — #2107 cannot go green until that base-branch failure is resolved. |
…e proofs Merge-readiness review at d1c11aa. ① was already fixed in 47c6bc2; this takes ③, the claim/code mismatches, and the test holes. ② is deferred to #2109 as a decision — see below. ③ The inflight half of the same defect. `laneCapacityFor` resolved `inflightLimit` from the pool while the row still published a lane-local `inflight`, so a shared row read `inflight: 0, inflightLimit: 2` — idle concurrency, while the pool was fully occupied and was the reason nothing drained. Pre-PR this was invisible: the lane limit was null, so no ratio was formable. `pressureInflight` now applies the `pressureQueued` rule to concurrency, on the row, in the log line under the same omit-when-equal rule, and as `dkg.backpressure.pressure_inflight`. Claim (2) was false as written. "The guard is implied by the comparisons it guards" holds only for a positive `degradedQueueUtilization`, and that threshold is caller-supplied public API — at 0 the old branch read `0 / limit >= 0` as degraded on an idle lane. Verified: base degraded, head healthy. The guard is now scoped to `shared`, so a private allocation is untouched by construction rather than by argument. `capacityModel` read `partitioned` until the first acquire, because the queue registers in its constructor and publishes capacity in `acquireInternal` — and permanently so when sync admission is disabled. The model is a property of the queue, so it is declared in the `super()` call. Also corrected: the CHANGELOG's log-volume arithmetic (per-lane records emit on transition at the 5 s interval, so the base was never 1/min and the bound is lanes x samples), the doc claim that partitioned lane allocations sum to the scheduler ceiling, `pressureQueued` on an unbounded pool, and 5 x 7 -> 5 x 8. Tests, each verified against the mutant it exists to kill: - the scheduler-level `capacityModel` is asserted from a real tracker (deleting it previously left every test green while the API contradicted its own rows); - the monitor's `"lane":"all"` suppression is driven from a real tracker, so the identity is under test rather than states this test chose — the old assertion survived the mutant that breaks it, this one does not; - an unbounded shared pool pins the `?? queued.length` fallback (`?? 0` would stamp a fabricated `pressureQueued: 0` beside `queued: 2` on every record); - `totals.inflightLimit` is asserted, so relaxing `sumLaneLimits` to skip nulls is caught; - the concentrated test asserts `capacityModel`/`pressureQueued` so its kill set is no longer a strict subset of the spread test's; - the agent witness asserts over ALL lane rows plus one genuinely lane-scoped fact, instead of a `find` that matched fields identical on every row and could hit a stale row on the module singleton. Deferred, deliberately: ② depth-first classification outranks the per-lane age signal, so a lane stuck 90 s and a lane queued 0 ms both read `saturated` when the pool is full. Real, and #2109 has three options and an acceptance test. The smallest one there — a `stateReason` discriminator — does not actually separate those two lanes, since both would report `depth`; that deserves the dedicated issue rather than a guess inherited from this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
|
Thanks — this is the most useful review the PR has had, and the differential harness caught things argument would not have. Everything actionable is now in, at ① Metrics mixed the pair — already fixed, before the review landedFixed in Your point that ③ The inflight half — correct, and the same defect
② Depth-first classification masking the age signal — deferred to #2109, as a decisionReal, and I am not shipping it as an inheritance. My reasoning for leaving it there: the smallest option you name, a Claim/code mismatches — all corrected
Test holes — closed, each against its mutant
Calibration noteTaken as a docs point rather than a code change — Not addressed, and out of scope by design: the still-open #2003 finding that the classifier ignores |
Jurij89
left a comment
Jurij89
left a comment
There was a problem hiding this comment.
Round-2 review at abcbc663e
All six round-1 findings are fixed, and all four invariants re-proven by execution at far greater scale than round 1. One ask before merge — a test — plus a short list of documentation corrections, one of which is an over-correction my own round-1 review prompted.
The invariants, re-proven rather than assumed
The round-2 delta touched laneSnapshot twice, so these were re-established from scratch against the base blob, not carried over:
| Invariant | Result |
|---|---|
| Store lane states unchanged vs base — now unconditionally | HOLDS. 86,400 differential scenarios sweeping lane limits (0, null, undefined, negative, fractional, NaN, ±Infinity, MAX_SAFE_INTEGER), depths, ages either side of every threshold, ghost lanes in the map with no traffic and traffic on lanes absent from it — and degradedQueueUtilization across {-1, 0, 0.0001, …, 1.5, NaN, ±Inf}. Zero divergence. The round-1 head does diverge in the same sweep, which is what proves fix (3) closed it |
/api/status rollup unchanged for an all-shared scheduler |
HOLDS. 1,400 shared scenarios, zero divergence in snapshot.state or the whole totals object |
sumLaneLimits can never sum a pool ceiling |
HOLDS. totals.queueLimit was always the normalized pool ceiling or null, never a multiple |
No "lane":"all" for sync-global; store [backpressure] records byte-identical |
HOLDS. 500 randomized store scenarios / 20,262 records byte-identical to base, none carrying pressureInflight. 19,525 shared records, zero "lane":"all" against base's 724 |
Also verified: packages/core 15/15 and packages/agent 34/34 pass with the dependency closure actually built and the artifact grepped rather than assumed; tsc --noEmit clean; and tools/observability/verify-w1-render.mjs parses telemetry-api.ts 71/71 instruments with both new gauges recognised and no duplicate name.
Fix-by-fix
| Round-1 finding | Verdict |
|---|---|
| ① Metrics path not carried through | FIXED. pressure_depth exports the pool depth beside a lane-local queue_depth; executed at pool capacity it gives utilization 1.00 against saturated and 0.75 against degraded, and equals queue_depth on every partitioned row. The ?? values.queued fallback preserves older hand-built sources |
| ② Inflight half left in place | FIXED, with a caveat — see below. The value is right where it matters: this.active.size equals totals.inflight on every shared row across 600 scenarios, and it is genuinely the count canRun compares against |
③ Claim (2)'s proof false at degradedQueueUtilization ≤ 0 |
FIXED, and fixed the better way — the guard is scoped to shared rather than the claim being softened, so invariant 2 is now unconditionally true instead of true-in-practice |
④ capacityModel wrong before first acquire() |
FIXED. Verified through the real PriorityAdmissionQueue: shared from registration onward, with limits still null (no ceiling fabricated). The DKG_SYNC_GLOBAL_MAX_INFLIGHT=0 permanent case is closed too |
| ⑤ Log-volume arithmetic | FIXED — 1.27× measured, stated as ~1.3× with the worst case named |
⑥ Docs (5 × 7, unbounded-pool wording) |
FIXED, and the unbounded-pool case now has a real test |
On the tests: four of six round-1 test findings are fixed properly. The vacuous expect(line('all')).toBeUndefined() is now driven from a real shared tracker (test:574) — mutating the shared numerator to this.queued.size - laneQueued now fails three assertions, which is exactly the kill that was missing. The snapshot-level capacityModel is asserted from real trackers at :161, :200, :340 and on the production singleton at agent :1388. The pressure_depth metrics test pins values and metric names, not just "an instrument fired".
The one ask: three of the four round-2 source fixes are revertible with the suite green
pressureInflighthas zero test references repo-wide (grepreturns hits only in src, telemetry-api and docs). Reverting:506toactive.length— an exact revert of the fix requested in round 1 — leaves both suites passing. So do deleting the log field at:890, dropping it from the snapshot at:544, or misspelling the gauge attelemetry-api.ts:420.- Fix (3) is pinned by nothing —
degradedQueueUtilizationappears in zero test files in the repo, so the scoped and unscoped forms are observationally identical across the whole suite. The invariant it restores rests on reading alone. - Fix (4) is pinned by nothing — every agent-test snapshot is taken post-
acquire(:1246,:1269,:1320,:1381), so deletingpriority-admission-queue.ts:128leaves 70/70 green.
Nothing is wrong today and none of these can move a lane state, the rollup, or /api/status — the classifier reads neither pressure field. But this is precisely the mechanism by which a review finding comes back silently, and each guard is a few lines. The pressure_inflight one is the one I would insist on: a shared tracker with two lanes each holding one active record, asserting inflight === 1 while pressure_inflight === 2, plus an equality check on a partitioned row.
One caution on that test — the obvious place to put it doesn't work. Extending the real-tracker monitor test at :574 can't express it: that test enqueues 4 into a queueLimit of 4 and starts none, so this.active.size === 0 and the omit-when-equal rule suppresses the field on every record; calling start() needs a ticket, but the four enqueue() return values are discarded, and starting one drops pool depth 4→3, flipping that test's asserted saturated to degraded. It needs its own tracker.
Documentation
The partitioned correction over-corrected — and this one is on me. :11 now says lane allocations "are not summed into it and are not validated against it". The second half is right; the first is false in both directions. sumLaneLimits derives the rollup ceiling as exactly the lane sum when a scheduler publishes none, and StorePriorityScheduler publishes its scheduler-level ceiling as literally that sum:
// packages/storage/src/store-priority-scheduler.ts:314
queueLimit: Object.values(this.queueLimits).reduce((sum, value) => sum + value, 0),My round-1 review relayed "sum() was never safe on this metric" — which is true of the lane="all" metric row sharing an instrument name with the lane rows — and that got generalised into "not summed", which is a different claim. The accurate statement is the independence one: nothing validates the scheduler ceiling against the lane allocations, so do not derive either from the other — while noting that a scheduler may publish the sum (store does) and that sumLaneLimits falls back to it. Same correction applies to docs:74.
pressureInflight does not follow "the same rule as pressureQueued", stated at :110-114, docs:86, and in the commit message. pressureQueued has two carve-outs and pressureInflight has neither:
- Idle lane.
depthAppliesrequireslaneQueued > 0undershared(:469), so an idle lane reports its own0.pressureInflightat:506keys offcapacityModelalone, so an idle shared lane publishes the whole pool — executed on the production shape, threehealthylanes withinflight: 0exportpressure_inflight: 2againstinflight_limit: 2. - No ceiling.
pressureQueuedfalls back to the lane's own backlog whenqueueLimitis null;pressureInflightnever consultsinflightLimit, so an unbounded shared pool publishespressureInflight: 2besideinflightLimit: null.
I went in expecting this to be a bug and came out satisfied it is not. pressureQueued must equal the classifier's numerator — otherwise a row reads pressureQueued/queueLimit = 4/4 next to state: healthy, a self-contradiction — whereas :503-504 says of the inflight pair, correctly, "the classifier reads neither — this pair is reported, not judged". No branch of laneSnapshot reads inflight or inflightLimit, so no contradiction is constructible, and on a shared row the denominator is the pool's too, making the ratio a pool ratio by construction, identical across rows and equal to the all row. So it is the three "same rule" statements that are wrong, not the code. Say what it actually is: on a shared row, the pool's occupancy, on every row, always. (I'd also drop the gating suggestion if it comes up — keying the numerator on queued.length > 0 would make the series step 0→2 on an unrelated enqueue with no change in real concurrency, which is worse than a constant.)
A comment that states a kill the code makes impossible — test:291-294: "Relax sumLaneLimits to skip nulls and sum the rest … and this reads 2× the pool while every other assertion stays green." That test's capacity is { queueLimit: 4, inflightLimit: 2 }, both non-null, so normalizeLimit short-circuits and sumLaneLimits is never reached. The mutant cannot be killed there. It is killed — by the unbounded-pool test at :246, where totals.queueLimit: null becomes 0 under that mutation. So the coverage is real and the comment points at the wrong test; move or correct it. Worth flagging because an unrun claim in a comment reads exactly like a passing check.
Smaller:
- CHANGELOG never mentions
pressureInflightordkg.backpressure.pressure_inflight, and still says "two optional fields" where there are now three. Release-note only —docs:86anddocs:232cover both correctly. Note the entry was accurate at round 1 and was falsified by the same commit that edited it. docs:80"Two depths sit on every lane row" is stale now that the table has three rows.docs:85's "where no depth applies — an empty backlog" describes onlysharedsince fix (3) scoped the guard; a partitioned lane now always has depth applied when it has a limit.- The metrics table doesn't say the two new pressure gauges must never be summed across lanes — the same warning the capacity table gives for
queueLimit. - Still not fixed from round 1: the "fills the queue on its own" test (
:183) remains a strict subset of the spread test's kill set. Its three added assertions all already exist at:161/:167-169, in a configuration where the two denominators actually disagree (2/1/1 of a pool of 4) rather than coincide (4 of 4). Low stakes; deleting it or giving it a shape only it can cover would both be fine. - New surface is +20 gauge series per node, 12 of which are always-equal duplicates on partitioned rows. Reasonable for a uniform consumer contract, but worth being deliberate about.
CI
Red again — Tornado: agent [7/10] and CI gate — and it is the same pre-existing failure, re-sharded from [3/10]: publish-finalized-agent-lane.test.ts > reads finalized assertions from the explicitly selected non-default agent lane, AssertionError: expected [] to have a length of 1 but got +0, with 36 other files passing in that shard. CI gate carries no test output and fails only because it gates on tornado-agent. Not attributable to this PR, as at round 1.
Merge-ready. The mechanism is right, the invariants are proven at scale, store is untouched down to the byte, and every round-1 finding landed — fix (3) in the stronger form. Add the pressure_inflight guard, correct the partitioned comment (my error to fix, not yours) and the three "same rule" statements, and this is done. #2109 tracks the lane-state age-masking design question separately.
…tated claims Round-2 review at abcbc66. Merge-ready verdict, with one ask and a list of documentation corrections. The ask: three of the four round-2 source fixes were revertible with both suites green — which is the mechanism by which a review finding comes back silently. Each now has a guard, and each guard was verified against the exact mutant it exists to kill: - `pressureInflight` had zero test references repo-wide. A dedicated tracker (as the review notes, the monitor test cannot express it: it starts nothing, so `active.size` is 0 and the omit-when-equal rule suppresses the field) asserts `inflight: 1` beside `pressureInflight: 2` on two shared lanes, the log line, and the metric BY NAME. Kills all four reverts: the field, the log field, the snapshot value, and a misspelt gauge. - `degradedQueueUtilization` appears in no other test in the repo, so scoping the empty-lane guard to `shared` rested on reading alone. A zero-threshold idle lane now asserts `degraded` under `partitioned` and `healthy` under `shared`. Unscoping the guard kills it. - Every agent snapshot was taken post-acquire, so the constructor's declaration was unpinned. A fresh queue now asserts `capacityModel: 'shared'` with null limits before any admission. Deleting the declaration kills it. Corrections, three of which are claims of mine that were wrong: - "lane allocations are not summed into it" over-corrected a round-1 note. `sumLaneLimits` derives the rollup as exactly the lane sum when a scheduler publishes no ceiling, and `StorePriorityScheduler` publishes its ceiling as literally that sum. The accurate claim is the independence one: nothing VALIDATES the two against each other, so derive neither from the other. - `pressureInflight` does not follow "the same rule as `pressureQueued`" — `pressureQueued` falls back to the lane's own backlog where no depth was classified, and `pressureInflight` has no such fallback. The code is right and the three statements were wrong: on a shared row it is the pool's occupancy, always. Gating it on queued work would step the series 0 -> N on an unrelated enqueue with no change in real concurrency. - A test comment named a mutant its own fixture makes unreachable: with both ceilings non-null, `normalizeLimit` short-circuits and `sumLaneLimits` is never reached there. The kill happens in the unbounded-pool test; the comment now says so. An unrun claim in a comment reads exactly like a passing check. - The concentrated test's kill set is a strict subset of the spread test's, and the comment now states that outright rather than the PR claiming each mutant kills exactly one test. Its role is the issue's literal acceptance criterion and the case where both denominators agree. Plus: CHANGELOG now names `pressureInflight` and the second gauge and says three optional fields, not two; the metrics table carries the never-sum warning the capacity table already had; "two depths" and the unbounded-pool wording are accurate; and the +20 series cost is stated rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round-2 review addressed at The ask — three fixes were revertible with the suite greenYou were right that this is how a finding comes back silently, and all three now have a guard. Each was verified against the exact revert you named, not just added:
Your caution about the placement was right and saved me the detour: the Documentation — three of these were my errors, one was the over-correction
Calibration and #2109
Verification on this head: core CI red remains the pre-existing |
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
Jurij89
left a comment
Jurij89
left a comment
There was a problem hiding this comment.
Final review at dfdec049e — ship it
Short pass, because the source delta since abcbc663e is provably comment-only: after stripping comments, backpressure-observability.ts, priority-admission-queue.ts and telemetry-api.ts are byte-identical, and no @ts-ignore / eslint-disable / v8 ignore directive was added (those would have made a strip-diff look clean while changing what the suite measures). So round 2's execution proofs — 86,400 partitioned scenarios with zero divergence from base, 20,262 store log records byte-identical, zero "lane":"all" — carry over untouched rather than needing a re-run.
The three unpinned fixes are now genuinely pinned
Each mutant applied one at a time, restored from a pre-made copy, tree provably clean beforehand and the mutation re-grepped as still present afterwards, with the Tests … matcher proven able to match on the green baseline first.
| Mutant | Sole failing test | Result |
|---|---|---|
pressureInflight → active.length |
pairs a shared row concurrency count with the ceiling that bounds it |
KILLED |
(!shared || laneQueued > 0) → laneQueued > 0 |
applies the empty-lane guard only where a pool is shared |
KILLED |
delete capacity: { capacityModel: 'shared' } |
declares the shared pool from registration, before any admission |
KILLED |
Kill sets are disjoint and singleton — no other test fails under any of them, so each new test is the sole guard for its fix and none is redundant with existing coverage.
Two things worth recording because they are the kind of thing that usually goes unchecked:
- The
pressureInflighttest's three surfaces each discriminate independently. Under the single revert the test fails at the first assertion andexpectthrows, so the log-line and metric assertions never execute — that mutant alone proves only the snapshot. Two further targeted mutants that leave the snapshot correct (dropping the omit-when-equal spread from the log record; recordingvalues.inflightinstead ofvalues.pressureInflight) each kill the same test at the later assertion. So the four-reverts claim holds — they are four distinct mutants, not four assertions behind one. applies the empty-lane guard only where a pool is shareddiffers in outcome, not just input. Partitioned idle lane →degraded, shared idle lane →healthy, and the mutant fails on the partitioned direction specifically. That is the exact point where round 1 diverged from base, so the invariant that used to rest on reading now rests on a test.
Baselines: packages/core backpressure suite 17/17, packages/agent sync-backpressure 35/35 (--config vitest.unit.config.ts), and the full packages/core suite 1665/1665 green — the pre-existing failures I expected to have to discount did not reproduce at this head. The agent lane was built and the artifact grepped in packages/core/dist rather than assumed, and re-confirmed after the core-src mutants so neither leaked into dist.
The four overstated claims are now accurate
- The
partitionedcomment no longer says allocations are "not summed" — it says nothing validates the scheduler limit against them, namesstoreas a scheduler that publishes their sum, and notessumLaneLimitsfalls back to the sum. That is true in both directions, where the previous wording was false in both. (My round-1 review prompted the over-correction, so: fixed correctly, and thank you for taking the note rather than the wording.) pressureInflightis now documented as the pool's occupancy always, with the asymmetry againstpressureQueuedexplained — no state is ever classified on concurrency, so a shared row's ratio is a pool ratio by construction and cannot contradict its own state. That is the right resolution: the code was correct and the "same rule" claim was the defect.- The
pressure_inflightmetrics row dropped "on the same rule aspressure_depth", and a new paragraph states that neither pressure gauge may be summed across lanes — correctly noting thelane="all"row shares the instrument names, so an unfilteredsum()double-counts on every scheduler. test:294's comment now says plainly that thesumLaneLimitsmutant cannot be killed by that test and names the unbounded-pool test that does kill it. An unrun claim in a comment reads exactly like a passing check, so this one mattered more than its size.
CI
Fully green: 57 pass, 0 fail, all 10 agent shards passing. publish-finalized-agent-lane.test.ts, which was red at r1 and r2, passes here with no related change — so it is flaky rather than a deterministic base failure. Worth correcting my earlier wording: "pre-existing red lane" was right about attribution, but "flaky" is the accurate description.
One residue
docs/use-dkg/backpressure-observability.md:89 still reads:
Both fields are optional on the type … absent
capacityModelmeanspartitioned, absentpressureQueuedmeansqueued.
There are three optional fields now, and the fallback list is missing absent pressureInflight means inflight. The CHANGELOG was corrected to "three optional fields" in this same commit; this sentence sits two lines under the table that gained the third row and was missed. One-line fix, and the last thing I have.
Also unchanged and fine: the concentrated "fills the queue on its own" test keeps a kill set that is a subset of the spread test's, now with a comment saying so deliberately — it is the issue's literal acceptance criterion and the case where the two denominators agree. Documenting the intent is a legitimate resolution.
Verdict: merge. Every finding from both prior rounds is closed, the fixes that were unpinned are now killed by disjoint single-purpose tests, the source delta cannot have changed behaviour, and CI is clean. #2109 tracks the lane-state age-masking design question on its own, as intended.
New surface for the record: three optional lane fields, one optional snapshot field, two gauges, ~+20 series per node.
The sentence under the lane-field table still said "both fields" and listed two fallbacks; it sits two lines below the row that gained `pressureInflight` and was missed when the CHANGELOG was corrected to three in the same commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Residue fixed at And a correction I owe on the CI question. You are right that "flaky" is the accurate word, and my earlier wording was wrong — not just imprecise. The sequence: What survives from those notes is only the attribution: the same test failed on Worth flagging for whoever owns #2099/#2101: Thanks for the three rounds — the differential harness caught two things I would have shipped: the metric pair I had made inconsistent, and the A short live confirmation on testnet is running now — boot, force |
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
|
Live testnet validation done, on the PR head The load was real sync traffic — peer dials and on-connect catch-up against the public network. Only the admission ceilings were tuned down ( Guard first, because the plan says a run that never fills the pool is inconclusive rather than a pass: The claim the PR exists to establish{"lane":"durable","state":"saturated","queued":4,"pressureQueued":4,"queueLimit":4,
"oldestQueuedAgeMs":3277,"rejectedTotal":0,"lastRejectedAgeMs":null}
Results
M5 in detail: The one R1 disagreement, since a bare 1213/1214 would be hand-wavingAt Honest gaps
Full report with environment, evidence paths and teardown: |
|
Post-merge correction — I retracted a claim that was actually right, and should put that straight. I first said the #2108 ( So: deterministic, as originally described, and now fixed at source. What I got wrong was the inference, not the observation — "it passed on a later head" is not evidence of flakiness when the base moved in between, and I did not check whether it had. #2108 also confirms the mechanism I traced from the log: a store-canonicalized Nothing here changes the merged code or the validation. Recording it so the thread does not leave a correct call retracted. |
2 participants
Summary
The bug.
GET /api/diagnostics/backpressureclassifies each lane with four signals, two of which are depth-based and need a per-lane queue limit. The sync admission queue publishes none, so for everysync-globallane both depth branches were dead and lanestatedegraded to a trailing indicator: a sync lane only lefthealthyafter 15 s of queue age (degradedQueueAgeMs, overridden from the 5 s default), 120 s of active age, or a rejection that had already happened.Why the issue's own suggested fix was not taken. Publishing the one global ceiling as each lane's
queueLimitwould satisfy the acceptance criteria literally and then rarely fire.sync-globallanes are fed by concurrent per-peer drivers (dkg-agent-lifecycle.ts:4238-4278fans out un-awaited), so with the defaultqueueLimit = 4a pool full atdurable: 2, changelog: 1, shared_memory: 1still reports every lanehealthy— one lane would have to hold 3 of 4 slots to trip anything. It would also publish a number no code enforces, turndkg.backpressure.queue_limit{scheduler="sync-global"}from 1 series into 5, and disarm thelanes.some(l => l[field] === null)guard that is the only thing keepingsumLaneLimitsfrom reporting 4× the real ceiling.What this does instead. A scheduler declares how its lanes divide capacity.
partitioned(the default, andStorePriorityScheduler) means private per-lane allocations — behaviour is unchanged, by construction.shared(PriorityAdmissionQueue) means every lane draws on one pool: a lane's ceilings resolve from the scheduler-level limits, and depth is measured against the pool's depth, charged only to lanes that actually hold queued work. So every lane waiting behind a full sync queue reportssaturatedthe moment it fills anddegradedat 75%, before anything is rejected; a lane with nothing queued stayshealthy, so per-lanequeuedandqueuedOperationsremain the attribution signal.Why not infer "shared" from a missing lane limit (the smaller diff, no producer change):
queueLimit === nulltoday means "shared pool" or "the producer has not published it" — and the second is exactly this bug. Collapsing them silently reclassifies the next scheduler that forgets, instead of leaving the omission visible.Two things that provably cannot change
/api/statusrollup. For an all-shared schedulertotals.queuedis the pool depth andtotals.queueLimitis the pool ceiling, so a shared lane's new predicate is the predicate the rollup already evaluates atbackpressure-observability.ts:295-307.maxStateover lanes can only reproduce a state the totals branch was going to produce anyway.laneCapacityForreturns adepthPressurewhose numerator is the lane's own backlog — literally the expression the classifier used before — and the addedlaneQueued > 0guard is implied by the comparisons it guards (queued >= limitwithlimit > 0;0 / limit >= 0.75is false). Pinned by two tests, including one where the store rollup is full while neither lane is near its own allocation.Called out deliberately
sync-globalstops emitting its"lane":"all"log record. That record is emitted only while the rollup strictly outranks every lane (:635), and the identity above makes that unreachable for a shared scheduler. Rather than relax:635to>=— which would also raise the store scheduler's log volume, a scheduler this PR otherwise proves untouched — a lane record carriespressureQueuedwhenever it differs fromqueued, so the depth that classified the lane and the ceiling it was measured against are both on the line that replaced it. Everystorerecord stays byte-identical. Log volume during a sync incident goes from 1 line/minute to k, where k is the number of lanes holding queued work (≤ 4) — which is exactly the set of lanes an operator needs named.priority-admission-queue.ts:227tests the whole queue;backpressure.ts:78gates on one module-level counter), so this reads it as "the limit it is subject to".capacityModel(the authoritative value — capacity division is a scheduler invariant), and each lane row gains two optional fields: a derivedcapacityModel, so a row still explains its ownqueueLimitwhen it travels alone in a log line or a metric series, andpressureQueued— the depth the state was classified against, equal toqueuedunderpartitionedand the pool's depth undershared. Utilization is thereforepressureQueued / queueLimiton every scheduler, with no model-specific branch in any consumer. Both are optional so a hand-builtBackpressureSourcewritten against an olderdkg-corestill satisfies the type (absentcapacityModel=partitioned, absentpressureQueued=queued). Shared lanes now report the pool'squeueLimit/inflightLimitinstead ofnull— one pool's ceilings repeated per lane, never to be summed (documented, and machine-readable viacapacityModel). That adds 4 identicalqueue_limit/inflight_limitseries forsync-global; no shipped dashboard or alert rule reads those gauges — the only backpressure metric any tooling consumes isoldest_queued_age_ms(tools/observability/w1/w1-rules.yaml:101,202).pre_authorizationandresponderstill appear in no snapshot, metric, or log line. Now stated in the guide so this fix cannot be read as covering it.inflightLimit. This change touches only the two queue-depth terms and adds no inflight branch, so that finding's hard-capped-lane hazard is unaffected.### Known issuesbullet (CHANGELOG.md:58) is left byte-for-byte — released sections are immutable; the fix is recorded under## [Unreleased]and the item simply is not carried forward at the next release.Related
v10.0.11)stateworth reading)Diagrams
A full
sync-globalqueue, spread across lanesBefore:
sequenceDiagram participant Sync as Sync drivers (4 peers) participant Q as PriorityAdmissionQueue participant T as SchedulerPressureTracker participant Op as Operator Sync->>Q: acquire x4 (durable x2, changelog, shared_memory) Q->>T: updateCapacity({queueLimit: 4, inflightLimit: 2}) Note over T: no lanes map -> every lane queueLimit = null Op->>T: GET /api/diagnostics/backpressure T-->>Op: scheduler saturated — durable healthy, changelog healthy, shared_memory healthy Note over Op: depth branches dead — lanes go non-healthy<br/>only at 15 s of age, or after a rejectionAfter:
sequenceDiagram participant Sync as Sync drivers (4 peers) participant Q as PriorityAdmissionQueue participant T as SchedulerPressureTracker participant Op as Operator Sync->>Q: acquire x4 (durable x2, changelog, shared_memory) Q->>T: updateCapacity({queueLimit: 4, inflightLimit: 2, capacityModel: 'shared'}) Note over T: shared -> lane ceiling = pool ceiling,<br/>depth = pool depth, charged to lanes holding work Op->>T: GET /api/diagnostics/backpressure T-->>Op: scheduler saturated — durable saturated (2/4), changelog saturated (1/4), shared_memory saturated (1/4) Note over Op: fires at the instant the queue fills,<br/>with no rejection and no elapsed timeAn idle lane on a full pool
Before:
sequenceDiagram participant T as SchedulerPressureTracker participant Op as Operator Note over T: durable 3/4 queued, swm_recovery running, nothing queued Op->>T: snapshot T-->>Op: durable healthy, swm_recovery healthyAfter:
sequenceDiagram participant T as SchedulerPressureTracker participant Op as Operator Note over T: durable 3/4 queued, swm_recovery running, nothing queued Op->>T: snapshot T-->>Op: durable degraded (75% band), swm_recovery healthy Note over Op: an idle lane is not held back by a full queue —<br/>lane `queued` stays the attribution signalFiles changed
packages/core/src/backpressure-observability.tsSchedulerLaneCapacityModel;SchedulerPressureCapacitybecomes a discriminated union so asharedcapacity cannot carry privatelanes;capacityModelonBackpressureSnapshot(authoritative — it is a scheduler invariant) and, derived, on eachBackpressureLaneSnapshotso a row travelling alone in a log line or a metric series still explains its ownqueueLimit; pluspressureQueuedper lane. A privatelaneCapacityForowns the whole capacity decision and returnsdepthPressure: {queued, limit} | null, solaneSnapshotis a model-agnostic classifier and "does depth apply" is not inferred from a nullable ceiling.BackpressureMonitor.messagereads the publishedpressureQueuedand emits it only when it differs fromqueued.sumLaneLimitsdocuments why a pool ceiling can never become a summand. All new fields are optional, so a hand-builtBackpressureSourcestill satisfies the types.packages/agent/src/sync/priority-admission-queue.tslaneCapacity: 'shared'in the existing capacity publish. The top-levelqueueLimit/inflightLimitpublish is load-bearing and unchanged.packages/core/test/backpressure-observability.test.tsBackpressureMonitortest over the emitted log JSON, registering a wholly shared and a wholly partitioned scheduler.packages/agent/test/sync-backpressure.test.tsPriorityAdmissionQueuewith a frozen clock driving the real producer path (spread shape), plus a witness that the productionsync-globalsingleton publishes the model and the scheduler-level ceilings.docs/use-dkg/backpressure-observability.mddegraded/saturatedsemantics for shared lanes, the idle-lane rule,poolQueued, and an explicit note that the responder limiter is uninstrumented.CHANGELOG.md### Fixedbullet under## [Unreleased].Test plan
Run in a fresh worktree with its own
pnpm installand a fullpnpm run build— the agent package resolves@origintrail-official/dkg-corefromdist, so a stale build silently fabricates results.packages/core> npx vitest run test/backpressure-observability.test.ts— 12 passed (5 before)packages/core> npx vitest run— 105 files / 1658 tests passedpackages/agent> npx vitest run test/sync-backpressure.test.ts(real CI config, Hardhat global setup) — 34 passed (32 before), exit 0packages/storage> npx vitest run test/store-priority-scheduler.test.ts— 22 passed (store non-regression)packages/cli> npx vitest run test/backpressure-route.test.ts— 3 passed, run without the chainglobalSetup: the CLI lane's Hardhat harness does not come up in this worktree for any cli test file (verified against an unrelated one), so that failure is environmental and pre-existing, not from this change. CI runs the lane normally.pnpm run build— exit 0; artifacts verified rather than assumed (grep -c capacityModel packages/core/dist/...js= 2,grep -c laneCapacity packages/agent/dist/...js= 1)Fail-before, verified:
packages/core/src/backpressure-observability.ts→ the 3 new lane tests fail.grep -c capacityModel packages/core/dist/...js= 0, so the failure is not a stale artifact) → both new tests fail.Mutants, verified:
boundedDepth = queued.length(drop the pool denominator) → kills only the spread test. This is the point: a concentrated-shape test alone would pass under the "publish the ceiling per lane" alternative and would not discriminate.queued.length > 0guard → kills only the idle-lane test.pressureQueuedfrom the emitted log JSON → kills only the new monitor test (added in review round 1, where nothing had covered the log path).healthylane reading as fully utilized).Every mutant above was re-run after each of the two structural refactors review asked for, and each is still killed by exactly the tests that killed it before.
Type-level, verified with
tsc: asharedcapacity carrying privatelanesis rejected(
Type '{ durable: … }' is not assignable to type 'undefined'), while{queueLimit, lanes},{laneCapacity:'shared', queueLimit, inflightLimit}and the pre-existing{queueLimit, inflightLimit}all compile.
Diagrams: all four mermaid blocks were parsed and rendered with mermaid 11.16 in headless
Chromium before publishing (harness:
mermaid.parse+mermaid.render, asserting real SVG output).Live-node check (testnet, separate): drive
sync-globalto a full queue and confirm (a) lanestatemoves at the instant the queue fills withrejectedTotalstill 0 andoldestQueuedAgeMswell under 15 000, (b)/api/statusreports the same scheduler state as before, and (c)[backpressure]lines for shared lanes carrypoolQueued.