Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Aug 14, 2026, 12:07 PM |
| updatedAt | Aug 24, 2026, 9:45 PM |
| closedAt | Aug 14, 2026, 2:09 PM |
| mergedAt | Aug 14, 2026, 2:09 PM |
| branches | dev ← agent/16997-shared-provider-timeout-notification |
| url | https://github.com/neomjs/neo/pull/17106 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The diff closes the real provider-timeout bypass with one shared predicate and one shared notifier while preserving the two lanes' different admission mechanics. Exact-head source, production composition, focused tests, and a reviewer falsifier support merge safety.
Peer-Review Opening: Grace, this is ready. The strongest part is the fixture evolution: wrong-reason greens were removed, and the final two-repository HTTP composition proves the queued repository never reaches the failed provider and resumes on a fresh sweep.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #16997 and its correction thread; #12814/#12818; current TenantRepoSync, IngestionService, VectorService, TextEmbeddingService queue/admission sources; exact merge-base diff; exact-head CI.
- Expected Solution Shape: One timeout classifier at the existing SSOT; one safe synchronous notifier; invocation at OpenAI-compatible final queue settlement before next selection and at Ollama provider settlement before slot release; caller-owned tenant circuit unchanged.
- Patch Verdict: Matches. The helper is shared, each lane invokes it at its real admission boundary, and the caller still owns abort/circuit-open semantics.
- Premise Coherence: Coheres with verify-before-assert: the production composition exercises real HTTP transport, real queueing, one run-scoped circuit, distinct A/B outcomes, and fresh-run recovery.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16997; related to #17072.
- Related Graph Nodes: #16995/#16996, #12814/#12818, #16869.
- Origin Session ID: 019ffcf3-1a96-7020-b1fc-e1673092fcca
🔬 Depth Floor
Challenge: I independently changed the exact head so the real TenantRepoSyncService circuit abort ran one microtask late, then ran only the production two-repository composition. It still passed. I restored the source byte-for-byte and git diff --exit-code passed. This falsifies the ticket's literal AC-4 prediction, not the implementation: deleting notification entirely is the load-bearing mutant because then no circuit ever opens and B reaches the provider.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: implementation claims match the exact diff.
- Anchor & Echo summaries: provider/admission terminology is precise.
-
[RETROSPECTIVE]tag: N/A — none added. - Linked anchors: #12814/#12818 establish the shared timeout contract.
Findings: Pass, with one non-blocking ticket-body correction: AC-4 should name the empirically true delete-notification control rather than the disproven one-microtask prediction.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: The original AC-4 falsifier was too specific; running it against the real circuit showed that one microtask of delay remains contained.[RETROSPECTIVE]: A fixture must prove that the queued request genuinely entered admission; request-count assertions before enqueue are vacuous.
🎯 Close-Target Audit
- Close-targets identified: #16997.
- #16997 confirmed not
epic-labeled.
Findings: Pass. The user-visible target is met: A retains provider timeout, B makes zero provider calls and receives circuit-open, and a later sweep uses a fresh circuit.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented PR diff matches the ledger's ownership boundaries.
Findings: Pass. Timeout identity remains with createTimeoutError.mjs; notification ordering stays in TextEmbeddingService; circuit authority remains in TenantRepoSyncService.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration. - Achieved L2 evidence covers all runtime behavior needed for the close target.
- No residual requires a separate owner.
- The PR distinguishes provider settlement from client timeout.
- No L2 receipt is promoted to L3.
- No external runtime receipt is used as a merge gate.
Findings: Pass.
N/A Audits — 📡 🔗
N/A across listed dimensions: no OpenAPI tool description, skill, startup, or turn-loaded substrate changes.
📜 Source-of-Authority Audit
createTimeoutError.mjs owns timeout identity. TextEmbeddingService owns the two local admission boundaries and notification ordering. TenantRepoSyncService owns the run-scoped AbortController and circuit-open reason. The diff preserves those boundaries and does not manufacture caller policy in either provider path.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head required CI green at
da938dd808c; focused reviewer run 171/171 passed. - Reviewer falsifier: delayed the real tenant circuit abort by one microtask; the production composition still passed; source restored cleanly.
- Test location: provider contract, embedding queue, and tenant composition specs sit with their owners.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 94 - Shared contract without false queue unification.[CONTENT_COMPLETENESS]: 94 - Both lanes, negative classification, production containment, and fresh-run recovery covered.[EXECUTION_QUALITY]: 95 - Strong ordering placement and mutation-sensitive production fixture.[PRODUCTIVITY]: 93 - Directly prevents repeated doomed repository dispatches.[IMPACT]: 91 - High-value hardening for constrained provider planes.[COMPLEXITY]: 89 - Small shared primitive with provider-specific boundaries preserved.[EFFORT_PROFILE]: Heavy Lift - Crosses provider identity, two admission lanes, and tenant-run containment.
Ready for the next private tenant deployment SHA once merged. 🧭
Confidentiality redaction (2026-08-24): one private client identifier was replaced with “private tenant deployment”; technical meaning is unchanged.
Resolves #16997
Related: #17072
A typed provider timeout now reaches the caller-owned tenant-run circuit before either local embedding lane can dispatch again, through one shared predicate and one shared ordered notification helper rather than a second policy implementation per provider.
Evidence: L2 (a two-run tenant-sync production composition over a real local HTTP provider, plus predicate/containment unit matrices) → L2 required (every close-target AC is an in-process ordering or classification property). No residuals.
What lands
ai/provider/createTimeoutError.mjs). The four-code list had been re-typed at four call sites in three shapes; the membership test now lives with the codes it tests.notifyProviderTimeout) invoked at BOTH real admission-settlement boundaries — native Ollama's provider-promise rejection before slot release, and the OpenAI-compatible drain's final queue-task rejection beforewhileselects another task and beforereject, so no caller continuation observes it first.onProviderTimeoutthreaded through the OpenAI-compatible single AND batch paths. The batch method did not accept the control at all, so the queue task could never reach the hook even once wired.Acceptance criteria
notifyProviderTimeout+isProviderTimeoutCode; no provider-specific copy of code matching, containment, or ordering[["org/oai-timeout-b"], ["org/oai-timeout-a"]]KB_VECTOR_EMBED_PROVIDER_TIMEOUT, queued repo getsKB_VECTOR_EMBED_PROVIDER_CIRCUIT_OPEN#postOpenAiCompatibleexhausted its contention/unload retriescreateTimeoutError.spec.mjs— abort, circuit-open, generic, busy/model-load, near-miss and non-string codes all falseDeltas from ticket
Step 2 is narrower than "replace the repeated four-code classifiers", deliberately. Two of the four sites are not pure four-code classifiers:
VectorService's cause-walk also matchesABORT_ERRandKB_VECTOR_EMBED_PROVIDER_CIRCUIT_OPEN. Those are composed around the predicate, never folded in — an abort and an open circuit are caller-owned facts with different outcomes, and absorbing them would make a cancelled request read as a provider timeout at every consumer at once, contradicting AC-7.TextEmbeddingService.isOpenAiCompatibleContentionTimeoutErrormatches three codes plus an HTTP contention regex —PROVIDER_TIMEOUTis absent because the OpenAI-compatible embedding transport never stamps it. Substituting the four-code predicate there would widen contention retry rather than deduplicate it, so it is left alone with a comment saying why.One behaviour change beyond the ticket's letter, called out rather than buried. The native Ollama site previously notified only on
PROVIDER_TIMEOUT; through the shared helper it now notifies on the full typed set. A native request dying at the socket layer (ETIMEDOUT/ESOCKETTIMEDOUT) was invisible to the circuit there — the same defect in the native path that this ticket fixes in the queued one. AC-1 requires one shared predicate, so it follows from the ticket; it is still a widening and reviewers should treat it as one.Test Evidence
TenantRepoSyncService.spec.mjs+TextEmbeddingService.retry.spec.mjs— 166 passed.createTimeoutError.spec.mjs,drainCycle.spec.mjs,TextEmbeddingService.spec.mjs,VectorService.batchFailureIsolation.spec.mjs— 121 passed.notifyProviderTimeoutcall turns the production composition RED with the queued repository making a real provider call (2 requests instead of 1). The composition cannot pass without the fix.Two corrections recorded, because both cost real time
My unit-level fixture could not exercise the ordering, and I initially reported that as a property of the lane. Three falsifiers — microtask and macrotask deferral of the hook's circuit-open, and delaying the drain's own notification — all left the queued repository at zero provider calls, and I concluded the ordering was not load-bearing. That conclusion was an artifact of the fixture: at that level the queue's own abort-listener removal wins the race the production path actually has. The ticket author's call that one compact two-run production composition was the right proof was correct, and the mutation above is the demonstration. The unit test keeps its narrower fact and now carries a scope note pointing at the composition.
An earlier fixture passed for the wrong reason. At a 25ms batch timeout the second repository never finished its async input prep, so it never entered the queue —
requestCount === 1was true for a reason unrelated to the circuit. Both unit tests now run at 400ms with a settle window so the queued repository genuinely enqueues.Post-Merge Validation
Authored by Grace (Claude Opus 5, Claude Code). Session 471d17f2-777c-4676-a137-fa37a9ac834d.