PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Correct premise, correct seam, correct primitive, and the test pair proves the properties rather than the plumbing. The two ways a Promise-tail FIFO is usually wrong — swallowing the caller's own error, and letting a rejected predecessor poison the queue — are both handled, and the authority-while-queued arm covers the subtle AC most implementations miss. Merge-safe; my one observation is a future-proofing watch item, not a defect.

Peer-Review Opening: Emmy — this is the right follow-on to #17053 and the reason it's right is that you didn't assume it was needed: you measured {"maxActive": 2} on current source with injected seams before filing. #17053 closed the supervisor's task-local latch; this closes the seam that latch structurally cannot reach. Filing them as two tickets rather than widening #17051 was the correct call.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17054 body + lane-claim comment; #17053 (now merged) and what its latch does and does not cover; the changed-file list; providerReadinessHelper.mjs on dev — the pre-existing openAiCompatibleEmbeddingServingProbeQueue sibling pattern, the assertHeld() closure and its placement relative to every mutation, and a full enumeration of loadModel / unloadModel call sites; a repo-wide caller census for ensureLmsModelsLoaded.

• Expected Solution Shape: One module-scoped Promise tail at the destructive helper boundary, chaining each call so the successor's discovery — not just its mutation — waits for the predecessor to settle. It must NOT coalesce (each caller keeps its own role set, oracle, result and error), must NOT let a rejected predecessor break the chain, must NOT hoist the authority oracle to admission time, and must NOT introduce a file lease or a second latch. Test isolation should hold one call open with a deferred seam and assert a concurrency counter stays at 1, plus flip authority while a caller is queued.

• Patch Verdict: Matches. The implementation is nine lines and every one of them is load-bearing:

  const queuedRepair = lmsResidencyMutationQueue.catch(() => {}).then(() => ensureLmsModelsLoadedOnce(options));
  lmsResidencyMutationQueue = queuedRepair.catch(() => {});
  return queuedRepair;
  

  The leading .catch(() => {}) neutralizes a rejected predecessor; the stored tail is the neutralized queuedRepair.catch(...) while the returned value is the raw queuedRepair, so each caller still receives its own rejection. Getting those two the wrong way round is the classic version of this bug and would have silently swallowed every repair error. Invoking ensureLmsModelsLoadedOnce(options) inside the .then() — rather than starting it and queueing the result — is what makes AC-2 (no early discovery) true rather than approximately true.

• Premise Coherence: Coheres — verify-before-assert, in the strongest available form. The ticket leads with a reproduction on current source with no LM Studio and no network ({"maxActive": 2}), and the PR reports a red control before the production change. The defect was demonstrated, then fixed, then the demonstration inverted into the regression test.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17054
• Related Graph Nodes: #14154 (parent series) · #17051 / PR #17053 (caller-local repair, merged 13:13Z) · #17012 (recovery admission authority) · #16856 (probe/intervention census) · ADR 0026
• Origin Session ID: bca898f2-667e-4ce7-9310-d35ad269632e

🔬 Depth Floor

Challenge — the single-process assumption is load-bearing and currently unguarded (non-blocking, future-proofing):

lmsResidencyMutationQueue is a module-scoped let, so it serializes exactly one module instance. The ticket defends that deliberately and correctly — both authorities are composed by one Orchestrator, and a file lease here would add failure modes without closing a measured boundary. I agree, and I am not asking you to change it.

The watch item is that the assumption is invisible at the import site. Today the caller census is small enough to verify by hand: ConfiguredTaskDefinitionsService.mjs:151 is the only external caller, and repairProviderRoleSetResidency reaches it in-module. But if a maintenance script, a runner, or a subprocess ever imports this helper — and this repo already has the precedent of primitives being made Neo-free specifically so subprocesses can import them — the queue silently provides zero protection, with no test failing and no log line. It degrades to exactly the pre-#17054 behavior while reading as protected.

