diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b298efd..7fd8ab16 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,11 +5,22 @@ on: # No `pull_request` trigger: this repo's PRs target `develop` (feature/fix → # develop, merge-commit style) and `master` (develop/release → master, # squash), never `main` — a `pull_request: branches: [main]` trigger would be - # permanently dead here since no such branch exists. `push` already covers - # every commit that matters, including the develop→master release merge - # (added below) which previously had ZERO CI build/test coverage — - # protect-master.yml only checks the source-branch name, not code quality. - branches: [develop, master, "feature/**", "features/**"] + # permanently dead here since no such branch exists. + # + # ⚠️ The branch list below is an ALLOWLIST OF PREFIXES, and that is a + # standing footgun: a branch whose prefix is missing gets NO ci.yml run at + # all, silently — the PR still shows green because CodeQL (a separate + # workflow, `pull_request`-triggered) runs and is the only check present. + # This is not hypothetical: `fix/**` was missing until PR #192, so every + # `fix/...` branch — the repo's own documented naming convention, named in + # the "feature/fix → develop" line above — merged into develop having never + # run fmt, clippy or a single test in CI. The local pre-push QC gate was the + # only thing standing between those branches and develop. + # + # When adding a new branch-naming convention, ADD IT HERE. To verify a + # branch is actually covered, check that `gh pr checks ` lists the CI + # jobs and not just CodeQL. + branches: [develop, master, "feature/**", "features/**", "fix/**"] # Cancel a stale in-flight run when the same branch is pushed again before the # previous run finished — e.g. a stage commit immediately followed by a diff --git a/AGENTS.md b/AGENTS.md index fb3e8795..21b9cef9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,6 +1,6 @@ -# AGENTS.md — codesearch (features/remote-mount-selection) +# AGENTS.md — codesearch -_Last updated: 2026-07-29_ +_Last updated: 2026-08-05_ ## Current state @@ -10,38 +10,24 @@ _Last updated: 2026-07-29_ ## Implemented Features -- **Opt-in remote mount selection** (commit `1a5b3fc`) — a peer's individual projects are no longer auto-exposed; the user explicitly `remote mount`s the ones to use. `remote_mounts` allowlist in `repos.json` is the single source of truth for routing (`resolve_remote_project`), discoverability (`list_projects`/`scope_required`), TUI display, and `@peer` group fan-out (restricted to mounted projects only, never the whole peer). CLI: `codesearch remote available|mount|unmount|mounts`. -- **Remote project mounting (1-to-1 passthrough)** (branch `features/codesearch-federation`, merged) — each project a peer exposes is addressable locally as `project=/`, same as a local project. TUI renders mounts in italic/cyan with an info panel (peer URL + live status) and disables doctor/reindex/remove (those act on a local index a mount doesn't have). `FederationClient::search_project` forwards a single-project query directly to the peer. Cloud indexer job builds one index per vendor sub-folder sequentially (avoids holding every vendor's embedding model in memory at once — see OOM fix below). +Release narratives live in `CHANGELOG.md`; this list keeps only the load-bearing facts. + - **Federation peers** — `codesearch remote add/rm/list` (local `repos.json` peer config: `alias → url, api_key, group, into_group`) + `@peer` group references; `FederationClient` search/get_chunk fan-out with RRF. -- **Cloud indexer-job split** — heavy 4 vCPU/8 GiB build job uploads a snapshot; light 1 vCPU/2 GiB serve restores it (DOCS corpus read-only). The serve replica additionally runs a **memory-bounded incremental reindex of the small custom-kb repo** after each KB `git pull` moves `HEAD` (fire-and-forget `POST /repos/custom-kb/reindex`), so new KB articles are searchable without a redeploy; the heavy DOCS corpus stays job-only. The DOCS-read-only state is now **enforced** via a per-repo `repo_read_only` flag in `repos.json` (set by the index job's `mark_docs_readonly` step): serve's warmup opens DOCS repos read-only and returns early — no embedding on the serve replica, so 1 vCPU / 2 GiB fits comfortably; only `custom-kb` stays writable. The index job also prunes ghost vendors (vanished source) before publishing the snapshot. Cloud peer live + validated. See `integrations/cloud/README.md`. -- **Remote index management (`--remote`)** — `--remote ` flag on `index list/add/rm` + `index reindex` verb drives a peer's management API via `FederationClient` (`ManagementOutcome`: `Ok` / `HttpError{status,reason}` / `Unreachable`). Endpoints: `GET /status`, `POST /repos {path}`, `DELETE /repos/:alias`, `POST /repos/:alias/reindex[?force=]`. `--json` on List/Reindex (requires `--remote`). Without `--remote`, every `index` verb is unchanged (local). -- **Local `index rm `** — resolves the argument as a registered alias before falling back to path interpretation. -- **CLI aliases** — `ls` is a visible alias for `list` (`index`/`groups`/`remote`); `rm` for `remove` (pre-existing). -- **Protobuf (`.proto`) language support — Niveau 1** (#162, PR #175) — `.proto` files parsed with `tree-sitter-proto` and chunked along `message`/`enum`/`service`/`rpc` boundaries (Struct/Enum/Interface/Method) with preceding `//`/`/* */` comments as docstrings, instead of naive line-windowing. Symbol-level `find_impact` (Niveau 2) deferred — no `scip-protobuf` emitter exists today. -- **Standalone remote TUI (`codesearch serve tui --url ...`) now supports authenticated peers** (branch `feat/remote-tui-auth`) — previously did an unauthenticated `/health` check and had no way to pass an API key, so it 401'd against any auth-required serve (e.g. the cloud peer). Now resolves the key from `repos.json` (`remotes.*.url` match) or a new `--api-key` CLI override, reusing the existing `build_serve_client_with_key` helper (same `Authorization: Bearer` header the federation client already uses — no new auth mechanism). The authenticated client is threaded through to all TUI actions (status/info/doctor/reindex/remove/reload), with distinct error messages for "no key configured" vs. "key rejected (401)". No behavior change for local/unauthenticated serves. -- **Embedded TUI federated `/status` polling now respects scale-to-zero** (branch `fix/tui-remote-discovery-scale-to-zero`) — background polling of a mounted peer's `/status` was fixed at 30s, defeating Azure Container Apps scale-to-0 for the cloud peer (kept it perpetually warm). Now polls at the serve's own configured `idle_suspend_secs` cadence (1h on the cloud deploy) instead. Stale federated peer activity (>5min since last poll) renders as `-` in the TUI rather than a misleadingly-fresh value; a new `remote_peer_activity` tracking map in `ServeState` also fires an event-driven immediate single-peer refresh whenever the operator performs a federated search/get_chunk. Local (non-federated) repos are unaffected. -- **Embedded TUI no longer pokes federated peers on startup** (branch `fix/tui-defer-federated-poll-on-startup`) — refinement of the scale-to-zero fix above: `spawn_remote_discovery` still fired its first poll immediately on startup (poll-then-sleep), so restarting the local serve pinged every federated peer once just to fill the dashboard, waking the cloud peer for no real reason. The first discovery cycle now builds the remote-project rows from config alone (no HTTP) and ships them with an empty `refreshed_at` map, so every federated peer renders stale `-` on startup; the first real `/status` refresh comes only from either the hourly cadence tick or an activity poke (a real federated tool call). Local repos are entirely unaffected. -- **Test-suite reorg** (branch `chore/test-suite-reorg`) — extracted embedded `#[cfg(test)]` blocks out of bloated `mod.rs` files into sibling `_tests.rs` files (mcp/serve/search/cache/db_discovery); collapsed ~109 near-duplicate predicate tests into table-driven tests; centralized 1 test helper (`state_with_repo`); added 3 previously-missing coverage cases (repo_read_only force-reindex refusal, federation slow-peer→Unreachable timeout, remove_repo-during-active-build end-to-end). Test count: 710 → ~604 (fewer, more assertive tests — no coverage lost; `cargo test --lib --bins` green). -- **Flaky Windows rename fix in `atomic_write_json`** (branch `fix/flaky-force-reindex-test`) — `force_reindex_stamps_model_when_metadata_has_only_schema_version` flaked under parallel `cargo test --lib --bins` on Windows with `Access is denied (os error 5)`, a Windows AV/Search-Indexer handle race on `fs::rename(&tmp_path, path)`. Added `is_transient_rename_error()` (raw OS errors 5/32/33 — ACCESS_DENIED/SHARING_VIOLATION/LOCK_VIOLATION — plus message-hint fallback, mirroring `ServeState::is_db_locked_error`) and a bounded retry (5 attempts, 20ms backoff) around the rename for transient errors only. Validated with 6 full `cargo test --lib --bins` runs (default + `--test-threads=32`), all green (1318 passed / 42 ignored each time). Root cause could not be force-reproduced live in this session — diagnosis is by analogy to the same documented Windows AV-race pattern already fixed elsewhere in this file (`ServeState::is_db_locked_error`, FTS commit retry in `fts/tantivy_store.rs`). +- **Opt-in remote mount selection** — the `remote_mounts` allowlist in `repos.json` is the **single source of truth** for routing (`resolve_remote_project`), discoverability (`list_projects`/`scope_required`), TUI display, and `@peer` group fan-out (restricted to mounted projects, never the whole peer). Nothing a peer exposes is auto-mounted. CLI: `codesearch remote available|mount|unmount|mounts`. +- **Remote project mounting (1-to-1 passthrough)** — each mounted project is addressable locally as `project=/`; `FederationClient::search_project` forwards a single-project query straight to the peer. The TUI renders mounts in italic/cyan with a peer URL + live-status panel, and disables doctor/reindex/remove (those act on a local index a mount doesn't have). +- **Remote index management (`--remote`)** — `--remote ` on `index list/add/rm` plus an `index reindex` verb, driven through `FederationClient` (`ManagementOutcome`: `Ok` / `HttpError{status,reason}` / `Unreachable`). Endpoints: `GET /status`, `POST /repos {path}`, `DELETE /repos/:alias`, `POST /repos/:alias/reindex[?force=]`. `--json` on List/Reindex requires `--remote`. Without `--remote` every `index` verb is local and unchanged. +- **Cloud indexer-job split** — a heavy 4 vCPU/8 GiB build job uploads a snapshot; a light 1 vCPU/2 GiB serve restores it. The DOCS-read-only state is **enforced** by a per-repo `repo_read_only` flag in `repos.json` (set by the job's `mark_docs_readonly` step): serve's warmup opens those repos read-only and returns early, so no embedding happens on the replica. Only `custom-kb` stays writable and gets a memory-bounded incremental reindex (fire-and-forget `POST /repos/custom-kb/reindex`) after each KB `git pull` moves `HEAD`. The job also prunes ghost vendors before publishing. See `integrations/cloud/README.md`. +- **Language coverage** — 17 tree-sitter grammars (table in README). `find_impact` has SCIP symbol precision for **C#** (bundled `scip-csharp`) and **TypeScript** (`npx scip-typescript`, host-resolved). Protobuf is Niveau 1 (text-aware chunking on `message`/`enum`/`service`/`rpc`) only — no `scip-protobuf` emitter exists today. +- **Scale-to-zero-safe federation: a federated peer is NEVER polled on a timer** — ⚠️ **design constraint, do not "improve" this.** Background polling of *local* repos is fine; a *federated* peer must never be contacted on any cadence. The embedded TUI's discovery tick is **config-only** (`REMOTE_ROW_REFRESH_SECS` = 5s, zero HTTP): it rebuilds mounted-remote rows from the `remote_mounts` allowlist so mount/unmount edits and `l` reloads surface, and contacts nobody. A peer is contacted only by (a) an **activity poke** — a real federated tool call just hit it, detected via `remote_peer_activity` in `ServeState`, refreshing that one peer, never a fan-out — or (b) the explicit `i` info-overlay keypress. Idle mounts therefore render activity as `-`, which is the correct steady state, not a fault. **Rejected reasoning (was shipped twice, PR #181/#184, and reverted):** "polling no faster than the host's idle-suspend term is harmless." It is not — each poll *woke* the peer's scale-to-zero replica, which then self-warmed for its own full idle window (~1h), giving ~50% duty cycle on a peer nobody queried (measured: wakes 120/121/120 min apart, zero searches). Not keeping a peer awake past its suspend term is strictly weaker than not waking it, and the two windows are unrelated values anyway (local host vs. remote peer). +- **Standalone remote TUI auth** — `codesearch serve tui --url ...` resolves the API key from `repos.json` (`remotes.*.url` match) or a `--api-key` override and threads the authenticated client through every TUI action, with distinct errors for "no key configured" vs. "key rejected (401)". +- **Keep-warm ping observability + spurious-wake fix** *(branch `fix/federated-silent-poll-diagnosis`)* — the `keep_warm_url` self-ping loop logs every ping (`debug!` on success, `warn!` on failure) instead of discarding both outcomes, and warns at startup when the target host isn't this server's own bind host — **except on a wildcard bind** (`0.0.0.0` / `::`), where our externally-visible host is unknown so the comparison proves nothing; without that carve-out the warning fired on every cold start of the *only* deployment where keep-warm is correct (Azure binds `0.0.0.0`, target is the ingress FQDN), which just trains operators to ignore it. Rule lives in the testable `keep_warm_foreign_target` helper. Keep-warm also **requires a real recorded tool call**: the old `most_recent_tool_call().unwrap_or(start)` fallback meant any wake that wasn't a tool call (`/status` and `/healthz` don't call `record_tool_call`) made the replica self-warm for its whole idle window — reachable *only* when the wake wasn't real work, so its sole practical effect was rewarding spurious wakes (~11× amplification). Full diagnosis, with Azure Log Analytics ground truth: `DIAGNOSE_FEDERATED_KEEP_WARM.md`. +- **CLI aliases** — `ls` for `list` (`index`/`groups`/`remote`), `rm` for `remove`. `index rm ` resolves a registered alias before falling back to path interpretation. > ℹ️ **Remote write verbs** (`add`, `reindex --force`) require a read-write peer; the cloud peer rejects them (`--force` → HTTP 500 "could only be opened read-only; cannot force-reindex"). An **incremental** `reindex` (no `--force`) of an already-registered repo *does* succeed on the cloud peer — that is the custom-kb auto-refresh path. `list` is always safe. `rm` is not durable — the next cold start re-registers from the restored snapshot. ## Open TODOs -Single source of truth for outstanding codesearch work. Items marked 🔒 live in a separate worktree — **do not touch on this branch**. - -### Code — small, ready to pick up - -- [x] **T1: Remove dead `wait_until_indexed()`** in `docker/entrypoint.sh` — superseded by `wait_active_build_done()`. Confirmed no callers anywhere in the repo (only 3 comment references). Deleted the function + updated the comments. -- [x] **T2: Extract shared `build_remote_search_body(request, mode, limit_value)`** in `src/mcp/mod.rs` — group fan-out (`federated_search`) and single-project fan-out (`federated_project_search`) duplicated the same `serde_json` body (differing only in the limit value); extracted to one shared builder. -- [x] **T3: Persist remote-project discovery** to `remote_project_cache` in `repos.json` — the field already existed but was never read/written. Wired `ReposConfig::cache_remote_projects()`/`cached_remote_project_aliases()`; both `codesearch remote available ` and `codesearch index list --remote ` now write-through-cache a peer's alias list on success and fall back to the last-known list (instead of hard-failing) when the peer is unreachable. `reconcile()` prunes cache entries for peers that no longer exist. Shared the mounted/cached row printing into `print_remote_project_row()` to keep the two CLI commands in sync. -- [x] ~~**T4: 0-chunk status bug**~~ — **closed as can't-reproduce.** Static trace of the full call-graph found no concrete defect (fresh LMDB read-txn per `stats()`, no `Arc` swap, no stale handle); the `total_chunks==0 → "building"` inference at `src/mcp/mod.rs:7557`/`:7618` only fires in the genuine 0-chunk window or an unconfirmed narrow cold-start/concurrent-reload race — not reproducible, not biting in steady state. Re-file with a deterministic live repro if the symptom recurs. -- [x] ~~TUI `i`/`d`/`f` diagnostics~~ — investigated, this was a stale reference in the TODO title, not a code bug. Actual TUI keybindings (`src/serve/tui_common.rs`: `handle_key` + `render_footer`) are `i` (info), `d` (doctor), `n` (reindex), `r` (remove), `l` (reload), `q` (quit) — footer hints match the handler exactly. No `f` binding exists or ever existed in the codebase; the title's "f" doesn't correspond to anything real. - -### Code — 🔒 separate worktrees (resolved) - -- [x] ~~🔒 **find_impact routing diagnose/fix**~~ — **resolved via PR #163** (merged 2026-07-27, Option D = nudges/reframe: recommend find_impact first; stop deflecting to `find kind=usages`; align rustdoc; auto-detect TS SCIP extensions). Diagnosis doc kept in repo root as `DIAGNOSE_FIND_IMPACT_ROUTING.md`. -- [x] ~~🔒 **TypeScript SCIP indexing**~~ — **resolved via PR #167** (merge `98a1979`, 2026-07-28). SCIP protobuf parsing, `TypeScriptSymbolIndexer` + registry wiring, file-watcher TS tracking, tests+fixture+smoke, TUI indicator, Windows `npx` fix. Plan doc kept as `PLAN_TYPESCRIPT_SCIP.md`. Follow-up SCIP-adapter dedup tracked as T5. +Single source of truth for outstanding codesearch work. ### Cloud / infra — needs decision before pickup @@ -61,15 +47,6 @@ Single source of truth for outstanding codesearch work. Items marked 🔒 live i **Still open:** retire `codesearch-indexer` entirely or keep for DR; scheduled script vs Logic App vs wrapper CLI command (`codesearch cloud rebuild --remote `?). -### GitHub issues - -- [x] **#162: include protobuf as a language aware** — Niveau 1 (text-aware `tree-sitter-proto` chunking on `message`/`enum`/`service`/`rpc` boundaries) shipped in PR #175. Niveau 2 (SCIP symbols → `find_impact`/call-graph) deferred pending a `.proto`-heavy repo — no `scip-protobuf` emitter exists today. -- [x] **#161: missing macOS binary in v1.1.31** — fixed: C1/C3/C4 (APFS disk-pressure retry: stage binary out of `target/` + `cargo clean` + tar/cp retry loops with `df -h` diagnostics) merged via #166; PR #173 pinned the `actions/checkout` `ref:` so `workflow_dispatch` builds the tagged commit (related mismatch class). GitHub issue #161 closed 2026-07-29. - -### Defensive / low priority - -- [x] **D1: Apply same cp-retry pattern to Linux `with-csharp` step** in `release.yml` — the "Package with-csharp (Linux)" step now retries the binary `cp` up to 3x with `df -h` diagnostics on failure and a hard `test -f` check, mirroring the macOS step's C3 pattern. Preventive consistency only (Linux runner has 84GB disk + ext4, no `fcopyfile` EIO failure mode) — no observed Linux failure, just aligning both platforms' failure behavior. - ### Historical context (for C1/C2 above) **Fixed — incremental-refresh OOM crash-loop (2026-07-04):** `IndexManager::perform_incremental_refresh_with_stores` (`src/index/manager.rs`) used to chunk + embed the ENTIRE changed-file delta in one unbounded in-memory `Vec` before writing anything to the stores. A normal incremental delta was harmless; a vendor sync dropping thousands of files at once OOM'd the 1 vCPU/2 GiB `codesearch-serve` container, which then crash-looped. Fixed by batching: `changed_files.chunks(batch_size)` processed sequentially (chunk+embed+insert+commit per batch, single `build_index()` at the end), bounding peak memory to O(batch) regardless of delta size. Batch size defaults to `INCREMENTAL_REFRESH_BATCH_SIZE = 200` (`src/constants.rs`), override via `CODESEARCH_INCREMENTAL_BATCH_SIZE`. No test for the multi-batch path itself (existing `manager.rs` tests avoid real embedding, same reasoning as the gated `csharp_helper_integration` test) — verify end-to-end on a real large corpus if in doubt. @@ -101,7 +78,9 @@ Common mistake: a subagent runs `/git pr create` with no explicit `--base`, the - **Runtime:** `C:\Users\develterf\.local\bin\` — `codesearch.exe` + `helpers/csharp/scip-csharp.exe` - **Build:** `target/release/` — outside repo (via `CARGO_TARGET_DIR`). `build.ps1` self-heals `core.bare=false` before invoking cargo — this checkout is a bare+working-tree hybrid whose `core.bare` intermittently resets to `true` (VS Code's git integration rewrites `.git/config` on ref changes), which makes cargo abort with `did not expect repo to be bare`. No need to flip it manually before building; `build.ps1` does it. - **Deploy:** `..\copy-to-common.ps1` — builds + copies both binaries to `~/.local/bin/`. A running `codesearch.exe` is file-locked on Windows; stop serve before deploying. +- **Tests live in sibling `_tests.rs` files**, not in embedded `#[cfg(test)]` blocks inside `mod.rs` (mcp/serve/search/cache/db_discovery follow this). Prefer table-driven tests over near-duplicate per-case fns. - **Canonical paths:** NEVER call `.canonicalize()` directly. Always use `safe_canonicalize()`. +- **Windows transient file errors:** `fs::rename` (and friends) can fail with raw OS errors 5/32/33 (ACCESS_DENIED/SHARING_VIOLATION/LOCK_VIOLATION) purely from an AV/Search-Indexer handle race — not a real conflict. Classify with `is_transient_rename_error()` / `ServeState::is_db_locked_error` and wrap in a bounded retry (see `atomic_write_json`, the FTS commit retry in `fts/tantivy_store.rs`). Never retry non-transient errors. - **LMDB rule:** No two `EnvOpenOptions::open()` on same dir in same process. All access via `get_or_open_stores()` → `Arc`. - **LMDB rule — commit, never drop, a txn whose DB handle you keep:** any `open_database` / `create_database` whose handle outlives the opening transaction MUST end that transaction with `commit()`. `drop()` aborts, and LMDB closes handles opened in an aborted transaction. Storing a DBI from a dropped `RoTxn` yields a bare `EINVAL (os error 22)` on first use, with no other symptom. This shipped in `open_readonly` from the initial commit and only surfaced once read-only became a permanent mode, diagnosed and fixed in commit `8f62482`. - **LMDB rule — open every env with `BASE_ENV_FLAGS`** (`src/lmdb_registry.rs`). heed refuses to reopen one path with different options, so a partial rollout turns a working reopen into an intermittent failure. diff --git a/CHANGELOG.md b/CHANGELOG.md index f418464a..8aa0a377 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ finalized in place with a date — no renaming/migration step needed. ### Fixed +- **A federated peer is now never polled on a timer — the two 1.2.0 "scale-to-zero" fixes did not actually stop the cloud peer being woken.** 1.2.0 replaced the TUI's hardcoded 30s peer poll with the local serve's own `idle_suspend_secs` cadence and suppressed the startup poke, on the theory that polling no faster than the host's suspend term is harmless. It is not, and the release notes above overstated the fix. Measured on the deployed Azure Container Apps peer over a period with **zero** federated searches: wakes exactly **120/121/120 minutes** apart, each warm period **~67 min** — roughly a 50% duty cycle on an index nobody queried, each wake additionally paying an `azcopy sync` of the docs blob and a KB `git pull`. Two independent defects combined. **(1) The trigger:** the poll *itself* was the ingress traffic that woke the replica. Not keeping a peer awake *past* its suspend term is strictly weaker than not *waking* it, and the two windows were unrelated values anyway — the cadence read the **local** host's 2h default, not the peer's ~1h (which is why the 120-minute spacing, not 60, is the tell). **(2) The amplifier:** the cloud keep-warm loop fell back to the process start time when no tool call was recorded (`most_recent_tool_call().unwrap_or(start)`), and since `/status` and `/healthz` never call `record_tool_call`, any non-tool-call wake made the replica self-ping every 120s for its whole idle window — ~11× amplification. That fallback was unreachable in the case it was written for: a real tool call always records itself, so it could only ever fire when the wake was *not* real work. Now: the TUI's discovery tick is **config-only** (5s, zero HTTP) and merely rebuilds mounted-remote rows from the `remote_mounts` allowlist so mount/unmount edits and `l` reloads still surface; a peer is contacted only by an **activity poke** (a real federated search/get_chunk just hit that peer, so it is demonstrably already awake — single-peer, never a fan-out) or the explicit `i` info-overlay keypress. Keep-warm requires a real recorded tool call and otherwise lets the host suspend the replica. A peer staying warm for an hour *after real use* is correct and unchanged. Idle mounts render activity as `-`, now the normal steady state rather than a fault. Also fixed: removing the *last* peer from `repos.json` left its rows on screen forever (the snapshot was gated on a non-empty peer list), and the 1.2.0 "keep-warm target isn't self" warning false-positived on the only deployment where keep-warm is correct (the process binds `0.0.0.0` while the target is the ingress FQDN — a wildcard bind means the external host is unknown, so the check now stays silent). Background polling of **local** repos is unchanged and unaffected: the local/federated split is a deliberate design constraint. - **`MDB_MAP_FULL` fatal crash on large corpora — LMDB mapsize cap raised + persistent embedding cache now auto-resizes too (#189).** Indexing a large corpus (e.g. a 1GB / 53k-file cargo-registry source producing >1.2M chunks) could crash with `MDB_MAP_FULL: Environment mapsize limit reached` once the vector store's auto-resize (already in place since an earlier fix) hit its old 8GB hard cap. Two changes: (1) the cap is raised to 16GB by default, and made runtime-overridable via `CODESEARCH_MAX_LMDB_MAP_SIZE_MB` (clamped to at least 1GB) for corpora that legitimately need more; (2) the **persistent embedding cache** (`~/.codesearch/embedding_cache//`) previously had no resize logic at all — it hit the same `MDB_MAP_FULL` on a hardcoded 512MB cap and silently degraded to a WARN-and-continue path, turning every subsequent embedding into a full ONNX-inference cache miss. It now retries with the same doubling-resize pattern as the vector store (up to 3 attempts, capped at the same runtime limit), persisting the grown size to `metadata.json` so a restart reopens at the correct size. When either store's cap is genuinely exhausted, the error/warning message now names the env var that raises it, instead of just reporting the size. - **`build.ps1` now self-heals `core.bare=false` before invoking cargo.** This repo lives at `codesearch.git` as a bare+working-tree hybrid — a full checked-out source tree + `.git/index`, but `core.bare=true` in `.git/config`. `core.bare` intermittently resets to `true` (VS Code's git integration rewrites `.git/config` on ref changes; smoking gun: `github-pr-owner-number` duplicated 7× for `develop`), and when it does, cargo's source fingerprinting aborts every build with `did not expect repo ...\.git to be bare`, breaking `copy-to-common.ps1` → `build.ps1` → `cargo build`. `build.ps1` now forces `core.bare=false` right after `Set-Location`, before any cargo invocation. Idempotent and harmless for a normal (truly non-bare) checkout; non-fatal if git is unreachable. @@ -41,68 +42,21 @@ finalized in place with a date — no renaming/migration step needed. - **Cloud serve OOM crash-loop + read-only search regression (#177).** The federation peer's heavy DOCS corpus couldn't run inside a 1 vCPU / 2 GiB serve replica: write-mode warmup of six vendor repos peaked at 1.94 GiB and crashed (exit 137). Fixed with a per-repo `repo_read_only` flag — the indexer job builds write-mode then marks DOCS read-only before snapshotting; serve restores read-only and skips warmup entirely (0.1 GiB steady-state). Also fixes a latent LMDB bug this exposed: `open_readonly` opened DB handles inside a transaction it then `drop()`ped instead of `commit()`ted, so LMDB closed them and every read-only store returned a bare `EINVAL (os error 22)` on first use — shipped since the initial commit, only visible once read-only became a permanent code path. Ghost-vendor (vanished source) and dead-vendor (empty index) pruning so one bad vendor can't veto a snapshot publish. Structurally closes the "a store that fails mid-request renders as an ordinary empty/short result" defect class via `respond_with_items()` / `respond_with_object()` (the warnings channel is a required parameter, not an optional field), `qualify_empty_result()`, and a `#[must_use]` `MultiReadOutcome`; and enforces caller-facing literal line-continuation correctness via `tests/caller_facing_literals.rs`. - **Index cancellation was a no-op for freshly-added repos; `remove_repo` reported "DB deleted" while the task kept writing (#178).** Diagnosed from a runaway `codesearch serve` (6 GB RSS, 40-52% CPU, machine unresponsive): `remove_repo`'s `CancellationToken` was never passed into the spawned task and the `JoinHandle` was never registered, so `cancel()` fired into the void and the DB dir was deleted under a still-writing task (Windows sharing violation → swallowed `warn!`). The token is now threaded through `force_reindex` / incremental refresh and checked inside the per-batch embed loop; `add_repo_handler` registers the handle so `remove_repo` actually cancels + awaits it; an early-bail guard prevents a removed alias being resurrected by its own in-flight task; and `remove_repo` now reports the DB-delete result honestly (`db_deleted: true|false` + reason). Test cache isolation also fixed — tests no longer write into the real `~/.codesearch/embedding_cache/`. - **Orphaned `.codesearch.db` dirs left behind by cancelled in-build index tasks (#179).** The await-shutdown from #178 dropped the `JoinHandle` on its timeout — in Tokio this only **detaches** a task, it doesn't cancel it, and a task parked inside the synchronous arroy `build_index` (on a `spawn_blocking` thread) has no cancellation point. So the detached task held its LMDB handle open and the `.codesearch.db` dir stayed undeletable after removal. Added a self-cleanup backstop: the detached uninterruptible-build task drops its LMDB handle (closing the env synchronously) and deletes the orphaned dir right after releasing it — wired into all six build paths (add / reindex-force / TUI reindex post-build, FSW-refresh, primary FSW warmup, incremental-reindex). The delete is deadline-bounded (60s) and retries only on lock-class errors; already-gone is treated as success. -- **Embedded serve TUI polled a federated peer's `/status` every 30s regardless of its scale-to-zero configuration.** This defeated Azure Container Apps scale-to-0 for the cloud peer, since the background polling itself was enough ingress traffic to keep the replica perpetually warm. The TUI now polls a mounted peer at the serve's own configured `idle_suspend_secs` cadence (1h on the cloud deploy) instead of a hardcoded interval. Federated peer activity in the TUI now renders as `-` when stale (>5min since the last successful poll) rather than showing a misleadingly-fresh value, and a new `remote_peer_activity` map in `ServeState` triggers an immediate, event-driven refresh of the specific peer whenever the operator performs a federated search/get_chunk — so activity is never more stale than the operator's own last interaction. Local (non-federated) repos are entirely unaffected. -- **Embedded serve TUI poked each federated peer's `/status` once on startup.** The scale-to-zero cadence fix above still left the discovery task firing its first poll immediately on startup (poll-then-sleep), so simply restarting the local serve pinged every federated peer once just to fill the dashboard — waking the cloud peer for no real reason. The first discovery cycle now builds the remote-project rows from config alone (no HTTP) and ships them with an empty refresh-time map, so every federated peer renders as stale `-` immediately; the first real `/status` refresh comes only from either the hourly cadence tick or an activity poke (a real federated tool call). Local repos are entirely unaffected. +- **Embedded serve TUI polled a federated peer's `/status` every 30s regardless of its scale-to-zero configuration.** This defeated Azure Container Apps scale-to-0 for the cloud peer, since the background polling itself was enough ingress traffic to keep the replica perpetually warm. The TUI now polls a mounted peer at the serve's own configured `idle_suspend_secs` cadence (1h on the cloud deploy) instead of a hardcoded interval. Federated peer activity in the TUI now renders as `-` when stale (>5min since the last successful poll) rather than showing a misleadingly-fresh value, and a new `remote_peer_activity` map in `ServeState` triggers an immediate, event-driven refresh of the specific peer whenever the operator performs a federated search/get_chunk — so activity is never more stale than the operator's own last interaction. Local (non-federated) repos are entirely unaffected. *(Superseded in 1.2.4 — this cadence still woke the peer; see below.)* +- **Embedded serve TUI poked each federated peer's `/status` once on startup.** The scale-to-zero cadence fix above still left the discovery task firing its first poll immediately on startup (poll-then-sleep), so simply restarting the local serve pinged every federated peer once just to fill the dashboard — waking the cloud peer for no real reason. The first discovery cycle now builds the remote-project rows from config alone (no HTTP) and ships them with an empty refresh-time map, so every federated peer renders as stale `-` immediately; the first real `/status` refresh comes only from either the hourly cadence tick or an activity poke (a real federated tool call). Local repos are entirely unaffected. *(Superseded in 1.2.4 — see below.)* - **Watcher-triggered reindexes were invisible in the serve TUI, and branch switches never rebuilt symbols.** Three related gaps in the `codesearch serve` file watcher: (1) the ordinary text-batch reindex (the most common watcher activity) never signalled the TUI, so editing a file showed nothing in the status column even though the index updated — despite the callback's own doc claiming it fired on "batch flushes"; (2) a C# symbol rebuild toggled only the general repo-state label, never the C#-specific indicator, so that column never showed "Indexing" during the (30–90s) rebuild; (3) a git **branch switch** refreshed only the text index and discarded the buffered `.cs`/`.ts` events without rebuilding symbols, leaving `find_impact` serving references from the previous branch until the next incidental `.cs` edit or a serve restart. Now: the text-batch flush toggles the TUI "Indexing" label; the C# notifier is a 3-state signal (`Started`/`Succeeded`/`Failed`) so the C# indicator shows "Indexing" for the rebuild duration; and a branch switch triggers a full C#/TypeScript symbol rebuild. Watcher symbol-rebuild log lines now carry the repo label for multi-repo attribution. - **`model: unknown` on indexes created via the serve / git-hook path (git worktrees especially).** When a repo was registered through `POST /repos` (the git-hook flow), the vector store was opened first and `ensure_schema_version` pre-created a `metadata.json` containing only `schema_version` — no model fields. The force-reindex path then saw the file already existed and skipped stamping the default model, so the index was left with no `model_short_name`. Every reader reported `model: unknown`, and that sentinel disabled the empty-index live-chunk-count self-heal, making a perfectly good worktree index look empty so agents fell back to grep. The serve/git-hook and incremental-refresh paths now always stamp the resolved model. As part of the fix, the model→metadata stamp (`model_short_name`/`model_name`/`dimensions`) is consolidated into a single `ModelType::write_metadata_fields` source of truth across all five index-creation sites — which also corrects a pre-existing drift where the auto-create-DB path wrote the Debug variant name (e.g. `AllMiniLML6V2Q`) as `model_name` instead of the real model name. Existing worktree indexes need one reindex to pick up the stamped model. - **Flaky `force_reindex_stamps_model_when_metadata_has_only_schema_version` test on Windows under parallel `cargo test`.** `atomic_write_json`'s `fs::rename(&tmp_path, path)` could race a Windows AV/Search-Indexer handle hold on the destination file, failing with `Access is denied (os error 5)` under parallel test execution. Added `is_transient_rename_error()` (classifies raw OS errors 5/32/33 — ACCESS_DENIED/SHARING_VIOLATION/LOCK_VIOLATION — plus a message-hint fallback, mirroring the existing `ServeState::is_db_locked_error` pattern) and wrapped the rename in a bounded retry (up to 5 attempts, 20ms backoff) for transient errors only. Validated with `cargo test --lib --bins` across 6 runs (default and `--test-threads=32`), all green. - **claude-code grep-guard hook leaked `grep` on every low-confidence codesearch result.** The hook blocked the first `Grep` on an indexed repo path but auto-unblocked the *same* query when retried within 5 minutes — intended as the "codesearch found nothing, fall back to grep" path. But a low-confidence or empty codesearch result is a *successful* call meaning "reformulate the query", not a dead server, so the retry-cache let `grep` through whenever a query merely scored below the relevance floor (e.g. punctuation-heavy or alternation patterns). Replaced the retry-cache with an active liveness probe: the hook now GETs the serve hub's unauthenticated `/healthz` endpoint (base URL from `CODESEARCH_SERVER`, else `127.0.0.1:$CODESEARCH_SERVE_PORT`, else the compiled default `:39725`) and keeps `grep` blocked whenever the server answers, allowing it only when the probe fails — i.e. codesearch is genuinely down. Both the PowerShell and bash hooks are updated (the bash hook now also requires `curl`), and the deny message steers to `find`/`explore`/single-clean-term reformulation instead of promising an auto-unblock. ## [1.1.31] - 2026-07-23 - -**Security hardening sweep (Aikido) + community bug/dependency fixes.** - -### Added - -- **EmbeddingGemma retrieval support (#155, original work by @markschroedr, superseding #147).** Adds support for Google's EmbeddingGemma embedding model as an additional embedder option, alongside model-selection hardening and improved error messages for unsupported/misconfigured embedding models. -- **`CODESEARCH_ALLOWED_HOSTS` / `CODESEARCH_DISABLE_HOST_VALIDATION` (#149, reported by @stdweird).** rmcp's DNS-rebinding defence defaults the MCP transport's `Host`-header allowlist to loopback-only, rejecting container/service hostnames in containerised deployments. `CODESEARCH_ALLOWED_HOSTS` lets you extend the allowlist with a comma-separated hostname list; `CODESEARCH_DISABLE_HOST_VALIDATION=1` disables the check entirely (only safe behind a reverse proxy). See README `## Security`. -- **`raise_fd_limit()` at serve startup (#150, contributed by @tony-nexartis).** `codesearch serve`'s fd demand scales with registered repo count; under process supervisors with a low default `ulimit -n` (notably macOS launchd, 256), this could silently exhaust file descriptors and wedge `accept()` with `EMFILE` while the daemon still looked healthy. Serve now raises its own soft `RLIMIT_NOFILE` to the hard limit at startup (Unix only) and warns if the effective limit still looks insufficient for the repo count. -- **`persist-credentials: false`** added to every `actions/checkout` step across all GitHub Actions workflows, and the CodeQL workflow's floating `actions/checkout@v4` pinned to the same SHA already used elsewhere — reduces the blast radius of a compromised CI step and closes a supply-chain drift gap. -- **CodeQL skipped on fork PRs.** Fork-originated PRs carry a restricted `GITHUB_TOKEN` that cannot upload SARIF results to the upstream repo, which was failing the CodeQL check on every external contribution (e.g. #150) with a confusing "Resource not accessible by integration" error unrelated to the PR's actual code. The analyze job is now skipped for fork PRs (still runs on `develop`/`master` push, same-repo PRs, and the schedule). - -### Fixed - -- **Panic on multi-byte UTF-8 boundary in search snippets (#148, reported by @tony-nexartis).** Search-result snippet truncation byte-sliced content at a fixed offset, panicking whenever that offset landed inside a multi-byte character (box-drawing glyphs, CJK, emoji). Now truncates on a char boundary. -- **Path-traversal hardening (critical).** `codesearch index`'s project-path resolution no longer silently falls back to the raw, unvalidated path when canonicalization fails — it now fails fast with an actionable error. The `.NET` symbol-helper CLI (`scip-csharp`) now canonicalizes every path argument (`--solution`, `--project`, `--output`, `--symbols-file`) before use, closing several path-traversal vectors flagged by Aikido SAST. -- **Registering a `.git`/build-artifact directory as a project root.** `codesearch index`/repo registration now rejects a root whose own directory name matches an always-excluded name (`.git`, `.svn`, `node_modules`, etc.), preventing accidental indexing and search-exposure of internal VCS metadata. -- **ANSI/control-sequence injection in terminal output.** Search results and sync/reindex logs now strip ANSI escape sequences (CSI, OSC, Fe) and stray control characters from indexed file content before printing, so a maliciously crafted file can no longer manipulate the user's terminal (clear screen, hide output, rewrite the title bar, etc.). -- **Unix path-cache key collision.** The path-normalization cache used for file metadata unconditionally converted `\` to `/`, which on Unix (where `\` is a legal filename character, not a separator) could collapse a literal-backslash filename with an unrelated subdirectory path into the same cache key. The conversion is now gated to Windows only. -- **Dependency CVE remediation.** `rmcp` floor bumped `1.5.0 → 1.8.0` (3 CVEs fixed); ~100 transitive dependencies refreshed via `cargo update`, including security-relevant bumps to `quinn-proto`, `h2`, `hyper`, `tokio`, `rustls`, `openssl`, `zerocopy`, `zeroize`, `webpki-roots`, `aws-lc-rs`. +- Security hardening sweep (Aikido): path-traversal fixes in `index` + `scip-csharp`, ANSI/control-sequence injection stripped from indexed content, `.git`/`node_modules` rejected as project roots, Unix path-cache key collision; `rmcp` 1.5.0 → 1.8.0 (3 CVEs) plus ~100 transitive dependency bumps. Also added EmbeddingGemma retrieval support (#155), `CODESEARCH_ALLOWED_HOSTS` / `CODESEARCH_DISABLE_HOST_VALIDATION` (#149), and `raise_fd_limit()` at serve startup (#150); fixed a multi-byte UTF-8 panic in search snippets (#148). ## [1.1.30] - 2026-07-10 - -### Added - -- **User-configurable extension→language map (#138).** A new optional `~/.codesearch/extensions.json` (or the path in `$CODESEARCH_EXTENSION_MAP`) maps a file extension to a language name, e.g. `{ "inc": "php", "h": "cpp" }`. Files with an unrecognised extension are `Unknown` and skipped **entirely** during indexing (there is no line-based fallback for `Unknown`), so a codebase using a non-standard convention — the reported case is legacy PHP in `*.class.inc` files — was previously invisible to codesearch. The map lets users opt in per codebase; entries take precedence over the built-in extension table (so a known extension can be remapped too). Kept **generic on purpose**: `.inc` is not hardcoded to PHP because it's language-agnostic (assembly, SQL, C/PHP includes). Missing/malformed maps and unknown language names are logged and ignored, never fatal. +- Added a user-configurable extension→language map (#138) at `~/.codesearch/extensions.json` (or `$CODESEARCH_EXTENSION_MAP`), letting a codebase opt in a non-standard extension (the reported case: legacy PHP in `*.inc`); user entries take precedence over the built-in extension table. ## [1.1.29] - 2026-07-10 - -**Project-level federation + cloud reindex hardening.** Builds on the 1.1.0 federation release: a peer's individual projects can now be **opt-in mounted** and queried by name, the serve TUI surfaces and inspects those mounts, and the cloud indexer was reworked to reindex reliably without OOM-killing itself. - -### Added - -- **Opt-in mounting of individual remote projects.** After adding a peer, the local user **explicitly picks** which of its individual projects to use, via a new `remote_mounts` allowlist in `repos.json` — nothing is auto-exposed. A mounted project is queried locally by name as `project=/` (e.g. `cloud/akeneo`), a 1-to-1 passthrough routed directly to that peer; a **non-mounted** project is unroutable even if the peer exposes it. The allowlist is the single source of truth for routing, discoverability, TUI display, and group fan-out. -- **`codesearch remote available|mount|unmount|mounts`.** Inspect the individual projects a peer exposes (marking which are mounted), then opt in/out. `remote available ` queries the peer's `GET /status`; `mount`/`unmount` edit the allowlist; `mounts` lists the current selection (and any local rename). -- **Mounts are discoverable.** `list_projects` gains a `remote_projects` array (name + peer + peer URL), and the `scope_required` error advertises mounted names in `available_projects`, so an agent can find and route to a mounted project as a first-class `project=` target. -- **Group fan-out restricted to mounts.** A whole-peer `@peer` group reference (e.g. `docs → [@cloud]`) now federates only the individual indexes you mounted for that peer — each queried as its own project — instead of the peer's entire corpus. -- **TUI: mounted remote projects.** Mounts render in **italic/cyan** in the serve status table to signal they live on a peer (not a local index). The `i` (info) key now works on a mount, opening a **Remote Mount** panel showing the peer URL and the peer-reported live status (status / lock / changes / calls / last call). The panel also fetches the peer's on-disk index stats (**chunks / files / db size / model**) on demand from `GET /repos/{alias}/info`, giving remote mounts parity with the local Info overlay — with a loading placeholder while the fetch is in flight and a graceful "stats unavailable from peer" fallback if the peer can't answer. When a mount is selected, the footer renders the local-index actions **doctor / reindex / remove struck-through (disabled)** so it's clear those don't apply to a peer-hosted index; info / reload / quit / navigation stay enabled. -- **KB near-instant propagation.** The custom-KB project now polls its remote `git` HEAD on a cheap `git ls-remote` interval (`KB_POLL_INTERVAL_SECS`) instead of waiting for the full reindex cadence, so a KB add/update/delete becomes visible to federated queries within seconds of the git push rather than up to ~15 minutes later. - -### Changed - -- **Remote mount selection is opt-in.** Replaced the earlier auto-discover-everything / opt-out `remote_hidden` filter with the explicit `remote_mounts` allowlist. Live peer discovery now only **enriches** TUI status; it no longer defines which projects are mounted (mounts resolve from config even while a peer is unreachable). -- **Cloud indexer job: one federated project per vendor.** The cloud indexer now builds each vendor as a separate federated project (`akeneo`, `vendor-a`, `bynder`, `digizuite`, `inriver`, `keyshot`, plus the custom KB) rather than one monolithic index, and builds them **sequentially** so the serve replica only ever holds one embedding model in memory at a time. -- **Cloud deployment docs** generalised for public release (customer identifiers scrubbed) and consolidated under `integrations/cloud/`. -- **Docker image** now built locally with **BuildKit** (`docker buildx --push`) instead of `az acr build`: the model-cache warmup is folded into the builder stage and shipped as a single tarball, working around ACR's classic builder failing to `COPY --from` a chained stage / symlink tree. - -### Fixed - -- **`codesearch hooks git install` now works from worktrees, honours `core.hooksPath`, and chains into existing hooks.** The generated `post-checkout` hook registered the checked-out worktree with `codesearch serve` using `$(pwd)`, which on Git Bash is an msys path (`/c/…`) that serve rejects with HTTP 400 ("cannot canonicalize") — so worktree auto-registration silently no-op'd on Windows. The hook now sends `$(pwd -W 2>/dev/null || pwd)` (native `C:/…` on Git Bash, plain `pwd` elsewhere). Install-time fixes: the hooks directory is resolved via `git rev-parse --git-path hooks` so it (a) writes to the shared **common-dir** hooks when run inside a linked worktree — git never runs a per-worktree gitdir hook, so the old behaviour installed a hook that never fired — and (b) honours a `core.hooksPath` override. Instead of refusing when a foreign `post-checkout` already exists, install now **chains** a delimited codesearch block into it (inserted before any trailing `exit 0`) and upgrades that block in place on re-run, so it is idempotent. The managed block is POSIX `sh` (valid when chained into a `#!/bin/sh` hook) and JSON-escapes the path. -- **Indexer job OOM-kill on reindex.** The container entrypoint submitted all vendor index builds at once (async HTTP 202), so the serve process held every vendor's embedding model + working set simultaneously and got OOM-killed on 8 GiB — leaving the job stuck "indexing" forever. Builds now run sequentially, waiting for each to settle before starting the next. -- **Incremental-refresh OOM crash-loop.** Bounded incremental-refresh embedding batches so a large change set no longer exhausts the heap. -- **claude-code grep-guard hook** now ignores an already-running codesearch process and requires a local index before nudging toward codesearch, so it stops blocking `grep` when codesearch can't actually serve the current repo. -- **`filter_path` on federated/mounted projects returned zero results.** `search(project="/", filter_path=...)` (and `@peer` group fan-out) forwarded `filter_path` to the peer, which matched it against its own **un-namespaced** store paths (and, in serve mode, against the wrong project root) — so it dropped every hit regardless of the value passed, while the caller only ever sees the `//…` **namespaced** path. `filter_path` is now applied **client-side** on the namespaced result paths for both the project-passthrough and group fan-out paths (the hub over-fetches from the peer and post-filters), so a federated `filter_path` matches exactly what the caller reads back. Consumers no longer need the over-fetch+post-filter workaround. -- **`filter_path` on a serve-routed local project returned zero results.** For a `search(project="")` (or local group) served by `codesearch serve`, `build_semantic_response` relativised result paths against the **service's own `project_path`** rather than the **routed project's root**, so the absolute stored path never stripped and every hit was filtered out. The filter now resolves the correct root per result (routed alias's root; the longest matching alias root for multi/group; the service path only as the stdio fallback), so `filter_path` behaves as a **repo-relative** prefix in every routing mode. stdio single-repo behaviour is unchanged. +- **Project-level federation + cloud reindex hardening.** Opt-in `remote_mounts` allowlist with `codesearch remote available|mount|unmount|mounts`; a mounted project is addressable as `project=/` and `@peer` group fan-out is restricted to mounts; TUI renders mounts with a Remote Mount info panel. Cloud indexer rebuilt as one sequential federated project per vendor (fixes the OOM-kill on reindex), image now built with BuildKit. Fixed `hooks git install` from worktrees (`core.hooksPath`, hook chaining, msys path) and `filter_path` returning zero results on federated/mounted *and* serve-routed local projects. ## [1.1.0] - 2026-07-01 - **Federation release.** Remote peer search fan-out (`search`/`get_chunk` over TLS, RRF-merged, never hard-fails), `--remote ` index management (`list/add/rm/reindex`), split cloud indexer/serve topology, README `## Security` section; fixed `active_sessions` overflow to `u64::MAX`, `index rm ` OS-path fallback bug, added `ls` alias. diff --git a/DIAGNOSE_FEDERATED_KEEP_WARM.md b/DIAGNOSE_FEDERATED_KEEP_WARM.md new file mode 100644 index 00000000..609d0b2b --- /dev/null +++ b/DIAGNOSE_FEDERATED_KEEP_WARM.md @@ -0,0 +1,220 @@ +# Diagnosis — a federated cloud peer waking up with nobody querying it + +_Branch: `fix/federated-silent-poll-diagnosis` — 2026-08-05_ + +> **Status: root cause CONFIRMED against Azure Log Analytics ground truth.** +> An earlier revision of this document blamed a misconfigured local +> `CODESEARCH_KEEP_WARM_URL`. That hypothesis was **disproven** — see +> [What was ruled out](#what-was-ruled-out) §4. The confirmed cause is a +> two-part defect described in [Root cause](#root-cause). The corresponding +> fixes are listed in [Fixes](#fixes). + +## The requirement being violated + +Background polling of **local** repos is fine and expected. Background +polling of a **federated peer** must never happen — it was an explicit +design constraint from the original federation design, restated by the +reporting user as: + +> "hij mag die repos pollen LOKAAL maar niet federated !!! dat had ik +> nochthans in de specs effectief gezegd bij het ontwerp" + +Two things follow, and conflating them is what caused three round-trips on +this same behaviour: + +- A peer staying warm for its full idle window **after real use** is + *correct*. That is what keep-warm is for. +- A peer being **woken** with no federated query behind it is the defect — + as is it then staying warm for an hour off that spurious wake. + +"Cannot keep a peer awake past the host's own suspend term" is a strictly +**weaker** property than "never wakes it", and only the latter was ever the +requirement. + +## Symptom reported + +A local `codesearch serve` instance kept a mounted cloud federation peer (an +Azure Container Apps replica, `minReplicas: 0`) alive. Quitting the local +instance stopped it. The peer would wake, stay up ~1 hour, sleep, and wake +again — with no federated searches performed in between. Nothing appeared in +the local logs for any of it. + +## Ground truth + +From Log Analytics (`ContainerAppSystemLogs_CL` / `ContainerAppConsoleLogs_CL`) +on the deployed peer, over a period with **zero** federated searches: + +| Observation | Value | +|---|---| +| Interval between wakes | **120, 121, 120 minutes** | +| Warm period per wake | **~67 min** (1h idle window + 5min KEDA `cooldownPeriod`) | +| Nightly sleeps | exactly **2h00m30s** apart | +| Resulting duty cycle | **≈13.4h warm/day, ~56%** — at zero searches | + +The 120-minute spacing is the tell: it is the **local** host's +`DEFAULT_IDLE_SUSPEND_SECS` (2h), not any value configured on the peer. + +Each wake additionally paid for an `azcopy sync` of the docs blob and a +`git pull` of the KB repo. + +## Root cause + +Two independent defects, one triggering and one amplifying. + +### Defect 1 — the trigger: the TUI polled federated peers on a timer + +`spawn_remote_discovery` in `src/serve/tui.rs` used +`Duration::from_secs(state.idle_suspend_secs())` as a baseline poll interval +and, on each elapse, ran a `JoinSet` `/status` fan-out to **every** +configured peer. On the local host that value is 2h — matching the observed +cadence exactly. + +Each fan-out woke the peer's scale-to-zero replica. Nothing else was needed: +the poll *itself* was the ingress traffic. + +The reasoning that shipped this — recorded here so it is not reintroduced a +fourth time — was that polling no faster than the host's own suspend term is +harmless. It is not, for two separate reasons: + +1. Not keeping a peer awake *past* its suspend term says nothing about not + *waking* it. The peer's warm time is bounded, but its wake **count** is + not zero, and each wake costs a full warm window. +2. The two windows are unrelated values. `idle_suspend_secs` was read from + the **local** process (2h default); the window the woken peer then + honoured was the **peer's** (~1h). PR #181's description claimed the + cadence was "1h on the cloud deploy" — it was reading the local value. + +### Defect 2 — the amplifier: keep-warm rewarded spurious wakes + +The cloud keep-warm loop in `src/serve/mod.rs` computed its idle check as: + +```rust +let last = kw_state.most_recent_tool_call().unwrap_or(start); +``` + +`/status` and `/healthz` do **not** call `record_tool_call`. So a replica +woken by anything other than a genuine tool call found no recorded tool +call, fell back to the process start time, and self-pinged its own ingress +every `KEEP_WARM_INTERVAL_SECS` (120s) for the entire idle window. + +The critical observation is that this fallback is **unreachable in the case +it was written for**: a real tool call always sets `last_tool_call`, so the +`unwrap_or` only ever fires when the wake was *not* real work. Its whole +practical effect was to convert a momentary spurious wake into a full warm +hour — roughly **11× amplification** (~67 min instead of the ~6 min a bare +wake would have cost). + +### How they combine + +Defect 1 wakes the peer every 2h. Defect 2 then holds it up for ~67 min per +wake. Neither alone produces the observed 56% duty cycle; together they do. + +## What was ruled out + +1. **Explicit federated tool calls** (`federated_search`, + `federated_project_search`, `federated_get_chunk` in `src/mcp/mod.rs`) — + the only callers of `record_remote_peer_activity`, and only reached when a + project resolves to a federated alias. No federation-shaped log lines + existed in a full day's logs for either the reporting instance or an + unrelated local hub used to cross-check. +2. **`Watch-CodesearchServeReplicas.ps1`** — does poll `/status` every 20s, + but last ran 2026-07-05, well before the observed window. +3. **A stale binary re-introducing an old bug** — the reported startup banner + was `v1.2.1`. Worth upgrading, but the 2h cadence exists in that version + too. +4. **A misconfigured local `CODESEARCH_KEEP_WARM_URL`** *(the earlier + revision's stated root cause — disproven)*, on four independent grounds: + - The env var is set **nowhere** locally: not in the process environment, + not in `HKCU`, not in `HKLM`, not in any shell profile. + - The one-time `🔥 keep-warm enabled` line appears in **zero** local logs + from 2026-04-26 onward. + - That absence is meaningful: `init_serve_logger` is *always* file-only in + serve mode, unconditional on `--no-tui`, and those logs do carry other + `INFO` lines — so the line would have been captured had it fired. + - No local `codesearch` process held any connection on `:443`. + +Note that the earlier revision also ruled out TUI federated polling, on the +grounds that `maybe_spawn_tui` is gated on `!no_tui && is_tty()`. That gating +is real, but the conclusion was wrong: the reporting user's *waking* instance +was a normal TTY serve with the TUI running. Only the separate `--no-tui` +cross-check instance was exempt. + +## Fixes + +### Shipped earlier on this branch (commit `55fa36b`) + +Keep-warm observability, in `src/serve/mod.rs`: + +1. **Per-ping logging** — success at `debug!`, failure at `warn!`. Previously + `let _ = client.get(&ping_url)...send().await;` discarded both, leaving a + single one-time "enabled" line as the feature's only trace. +2. **Startup misconfiguration warning** — `extract_host_from_url` (no new + dependency) compares the keep-warm target host against the server's own + bind host and warns when they differ. + +Tests: `src/serve/tests.rs::keep_warm_host_extraction_tests`. + +### Defect 1 — no timer poll of federated peers + +`spawn_remote_discovery` no longer polls on any cadence. The periodic tick is +**config-only** (`REMOTE_ROW_REFRESH_SECS` = 5s, zero HTTP): it rebuilds +mounted-remote rows from the `remote_mounts` allowlist so mount/unmount edits +and `l` reloads surface promptly, and contacts nobody. + +A peer is contacted only by: + +- an **activity poke** — a real federated tool call just landed on that peer, + so it is demonstrably already awake; only that peer is refreshed, never a + fan-out, so an idle sibling peer is untouched; +- the explicit **`i`** info-overlay keypress on a remote row. + +Consequences: an idle mount renders its activity as `-`, which is now the +correct steady state rather than a fault. `ServeState::idle_suspend_secs` +(field, env init, getter and `--idle-suspend-secs` override) is removed — it +existed only to feed the poll cadence and became write-only. The keep-warm +task resolves flag > env > default directly, so `--idle-suspend-secs` is +unchanged. The `initial_cycle` startup gate is gone: every cycle is now +config-only, so it had nothing left to gate. + +Also fixed in passing: the snapshot emit was gated on a non-empty peer list, +so removing the *last* peer from `repos.json` left its rows on screen +forever. It is now unconditional. + +### Defect 2 — keep-warm requires a real tool call + +The `unwrap_or(start)` fallback is removed: with no tool call recorded there +is nothing to keep warm for, so the loop simply does not ping. A freshly +deployed replica now sleeps until first real use instead of self-warming for +an hour, which is the intended behaviour of scale-to-zero. + +### Follow-up — the `55fa36b` warning false-positived on the correct deploy + +The startup "target isn't self" warning fired on the **only deployment where +keep-warm is correct**: on Azure the process binds `0.0.0.0` while +`keep_warm_url` is the ingress FQDN, so `looks_like_self` was false and the +warning fired on every cold start. A wildcard bind means the +externally-visible host is genuinely unknown, so the comparison cannot +conclude anything and must stay silent — a check that cries wolf on the +correct configuration trains operators to ignore the case that matters. + +Fixed alongside Defect 2. The rule now lives in a testable +`keep_warm_foreign_target(ping_url, self_host) -> Option` helper +(`None` = do not warn), covered by tests for wildcard binds, a genuine +foreign host, a matching host, loopback targets, and an unparseable URL. + +## Residual surface (known, not currently exploitable) + +The MCP **`status` tool** passes `allow_unscoped = true`, but when it is +*project-scoped* (or the replica is single-repo) `is_multi` is false, so the +`!allow_unscoped || !is_multi` guard lets it through and it **does** record a +tool call. An automated poller calling the MCP `status` *tool* with +`project=` would therefore still buy a full warm window. + +No such poller is known to exist: both `Watch-CodesearchServeReplicas.ps1` +and `FederationClient::list_repos` use the **HTTP** `/status` endpoint +(`status_handler`), which does not record. Noted here so that if the +symptom ever recurs, this is the first place to look. + +## Local repos + +Unaffected by all of the above, by design. diff --git a/README.md b/README.md index 67afeab5..14f5b760 100644 --- a/README.md +++ b/README.md @@ -224,7 +224,7 @@ OpenCode: put this in the user-level `~/.config/opencode/AGENTS.md` (applies acr To make the preference **structural** instead of advisory, this repo ships three Claude Code `PreToolUse` hooks: -- **`grep-guard`** — on `Grep`. Blocks the first grep against an in-repo path when codesearch looks available (a local `.codesearch.db` at the git root, or a `CODESEARCH_SERVER` env var for remote-serve setups), with a message telling the model how to load and call codesearch instead. A retry of the same query within 5 minutes is let through unblocked — the legitimate "codesearch found nothing, falling back" path. Greps outside the current repo are never blocked, and the hook fails open (never traps the model). +- **`grep-guard`** — on `Grep`. Blocks a grep against an in-repo path when codesearch covers that repo (a local `.codesearch.db` at the git root, or a `CODESEARCH_SERVER` env var for remote-serve setups), with a message telling the model how to load and call codesearch instead. Grep is auto-allowed **only when the serve hub is genuinely down**, established by a live probe of the unauthenticated `/healthz` endpoint (`CODESEARCH_SERVER` > `127.0.0.1:$CODESEARCH_SERVE_PORT` > `127.0.0.1:39725`); only a connection-level failure counts as down. A low-confidence or empty codesearch result is a *successful* call meaning "reformulate the query", so it does **not** open the escape hatch — the deny message steers to `find`/`explore`/a single clean term instead. Greps outside the current repo are never blocked, and the hook fails open (never traps the model). - **`subagent-preamble`** — on `Agent` (the subagent-spawn tool). Prepends a short codesearch preamble to every subagent prompt, since subagents otherwise don't inherit `AGENTS.md` or MCP instructions at all. - **`web-guard`** — on `WebSearch`/`WebFetch`. When you have remote documentation projects mounted (`codesearch remote mount`, e.g. `cloud/inriver`, `cloud/example-dam`), it blocks the first web call with guidance to search those indexed mounts first — often more precise and current than the open web. Same 5-minute retry-escape; when no mounts are configured it does nothing. diff --git a/docs/federated-silent-poll/worklog.md b/docs/federated-silent-poll/worklog.md new file mode 100644 index 00000000..5bb5deea --- /dev/null +++ b/docs/federated-silent-poll/worklog.md @@ -0,0 +1,140 @@ +# Worklog — federated peer woken with no query behind it + +| | | +|---|---| +| **Branch** | `fix/federated-silent-poll-diagnosis` | +| **Base SHA** | `55fa36b` (🐛 fix: log keep-warm pings + warn when target isn't self) | +| **Scope** | Stop a scale-to-zero federated cloud peer being woken, and kept warm, with no federated query behind it. Local repo polling must stay untouched. | +| **Status** | Complete — 3 commits, all reviewed and passed. Not pushed. | +| **Latest test result** | `cargo test --lib --bins` → **1134 passed, 42 ignored**; `cargo fmt --check` clean; `cargo clippy --all-targets -- -D warnings` clean | + +## The requirement + +Background polling of **local** repos is fine. Background polling of a +**federated peer** must never happen — an explicit design constraint from the +original federation design, restated by the user as: + +> "hij mag die repos pollen LOKAAL maar niet federated !!! dat had ik nochthans +> in de specs effectief gezegd bij het ontwerp" + +A peer staying warm for an hour *after real use* is **correct** and was +explicitly confirmed as such. The defect was the peer being **woken** with no +query, and then staying warm off that spurious wake. + +## Stage 0 — diagnosis (no commit) + +The pre-existing `DIAGNOSE_FEDERATED_KEEP_WARM.md` blamed a misconfigured local +`CODESEARCH_KEEP_WARM_URL`. **Disproven** on four independent grounds: the env +var is set nowhere locally (process env, HKCU, HKLM, every shell profile); the +one-time `🔥 keep-warm enabled` line appears in zero logs from 2026-04-26 on; +that absence is meaningful because `init_serve_logger` is always file-only in +serve mode and those logs do carry other INFO lines; and no local process held +a `:443` connection. `Watch-CodesearchServeReplicas.ps1` was also eliminated +(polls `/status` every 20s, but last ran 2026-07-05). + +Azure Log Analytics ground truth, over a window with **zero** federated +searches: wakes **120 / 121 / 120 minutes** apart, each warm period **~67 min**, +nightly sleeps exactly 2h00m30s apart → **≈13.4h warm/day, ~56% duty cycle**. +The 120-minute spacing is the tell: it is the **local** host's 2h default, not +any value configured on the peer. + +Two defects, one triggering and one amplifying: + +1. **Trigger** — the TUI's `spawn_remote_discovery` used + `state.idle_suspend_secs()` as a baseline poll interval and ran a `JoinSet` + `/status` fan-out to every peer. The poll *itself* was the ingress traffic. +2. **Amplifier** — keep-warm's `most_recent_tool_call().unwrap_or(start)`. + `/status` and `/healthz` never call `record_tool_call`, so any non-tool-call + wake self-warmed for the full idle window. Unreachable in the case it was + written for, so its only practical effect was rewarding spurious wakes + (~11×). + +## Stage 1 — remove the federated timer poll + +- **Commit:** `6f1d1c5` · **Review:** ⚠️ PASS WITH REMARKS (round 1) → + fixes amended → **PASS, zero code defects** (round 2, cap reached). +- `spawn_remote_discovery` no longer polls on any cadence; the tick is + **config-only** (`REMOTE_ROW_REFRESH_SECS` = 5s, zero HTTP) so mount/unmount + edits and `l` reloads still surface. Contact is activity-poke (single peer, + never a fan-out) or the `i` keypress only. +- Removed `ServeState::idle_suspend_secs` (field, env init, getter, + `--idle-suspend-secs` override) — write-only once the cadence went. + Keep-warm resolves flag > env > default itself, so the flag still works. +- Round-1 fixes amended in: `tui_common.rs` `activity_stale` doc; and the + snapshot emit, previously gated on a non-empty peer list, which left rows on + screen forever after the last peer was removed. + +**Files:** `src/constants.rs`, `src/serve/mod.rs`, `src/serve/tui.rs`, +`src/serve/tui_common.rs` + +## Stage 2 — keep-warm requires a real tool call + +- **Commit:** `12edcf2` · **Review:** ✅ **PASS, zero findings.** +- The `unwrap_or(start)` fallback is gone; with no recorded tool call the loop + does not ping and lets the host suspend the replica. +- Reviewer independently confirmed warm-after-real-use does **not** regress: an + inbound federated search forces `project=`, reaching + `record_tool_call`; and `last_tool_call` is **insert-only** (no + `remove`/`clear`/`retain`, untouched by repo idle-eviction), so once one real + query lands the old behaviour holds for the process lifetime. +- Also fixed the `55fa36b` startup warning, which false-positived on the only + correct deployment: Azure binds `0.0.0.0` while the target is the ingress + FQDN. Wildcard bind ⇒ external host unknown ⇒ stay silent. Rule extracted to + the testable `keep_warm_foreign_target` helper (5 new tests). + +**Files:** `src/serve/mod.rs`, `src/serve/tests.rs` + +## Stage 3 — documentation + +- **Commit:** `3bcf153` · **Review:** ✅ **PASS** (final full-branch review, + `55fa36b...3bcf153`) — "Nothing further is owed on this branch." +- `DIAGNOSE_FEDERATED_KEEP_WARM.md` rewritten (moved from `docs/`): confirmed + root cause, ground truth, the violated requirement, and the **rejected + reasoning** recorded so it is not re-litigated a fourth time. +- `AGENTS.md` bullet rewritten as an explicit design constraint; it had + asserted the removed cadence as current and named a deleted field — the very + mechanism by which this behaviour was re-introduced twice. +- `CHANGELOG.md`: fix entry under `[1.2.4] (unreleased)`. An earlier draft had + put it under `[1.2.0]` — a **real tag** that shipped #181/#184; the reviewer + caught this, and both original entries were restored **verbatim** (confirmed + by diff) and marked superseded. +- `README.md`: grep-guard bullet corrected to the `/healthz` liveness probe. + +**Files:** `AGENTS.md`, `CHANGELOG.md`, `README.md`, +`DIAGNOSE_FEDERATED_KEEP_WARM.md` *(new)*, `docs/diagnose-federated-keep-warm.md` *(deleted)* + +## Why this took three attempts across three PRs + +PR #181 introduced the cadence on the reasoning that polling no faster than the +host's suspend term is harmless; PR #184 named "waking the scale-to-zero cloud +peer for no real reason" as the defect and then explicitly sanctioned that same +cadence. The flaw: **not keeping a peer awake past its suspend term is strictly +weaker than not waking it**, and the two windows were unrelated values anyway +(local host vs. remote peer). Both PRs correctly said "local repos unaffected", +confirming the local/federated split was real — but honoured it in only one +direction. + +## Open follow-ups + +- **Not pushed.** Three commits sit locally on `fix/federated-silent-poll-diagnosis`. + Per project workflow a PR targets `develop`. +- **Residual, known and not currently exploitable:** the MCP `status` **tool**, + when project-scoped, *does* record a tool call (`allow_unscoped=true` reduces + the guard to `!is_multi`), so an automated poller of that tool would still buy + a warm window. No such poller exists — both `Watch-CodesearchServeReplicas.ps1` + and `FederationClient::list_repos` use the HTTP `/status` endpoint, which does + not record. First place to look if the symptom recurs. +- **Unverified in production:** the fix is validated by tests and review, not yet + by observing the deployed peer stay asleep. Worth re-running the Log Analytics + query after this reaches the cloud replica — expected: no wakes without a + federated query. + +## Security note + +None. No auth, network-exposure or data-handling surface changed; the branch +strictly *reduces* outbound traffic. + + diff --git a/src/constants.rs b/src/constants.rs index 21546730..d1514472 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -452,17 +452,23 @@ pub const DEFAULT_REMOTE_TIMEOUT_SECS: u64 = 15; /// considers that peer's activity "live" before reverting the activity column to /// a stale `-`. /// -/// The baseline re-discovery poll runs on the **serve idle-suspend window** (see -/// [`IDLE_SUSPEND_SECS_ENV`] / [`DEFAULT_IDLE_SUSPEND_SECS`]) — the same term -/// after which the host is allowed to scale the replica to zero — so the TUI no -/// longer pins a federated peer awake with a fixed 30s ping. Instead, an -/// immediate per-peer refresh is triggered the moment a real tool call hits that -/// peer (event-driven, see `ServeState::record_remote_peer_activity`), and -/// *between* refreshes the activity column shows `-`. This window is how long a -/// freshly polled value stays visible before it goes stale again; it is short -/// relative to the hourly baseline poll. +/// There is **no background `/status` poll of a federated peer at all**: a peer +/// is contacted only when a real tool call hits it (event-driven, see +/// `ServeState::record_remote_peer_activity`) or on an explicit operator +/// keypress (`i` info overlay). Outside of active use a mount's activity column +/// simply reads `-`, so this window only governs how long a *poked* value stays +/// visible before going stale again. pub const REMOTE_ACTIVITY_FRESH_SECS: u64 = 5 * 60; // 5 minutes +/// Cadence of the embedded TUI's **config-only** mounted-remote row rebuild. +/// +/// This tick issues NO HTTP to any peer: it re-reads the repos config (via +/// `ServeState::config_snapshot`) and rebuilds the mounted-remote rows so +/// mount/unmount edits and `l` reloads show up promptly. Because it never +/// contacts a peer it cannot wake a scale-to-zero replica, which is precisely +/// why it is safe to run on a short interval. +pub const REMOTE_ROW_REFRESH_SECS: u64 = 5; + /// Maximum wall-clock duration a single reindex may take before its /// `active_reindexes` entry is considered **stale** (leaked). /// diff --git a/src/serve/mod.rs b/src/serve/mod.rs index d6eb0c56..34c5450e 100644 --- a/src/serve/mod.rs +++ b/src/serve/mod.rs @@ -277,14 +277,6 @@ pub(crate) struct ServeState { reload_count: std::sync::atomic::AtomicUsize, /// Instant when ServeState was created — used to compute uptime for TUI header. started_at: std::time::Instant, - /// Resolved idle-before-suspend window (seconds) — the same value the - /// keep-warm task uses to decide when to let the host scale the replica to - /// zero. The embedded TUI reuses it as the federated-peer `/status` baseline - /// poll interval, so its background polling can never keep a peer awake past - /// the host's own suspend term. Resolved in [`Self::new`] from - /// `IDLE_SUSPEND_SECS_ENV` (falling back to `DEFAULT_IDLE_SUSPEND_SECS`) and - /// overridden by the `--idle-suspend-secs` flag in `run_serve`. - idle_suspend_secs: u64, } impl std::fmt::Debug for ServeState { @@ -354,11 +346,6 @@ impl ServeState { #[cfg(test)] reload_count: std::sync::atomic::AtomicUsize::new(0), started_at: std::time::Instant::now(), - idle_suspend_secs: std::env::var(crate::constants::IDLE_SUSPEND_SECS_ENV) - .ok() - .and_then(|s| s.parse().ok()) - .filter(|s| *s > 0) - .unwrap_or(crate::constants::DEFAULT_IDLE_SUSPEND_SECS), } } @@ -2455,6 +2442,13 @@ impl ServeState { /// "active". Only genuine tool calls update `last_tool_call`; health/status /// probes and the keep-warm self-ping do not, so this reflects real query /// activity — not the keep-warm traffic that keeps the replica alive. + /// + /// `None` therefore means "this replica has served no real query since it + /// started", and keep-warm treats that as *do not ping* rather than falling + /// back to the process start time. Substituting the start time would make + /// every spurious wake (a probe, a dashboard poll) self-sustain for the + /// whole idle window — and, because a real tool call always sets this, + /// such a fallback can only ever fire when the wake was not real work. pub(crate) fn most_recent_tool_call(&self) -> Option { self.last_tool_call .iter() @@ -2477,20 +2471,15 @@ impl ServeState { /// The embedded TUI polls this every render tick; an advance (a newer /// `Instant` than the value seen on the previous tick) means a real tool call /// just used that peer, so the TUI pokes an immediate per-peer `/status` - /// refresh instead of waiting for the slow baseline poll. + /// refresh. This poke is the ONLY thing that ever makes the dashboard contact + /// a federated peer — there is no baseline poll, so a peer nobody queries is + /// left asleep (see `spawn_remote_discovery`). pub(crate) fn remote_peer_last_activity(&self, peer_name: &str) -> Option { self.remote_peer_activity .get(peer_name) .map(|entry| *entry.value()) } - /// The resolved idle-before-suspend window (seconds) — used by the embedded - /// TUI as the federated-peer `/status` baseline poll interval so background - /// polling can never keep a peer awake past the host's own suspend term. - pub(crate) fn idle_suspend_secs(&self) -> u64 { - self.idle_suspend_secs - } - /// Record that changes were made to a repo (index/reindex). #[allow(dead_code)] pub(crate) fn record_changes(&self, alias: &str, count: u64) { @@ -4591,6 +4580,71 @@ fn build_streamable_http_config() -> StreamableHttpServerConfig { } } +/// Extracts the host (no scheme, no port, no path) from a URL string, without +/// pulling in the `url` crate as a new direct dependency (it is only +/// transitive via reqwest today). Deliberately best-effort: used solely for +/// the keep-warm misconfiguration warning in `run_serve`, where a parse +/// failure just means the sanity check is skipped, not a hard error. +fn extract_host_from_url(url: &str) -> Option { + let after_scheme = url.split("://").nth(1).unwrap_or(url); + let host_and_rest = after_scheme.split(['/', '?', '#']).next()?; + // Strip a trailing `:port`, but not the `:` inside an IPv6 literal like + // `[::1]:8080` — only split on the LAST colon when the host isn't + // bracketed. + let host = if host_and_rest.starts_with('[') { + host_and_rest + .split(']') + .next() + .map(|h| format!("{h}]")) + .unwrap_or_else(|| host_and_rest.to_string()) + } else { + host_and_rest + .rsplit_once(':') + .map(|(h, _)| h.to_string()) + .unwrap_or_else(|| host_and_rest.to_string()) + }; + if host.is_empty() { + None + } else { + Some(host) + } +} + +/// Decide whether the keep-warm target looks like it points at a host *other* +/// than this replica, returning the offending target host when it does. +/// +/// `None` means "do not warn" — either the target does look like self, or we +/// cannot tell. Returning `None` for "cannot tell" is deliberate: +/// +/// - A **wildcard bind** (`0.0.0.0`, `::`) means our externally-visible host is +/// genuinely unknown. This is the normal cloud case — on Azure Container Apps +/// the process binds `0.0.0.0` while `keep_warm_url` is correctly the ingress +/// FQDN — so comparing the two proves nothing. Warning here would fire on +/// every cold start of the one deployment where keep-warm is *supposed* to +/// run, and a check that cries wolf on the correct configuration trains +/// operators to ignore the case that actually matters. +/// - A URL with no extractable host cannot be compared at all. +fn keep_warm_foreign_target(ping_url: &str, self_host: &str) -> Option { + // Wildcard / unspecified binds: externally-visible host unknown. + if matches!( + self_host, + "0.0.0.0" | "::" | "[::]" | "0:0:0:0:0:0:0:0" | "[0:0:0:0:0:0:0:0]" | "" + ) { + return None; + } + let target_host = extract_host_from_url(ping_url)?; + let looks_like_self = target_host == self_host + || target_host == "localhost" + || target_host == "127.0.0.1" + || target_host == "::1" + || target_host == "[::1]"; + if looks_like_self { + None + } else { + Some(target_host) + } +} + pub async fn run_serve( host: Option, port: Option, @@ -4669,17 +4723,11 @@ pub async fn run_serve( #[cfg(unix)] raise_fd_limit(config.repos.len()); - let mut serve_state = ServeState::new(config, None); - // The `--idle-suspend-secs` flag takes precedence over the env/default the - // constructor already resolved; mirror the keep-warm task's resolution so - // the embedded TUI's federated-peer poll interval matches exactly. `0` - // means "disabled" for keep-warm, so treat it as "leave the default". - if let Some(secs) = idle_suspend_secs { - if secs > 0 { - serve_state.idle_suspend_secs = secs; - } - } - let serve_state = Arc::new(serve_state); + // The idle-suspend window is resolved by the keep-warm task alone (flag > + // env > default); nothing else consumes it, so `ServeState` does not carry + // it. In particular the embedded TUI must NOT derive a poll cadence from it + // — it never polls a federated peer on a timer at all. + let serve_state = Arc::new(ServeState::new(config, None)); // Construct the bind address from resolved host + port. // Using `format!` with `parse::()` handles both IPv4 and IPv6. @@ -4914,13 +4962,47 @@ pub async fn run_serve( let ping_url = format!("{}{}", base_url.trim_end_matches('/'), HEALTHZ_PATH); let kw_state = serve_state.clone(); let kw_cancel = cancel_token.clone(); - let start = Instant::now(); info!( "🔥 keep-warm enabled: pinging {} every {}s while idle < {}s", ping_url, crate::constants::KEEP_WARM_INTERVAL_SECS, idle_suspend ); + // Sanity check: keep-warm exists to self-ping THIS replica's own + // ingress so the platform sees traffic and doesn't suspend it — it + // is not meant to point at any other host, and nothing upstream of + // this function validates that. If CODESEARCH_KEEP_WARM_URL (or + // --keep-warm-url) was ever set to a DIFFERENT host — e.g. copied + // from a cloud deployment's env into a local shell profile — this + // task would silently generate periodic outbound traffic to that + // other host with zero per-request log line (only this one-time + // "enabled" message), which is exactly the failure mode a user + // reported: a local `serve --no-tui` process quietly keeping a + // mounted federation peer's cloud replica warm every + // KEEP_WARM_INTERVAL_SECS, defeating its scale-to-zero, discoverable + // only by noticing outbound network traffic — not by anything in + // the local server's own logs. This can't be fully auto-corrected + // (we don't reliably know our own externally-visible host), but a + // loud one-time warning when the target doesn't look like "self" + // (differs from the bind host/port this process is actually + // listening on) turns a silent misconfiguration into a visible one. + // + // A WILDCARD bind is the one case where this check must stay silent — + // see [`keep_warm_foreign_target`], which owns that rule so it can be + // unit-tested. + let self_host = effective_host.as_str(); + if let Some(target_host) = keep_warm_foreign_target(&ping_url, self_host) { + tracing::warn!( + "⚠️ keep-warm target host '{target_host}' does not match this \ + server's own bind host '{self_host}'. keep-warm exists to \ + self-ping THIS replica, not another peer — verify \ + CODESEARCH_KEEP_WARM_URL / --keep-warm-url is not \ + accidentally pointing at a different (e.g. cloud/federated) \ + server, which would silently keep that OTHER server warm \ + every {}s.", + crate::constants::KEEP_WARM_INTERVAL_SECS + ); + } tokio::spawn(async move { let interval = std::time::Duration::from_secs(crate::constants::KEEP_WARM_INTERVAL_SECS); @@ -4928,16 +5010,53 @@ pub async fn run_serve( loop { tokio::select! { _ = tokio::time::sleep(interval) => { - // Fall back to the server start time when no query has - // happened yet, so a freshly deployed replica stays warm - // for the full idle window before first use. - let last = kw_state.most_recent_tool_call().unwrap_or(start); + // Keep-warm sustains warmth only AFTER real use. With no + // tool call recorded there is nothing to keep warm for, + // so we simply don't ping and let the host suspend us; + // the next real request wakes us. + // + // This previously fell back to the process start time + // ("a freshly deployed replica stays warm for the full + // idle window before first use"). That was actively + // harmful, and unreachable in the case it was written + // for: a genuine tool call always records itself, so the + // fallback could only ever fire when the wake was NOT + // real work. `/status` and `/healthz` have their own + // handlers and never call `record_tool_call`, so ANY + // spurious wake — a dashboard poll, a platform probe — + // made the replica self-ping for the whole idle window. + // Measured on the cloud peer: ~67 min warm instead of + // the ~6 min a bare wake costs, ≈11x amplification. Its + // entire practical effect was rewarding spurious wakes. + let Some(last) = kw_state.most_recent_tool_call() else { + continue; + }; if last.elapsed().as_secs() < idle_suspend { - let _ = client + // Previously this ping was completely silent — no log + // line at all, success or failure. That silence is + // exactly what made a misconfigured keep-warm target + // (see the sanity check above) undiagnosable from the + // logs alone. debug! on success keeps normal operation + // quiet by default while still being traceable with + // RUST_LOG=debug; failures are always worth a warn. + match client .get(&ping_url) .timeout(std::time::Duration::from_secs(10)) .send() - .await; + .await + { + Ok(resp) => { + tracing::debug!( + "keep-warm ping to {ping_url} -> {}", + resp.status() + ); + } + Err(e) => { + tracing::warn!( + "keep-warm ping to {ping_url} failed: {e:#}" + ); + } + } } } _ = kw_cancel.cancelled() => break, diff --git a/src/serve/tests.rs b/src/serve/tests.rs index 2cde7666..baf5c586 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -1593,3 +1593,131 @@ mod allowed_hosts_tests { ); } } + +/// Tests for `extract_host_from_url` — used solely by the keep-warm +/// misconfiguration sanity check (a keep-warm target host that doesn't look +/// like "self" gets a loud warning; see the diagnosis this shipped with in +/// docs/diagnose-federated-keep-warm.md). +mod keep_warm_host_extraction_tests { + use super::*; + + #[test] + fn extracts_host_from_plain_http_url() { + assert_eq!( + extract_host_from_url("http://127.0.0.1:8080/healthz"), + Some("127.0.0.1".to_string()) + ); + } + + #[test] + fn extracts_host_from_https_url_without_port() { + assert_eq!( + extract_host_from_url("https://happywave-063747be.azurecontainerapps.io/healthz"), + Some("happywave-063747be.azurecontainerapps.io".to_string()) + ); + } + + #[test] + fn extracts_host_with_no_scheme() { + // The keep-warm URL is user-supplied (CLI flag or env var) and never + // validated to include a scheme — must not panic or silently return + // the whole string including a path. + assert_eq!( + extract_host_from_url("localhost:39725/healthz"), + Some("localhost".to_string()) + ); + } + + #[test] + fn extracts_ipv6_host_preserving_brackets() { + // A bare rsplit_once(':') would wrongly split inside the IPv6 + // literal itself (e.g. on the last `:` in `::1`) if not guarded. + assert_eq!( + extract_host_from_url("http://[::1]:8080/healthz"), + Some("[::1]".to_string()) + ); + } + + #[test] + fn strips_query_and_fragment_before_host_ends() { + assert_eq!( + extract_host_from_url("http://example.com/healthz?x=1#frag"), + Some("example.com".to_string()) + ); + } + + #[test] + fn returns_none_for_empty_host() { + assert_eq!(extract_host_from_url("http:///healthz"), None); + } +} + +/// The keep-warm "target isn't self" warning must fire on a genuine +/// misconfiguration and stay silent on the cloud deployment where keep-warm is +/// actually supposed to run. Getting the latter wrong is worse than having no +/// check at all: a warning that fires on every correct cold start trains +/// operators to ignore it. +#[cfg(test)] +mod keep_warm_foreign_target_tests { + use super::*; + + /// The regression this rule exists for: on Azure Container Apps the process + /// binds `0.0.0.0` while the keep-warm target is correctly the ingress + /// FQDN. A naive host comparison flags that as "not self" and warns on + /// every cold start of the only correct deployment. + #[test] + fn wildcard_bind_never_warns_even_for_a_foreign_looking_fqdn() { + for wildcard in ["0.0.0.0", "::", "[::]", "0:0:0:0:0:0:0:0", ""] { + assert_eq!( + keep_warm_foreign_target( + "https://codesearch-serve.azurecontainerapps.io", + wildcard + ), + None, + "wildcard bind {wildcard:?} must not warn — our external host is unknown" + ); + } + } + + /// The case the check exists to catch: a concretely-bound local serve whose + /// keep-warm URL points at somebody else's cloud replica. + #[test] + fn concrete_bind_warns_for_a_different_host() { + assert_eq!( + keep_warm_foreign_target("https://peer.example.com/healthz", "192.168.1.10"), + Some("peer.example.com".to_string()) + ); + } + + #[test] + fn matching_host_does_not_warn() { + assert_eq!( + keep_warm_foreign_target("http://192.168.1.10:39725/healthz", "192.168.1.10"), + None + ); + } + + #[test] + fn loopback_targets_are_always_treated_as_self() { + for target in [ + "http://localhost:39725/healthz", + "http://127.0.0.1:39725/healthz", + "http://[::1]:39725/healthz", + ] { + assert_eq!( + keep_warm_foreign_target(target, "192.168.1.10"), + None, + "{target} is loopback and must not warn" + ); + } + } + + /// No extractable host → nothing to compare → no warning. + #[test] + fn unparseable_target_does_not_warn() { + assert_eq!( + keep_warm_foreign_target("http:///healthz", "192.168.1.10"), + None + ); + } +} diff --git a/src/serve/tui.rs b/src/serve/tui.rs index ef3c783d..de7ea5d8 100644 --- a/src/serve/tui.rs +++ b/src/serve/tui.rs @@ -98,11 +98,11 @@ async fn run_tui_loop( // Monotonic id of the most recent doctor request; bumped on every spawn. let mut doctor_gen: u64 = 0; - // Mounted remote projects (peer-hosted indexes, shown italic). Discovered on - // a background task whose baseline cadence is the serve idle-suspend window - // (so it never keeps a peer awake past the host's own suspend term); an - // immediate per-peer refresh is poked the moment a real tool call hits a - // peer. The latest snapshot is cached here and appended after the local rows. + // Mounted remote projects (peer-hosted indexes, shown italic). The background + // task rebuilds these rows from config alone and NEVER polls a peer on a + // timer; the only refresh trigger is a poke sent the moment a real tool call + // hits a peer (so a scale-to-zero peer is never woken by the dashboard). The + // latest snapshot is cached here and appended after the local rows. let (remote_tx, mut remote_rx) = tokio::sync::mpsc::channel::(1); // Poke channel: the render loop sends a peer name here when it detects that // peer's activity advanced (a real tool call), triggering an immediate @@ -137,8 +137,9 @@ async fn run_tui_loop( // peer's last refresh time, and (b) detect peers whose real activity // advanced since the last tick and poke the discovery task to refresh // just them. A peer with no mounts contributes nothing and is never - // polled — the idle-suspend cadence + activity poke fully replace the - // old fixed 30s ping so federated peers can scale to zero. + // polled — the activity poke is the *only* thing that ever contacts a + // peer from here, so a federated peer can stay scaled to zero for as + // long as nobody actually queries it. let fresh_window = Duration::from_secs(crate::constants::REMOTE_ACTIVITY_FRESH_SECS); let mut alias_to_peer: std::collections::HashMap = std::collections::HashMap::new(); @@ -516,16 +517,35 @@ async fn poll_peer_status( /// Spawn the background task that discovers mounted remote projects and pushes /// snapshots through `tx`. /// -/// **Scale-to-zero design.** The baseline re-discovery cadence is the serve -/// idle-suspend window ([`ServeState::idle_suspend_secs`]) — the same term after -/// which the host may scale the replica to zero — NOT a fixed 30s ping, so the -/// TUI no longer pins a federated peer awake. Between baseline polls the cached -/// activity is stale and the render loop shows `-`. The moment a real tool call -/// hits a peer, the render loop detects the advance (via -/// [`ServeState::remote_peer_last_activity`]) and sends the peer name on -/// `poke_rx`, triggering an **immediate per-peer** refresh — never a full poll, -/// so an idle sibling peer is not woken. A peer that blips a round keeps its -/// cached row (a mount never vanishes on a transient failure). +/// **Scale-to-zero design: a federated peer is NEVER polled on a timer.** Local +/// repos are refreshed freely by the render loop; a *federated* peer is +/// contacted only when there is a real reason to: +/// +/// - an **activity poke** — a genuine federated tool call just hit that peer, so +/// it is already awake and refreshing it costs nothing. The render loop detects +/// the advance via [`ServeState::remote_peer_last_activity`] and sends the peer +/// name on `poke_rx`; only that peer is refreshed, never a full fan-out, so an +/// idle sibling peer is not touched. +/// - an explicit operator keypress — `i` on a remote row fetches that peer's +/// index stats for the info overlay (see `spawn_remote_info`). That is a +/// deliberate human action, not background traffic. `l` (reload) only re-reads +/// the local config and contacts nobody. +/// +/// The periodic tick below is **config-only** +/// ([`crate::constants::REMOTE_ROW_REFRESH_SECS`]): it rebuilds the rows from the +/// `remote_mounts` allowlist so mount/unmount edits and `l` reloads appear +/// promptly, and issues no HTTP whatsoever. Outside of active use a mount +/// therefore renders its activity as a stale `-` and a scale-to-zero peer stays +/// asleep indefinitely. A peer that blips on a poke keeps its cached row (a mount +/// never vanishes on a transient failure). +/// +/// **Why there is no baseline poll.** An earlier version ran a `/status` fan-out +/// on the *local* serve's idle-suspend window (2h by default), on the theory that +/// polling no faster than the suspend term was harmless. It is not: each poll +/// *woke* a sleeping replica, which then held itself warm for its own full idle +/// window (1h on the cloud deploy) — a ~50% duty cycle on a peer nobody queried. +/// Not keeping a peer awake past its suspend term is not the same as not waking +/// it, and the two windows were unrelated values besides (local vs. peer). fn spawn_remote_discovery( state: Arc, tx: tokio::sync::mpsc::Sender, @@ -541,10 +561,10 @@ fn spawn_remote_discovery( return; } }; - // Baseline poll cadence = the serve idle-suspend window, so background - // polling can never keep a federated peer awake past the host's own - // suspend term. The real "go live again" trigger is the activity poke. - let interval = Duration::from_secs(state.idle_suspend_secs().max(1)); + // Cadence of the CONFIG-ONLY row rebuild. This tick contacts no peer, so + // it cannot wake a scale-to-zero replica and is safe to run often; it + // exists purely so mount/unmount edits and `l` reloads surface promptly. + let row_refresh = Duration::from_secs(crate::constants::REMOTE_ROW_REFRESH_SECS.max(1)); // Cached per-(peer, remote_alias) status, retained across cycles so a // peer that blips this round keeps showing its last-known row. @@ -557,97 +577,66 @@ fn spawn_remote_discovery( let mut refreshed_at: std::collections::HashMap = std::collections::HashMap::new(); - // Startup gate: the first cycle builds rows from config ALONE — so - // mounted remotes render immediately as stale `-` (no `refreshed_at` - // entry → stale) — WITHOUT pinging any peer. A scale-to-zero cloud - // peer must not be woken just to fill the dashboard when the operator - // restarts their local serve. The first real refresh comes from either - // the baseline cadence tick (idle-suspend window) or an activity poke - // (a real federated tool call). - let mut initial_cycle = true; - - 'outer: loop { + loop { + // ── Config-only snapshot. Rows come from the `remote_mounts` + // allowlist and are merely *enriched* by whatever status is already + // cached, so this issues no HTTP and cannot wake a sleeping peer. A + // mount with no cached refresh (`refreshed_at` miss) renders its + // activity as a stale `-`, which is the correct display for a peer + // that is scaled to zero. + // + // Emitted UNCONDITIONALLY, including when no peers are configured: + // the old code gated this on `!cfg.remotes.is_empty()`, so removing + // the last peer from `repos.json` left the previously emitted rows + // rendered forever (no snapshot was ever sent to clear them). With + // no peers `build_remote_rows` yields an empty vec, which clears + // them. Still zero HTTP, so this costs nothing. let cfg = state.config_snapshot(); - if !cfg.remotes.is_empty() { - if initial_cycle { - // First cycle: emit config-derived rows only; skip the poll - // (empty status_lookup + refreshed_at → every row stale `-`). - initial_cycle = false; - } else { - // ── Full poll: refresh EVERY configured peer concurrently. ── - let now = std::time::Instant::now(); - let mut join = tokio::task::JoinSet::new(); - for (peer_name, peer) in cfg.remotes.iter() { - let client = client.clone(); - let peer = peer.clone(); - let peer_name = peer_name.clone(); - join.spawn( - async move { (peer_name, poll_peer_status(&client, &peer).await) }, - ); + // Capacity-1 channel: replace the pending snapshot if the render + // loop hasn't consumed it yet (try_send drops on Full — fine, the + // next tick supersedes it anyway). + let _ = tx.try_send(RemoteDiscoveryUpdate { + rows: build_remote_rows(&status_lookup, &cfg), + refreshed_at: refreshed_at.clone(), + }); + + // ── Wait: config-only tick OR an activity poke. ── + // The tick merely loops back and re-emits rows. A poke means a real + // federated tool call just landed on that peer, so it is provably + // awake already — refresh that ONE peer; idle sibling peers are never + // contacted. There is deliberately no timer branch that polls peers. + tokio::select! { + _ = cancel.cancelled() => return, + _ = tokio::time::sleep(row_refresh) => {} + peer = poke_rx.recv() => { + // poke_rx closes only when the render loop is shutting + // down (it owns poke_tx) → exit the discovery task. + let Some(first) = peer else { return; }; + // Drain queued pokes; refresh each unique peer once. + let mut targets = std::collections::HashSet::new(); + targets.insert(first); + while let Ok(more) = poke_rx.try_recv() { + targets.insert(more); } - while let Some(res) = join.join_next().await { - if let Ok((peer_name, Some(repos))) = res { + let cfg = state.config_snapshot(); + for peer_name in targets { + let Some(peer) = cfg.remotes.get(&peer_name) else { + continue; + }; + if let Some(repos) = poll_peer_status(&client, peer).await { // Drop stale entries for this peer before inserting the // fresh set (handles repos that vanished on the peer). status_lookup.retain(|(p, _), _| p != &peer_name); for r in repos { - status_lookup.insert((peer_name.clone(), r.alias.clone()), r); + status_lookup + .insert((peer_name.clone(), r.alias.clone()), r); } - refreshed_at.insert(peer_name, now); + refreshed_at.insert(peer_name, std::time::Instant::now()); } - // Unreachable peers keep their cached row + aged refresh + // An unreachable peer keeps its cached row + aged refresh // time (→ stale `-`), never vanishing from the table. } - } - // Capacity-1 channel: replace the pending snapshot if the render - // loop hasn't consumed it yet (try_send drops on Full — fine, the - // next round supersedes it anyway). On the skipped first cycle - // this ships rows built from an empty status_lookup → stale `-`. - let _ = tx.try_send(RemoteDiscoveryUpdate { - rows: build_remote_rows(&status_lookup, &cfg), - refreshed_at: refreshed_at.clone(), - }); - } - - // ── Wait: baseline interval OR an activity poke. ── - // Baseline elapse → continue 'outer (full poll). A poke → single- - // peer refresh only, then keep waiting (no full poll, so idle - // sibling peers are NOT woken). - loop { - tokio::select! { - _ = cancel.cancelled() => return, - _ = tokio::time::sleep(interval) => continue 'outer, - peer = poke_rx.recv() => { - // poke_rx closes only when the render loop is shutting - // down (it owns poke_tx) → exit the discovery task. - let Some(first) = peer else { return; }; - // Drain queued pokes; refresh each unique peer once. - let mut targets = std::collections::HashSet::new(); - targets.insert(first); - while let Ok(more) = poke_rx.try_recv() { - targets.insert(more); - } - let cfg = state.config_snapshot(); - for peer_name in targets { - let Some(peer) = cfg.remotes.get(&peer_name) else { - continue; - }; - if let Some(repos) = poll_peer_status(&client, peer).await { - status_lookup.retain(|(p, _), _| p != &peer_name); - for r in repos { - status_lookup.insert( - (peer_name.clone(), r.alias.clone()), - r, - ); - } - refreshed_at.insert(peer_name, std::time::Instant::now()); - } - } - let _ = tx.try_send(RemoteDiscoveryUpdate { - rows: build_remote_rows(&status_lookup, &cfg), - refreshed_at: refreshed_at.clone(), - }); - } + // Loop back: the snapshot at the top ships the refreshed rows. } } } diff --git a/src/serve/tui_common.rs b/src/serve/tui_common.rs index 038b0f23..e98cd29b 100644 --- a/src/serve/tui_common.rs +++ b/src/serve/tui_common.rs @@ -76,8 +76,10 @@ pub struct RepoRow { pub is_remote: bool, /// True when this *remote* row's activity (`last_tool_call`) is considered /// stale by the embedded TUI — i.e. the peer's `/status` hasn't been - /// refreshed within `REMOTE_ACTIVITY_FRESH_SECS` (the slow baseline poll - /// hasn't fired and no real tool call has poked an immediate refresh). When + /// refreshed within `REMOTE_ACTIVITY_FRESH_SECS`. There is no background + /// poll that could refresh it: a federated peer is contacted only when a + /// real tool call pokes an immediate refresh (or on the `i` keypress), so + /// for an idle mount this is the *normal* steady state, not a fault. When /// stale, the activity column renders `-` instead of a possibly-hours-old /// "Xh ago". Always `false` for local repos (which carry live serve state) /// and for the standalone remote dashboard.