Files
openclaw/src/process/command-queue.capacity-groups.ts
T
Spencer Fuller 3374f78ad8 fix(queue): prevent cron saturation from starving hook dispatch (#116666)
* feat(hooks): dispatch hook agent runs into a dedicated command lane

Hook agent runs passed lane:"cron", which resolveCronAgentLane remaps to
cron-nested — the same lane cron's own inner agent work uses, capped at the
hardcoded cron budget of 8. Eight busy cron turns therefore starved every hook.

Adds CommandLane.HookDispatch and dispatches hook runs into it. Neither lane
resolver needs changing: resolveCronAgentLane (agents/lanes.ts:15-22) and
resolveGlobalLane (embedded-agent-runner/lanes.ts:11-18) special-case only
"cron" and pass every other lane through.

This is lane identity only. It does NOT yet bound aggregate capacity — that is
the capacity group in the following commits. On its own this widens total
command-lane concurrency by the hook lane's width (1).

Consumers that inferred cron-ness from the lane, both preserved rather than
silently changed:
- heartbeat-runner-execution: HookDispatch added to the busy-lane check so hook
  work still suppresses heartbeats; only the lane it occupies changed.
- session-suspension: explicit resume concurrency and gateway-managed-lane
  membership, instead of falling through to the custom-lane default.

server-lanes publishes the lane at width 1: the guarantee is that a hook can
always start under cron saturation, not that hooks run concurrently.

Test: server.hooks-lane.test.ts asserts the dispatched lane and that it survives
cron lane resolution unremapped, with a positive control on the inputs that DO
remap. Mutation-verified — reverting the call site to "cron" fails it with
'expected cron to be hook-dispatch'. Nothing else in the suite reads the
dispatched lane, so without this assertion the change regresses silently.

Refs: openclaw#98813, openclaw#43235

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(queue): capacity groups with hard per-member reservations

Adds optional capacity groups to the command queue: lanes in a group share one
hard aggregate budget, and a member may hold a non-borrowable reservation
within it. This is what makes a separate hook lane safe — it bounds hook and
cron-nested work together at the existing cron cap instead of adding a slot
outside it (openclaw#98813 maintainer audit measured cron-nested=8 + hook-a=1 +
hook-b=1 = 10).

Group capacity is always DERIVED from members' activeTaskIds, never a separate
counter. Timeout, abort, clear, reset and stale-generation completion therefore
release capacity for free, because they all remove the task id; the only
remaining obligation is that those paths re-drain the group.

- setCommandLaneGroup / clearCommandLaneGroup / drainCommandLaneGroup
- admission: lane max, then group budget, then sibling reservations. A member
  may burst above its own reservation only into unreserved capacity.
- both completion paths (success AND error) wake group siblings; freed capacity
  belongs to the group, so a lane-local pump would strand a queued sibling
  behind capacity that is already free. resetCommandLane likewise.
- membership lives in the queue singleton keyed by lane name, NOT in LaneState,
  so setCommandLaneConcurrency cannot detach a member from its group.
- deadlock guard: rejects cron/main/subagent/nested and session:*/nested:*/
  context-engine-turn-maintenance:* — lanes that can be synchronously awaited,
  where a group wait would become a deadlock.
- rejects sum(reservations) > budget rather than starving silently.
- snapshot exposes group/groupActive/groupBudget/reservedForLane/blockedBy.

Tests: 9 new, all mutation-verified — dropping group admission fails 5,
removing the sibling wake fails 2, making reservations borrowable fails 2
("expected 4 to be 3": an idle sibling's reserve being borrowed). 57 pass
across all command-queue suites.

Not yet wired: no group is configured by default. That, plus group-wait
visibility and atomic publish, are the following commits.

Refs: openclaw#98813, openclaw#43235

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(queue): blockedBy answers hypothetical immediate admission, not queue state

Round-4 review (costaff-lapclaw-001) named this as the precision requirement
that decides whether the wait-visibility fix is vacuous:

  noteLaneWaitIfBusy runs BEFORE enqueue, so it snapshots the lane with
  queuedCount === 0. If blockedBy were populated only for an already-queued
  head entry, that snapshot would read "not blocked", no
  onLaneWait(waiting:true) would fire, and agent-watchdog's setup-timeout
  suppression would never engage — a run merely waiting on group capacity
  would take a false setup timeout.

resolveLaneBlockReason already answers "could this lane start work right now?"
independent of queue contents; these tests pin that contract:

- 7 cron active, hook holding the group's reserved slot: cron reports
  sibling-reservation with queuedCount 0 and activeCount < maxConcurrent, while
  the hook reports null (so the assertion discriminates rather than being
  always-truthy).
- a member lane that was never enqueued or was retired while idle still reports
  its group block state, instead of the not-found path returning a bare default
  that reads as free.

Mutation-verified: gating blockedBy on queue.length > 0 fails 3 tests with
'expected null to be sibling-reservation' — the exact symptom predicted.

Refs: openclaw#98813

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(queue): atomic lane publication, group-aware wait visibility, opt-in group

Closes the remaining two round-2 blockers and wires the default group.

publishLaneConfiguration({lanes, groups, clearGroups}) applies lane maxima and
group definitions as ONE transaction: install with dispatch suppressed, then a
single commit-time drain. The per-lane setter drains the instant a lane goes
positive and gateway publication was sequential, so a member could be widened
and dispatch BEFORE its group existed — admitting work above the budget the
group was meant to enforce. Validation throws before any drain, so a rejected
configuration cannot strand lanes widened and ungoverned.

lane-controller's noteLaneWaitIfBusy now also emits a wait when
snapshot.blockedBy != null. A group-blocked member has
activeCount < maxConcurrent and can have queuedCount === 0, so both lane-local
terms were false while the task genuinely could not start. This is not just
observability: agent-watchdog.ts suppresses the cron setup timeout only while
waitingForLane is true, so an invisible group wait produced a FALSE setup
timeout for cron-shaped runs.

The group is opt-in on hooks.enabled. The reservation is a real cost — it
withholds a slot from cron inner work even while the hook lane is idle — so it
is only paid where it buys something. This surfaced as a genuine regression:
server-lanes.test.ts asserts cron-nested alone reaches all 8, which an
unconditional group breaks. With hooks off, no group is installed, cron keeps
the entire budget, and such a deployment sees no behaviour change at all.
Turning hooks off on reload tears the group down (clearGroups).

With hooks ON, cron inner work trades one slot for the guarantee that hooks
cannot be starved. Aggregate stays exactly the pre-existing cron cap — no slot
added outside it, which is what openclaw#98813 was held for.

Tests: 6 new (3 publication, 3 opt-in), 135 passing across all affected suites.
Mutation-verified:
- sequential per-lane publication: 'expected 12 to be less than or equal to 8',
  the exact 8+4 additive leak, caught at PEAK not post-state as review required
- unconditional group: 'expected cron-hooks to be undefined'

Refs: openclaw#98813, openclaw#43235

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(queue): pin group-blocked lane waits to the setup-timeout suppression chain

Round-4 review (fiducian-spencer-001) asked for the 8-cron / hook-holding-
reserve / 8th-cron-waiting regression asserting no false setup timeout.

The chain spans three files:
  lane-controller.noteLaneWaitIfBusy -> onLaneWait({waiting:true})
  -> timer-job-runner.noteLaneState  -> watchdog.noteLaneWait()
  -> agent-watchdog:159-164          -> waitingForLane = true, clear timeout
  -> agent-watchdog:98               -> setup timeout suppressed

The watchdog end is already covered by agent-watchdog.test.ts. The link this
change introduced is the FIRST one, and it is the one that fails silently: a
group-blocked lane looks idle to a lane-local view, so no wait is reported and
a healthy run queued behind group capacity takes a false setup timeout.

The predicate was an inline closure, so it was untestable without the full
runner harness — and asserting a copy of it in a test would prove nothing.
Extracted as shouldNoteLaneWait(snapshot) and driven with real snapshots from a
real group:

- 7 cron active, hook holding the reserve: the test asserts explicitly that
  BOTH lane-local terms are false (activeCount 7 < maxConcurrent 8,
  queuedCount 0) and that the predicate still reports a wait.
- a hook blocked by a full group budget reports a wait.
- negative control: lanes that can start immediately report no wait, so a
  predicate hardcoded to true would fail.
- ordinary lane-local saturation still reports a wait (pre-existing behaviour).

Mutation-verified: reverting to the lane-local predicate fails 3 tests with
'expected false to be true'. 182 pass across all affected suites.

Refs: openclaw#98813

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(queue): make rejected publication a no-op; always tear down the group on hooks-off

Round-5 implementation review (fiducian-spencer-001, CHANGES REQUESTED) found
one real atomicity bug and two test gaps. Both bugs are fixed and both are now
mutation-guarded.

BLOCKER — rejected publishLaneConfiguration left lane maxima mutated.
Phase 1 widened lanes, then setCommandLaneGroup could throw (e.g.
sum(reservations) > budget) with no rollback. No drain ran, so the old test's
activeCount === 0 assertion passed — but the lane sat at the new width governed
by NO group, and the next unrelated drain trigger would dispatch its preserved
queue ungoverned. The function comment promised exactly what the code did not
do. Validation is now a distinct phase 0 over every group spec before anything
is mutated; validateCommandLaneGroupSpec/installCommandLaneGroup split out so
setCommandLaneGroup and the transaction share one validation path.

BUG — hooks-off skipped group teardown when the grouped lane was suspended.
applyGatewayLaneConcurrency published only when the lane map was non-empty. With
hooks off, cron-nested is the only lane that can enter it, so a suspended
cron-nested left the map empty and clearGroups was never published. A previously
installed cron-hooks group survived, and the member resumed still paying a
reservation for a hook lane receiving no work. Now publishes whenever hooks are
disabled, regardless of the lane map.

Also (review item 4): a lane may now belong to at most one group.
installCommandLaneGroup removes it from any prior owner's members, which
otherwise kept counting its active tasks toward a budget it had left. Not
reachable with the single default group, but this is a public API.

Tests: 3 new. Mutation-verified —
- validating during install instead of before: 'expected 8 to be +0'
- restoring the non-empty-lane-map guard: 'expected cron-hooks to be undefined'
  (this one initially SURVIVED; the first version of the teardown test never
  simulated suspension, so it could not see the bug. Now seeds a cleared lane
  resume and publishes via the gatewayStart path.)
- hooks-off now proves it DRAINS the work its teardown releases, not just that
  membership was deleted.

Test teardown fixed: resetAllLanes preserves queued entries by design, so work
on a lane that never opened never settles. clearCommandLane rejects it instead.

Typecheck clean (tsgo core + core test, exit 0).

Refs: openclaw#98813

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(queue): make setCommandLaneGroup self-waking; guard the clearGroups+invalid case

Round-5 implementation review from costaff-lapclaw-001 independently found the
same two bugs fiducian did (rejected-publish partial mutation, and hooks-off
teardown skipped when the grouped lane is suspended) — both already fixed in
0eb96a7. It raised two things fiducian did not:

1. The exported setCommandLaneGroup primitive was not self-waking. Replacing a
   group can FREE capacity — wider budget, dropped reservation, removed member —
   and queued members must not sit behind capacity that is already available.
   publishLaneConfiguration drains at commit, but the bare primitive is exported
   and its "replace" semantics silently stranded members until an unrelated
   poke. Now drains the union of previous and next members.

2. clearGroups combined with an invalid replacement was the worst case: the old
   group could be removed before the new one threw, leaving BOTH lane width and
   group membership partially committed. Phase 0 validation already ran before
   the clear after 0eb96a7, but nothing pinned it.

Also documents a limitation costaff noted: reservations are not validated
against the member's own maxConcurrent, because lane widths and group
definitions are published together and the width may not be applied yet at
validation time. A too-large reservation is accepted but partly unusable.

Tests: 2 new. Mutation-verified —
- removing the self-wake: 'expected 2 to be 5'
- validating during install instead of before the clear:
  'expected undefined to be cron-hooks' (the existing group torn down by a
  rejected replacement — costaff's exact worst case)

Reviewer agreement on the rest: admission arithmetic sound for the concrete
config, clearCommandLane correctly not wired (frees no active capacity), the
peak-occupancy publication test is the right shape, the static deadlock deny set
matches the known synchronous-wait lanes, and shouldNoteLaneWait's export is the
right trade over asserting a copied closure.

Typecheck clean (tsgo core + core test, exit 0).

Refs: openclaw#98813

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* style(queue): satisfy oxlint in the capacity-group tests

`check-lint` was failing on 21 errors across the four new suites:

- 17 `curly`: single-statement `for`/`for-of` bodies without braces.
- 4 `no-promise-executor-return`: `new Promise((resolve) => setTimeout(resolve, 0))`
  implicitly returns the Timeout handle from the executor. Rewritten to the
  braced form already used ~85 times elsewhere in the repo, e.g.
  `src/plugins/install-paths.test.ts:41`.

No behaviour change; the suites pass unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor(queue): split capacity groups and shared state out of command-queue.ts

`check-lint` was failing `max-lines` on src/process/command-queue.ts: 918
counted lines against the repo cap of 700. The file was already only 51 lines
under the cap before this branch, so the new capacity-group code could not fit
in it.

Two pure moves, no logic change:

- `command-queue.state.ts` — the globalThis-backed queue singleton
  (`getQueueState`, same `Symbol.for` key), `normalizeLane`, and the
  `QueueEntry` / `LaneState` / `ActiveTaskWaiter` / `CommandLaneTaskMarker`
  types. Lets the group policy read lane state without importing the queue.
- `command-queue.capacity-groups.ts` — the group registry, eligibility policy,
  spec validation, install, and the block-reason computation.

The four near-identical "drain these member lanes" loops collapse into one
`drainMembers` helper. It keeps the load-bearing part of each original: the
lane is looked up rather than created, because `drainLane` goes through
`getLaneState` and would resurrect a scoped lane that
`retireIdleScopedCommandLane` had just removed. The one loop that additionally
tested `maxConcurrent > 0` loses that check, which was an optimisation only —
a zero-width lane's pump admits nothing.

The dependency on `drainLane` is passed in as a parameter rather than imported,
so the new modules stay acyclic; `setCommandLaneGroup`, `clearCommandLaneGroup`
and `drainCommandLaneGroup` remain exported from command-queue.ts as thin
wrappers, and every previously exported symbol is still exported from there.

command-queue.ts is now 676 counted lines; all three modules are under the cap.

Verified: `tsgo:core`, `tsgo:core:test`, `oxfmt --check`, `oxlint` all clean;
1007 tests across the 43 suites that touch command-queue pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(queue): allow concurrent hook dispatch within cron budget

* test(gateway): prove hook burst concurrency stays bounded

* refactor(queue): keep capacity groups internal

* test(gateway): isolate steady-state hook admission

* fix(gateway): close hook lane on disable

* fix(gateway): retarget suspended hook resumes

* fix(gateway): restore hook group before lane resume

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
2026-08-01 11:49:16 +08:00

247 lines
9.1 KiB
TypeScript

// Capacity groups: a shared, hard aggregate budget across several command
// lanes, with per-member reservations. Split out of command-queue.ts to keep
// that file within its size budget; the queue supplies its own `drainLane` so
// this module never has to import back into it.
import { getQueueState, normalizeLane } from "./command-queue.state.js";
import { CommandLane } from "./lanes.js";
/** Drains a single lane. Supplied by command-queue.ts to avoid a cycle. */
type DrainLaneFn = (lane: string) => void;
/** Why a lane cannot admit, from the narrowest cause outward. */
export type CommandLaneBlockReason = "lane" | "group-budget" | "sibling-reservation" | null;
/** Declares a group's shared budget and its members' hard reservations. */
export type CommandLaneGroupSpec = {
/** Hard aggregate cap across all members. */
budget: number;
members: readonly string[];
/**
* Slots a member may always claim, non-borrowable by siblings.
*
* Not validated against the member's own `maxConcurrent`, because lane widths
* and group definitions are published together and the width may not be
* applied yet at validation time. A reservation larger than the lane's width
* is therefore accepted but partly unusable: the excess is withheld from
* siblings while its owner cannot claim it.
*/
reservations?: Readonly<Record<string, number>>;
};
export type LaneGroupState = {
group: string;
budget: number;
members: Set<string>;
reservations: Map<string, number>;
};
/**
* Lanes that must never join a group, because a group member can be made to
* wait for a sibling and these lanes can be synchronously awaited by other
* lanes — which would turn a wait into a deadlock.
*
* Known wait edges at this base: outer `cron` -> `cron-nested`
* (`server-cron.ts` passes lane "cron"; `agents/lanes.ts` remaps inner work),
* and `session:<key>` -> global lane (embedded-agent-runner run + compaction).
*/
const GROUP_INELIGIBLE_LANES: ReadonlySet<string> = new Set<string>([
CommandLane.Cron,
CommandLane.Main,
CommandLane.Subagent,
CommandLane.Nested,
]);
const GROUP_INELIGIBLE_PREFIXES = ["session:", "nested:", "context-engine-turn-maintenance:"];
function assertGroupEligibleLane(lane: string): void {
if (GROUP_INELIGIBLE_LANES.has(lane)) {
throw new Error(
`command lane "${lane}" cannot join a capacity group: it can be synchronously awaited by another lane`,
);
}
for (const prefix of GROUP_INELIGIBLE_PREFIXES) {
if (lane.startsWith(prefix)) {
throw new Error(
`command lane "${lane}" cannot join a capacity group: "${prefix}*" lanes can be synchronously awaited`,
);
}
}
}
/** Group registry, keyed by group id and by member lane name. */
export function getGroupRegistry(): {
groups: Map<string, LaneGroupState>;
groupByLane: Map<string, string>;
} {
const state = getQueueState() as unknown as {
laneGroups?: Map<string, LaneGroupState>;
laneGroupByLane?: Map<string, string>;
};
// Migration: an older singleton (pre-upgrade, inherited via globalThis after
// a SIGUSR1 in-process restart) has neither field. Active counts are derived,
// so a late-initialized registry cannot desynchronize from lane state.
if (!state.laneGroups) {
state.laneGroups = new Map<string, LaneGroupState>();
}
if (!state.laneGroupByLane) {
state.laneGroupByLane = new Map<string, string>();
}
return { groups: state.laneGroups, groupByLane: state.laneGroupByLane };
}
export function getLaneGroup(lane: string): LaneGroupState | undefined {
const { groups, groupByLane } = getGroupRegistry();
const groupId = groupByLane.get(lane);
return groupId ? groups.get(groupId) : undefined;
}
/**
* Active task count for a group member WITHOUT creating the lane. Creating it
* here would resurrect lanes that `retireIdleScopedCommandLane` just removed.
*/
export function getMemberActiveCount(lane: string): number {
return getQueueState().lanes.get(lane)?.activeTaskIds.size ?? 0;
}
/**
* Why `lane` cannot admit another task, or null if it can.
*
* Group capacity is always DERIVED from members' `activeTaskIds`, never tracked
* in a separate counter. That is what makes timeout, abort, clear, reset and
* stale-generation completion release capacity for free: they all remove the
* task id, so the next admission decision simply sees a smaller number. The
* only remaining obligation is that those paths re-drain the group.
*/
export function resolveLaneBlockReason(lane: string): CommandLaneBlockReason {
const state = getQueueState().lanes.get(lane);
if (state && state.activeTaskIds.size >= state.maxConcurrent) {
return "lane";
}
const group = getLaneGroup(lane);
if (!group) {
return null;
}
let groupActive = 0;
let siblingReserveHeld = 0;
for (const member of group.members) {
const active = getMemberActiveCount(member);
groupActive += active;
if (member !== lane) {
// Unused portion of a sibling's reservation. Held back even while that
// sibling is idle — a hard reservation that siblings can borrow is not a
// reservation at all.
siblingReserveHeld += Math.max(0, (group.reservations.get(member) ?? 0) - active);
}
}
if (groupActive >= group.budget) {
return "group-budget";
}
// Own reservation still unfilled: admit regardless of what siblings hold.
if (getMemberActiveCount(lane) < (group.reservations.get(lane) ?? 0)) {
return null;
}
// Otherwise this task would be borrowing unreserved capacity, which must not
// eat into what siblings are guaranteed.
return groupActive + siblingReserveHeld < group.budget ? null : "sibling-reservation";
}
export function canAdmitInGroup(lane: string): boolean {
const reason = resolveLaneBlockReason(lane);
return reason === null || reason === "lane";
}
/**
* Define or replace a capacity group.
*
* Membership is held here, keyed by lane name, and deliberately NOT inside
* `LaneState`: `setCommandLaneConcurrency` must not be able to detach a lane
* from its group, or session suspend/resume would silently restore a member to
* ungoverned concurrency.
*/
export function validateCommandLaneGroupSpec(
group: string,
spec: CommandLaneGroupSpec,
): LaneGroupState {
const members = spec.members.map((member) => normalizeLane(member));
for (const member of members) {
assertGroupEligibleLane(member);
}
const reservations = new Map<string, number>();
let reservedTotal = 0;
for (const [rawLane, count] of Object.entries(spec.reservations ?? {})) {
const member = normalizeLane(rawLane);
if (!members.includes(member)) {
throw new Error(`command lane group "${group}" reserves for non-member lane "${member}"`);
}
const reserved = Math.max(0, Math.floor(count));
reservations.set(member, reserved);
reservedTotal += reserved;
}
const budget = Math.max(0, Math.floor(spec.budget));
if (reservedTotal > budget) {
// Silent starvation otherwise: reservations that cannot all be honoured
// would permanently withhold capacity no member is able to claim.
throw new Error(
`command lane group "${group}" reserves ${reservedTotal} slots but its budget is ${budget}`,
);
}
return { group, budget, members: new Set(members), reservations };
}
/** Install a validated group, detaching its members from any previous owner. */
export function installCommandLaneGroup(next: LaneGroupState): void {
const { groups, groupByLane } = getGroupRegistry();
const previous = groups.get(next.group);
if (previous) {
for (const member of previous.members) {
groupByLane.delete(member);
}
}
for (const member of next.members) {
// A lane may belong to at most one group. Without this, the old owner's
// `members` would still contain the lane and would keep counting its active
// tasks toward a budget it no longer participates in.
const owner = groupByLane.get(member);
if (owner && owner !== next.group) {
groups.get(owner)?.members.delete(member);
}
}
groups.set(next.group, next);
for (const member of next.members) {
groupByLane.set(member, next.group);
}
}
/**
* Drain the given member lanes.
*
* Never creates a lane: `drainLane` calls `getLaneState`, which would resurrect
* a scoped lane that `retireIdleScopedCommandLane` had just removed. A lane
* whose width is 0 admits nothing when pumped, so no width check is needed
* here — it would only skip a call that is already a no-op.
*/
function drainMembers(lanes: Iterable<string>, drainLane: DrainLaneFn): void {
for (const lane of lanes) {
const state = getQueueState().lanes.get(lane);
if (state && state.queue.length > 0 && !state.draining) {
drainLane(lane);
}
}
}
/**
* Re-drain every OTHER member of `lane`'s group. Capacity that a completion
* frees belongs to the group, not to the lane that freed it, so a lane-local
* pump would leave siblings queued behind capacity that is already available.
*/
export function drainGroupSiblings(lane: string, drainLane: DrainLaneFn): void {
const group = getLaneGroup(lane);
if (!group) {
return;
}
drainMembers(
[...group.members].filter((member) => member !== lane),
drainLane,
);
}