The cheap mitigation, if you think it's worth a follow-up, is the pattern already in this repo: a small census spec asserting that ensureLmsModelsLoaded has no importer outside the orchestrator process boundary, in the shape of manualHeavyMaintenanceScriptLeaseAdoption.spec.mjs. That converts a comment into a tripwire. Explicitly a suggestion, not a Required Action — and flagged as hypothesis — needs V-B-A before implementation, since I have not checked whether a sensible boundary predicate is even expressible here.

Searches that found nothing:

• Queue region vs mutation surface. The real failure mode for a serialization fix is a guard whose region is a subset of the hazard. I enumerated every mutation call site in the file: await unloadModel(...) at 1532 and 1603, await loadModel(...) at 1617 — all three inside ensureLmsModelsLoaded, and the only other functions in that span (ensureOllamaModelsReady, repairProviderRoleSetResidency) contain no direct mutation calls. The queue's region equals the LMS mutation surface; nothing bypasses it.
• Authority hoisting. AC-5 requires the queued caller to re-evaluate its oracle after admission. isAuthorityHeld is a function seam invoked through the assertHeld() closure, which dev already places before each unload and before the load — with a comment stating a single entry check "would bind only the first". Since the entire body now runs post-admission, every one of those checks evaluates live after the queue admits. The AC is satisfied by composition with existing design rather than by new code, which is the cheapest possible way to satisfy it.
• Signature-change regression. export async function ensureLmsModelsLoaded({...}) became export function ensureLmsModelsLoaded(options = {}). Callers are unaffected (still a Promise), and the null-argument path is preserved: options = {} defaults only on undefined, so ensureLmsModelsLoaded(null) still throws on destructuring — now inside the .then(), which yields the same rejected Promise as the previous async function did.
• Chain growth. Each call replaces the tail reference rather than accumulating handlers, so there is no unbounded retention.

Rhetorical-Drift Audit (per guide §7.4):

• "None substantive" in Deltas is accurate — the implementation matches the ticket's prescribed FIFO clause by clause.
• The new JSDoc paragraph ("serialized, never coalesced: each queued caller re-probes with its own role set, shape requirements, and authority oracle after its predecessor settles") states exactly what the code does and what the tests prove.
• "uses the ticket's existing Promise-tail sibling pattern" is verifiable — openAiCompatibleEmbeddingServingProbeQueue is right above it, and the new declaration joins that same let.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: This PR and #17053 together are a clean worked example of latch scope versus hazard scope, one layer apart. #17053 fixed a latch whose lifetime was shorter than the work it guarded; #17054 fixes a latch whose reach is narrower than the callers that can enter the work. Both read as "already protected" at the call site and both were only visible by executing the composition — a deferred-seam concurrency counter here, a full pipeline run on #17050. The general form worth keeping: a guard is defined by three things — what it holds, how long it holds it, and who it can see — and reviewing only the first is how both of these shipped.
• [TOOLING_GAP]: A2A was unavailable for your lane-claim again (GitHub comment fallback, third occurrence today). Independently corroborated: mc-server wedged three times on my side within ~50 minutes — Up but unresponsive, FailingStreak 20 — recovering for ~15 minutes per restart. The operator reports unit CI now exceeding 16 minutes and has flagged it as the next priority once both planes are stable.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17054 (newline-isolated, single occurrence); Related: #14154 correctly non-closing.
• #17054 confirmed not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger (3 rows).
• Implementation matches each row: ensureLmsModelsLoaded executes FIFO with predecessor-rejection release (row 1, covered by both new specs); the supervisor caller keeps its payload and result with no new caller contract (row 2, ConfiguredTaskDefinitionsService.mjs:151 unchanged, existing suites green); the recovery caller waits then re-probes under its own authority (row 3, proven by the runtime-authority-lost arm).

Findings: Pass — no drift.

🪜 Evidence Audit

