From 0928ac7d7f760ea3491767400f290f8646dadf4f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 28 Jul 2026 16:17:26 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=A2=20fix:=20Cap=20Label=20Generation,?= =?UTF-8?q?=20Order=20the=20Flag=20Persist,=20Detach=20Settled=20Listeners?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-nineteen review: three fixed; the fourth is the ninth instance of the edited+reconnect index limitation, answered on-thread. - Generation cap (host.ts): stripping the primary output caps left label calls with NO cap at all — `normalizeLabelOutput` bounds what persists, not what the provider generates and bills, so a model ignoring the 4–9-word instruction (or steered by injected tool output) could emit its provider-default output per batch. The sanitize step now installs a 256-token label cap (per provider family: `maxOutputTokens` for Google-style wrappers, `maxTokens` otherwise), after the filter so the omit set cannot remove it. - Flag-persist ordering (client.js): the `markActivityLabels` write was fire-and-forget, so an immediate cross-replica reconnect could read the job between the write and the first claim, see neither flag nor snapshot label, and skip gap reconciliation. Label emission now awaits the (settled-on-failure) persist chain, making "a label event exists" imply "the flag is durable" — the race window is gone; only the documented double-write-failure residual remains. - Listener detach (client.js): each HITL approval cycle's wiring adds a `once` abort listener to the shared job signal that only an actual abort removes; settled segments now detach theirs in `settleActivityLabels`, so long multi-approval runs cannot accumulate dead closures toward the listener-limit warning. Tests: the primary cap is REPLACED by the 256-token label cap (host.spec). --- api/server/controllers/agents/client.js | 31 ++++++++++++++++--- .../activityLabels/__tests__/host.spec.ts | 8 +++-- .../api/src/agents/activityLabels/host.ts | 17 ++++++++++ 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 7d47903f3c..e8e3cc2dc9 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -658,6 +658,12 @@ class AgentClient extends BaseClient { scope.abort.abort(); } }); + /** Settled either way — the abort listener has no remaining work, and + * leaving it attached across HITL approval cycles accumulates dead + * closures on the shared job signal. Idempotent on double settle. */ + for (const scope of this.activityLabelScopes ?? []) { + scope.detach?.(); + } } /** @@ -712,7 +718,10 @@ class AgentClient extends BaseClient { * content itself recorded perfectly well. One retry costs nothing at run * setup and removes the only realistic way the gate goes stale. */ - void GenerationJobManager.markActivityLabels(streamId).catch(() => + /** Retained (not fire-and-forget): label emission is ORDERED after this + * persist, below. The chain settles on failure (warned retry), so a + * lost write can never wedge label emission. */ + const activityLabelsMarked = GenerationJobManager.markActivityLabels(streamId).catch(() => GenerationJobManager.markActivityLabels(streamId).catch(() => { logger.warn( `[AgentClient] Could not flag activity labels for ${streamId}; a label resolving during a resume gap may not be reconciled.`, @@ -756,6 +765,12 @@ class AgentClient extends BaseClient { closeOnAbort(); } else { abortSignal.addEventListener('abort', closeOnAbort, { once: true }); + /** Detached once this segment settles: HITL runs rebuild a wiring + * per approval cycle on the SAME job signal, and `once` only + * removes the listener if an abort actually fires — long + * multi-approval runs would otherwise accumulate obsolete + * closures toward the listener-limit warning. */ + labelScope.detach = () => abortSignal.removeEventListener('abort', closeOnAbort); } } /** Thin wrapper: slot claiming, lane stamping, emit ordering, and settle @@ -770,8 +785,15 @@ class AgentClient extends BaseClient { bumpIndexOffset: () => { this.steerOffsetState.offset += 1; }, - emitLabelEvent: (index, part) => - GenerationJobManager.emitChunk( + emitLabelEvent: async (index, part) => { + /** ORDERED after the flag persist: resume-gap reconciliation is + * gated on the flag, so a label event must never exist before the + * flag does — an immediate cross-replica reconnect could otherwise + * read the job between the two writes, see neither flag nor + * snapshot label, and skip reconciling a label claimed in the + * snapshot→subscribe window. */ + await activityLabelsMarked; + return GenerationJobManager.emitChunk( streamId, { event: ActivityLabelEvents.ON_ACTIVITY_LABEL, @@ -788,7 +810,8 @@ class AgentClient extends BaseClient { * the new response — invisibly, since an empty label renders * nothing — overwriting whatever occupies that slot. */ { durable: true, expectedCreatedAt: this.jobCreatedAt }, - ), + ); + }, trackPendingFill: (fillDone) => { this.pendingActivityLabelFills = this.pendingActivityLabelFills ?? []; this.pendingActivityLabelFills.push(fillDone); diff --git a/packages/api/src/agents/activityLabels/__tests__/host.spec.ts b/packages/api/src/agents/activityLabels/__tests__/host.spec.ts index d201ec2d9f..f6d5d35927 100644 --- a/packages/api/src/agents/activityLabels/__tests__/host.spec.ts +++ b/packages/api/src/agents/activityLabels/__tests__/host.spec.ts @@ -185,12 +185,16 @@ describe('resolveActivityLabelModel model precedence', () => { }); }); - it('strips the primary maxTokens like the title path', async () => { + /** The primary cap is not merely stripped — it is REPLACED with a small + * label-specific one, so a model ignoring the 4–9-word instruction (or + * steered by injected tool output) cannot generate and bill its + * provider-default output for a header. */ + it('replaces the primary maxTokens with the small label cap', async () => { mockGetOptions.mockResolvedValueOnce({ llmConfig: { model: 'resolved', maxTokens: 64_000 }, } as never); const resolved = await resolve({ activityLabel: true, activityModel: 'label-model' }); - expect((resolved.clientOptions as Record).maxTokens).toBeUndefined(); + expect((resolved.clientOptions as Record).maxTokens).toBe(256); }); /** The Anthropic carrier holds client CONSTRUCTION options — for diff --git a/packages/api/src/agents/activityLabels/host.ts b/packages/api/src/agents/activityLabels/host.ts index b62672b93f..92bc5bd096 100644 --- a/packages/api/src/agents/activityLabels/host.ts +++ b/packages/api/src/agents/activityLabels/host.ts @@ -116,6 +116,14 @@ type MaybeAzureConfig = ClientOptions & { configuration?: OpenAIConfiguration; }; +/** Generation cap for label calls — ~25x the largest legitimate 4–9 word + * header, so truncation can never clip a real label. Replaces the stripped + * primary caps: with NO cap, a model that ignores the instruction (or is + * steered by injection in untrusted tool output) generates and BILLS its + * provider-default output on every batch; `normalizeLabelOutput` bounds + * only what persists, not what the provider generates. */ +const LABEL_MAX_OUTPUT_TOKENS = 256; + /** Effective activity-label settings for one endpoint. */ export interface ResolvedActivityConfig { enabled: boolean; @@ -330,6 +338,15 @@ export async function resolveActivityLabelModel({ if (anthropicCarrier != null && clientOptions.clientOptions == null) { clientOptions.clientOptions = anthropicCarrier; } + /** Replace the stripped primary caps with a SMALL one — see + * {@link LABEL_MAX_OUTPUT_TOKENS}. Installed after the filter so the + * omit set cannot remove it; keyed per provider family (Google-style + * wrappers read `maxOutputTokens`, the rest `maxTokens`). */ + if (provider === Providers.GOOGLE || provider === Providers.VERTEXAI) { + (clientOptions as { maxOutputTokens?: number }).maxOutputTokens = LABEL_MAX_OUTPUT_TOKENS; + } else { + (clientOptions as { maxTokens?: number }).maxTokens = LABEL_MAX_OUTPUT_TOKENS; + } if (options.configOptions) { clientOptions.configuration = options.configOptions; }