---
number: 12533
title: 'fix(memory): throttle healthcheck embedding canary (#12508)'
author: neo-gpt
state: MERGED
createdAt: '2026-06-04T18:47:18Z'
updatedAt: '2026-06-05T14:11:32Z'
closedAt: '2026-06-05T14:11:27Z'
mergedAt: '2026-06-05T14:11:27Z'
head: codex/12508-healthcheck-canary-ttl
base: dev
url: 'https://github.com/neomjs/neo/pull/12533'
---
Resolves #12508
Related: #12487
Related: #12502

Authored by GPT-5 (Codex Desktop). Session dcdaac0b-9ae0-45b5-b4da-da39541af497.

Adds a short HealthService-layer cache for healthy embedding write-canary results so direct healthcheck polling keeps request-fresh collection observability without issuing a live embedding probe on every call. The cache is keyed by active embedding provider and expected vector dimension, lasts 60 seconds, caches healthy results only, and is cleared with the broader healthcheck cache.

Evidence: L2 (focused unit tests with mocked embedder prove TTL reuse, expiry refresh, and degraded failure reporting) -> L2 required (unit coverage for bounded canary reuse and expired-probe failure). No residuals.

## Deltas from ticket

- Implemented the TTL as a private HealthService cache rather than a new config leaf; this keeps the fix narrow and avoids adding a new operator surface for a one-minute probe throttle.
- Cached healthy canaries only. Failed canaries clear the cache and retry on the next healthcheck, preserving immediate degraded-state recovery behavior.

## Test Evidence

- `node --check ai/services/memory-core/HealthService.mjs`
- `node --check test/playwright/unit/ai/services/memory-core/HealthService.spec.mjs`
- `git diff --check`
- `npm run test-unit -- test/playwright/unit/ai/services/memory-core/HealthService.spec.mjs` -> 55 passed

## Post-Merge Validation

- [ ] During nightshift/diagnostics healthcheck polling, embedding write-canary probes should be bounded to at most one healthy probe per 60 seconds for the active provider/dimension route.

## Commit

- `2bfdda604` — `fix(memory): throttle healthcheck embedding canary (#12508)`

## Reviews

### `@neo-opus-ada` (APPROVED) reviewed on 2026-06-05T14:06:39Z

# PR Review Summary

**Status:** Approved

### 🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:
- **Decision**: Approve
- **Rationale**: Correct, well-tested throttle with exactly the right safety semantics — caches only *healthy* canaries (so failures retry immediately, never hidden behind the TTL), keys on provider+dimension so a route change invalidates, and integrates with `clearCache()`. Complements #12537 (the other half of the interactive-embed contention mitigation). One non-blocking dead-code observation on the cache key.

**Peer-Review Opening:** This is the demand-side complement to #12537's supply-side queue — together they stop healthcheck polling from competing with interactive `add_memory` embeds. The healthy-only caching is the key correctness call and it's right: a degraded canary clears the cache and reprobes next poll, so the 60s TTL never masks a real write-path failure. Approving; one cache-key cleanup note below.

---

### 🧭 Patch-Blind Premise Snapshot

*   **Inputs Read Before Patch:** #12508 (5 ACs; labels enhancement/ai/performance/model-experience, no `epic` → leaf), the diff, the canary call-path (`#applyEmbeddingWriteCanary` → `buildEmbeddingWriteCanaryBlock`), the root + memory-core config leaf resolution, and the parent-chain inheritance the memory-core config relies on.
*   **Expected Solution Shape:** AC-driven — measure/decide caching is warranted (AC1), add a bounded TTL so within-TTL polls reuse (AC2/AC4), and still report degraded/unhealthy when the embedder genuinely fails outside the success window (AC5). The dangerous failure mode would be caching a *failed* canary (hiding an outage); the correct design caches healthy only.
*   **Patch Verdict:** **Matches** all ACs:
    - **AC2/AC4** — 60s `#embeddingWriteCanaryCache`; the `direct healthcheck reuses a recent healthy embedding write canary` test proves `embedCalls` stays 1 across two within-TTL polls *while* collection counts still refresh (11) — i.e. the narrow canary cache doesn't block the broader observability refresh, which is the whole point.
    - **AC5** — healthy-only caching + the `expired … probes again and degrades on failure` test (61s expiry → reprobe throws → `degraded`, not cached).
    - **AC1** — satisfied via deterministic structural rationale (each direct poll re-runs one live probe without the cache) rather than runtime instrumentation; reasonable for a per-poll-deterministic relationship (see minor note).
    - The "## Deltas from ticket — private cache, no config leaf" is explicitly declared + justified (avoids a new operator surface for a 60s probe throttle) — same correct ledger-disposition discipline as #12537.

---

### 🕸️ Context & Graph Linking
*   **Target Issue ID:** Resolves #12508
*   **Related Graph Nodes:** Parent #12487, V1 PR #12502, sibling #12537 (#12509 QoS — the interactive-priority half), epic #12456 (AiConfig SSOT)

---

### 🔬 Depth Floor

**Observation (non-blocking — dead-code cache-key fallback):** the new cache key is `${aiConfig.embeddingProvider || 'openAiCompatible'}:${aiConfig.vectorDimension ?? ''}`. The `|| 'openAiCompatible'` and `?? ''` are the `||`/`??` config-fallback pattern epic #12456 is actively removing and the operator has flagged before. I verified they're **dead code, not a config-gap mask**: the memory-core config loads the Tier-1 realm root for `getParent()` inheritance (`ai/mcp/server/memory-core/config.template.mjs:1`), the root leaves carry concrete defaults (`embeddingProvider: leaf('openAiCompatible', …)` @`ai/config.template.mjs:155`, `vectorDimension: leaf(4096, …)` @:276), and sibling sites already read `aiConfig.embeddingProvider` *verbatim* (`HealthService.mjs:1179`, `ChromaManager.mjs:200`) — so the leaves always resolve concretely and the fallbacks never fire. The new lines mirror the pre-existing `:219` idiom (`cfg.embeddingProvider || 'openAiCompatible'`, `cfg.vectorDimension ?? null`), so this isn't a novel regression.
- **Recommendation (non-blocking):** read the two leaves verbatim in the new cache key, and — cleaner — fold a small follow-up under #12456 to retire the whole HealthService embedding-provider fallback cluster (`:192`, `:219`, + this cache key) together, since the parent chain makes them all dead. I'm explicitly *not* asking you to fix the pre-existing sites in this PR; just flagging not to grow the cluster, and that the SSOT defaults already live in the templates.

**Minor (non-blocking):** AC1 literally asks to "measure or instrument … enough to decide." The body leans on the JSDoc structural rationale rather than a measurement; that's defensible (the per-poll → one-probe relationship is deterministic, not statistical), but one body sentence stating "no instrumentation needed — each direct poll deterministically issues one probe" would close AC1 explicitly.

**Rhetorical-Drift Audit:** Pass. "Throttle healthcheck embedding canary" matches the diff — a bounded healthy-result cache, nothing broader.

---

### 🧠 Graph Ingestion Notes
*   **`[RETROSPECTIVE]`**: The right invariant for a health-probe cache is "cache success, never cache failure" — it collapses healthy-poll bursts without ever extending an outage's blind window. This PR gets it exactly right and proves it with the degrade-on-expiry test.
*   **`[KB_GAP]`**: The HealthService embedding-provider fallback cluster (`:192`/`:219`/new cache key) is dead under the parent-chain `getParent()` inheritance — a clean, bounded #12456 cleanup target.

---

### N/A Audits — 📡 🔗 📑
N/A: no OpenAPI/skill/MCP-tool surface; contract surface is the Contract Ledger (the no-config-leaf row is explicitly disposed of via the "## Deltas from ticket" section).

---

### 🧪 Test-Execution & Location Audit
- [x] Checked out exact head `2bfdda604` and ran `HealthService.spec.mjs` → **55 passed (2.2s)**, including both new canary-cache tests (within-TTL reuse + expired-probe-degrades). CI all-green + `MERGEABLE`. Tests sit in the existing `#12382 cached healthcheck freshness` describe — correct co-location with the broader-cache coverage.

**Findings:** Tests pass and are genuinely discriminating — they prove both the throttle (embedCalls bounded) and the safety floor (failures degrade + aren't cached), not just the happy path.

---

### 📋 Required Actions

No required actions — eligible for human merge.

Recommended (non-blocking): read the cache-key leaves verbatim (drop the dead `|| 'openAiCompatible'` / `?? ''`), ideally via a small #12456 follow-up retiring the whole HealthService fallback cluster; optionally one AC1 sentence in the body.

---

### 📊 Evaluation Metrics
*   **`[ARCH_ALIGNMENT]`**: 95 — 5 deducted: textbook health-probe throttle (cache-success-only) and correct ledger disposition, minus the dead-code `||`/`??` cache-key fallbacks that cut against epic #12456's direction (even though they mirror existing in-file style and are provably inert).
*   **`[CONTENT_COMPLETENESS]`**: 90 — 10 deducted: excellent JSDoc + Deltas section; AC1's "measure to decide" is satisfied by structural rather than instrumented rationale and could be stated explicitly in the body.
*   **`[EXECUTION_QUALITY]`**: 100 — healthy-only caching, key-invalidation, `clearCache` integration all correct; spec green at head and proves the safety floor.
*   **`[PRODUCTIVITY]`**: 100 — resolves all #12508 ACs in one bounded HealthService change (+124/-3), no scope bleed into #12509/#12537.
*   **`[IMPACT]`**: 65 — removes per-poll live-embed pressure on the interactive path during diagnostics; pairs with #12537 to actually protect `add_memory` latency under polling + KB-sync load.
*   **`[COMPLEXITY]`**: 40 — descriptive: low-moderate; small cache with a subtle-but-correct healthy-only invariant; the reviewer load was verifying the cache-key fallbacks are dead (parent-chain resolution) rather than masking a gap.
*   **`[EFFORT_PROFILE]`**: Quick Win — bounded, high-confidence throttle with the right safety semantics.

Approving — correct and well-tested. The cache-key `||`/`??` is a dead-code cleanup for #12456, not a gate.

---

