mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
💳 fix: Price Label Cache Correctly, Honor endpoints.agents, Cancel Every Retry
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).
This commit is contained in:
parent
bb5c4c5e3b
commit
d5e86a23db
5 changed files with 106 additions and 12 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<number | null>(null);
|
||||
const activityLabelRetryRef = useRef<number | null>(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<Set<number>>(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;
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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<K extends keyof TEndpoint>(
|
|||
endpoint: string,
|
||||
customEndpointConfig: Partial<TEndpoint> | undefined,
|
||||
key: K,
|
||||
publicEndpoint?: string,
|
||||
): TEndpoint[K] | undefined {
|
||||
const endpoints = appConfig?.endpoints as
|
||||
| (Record<string, TEndpoint | undefined> & { all?: TEndpoint })
|
||||
| undefined;
|
||||
const all = endpoints?.all as Partial<TEndpoint> | 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<TEndpoint> | undefined)
|
||||
: undefined;
|
||||
const named = (endpoints?.[endpoint] ?? customEndpointConfig) as Partial<TEndpoint> | undefined;
|
||||
return all?.[key] ?? named?.[key];
|
||||
return all?.[key] ?? publicBlock?.[key] ?? named?.[key];
|
||||
}
|
||||
|
||||
export function resolveActivityConfig(
|
||||
appConfig: AppConfig | undefined,
|
||||
endpoint: string,
|
||||
customEndpointConfig?: Partial<TEndpoint>,
|
||||
publicEndpoint?: string,
|
||||
): ResolvedActivityConfig {
|
||||
const pick = <K extends keyof TEndpoint>(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<ActivityLabelLLM> {
|
||||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue