Repository navigation
Conversation
📝 WalkthroughWalkthroughThe change adds per-connection admission control for provider operations. Provider creation now receives connection IDs across routes and services. Image and fallback generation handle admission failures explicitly. Regression coverage validates admission, cleanup, and rejection behavior. ChangesConnection admission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RouteOrService
participant createLLMProvider
participant ConnectionAdmissionProvider
participant LLMProvider
RouteOrService->>createLLMProvider: provide connection ID
createLLMProvider->>ConnectionAdmissionProvider: wrap configured provider
ConnectionAdmissionProvider->>LLMProvider: admit and execute operation
LLMProvider-->>ConnectionAdmissionProvider: return result or error
ConnectionAdmissionProvider-->>RouteOrService: return admitted result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gunterlie
changed the title
Gunterlie
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/services/image/image-generation.ts`:
- Around line 123-124: Update generateImage and ImageGenRequest to carry the
selected primary connectionId/admission key, and use that key for admission on
both primary and fallback image-generation attempts instead of only
normalizedBaseUrl. When switching to the fallback provider, replace it with the
image-specific fallback connection id so each request is keyed by its actual
connection row.
In `@packages/server/src/services/llm/connection-fallback-provider.ts`:
- Around line 165-171: Update the finally cleanup around
generation.return(undefined) in the fallback provider to suppress any rejection
from the cleanup call, ensuring it cannot replace an error from
generation.next(). Preserve the cleanup attempt for abandoned consumers while
allowing the subsequent catch to inspect and handle the original provider error.
In `@packages/server/src/services/llm/provider-registry.ts`:
- Around line 165-166: The admission wrapper currently has competing owners,
causing duplicate wrapping and preventing background-mode requests. In
packages/server/src/services/llm/provider-registry.ts lines 165-166, choose one
ownership model by either passing an admission mode alongside connectionId or
removing the implicit withConnectionAdmissionProvider wrapping, and verify
background callers do not receive an implicit foreground wrapper. In
packages/server/src/services/llm/connection-fallback-provider.ts lines 207-233,
apply the fallback rule to the primary path by requiring an unwrapped primary or
detecting an existing ConnectionAdmissionProvider for primaryConnectionId before
wrapping, preserving the requested mode and preventing duplicate background
admission.
In `@scripts/regressions/provider-compat.regression.ts`:
- Around line 769-816: Add post-run release assertions to the two scenarios:
after `primaryRun` rejects, verify
`tryBackgroundConnection("primary-connection", ...)` succeeds, and after
`fallbackRun` resolves, verify the equivalent for `"fallback-connection"`. Reuse
Scenario 3’s `BACKGROUND_CONNECTION_IDLE_MS` time offset and release each
acquired admission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3699ee69-6318-4abc-9263-171a8072c91d
📒 Files selected for processing (14)
packages/server/src/routes/connections.routes.tspackages/server/src/routes/encounter.routes.tspackages/server/src/routes/generate/dry-run-route.tspackages/server/src/routes/lorebooks.routes.tspackages/server/src/services/capability-packages/capability-language-model.service.tspackages/server/src/services/generation/connection-admission.tspackages/server/src/services/image/image-generation.tspackages/server/src/services/llm/connection-fallback-provider.tspackages/server/src/services/llm/provider-registry.tspackages/server/src/services/memory-recall-embedding.tspackages/server/src/services/professor-mari/workspace-agent.service.tspackages/server/src/services/spotify/dj-mari-playlist.service.tspackages/server/src/services/turn-games/turn-game-bot-runner.service.tsscripts/regressions/provider-compat.regression.ts
|
@coderabbitai Thanks — worked through all four. Two were valid and are fixed in 72aa681; two I am skipping, with reasoning below. Fixed — Correct, and it undercut the fix it was part of. The Fixed — post-run release assertions in Added to both scenarios, reusing the Skipping — threading The fallback concern does not apply: the fallback path re-enters On keying generally, endpoint-over-connection-id is deliberate and documented at lines 279-284: most image callers have no connection row in scope, so keying on Skipping — competing admission-wrapper ownership I could not reproduce the premise on this branch. No call site both passes The background half is likewise not reachable: Worth noting the latent footgun is real if a future caller does both, though the failure is mild for foreground (symmetric double increment/decrement, so no leak — just background held off while active). A nested background wrap would throw Validation on 72aa681: |
|
Tip For best results, initiate chat on the files or code changes.
The two fixed items address the reported failure modes. Suppressing teardown rejection preserves the provider error for fallback classification. The added release assertions cover both failed and completed calls. I accept the two skipped items for this PR. Image generation re-enters The current provider call graph does not double-wrap an admission provider. The nested background-wrapper case is a future integration risk only. A guard can be added when production background callers or combined fallback/provider construction are introduced. The reported validation is sufficient for these findings. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/services/llm/connection-fallback-provider.ts (1)
169-172: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLog cleanup failures without masking the primary error.
At Line 172,
.catch(() => undefined)drops every cleanup error. A rejectedgeneration.return(undefined)can indicate incomplete provider or connection-admission cleanup. If cleanup fails, log the error withlogger.warnand continue without rethrowing. This preserves the primary error for fallback decisions and keeps the failure observable.Proposed fix
- await generation.return(undefined).catch(() => undefined); + try { + await generation.return(undefined); + } catch (cleanupError) { + logger.warn( + cleanupError, + "[%s-fallback] Failed to close the primary generation during cleanup", + this.category, + ); + }As per coding guidelines, server failures must use the shared Pino logger, use
logger.warnfor non-fatal issues, and pass the error object first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/services/llm/connection-fallback-provider.ts` around lines 169 - 172, Update the cleanup call in the fallback provider flow around generation.return to catch the rejection as an error, log it with the shared logger.warn passing the error object first, and then continue without rethrowing so the primary provider error remains available for fallback decisions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/server/src/services/llm/connection-fallback-provider.ts`:
- Around line 169-172: Update the cleanup call in the fallback provider flow
around generation.return to catch the rejection as an error, log it with the
shared logger.warn passing the error object first, and then continue without
rethrowing so the primary provider error remains available for fallback
decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d13dfef-02ba-4638-a113-7129eea81861
📒 Files selected for processing (2)
packages/server/src/services/llm/connection-fallback-provider.tsscripts/regressions/provider-compat.regression.ts
|
@coderabbitai Valid — fixed in 1e3ce0f. The previous await generation.return(undefined).catch((closeError: unknown) => {
logger.warn(closeError, "[%s-fallback] Failed to close the primary generation stream", this.category);
});Error object first and a Pino format specifier for
|
|
Tip For best results, initiate chat on the files or code changes.
The reported validation covers the updated behavior. 🐇 ✅ You are interacting with an AI system. |
|
@Xelvanis Thanks for the review — all three findings were real, and F1 in particular was a good catch. Fixed in b647ac8, and the branch is now merged up with staging as of 8cce9fe. F1 — RunPod admission conflating endpoints Confirmed exactly as described: Added On your account-wide uncertainty: I kept the repository contract you identified — admission is per physical endpoint. If RunPod rate limits turn out to be account-wide, that is a separate policy layer and should not be smuggled in through the admission key. F2 — wrapper replacing capability diagnostics Confirmed at Scoped deliberately to the admission decorator. F3 — dead admission surface Confirmed — the only references to Validation
Coverage for F1 is on the key derivation — same-base/different-id, trimming, and the bare-URL case — and it is mutation-checked: collapsing the key back to the base URL makes the lane exit 1. Your residual-risk note stands that nothing drives two live RunPod endpoints over HTTP end to end; that would need a two-endpoint fake server and I did not add one. Deferred item noted — background admission from Noodle owners and |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/services/image/image-generation.ts`:
- Around line 302-304: Update the background image-generation entry points,
including runImageGenerationRequest with queue false and illustrated background
variants, to pass admissionMode { kind: "background" } plus the appropriate
queue/cooldown hook into generateImage; ensure generateImage’s admission path
receives these values instead of defaulting to foreground admission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2b5aaa0c-9468-42b6-814b-2c8df84d4f15
📒 Files selected for processing (2)
packages/server/src/services/image/image-generation.tsscripts/regressions/provider-compat.regression.ts
Extracts connection admission accounting into a shared service so image generation, the provider registry, and the fallback provider all route through one place instead of each tracking slots independently. Split out as the base layer of a stack; the NoodleR work builds on it.
ConnectionFallbackProvider drains the primary with a manual loop rather than yield*, so a consumer that stopped reading mid-stream never propagated the early return to the primary generator. With an admission wrapper in place that left the connection's foreground slot held for the lifetime of the process, silently locking out all background generation on that connection. Covered by a provider-compat case that fails without the fix.
The finally that closes the primary generator awaited generation.return() directly, so a rejection from teardown replaced the provider error the catch below inspects to decide whether to fall back. Swallow it. Also assert both existing admission scenarios release their foreground slot, covering the failed-call and completed-call paths.
…ping them The cleanup catch discarded the rejection silently, so a generator that failed to close left no trace. Log it with the shared logger and continue, keeping the provider error available for the fallback decision.
…agnostics concrete RunPod connections share one API base URL and select the physical endpoint with a separate id, so keying admission on the base URL alone let two independent endpoints contend for one slot. Key on the endpoint instead. The admission decorator also became the class reported through the capability language-model host, so every configured provider surfaced as ConnectionAdmissionProvider in package diagnostics. Unwrap before reading the name. Drops withForegroundConnection and the none mode, which had no consumers.
imageAdmissionKey appended any imageEndpointId regardless of backend, so a stale id left on an imported, copied, or API-created ComfyUI/A1111 connection split one physical endpoint into separate admission slots — letting background work bypass foreground admission and its cooldown. Key on the resolved source so only runpod_comfyui, whose base URL does not identify the endpoint, carries the id.
5bc8524 to
f727e13
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/services/generation/connection-admission.ts`:
- Around line 9-16: Prune idle entries from the states map within the foreground
and background release paths. When a state has foregroundActive === 0,
backgroundActive === false, and lastForegroundFinishedAt is older than
BACKGROUND_CONNECTION_IDLE_MS, delete its corresponding key; preserve state
entries while either activity remains active or the idle window has not elapsed.
- Around line 161-171: Update BaseLLMProvider.chatComplete() to close the
AsyncGenerator returned by provider.chat() by calling await
gen.return(undefined) in a finally block around its manual gen.next() loop,
ensuring ConnectionAdmissionProvider.chat() executes finalizeConnectionAttempt.
Preserve the existing iteration and result handling.
- Around line 122-134: The completed-outcome path in finalizeConnectionAttempt
must not promote late finalizer errors into a stream failure after usable tokens
have been emitted. Adjust the coordination between
ConnectionAdmissionProvider.chat, finalizeConnectionAttempt, and
ConnectionFallbackProvider.chat so completed finalization errors are retained
for non-streaming calls but logged and suppressed for already-emitting streams,
preserving normal propagation before output is delivered.
In `@packages/server/src/services/llm/connection-fallback-provider.ts`:
- Around line 222-236: Prevent fallback retries from reusing the one-shot
ConnectionAdmissionMode.beforeAttempt hook. In
packages/server/src/services/llm/connection-fallback-provider.ts lines 222-236,
derive a separate fallback mode without beforeAttempt (or claim the hook once
and pass plain background mode) before calling withConnectionAdmissionProvider;
in packages/server/src/services/image/image-generation.ts lines 296-309,
override admissionMode in the recursive fallback request spread with a mode that
omits beforeAttempt, while preserving the existing fallback override.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b21fc3b6-5eea-468d-9ee6-03e3214af6a5
📒 Files selected for processing (14)
packages/server/src/routes/connections.routes.tspackages/server/src/routes/encounter.routes.tspackages/server/src/routes/generate/dry-run-route.tspackages/server/src/routes/lorebooks.routes.tspackages/server/src/services/capability-packages/capability-language-model.service.tspackages/server/src/services/generation/connection-admission.tspackages/server/src/services/image/image-generation.tspackages/server/src/services/llm/connection-fallback-provider.tspackages/server/src/services/llm/provider-registry.tspackages/server/src/services/memory-recall-embedding.tspackages/server/src/services/professor-mari/workspace-agent.service.tspackages/server/src/services/spotify/dj-mari-playlist.service.tspackages/server/src/services/turn-games/turn-game-bot-runner.service.tsscripts/regressions/provider-compat.regression.ts
| type ConnectionState = { | ||
| foregroundActive: number; | ||
| backgroundActive: boolean; | ||
| lastForegroundFinishedAt: number; | ||
| }; | ||
|
|
||
| const states = new Map<string, ConnectionState>(); | ||
| export const BACKGROUND_CONNECTION_IDLE_MS = 30_000; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
states grows without bound and is never pruned.
states keeps 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/services/generation/connection-admission.ts` around lines
9 - 16, Prune idle entries from the states map within the foreground and
background release paths. When a state has foregroundActive === 0,
backgroundActive === false, and lastForegroundFinishedAt is older than
BACKGROUND_CONNECTION_IDLE_MS, delete its corresponding key; preserve state
entries while either activity remains active or the idle window has not elapsed.
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 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.
ConnectionAdmissionProvider.chat marks the streaming call completed after yield* this.provider.chat(...) returns, then finalizeConnectionAttempt can later throw ConnectionAttemptFinalizationError. ConnectionFallbackProvider.chat includes that admission failure in its catch and rethrows it when emittedUsableOutput is true, so the consumer reports a failed generation for a token stream that already delivered content. Keep the throw for non-streaming paths only, or log and suppress it when tokens were already delivered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/services/generation/connection-admission.ts` around lines
122 - 134, The completed-outcome path in finalizeConnectionAttempt must not
promote late finalizer errors into a stream failure after usable tokens have
been emitted. Adjust the coordination between ConnectionAdmissionProvider.chat,
finalizeConnectionAttempt, and ConnectionFallbackProvider.chat so completed
finalization errors are retained for non-streaming calls but logged and
suppressed for already-emitting streams, preserving normal propagation before
output is delivered.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 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 provider.chat(...) generators when calling chatComplete.
BaseLLMProvider.chatComplete() calls gen.next() in a loop but does not call return() on the AsyncGenerator, so the ConnectionAdmissionProvider.chat() finally block never runs. Manual callers must close an await gen.next() loop with await gen.return(undefined) in finally; for await closes the generator automatically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/services/generation/connection-admission.ts` around lines
161 - 171, Update BaseLLMProvider.chatComplete() to close the AsyncGenerator
returned by provider.chat() by calling await gen.return(undefined) in a finally
block around its manual gen.next() loop, ensuring
ConnectionAdmissionProvider.chat() executes finalizeConnectionAttempt. Preserve
the existing iteration and result handling.
| const fallback = withConnectionAdmissionProvider( | ||
| createLLMProvider( | ||
| fallbackConnection.provider, | ||
| fallbackBaseUrl, | ||
| fallbackConnection.apiKey, | ||
| fallbackConnection.maxContext, | ||
| fallbackConnection.openrouterProvider, | ||
| fallbackConnection.maxTokensOverride, | ||
| isEnabled(fallbackConnection.claudeFastMode), | ||
| isEnabled(fallbackConnection.treatAsLocalEndpoint), | ||
| fallbackConnection.defaultParameters, | ||
| ), | ||
| fallbackConnection.id, | ||
| admissionMode, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
ConnectionAdmissionMode carries a one-shot beforeAttempt hook, but both fallback paths reuse the same mode object for the retry. In background mode beforeAttempt returns a finalizer for one attempt. Each fallback path passes the identical mode object to the retry, so a single logical generation invokes beforeAttempt twice and produces two finalizers: the primary attempt finalizes as "failed" and the fallback attempt finalizes as "completed". Attempt accounting then double-counts the generation.
packages/server/src/services/llm/connection-fallback-provider.ts#L222-L236: derive a separate mode for the fallback wrapper instead of reusing theadmissionModereference already applied toprimaryat Line 213. Either stripbeforeAttemptfor the fallback wrap, or make the hook claim once perwithConnectionFallbackProviderinvocation and hand the retry a plain{ kind: "background" }mode.packages/server/src/services/image/image-generation.ts#L296-L309: the recursive fallback call at Line 326 spreads...requestand carriesadmissionModethrough unchanged. OverrideadmissionModein that spread with a mode that has nobeforeAttempt, matching the wayfallback: undefinedis already overridden.
📍 Affects 2 files
packages/server/src/services/llm/connection-fallback-provider.ts#L222-L236(this comment)packages/server/src/services/image/image-generation.ts#L296-L309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/services/llm/connection-fallback-provider.ts` around
lines 222 - 236, Prevent fallback retries from reusing the one-shot
ConnectionAdmissionMode.beforeAttempt hook. In
packages/server/src/services/llm/connection-fallback-provider.ts lines 222-236,
derive a separate fallback mode without beforeAttempt (or claim the hook once
and pass plain background mode) before calling withConnectionAdmissionProvider;
in packages/server/src/services/image/image-generation.ts lines 296-309,
override admissionMode in the recursive fallback request spread with a mode that
omits beforeAttempt, while preserving the existing fallback override.
|
Closing in favour of #4514, which is the same work re-cut as a fresh branch off |
1 participant
Why
NoodleR posts and images generate in the background, on the same API connection the user is actively chatting on. Without a gate, a scheduled creator post fires mid-roleplay and the user queues behind their own bot, or burns rate limit they wanted for themselves.
This adds a shared admission service so background work yields to foreground work: it is admitted only when nothing is running in the foreground on that connection and the last foreground request finished at least 30s ago. Foreground work never asks permission.
Admission failures are typed distinctly (
isConnectionAdmissionFailure) so the fallback chain can tell "connection busy" apart from "provider failed" — otherwise a busy connection would look like an outage and trigger a pointless failover to the backup model.Relationship to #4503
Standalone against
staging— not a stack. This PR is the connection-admissionwork only, and it stands on its own.
The rest of what was originally split across
stack/2-docsandstack/3-servernow lives in #4503 (NoodleR Slice 8f), which is also based on
stagingand isreviewed independently.
#4503's branch does contain this PR's commits, because the Slice 8f code is
built on the admission service. Merge this PR first — once it lands, these
changes drop out of #4503's diff and what remains there is the Slice 8f work.
Notes
statesis a module-level map keyed per connection id and per image base URL, and is never pruned. Bounded by configured connections, so not a leak that matters in practice, but it is an unmarked deliberate call.Test plan
pnpm checkpnpm regression:providersSummary by CodeRabbit
New Features
Bug Fixes
Tests