PR Review Summary

Status: Approved

Peer-Review Opening: This is the sharpest small diff on the deploy gate. Both hunks fix a wait-misread, and the second is a finding I had in a weaker form and never connected to timing.

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Both changes correct the defect class the whole #16706 incident turned on — conflating "waiting" with "failing" — and neither introduces a risk that was not already present. The one residual pre-dates the PR and was previously masked by the behaviour this removes, so holding the PR would preserve a strictly worse state.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16780's framing, the deployed-plane measurement at 08:29Z (two repos carrying KB_TENANT_REPO_SYNC_CONCURRENCY_GATE_TIMEOUT), TenantRepoSyncService.mjs at the PR head, and the semaphore acquire/release sites.
• Expected Solution Shape: a waiter that never obtained a slot must not be recorded as a failed sync, because that manufactures backoff for work never attempted.
• Patch Verdict: Matches, and improves on it — the second hunk addresses a consequence I had not anticipated.
• Premise Coherence: Coheres with verify-before-assert — the JSDoc states the mechanism ("timing out a waiter cannot bound runTask while the active holder is still pending") rather than asserting a symptom.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16961
• Related Graph Nodes: #16706 (incident epic), #16780, #16948, #16949
• Origin Session ID: 1d4a0d9f-87fb-4d68-827c-ffe16f80035e

🔬 Depth Floor

Challenge: concurrencyGateTimeoutMs_: 0 removes the only bound a waiter had, and the holder has no task-level bound — I grepped for taskTimeoutMs / runTimeoutMs / AbortSignal on the work itself and found none. If a holder's git or embed phase hangs, every waiter now blocks indefinitely.

Not blocking, and the reasoning matters: the old 30s timeout never bounded the holder either. It converted an unbounded wait into a false failure — inflating consecutiveFailures, triggering exponential backoff, and suppressing a repo that had attempted nothing. That is strictly worse than waiting, and it is visible on the external plane as two repos suppressed on gate timeouts. This PR removes the false failure; it does not create the unbounded wait.

Residual, to be owned rather than lost: nothing bounds a holder. This incident is precisely a provider retaining work for 16+ minutes, and ollama/ollama#11889 reports abandoned provider work running to completion upstream, so a hung holder is not hypothetical here. That belongs in #16706's open set, not in this diff.

Verified rather than assumed: semaphore.release() sits in a finally guarded by if (slotAcquired) (:2420), so a slot is released on the error path. The risk is confined to a holder that never returns, not one that throws.

Rhetorical-Drift Audit: N/A — mechanism-stating JSDoc, no architectural prose overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A waiter timing out is not a failed attempt. Recording it as one manufactures backoff for work never tried — the same "waiting read as broken" shape as the ollama list healthcheck, the stale canary classifier, and the residency tag compare. Four independent instances in one incident makes it a class, not a coincidence. The startedMs relocation is that same confusion at the timing layer, mirroring what avgExecutionMs does at the reporting layer (it measures dispatch→settlement residence while named for compute) — worth fixing as a pair.

🎯 Close-Target Audit

• Close-targets identified: #16961
• Confirmed not epic-labeled

Findings: Pass.

N/A Audits — 📑 📡 🪜

N/A across listed dimensions: no public/consumed contract surface, no MCP/OpenAPI description surface, and the ACs are covered by unit tests rather than unreachable runtime effect.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI CLEAN at 86ce255cc1
• Reviewer falsifier: checked whether the disabled gate leaves a waiter unbounded — it does, and the condition pre-dates the PR, making it a residual rather than a regression
• Test location: 100 added spec lines against 14 changed source lines, correct unit subtree

Findings: Pass.

📋 Required Actions

None. Approved as-is.

One item is recorded as a residual rather than a required action, because it pre-dates this diff and is out of its scope: no task-level bound exists on a concurrency-slot holder. It belongs to #16706's open set alongside O1, and should not gate this merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - corrects a misclassification at its source instead of compensating downstream.
• [CONTENT_COMPLETENESS]: 88 - the JSDoc carries the mechanism; the holder-bound residual is unstated.
• [EXECUTION_QUALITY]: 90 - small diff, heavy spec coverage, CI clean.
• [PRODUCTIVITY]: 95 - two real defects in 14 lines.
• [IMPACT]: 90 - directly unsuppresses the two repos observed stuck on gate timeouts.
• [COMPLEXITY]: 45 - a default change plus a variable relocation, both mechanism-justified.
• [EFFORT_PROFILE]: Targeted Correction - one enforcement default and one timing origin.

Merge-eligible; human merge gate per §0.

— @neo-opus-vega 🌿