PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The smallest-substrate QoS shape the ticket asked for — an in-service interactive-first request queue — implemented correctly, with all five #12509 ACs met (including the body-content ones: the 3-shape comparison and the V1/V2 distinction), a genuine contention test that proves the priority jump + single-active invariant, and an explicitly-declared "no config leaf" delta. One non-blocking observation about the unconditional single-active cap; not a gate.

Peer-Review Opening: This is the strongest PR in the current AiConfig/memory-core batch. The Design Comparison section actually does the V-B-A AC1 requires (not a rubber-stamp "I picked a queue"), the scheduler logic is clean, and the test is well-built — the 25ms delay on the first batch chunk creates exactly the in-flight window an interactive request needs to prove it jumps ahead. Approving with one design-scope note below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12509 (full body + Contract Ledger + 5 ACs; labels enhancement/ai/architecture/performance/model-experience, no epic → leaf), parent #12487 + V1 PR #12502 lineage, the diff, and the existing embedTexts/embedText dispatch.
• Expected Solution Shape: AC1 mandates comparing three QoS shapes (batch-yield / priority-lane / orchestrator-pause) and selecting the smallest substrate that proves interactive-first without turning the service into a broad scheduler; AC2/AC3 require interactive-completes + batch-ordering-preserved; AC4 mocked-provider coverage; AC5 a body-level V1/V2 distinction.
• Patch Verdict: Matches all five:
  • AC1 — "## Design Comparison" evaluates all three: batch-yield (kept + made the pause useful), priority-lane (selected, "smallest substrate… without an orchestrator scheduler"), orchestrator-pause (rejected as crossing #12508/#12533 scope). Selection rationale matches the ticket's "smallest substrate" guidance, and the in-service queue correctly avoids the Decision-Record trigger (no shared cross-service scheduling primitive).
  • AC2/AC3 — the qos-priority test asserts interactive 'urgent' completes between batch chunks and batchResult preserves [a,b,c] ordering + eventual completion.
  • AC4 — Evidence line declares L2 (mocked contention), no residuals; live probe declared as Post-Merge Validation.
  • AC5 — explicit: "V1 retried after contention/timeouts, while this PR schedules request order before provider POSTs."
  • Ledger QoS-control-surface row — "## Deltas from ticket — No new config leaf" explicitly declares the no-toggle decision + rationale (existing batchEmbeddingChunkSize/batchEmbeddingYieldMs remain the operator controls). That's the correct way to dispose of a ledger row, not a silent skip.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12509
• Related Graph Nodes: Parent #12487 (interactive-vs-batch contention), V1 PR #12502, siblings #12508/#12533 (canary/health, deliberately out of scope)

🔬 Depth Floor — Scheduler correctness (traced)

I traced the three private methods against the async dispatch, not just the test green:

• Single-active invariant: #drainOpenAiCompatiblePostQueue early-returns if #openAiCompatiblePostQueueActive, so exactly one drain loop runs → exactly one #postOpenAiCompatible in flight. Confirmed by maxInFlightRequests === 1.
• Priority selection: #getNextOpenAiCompatiblePostQueueIndex starts at the FIFO head and promotes to the first interactive item only while the current best is batch — so it yields "first interactive if any, else FIFO head," and once best becomes interactive it stops promoting. That's interactive-first with FIFO within each lane, matching the JSDoc. Traced [batch0, interactive1, batch2] → interactive1, batch0, batch2 (batch stays FIFO).
• No stall on failure: per-task try/catch rejects the individual task and the while continues; the outer finally resets the active flag even on an unexpected throw. No queue-wedge path.
• Why the test genuinely proves the jump: chunks enqueue one-at-a-time (embedTexts awaits each #enqueueOpenAiCompatiblePost), so the queue is never pre-loaded; the 25ms 'a' delay is what lets 'urgent' enter while [a] is in-flight, and interactive-preference (or simply being the only queued item at that instant) sends it ahead of [b].

Observation (non-blocking — design-scope, recommend a one-line JSDoc): the queue makes provider POSTs unconditionally single-active for every openAiCompatible endpoint, changing concurrency from previously-unbounded to 1. For the documented use case — local serializing servers (LM Studio / llama.cpp, the #12487 lineage) — this is correct and strictly better (it gives the client priority authority the server's blind FIFO can't). But pointed at a concurrency-capable openAiCompatible endpoint, it would silently cap throughput at 1. The JSDoc already frames it as "competing local-provider concurrency," so intent is clear; I'd just add one line stating the single-active serialization is unconditional (not provider-probed) so a future operator at a concurrent endpoint isn't surprised. Not a blocker — it's faithful to the ticket's "smallest substrate" mandate, and provider-capability probing would be exactly the scope creep #12509 says to avoid.

Rhetorical-Drift Audit: Pass. "Prioritize interactive embeddings" matches the diff — a priority queue + the two call-site lane tags, nothing broader.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: This is the reference pattern for resolving a design-and-implement ticket: the PR body carries the V-B-A comparison (AC1) and the V1/V2 distinction (AC5) as first-class sections, so the close is auditable from the body alone — exactly what prevents a "Resolves #N" over-claim on a process-AC ticket.
• [KB_GAP]: None — the in-service priority lane is the documented smallest-substrate answer to add_memory-vs-KB-sync contention.

N/A Audits — 📡 🔗 📑

N/A: no OpenAPI/skill/MCP-tool surface; the contract surface is the Contract Ledger, audited above (the QoS-control-surface row is explicitly disposed of via the "no config leaf" delta).

🧪 Test-Execution & Location Audit

• Checked out exact head 6a21f7ff6 and ran TextEmbeddingService.retry.spec.mjs → 14 passed (2.5s), including interactive single embeddings queue ahead of subsequent batch chunks and preserve completion (:368). CI all-green + MERGEABLE. The spec lives beside the existing retry coverage and extends the same serial describe — correct location + no parallel-worker contention on the shared http mock.

Findings: Tests pass; coverage proves all three runtime ACs (single-active, interactive-priority, batch-ordering+completion) rather than just exercising the happy path.

📋 Required Actions

No required actions — eligible for human merge.

Recommended (non-blocking): one JSDoc line on #enqueueOpenAiCompatiblePost/#drainOpenAiCompatiblePostQueue noting the single-active serialization is unconditional across all openAiCompatible endpoints (intended for serializing local servers).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — smallest-substrate in-service queue, correctly avoids the orchestrator-scheduler / shared-primitive (Decision-Record) trigger the ticket warned against; I verified the selection against all three candidate shapes.
• [CONTENT_COMPLETENESS]: 95 — 5 deducted: the body carries the comparison, V1/V2 distinction, and the no-config-leaf delta; the only gap is the in-code JSDoc not stating the single-active cap is unconditional (the design observation above).
• [EXECUTION_QUALITY]: 100 — scheduler traced correct across single-active/priority/failure paths; the test is genuinely discriminating (the 25ms window proves the jump, not just that it completes).
• [PRODUCTIVITY]: 100 — resolves all five #12509 ACs in one tightly-scoped service change (+147/-5), no scope bleed into #12508/#12533.
• [IMPACT]: 70 — proves an operator-level invariant (interactive add_memory no longer starves behind KB-sync batches) on a critical per-turn write path; above routine because it converts V1's best-effort retry into actual scheduling authority.
• [COMPLEXITY]: 55 — descriptive: moderate; concurrency/scheduling correctness with subtle microtask-ordering reasoning to confirm the priority jump, even though the surface is one service + one spec.
• [EFFORT_PROFILE]: Core Feature — the V2 follow-up #12502 deferred, delivered at the intended minimal scope.

Approving — clean, well-tested, all ACs met. The single-active-cap note is a one-liner for the next reader, not a gate.