Skip to content

fix(vision): bounded wait for in-flight VDS description on first message (#970 stage 1) #973

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.

Sign up for GitHub

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

Open
joelteply wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(vision): bounded wait for in-flight VDS description on first message (#970 stage 1) #973

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/system/user/server/modules/PersonaResponseGenerator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,16 +373,33 @@ export class PersonaResponseGenerator {
if (!base64) {
return null; // Nothing to send to the model
}
// Pull cached description (populated by prewarmVisionDescriptions
// at chat-send time). Cache hit takes ~0ms; miss returns
// undefined — text-only personas downstream get a "no
// description available" marker instead of fabricating.
// Pull description from VDS — populated by prewarmVisionDescriptions
// at chat-send time. Two states are valid waits:
// 'cached' → ~0ms instant lookup (pre-warm finished).
// 'inflight' → bounded wait. Pre-warm started but hasn't
// resolved yet; we'd rather wait up to 8s than
// hand the persona an empty description and
// let it hallucinate "I don't see any image."
// VDS already deduplicates inflight requests, so
// this await piggybacks on the existing call —
// no extra inference cost.
// Status `none` / `error` → don't trigger a blocking describe
// here; the chat-send path is responsible for prewarming. Stage
// 2 (Rust-side) is responsible for emitting an [Attached image:
Comment on lines +386 to +388
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment mentions status error, but VisionDescriptionService.descriptionStatus() currently only returns 'cached' | 'inflight' | 'none' (no error state). Please update the comment to match the actual status values so future readers don’t assume there’s an error state to handle here.

Suggested change
// Status `none` / `error` → don't trigger a blocking describe
// here; the chat-send path is responsible for prewarming. Stage
// 2 (Rust-side) is responsible for emitting an [Attached image:
// Status `none` → don't trigger a blocking describe here; the
// chat-send path is responsible for prewarming. Stage 2
// (Rust-side) is responsible for emitting an [Attached image:

Copilot uses AI. Check for mistakes.
// unavailable] marker when description ends up undefined, so a
// text-only persona at least KNOWS an image was attached
// instead of fabricating absence. Tracked in #970.
let description: string | undefined;
if (m.type === 'image') {
try {
const visionSvc = VisionDescriptionService.getInstance();
if (visionSvc.descriptionStatus(base64) === 'cached') {
const desc = await visionSvc.describeBase64(base64, m.mimeType ?? 'image/png', { maxLength: 200 });
const status = visionSvc.descriptionStatus(base64);
if (status === 'cached' || status === 'inflight') {
const VDS_WAIT_MS = 8000;
const desc = await Promise.race([
visionSvc.describeBase64(base64, m.mimeType ?? 'image/png', { maxLength: 200 }),
new Promise<null>((resolve) => setTimeout(() => resolve(null), VDS_WAIT_MS)),
]);
description = desc?.description;
Comment on lines +397 to 403
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promise.race([... setTimeout(...)]) creates a timer that is never cleared/unref’d; even when the description resolves immediately (especially for status === 'cached'), the pending 8s timer still keeps the event loop alive and can accumulate under load. Consider: (1) only using the timeout path for 'inflight' (skip creating a timer for 'cached'), and (2) clearing the timeout (or using unref() where available) once the describe promise resolves first.

Suggested change
if (status === 'cached' || status === 'inflight') {
const VDS_WAIT_MS = 8000;
const desc = await Promise.race([
visionSvc.describeBase64(base64, m.mimeType ?? 'image/png', { maxLength: 200 }),
new Promise<null>((resolve) => setTimeout(() => resolve(null), VDS_WAIT_MS)),
]);
description = desc?.description;
if (status === 'cached') {
const desc = await visionSvc.describeBase64(base64, m.mimeType ?? 'image/png', { maxLength: 200 });
description = desc?.description;
} else if (status === 'inflight') {
const VDS_WAIT_MS = 8000;
let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
try {
const timeoutPromise = new Promise<null>((resolve) => {
timeoutHandle = setTimeout(() => resolve(null), VDS_WAIT_MS);
if (typeof timeoutHandle.unref === 'function') {
timeoutHandle.unref();
}
});
const desc = await Promise.race([
visionSvc.describeBase64(base64, m.mimeType ?? 'image/png', { maxLength: 200 }),
timeoutPromise,
]);
description = desc?.description;
} finally {
if (timeoutHandle) {
clearTimeout(timeoutHandle);
}
}

Copilot uses AI. Check for mistakes.
}
Comment on lines 395 to 404
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gating on descriptionStatus(base64) risks missing fast cache hits: descriptionStatus() only reflects the TS L1 + in-flight map, and does not consult Rust L1.5 or the on-disk sidecar cache that describeBase64() can return from quickly. This means a TS-cold start (or a race where prewarm hasn’t registered inflight yet) can still produce status === 'none' even when describeBase64() would return immediately without inference. Consider adding a cached-only accessor that checks Rust/sidecar (no inference), or broadening this path to attempt a bounded describeBase64() when status is none but a cache hit is likely.

Copilot uses AI. Check for mistakes.
} catch {
Expand Down
Loading