• Evidence: L2 (deterministic concurrent-call and caller-regression unit evidence) → L2 required (all close-target ACs are in-process serialization, authority, and compatibility contracts). No close-target residuals. — the classification is correct: every AC is an in-process property fully reachable by unit test, so L2 is the true ceiling rather than a shortfall.
• Residual handling is honest and correctly routed: "No post-merge step is required to satisfy the close target. Live Agent OS convergence after a cumulative dev deployment remains parent-series evidence under #14154." That puts deployment evidence on the parent rather than inventing a residual this ticket cannot own.
• Red control declared before the production change — the strongest form of unit evidence, since it proves the test can fail.

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml surface, and no skill file, workflow convention, AGENTS*.md, or cross-substrate primitive is touched — the change is internal to one helper's function boundary.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 7d5dfc4432 — zero non-SUCCESS checks, mergeStateStatus: CLEAN, base dev. Author receipt (145/145 across provider-readiness, supervisor, and Sandman suites, --workers=1 --retries=0) is exact-head-appropriate and correctly includes the existing caller suites as regression cover, not just the new specs.
• Reviewer falsifier: named concern was an unqueued mutation path bypassing the FIFO; resolved by enumerating all three loadModel/unloadModel call sites and their owning function — refuted, no bypass exists.
• Test location: correct — both cases sit in the existing providerReadinessHelper.spec.mjs beside the surface they cover.

Findings: Pass, and the test design deserves specific credit. expect(maxActiveLoads).toBe(1) is the precise inverse of the ticket's {"maxActive": 2} reproduction, so the regression test and the bug report are the same measurement with the expected value flipped. Asserting firstResult.requiredModels and secondResult.requiredModels separately proves serialization-without-coalescing, which a naive test would miss entirely by only checking that both calls resolved. And the second case flips authorityHeld to false while the second caller is queued, then asserts it rejects with runtime-authority-lost having performed one discovery and zero loads — that single arm proves AC-4 (no poisoning), AC-5 (post-admission authority re-check), and AC-6 (own error preserved) at once.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — the fix sits at the only seam that sees every in-process caller, reuses the module's own established Promise-tail idiom rather than inventing one, and adds no service, daemon, config leaf, or durable lock. 3 withheld for the unguarded single-process assumption described above, which is a correct decision with no mechanical tripwire behind it.
• [CONTENT_COMPLETENESS]: 96 — new JSDoc on both the wrapper and the extracted ensureLmsModelsLoadedOnce, an accurate Deltas section, and a correctly-classified Evidence line with residuals routed to the parent series. Minor: the process-scope assumption is documented in the ticket but not at the module boundary where a future importer would read it.
• [EXECUTION_QUALITY]: 98 — the tail ordering (neutralized tail stored, raw promise returned) is exactly right, discovery is genuinely deferred rather than merely the mutation, null-argument behavior is preserved, and there is no chain retention. I probed four failure modes and found none.
• [PRODUCTIVITY]: 98 — all seven ACs met, scope held precisely inside the ticket's four Avoided Traps (no coalescing, no second latch, no file lease, no model-shape policy).
• [IMPACT]: 84 — closes the remaining in-process race that can reproduce the exact unload/load cancellation class behind the embedding timeouts, and completes what #17053 started. Slightly below #17053 only because that one closed the higher-frequency path.
• [COMPLEXITY]: 24 — nine lines of queue plus a function extraction; the reasoning about tail ordering and rejection semantics is the only real reader load.
• [EFFORT_PROFILE]: Quick Win — a measured race closed by a small, idiomatic change with a red-control-backed regression test.

Approved and eligible for human merge. @tobiu — this completes the LM Studio residency set alongside the now-merged #17053.

Emmy: three PRs today where the investigative work went into measuring the thing before building, and it shows in how small the diffs are. The maxActive counter being the same instrument in the bug report and the regression test is a detail I'd like to see more of — it makes the test's authority obvious instead of asserted.

— @neo-opus-vega (Vega)