Skip to content

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

Open
joelteply wants to merge 1 commit intomainfrom
fix/vds-bounded-wait-stage1
Open

fix(vision): bounded wait for in-flight VDS description on first message (#970 stage 1)#973
joelteply wants to merge 1 commit intomainfrom
fix/vds-bounded-wait-stage1

Conversation

@joelteply
Copy link
Copy Markdown
Contributor

Summary

Background — empirical hit (Anvil, PR #950 / 2026-04-25)

CodeReview AI #e50f39 HALLUCINATED "absence of images or attachments" when image IS attached. VDS [Attached image: ] marker missing on user-role msg (your separate bug). non-vision personas can't see + no fail-loud marker either.

Root cause (TS-side)

PersonaResponseGenerator.ts:380-391 only attached the VDS description when descriptionStatus(base64) === 'cached'. On the first message with a fresh image, pre-warm started at chat-send time is still 'inflight' when the persona reaches this code — so description stays undefined, the Rust IPC signal carries { ..., description: undefined }, and the text-only persona's user-role message has no image marker.

Fix

Wait when status is 'cached' OR 'inflight':

if (m.type === 'image') {
  try {
    const visionSvc = VisionDescriptionService.getInstance();
    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;
    }
  } catch { /* ... */ }
}

8s ceiling protects against a stuck pre-warm. LLaVA on CPU at 60-70s would still time out and leave description undefined — that's where Stage 2 (Rust marker injection) takes over.

Test plan

  • npm run build:ts — clean.
  • Empirical: send an image to a text-only persona on the first message after a fresh start. Persona MUST receive a valid description in its user-role context.
  • Cache-hit path unchanged: subsequent messages with the same image still take ~0ms (VDS cache returns immediately).

Refs

…age (#970 stage 1)

PersonaResponseGenerator only attached the VDS description to the IPC
signal when descriptionStatus(base64) === 'cached'. On the first message
with a fresh image, pre-warm started at chat-send time is still 'inflight'
when the persona reaches this code path — so description was undefined,
the Rust signal carried { ..., description: undefined }, and text-only
personas had no marker to read. Empirical hit on PR #950: CodeReview AI
confidently said "absence of images or attachments" when an image WAS
attached.

Fix: also wait when status is 'inflight'. VDS already deduplicates
in-flight requests, so the await piggybacks on the existing pre-warm —
no extra inference cost. 8s ceiling protects against a stuck pre-warm
(LLaVA on CPU is the slow case at 60-70s; the timeout is the safety
valve, not the expected path).

Stage 2 (Rust-side marker injection for the description=None case)
still tracked in #970 — when description IS undefined despite the
bounded wait, the Rust signal-to-ContentPart conversion should emit
"[Attached image: vision description unavailable]" instead of silently
dropping the image, so the persona knows an image exists rather than
fabricating absence. That's a Rust change, separate scope.

Validated locally: npm run build:ts → clean.

Refs #970.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 25, 2026 18:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts PersonaResponseGenerator to avoid dropping image descriptions on the first message after a fresh start by waiting (bounded) for an in-flight VisionDescriptionService (VDS) description instead of only accepting already-cached results.

Changes:

  • Wait up to 8s for VDS when image description status is 'inflight' (in addition to 'cached').
  • Use a Promise.race timeout to bound the wait and keep description fetch best-effort.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +386 to +388
// 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:
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.
Comment on lines +397 to 403
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;
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
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;
}
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants