Repository navigation
AFTER UPDATE: fix(generation): centralize connection admission and provider fallback #4423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5a4c047
7f5a6cc
84f28a3
c5e88a6
4499a53
8b8cb1f
f727e13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| import type { ChatCompletionResult, ChatMessage, ChatOptions, LLMUsage } from "../llm/base-provider.js"; | ||
| import { BaseLLMProvider } from "../llm/base-provider.js"; | ||
| import { logger } from "../../lib/logger.js"; | ||
|
|
||
| // Admission keys identify one physical provider endpoint. Text work keys on the configured | ||
| // connection id; image work keys on the resolved base URL plus the endpoint id where the | ||
| // backend needs one, because most image callers have no connection row in scope and | ||
| // unregistered foreground work would defeat the priority rule. The keyspaces do not overlap. | ||
| type ConnectionState = { | ||
| foregroundActive: number; | ||
| backgroundActive: boolean; | ||
| lastForegroundFinishedAt: number; | ||
| }; | ||
|
|
||
| const states = new Map<string, ConnectionState>(); | ||
| export const BACKGROUND_CONNECTION_IDLE_MS = 30_000; | ||
|
|
||
| export type ConnectionAttemptOutcome = "completed" | "failed"; | ||
| export type ConnectionAttemptFinalizer = (outcome: ConnectionAttemptOutcome) => void | Promise<void>; | ||
| export type ConnectionAdmissionMode = | ||
| | { kind: "foreground" } | ||
| | { | ||
| kind: "background"; | ||
| beforeAttempt?: () => void | ConnectionAttemptFinalizer | Promise<void | ConnectionAttemptFinalizer>; | ||
| }; | ||
|
|
||
| export class BackgroundConnectionBusyError extends Error { | ||
| constructor(readonly connectionId: string) { | ||
| super(`Connection ${connectionId} is not available for background generation.`); | ||
| this.name = "BackgroundConnectionBusyError"; | ||
| } | ||
| } | ||
|
|
||
| export class ConnectionAttemptRejectedError extends Error { | ||
| constructor(readonly cause: unknown) { | ||
| super("Connection attempt was rejected before provider work started.", { cause }); | ||
| this.name = "ConnectionAttemptRejectedError"; | ||
| } | ||
| } | ||
|
|
||
| export class ConnectionAttemptFinalizationError extends Error { | ||
| constructor(readonly cause: unknown) { | ||
| super("Connection attempt accounting failed after provider work finished.", { cause }); | ||
| this.name = "ConnectionAttemptFinalizationError"; | ||
| } | ||
| } | ||
|
|
||
| export function isConnectionAdmissionFailure(error: unknown): boolean { | ||
| return ( | ||
| error instanceof BackgroundConnectionBusyError || | ||
| error instanceof ConnectionAttemptRejectedError || | ||
| error instanceof ConnectionAttemptFinalizationError | ||
| ); | ||
| } | ||
|
|
||
| function stateFor(connectionId: string): ConnectionState { | ||
| const existing = states.get(connectionId); | ||
| if (existing) return existing; | ||
| const state = { foregroundActive: 0, backgroundActive: false, lastForegroundFinishedAt: 0 }; | ||
| states.set(connectionId, state); | ||
| return state; | ||
| } | ||
|
|
||
| export function beginForegroundConnection(connectionId: string): () => void { | ||
| const state = stateFor(connectionId); | ||
| state.foregroundActive += 1; | ||
| let released = false; | ||
| return () => { | ||
| if (released) return; | ||
| released = true; | ||
| state.foregroundActive -= 1; | ||
| state.lastForegroundFinishedAt = Date.now(); | ||
| }; | ||
| } | ||
|
|
||
| export function tryBackgroundConnection( | ||
| connectionId: string, | ||
| at: Date, | ||
| ): { acquired: false } | { acquired: true; release: () => void } { | ||
| const state = stateFor(connectionId); | ||
| if ( | ||
| state.backgroundActive || | ||
| state.foregroundActive > 0 || | ||
| at.getTime() - state.lastForegroundFinishedAt < BACKGROUND_CONNECTION_IDLE_MS | ||
| ) { | ||
| return { acquired: false }; | ||
| } | ||
| state.backgroundActive = true; | ||
| return { | ||
| acquired: true, | ||
| release: () => { | ||
| state.backgroundActive = false; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export function resetConnectionAdmissionForTests(): void { | ||
| states.clear(); | ||
| } | ||
|
|
||
| async function beginConnectionAttempt( | ||
| connectionId: string, | ||
| mode: ConnectionAdmissionMode, | ||
| ): Promise<{ release: () => void; finalize?: ConnectionAttemptFinalizer }> { | ||
| if (mode.kind === "foreground") return { release: beginForegroundConnection(connectionId) }; | ||
|
|
||
| const admission = tryBackgroundConnection(connectionId, new Date()); | ||
| if (!admission.acquired) throw new BackgroundConnectionBusyError(connectionId); | ||
| try { | ||
| return { release: admission.release, finalize: (await mode.beforeAttempt?.()) || undefined }; | ||
| } catch (error) { | ||
| admission.release(); | ||
| throw new ConnectionAttemptRejectedError(error); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Record the attempt outcome without letting accounting failures overwrite a provider | ||
| * error: the operation's own failure is what the caller (and the fallback chain) needs to | ||
| * see. A finalization failure only surfaces when the operation itself succeeded. | ||
| */ | ||
| async function finalizeConnectionAttempt( | ||
| attempt: { release: () => void; finalize?: ConnectionAttemptFinalizer }, | ||
| outcome: ConnectionAttemptOutcome, | ||
| ): Promise<void> { | ||
| try { | ||
| await attempt.finalize?.(outcome); | ||
| } catch (error) { | ||
| if (outcome === "completed") throw new ConnectionAttemptFinalizationError(error); | ||
| logger.error(error, "[connection-admission] Attempt accounting failed after a failed provider call"); | ||
| } finally { | ||
| attempt.release(); | ||
| } | ||
| } | ||
|
Comment on lines
+122
to
+134
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Inspect background finalizers passed as beforeAttempt to confirm what a failed finalize means for streamed output.
set -euo pipefail
rg -n -C 12 'beforeAttempt' --glob '*.ts' packages/server/src scriptsRepository: Pasta-Devs/Marinara-Engine Length of output: 9339 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '--- connection-admission relevant section ---\n'
sed -n '70,190p' packages/server/src/services/generation/connection-admission.ts | cat -n
printf '\n--- ConnectionFallbackProvider relevant references ---\n'
rg -n -C 8 'finalizeConnectionAttempt|ChatCompletion|chat\(|chatComplete|ConnectionAttemptFinalizationError|chatComplete' packages/server/src/services/llm/connection-fallback-provider.ts packages/server/src/services/generation/connection-admission.ts
printf '\n--- all finalizeConnectionAttempt usages ---\n'
rg -n -C 8 'finalizeConnectionAttempt' packages/server/srcRepository: Pasta-Devs/Marinara-Engine Length of output: 255 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- connection-admission relevant section ---'
sed -n '70,190p' packages/server/src/services/generation/connection-admission.ts | cat -n
printf '%s\n' ''
printf '%s\n' '--- ConnectionFallbackProvider relevant references ---'
rg -n -C 8 'finalizeConnectionAttempt|ChatCompletion|chat\(|chatComplete|ConnectionAttemptFinalizationError|chatComplete' packages/server/src/services/llm/connection-fallback-provider.ts packages/server/src/services/generation/connection-admission.ts
printf '%s\n$' ''
printf '%s\n' '--- all finalizeConnectionAttempt usages ---'
rg -n -C 8 'finalizeConnectionAttempt' packages/server/srcRepository: Pasta-Devs/Marinara-Engine Length of output: 29615 Avoid promoting late finalizer failures to stream failure after output is emitted.
🤖 Prompt for AI Agents |
||
|
|
||
| export async function withConnectionAdmission<T>( | ||
| connectionId: string, | ||
| mode: ConnectionAdmissionMode, | ||
| operation: () => Promise<T>, | ||
| ): Promise<T> { | ||
| const attempt = await beginConnectionAttempt(connectionId, mode); | ||
| let outcome: ConnectionAttemptOutcome = "failed"; | ||
| try { | ||
| const result = await operation(); | ||
| outcome = "completed"; | ||
| return result; | ||
| } finally { | ||
| await finalizeConnectionAttempt(attempt, outcome); | ||
| } | ||
| } | ||
|
|
||
| export class ConnectionAdmissionProvider extends BaseLLMProvider { | ||
| constructor( | ||
| readonly provider: BaseLLMProvider, | ||
| private readonly connectionId: string, | ||
| private readonly mode: ConnectionAdmissionMode = { kind: "foreground" }, | ||
| ) { | ||
| super("", "", provider.maxContextValue ?? undefined, null, provider.maxTokensOverrideValue); | ||
| } | ||
|
|
||
| async *chat(messages: ChatMessage[], options: ChatOptions): AsyncGenerator<string, LLMUsage | void, unknown> { | ||
| const attempt = await beginConnectionAttempt(this.connectionId, this.mode); | ||
| let outcome: ConnectionAttemptOutcome = "failed"; | ||
| try { | ||
| const result = yield* this.provider.chat(messages, options); | ||
| outcome = "completed"; | ||
| return result; | ||
| } finally { | ||
| await finalizeConnectionAttempt(attempt, outcome); | ||
| } | ||
| } | ||
|
Comment on lines
+161
to
+171
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Find manual iteration of provider chat generators that may skip generator close.
set -euo pipefail
echo "--- manual .next() iteration over chat generators ---"
rg -n -C 10 --glob '*.ts' '\.chat\s*\(' packages/server/src | rg -n -C 6 '\.next\s*\(' || true
echo "--- explicit generator close calls ---"
rg -n -C 4 --glob '*.ts' '\.return\s*\(\s*undefined\s*\)' packages/server/src || true
echo "--- for await consumers of chat ---"
rg -n -C 4 --glob '*.ts' 'for await' packages/server/src | rg -n 'chat' || trueRepository: Pasta-Devs/Marinara-Engine Length of output: 5412 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- locate relevant files ---"
fd -a 'connection-admission|connection-fallback-provider|base-provider|game\.routes|connections\.routes|providers' packages/server/src | sed 's#^\./##'
echo "--- connection-admission outline and relevant section ---"
ast-grep outline packages/server/src/services/generation/connection-admission.ts --view compact || true
sed -n '1,230p' packages/server/src/services/generation/connection-admission.ts
echo "--- base-provider chat generation outline/search around generateContentWithStream ---"
ast-grep outline packages/server/src/services/llm/base-provider.ts --view compact || true
sed -n '620,760p' packages/server/src/services/llm/base-provider.tsRepository: Pasta-Devs/Marinara-Engine Length of output: 13692 Close
🤖 Prompt for AI Agents |
||
|
|
||
| async chatComplete(messages: ChatMessage[], options: ChatOptions): Promise<ChatCompletionResult> { | ||
| return withConnectionAdmission(this.connectionId, this.mode, () => this.provider.chatComplete(messages, options)); | ||
| } | ||
|
|
||
| async embed(texts: string[], model: string, signal?: AbortSignal): Promise<number[][]> { | ||
| return withConnectionAdmission(this.connectionId, this.mode, () => this.provider.embed(texts, model, signal)); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Look through the admission decorator to the provider that actually talks to the backend. | ||
| * Diagnostics that report a provider class need the concrete one, not the wrapper. | ||
| */ | ||
| export function unwrapConnectionAdmissionProvider(provider: BaseLLMProvider): BaseLLMProvider { | ||
| return provider instanceof ConnectionAdmissionProvider ? provider.provider : provider; | ||
| } | ||
|
|
||
| export function withConnectionAdmissionProvider( | ||
| provider: BaseLLMProvider, | ||
| connectionId: string, | ||
| mode: ConnectionAdmissionMode = { kind: "foreground" }, | ||
| ): BaseLLMProvider { | ||
| return new ConnectionAdmissionProvider(provider, connectionId, mode); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
statesgrows without bound and is never pruned.stateskeeps one entry per connection id and per image admission key forever. Entries stay after the connection is deleted or the endpoint changes. The entry is small, so this is a slow leak rather than an outage, but nothing removes idle entries.Consider deleting the state when
foregroundActive === 0,backgroundActive === false, and the idle window has elapsed. Do this inside the release paths.🤖 Prompt for AI Agents