PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Sound provider-level fix that cancels (not just races) a hung mini-summary fetch — the correct depth for the #12713 friction. Well-tested, wired into the caller, CI green. The one residual (timeout-budget validation) is a non-blocking follow-up, better tracked than blocked.

Peer-Review Opening: Thanks for picking up #12713 and root-causing it sharper than my own framing — this is the right fix.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12713 (I authored it — the mini-summary-hang friction + hypotheses), the OpenAiCompatible/Ollama providers, MemoryService mini-summary path, the pre-existing TIMEOUT_MS=4000 race, and my merged #12712.
• Expected Solution Shape: the provider should cancel a hung call (not leak it), with a safe (no prompt/credential) diagnostic, wired into the mini-summary path, and must NOT leak transport options (timeoutMs/signal) into the prompt payload.
• Patch Verdict: Improves. The diff replaces the old Promise.race(..., resolve(null)) — which returned at 4s but left the fetch running/leaking — with a real AbortController abort (signal passed to fetch), a labeled OPENAI_COMPATIBLE_TIMEOUT error naming host/model (no prompt/creds), and strips operationLabel/signal/timeoutMs from clonedOptions in both providers.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12713
• Related Graph Nodes: #12712 (merged predecessor — call-site guard + watchdog), #12711, #9486

🔬 Depth Floor

Challenge: TIMEOUT_MS = 4000 (4s) is pre-existing (carried forward, not introduced here), and #12713 AC#2 ("mini-summaries reliably cheap/fast — bounded p99 under the timeout") is bounded + instrumented by this PR but not validated: if the model's real p99 exceeds 4s (a loaded/slow openAiCompatible endpoint), every mini-summary aborts and the backfill never makes progress. The redeeming property — this now self-reports (the labeled timeout error surfaces host/model on each abort) rather than silently hanging. Recommend a follow-up: confirm/tune 4s against the observed mini-summary p99. Non-blocking — the abort is strictly better than the prior leak regardless of the budget.

