3 commits

Author	SHA1	Message	Date
Gergő Magyar	561f913a32	fix(embeddings): retry unparseable 200 responses and survive partial embedding failures (#2790) (#2795) ... * fix(embeddings): retry unparseable 200 responses and survive partial embedding failures (#2790) A long-running embedding job against an OpenAI-compatible endpoint could lose hours of work to a single transient glitch, then refuse to recover on the next run. Four defects compounded: 1. An HTTP 200 carrying a truncated or non-JSON body was never retried. `classifyOutcome` treats any 2xx as success, and the `resp.json()` parse ran after `resilientFetch` had already returned, so the parse failure surfaced as a terminal error. Measured: a 503 got 3 attempts, a garbage 200 got 1. The parse and the response-shape check now run inside the `fetchImpl` callback, so a bad body is classified as a retryable failure and gets the same backoff as a 5xx. This also stops a garbage 200 from calling the circuit breaker's `recordSuccess()`, which previously erased accumulated failures and meant an endpoint alternating 5xx and garbage-200 could never trip it. 2. One failed `embedBatch` sub-batch aborted the entire pipeline. Failures are now tolerated: the sub-batch's node ids are collected and all of their embedding rows are deleted, so those nodes hold zero rows and are re-embedded later. Deleting rather than keeping partial rows is deliberate — chunk arrays are flat over a 16-node batch and sliced by 8, so a node's chunks can straddle a sub-batch boundary, and surviving rows carry the current content hash. The hash maps collapse per-chunk rows last-row-wins, so a partially embedded node would read as fresh forever and never regenerate its missing chunks. A run that fails 5 sub-batches in a row still aborts, and rethrows the first error of the streak rather than the last: after 3 failures the circuit breaker opens, so later errors degrade into "circuit open, retry in 30s" while the first still names the real defect. 3. The Phase 5 `embeddingCount === 0` fail-fast could not tell "wrote nothing" from "could not ask" — the count query's catch was silent. The count is now tri-state and only a known zero after real work is fatal. A non-numeric count previously bypassed the gate entirely, because `Number()` returns NaN and `NaN === 0` is false, and then serialized as `embeddings: null`. An unverified count no longer certifies `capabilities.vectorSearch.status`. 4. `saveEmbeddingCheckpoint` wrote a completion-shaped meta: it advanced `lastCommit`, wrote the new `fileHashes` and cleared `incrementalInProgress`. The first checkpoint window fires before a single embedding exists, and on a full rebuild the graph is still in a staging database that a crash discards. The next run then diffed against the advanced hashes, saw no changes and preserved the old graph — the "skipping wipe" symptom in the report. It now re-reads meta and replaces only the checkpoint, matching what the server endpoint already did. A partially failed run keeps its checkpoint with the failed ids in `pendingNodeIds`, so the next plain `analyze` regenerates them through the existing resume path. Clearing it would have been silent data loss: a plain run derives `shouldGenerateEmbeddings: false` once embeddings exist, so the pipeline would never have run again. The old crash-and-abort self-healed only by accident, via the checkpoint its crash left behind. `gitnexus status` reports the index incomplete until the nodes recover, and `--drop-embeddings` still abandons them. `POST /api/embed` is the pipeline's other caller and was discarding the result, reporting "Embeddings complete" for a partial run. It now persists the pending ids and reports the run as failed with the underlying endpoint error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(embeddings): abort a run whose sub-batch failure ratio is too high (#2790) The consecutive-failure ceiling only catches a total outage, because any successful sub-batch resets it. An endpoint under load shedding that alternates success and failure never trips it, so the run walks the whole corpus, deletes every failed node's rows and exits 0 having dropped a large fraction of the index. The retained checkpoint made that visible in `gitnexus status`, but a run that drops a quarter of the corpus should tell the operator to fix their endpoint, not leave them to notice a status flag. Adds a cumulative guard: abort once more than 25% of attempted sub-batches have failed, evaluated as the run progresses and gated behind a floor of 20 attempted sub-batches. The shape follows Resilience4j's circuit breaker (failure rate plus a minimum-sample floor) because it is the only one of the surveyed designs that answers the small-repo case — a three node repo can fail one sub-batch and never accumulate enough sample for a ratio to mean anything. The rate sits below a live traffic breaker's 50% because a batch indexer's job is to index the whole corpus rather than serve degraded traffic, and above Hadoop's single-digit `failures.maxpercent` because tolerating transient hiccups is the point of the change this follows. The guard reuses the existing break-then-cleanup path, so the failed batch's DELETE still runs before the rethrow, and it wraps the retained first-error-of- streak rather than inventing a new one, so the message names both the ratio and the underlying endpoint failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(server): record the embedding count after /api/embed so the next analyze cannot wipe it `POST /api/embed` generated embeddings and wrote them to the database but never wrote `stats.embeddings` into meta.json. Its checkpoint writer replaced only `embeddingCheckpoint`, and the finalize write folded in nothing else. So a repo embedded purely through the server kept whatever count the last CLI `analyze` stamped, which is 0 for a repo analyzed without embeddings. The next CLI run read `existingEmbeddingCount = 0`, `deriveEmbeddingMode` returned `shouldLoadCache: false`, and `gitnexus analyze --force` wiped the database with no cache load. Every server generated embedding was silently destroyed, with no warning — the user just lost semantic search. The route now measures the live count with the same query the CLI uses and folds it into both meta writes. The measurement is tri-state and deliberately never falls back to 0: an unverified count is written as absent rather than as zero, because a wrong-low value is exactly what arms the wipe. It is taken after `flushWAL()` and inside `withLbugDb`, so it describes durable rows and the connection is still open. A partial run records its honest count too, alongside the retained checkpoint, so the next CLI run preserves the partial index instead of discarding it. Found while working #2790; not part of that issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(embeddings): retry short 200 bodies and stop laundering body-phase timeouts Two gaps in the #2790 retry fix, both found by review. A 200 carrying `{"data": []}` or fewer vectors than inputs passed the in-`fetchImpl` shape check, because `every(isEmbeddingItem)` is vacuously true for an empty array. `resilientFetch` then classified it `success` and called `recordSuccess()`, erasing the outage signal, and the cardinality check in `httpEmbed` threw terminally one attempt later. That is exactly the pair of properties #2790 was filed about, still broken for this body shape — and worse than before the fix, since the pipeline now tolerates the error by deleting those nodes' rows instead of aborting loudly. The count check moves inside the retried callback; the outer one stays as a backstop. The `.json()` catch also swallowed every rejection, not just parse errors. `AbortSignal.any([caller, timeout])` is wired to the body stream, so a stalled body rejects with a DOMException — which, wrapped in a plain Error, defeated `classifyOutcome`'s terminal-network test. Measured: the same TimeoutError got 3 attempts and "unparseable response" when raised during the body read, but 1 attempt and "timed out after 180000ms" when raised by fetch itself, and three such sub-batches opened the process-global breaker that `recordNeutral()` exists to protect. Abort-like DOMExceptions are now re-raised unchanged. The dimension check stays outside the loop deliberately: it validates against `config.dimensions ?? DEFAULT_DIMS`, not the request-dimensions argument, and a width mismatch is a configuration error where retrying only triples latency and books failures against a healthy endpoint. Adds the negative assertion the review found missing: response body text must never reach the user-facing error string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(embeddings): scale the sub-batch failure-ratio floor to the run The cumulative guard needed 20 attempted sub-batches before a failure rate could abort anything — roughly 160 chunks, or ~80 embeddable nodes at the default subBatchSize of 8. A 50-node repo whose endpoint sheds every other sub-batch fails half of them and still exits 0: the ratio guard is below its floor, and every intervening success resets the consecutive ceiling. The floor was a good choice for a first run over a small repo, where one failure out of one sub-batch is 100% and means nothing. The defect is that every resume run has that shape by construction — its node set is only the pending ids — so the guard was structurally off in the one run whose entire purpose is retrying against the endpoint that already failed. The floor is now sized to the run: clamp(ceil(totalNodes / 16), 5, 20). The lower bound keeps the case the flat floor protected; the upper bound preserves today's behavior above 320 nodes and avoids a proportional-only floor perversely weakening the guard at scale, where a sixteenth of a 20k-node repo would be 1250 sub-batches of damage before a rate could fire. Resilience4j can use a constant minimumNumberOfCalls because a breaker sits on an unbounded call stream; a batch indexer has a finite budget, so a constant can exceed the whole run. The ratio is still evaluated only inside the catch. That is already its local maximum — both counters have just incremented — so sampling more often would only ever observe lower ratios. Also: a failing cleanup DELETE no longer swallows the abort, which was discarding the retained first-error-of-the-streak that names the real endpoint fault; `ceilingError` is renamed `abortError` since it carries the ratio abort too; and three `{ error }` log keys become `{ err }` (#2114 — an arbitrary key serializes to `{}`, losing message and stack). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): one tri-state embedding counter, and stop partial runs wedging later runs The tri-state count doctrine this branch introduced was applied at two of its three CLI sites, and the two implementations that were meant to mirror each other had already drifted. `measurePersistedEmbeddingCount` moves to `core/embedding-count.ts` — beside `embedding-mode.ts`, with the same no-native-imports property, and outside `core/embeddings/` so the lazy-embeddings convention (#2370) still holds. All three call sites now share it. - The mid-run `onCheckpoint` counter ran the query bare. A throw there — DB busy, connection closed, read-only, the VECTOR DML lock (#2623) — rejected the callback out of `runEmbeddingPipeline` and killed the analyze before Phase 5 could apply the tri-state that exists for exactly this case. A non-numeric cell wrote `stats.embeddings: null` to disk mid-run. - Phase 5 used `?? 0` while the server used `?? Number.NaN`, under a comment asserting both measured the field the same way. `Number.isFinite(0)` is true, so a no-row answer became a *measured* zero and hard-failed a run whose embeddings had all persisted. - The unknown-count fallback read `existingMeta`, assigned once at run start, so it republished the pre-run figure over the fresher count the terminal checkpoint had already written. With a prior count of 0 that armed the wipe chain: hasExisting false, shouldLoadCache false, and the next --force discards live embeddings. It now re-reads the latest on-disk meta, and an unverifiable count retains a recovery marker instead of clearing it. A completed-but-partial run also planted a landmine. Its checkpoint is stamped with the run's embedding identity, so a later plain `gitnexus analyze` from a hook, a CI job, or a shell without GITNEXUS_EMBEDDING_URL resolved provider 'local' and threw before any phase ran — after an exit-0 run, where previously only a visible crash left that state. `--force` did not help: the resume gate inspected only `--drop-embeddings`. `RepoMeta.embeddingCheckpoint` gains `kind` to tell the two situations apart. An 'interrupted' marker (or one with no kind, so markers already on disk keep the stricter path) still fails closed — its nodes may be half-written, and resuming under a foreign model would mix vector spaces. A 'partial' marker names nodes the pipeline already deleted to zero rows, so nothing is at risk: an identity mismatch drops the pending set with a warning and continues. `--force` now discards a checkpoint, and `attempts` bounds the retry at EMBEDDING_RESUME_MAX_ATTEMPTS (3, matching the HTTP embedder's and the WAL driver's existing per-operation budgets) so a node the endpoint deterministically rejects converges instead of keeping the repo incomplete forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(server): close the SSE stream on terminal job status, not a progress phase A tolerated partial run reached SSE clients as a clean success — a regression in this branch's own claim that /api/embed reports a partial run as failed. The pipeline emits `phase:'ready'` unconditionally before returning, including when it dropped nodes. The route mapped that to `'complete'`, and `mountSSEProgress` treated a terminal-looking *progress phase* as terminal: write the event, `res.end()`, `unsubscribe()`. The route's own `updateJob({status:'failed'})` then fired into a stream with no listener, and the web app had already shown "ready". Before this branch the pipeline threw, which produced `phase:'error'` and did reach the client. Pollers on GET /api/embed/:jobId were unaffected, so the two consumers disagreed. Terminality is a property of the job, so the relay now asks the job. Remapping `ready` alone would have left the trap armed: the `error -> 'failed'` mapping has the identical shape and would emit `event: failed` with `error: undefined` before the catch block fills the message in. `ready` is additionally remapped to `finalizing` so a poller no longer sees `status:'analyzing'` next to `progress.phase:'complete'`. The single-terminal-event property (#2264) is preserved on both the clean and partial paths, and /api/analyze is unaffected — its terminal progress phase is 'done', never 'complete'. `AnalyzeJob` gains an optional `partial` payload so a client can tell a partial run from a total failure without a new status member; it is absent on every other job, so existing payloads stay byte-identical. Consuming it in gitnexus-web is left to that app's owner — today it renders both as the same red retry chip. `resolveEmbedRunOutcome` moves to `embed-run-outcome.ts` and `mountSSEProgress` to `sse-progress.ts`, both free of Express/LadybugDB/MCP imports, and the local count copy is replaced by the shared `core/embedding-count.ts`. Reaching three pure functions previously meant importing the whole server: measured at ~20s against a 30s test timeout, with one observed timeout failure. That file is now 1.6s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: document the partial embedding index and its recovery A run can now finish exit 0 with a partial embedding index, which neither operator doc described. GUARDRAILS' "Embeddings vanished after analyze" Sign keys its trigger on `stats.embeddings` being 0 and lists "the only ways to end up at zero". A partial run stamps an honest non-zero count and sets `embeddingCheckpoint`, so the operator's actual symptom is `incompleteReasons: ["embedding-checkpoint-pending"]` — a state that Sign cannot match. Adds a Sign for it and drops the exhaustive framing from the existing one. RUNBOOK gains the recovery path: a plain `gitnexus analyze` is correct and needs no flag, because a retained checkpoint forces generation for the pending nodes regardless of flags. Also corrects two stale claims — that `stats.embeddings` is always freshly measured (it can carry forward when the count query cannot answer, which is why `capabilities.vectorSearch.status` is the certified read), and that later analyzes must always pass `--embeddings` or lose their vectors, which contradicts Non-negotiable 5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(embeddings): one owner for the checkpoint record and the abort predicate Cleanup pass over the #2790 review fixes. No behavior change except where noted; the two exceptions are both cases where the code was lying to the operator or to the other half of itself. The previous pass extracted `core/embedding-count.ts` because two hand-copied bodies of "measure the embedding count" had drifted inside a single change. It then created a second pair of hand-copied publishers — of `RepoMeta.embeddingCheckpoint` — and those had drifted too: the CLI armed the attempt counter only after clearing its identity gate, the server derived it from the resumed marker alone. Only one of the two READERS implemented `kind` at all, so a 'partial' marker written by `gitnexus analyze` and resumed through POST /api/embed still hit the permanent wedge `kind` exists to remove. `core/embedding-checkpoint.ts` now owns the record: `checkpointKind` (the one home for absent-means-interrupted), the three minters, `nextAttemptCount`, and `decideEmbeddingResume`, which both gates route through. Five mint sites and two resume gates become one implementation each. `resilient-fetch.ts` exports `isTerminalNetworkError` and `classifyOutcome` calls it, replacing a caller-side copy of the same DOMException test whose docstring promised it "mirrors classifyOutcome exactly" — an invariant enforced by prose, where a divergence silently reverts body-phase timeouts to being retried three times and charged to the shared breaker. The ratio-guard floor now divides by the run's actual `subBatchSize` instead of a constant 16 that assumed the default of 8. At `subBatchSize: 32` the old formula demanded more sub-batches than the run contains, leaving the guard structurally off — the exact failure the scaled floor was introduced to fix, and sub-batch size is tuned mainly for the flaky endpoints it protects. Two operator-facing corrections: - The count-recovery marker was stamped `kind: 'partial'` with an empty pending set, so `gitnexus status` reported "N node(s) lost their embeddings" where N is zero. It gets its own kind and its own incomplete reason. - `decideEmbeddingResume` initially keyed its skip-the-identity-gate branch on an empty pending set, assuming that meant the count-recovery marker. It does not: `onCheckpoint` mints an 'interrupted' marker with no pending nodes after every post-window save. That silently cleared an interrupted marker under a foreign provider instead of failing closed. Keyed on `kind` now, with a regression test. Also: `isTerminalJobStatus` adopted at the seven sites that still hand-copied it, including the one gating the single-terminal-event emit; `mountSSEProgress` re-export dropped and `server-sse-payload.test.ts` repointed at the extracted module, which takes it from 24.60s to 0.408s — the test that motivated the extraction was still paying the cost it was meant to remove; the count-mismatch message and the SSE test harness deduplicated; per-batch error strings made lazy (~75k needless `new URL()` per large run); `retryable: true` dropped as a field that can never be false; ~110 lines of restated rationale reduced to pointers at their canonical home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-02 20:43:59 +00:00
Eva	711ff8721d	fix(embeddings): make HTTP generation resumable	2026-07-14 02:15:57 +07:00
Gergő Magyar	152a0506c9	feat: shared resilient-fetch (retries + circuit breaker) (#1448) ... * feat: shared resilient-fetch (retries + circuit breaker) Add a small, runtime-agnostic resilience layer in gitnexus-shared and migrate every backend HTTP outbound call (CLI, MCP, wiki LLM, web → backend) through it. Helpers (gitnexus-shared/src/integrations/): - retry.ts — withRetry(fn, opts) with caller-supplied retryability classification and full-jitter exponential backoff. - circuit-breaker.ts — closed/open/half-open per-process breaker with injectable clock, plus a keyed registry so callers targeting the same endpoint share state. - resilient-fetch.ts — composed wrapper: retries 5xx + 429 + retryable network throws, treats AbortSignal.timeout() and 4xx (other than 429) as terminal, honors Retry-After (capped at 30s), throws CircuitOpenError when the breaker opens. Migrations (no behaviour regression — all existing tests pass): - gitnexus/src/core/embeddings/http-client.ts (covers analyze + MCP query path) — replaces inline linear-backoff retry. - gitnexus/src/core/wiki/llm-client.ts — preserves Azure content-filter branch; resilientFetch handles 5xx/429. - gitnexus-web/src/services/backend-client.ts (fetchWithTimeout helper) — small retry budget (2 attempts, 250–1500 ms) so a dead local backend still fails fast for the user. - gitnexus-web/src/core/llm/settings-service.ts (OpenRouter model list). Deliberately not migrated: - gitnexus-web/src/services/backend-client.ts streamJob() — Server-Sent Events stream; the existing reconnect-with-Last-Event-ID logic is not unary-fetch shaped. - gitnexus-web/src/components/SettingsPanel.tsx checkOllamaStatus() — one-shot health probe; retrying delays the "Ollama not running" error rather than improving UX. 41 new helper tests cover backoff math, breaker state transitions, Retry-After parsing (delta-seconds + HTTP-date), 401/422 terminal classification, and breaker fail-fast on three exhausted retry batches. * fix(review): apply autofix feedback Address Claude's two MEDIUM blocking findings on PR #1448 plus the CodeQL SSRF false-positive flag. - backend-client `fetchWithTimeout` now uses `AbortSignal.timeout()` merged with the caller's signal via `AbortSignal.any()`. Timer-fired aborts surface as `DOMException(name='TimeoutError')` so resilientFetch routes them through the terminal-network branch (no retry, no breaker hit), instead of incrementing the breaker for user-side network slowness. - Method-aware retry budget in `fetchWithTimeout`: idempotent verbs (GET/HEAD/OPTIONS) keep the 2-attempt budget; POST/PATCH/PUT/DELETE default to single-attempt so a 5xx on `startAnalyze` cannot start a duplicate job. New `forceRetry` parameter for callers that know-idempotent mutations (e.g. DELETE of a known-deleted resource). - `resilient-fetch.ts` carries a documented suppression for CodeQL js/server-side-request-forgery on the inner fetch call. Every concrete caller passes a hardcoded URL constant or a value from configuration (env vars, saved settings); user request input never flows into the URL parameter. - New test file `backend-client-retry.test.ts` covers all three paths: GET retries on 503, POST does not retry, timeout does not increment the breaker. * fix(resilient-fetch): address Codex adversarial findings Closes the three blocking issues from Codex's review on PR #1448. U1 — Add `recordNeutral()` to CircuitBreaker. Third outcome path that's an explicit no-op for state and the consecutive-failure counter. Distinct from `recordSuccess` (closes the breaker) and `recordFailure` (may open it). Used for outcomes that are neither evidence of backend health nor evidence of backend failure. U2 — Route terminal-client / terminal-network through `recordNeutral`. Previously a 401 or local timeout called `recordSuccess`, which reset `consecutiveFailures` to 0. A 5xx → 401 → 5xx → 401 → 5xx sequence would NEVER trip the breaker because each 4xx in between erased the running count. Also classify external `AbortError` as terminal-network (was retryable-network), so caller-driven cancellation no longer retries against an already-aborted signal or counts toward breaker failures on exhaustion. U3 — Per-origin breaker key in web `fetchWithTimeout`. Was hardcoded to `'web-backend'` even though `_backendUrl` is mutable via `setBackendUrl`. Switching backend URLs after a circuit tripped on host-A would strand the user during the full cooldown. Key is now `web-backend:<origin>`, so each backend URL gets its own breaker state. Tests: +5 recordNeutral, +4 resilient-fetch (interleaved 4xx/5xx, external AbortError, prior-state preservation), +1 web switch-backend regression. All 70 gitnexus integration tests + 15 web tests green. * fix(resilient-fetch): tolerate header-less fetch mocks on 429 `classifyOutcome` called `resp.headers.get('Retry-After')` directly, which crashed when a test stubs `fetch` with a plain object like `{ ok: false, status: 429 }` (no `headers` field). Real `Response` always has Headers, so this surfaces only in test setups, but the helper has no business assuming caller-side correctness on this — the defensive guard is cheap and a missing `Retry-After` falls through to exponential-backoff retry like any 429 without the header. Surfaced by `gitnexus/test/unit/http-embedder.test.ts > retries on rate limit`, which the embeddings migration exercises against a plain-object 429 stub. Locked in with a new `classifies 429 from a header-less fetch mock without throwing` case. * fix(review): apply autofix feedback Closes findings from the third multi-agent review pass on PR #1448. #1 (P1) callLLM had no per-attempt timeout Wiki LLM calls passed no `signal` to resilientFetch; each of three retry attempts could hang indefinitely on a frozen TCP connection. Add `signal: AbortSignal.timeout(60_000)` so the per-attempt budget matches what http-client.ts and backend-client.ts already provide. #2 (P2) drop dead `lastRetryableResp` post-loop fallback Variable was set in one switch arm but only read in unreachable code after the loop. The retry loop always returns/throws on every iteration. Keep only the defensive `throw` so TypeScript's control-flow analysis still sees `Promise<Response>` as the return. #5 (P2) gate test-only exports behind a subpath `__resetBreakerRegistry__` and `classifyOutcome` were reachable from the main `gitnexus-shared` barrel — production code calling `__resetBreakerRegistry__` from a tool implementation would silently nuke every circuit breaker process-wide. Move to a new `gitnexus-shared/test-helpers` subpath export. Production callers see the cleaner public API; tests import via the explicit `gitnexus-shared/test-helpers` path. #6 (P2) exhaustiveness guard on Outcome switch Add a `default: const _: never = outcome` arm so a future sixth `Outcome.kind` won't compile silently — it'll surface at the switch site rather than fall through to a retry/no-retry default. #9 (P3) document cumulative wall-clock budget Add a "Cumulative wall-clock budget" paragraph to resilientFetch's JSDoc explaining the worst-case total wait (`maxAttempts × (per-attempt timeout + capDelayMs)` ≈ 60s with defaults) and pointing callers at outer `AbortSignal.timeout()` when they want a tighter bound. Deferred to follow-up PRs (per review's Auto-resolve recommendation): - #3 idempotency knob to shared API (forceRetry into ResilientFetchOptions) - #4 publish.ts migration to resilientFetch - #7 parseRetryAfter past-HTTP-date / negative-seconds asymmetry - #8 recordNeutral counter time-decay (documented breaker semantic) * fix(circuit-breaker): gate half-open to a single in-flight probe Closes the Codex adversarial-review finding on PR #1448 that flagged a recovery-time thundering herd: when cooldown expired, every concurrent caller transitioned the breaker to half-open and probed the still- recovering dependency in lockstep, defeating the breaker's "fail fast" promise. U1 — probe-permit gate in CircuitBreaker.check() Added a `probeInFlight: boolean` field. After cooldown expires, the first `check()` admits the probe and consumes the permit; subsequent callers throw `CircuitOpenError` with a configurable `halfOpenRetryAfterMs` (default 1000ms) until the probe resolves. Critical design point: `recordNeutral` now RELEASES the permit but does NOT transition state. Without that split, a single `TimeoutError` from per-attempt `AbortSignal.timeout` (which routes through neutral classification) would permanently park the breaker in half-open. By separating permit-release from state-resolution, we keep the "neutral doesn't claim health" semantic without creating that wedge. Other changes: - `halfOpenRetryAfterMs` is now a constructor option for consumers with long-running protected ops (LLM streaming, large uploads). - `getState()` is documented as a pure read; the implicit Open -> Half-Open transition lives in `check()` only, so tests that inspect state never inadvertently consume a probe permit. - `isProbeInFlight()` test-only accessor for assertion clarity. - JSDoc on `check()` records the JS event-loop atomicity dependency and the load-bearing `try/finally` pairing invariant. U2 — End-to-end concurrency regression through resilientFetch Three new scenarios in resilient-fetch.test.ts (26 -> 29): - 3 concurrent calls + probe gets 200 -> 1 hits fetch, 2 throw CircuitOpenError, breaker closes. - 3 concurrent calls + probe gets 503 -> ResilientFetchExhaustedError on probe; concurrent callers see halfOpenRetryAfterMs (1000ms); fresh caller after probe resolves sees the FULL new cooldown (10000ms), not the probe-in-flight default. - Probe cancelled mid-flight via AbortError -> permit released, state stays half-open, next caller becomes the new probe and succeeds. Plus 9 new circuit-breaker unit tests (16 -> 25) covering the permit gate, recordNeutral-releases-permit semantic, fresh-cooldown distinction, default vs configurable halfOpenRetryAfterMs, getState() purity, and the three-probes-via-neutrals chain. Total integration test count: 70 -> 82. All 106 gitnexus + 15 web tests pass; both packages typecheck. Maintainer decisions (deferred per plan 003 Open Questions): - Plan 002's deferral judgement was reversed on Codex's argument without new measurement / incident data. The reversal is defensible on principle (Hystrix / Resilience4j alignment) but lacks workload- driven evidence. - Probe-blocked callers throw silently (no log / event hook). R4's "no new public API" prevents adding observability; loosen if a debug log on probe-blocked is wanted. * refactor(embeddings): replace bespoke HF breaker with shared CircuitBreaker Deleted the local `HfDownloadCircuitBreaker` class and the manual retry loop in `withHfDownloadRetry`. Both are now backed by the shared `gitnexus-shared` primitives: - `hfDownloadCircuit` is `new CircuitBreaker({ failureThreshold, cooldownMs, key: 'hf-download' })` — same state machine as before PLUS the single-permit half-open gate that prevents recovery-time stampedes when CLI + MCP embedders concurrently re-load the model. - `withHfDownloadRetry` delegates the loop to `withRetry` from the shared package. Per-attempt timeout (`withDownloadTimeout`), network-vs-non-network classification, circuit recording, and the `onRetry` callback wire through `withRetry`'s `isRetryable` callback. Behaviour preserved: - Pre-flight `CIRCUIT_OPEN_TAG` rejection when the breaker is open. - Mid-loop `CIRCUIT_OPEN_TAG` "opened after N consecutive failures" when a network error trips the threshold. - Non-network errors (e.g. CUDA unavailable) bypass retry and go through `recordNeutral` instead of resetting the breaker's failure-count progress. - `onRetry(attempt+1, max, err)` fires only when there's a next attempt, matching the prior semantic. Generic CircuitBreaker gained two inspection accessors: - `getOpenedAt(): number | null` - `getCooldownMs(): number` Used by `withHfDownloadRetry` to compute `secsUntilReset` without consuming a probe permit (which `check()` would do). Test consolidation: the 7 bespoke `HfDownloadCircuitBreaker` state-machine tests in hf-env.test.ts were 1:1 duplicates of existing tests in `circuit-breaker.test.ts` and were deleted. Remaining 42 hf-env tests all pass; full integration sweep (148 gitnexus + 15 web) green.	2026-05-09 15:18:09 +01:00