From d5e86a23dbee8b2d3ba317724c219e187bd52767 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 28 Jul 2026 09:22:28 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=B3=20fix:=20Price=20Label=20Cache=20C?= =?UTF-8?q?orrectly,=20Honor=20endpoints.agents,=20Cancel=20Every=20Retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-twelve review: four fixed here; the remaining P1 (move the client.js bridge into packages/api) is an architecture call answered on-thread for the maintainer. - Provider on billed entries (client.js, P1): round ten added cache details to label usage entries but not `provider`, and `splitUsage` treats an unknown provider as additive — re-adding cache_read and cache_creation on top of an input count that already contains them, double-charging Anthropic/OpenAI cached label calls while the streamed cost (which carried the provider) disagreed. Every mapped entry now carries the label endpoint's provider. - endpoints.agents honored (host.ts, client.js): `initializeAgent` rewrites `agent.endpoint` to the backing provider, so activity settings under the PUBLIC `agents` endpoint — valid config, inherited by `agentsEndpointSchema` — were silently ignored. Field resolution is now `all` > public endpoint > backing provider/custom, applied to both the enable gate and the model/titleModel resolution. - E2E_LABEL_PORT reaches the YAML (playwright.config.mock.ts): an overridden port moved the fake label server and its health check but not the generated config's hard-coded 8889 baseURLs, so readiness passed while every label request targeted the wrong port. The override is now substituted into the generated copy. - Every retry frame cancelled (useResumableSSE.ts): concurrent label retry chains (reservation + fill per slot) overwrote one rAF handle, so cleanup cancelled only the newest chain; the rest ran up to 120 frames past unmount and could apply a stale label to a replacement generation reusing the same response id. Outstanding frame ids now live in a Set that cleanup drains. Tests: public-endpoint gate/precedence/all-above-public (host.spec). --- api/server/controllers/agents/client.js | 17 +++++++- client/src/hooks/SSE/useResumableSSE.ts | 22 +++++++--- e2e/playwright.config.mock.ts | 12 +++++- .../activityLabels/__tests__/host.spec.ts | 43 +++++++++++++++++++ .../api/src/agents/activityLabels/host.ts | 24 ++++++++++- 5 files changed, 106 insertions(+), 12 deletions(-) diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 8ec4de9909..7d47903f3c 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -369,6 +369,8 @@ class AgentClient extends BaseClient { resolveActivityLabelModel({ req: this.options.req, agent: this.options.agent, + /** Same public-endpoint-first field resolution as the wiring gate. */ + publicEndpoint: this.options.endpoint, ids: { messageId: this.responseMessageId, conversationId: this.conversationId, @@ -405,7 +407,15 @@ class AgentClient extends BaseClient { provider = undefined, ) { const appConfig = this.options.req?.config; - const collectedUsage = mapCollectedMetadataToUsage(collectedMetadata); + /** Provider ON EVERY ENTRY, not just the streamed event: `splitUsage` + * keys additive-vs-subset cache math on `usage.provider`, and an + * unknown provider takes the additive branch — for Anthropic/OpenAI + * (cache already inside `input_tokens`) that re-adds cache_read and + * cache_creation on top, double-charging the balance while the + * streamed cost (which carries the provider) disagrees. */ + const collectedUsage = mapCollectedMetadataToUsage(collectedMetadata).map((usage) => + provider != null ? { ...usage, provider } : usage, + ); if (collectedUsage.length === 0) { return; } @@ -684,6 +694,11 @@ class AgentClient extends BaseClient { appConfigForActivity, agentEndpoint, customEndpointConfig, + /** The PUBLIC endpoint (`agents`): `initializeAgent` rewrites + * `agent.endpoint` to the backing provider, so without this an + * admin's `endpoints.agents.activityLabel: true` reads the + * provider's block instead and the feature stays off. */ + this.options.endpoint, ); if (!activityConfig.enabled) { return undefined; diff --git a/client/src/hooks/SSE/useResumableSSE.ts b/client/src/hooks/SSE/useResumableSSE.ts index 7f04b9cfea..efcb5f7d64 100644 --- a/client/src/hooks/SSE/useResumableSSE.ts +++ b/client/src/hooks/SSE/useResumableSSE.ts @@ -549,7 +549,13 @@ export default function useResumableSSE( * bounded next-frame retry as pending actions, on its own handle so the * two retries can't cancel each other. */ const steerRetryRef = useRef(null); - const activityLabelRetryRef = useRef(null); + /** EVERY outstanding label-retry frame, not a single handle: labels fire + * two events per slot (reservation, then fill), so concurrent retry + * chains are the norm — a single ref would let cleanup cancel only the + * newest chain while the others kept running for up to 120 frames and + * could apply a stale label to a replacement generation that reuses the + * same response id (edits do). */ + const activityLabelRetryFramesRef = useRef>(new Set()); /** * Set once a SYNC has replaced the response with the server's * completion-local snapshot, which discards the prefix an edited @@ -836,9 +842,11 @@ export default function useResumableSSE( const applyActivityLabelToMessages = (event: TActivityLabelEvent, attempt = 0) => { const retryNextFrame = () => { if (attempt < PENDING_ACTION_MAX_RETRY_FRAMES) { - activityLabelRetryRef.current = requestAnimationFrame(() => - applyActivityLabelToMessages(event, attempt + 1), - ); + const frameId = requestAnimationFrame(() => { + activityLabelRetryFramesRef.current.delete(frameId); + applyActivityLabelToMessages(event, attempt + 1); + }); + activityLabelRetryFramesRef.current.add(frameId); } }; /** Same boundary as pending actions and steers: land queued deltas @@ -1888,10 +1896,10 @@ export default function useResumableSSE( cancelAnimationFrame(pendingActionRetryRef.current); pendingActionRetryRef.current = null; } - if (activityLabelRetryRef.current != null) { - cancelAnimationFrame(activityLabelRetryRef.current); - activityLabelRetryRef.current = null; + for (const frameId of activityLabelRetryFramesRef.current) { + cancelAnimationFrame(frameId); } + activityLabelRetryFramesRef.current.clear(); if (steerRetryRef.current != null) { cancelAnimationFrame(steerRetryRef.current); steerRetryRef.current = null; diff --git a/e2e/playwright.config.mock.ts b/e2e/playwright.config.mock.ts index 721df606ce..8c808b28dd 100644 --- a/e2e/playwright.config.mock.ts +++ b/e2e/playwright.config.mock.ts @@ -9,7 +9,8 @@ const mcpHttpServerPath = path.resolve(rootPath, 'e2e/setup/fake-mcp-http-server /** Must match the `e2e-http` server URL in e2e/config/librechat.e2e.yaml. */ const MCP_HTTP_PORT = process.env.E2E_MCP_HTTP_PORT || '8765'; const labelServerPath = path.resolve(rootPath, 'e2e/setup/fake-label-server.js'); -/** Must match the custom endpoints' `baseURL` in e2e/config/librechat.e2e.yaml. */ +/** The template's custom-endpoint `baseURL`s hard-code 8889; + * `writeRuntimeMockConfig` substitutes any override into the generated copy. */ const LABEL_PORT = process.env.E2E_LABEL_PORT || '8889'; const fakeModelHookPath = path.resolve(rootPath, 'e2e/setup/fake-model.js'); const configTemplatePath = path.resolve(rootPath, 'e2e/config/librechat.e2e.yaml'); @@ -58,10 +59,17 @@ const preservedCredentialEnvKeys = new Set([ */ function writeRuntimeMockConfig() { const template = fs.readFileSync(configTemplatePath, 'utf8'); - const config = + let config = process.env.E2E_MODEL_SPECS_ENFORCE === 'true' ? template.replace('\n enforce: false\n', '\n enforce: true\n') : template; + /** Keep the generated config in lockstep with the overridable label-server + * port: the template hard-codes 8889, so an `E2E_LABEL_PORT` override that + * moved only the server and its health check would report ready while + * every activity-label request went to the wrong port. */ + if (LABEL_PORT !== '8889') { + config = config.split('127.0.0.1:8889').join(`127.0.0.1:${LABEL_PORT}`); + } fs.mkdirSync(path.dirname(configPath), { recursive: true }); fs.writeFileSync(configPath, config); } diff --git a/packages/api/src/agents/activityLabels/__tests__/host.spec.ts b/packages/api/src/agents/activityLabels/__tests__/host.spec.ts index d31f465d09..50a3ff6196 100644 --- a/packages/api/src/agents/activityLabels/__tests__/host.spec.ts +++ b/packages/api/src/agents/activityLabels/__tests__/host.spec.ts @@ -78,6 +78,49 @@ describe('resolveActivityConfig', () => { expect(config.enabled).toBe(false); expect(config.model).toBe('gpt-4o-mini'); }); + + /** `initializeAgent` rewrites `agent.endpoint` to the backing provider, so + * the PUBLIC endpoint's block (`endpoints.agents`) must still be honored + * — it inherits every activity field via `agentsEndpointSchema`. */ + it('honors the public agents endpoint when the agent endpoint was rewritten', () => { + const config = resolveActivityConfig( + appConfig({ agents: { activityLabel: true, activityModel: 'agents-mini' } }), + 'openAI', + undefined, + 'agents', + ); + expect(config.enabled).toBe(true); + expect(config.model).toBe('agents-mini'); + }); + + it('lets the public endpoint win per field over the backing provider', () => { + const config = resolveActivityConfig( + appConfig({ + agents: { activityModel: 'agents-mini' }, + openAI: { activityLabel: true, activityModel: 'provider-mini', activityMaxPerRun: 3 }, + }), + 'openAI', + undefined, + 'agents', + ); + /** Field-wise: model from `agents`, the rest falls through to `openAI`. */ + expect(config.enabled).toBe(true); + expect(config.model).toBe('agents-mini'); + expect(config.maxPerRun).toBe(3); + }); + + it('keeps endpoints.all above the public endpoint', () => { + const config = resolveActivityConfig( + appConfig({ + all: { activityModel: 'shared-model' }, + agents: { activityLabel: true, activityModel: 'agents-mini' }, + }), + 'openAI', + undefined, + 'agents', + ); + expect(config.model).toBe('shared-model'); + }); }); describe('resolveActivityLabelModel model precedence', () => { diff --git a/packages/api/src/agents/activityLabels/host.ts b/packages/api/src/agents/activityLabels/host.ts index 18d68f46c7..846f874d1d 100644 --- a/packages/api/src/agents/activityLabels/host.ts +++ b/packages/api/src/agents/activityLabels/host.ts @@ -100,6 +100,10 @@ export interface ActivityLabelAgent { export interface ResolveActivityLabelModelParams { req: ServerRequest; agent: ActivityLabelAgent; + /** The PUBLIC endpoint the request came in on (e.g. `agents`) when it + * differs from the agent's rewritten provider endpoint — its config block + * wins per field over the provider's. */ + publicEndpoint?: string; /** Request-scoped ids for header placeholder resolution. */ ids: { messageId?: string; conversationId?: string; parentMessageId?: string }; db: EndpointDbMethods; @@ -141,22 +145,35 @@ function pickEndpointField( endpoint: string, customEndpointConfig: Partial | undefined, key: K, + publicEndpoint?: string, ): TEndpoint[K] | undefined { const endpoints = appConfig?.endpoints as | (Record & { all?: TEndpoint }) | undefined; const all = endpoints?.all as Partial | undefined; + /** The PUBLIC endpoint the request came in on, when it differs from the + * backing provider. `initializeAgent` rewrites `agent.endpoint` to the + * provider (an agents-endpoint run backed by OpenAI reads `openAI`), so + * without this an admin's `endpoints.agents.activityLabel: true` — valid + * config, since `agentsEndpointSchema` inherits every activity field — + * is silently ignored. Public wins per field over the backing provider, + * mirroring how request-path options resolve. */ + const publicBlock = + publicEndpoint != null && publicEndpoint !== endpoint + ? (endpoints?.[publicEndpoint] as Partial | undefined) + : undefined; const named = (endpoints?.[endpoint] ?? customEndpointConfig) as Partial | undefined; - return all?.[key] ?? named?.[key]; + return all?.[key] ?? publicBlock?.[key] ?? named?.[key]; } export function resolveActivityConfig( appConfig: AppConfig | undefined, endpoint: string, customEndpointConfig?: Partial, + publicEndpoint?: string, ): ResolvedActivityConfig { const pick = (key: K): TEndpoint[K] | undefined => - pickEndpointField(appConfig, endpoint, customEndpointConfig, key); + pickEndpointField(appConfig, endpoint, customEndpointConfig, key, publicEndpoint); return { enabled: pick('activityLabel') === true, model: pick('activityModel'), @@ -179,6 +196,7 @@ export function resolveActivityConfig( export async function resolveActivityLabelModel({ req, agent, + publicEndpoint, ids, db, }: ResolveActivityLabelModelParams): Promise { @@ -189,6 +207,7 @@ export async function resolveActivityLabelModel({ appConfig, agentEndpoint, providerConfig.customEndpointConfig, + publicEndpoint, ); /** @@ -205,6 +224,7 @@ export async function resolveActivityLabelModel({ agentEndpoint, providerConfig.customEndpointConfig, 'titleModel', + publicEndpoint, ); let endpoint = agentEndpoint; if (activity.endpoint != null && activity.endpoint !== agentEndpoint) {