From bb655b7cc1d59e5489d0232f3203a6d8febba555 Mon Sep 17 00:00:00 2001 From: flupkede Date: Thu, 17 Sep 2026 11:46:03 +0200 Subject: [PATCH] =?UTF-8?q?[worker]=20fix:=20federated=20chunk=20fetch=20?= =?UTF-8?q?=E2=80=94=20URL=20residue=20+=20project-scope=20routing=20(todo?= =?UTF-8?q?=20#153)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent defects in remote chunk fetch: 1. The peer URL replaced '{id' without the closing brace, producing /chunk/2058%7D — axum's {id} param swallowed the stray '}' into the captured value, so mock-based tests passed while real peers answered 400. Replace the full '{id}' placeholder; pinned by a test whose route echoes the exact path it was hit on. 2. get_chunk(project=/, chunk_id) died with 'Unknown alias': mounted remote projects now route through the same federated fetch search uses (local aliases win name clashes). Hermetic routing test (temp config file — the in-memory config is reloaded from disk, which previously leaked the developer's real peers into the test). --- CHANGELOG.md | 4 ++++ src/federation/mod.rs | 47 +++++++++++++++++++++++++++++++++++- src/mcp/get_chunk.rs | 22 +++++++++++++++++ src/serve/tests.rs | 56 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9633ee3b..6b90526b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,10 @@ finalized in place with a date — no renaming/migration step needed. - **Serve auto-recovers LMDB storage-format corruption with a sequential wipe + rebuild.** After the arroy 0.5→0.8 / heed 0.20→0.22 major upgrades, every repo whose on-disk database was written by the previous binary failed its symbol rebuild with `MDB_BAD_VALSIZE: Unsupported size of key/DB name/data, or wrong DUPFIXED size` (observed on all C# repos after deploy). When a symbol rebuild now fails with that error class, serve wipes the repo's DB directory — closing the LMDB envs first via the same eviction sequence `remove_repo` uses, with the same bounded retry for transient Windows lock holders — and force-reindexes it through the existing force-reindex machinery, whose store-open path recreates everything on the new formats. Recoveries are queued and processed strictly **one repo at a time**: each rebuild runs a full CPU-bound embed pass, so parallel recoveries would thrash the machine. Read-only repos are skipped with a pointer to the owning writer. This closes the gap the tantivy FTS graceful reset (above) already covered on the FTS side — the vector/symbol stores now self-heal the same upgrade boundary instead of staying red until an operator force-reindexes by hand. +### Fixed + +- **Federated chunk fetch works again — URL residue and project-scope routing (todo #153).** Two independent defects: (1) the peer URL for a `chunk_ref` fetch was built by replacing `{id` without the closing brace, so the constructed path carried a stray `}` (`/chunk/2058%7D`) that real peers answered with 400 Bad Request — axum's `{id}` parameter happily swallowed the stray brace into the captured value, which is exactly why the mock-based tests never caught it; the replacement now covers the full `{id}` placeholder, pinned by a test whose route echoes back the exact path it was hit on. (2) `get_chunk` with `project=/` plus a plain `chunk_id` died in local routing with "Unknown alias" — mounted remote projects are now routed through the same federated fetch search uses (local aliases still win a name clash), so both `chunk_ref` and `project=`+`chunk_id` forms work against remote peers. + ## [1.3.19] ### Changed diff --git a/src/federation/mod.rs b/src/federation/mod.rs index 394cdcbb..d16a56e4 100644 --- a/src/federation/mod.rs +++ b/src/federation/mod.rs @@ -402,7 +402,7 @@ impl FederationClient { ) -> Outcome { let mut url = Self::peer_url( peer, - &crate::constants::CHUNK_PATH.replace("{id", &chunk_id.to_string()), + &crate::constants::CHUNK_PATH.replace("{id}", &chunk_id.to_string()), ); // Scope the lookup: prefer a single-project scope (`project=`) // so the multi-repo peer can disambiguate the chunk_id; fall back to @@ -1285,6 +1285,51 @@ mod tests { ); } + #[tokio::test] + #[serial_test::serial] + async fn get_chunk_requests_the_exact_chunk_path_without_placeholder_residue() { + // Regression (todo #153): the URL template is `/chunk/{id}`; a + // placeholder replacement that missed the closing brace produced + // `/chunk/2058%7D`. Axum's `{id}` param happily swallowed the stray + // `}` into the captured value, so mock-based route tests passed while + // real peers answered 400 Bad Request on the mangled id. The route + // below echoes back the EXACT path it was hit on, so any residue in + // the constructed URL fails the assertion. + let seen = std::sync::Arc::new(tokio::sync::Mutex::new(String::new())); + let seen_clone = seen.clone(); + let router = axum::Router::new().route( + "/chunk/{id}", + axum::routing::get(move |uri: axum::http::Uri| { + let seen = seen_clone.clone(); + async move { + *seen.lock().await = uri.path().to_string(); + axum::Json(serde_json::json!({ + "chunk_id": 2058, + "content": "ok", + "path": "kb/x.md", + "start_line": 1, + "end_line": 2 + })) + } + }), + ); + let addr = spawn_test_server(router).await; + + let client = FederationClient::new().unwrap(); + let outcome = client + .get_chunk(&peer(format!("http://{addr}")), Some("bynder"), 2058, None) + .await; + assert!( + matches!(outcome, Outcome::Ok(_)), + "the peer must answer the clean path" + ); + assert_eq!( + *seen.lock().await, + "/chunk/2058", + "the constructed URL must carry the bare chunk id — no placeholder residue" + ); + } + #[tokio::test] #[serial_test::serial] async fn get_chunk_persistent_503_reports_cold_start_hint() { diff --git a/src/mcp/get_chunk.rs b/src/mcp/get_chunk.rs index 8f4ffaed..c32cdd59 100644 --- a/src/mcp/get_chunk.rs +++ b/src/mcp/get_chunk.rs @@ -34,6 +34,28 @@ impl CodesearchService { .await; } + // Federated mounts: `project=/` routes to the peer's own + // project, exactly like search's project-level federation — chunk_ids + // are peer-local, so the fetch reuses the chunk_ref path with a + // synthetic "/:" ref. Local aliases ALWAYS win a name + // clash: only route remotely when the name is not a local project. + if let Some(proj) = request.project.as_deref() { + let cfg = self.federation_config(); + if cfg.resolve(proj).is_none() { + if let Some(crate::db_discovery::repos::Target::RemoteProject { + peer_name, + peer: _, + remote_alias, + }) = cfg.resolve_remote_project(proj) + { + let chunk_ref = format!("{peer_name}/{remote_alias}:{}", request.chunk_id); + return self + .federated_get_chunk(&chunk_ref, request.context_lines) + .await; + } + } + } + // In multi-repo serve mode, require explicit project or group scope. // Unscoped get_chunk would fan-out over all repos, opening all DBs unnecessarily. // Consistent with search/find/explore which also require scope. diff --git a/src/serve/tests.rs b/src/serve/tests.rs index 80a4d8fe..53081376 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -30,6 +30,62 @@ fn rest_service_drop_does_not_touch_active_sessions() { ); } +#[tokio::test] +#[serial] +async fn get_chunk_routes_mounted_remote_projects_through_federation() { + // todo #153: `get_chunk(project="/", chunk_id=…)` must route + // through the federated fetch exactly like search's project-level + // federation, instead of dying in local routing with "Unknown alias". + // The peer URL here is unreachable, so the correctly routed answer is the + // federation failure message — "Unknown alias" means the routing did not + // happen. + let mut config = ReposConfig::default(); + config.remotes.insert( + "cloud".to_string(), + crate::db_discovery::repos::RemotePeer { + url: "http://127.0.0.1:1".to_string(), + api_key: "test-key".to_string(), + group: None, + timeout_secs: None, + }, + ); + config.remote_mounts.push("cloud/bynder".to_string()); + // Hermetic config: persist to a temp file and pass the override, so + // `reload_if_changed` reads THIS config — not the developer's real + // ~/.codesearch/repos.json (which would leak real peers into the test). + let tmp = tempfile::tempdir().unwrap(); + let config_file = tmp.path().join("repos.json"); + config.save_to(&config_file).unwrap(); + let state = std::sync::Arc::new(ServeState::new(config, Some(config_file))); + let service = crate::mcp::CodesearchService::new_for_serve(state).unwrap(); + + let _env = + crate::testing::EnvRestore::set(&[(crate::constants::REMOTE_PEER_RETRY_BACKOFF_ENV, "1")]); + let req = crate::mcp::types::GetChunkRequest { + chunk_id: 2058, + chunk_ref: None, + context_lines: None, + project: Some("cloud/bynder".to_string()), + group: None, + }; + let res = service + .get_chunk(rmcp::handler::server::wrapper::Parameters(req)) + .await + .expect("handler must not error"); + let text = match res.content.first() { + Some(rmcp::model::ContentBlock::Text(t)) => t.text.clone(), + other => panic!("expected text content, got {other:?}"), + }; + assert!( + text.contains("Could not fetch chunk from remote peer 'cloud'"), + "get_chunk must route mounted remote projects to the peer, got: {text}" + ); + assert!( + !text.contains("Unknown alias"), + "a mounted remote project is not a local alias — routing failed: {text}" + ); +} + #[test] fn tracked_session_drop_balances_active_sessions() { // A genuine MCP session increments on connect and the serve factory