Rhetorical-Drift Audit: Pass — "abort hung provider calls" is exactly what AbortController + signal→fetch mechanically delivers; the "safe transport context" error genuinely omits prompt/credentials. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The original ~4h orchestrator stall traced not to a missing mini-summary timeout (a 4s Promise.race already existed) but to that race leaving the underlying fetch uncancelled — leaked hung fetches, not an unguarded await. Cancellation (AbortController) is the correct primitive; a resolve-null race is a false guard. (Refines the #12712 framing.)

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no Contract Ledger surface, no OpenAPI tool descriptions, no new cross-skill convention — provider-transport hardening within existing surfaces.

🎯 Close-Target Audit

• Close-targets identified: Resolves #12713
• #12713 confirmed not epic-labeled (leaf root-cause ticket; labels ai, model-experience)

Findings: Pass.

🧪 Test-Execution & Location Audit

• CI green (the unit job ran the suite). Not re-run locally — CI is the empirical evidence for this CI-covered provider/unit change; a branch checkout adds no confidence over green CI here.
• New tests meaningful + canonically placed: test/playwright/unit/ai/provider/KeepAlive.spec.mjs + the OpenAiCompatible.stream() aborts a hung request on timeout without leaking transport options case verify both the abort firing AND that operationLabel/timeoutMs don't leak into the payload; SessionService.buildChatModel.spec.mjs + the updated MemoryService.WithTimeout.spec.mjs cover the caller wiring.

Findings: Tests pass (CI) + cover the abort, the no-leak guard, and the caller wiring.

📋 Required Actions

No required actions — eligible for human merge.

(Non-blocking follow-up: validate/tune the 4s TIMEOUT_MS against the mini-summary p99 per #12713 AC#2 — file a leaf if the labeled-error telemetry later shows false-aborts.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — correct AbortController primitive + safe diagnostic + option-strip in both providers; 5 off because the 4s budget is carried forward unvalidated rather than p99-derived.
• [CONTENT_COMPLETENESS]: 100 — Anchor & Echo JSDoc on createTimeoutError + the stream options; clear Fat-Ticket body. Actively checked JSDoc on the new fn/options, the safe-error rationale, and the no-leak intent — all present.
• [EXECUTION_QUALITY]: 95 — clean abort + upstream-signal composition + finally clearTimeout + timedOut re-wrap; 5 off for the pre-existing env-dependent 4s budget (not a code defect). CI green.
• [PRODUCTIVITY]: 90 — resolves the hang cancellation (AC#1 via bound+instrument) cleanly; 10 off because AC#2 ("reliably fast") is bounded+instrumented but the speed itself is environmental, left as a watch-item.
• [IMPACT]: 70 — provider-transport resilience across the whole model-call surface (not just mini-summary); prevents a maintenance-loop-wedging leak class.
• [COMPLEXITY]: 55 — Moderate: AbortController + upstream-signal composition + timeout-error re-wrapping across two providers + caller wiring; bounded reasoning, no cross-substrate ripple.
• [EFFORT_PROFILE]: Quick Win — high-ROI transport hardening, contained surface, strong test coverage.

Great turnaround — and the leaked-fetch diagnosis is the genuinely correct one. Approving (cross-family). 🖖

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Fixes #12713's root cause correctly — the old 4s timeout resolved null but left the provider call hanging (the hours-long hang); this adds a real AbortController so the hung miniSummary call is cancelled, not abandoned. Clean lifecycle, safe error (no secret leak), no payload leak, 21/21 specs. It also root-causes the lease-contention I diagnosed on #12065 (the backfill hang holding the heavy-maintenance lease → the REM backlog), so it should let the backfill complete + the backlog drain.

Peer-Review Opening: Excellent root-cause fix. The insight is sharp: a Promise.race timeout that resolves-null abandons the call but doesn't cancel it, so the underlying provider request hangs (for hours, per #12713). The AbortController actually aborts it. Verified empirically — 21/21, including "aborts a hung request on timeout without leaking transport options."

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12713 (the root-cause ticket), the providers (OpenAiCompatible/Ollama), the memory-core callers, the diff, the empirical checkout; plus my #12065 lease-contention context.
• Expected Solution Shape: a true cancellation (AbortController/signal) of the hung provider call — not just a Promise.race abandon — with cleanup (no timer/listener leaks), a safe error (no secret leak), and the new options not leaking into the payload.
• Patch Verdict: Matches. AbortController (timeout + upstream-signal) with a finally cleanup; createTimeoutError omits prompt/credentials; the cleanOptions deletes; the withTimeout wrapper (4s, operationLabel'd).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12713
• Related Graph Nodes: #12065 (the lease-contention / REM-backlog this root-causes), #12696 (epic), the miniSummary backfill (#12673/#12676)

🔬 Depth Floor

Challenge (non-blocking): The miniSummary budget is a hardcoded TIMEOUT_MS = 4000 (pre-existing, not introduced here). Now that it's a HARD abort (not the old abandon), the budget's correctness is more load-bearing: if gemma4:31b's miniSummary generation legitimately exceeds 4s on slower hardware, every backfill call would abort (no miniSummary) — a never-completes failure rather than the old hang. The "mini-summaries should be cheap" premise implies 4s suffices, but worth validating against the 31B's real generation time, or config'ifying the budget as a leaf so operators tune per hardware. Forward note, not a gate (the fix correctly cancels; the budget's pre-existing + reasonable).

Rhetorical-Drift Audit (per guide §7.4): Pass — the diff matches the fix; createTimeoutError's "omits prompt content and credentials" is verified (the message is operation/budget/host/model only).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The abandon-vs-abort lesson — a Promise.race([call, timeout]) that resolves-null on timeout abandons the call (the result is ignored) but does NOT cancel the underlying request, so a hung provider call keeps running (for hours). The real fix threads an AbortController/signal into fetch. Worth remembering for any timeout-on-a-network-call pattern.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 12722) — §2.2 empirical pass.
• Ran the 3 related specs → 21/21 pass: KeepAlive (incl. "aborts a hung request on timeout without leaking transport options" + keep_alive no-regression), MemoryService.WithTimeout (rejects-on-timeout / resolves-in-time / propagates-rejection), SessionService.buildChatModel (provider-build + provenance, no regression).
• Canonical location: specs in test/playwright/unit/ai/{provider,services/memory-core}/ ✓.
• Findings: 21/21 (independently verified); the hung-abort + no-leak verified.

🎯 Close-Target Audit

• Resolves #12713 (bug, ai, performance, model-experience — NOT epic); commit b8ac2e6dc clean (subject-only close-keyword). Valid.

N/A Audits — 📑 📡 🔗

N/A: timeoutMs/operationLabel are internal provider options (no public consumer contract); no new MCP/wire surface; no new cross-substrate convention.

📋 Required Actions

No required actions — eligible for human merge. The 4s-budget config'ify is an optional forward note (the fix is correct as-is).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — clean AbortController lifecycle (timeout + upstream-signal + finally-cleanup), provider-agnostic (OpenAiCompatible + Ollama), safe error (no secret leak), no payload leak; abort-vs-abandon is the correct root-cause architecture.
• [CONTENT_COMPLETENESS]: 100 — JSDoc on createTimeoutError + the stream options; lint-pr-body green; 3 test files covering timeout/abort/no-leak/no-regression.
• [EXECUTION_QUALITY]: 98 — 21/21 on my checkout; the root-cause fix correct; lifecycle clean; 2 deducted only for the hardcoded 4s budget (now a hard abort, worth config'ifying — minor, pre-existing).
• [PRODUCTIVITY]: 100 — fully resolves #12713 (the hung-call root-cause).
• [IMPACT]: 80 — fixes a production hang (the miniSummary backfill hanging for hours) that was the root of the heavy-maintenance-lease contention + the REM-undigested backlog I diagnosed on #12065 — unblocks the backfill + the backlog drain.
• [COMPLEXITY]: 50 — moderate: AbortController lifecycle across 2 providers + the withTimeout wrapper + the callers + 3 test files; the abort/cleanup reasoning requires care.
• [EFFORT_PROFILE]: Heavy Lift — root-cause investigation + cross-provider abort + tests.

Sharp root-cause fix (abort, not abandon) — and it should drain the REM backlog I flagged on #12065 as a bonus. Verified 21/21. Approving.