From 471113fbcffcbdf29bda013d56943737e91eaaf5 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 9 Oct 2026 19:17:05 +0900 Subject: [PATCH 1/5] fix: inspect only the requested WIP entity --- crates/worker/src/checkout_http_tests.rs | 5 +- crates/worker/src/checkout_wip_tests.rs | 14 +- .../workspace_config/production_tests.rs | 16 +- crates/worker/src/wip.rs | 137 ++++++-- crates/worker/src/wip/ancestor_tests.rs | 28 +- crates/worker/src/wip/binding.rs | 320 +++++++++++++----- .../worker/src/wip/binding/inspect_target.rs | 151 +++++++++ crates/worker/src/wip/provider_tests.rs | 8 +- .../src/server_workspace_config_wip_tests.rs | 39 ++- docs/design/worker-wip-mode.md | 15 +- 10 files changed, 588 insertions(+), 145 deletions(-) create mode 100644 crates/worker/src/wip/binding/inspect_target.rs diff --git a/crates/worker/src/checkout_http_tests.rs b/crates/worker/src/checkout_http_tests.rs index b69c99d5b..e74e054f6 100644 --- a/crates/worker/src/checkout_http_tests.rs +++ b/crates/worker/src/checkout_http_tests.rs @@ -337,7 +337,10 @@ async fn observe_interface(runtime: &WipRuntime, path: &str) -> wip_protocol::In .interfaces[0] .clone() }; - runtime.inspect(path.into(), true).await.unwrap(); + runtime + .prepare_call_observations(path.into(), true) + .await + .unwrap(); interface } diff --git a/crates/worker/src/checkout_wip_tests.rs b/crates/worker/src/checkout_wip_tests.rs index 89972eefe..c4ca3698f 100644 --- a/crates/worker/src/checkout_wip_tests.rs +++ b/crates/worker/src/checkout_wip_tests.rs @@ -36,7 +36,9 @@ fn runtime( async fn interface(r: &WipRuntime, path: &str) -> wip_protocol::InterfaceReference { r.tree(path.into(), 0, true).await.unwrap(); let p = r.host.projection_live(path).await.unwrap().unwrap(); - r.inspect(path.into(), true).await.unwrap(); + r.prepare_call_observations(path.into(), true) + .await + .unwrap(); p.interface } async fn call( @@ -764,8 +766,14 @@ async fn checkout_model_list_glob_grep_entries_are_lazy_and_never_become_tree_ed entrance["object_signature"], wip_text_view::render_object(&projection.object).unwrap() ); + assert!(entrance["interfaces"][0].get("signature").is_none()); + let definition = model_inspect( + r.clone(), + entrance["interfaces"][0]["path"].as_str().unwrap(), + ) + .await; assert_eq!( - entrance["interfaces"][0]["signature"], + definition["interface_signature"], wip_text_view::render_interface(&wip_text_view::Interface { reference: &projection.interface, descriptor: &projection.descriptor @@ -773,7 +781,7 @@ async fn checkout_model_list_glob_grep_entries_are_lazy_and_never_become_tree_ed .unwrap() ); assert!( - entrance["interfaces"][0]["signature"] + definition["interface_signature"] .as_str() .unwrap() .contains("entry") diff --git a/crates/worker/src/feature/builtin/workspace_config/production_tests.rs b/crates/worker/src/feature/builtin/workspace_config/production_tests.rs index 103db15c7..3473d176d 100644 --- a/crates/worker/src/feature/builtin/workspace_config/production_tests.rs +++ b/crates/worker/src/feature/builtin/workspace_config/production_tests.rs @@ -440,7 +440,10 @@ fn interface(path: &str, attached: bool) -> wip_protocol::InterfaceReference { } async fn observe_interface(runtime: &WipRuntime, path: &str, _attached: bool) { runtime.tree(path.into(), 0, true).await.unwrap(); - runtime.inspect(path.into(), true).await.unwrap(); + runtime + .prepare_call_observations(path.into(), true) + .await + .unwrap(); } async fn call( runtime: &WipRuntime, @@ -630,13 +633,20 @@ async fn workspace_config_production_revocation_ro_cross_workspace_and_cache_iso ); observe_interface(&runtime, path, true).await; let descriptor = runtime - .inspect(path.into(), true) + .inspect( + format!( + "{}::{}", + serde_json::to_string(path).unwrap(), + serde_json::to_string(&interface(path, true).name).unwrap() + ), + true, + ) .await .unwrap() .content .unwrap(); let descriptor: Json = serde_json::from_str(&descriptor).unwrap(); - let signature = descriptor["interfaces"][0]["signature"].as_str().unwrap(); + let signature = descriptor["interface_signature"].as_str().unwrap(); assert!(signature.contains("operation read(")); assert!(!signature.contains("operation write(")); assert!( diff --git a/crates/worker/src/wip.rs b/crates/worker/src/wip.rs index 3c7145ad6..af33f29bc 100644 --- a/crates/worker/src/wip.rs +++ b/crates/worker/src/wip.rs @@ -2884,6 +2884,25 @@ pub(crate) fn wip_to_json(value: &Value) -> Result { } } +// Low-level call() tests explicitly acquire both observation kinds. Unlike +// Invoke, call() deliberately does not perform automatic retrieval. +#[cfg(test)] +impl WipRuntime { + pub(crate) async fn prepare_call_observations( + &self, + path: String, + refresh: bool, + ) -> Result { + let output = self.inspect(path, refresh).await?; + let value: Json = serde_json::from_str(output.content.as_deref().unwrap()).unwrap(); + for interface in value["interfaces"].as_array().unwrap() { + self.inspect(interface["path"].as_str().unwrap().into(), refresh) + .await?; + } + Ok(output) + } +} + #[cfg(test)] mod tests { use super::*; @@ -3092,8 +3111,19 @@ mod tests { assert_eq!(inspected["path"], "/tools/Echo"); let interface = &inspected["interfaces"][0]["reference"]; assert_eq!(interface, &json!({"scope":"/","name":"yoi.tool/Echo/v1"})); + assert!(inspected["interfaces"][0].get("signature").is_none()); + let definition = transport_output_json( + tools + .call_tool( + "Inspect", + &json!({"path": inspected["interfaces"][0]["path"]}).to_string(), + ToolExecutionContext::direct(), + ) + .await + .unwrap(), + ); assert!( - inspected["interfaces"][0]["signature"] + definition["interface_signature"] .as_str() .unwrap() .contains("operation call(") @@ -3197,7 +3227,10 @@ mod tests { ) .unwrap(); runtime.tree("/assets/A-1".into(), 0, true).await.unwrap(); - runtime.inspect("/assets/A-1".into(), true).await.unwrap(); + runtime + .prepare_call_observations("/assets/A-1".into(), true) + .await + .unwrap(); let error = runtime .call( "/assets/A-1".into(), @@ -3569,7 +3602,7 @@ mod tests { .await .unwrap(); old_runtime - .inspect(old_projection.route.clone(), false) + .prepare_call_observations(old_projection.route.clone(), false) .await .unwrap(); let restored_host = WipHost::new(asset_registry( @@ -3609,7 +3642,7 @@ mod tests { assert_eq!(calls.load(Ordering::SeqCst), 0); restored.tree("/assets/A-1".into(), 0, false).await.unwrap(); restored - .inspect(current.route.clone(), false) + .prepare_call_observations(current.route.clone(), false) .await .unwrap(); restored @@ -4165,7 +4198,10 @@ mod tests { .unwrap(); runtime.tree("/assets/A-1".into(), 0, false).await.unwrap(); let interface = runtime.host.projection("/assets/A-1").unwrap().interface; - runtime.inspect("/assets/A-1".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/assets/A-1".into(), false) + .await + .unwrap(); contributor.permitted.store(false, Ordering::SeqCst); // The Client still knows the old descriptor; the Host must independently // check its current filtered descriptor rather than trust Known Space. @@ -4194,7 +4230,10 @@ mod tests { ); assert_eq!(owner.calls.load(Ordering::SeqCst), 0); assert_eq!(contributor.calls.load(Ordering::SeqCst), 0); - runtime.inspect("/assets/A-1".into(), true).await.unwrap(); + runtime + .prepare_call_observations("/assets/A-1".into(), true) + .await + .unwrap(); runtime .call( "/assets/A-1".into(), @@ -4208,7 +4247,7 @@ mod tests { owner.visible.store(false, Ordering::SeqCst); assert!(runtime.host.fetch_interface(&interface).is_err()); let error = runtime - .inspect("/assets/A-1".into(), true) + .prepare_call_observations("/assets/A-1".into(), true) .await .unwrap_err(); assert!(error.to_string().contains("NotFound")); @@ -4370,7 +4409,10 @@ mod tests { } async fn observed_contextual_interface(runtime: &WipRuntime, path: &str) -> InterfaceReference { - runtime.inspect(path.into(), true).await.unwrap(); + runtime + .prepare_call_observations(path.into(), true) + .await + .unwrap(); let state = runtime.state.lock().unwrap(); state .client @@ -5418,11 +5460,20 @@ mod tests { WipRuntime::new(WipHost::new(registry), SecurityContext::new("worker-a"), 0).unwrap(); runtime.tree("/".into(), 2, false).await.unwrap(); let inspected = runtime - .inspect("/native/typed".into(), false) + .prepare_call_observations("/native/typed".into(), false) .await .unwrap(); let inspected: Json = serde_json::from_str(inspected.content.as_deref().unwrap()).unwrap(); - let signature = inspected["interfaces"][0]["signature"].as_str().unwrap(); + let definition = runtime + .inspect( + inspected["interfaces"][0]["path"].as_str().unwrap().into(), + false, + ) + .await + .unwrap(); + let definition: Json = + serde_json::from_str(definition.content.as_deref().unwrap()).unwrap(); + let signature = definition["interface_signature"].as_str().unwrap(); assert!(signature.contains("type Payload")); assert!(signature.contains("red tag")); assert!(signature.contains("tags?: [enum")); @@ -5476,7 +5527,10 @@ mod tests { let runtime = WipRuntime::new(host, SecurityContext::new("worker-a"), 4096).unwrap(); runtime.tree("/".into(), 2, false).await.unwrap(); - runtime.inspect("/tools/Echo".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/tools/Echo".into(), false) + .await + .unwrap(); let output = runtime .call( "/tools/Echo".into(), @@ -5514,7 +5568,10 @@ mod tests { let host = WipHost::new(registry_with_tool("Echo", Arc::clone(&calls), None)); let runtime = WipRuntime::new(host, SecurityContext::new("worker-a"), 0).unwrap(); runtime.tree("/".into(), 2, false).await.unwrap(); - runtime.inspect("/tools/Echo".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/tools/Echo".into(), false) + .await + .unwrap(); let projection = runtime.host.projection("/tools/Echo").unwrap(); let mut request = CallOperationRequest { @@ -5963,7 +6020,10 @@ mod tests { assert!(!discovered.contains("/features")); assert!(!discovered.contains("/tools/QueryTicket")); assert!(!discovered.contains("/tools/TicketCreate")); - authoring.inspect("/tickets".into(), false).await.unwrap(); + authoring + .prepare_call_observations("/tickets".into(), false) + .await + .unwrap(); let collection = "/tickets"; authoring .call( @@ -5988,7 +6048,10 @@ mod tests { assert!(created.content.unwrap().contains("/tickets/T-9")); let item = "/tickets/T-9"; authoring.tree(item.into(), 0, false).await.unwrap(); - authoring.inspect(item.into(), false).await.unwrap(); + authoring + .prepare_call_observations(item.into(), false) + .await + .unwrap(); authoring .call( item.into(), @@ -6059,7 +6122,10 @@ mod tests { ) .unwrap(); workflow.tree(item.into(), 0, false).await.unwrap(); - workflow.inspect(item.into(), false).await.unwrap(); + workflow + .prepare_call_observations(item.into(), false) + .await + .unwrap(); let result = workflow .call( item.into(), @@ -6163,7 +6229,7 @@ mod tests { assert!(!discovered.contains("/tools/OpenMergeRequest")); assert!(!discovered.contains("/tools/ShowMergeRequest")); coder - .inspect("/merge-requests".into(), false) + .prepare_call_observations("/merge-requests".into(), false) .await .unwrap(); let collection = "/merge-requests"; @@ -6190,7 +6256,10 @@ mod tests { ); let item = format!("{collection}/MR-1"); coder.tree(item.clone(), 0, false).await.unwrap(); - coder.inspect(item.clone(), false).await.unwrap(); + coder + .prepare_call_observations(item.clone(), false) + .await + .unwrap(); coder .call( item.clone(), @@ -6277,7 +6346,10 @@ mod tests { ) .unwrap(); reviewer.tree(item.clone(), 0, false).await.unwrap(); - reviewer.inspect(item.clone(), false).await.unwrap(); + reviewer + .prepare_call_observations(item.clone(), false) + .await + .unwrap(); reviewer .call( item.clone(), @@ -6352,7 +6424,10 @@ mod tests { ) .unwrap(); orchestrator.tree(item.clone(), 0, false).await.unwrap(); - orchestrator.inspect(item.clone(), false).await.unwrap(); + orchestrator + .prepare_call_observations(item.clone(), false) + .await + .unwrap(); orchestrator .call( item.clone(), @@ -6386,7 +6461,7 @@ mod tests { .await .unwrap(); orchestrator - .inspect("/merge-requests".into(), false) + .prepare_call_observations("/merge-requests".into(), false) .await .unwrap(); let rejected = orchestrator @@ -6491,7 +6566,10 @@ mod tests { let discovered = discovered.content.unwrap(); assert!(discovered.contains("/objectives")); assert!(!discovered.contains("/tools/QueryObjective")); - runtime.inspect("/objectives".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/objectives".into(), false) + .await + .unwrap(); let collection_path = "/objectives"; runtime .call( @@ -6529,7 +6607,10 @@ mod tests { let item_path = "/objectives/O-3"; runtime.tree(item_path.into(), 0, false).await.unwrap(); - runtime.inspect(item_path.into(), false).await.unwrap(); + runtime + .prepare_call_observations(item_path.into(), false) + .await + .unwrap(); for (operation, arguments) in [ ("read", json!({})), ("edit", json!({"title": "Changed"})), @@ -6590,7 +6671,7 @@ mod tests { WipRuntime::new(WipHost::new(registry), SecurityContext::new("worker-a"), 0).unwrap(); runtime.tree("/".into(), 2, false).await.unwrap(); runtime - .inspect(format!("/tools/{name}"), false) + .prepare_call_observations(format!("/tools/{name}"), false) .await .unwrap(); runtime @@ -6754,7 +6835,10 @@ mod tests { let runtime = WipRuntime::new(WipHost::new(registry), SecurityContext::new("worker-a"), 0).unwrap(); runtime.tree("/assets/A-1".into(), 0, false).await.unwrap(); - runtime.inspect("/assets/A-1".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/assets/A-1".into(), false) + .await + .unwrap(); let execution = ToolExecutionContext::direct(); let call = runtime.call( "/assets/A-1".into(), @@ -6893,7 +6977,10 @@ mod tests { )); let runtime = WipRuntime::new(host, SecurityContext::new("worker-a"), 0).unwrap(); runtime.tree("/".into(), 2, false).await.unwrap(); - runtime.inspect("/tools/Echo".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/tools/Echo".into(), false) + .await + .unwrap(); let error = runtime .call( "/tools/Echo".into(), diff --git a/crates/worker/src/wip/ancestor_tests.rs b/crates/worker/src/wip/ancestor_tests.rs index d645a3436..a61d53301 100644 --- a/crates/worker/src/wip/ancestor_tests.rs +++ b/crates/worker/src/wip/ancestor_tests.rs @@ -160,7 +160,7 @@ async fn live_root_and_intermediate_ancestors_use_one_current_publication_not_pl let fetched = runtime.host.fetch_interface_live(&selected).await.unwrap(); assert_eq!(fetched.scope_ref, with_ref.then(|| "live-scope".into())); runtime - .inspect(provider.target_path.clone(), false) + .prepare_call_observations(provider.target_path.clone(), false) .await .unwrap(); provider.state.lock().unwrap().lookups.clear(); @@ -217,7 +217,7 @@ async fn deletion_without_prior_scope_inspect_rejects_cached_invoke_and_orphan_o let (runtime, provider) = fixture(scope, with_ref, false); let selected = reference(scope, "read"); runtime - .inspect(provider.target_path.clone(), false) + .prepare_call_observations(provider.target_path.clone(), false) .await .unwrap(); provider.state.lock().unwrap().visible = false; @@ -235,7 +235,7 @@ async fn deletion_without_prior_scope_inspect_rejects_cached_invoke_and_orphan_o assert_eq!(provider.calls.load(Ordering::SeqCst), 0); assert!( runtime - .inspect(provider.target_path.clone(), true) + .prepare_call_observations(provider.target_path.clone(), true) .await .is_err() ); @@ -267,7 +267,7 @@ async fn republishing_scope_never_restores_old_registration_even_without_refs() let (runtime, provider) = fixture("/github", with_ref, false); let old = reference("/github", "read"); runtime - .inspect(provider.target_path.clone(), false) + .prepare_call_observations(provider.target_path.clone(), false) .await .unwrap(); provider.state.lock().unwrap().visible = false; @@ -311,7 +311,7 @@ async fn republishing_scope_never_restores_old_registration_even_without_refs() ); assert_eq!(provider.calls.load(Ordering::SeqCst), 0); runtime - .inspect(provider.target_path.clone(), true) + .prepare_call_observations(provider.target_path.clone(), true) .await .unwrap(); let new = reference("/github", "new"); @@ -401,7 +401,7 @@ async fn ancestor_replacement_during_in_flight_call_keeps_the_original_descripto let (runtime, provider) = fixture("/github/team", true, true); let selected = reference("/github/team", "read"); runtime - .inspect(provider.target_path.clone(), false) + .prepare_call_observations(provider.target_path.clone(), false) .await .unwrap(); let pending = runtime.invoke( @@ -419,10 +419,18 @@ async fn ancestor_replacement_during_in_flight_call_keeps_the_original_descripto state.generation = 1; } let updated = runtime - .inspect(provider.target_path.clone(), true) + .prepare_call_observations(provider.target_path.clone(), true) .await .unwrap(); - assert!(updated.content.unwrap().contains("integer")); + let updated: Json = serde_json::from_str(updated.content.as_deref().unwrap()).unwrap(); + let definition = runtime + .inspect( + updated["interfaces"][0]["path"].as_str().unwrap().into(), + false, + ) + .await + .unwrap(); + assert!(definition.content.unwrap().contains("integer")); provider.release.notify_one(); }; let (old, ()) = tokio::time::timeout(std::time::Duration::from_secs(5), async { @@ -464,7 +472,7 @@ async fn same_name_scope_replacement_or_loss_of_ref_rejects_old_metadata_without let (runtime, provider) = fixture("/github/team", true, false); let selected = reference("/github/team", "read"); runtime - .inspect(provider.target_path.clone(), false) + .prepare_call_observations(provider.target_path.clone(), false) .await .unwrap(); { @@ -510,7 +518,7 @@ async fn same_name_scope_replacement_or_loss_of_ref_rejects_old_metadata_without } ); runtime - .inspect(provider.target_path.clone(), true) + .prepare_call_observations(provider.target_path.clone(), true) .await .unwrap(); runtime diff --git a/crates/worker/src/wip/binding.rs b/crates/worker/src/wip/binding.rs index 9f20bca3e..917238fa9 100644 --- a/crates/worker/src/wip/binding.rs +++ b/crates/worker/src/wip/binding.rs @@ -1,4 +1,4 @@ -//! Object-centered AI binding. Retrieval, display and invocation are separate: +//! Target-specific AI binding. Retrieval, display and invocation are separate: //! signatures are untrusted display data, never parsed back into call targets. use super::*; @@ -6,6 +6,8 @@ use wip_client::{InterfaceObservation, PreparedRequest}; use wip_protocol::InterfaceReference; use wip_text_view::{Interface, render_interface, render_object}; +mod inspect_target; + const MAX_TREE_DEPTH: u32 = 8; const MAX_TREE_NODES: usize = 2048; @@ -293,79 +295,66 @@ impl WipRuntime { ) } - /// Direct Object acquisition plus every published Interface in Host order. - /// Path context is a JSON string outside the upstream path-free signatures. + /// Inspect exactly one Object or explicitly addressed Interface. + /// Compact references are decoded only at this input boundary, never from signatures. pub async fn inspect(&self, path: String, refresh: bool) -> Result { - self.binding_observe(&path, 0, refresh, false).await?; - let object_observation = { - let state = self.state.lock().unwrap_or_else(|error| error.into_inner()); - fresh_object(&state, &path).map_err(|issue| issue.error("inspect object"))? - }; - let object = object_observation - .object - .as_ref() - .expect("fresh_object requires an Object"); - let object_signature = render_object(object) - .map_err(|error| binding_render_error(json!({"path": path}), error))?; - let mut bytes = object_signature.len(); - let mut observations = Vec::new(); - let mut interfaces = Vec::new(); - for reference in &object.interfaces { - let observation = self.binding_interface(reference, refresh, false).await?; - let signature = render_interface(&Interface { - reference, - descriptor: observation - .descriptor - .as_ref() - .expect("fresh_interface requires a descriptor"), - }) - .map_err(|error| { - binding_render_error( - json!({"path": path, "reference": reference_json(reference)}), - error, + match inspect_target::parse(&path)? { + inspect_target::Target::Object(object_path) => { + self.binding_observe(&object_path, 0, refresh, false) + .await?; + let observation = { + let state = self.state.lock().unwrap_or_else(|error| error.into_inner()); + fresh_object(&state, &object_path) + .map_err(|issue| issue.error("inspect object"))? + }; + let object = observation.object.as_ref().expect("fresh Object"); + let signature = render_object(object) + .map_err(|error| binding_render_error(json!({"path": path}), error))?; + let interfaces: Vec<_> = object + .interfaces + .iter() + .map(|reference| { + json!({ + "reference": reference_json(reference), + "path": inspect_target::interface_path(reference), + }) + }) + .collect(); + self.binding_output( + "Inspected WIP Object".into(), + json!({ + "path": object_path, + "object_signature": signature, + "interfaces": interfaces, + "metrics": metrics_json(self.metrics.snapshot()), + }), ) - })?; - bytes = bytes.saturating_add(signature.len()); - if bytes > self.wire_limits.max_response_bytes() { - return Err(ToolError::ExecutionFailed( - "complete Inspect signatures exceed the binding response limit".into(), - )); } - interfaces - .push(json!({"reference": reference_json(reference), "signature": signature})); - observations.push(observation); - } - // Later interface completions may invalidate earlier scope bindings or - // evict data. Never return those earlier snapshots as a coherent Inspect. - { - let state = self.state.lock().unwrap_or_else(|error| error.into_inner()); - if fresh_object(&state, &path).map_err(|issue| issue.error("inspect final object"))? - != object_observation - { - return Err(ToolError::ExecutionFailed( - "WIP Inspect target changed during acquisition; refresh".into(), - )); - } - for observation in &observations { - if fresh_interface(&state, &observation.reference) - .map_err(|issue| issue.error("inspect final interface"))? - != *observation - { - return Err(ToolError::ExecutionFailed( - "WIP Inspect interface changed during acquisition; refresh".into(), - )); - } + inspect_target::Target::Interface(reference) => { + let observation = self.binding_interface(&reference, refresh, false).await?; + let signature = render_interface(&Interface { + reference: &reference, + descriptor: observation.descriptor.as_ref().expect("fresh Interface"), + }) + .map_err(|error| { + binding_render_error( + json!({ + "path": path, "reference": reference_json(&reference), + }), + error, + ) + })?; + self.binding_output( + "Inspected WIP Interface".into(), + json!({ + "path": inspect_target::interface_path(&reference), + "reference": reference_json(&reference), + "interface_signature": signature, + "metrics": metrics_json(self.metrics.snapshot()), + }), + ) } } - self.binding_output( - "Inspected WIP Object and all published Interfaces".into(), - json!({ - "path": path, - "object_signature": object_signature, - "interfaces": interfaces, - "metrics": metrics_json(self.metrics.snapshot()), - }), - ) } /// Recover only observations before dispatch. The parent call owns argument @@ -468,9 +457,11 @@ fn binding_one() -> u32 { #[derive(Debug, Deserialize, schemars::JsonSchema)] #[serde(deny_unknown_fields)] struct WipInspectInput { - /// Canonical Object path; may be supplied directly without a prior Tree. + /// Object path, or absolute Interface address scope::name (e.g. /::example). + /// JSON-quote special reference components; quote an entire Object path containing ::. + /// No prior Tree or Inspect is required; only the selected entity is displayed. path: String, - /// Explicitly refresh the Object and every published Interface. + /// Explicitly refresh only the selected Object or Interface. #[serde(default)] refresh: bool, } @@ -844,7 +835,7 @@ mod tests { } #[tokio::test] - async fn direct_inspect_acquires_object_and_full_signatures_from_empty_observations() { + async fn direct_inspect_only_acquires_and_displays_the_selected_entity() { let fixture = fixture("/tools/item", reference("/", "fixture")); let output = content( fixture @@ -860,7 +851,26 @@ mod tests { .unwrap() .contains("Object description") ); - let signature = output["interfaces"][0]["signature"].as_str().unwrap(); + assert!(output["interfaces"][0].get("signature").is_none()); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 0); + assert_eq!(output["interfaces"][0]["path"], "/::fixture"); + let definition = content( + fixture + .runtime + .inspect( + output["interfaces"][0]["path"].as_str().unwrap().into(), + false, + ) + .await + .unwrap(), + ); + assert!(definition.get("object_signature").is_none()); + assert!(definition.get("interfaces").is_none()); + assert_eq!( + definition["reference"], + reference_json(&reference("/", "fixture")) + ); + let signature = definition["interface_signature"].as_str().unwrap(); let expected = render_interface(&Interface { reference: &reference("/", "fixture"), descriptor: &descriptor(), @@ -879,9 +889,109 @@ mod tests { .unwrap(); assert_eq!(fixture.runtime.metrics().discover_round_trips, 1); assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); + fixture + .runtime + .inspect("/::fixture".into(), false) + .await + .unwrap(); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); + fixture + .runtime + .inspect("/tools/item".into(), true) + .await + .unwrap(); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); assert_eq!(fixture.calls.load(Ordering::SeqCst), 0); } + #[tokio::test] + async fn interface_inspect_from_empty_cache_never_observes_objects_and_refresh_is_scoped() { + let fixture = fixture("/tools/item", reference("/", "fixture")); + let tools = model_tools(Arc::clone(&fixture.runtime)); + let definition = content( + model_execute(&tools, "Inspect", json!({"path": "/::fixture"})) + .await + .unwrap(), + ); + assert!( + definition["interface_signature"] + .as_str() + .unwrap() + .contains("read(") + ); + assert!(definition.get("object_signature").is_none()); + assert!(definition.get("interfaces").is_none()); + assert_eq!(fixture.runtime.metrics().discover_round_trips, 0); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); + model_execute( + &tools, + "Inspect", + json!({"path": "/::fixture", "refresh": true}), + ) + .await + .unwrap(); + assert_eq!(fixture.runtime.metrics().discover_round_trips, 0); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 2); + assert!( + model_execute(&tools, "Inspect", json!({"path": "/::missing"})) + .await + .is_err() + ); + assert_eq!(fixture.runtime.metrics().discover_round_trips, 0); + assert_eq!(fixture.calls.load(Ordering::SeqCst), 0); + } + + #[tokio::test] + async fn failed_interface_does_not_block_or_get_refreshed_by_object_inspect() { + let selected = reference("/", "fixture"); + let fixture = fixture("/tools/item", selected.clone()); + fail_interface(&fixture.runtime, selected); + for refresh in [false, true] { + let object = content( + fixture + .runtime + .inspect("/tools/item".into(), refresh) + .await + .unwrap(), + ); + assert!(object.get("object_signature").is_some()); + assert!(object["interfaces"][0].get("signature").is_none()); + } + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 0); + assert!( + fixture + .runtime + .inspect("/::fixture".into(), false) + .await + .is_err() + ); + fixture + .runtime + .inspect("/::fixture".into(), true) + .await + .unwrap(); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); + } + + #[tokio::test] + async fn invalid_inspect_addresses_fail_before_any_host_retrieval() { + let fixture = fixture("/tools/item", reference("/", "fixture")); + for path in [ + "relative::fixture", + "/::", + "/::fixture#read", + "/::\"broken", + "/::same::name", + ] { + assert!(matches!( + fixture.runtime.inspect(path.into(), false).await, + Err(ToolError::InvalidArgument(_)) + )); + } + assert_eq!(fixture.runtime.metrics().discover_round_trips, 0); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 0); + } + #[tokio::test] async fn inspect_path_context_is_json_escaped_and_not_inserted_into_signatures() { let path = "/tools/item\"\n} injected"; @@ -951,8 +1061,14 @@ mod tests { assert_eq!(interfaces[0]["reference"], reference_json(&global)); assert_eq!(interfaces[1]["reference"], reference_json(&local)); for (shown, reference) in interfaces.iter().zip([global, local]) { + assert!(shown.get("signature").is_none()); + let definition = content( + model_execute(&tools, "Inspect", json!({"path": shown["path"]})) + .await + .unwrap(), + ); assert_eq!( - shown["signature"], + definition["interface_signature"], render_interface(&Interface { reference: &reference, descriptor: &descriptor() @@ -1144,14 +1260,14 @@ mod tests { assert!( fixture .runtime - .inspect("/tools/item".into(), false) + .inspect("/::fixture".into(), false) .await .is_err() ); assert_eq!(fixture.runtime.metrics().inspect_round_trips, 0); fixture .runtime - .inspect("/tools/item".into(), true) + .inspect("/::fixture".into(), true) .await .unwrap(); assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); @@ -1201,14 +1317,14 @@ mod tests { assert!( fixture .runtime - .inspect("/tools/item".into(), false) + .inspect("/::fixture".into(), false) .await .is_err() ); assert_eq!(fixture.runtime.metrics().inspect_round_trips, 0); fixture .runtime - .inspect("/tools/item".into(), true) + .inspect("/::fixture".into(), true) .await .unwrap(); assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); @@ -1240,7 +1356,7 @@ mod tests { let fixture = fixture("/tools/item", selected.clone()); fixture .runtime - .inspect("/tools/item".into(), false) + .inspect("/tools/item::fixture".into(), false) .await .unwrap(); // A new scope identity retires the previously acquired descriptor. @@ -1256,13 +1372,13 @@ mod tests { assert!( fixture .runtime - .inspect("/tools/item".into(), false) + .inspect("/tools/item::fixture".into(), false) .await .is_err() ); fixture .runtime - .inspect("/tools/item".into(), true) + .inspect("/tools/item::fixture".into(), true) .await .unwrap(); } @@ -1411,11 +1527,20 @@ mod tests { children: None, }, ); - assert!( + let object = content( fixture .runtime .inspect("/tools/item".into(), false) .await + .unwrap(), + ); + assert_eq!(fixture.runtime.metrics().inspect_round_trips, 0); + assert_eq!(object["interfaces"][0]["path"], "/::missing"); + assert!( + fixture + .runtime + .inspect("/::missing".into(), false) + .await .is_err() ); assert_eq!(fixture.runtime.metrics().inspect_round_trips, 1); @@ -1494,7 +1619,7 @@ mod tests { } #[tokio::test] - async fn inspect_fetches_all_missing_interfaces_without_flattening_operations() { + async fn object_inspect_lists_references_without_fetching_any_interface() { let global = reference("/", "same"); let local = reference("/tools/item", "same"); let mut registry = WipMountRegistry::new(); @@ -1518,14 +1643,26 @@ mod tests { let runtime = WipRuntime::from_mounts(registry, "binding-test".into()).unwrap(); let output = content(runtime.inspect("/tools/item".into(), false).await.unwrap()); assert_eq!(runtime.metrics().discover_round_trips, 1); - assert_eq!(runtime.metrics().inspect_round_trips, 2); + assert_eq!(runtime.metrics().inspect_round_trips, 0); assert_eq!( output["interfaces"][0]["reference"], reference_json(&global) ); assert_eq!(output["interfaces"][1]["reference"], reference_json(&local)); for shown in output["interfaces"].as_array().unwrap() { - assert!(shown["signature"].as_str().unwrap().contains("read(")); + assert!(shown.get("signature").is_none()); + let definition = content( + runtime + .inspect(shown["path"].as_str().unwrap().into(), false) + .await + .unwrap(), + ); + assert!( + definition["interface_signature"] + .as_str() + .unwrap() + .contains("read(") + ); } } @@ -1724,8 +1861,17 @@ mod tests { catalog["interfaces"][0]["reference"], reference_json(&collection_reference) ); + let definition = content( + model_execute( + &tools, + "Inspect", + json!({"path": catalog["interfaces"][0]["path"]}), + ) + .await + .unwrap(), + ); assert_eq!( - catalog["interfaces"][0]["signature"], + definition["interface_signature"], render_interface(&Interface { reference: &collection_reference, descriptor: &entry_descriptor( @@ -1910,7 +2056,7 @@ pub(super) fn wip_tool_definitions(runtime: Arc) -> Vec, ) diff --git a/crates/worker/src/wip/binding/inspect_target.rs b/crates/worker/src/wip/binding/inspect_target.rs new file mode 100644 index 000000000..6ebd6b867 --- /dev/null +++ b/crates/worker/src/wip/binding/inspect_target.rs @@ -0,0 +1,151 @@ +//! Input-only compact Interface addresses. Never parse rendered signatures. +use super::{InterfaceReference, ToolError}; + +#[derive(Debug, PartialEq, Eq)] +pub(super) enum Target { + Object(String), + Interface(InterfaceReference), +} + +fn invalid() -> ToolError { + ToolError::InvalidArgument( + "Inspect requires an absolute Object path or scope::name Interface address; JSON-quote special components, and quote an entire Object path containing ::. Operation suffixes are not Inspect targets.".into(), + ) +} + +fn quoted(input: &str) -> Result<(String, &str), ToolError> { + let mut stream = serde_json::Deserializer::from_str(input).into_iter::(); + let value = stream.next().ok_or_else(invalid)?.map_err(|_| invalid())?; + Ok((value, &input[stream.byte_offset()..])) +} + +fn bare(value: &str, scope: bool) -> bool { + !value.is_empty() + && value + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b"_.-".contains(&b) || (scope && b == b'/')) +} + +pub(super) fn parse(input: &str) -> Result { + let (scope, remainder) = if input.starts_with('"') { + quoted(input)? + } else if let Some(boundary) = input.find("::") { + let (scope, remainder) = input.split_at(boundary); + if !bare(scope, true) { + return Err(invalid()); + } + (scope.to_owned(), remainder) + } else { + // Ordinary Object paths are Protocol paths, not restricted Text View tokens. + wip_protocol::validate_path(input).map_err(|_| invalid())?; + return Ok(Target::Object(input.to_owned())); + }; + wip_protocol::validate_path(&scope).map_err(|_| invalid())?; + if remainder.is_empty() { + return Ok(Target::Object(scope)); + } + let name = remainder.strip_prefix("::").ok_or_else(invalid)?; + let name = if name.starts_with('"') { + let (value, rest) = quoted(name)?; + if !rest.is_empty() { + return Err(invalid()); + } + value + } else { + if !bare(name, false) { + return Err(invalid()); + } + name.to_owned() + }; + let reference = InterfaceReference { scope, name }; + reference.validate().map_err(|_| invalid())?; + Ok(Target::Interface(reference)) +} + +pub(super) fn interface_path(reference: &InterfaceReference) -> String { + fn component(value: &str, scope: bool) -> String { + if bare(value, scope) { + value.to_owned() + } else { + serde_json::to_string(value).expect("string serializes") + } + } + format!( + "{}::{}", + component(&reference.scope, true), + component(&reference.name, false) + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn compact_addresses_roundtrip_exact_components_without_normalization() { + for scope in ["/", "/github", "/github::archive", "/開発", "/x\"\n"] { + for name in [ + "issue.read", + "same::name", + "yoi.tool/Read/v1", + "課題#read", + "x\"\\\n", + ] { + let reference = InterfaceReference { + scope: scope.into(), + name: name.into(), + }; + assert_eq!( + parse(&interface_path(&reference)).unwrap(), + Target::Interface(reference) + ); + } + } + assert_eq!( + parse("\"/github\"::\"issue.read\"").unwrap(), + Target::Interface(InterfaceReference { + scope: "/github".into(), + name: "issue.read".into() + }) + ); + } + + #[test] + fn object_paths_are_not_interfaces_and_quoted_delimiters_remain_literal() { + for path in ["/", "/tools/Read", "/開発", "/x\"\n", "/item#read"] { + assert_eq!(parse(path).unwrap(), Target::Object(path.into())); + assert_eq!( + parse(&serde_json::to_string(path).unwrap()).unwrap(), + Target::Object(path.into()) + ); + } + assert_eq!( + parse("\"/literal::name\"").unwrap(), + Target::Object("/literal::name".into()) + ); + } + + #[test] + fn invalid_or_ambiguous_addresses_do_not_infer_scope_or_operation() { + for input in [ + "", + "relative", + "relative::name", + "::name", + "/::", + "/::\"\"", + "/::name#read", + "/::name::other", + "/::unquoted/name", + "/::\"name\"#read", + "\"/\"junk", + "\"/unterminated", + "/::\"bad\\q\"", + "/bad/../scope::name", + "/:: name", + "/::\"name\" ", + ] { + assert!(parse(input).is_err(), "accepted {input:?}"); + } + } +} diff --git a/crates/worker/src/wip/provider_tests.rs b/crates/worker/src/wip/provider_tests.rs index b6e334dc5..a0e3bb106 100644 --- a/crates/worker/src/wip/provider_tests.rs +++ b/crates/worker/src/wip/provider_tests.rs @@ -311,7 +311,10 @@ async fn current_self_scope_fetch_does_not_attach_a_new_ref_to_an_old_target_sna .unwrap(); let runtime = WipRuntime::from_mounts(registry, "scope-test".into()).unwrap(); let reference = contextual_reference("read", "/github/item"); - runtime.inspect("/github/item".into(), false).await.unwrap(); + runtime + .prepare_call_observations("/github/item".into(), false) + .await + .unwrap(); assert_eq!( runtime .host @@ -524,7 +527,8 @@ async fn in_flight_result_uses_frozen_host_and_client_descriptors_after_refresh( let update = async { started.notified().await; generation.store(1, Ordering::SeqCst); - let output = runtime.inspect("/github".into(), true).await.unwrap(); + runtime.inspect("/github".into(), true).await.unwrap(); + let output = runtime.inspect("/github::read".into(), true).await.unwrap(); assert!(output.content.unwrap().contains("integer")); release.notify_one(); }; diff --git a/crates/workspace-server/src/server_workspace_config_wip_tests.rs b/crates/workspace-server/src/server_workspace_config_wip_tests.rs index b14affe7c..2948e3bf8 100644 --- a/crates/workspace-server/src/server_workspace_config_wip_tests.rs +++ b/crates/workspace-server/src/server_workspace_config_wip_tests.rs @@ -152,18 +152,25 @@ fn config_interface(path: &str) -> wip_protocol::InterfaceReference { async fn inspect_interface(runtime: &worker::wip::WipRuntime, path: &str, base: &str) -> Value { let tree = runtime.tree(path.into(), 0, true).await.unwrap(); let tree: Value = serde_json::from_str(tree.content.as_deref().unwrap()).unwrap(); - assert_eq!(tree["path"], path, "Object must come from live Tree: {tree}"); + assert_eq!( + tree["path"], path, + "Object must come from live Tree: {tree}" + ); let output = runtime.inspect(path.into(), true).await.unwrap(); let observed: Value = serde_json::from_str(output.content.as_deref().unwrap()).unwrap(); assert_eq!(observed["path"], path); + for interface in observed["interfaces"].as_array().unwrap() { + runtime + .inspect(interface["path"].as_str().unwrap().into(), true) + .await + .unwrap(); + } assert!( observed["interfaces"] .as_array() .unwrap() .iter() - .any(|interface| { - interface["reference"] == json!({"scope": path, "name": base}) - }), + .any(|interface| { interface["reference"] == json!({"scope": path, "name": base}) }), "interface must come from live inspection: {observed}" ); observed @@ -628,14 +635,22 @@ async fn assert_config_production_http_adapter( .unwrap(); assert_config_catalog(&wip, &attached).await; let main_interface = config_inspect(&wip, "/workspace-config/main.dcdl").await; - assert!( - main_interface["interfaces"] - .as_array() - .unwrap() - .iter() - .all(|interface| !interface["signature"].as_str().unwrap().contains("delete(")), - "required entrypoint cannot advertise delete: {main_interface}" - ); + for interface in main_interface["interfaces"].as_array().unwrap() { + assert!(interface.get("signature").is_none()); + let definition = wip + .inspect(interface["path"].as_str().unwrap().into(), false) + .await + .unwrap(); + let definition: Value = + serde_json::from_str(definition.content.as_deref().unwrap()).unwrap(); + assert!( + !definition["interface_signature"] + .as_str() + .unwrap() + .contains("delete("), + "required entrypoint cannot advertise delete: {definition}" + ); + } let main = config_call(&wip, "/workspace-config/main.dcdl", "read", json!({})) .await .unwrap(); diff --git a/docs/design/worker-wip-mode.md b/docs/design/worker-wip-mode.md index 40feeeb9d..107bf14ea 100644 --- a/docs/design/worker-wip-mode.md +++ b/docs/design/worker-wip-mode.md @@ -31,10 +31,21 @@ The implementation uses the published **WIP 0.2.0** Client, HTTP, Protocol and o A WIP Worker exposes exactly these generic LLM tools instead of exposing every enabled ordinary tool twice: - `Tree(path, depth)`: retrieve the full indexable range within depth 0..8, or report incomplete/loading/failed/limited coverage as an error. Children at the requested boundary remain unobserved (null), not empty. Cached nonindexable paths are never included by prefix scanning Known Space. -- `Inspect(path)`: directly acquire an Object, then every published Interface, and return complete official Object/Interface signatures in declaration order. It requires no prior Tree or manual descriptor fetching. The path is explicit JSON envelope context outside the path-free signature. Summaries are complete; runtime metadata and documentation details are not appended to signatures. +- `Inspect(path)`: acquire and display exactly the selected entity. An Object path returns only its official Object signature and ordered Interface references (each with an explicit inspection path); it does not fetch Interface descriptors. An absolute `scope::name` Interface address returns only that Interface's complete official signature. Neither form requires a prior Tree or Inspect. The path is explicit JSON envelope context outside the path-free signature. Summaries are complete; runtime metadata and documentation details are not appended to signatures. - `Invoke(path, interface, operation, arguments)`: use a structured `{scope, name}` reference and named argument record. The Client owns validators and observed scope_ref. Missing/stale/failed observations are recovered with at most one pre-dispatch retrieval per subject. A dispatched operation is never automatically retried. -Tree and Inspect accept explicit `refresh: true` for blocked observations. An ensure API returning None is never itself readiness evidence: freshness, complete edge coverage and retained data are checked. Loading and capacity pressure are reported rather than superseded or busy-looped. +Tree and Inspect accept explicit `refresh: true` for blocked observations. Inspect refreshes only the selected Object or Interface, not referenced entities. An ensure API returning None is never itself readiness evidence: freshness, complete edge coverage and retained data are checked. Loading and capacity pressure are reported rather than superseded or busy-looped. + +The Yoi binding uses the Text View compact reference notation as an **input address**, not a parser for rendered signatures. For example: + +```json +{"path":"/tickets"} +{"path":"/::\"yoi.tool/Read/v1\""} +``` + +The first inspects an Object; the second inspects one root-scoped Interface. Object results contain `object_signature` and `interfaces: [{reference: {scope, name}, path}]`, without Interface signatures. Pass an entry's `path` directly to Inspect to obtain `{path, reference, interface_signature}`. This explicit per-entity display selection is a Yoi binding policy; it replaces the earlier automatic expansion of every Interface. Invoke continues to acquire missing observations automatically and accepts structured references, never a compact string. + +Reference components follow Text View quoting: scope allows unquoted `[A-Za-z0-9_./-]+`, name allows `[A-Za-z0-9_.-]+`; other components use JSON string quoting/escapes. There is no scope inference, normalization, relative resolution, or operation suffix. A literal Object path containing `::` must be JSON-quoted inside the `path` value (for example `{"path":"\"/literal::name\""}`) to distinguish it from an Interface address. Ordinary Object paths retain Protocol path syntax, including Unicode. Invalid or unavailable Interface addresses fail rather than falling back to Object lookup. The compatibility projection mounts enabled ordinary tools at: From 2060e8f2c140eb1f5a138035c94f006ed51911e3 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 9 Oct 2026 19:49:06 +0900 Subject: [PATCH 2/5] fix: expose compatibility input contracts and guide WIP usage --- .../src/feature/builtin/workspace_config.rs | 2 +- crates/worker/src/wip.rs | 14 +- crates/worker/src/wip/binding.rs | 28 +- crates/worker/src/wip/compatibility_schema.rs | 409 ++++++++++++++++++ docs/design/worker-wip-mode.md | 10 +- 5 files changed, 453 insertions(+), 10 deletions(-) create mode 100644 crates/worker/src/wip/compatibility_schema.rs diff --git a/crates/worker/src/feature/builtin/workspace_config.rs b/crates/worker/src/feature/builtin/workspace_config.rs index 16a5db133..1549c1792 100644 --- a/crates/worker/src/feature/builtin/workspace_config.rs +++ b/crates/worker/src/feature/builtin/workspace_config.rs @@ -318,7 +318,7 @@ impl FeatureInvocationHandler for WorkspaceConfigFeature { result.access.as_str() ), context: Some(format!( - "Workspace configuration is available as logical attachment `{ATTACHMENT_ALIAS}` at WIP `{CONTENT_ROOT}` with {} access. Use Tree for indexable structure, Inspect with a known Object path for its complete signatures, and Invoke for published Operations. This attachment grants no OS paths or command execution. Backend validation, concurrency checks and activation rules apply; saving does not imply running Workers adopted new configuration.", + "Workspace configuration is available as logical attachment `{ATTACHMENT_ALIAS}` at WIP `{CONTENT_ROOT}` with {} access. Use a shallow Tree only when paths are unknown. Inspect a needed Object to learn its Interface references, then inspect only unknown Interface contracts via their returned paths. Reuse known exact references/contracts and Invoke directly; do not repeat Inspect before each operation. Invoke handles observation freshness. Use published read/list Operations for configuration content, not Inspect. This attachment grants no OS paths or command execution. Backend validation, concurrency checks and activation rules apply; saving does not imply running Workers adopted new configuration.", result.access.as_str() )), }) diff --git a/crates/worker/src/wip.rs b/crates/worker/src/wip.rs index af33f29bc..69f3eb453 100644 --- a/crates/worker/src/wip.rs +++ b/crates/worker/src/wip.rs @@ -45,6 +45,7 @@ use wip_protocol::{ use crate::permission::permission_action_for; mod binding; +mod compatibility_schema; #[cfg(test)] mod provider_tests; use binding::wip_tool_definitions; @@ -2025,10 +2026,12 @@ fn compatibility_projection( name: "input".into(), required: true, documentation: Some(Documentation { - summary: "Original JSON tool arguments".into(), + summary: format!( + "Pass the original tool arguments unchanged as input. The WIP type is a structural guide; JSON-only shapes remain json, not tagged unions. Authoritative input JSON Schema (including all constraints and local definitions): {schema}" + ), details: Some(schema), }), - r#type: TypeExpr::Json, + r#type: compatibility_schema::input_type(&meta.input_schema), }], returns: ReturnDeclaration { documentation: Some(Documentation { @@ -3087,7 +3090,12 @@ mod tests { ["Inspect", "Invoke", "Tree"] ); let metrics = runtime.metrics(); - assert!(metrics.ordinary_schema_bytes > metrics.wip_schema_bytes); + assert!( + metrics.ordinary_schema_bytes > metrics.wip_schema_bytes, + "ordinary={} WIP={}", + metrics.ordinary_schema_bytes, + metrics.wip_schema_bytes + ); } #[tokio::test] diff --git a/crates/worker/src/wip/binding.rs b/crates/worker/src/wip/binding.rs index 917238fa9..37b8017c5 100644 --- a/crates/worker/src/wip/binding.rs +++ b/crates/worker/src/wip/binding.rs @@ -459,7 +459,7 @@ fn binding_one() -> u32 { struct WipInspectInput { /// Object path, or absolute Interface address scope::name (e.g. /::example). /// JSON-quote special reference components; quote an entire Object path containing ::. - /// No prior Tree or Inspect is required; only the selected entity is displayed. + /// No prior discovery required. path: String, /// Explicitly refresh only the selected Object or Interface. #[serde(default)] @@ -2003,6 +2003,26 @@ mod tests { assert!(detail.contains("ResourceLimit")); } + #[test] + fn tool_instructions_reuse_known_contracts_without_inspect_rituals() { + let fixture = fixture("/tools/item", reference("/", "fixture")); + let descriptions: BTreeMap<_, _> = wip_tool_definitions(fixture.runtime) + .into_iter() + .map(|definition| { + let meta = definition().0; + (meta.name, meta.description) + }) + .collect(); + assert!(descriptions["Tree"].contains("Start shallow")); + assert!(descriptions["Tree"].contains("list/search/query")); + assert!(descriptions["Inspect"].contains("Reuse known exact scope/name")); + assert!(descriptions["Inspect"].contains("Do not repeat Inspect before Invoke")); + assert!(descriptions["Inspect"].contains("new/changed/forgotten")); + assert!(descriptions["Inspect"].contains("authoritative JSON Schema")); + assert!(descriptions["Invoke"].contains("no preparatory Tree or Inspect")); + assert!(descriptions["Invoke"].contains("Never automatically retry dispatched operations")); + } + #[test] fn tool_schemas_expose_only_object_centered_names_and_structured_reference_input() { let fixture = fixture("/tools/item", reference("/", "fixture")); @@ -2048,7 +2068,7 @@ pub(super) fn wip_tool_definitions(runtime: Arc) -> Vec, ) @@ -2056,7 +2076,7 @@ pub(super) fn wip_tool_definitions(runtime: Arc) -> Vec, ) @@ -2064,7 +2084,7 @@ pub(super) fn wip_tool_definitions(runtime: Arc) -> Vec, ) diff --git a/crates/worker/src/wip/compatibility_schema.rs b/crates/worker/src/wip/compatibility_schema.rs new file mode 100644 index 000000000..90d56649b --- /dev/null +++ b/crates/worker/src/wip/compatibility_schema.rs @@ -0,0 +1,409 @@ +//! Conservative JSON-shape-preserving types for the compatibility `input` value. +//! The original JSON Schema remains authoritative and visible in Inspect. +//! In particular, open records and JSON unions must not become closed records +//! or WIP's tagged unions: that would reject/change valid original tool inputs. +use super::{Documentation, Json, TypeExpr}; +use wip_protocol::{EnumCase, FieldDeclaration}; + +pub(super) fn input_type(schema: &Json) -> TypeExpr { + project(schema, schema, &mut Vec::new(), 0, &mut 2048) +} + +fn project( + schema: &Json, + root: &Json, + visiting: &mut Vec, + depth: usize, + budget: &mut usize, +) -> TypeExpr { + if depth >= 16 || *budget == 0 { + return TypeExpr::Json; + } + *budget -= 1; + let Some(object) = schema.as_object() else { + return TypeExpr::Json; + }; + // Composition and resource-relative reference semantics are not equivalent + // to WIP's wire types. Keep their exact schema visible instead of guessing. + if [ + "anyOf", + "oneOf", + "allOf", + "$dynamicRef", + "$recursiveRef", + "$id", + "id", + ] + .iter() + .any(|key| object.contains_key(*key)) + { + return TypeExpr::Json; + } + if let Some(reference) = schema.get("$ref").and_then(Json::as_str) { + let Some(pointer) = reference.strip_prefix('#') else { + return TypeExpr::Json; + }; + if (!pointer.is_empty() && !pointer.starts_with('/')) + || visiting.iter().any(|value| value == reference) + { + return TypeExpr::Json; + } + let Some(target) = root.pointer(pointer) else { + return TypeExpr::Json; + }; + visiting.push(reference.into()); + let result = project(target, root, visiting, depth + 1, budget); + visiting.pop(); + return result; + } + match schema.get("type").and_then(Json::as_str) { + Some("null") => TypeExpr::Unit, + Some("boolean") => TypeExpr::Boolean, + Some("integer") => TypeExpr::Integer, + Some("number") => TypeExpr::Number, + Some("string") => { + if let Some(values) = schema.get("enum").and_then(Json::as_array) { + if !values.is_empty() + && values.len() <= *budget + && values.iter().enumerate().all(|(index, value)| { + value.as_str().is_some_and(|s| !s.is_empty()) + && !values[..index].contains(value) + }) + { + *budget -= values.len(); + return TypeExpr::Enum { + cases: values + .iter() + .map(|value| EnumCase { + name: value.as_str().unwrap().into(), + documentation: None, + }) + .collect(), + }; + } + } + TypeExpr::String + } + Some("array") => TypeExpr::List { + items: Box::new( + // Neither legacy tuples nor prefixItems describe one uniform element type. + schema + .get("items") + .filter(|items| items.is_object() && !object.contains_key("prefixItems")) + .map(|items| project(items, root, visiting, depth + 1, budget)) + .unwrap_or(TypeExpr::Json), + ), + }, + Some("object") + if schema.get("additionalProperties") == Some(&Json::Bool(false)) + && !object.contains_key("patternProperties") => + { + let properties = schema.get("properties").and_then(Json::as_object); + if properties.is_some_and(|properties| { + properties.len() > *budget || properties.keys().any(String::is_empty) + }) { + return TypeExpr::Json; + } + // Reserve sibling fields before expanding descendants: a wide early + // child must not spend the allocation already promised to siblings. + *budget -= properties.map_or(0, |properties| properties.len()); + let required = schema.get("required").and_then(Json::as_array); + TypeExpr::Record { + fields: properties + .into_iter() + .flat_map(|properties| properties.iter()) + .map(|(name, value)| FieldDeclaration { + name: name.clone(), + required: required.is_some_and(|required| { + required.iter().any(|field| field.as_str() == Some(name)) + }), + documentation: value.get("description").and_then(Json::as_str).map( + |summary| Documentation { + summary: summary.into(), + details: None, + }, + ), + r#type: project(value, root, visiting, depth + 1, budget), + }) + .collect(), + } + } + _ => TypeExpr::Json, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn closed_records_lists_required_fields_and_enum_are_not_erased() { + let schema = json!({"type":"object","additionalProperties":false,"required":["items"],"properties":{ + "items":{"type":"array","items":{"$ref":"#/$defs/Item"}}, + "limit":{"type":"integer","minimum":1}, + "enabled":{"type":"boolean"} + },"$defs":{"Item":{"type":"object","additionalProperties":false,"required":["kind"],"properties":{"kind":{"type":"string","enum":["file","folder"]},"weight":{"type":"number"}}}}}); + let TypeExpr::Record { fields } = input_type(&schema) else { + panic!("record lost") + }; + let items = fields.iter().find(|field| field.name == "items").unwrap(); + assert!(items.required); + let TypeExpr::List { items } = &items.r#type else { + panic!("list lost") + }; + let TypeExpr::Record { fields: nested } = items.as_ref() else { + panic!("local ref lost") + }; + assert!(matches!(nested[0].r#type, TypeExpr::Enum { .. })); + assert_eq!( + fields + .iter() + .find(|field| field.name == "limit") + .unwrap() + .r#type, + TypeExpr::Integer + ); + assert!( + !fields + .iter() + .find(|field| field.name == "limit") + .unwrap() + .required + ); + } + + #[test] + fn unrepresentable_json_shapes_remain_json_not_invented_tagged_values() { + for schema in [ + json!({"type":"object","properties":{"known":{"type":"string"}}}), + json!({"type":"object","additionalProperties":{"type":"integer"}}), + json!({"type":["string","null"]}), + json!({"anyOf":[{"type":"string"},{"type":"null"}]}), + json!({"oneOf":[{"type":"string"},{"type":"integer"}]}), + json!({"allOf":[{"type":"object"}]}), + json!({"$ref":"#"}), + json!({"$ref":"https://example.invalid/schema"}), + json!({"type":"object","additionalProperties":false,"patternProperties":{"^x":{"type":"string"}}}), + ] { + assert_eq!(input_type(&schema), TypeExpr::Json, "{schema}"); + } + assert_eq!( + input_type(&json!({"type":"array","items":[{"type":"string"},{"type":"integer"}]})), + TypeExpr::List { + items: Box::new(TypeExpr::Json) + } + ); + } + + #[test] + fn acyclic_reference_expansion_is_bounded_before_allocating_siblings() { + let mut definitions = serde_json::Map::new(); + for depth in 0..12 { + let properties: serde_json::Map = (0..40) + .map(|index| { + ( + format!("field{index}"), + if depth == 11 { + json!({"type":"string"}) + } else { + json!({"$ref":format!("#/$defs/Level{}", depth + 1)}) + }, + ) + }) + .collect(); + definitions.insert( + format!("Level{depth}"), + json!({"type":"object","additionalProperties":false,"properties":properties}), + ); + } + let projected = input_type(&json!({"$ref":"#/$defs/Level0","$defs":definitions})); + fn nodes(ty: &TypeExpr) -> usize { + match ty { + TypeExpr::Record { fields } => { + 1 + fields + .iter() + .map(|field| 1 + nodes(&field.r#type)) + .sum::() + } + TypeExpr::List { items } => 1 + nodes(items), + _ => 1, + } + } + assert!(nodes(&projected) < 4096); + assert!(matches!(projected, TypeExpr::Record { .. })); + } + + #[test] + fn recursive_refs_terminate_without_losing_outer_fields() { + let schema = json!({"$ref":"#/$defs/Node","$defs":{"Node":{"type":"object","additionalProperties":false,"properties":{"next":{"$ref":"#/$defs/Node"}}}}}); + let TypeExpr::Record { fields } = input_type(&schema) else { + panic!("outer record lost") + }; + assert_eq!(fields[0].r#type, TypeExpr::Json); + } +} + +#[cfg(test)] +mod integration_tests { + use super::super::*; + use serde_json::json; + use std::sync::atomic::{AtomicUsize, Ordering}; + + struct EchoInput(Arc); + #[async_trait] + impl Tool for EchoInput { + async fn execute( + &self, + input: &str, + _: ToolExecutionContext, + ) -> Result { + self.0.fetch_add(1, Ordering::SeqCst); + Ok(ToolOutput { + summary: "Echo input".into(), + content: Some(input.into()), + attachments: Vec::new(), + }) + } + } + fn runtime(schema: Json) -> (WipRuntime, Arc) { + let calls = Arc::new(AtomicUsize::new(0)); + let projection = compatibility_projection( + ToolMeta::new("Fixture") + .description("Typed compatibility fixture") + .input_schema(schema), + Arc::new(EchoInput(calls.clone())), + None, + ) + .unwrap(); + let mut registry = WipMountRegistry::new(); + registry.mount(projection).unwrap(); + ( + WipRuntime::from_mounts(registry, "schema-test".into()).unwrap(), + calls, + ) + } + async fn invoke(runtime: &WipRuntime, input: Json) -> Result { + runtime + .invoke( + "/tools/Fixture".into(), + root_reference("yoi.tool/Fixture/v1"), + "call".into(), + json!({"input":input}), + ToolExecutionContext::direct(), + ) + .await + } + + #[tokio::test] + async fn inspect_exposes_nested_types_and_exact_constraints_and_invoke_preserves_json() { + let schema = json!({ + "type":"object", "additionalProperties":false, "required":["items","mode","nullable","attributes"], + "properties":{ + "items":{"type":"array","minItems":1,"items":{"$ref":"#/$defs/Item"}}, + "mode":{"type":"string","enum":["fast","safe"]}, + "limit":{"type":"integer","minimum":1,"maximum":10,"description":"Bounded count"}, + "nullable":{"anyOf":[{"type":"string"},{"type":"null"}]}, + "attributes":{"type":"object","additionalProperties":{"type":"integer"}} + }, + "$defs":{"Item":{"type":"object","additionalProperties":false,"required":["name"],"properties":{"name":{"type":"string","minLength":2,"pattern":"^[a-z]+$"}}}} + }); + let (runtime, calls) = runtime(schema.clone()); + let output = runtime + .inspect("/::\"yoi.tool/Fixture/v1\"".into(), false) + .await + .unwrap(); + let inspected: Json = serde_json::from_str(output.content.as_deref().unwrap()).unwrap(); + let signature = inspected["interface_signature"].as_str().unwrap(); + for text in [ + "items", + "mode", + "enum", + "limit?: integer", + "Bounded count", + "nullable", + "anyOf", + "attributes", + "additionalProperties", + "minItems", + "minimum", + "maximum", + "minLength", + "pattern", + "$defs", + ] { + assert!(signature.contains(text), "missing {text}: {signature}"); + } + // Full schema is in the rendered summary, not exclusively hidden details. + let escaped_schema = serde_json::to_string(&schema.to_string()).unwrap(); + assert!(signature.contains(&escaped_schema[1..escaped_schema.len() - 1])); + assert_eq!(runtime.metrics().discover_round_trips, 0); + let good = json!({"items":[{"name":"entry"}],"mode":"safe","nullable":null,"attributes":{"arbitrary-key":4},"limit":3}); + let output = invoke(&runtime, good.clone()).await.unwrap(); + assert_eq!( + serde_json::from_str::(output.content.as_deref().unwrap()).unwrap(), + good + ); + assert_eq!(calls.load(Ordering::SeqCst), 1); + for invalid in [ + json!({"items":[{"name":"x"}],"mode":"safe","nullable":null,"attributes":{}}), + json!({"items":[{"name":"Entry"}],"mode":"safe","nullable":null,"attributes":{}}), + json!({"items":[],"mode":"safe","nullable":null,"attributes":{}}), + json!({"items":[{"name":"entry"}],"mode":"fast","nullable":null,"attributes":{},"limit":11}), + json!({"items":[{"name":"entry"}],"mode":"other","nullable":null,"attributes":{}}), + json!({"items":[{"name":"entry"}],"mode":"safe","attributes":{}}), + json!({"items":[{"name":"entry"}],"mode":"safe","nullable":false,"attributes":{}}), + json!({"items":[{"name":"entry"}],"mode":"safe","nullable":null,"attributes":{"x":"wrong"}}), + ] { + assert!(invoke(&runtime, invalid).await.is_err()); + } + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "invalid inputs must not reach the original tool" + ); + } + + #[tokio::test] + async fn open_records_nullable_unions_and_recursive_inputs_keep_their_original_shape() { + for (schema, input) in [ + ( + json!({"type":"object","properties":{"known":{"type":"string"}}}), + json!({"known":"yes","extra":[1,null,{"free":true}]}), + ), + (json!({"type":["string","null"]}), Json::Null), + (json!({"type":"null"}), Json::Null), + (json!({"type":"boolean"}), json!(true)), + (json!({"type":"number"}), json!(1.5)), + (json!({"type":"string","enum":["", "value"]}), json!("")), + ( + json!({"type":"array","prefixItems":[{"type":"string"}],"items":{"type":"integer"}}), + json!(["first", 2]), + ), + ( + json!({"type":"object","additionalProperties":false,"properties":{"defaulted":{"type":"string","default":"do not insert"}}}), + json!({}), + ), + ( + json!({"$ref":"#/$defs/A~1B","$defs":{"A/B":{"type":"string"}}}), + json!("escaped pointer"), + ), + ( + json!({"oneOf":[{"type":"integer"},{"type":"string"}]}), + json!(7), + ), + ( + json!({"$ref":"#/$defs/Node","$defs":{"Node":{"type":"object","additionalProperties":false,"properties":{"next":{"$ref":"#/$defs/Node"}}}}}), + json!({"next":{"next":{}}}), + ), + ] { + let (runtime, calls) = runtime(schema); + let output = invoke(&runtime, input.clone()).await.unwrap(); + assert_eq!( + serde_json::from_str::(output.content.as_deref().unwrap()).unwrap(), + input + ); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + } +} diff --git a/docs/design/worker-wip-mode.md b/docs/design/worker-wip-mode.md index 107bf14ea..e1d957568 100644 --- a/docs/design/worker-wip-mode.md +++ b/docs/design/worker-wip-mode.md @@ -47,6 +47,10 @@ The first inspects an Object; the second inspects one root-scoped Interface. Obj Reference components follow Text View quoting: scope allows unquoted `[A-Za-z0-9_./-]+`, name allows `[A-Za-z0-9_.-]+`; other components use JSON string quoting/escapes. There is no scope inference, normalization, relative resolution, or operation suffix. A literal Object path containing `::` must be JSON-quoted inside the `path` value (for example `{"path":"\"/literal::name\""}`) to distinguish it from an Interface address. Ordinary Object paths retain Protocol path syntax, including Unicode. Invalid or unavailable Interface addresses fail rather than falling back to Object lookup. +### Model usage guidance + +The three tool descriptions are the recurring model-facing usage instructions, including after compaction. Use a shallow Tree only when needed to locate entrances, inspect only relevant Objects and unknown Interface contracts, and then work through Invoke. Reuse known paths and understood contracts for the **same exact scope/name reference**; do not inspect every node or repeat Inspect as a prerequisite for every operation. Same local names under different scopes are not evidence of the same contract. Reinspect only when a reference/contract is new, changed, or no longer available in the conversation context. Cache maintenance is the Client's responsibility, not a reason for extra model calls. Use domain read/list/search/query Operations for content and large/nonindexable collections. Current authorization checks and the prohibition on blindly retrying dispatched operations remain unchanged. + The compatibility projection mounts enabled ordinary tools at: ```text @@ -59,9 +63,11 @@ Each compatibility Object publishes an explicit root-scoped reference: {"scope":"/", "name":"yoi.tool//v1"} ``` -Its single operation is `call`. For this compatibility route, `Invoke.arguments` is `{ "input": }`, matching the descriptor's named Json parameter. Native projections instead receive a JSON object keyed by the descriptor's declared parameter names. Native input is decoded with the published `wip-http` descriptor-bound codec, so named and composite types are resolved, an integral JSON number remains a WIP `Number` when declared as such, and WIP `Bytes` use the codec's canonical RFC 4648 base64 JSON representation. Tool argument IDs remain JSON values; the adapter never infers a domain object or Worldspace route from them. +Its single operation is `call`. For this compatibility route, `Invoke.arguments` is `{ "input": }`, matching the descriptor's named `input` parameter. Native projections instead receive a JSON object keyed by the descriptor's declared parameter names. Native input is decoded with the published `wip-http` descriptor-bound codec, so named and composite types are resolved, an integral JSON number remains a WIP `Number` when declared as such, and WIP `Bytes` use the codec's canonical RFC 4648 base64 JSON representation. Tool argument IDs remain JSON values; the adapter never infers a domain object or Worldspace route from them. -The interface descriptor uses WIP `Json` for compatibility input and includes the exact original JSON Schema in descriptor documentation. Before execution, the Host validates the JSON value with that original schema and then delegates to the original async `Tool`. Constraints are therefore neither approximated nor silently dropped. A schema that cannot be compiled prevents WIP startup instead of creating a weaker projection. +Compatibility input is projected conservatively into JSON-shape-preserving WIP types: primitives, closed records with required/optional fields, homogeneous lists, and string enums. Local JSON Pointer references are expanded within bounded depth/node budgets; cycles and unsupported constructs stay `Json`. Open records, nullable/mixed unions, composition, and tuple elements are not falsely modeled as closed records or tagged WIP unions. The original input JSON Schema, including constraints, descriptions, defaults, and local definitions, is included in the **visible parameter summary** as well as retained documentation details. Thus a single Interface Inspect reveals the usable input contract even where WIP's type system cannot express it or an expansion budget is reached; it is never just an opaque `input: json` with hidden instructions. Full Inspect output still obeys response limits and is not silently truncated. + +The call envelope remains `{ "input": }`; no tagged union wrapper, field renaming, or default insertion is introduced. Before execution, the Host validates the reconstructed JSON value with the exact original schema and then delegates to the original async `Tool`. Structural projection never replaces original-schema constraints or permission checks. A schema that cannot be compiled prevents WIP startup instead of creating a weaker projection. ## Native projection extension and collisions From 7321d6c7c3f9e13f60559cab4e48ca6863ff62d6 Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 10 Oct 2026 02:12:29 +0900 Subject: [PATCH 3/5] fix: forward scoped Workdir delegation checks Forward provider authorization and overlap checks through ScopedWorkdirSession while preserving capability ceilings, path rebasing, write leases, and revocation. Exercise stacked wrappers, External providers, and remote HTTP sessions without weakening resolved symlink policy. --- crates/workdir/src/checkout_search_tests.rs | 10 +- crates/workdir/src/scope.rs | 47 +++ crates/workdir/src/scope_delegation_tests.rs | 334 ++++++++++++++++++ crates/workspace-server/src/server.rs | 1 + .../server/tests/workdir_delegation_tests.rs | 259 ++++++++++++++ 5 files changed, 646 insertions(+), 5 deletions(-) create mode 100644 crates/workdir/src/scope_delegation_tests.rs create mode 100644 crates/workspace-server/src/server/tests/workdir_delegation_tests.rs diff --git a/crates/workdir/src/checkout_search_tests.rs b/crates/workdir/src/checkout_search_tests.rs index 44c81488e..2777088b7 100644 --- a/crates/workdir/src/checkout_search_tests.rs +++ b/crates/workdir/src/checkout_search_tests.rs @@ -212,17 +212,17 @@ async fn checkout_search_composes_actual_nested_wrappers_and_output_root() { }; assert_eq!(result.entries[0].path, path("keep/yes.txt")); // A second actual scoped wrapper has its own cwd and nonrecursive layer. - // Logical rules avoid requesting resolved authorization from a wrapper - // that conservatively does not expose host-side resolution authority. + // Default resolved authorization must survive every wrapper. std::fs::create_dir(root.path().join("child/keep/deep")).unwrap(); std::fs::write( root.path().join("child/keep/deep/secret"), "needle hidden nested", ) .unwrap(); - let mut own_rule = rule("keep", false); - own_rule.symlink_policy = manifest::SymlinkPolicy::Logical; - let second = stacked.scope(scope(vec![own_rule], "keep")).await.unwrap(); + let second = stacked + .scope(scope(vec![rule("keep", false)], "keep")) + .await + .unwrap(); let result = second .tool_session() .grep(grep(GrepOutputMode::Content)) diff --git a/crates/workdir/src/scope.rs b/crates/workdir/src/scope.rs index e36e3cd88..8bde43c29 100644 --- a/crates/workdir/src/scope.rs +++ b/crates/workdir/src/scope.rs @@ -994,6 +994,51 @@ impl WorkdirSession for ScopedWorkdirSession { self.capabilities } + async fn authorize_scope_path( + &self, + mut request: WorkdirScopeAuthorizationRequest, + ) -> Result<(), WorkdirError> { + let _scope_guard = self.scope_lock.lock().await; + // Like filesystem operations, requests use this session's coordinates. + // Keep every wrapper's attenuation before asking the provider to resolve + // symlinks; forwarding the caller's rules alone would discard our scope. + for rule in &mut request.rules { + rule.target = self.resolve_path(&rule.target)?; + } + self.validate_scope(&request.rules, false)?; + request.path = self + .resolve_operation_path(&request.path, request.permission) + .await?; + match request.permission { + WorkdirToolScopePermission::Read => { + self.ensure_read(&request.path, WorkdirSessionCapability::Read)?; + } + WorkdirToolScopePermission::Write => { + self.ensure_write(&request.path, WorkdirSessionCapability::Write)?; + } + } + self.source.authorize_scope_path(request).await?; + self.ensure_active() + } + + async fn scope_rules_overlap( + &self, + mut request: WorkdirScopeOverlapRequest, + ) -> Result { + let _scope_guard = self.scope_lock.lock().await; + // Comparing write rules is a read-only query, not a write authorization. + // In particular it must work while those paths are leased to children. + for rule in [&mut request.left, &mut request.right] { + rule.target = self + .resolve_operation_path(&rule.target, WorkdirToolScopePermission::Read) + .await?; + self.ensure_read(&rule.target, WorkdirSessionCapability::Read)?; + } + let overlaps = self.source.scope_rules_overlap(request).await?; + self.ensure_active()?; + Ok(overlaps) + } + async fn checkout_search( &self, mut request: crate::CheckoutSearchRequest, @@ -1712,6 +1757,8 @@ mod tests { use super::*; use crate::LocalWorkdirSession; + include!("scope_delegation_tests.rs"); + fn fs_path(path: &str) -> FsPath { FsPath::new(path).unwrap() } diff --git a/crates/workdir/src/scope_delegation_tests.rs b/crates/workdir/src/scope_delegation_tests.rs new file mode 100644 index 000000000..9073c0221 --- /dev/null +++ b/crates/workdir/src/scope_delegation_tests.rs @@ -0,0 +1,334 @@ +// Included in scope::tests; exercise real provider resolution through stacked brokers. + +fn filesystem_scope(path: &str, permission: WorkdirToolScopePermission) -> WorkdirToolScope { + let mut scope = request(path, permission); + scope.command = false; + scope +} + +fn authorization( + path: &str, + permission: WorkdirToolScopePermission, +) -> WorkdirScopeAuthorizationRequest { + WorkdirScopeAuthorizationRequest { + rules: filesystem_scope("", permission).rules, + path: fs_path(path), + permission, + } +} + +#[tokio::test] +async fn stacked_brokers_delegate_all_filesystem_provider_kinds() { + use crate::{BoundedReadLimits, ExternalWorkdirRoot}; + for kind in [ + "repository", + "read-only-wrapper", + "external-read", + "external-write", + "external-read-command", + "external-all", + ] { + let root = TempDir::new().unwrap(); + fs::create_dir(root.path().join("child")).unwrap(); + fs::create_dir(root.path().join("other")).unwrap(); + fs::write(root.path().join("child/input"), "visible").unwrap(); + fs::write(root.path().join("other/secret"), "hidden").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink("../other", root.path().join("child/escape")).unwrap(); + let capabilities = match kind { + "read-only-wrapper" | "external-read" => WorkdirSessionCapabilities::READ_ONLY, + "external-write" => WorkdirSessionCapabilities::READ_WRITE, + "external-read-command" => WorkdirSessionCapabilities::READ_ONLY + .union(WorkdirSessionCapabilities::COMMAND_ONLY), + _ => WorkdirSessionCapabilities::ALL, + }; + let source: WorkdirSessionHandle = if kind.starts_with("external-") { + Arc::new( + LocalWorkdirSession::external_with_capabilities_pinned( + Workdir::new(kind), + ExternalWorkdirRoot::pin(root.path()).unwrap(), + BoundedReadLimits::new(4096, 1024).unwrap(), + capabilities, + ) + .unwrap(), + ) + } else { + let source: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new( + Scope::writable(root.path()).unwrap(), + root.path().to_path_buf(), + )); + if kind == "read-only-wrapper" { + Arc::new(ReadOnlyWorkdirSession::new(source)) + } else { + source + } + }; + // Backend capability wrapper, attachment routing, then the Worker broker. + let backend = WorkdirToolBroker::with_capabilities(source, capabilities); + let router = crate::WorkdirSessionRouter::new(); + let routed = router + .attach( + crate::WorkdirAttachmentAlias::new("selected").unwrap(), + backend.tool_session(), + ) + .unwrap(); + let worker = WorkdirToolBroker::new(routed); + let child = worker + .scope(filesystem_scope("child", WorkdirToolScopePermission::Read)) + .await + .unwrap(); + assert_eq!( + child.read(read("input")).await.unwrap().bytes, + b"visible", + "{kind}" + ); + assert!(child.write(write("denied", "no")).await.is_err(), "{kind}"); + #[cfg(unix)] + assert!(child.read(read("escape/secret")).await.is_err(), "{kind}"); + child.close().await.unwrap(); + let writable = worker + .scope(filesystem_scope("child", WorkdirToolScopePermission::Write)) + .await; + if !capabilities.supports(WorkdirSessionCapability::Write) { + assert!(writable.is_err(), "{kind}"); + continue; + } + let writable = writable.unwrap(); + writable.write(write("created", "child")).await.unwrap(); + assert!( + worker.write(write("child/blocked", "no")).await.is_err(), + "{kind}" + ); + assert!( + worker + .scope(filesystem_scope("child", WorkdirToolScopePermission::Write)) + .await + .is_err(), + "{kind}" + ); + // A non-overlapping write lease requires the second missing provider method. + let sibling = worker + .scope(filesystem_scope("other", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + sibling.write(write("created", "sibling")).await.unwrap(); + assert!( + !writable + .capabilities + .supports(WorkdirSessionCapability::Command) + ); + writable.close().await.unwrap(); + worker + .write(write("child/released", "parent")) + .await + .unwrap(); + sibling.close().await.unwrap(); + let command_child = worker + .scope(request("child", WorkdirToolScopePermission::Write)) + .await; + if capabilities.supports(WorkdirSessionCapability::Command) { + let command_child = command_child.unwrap(); + assert_eq!( + run_command(&command_child.tool_session(), "printf delegated", kind) + .await + .content, + "delegated" + ); + command_child.close().await.unwrap(); + } else { + assert!(command_child.is_err()); + } + } +} + +#[tokio::test] +async fn stacked_authorization_preserves_ceiling_scope_and_revocation() { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("allowed/deep")).unwrap(); + let backend = session(root.path()); + let read_only = WorkdirToolBroker::with_capabilities( + backend.tool_session(), + WorkdirSessionCapabilities::READ_ONLY, + ); + read_only + .authorize_scope_path(authorization("allowed", WorkdirToolScopePermission::Read)) + .await + .unwrap(); + assert!( + read_only + .authorize_scope_path(authorization("allowed", WorkdirToolScopePermission::Write)) + .await + .is_err() + ); + let command_only = WorkdirToolBroker::with_capabilities( + backend.tool_session(), + WorkdirSessionCapabilities::COMMAND_ONLY, + ); + assert!( + command_only + .authorize_scope_path(authorization("allowed", WorkdirToolScopePermission::Read)) + .await + .is_err() + ); + assert!( + command_only + .scope(filesystem_scope( + "allowed", + WorkdirToolScopePermission::Read + )) + .await + .is_err() + ); + + let mut limited = filesystem_scope("allowed", WorkdirToolScopePermission::Read); + limited.rules[0].recursive = false; + let parent = backend.scope(limited).await.unwrap(); + let stacked = WorkdirToolBroker::new(parent.tool_session()); + let mut valid = filesystem_scope("", WorkdirToolScopePermission::Read); + valid.rules[0].recursive = false; + let child = stacked.scope(valid.clone()).await.unwrap(); + assert!( + stacked + .scope(filesystem_scope("", WorkdirToolScopePermission::Read)) + .await + .is_err() + ); + let mut logical = valid.clone(); + logical.rules[0].symlink_policy = SymlinkPolicy::Logical; + assert!(stacked.scope(logical).await.is_err()); + assert!( + stacked + .authorize_scope_path(authorization("deep/file", WorkdirToolScopePermission::Read)) + .await + .is_err() + ); + parent.close().await.unwrap(); + assert!(stacked.scope(valid).await.is_err()); + assert!(child.read(read("deep/file")).await.is_err()); + assert!( + stacked + .scope_rules_overlap(WorkdirScopeOverlapRequest { + left: filesystem_scope("", WorkdirToolScopePermission::Read) + .rules + .remove(0), + right: filesystem_scope("", WorkdirToolScopePermission::Read) + .rules + .remove(0), + }) + .await + .is_err() + ); +} + +#[tokio::test] +async fn stacked_scope_rebases_rules_paths_and_overlap_once() { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("base/inner/one")).unwrap(); + fs::create_dir_all(root.path().join("base/inner/two")).unwrap(); + fs::write(root.path().join("base/inner/one/input"), "nested").unwrap(); + let backend = session(root.path()); + let first = backend + .scope(filesystem_scope("base", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + let worker = WorkdirToolBroker::new(first.tool_session()); + let second = worker + .scope(filesystem_scope("inner", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + let nested = WorkdirToolBroker::new(second.tool_session()); + let leaf = nested + .scope(filesystem_scope("one", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + assert_eq!(leaf.read(read("input")).await.unwrap().bytes, b"nested"); + leaf.write(write("output", "yes")).await.unwrap(); + assert_eq!( + fs::read(root.path().join("base/inner/one/output")).unwrap(), + b"yes" + ); + assert!(nested.write(write("one/blocked", "no")).await.is_err()); + let sibling = nested + .scope(filesystem_scope("two", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + sibling.write(write("output", "sibling")).await.unwrap(); + assert!( + nested + .scope(filesystem_scope("one", WorkdirToolScopePermission::Write)) + .await + .is_err() + ); + leaf.close().await.unwrap(); + nested.write(write("one/released", "yes")).await.unwrap(); + sibling.close().await.unwrap(); + first.close().await.unwrap(); + assert!( + nested + .authorize_scope_path(authorization("one", WorkdirToolScopePermission::Read)) + .await + .is_err() + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn stacked_scope_keeps_resolved_symlink_and_lease_checks() { + use std::os::unix::fs::symlink; + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("base/target")).unwrap(); + fs::create_dir(root.path().join("secret")).unwrap(); + fs::write(root.path().join("secret/key"), "hidden").unwrap(); + symlink("target", root.path().join("base/alias-a")).unwrap(); + symlink("target", root.path().join("base/alias-b")).unwrap(); + symlink("../../secret", root.path().join("base/target/escape")).unwrap(); + let backend = session(root.path()); + let parent = backend + .scope(filesystem_scope("base", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + let worker = WorkdirToolBroker::new(parent.tool_session()); + let first = worker + .scope(filesystem_scope( + "alias-a", + WorkdirToolScopePermission::Write, + )) + .await + .unwrap(); + first.write(write("ok", "allowed")).await.unwrap(); + assert!(first.read(read("escape/key")).await.is_err()); + assert!( + worker + .scope(filesystem_scope( + "alias-a/escape", + WorkdirToolScopePermission::Read + )) + .await + .is_err() + ); + assert!( + worker + .scope(filesystem_scope( + "alias-b", + WorkdirToolScopePermission::Write + )) + .await + .is_err() + ); + assert!(worker.write(write("target/blocked", "no")).await.is_err()); + // Direct authorization through the wrapper must not bypass its existing write lease. + assert!( + worker + .authorize_scope_path(authorization( + "target/blocked", + WorkdirToolScopePermission::Write + )) + .await + .is_err() + ); + first.close().await.unwrap(); + worker + .write(write("alias-b/released", "yes")) + .await + .unwrap(); +} diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 9f1edc803..134cf130d 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -32912,6 +32912,7 @@ mod tests { mod ticket_worker_claim_tests; mod ticket_worker_launch_tests; mod value_profiles_tests; + mod workdir_delegation_tests; mod worker_drive_tests; mod worker_operations_tests; include!("server_workspace_config_tests.rs"); diff --git a/crates/workspace-server/src/server/tests/workdir_delegation_tests.rs b/crates/workspace-server/src/server/tests/workdir_delegation_tests.rs new file mode 100644 index 000000000..17240665a --- /dev/null +++ b/crates/workspace-server/src/server/tests/workdir_delegation_tests.rs @@ -0,0 +1,259 @@ +use super::*; +use workdir::{ + BoundedReadLimits, LocalWorkdirSession, Workdir, WorkdirPath, + WorkdirSessionCapabilities as Caps, WorkdirToolBroker, WorkdirToolScope, + WorkdirToolScopePermission as Permission, WorkdirToolScopeRule, +}; + +fn scope(target: &str, permission: Permission) -> WorkdirToolScope { + WorkdirToolScope { + rules: vec![WorkdirToolScopeRule { + target: WorkdirPath::new(target).unwrap(), + permission, + recursive: true, + symlink_policy: Default::default(), + }], + cwd: WorkdirPath::new(target).unwrap(), + command: false, + } +} + +async fn verify_backend_delegation(source: WorkdirSessionHandle, capabilities: Caps) { + // The same capability wrapper returned by both Backend attachment branches. + let backend = WorkdirToolBroker::with_capabilities(source, capabilities); + let session = backend.tool_session(); + let authorization = workdir::WorkdirScopeAuthorizationRequest { + rules: scope("child", Permission::Read).rules, + path: WorkdirPath::new("child").unwrap(), + permission: Permission::Read, + }; + assert!(matches!( + execute_workdir_session_operation( + &session, + WorkdirSessionOperation::AuthorizeScope(authorization) + ) + .await + .unwrap(), + WorkdirSessionOperationResult::AuthorizeScope + )); + let worker = WorkdirToolBroker::new(session.clone()); + let child = worker + .scope(scope("child", Permission::Read)) + .await + .unwrap(); + assert_eq!( + child + .read(workdir::ReadRequest { + path: WorkdirPath::new("input").unwrap(), + offset: 0, + limit: 10, + max_bytes: 1024, + }) + .await + .unwrap() + .bytes, + b"visible" + ); + child.close().await.unwrap(); + + // Comparison is read-only even when the compared rules describe writes. + let overlap = workdir::WorkdirScopeOverlapRequest { + left: scope("child", Permission::Write).rules.remove(0), + right: scope("other", Permission::Write).rules.remove(0), + }; + assert!(matches!( + execute_workdir_session_operation( + &session, + WorkdirSessionOperation::ScopeRulesOverlap(overlap) + ) + .await + .unwrap(), + WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps: false } + )); + let writer = worker.scope(scope("child", Permission::Write)).await; + if !capabilities.supports(workdir::WorkdirSessionCapability::Write) { + assert!(writer.is_err()); + return; + } + let writer = writer.unwrap(); + let sibling = worker + .scope(scope("other", Permission::Write)) + .await + .unwrap(); + let write = |path: &str| workdir::WriteRequest { + path: WorkdirPath::new(path).unwrap(), + content: b"written".to_vec(), + expected_hash: None, + }; + writer.write(write("created")).await.unwrap(); + assert!(worker.write(write("child/blocked")).await.is_err()); + assert!( + worker + .scope(scope("child", Permission::Write)) + .await + .is_err() + ); + sibling.write(write("created")).await.unwrap(); + writer.close().await.unwrap(); + worker.write(write("child/released")).await.unwrap(); + sibling.close().await.unwrap(); +} + +fn fixture() -> tempfile::TempDir { + let root = tempfile::tempdir().unwrap(); + fs::create_dir(root.path().join("child")).unwrap(); + fs::create_dir(root.path().join("other")).unwrap(); + fs::write(root.path().join("child/input"), "visible").unwrap(); + root +} + +#[tokio::test] +async fn backend_delegation_reaches_external_provider_resolution() { + for capabilities in [Caps::READ_ONLY, Caps::READ_WRITE] { + let root = fixture(); + let local = LocalWorkdirSession::external_with_capabilities_pinned( + Workdir::new("external-delegation"), + workdir::ExternalWorkdirRoot::pin(root.path()).unwrap(), + BoundedReadLimits::new(4096, 1024).unwrap(), + capabilities, + ) + .unwrap(); + let (sender, mut commands) = tokio::sync::mpsc::channel(16); + let source = Arc::new(ExternalProviderWorkdirSession::new( + Arc::new(ExternalProviderConnection { + grant_id: "delegation-grant".into(), + workdir_id: "external-delegation".into(), + provider_instance_id: "delegation-provider".into(), + generation: 1, + expires_at: None, + capabilities, + admission: Arc::new(tokio::sync::Semaphore::new(16)), + shutdown_confirmed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + sender, + }), + None, + )); + // Real external session transport and real provider authorization, not + // canned allow responses. Wire serialization also preserves the rules. + let provider = tokio::spawn(async move { + let mut authorizations = 0; + let mut overlaps = 0; + while let Some(command) = commands.recv().await { + let ExternalProviderCommand::Operation { + operation, + response, + .. + } = command + else { + panic!("unexpected provider control command"); + }; + let operation = + serde_json::from_slice(&serde_json::to_vec(&operation).unwrap()).unwrap(); + match &operation { + WorkdirSessionOperation::AuthorizeScope(_) => authorizations += 1, + WorkdirSessionOperation::ScopeRulesOverlap(_) => overlaps += 1, + _ => {} + } + let result = workdir::dispatch_workdir_session_operation(&local, operation) + .await + .map_err(|error| WorkdirTransportError::from_workdir_error(&error)); + response.send(result).unwrap(); + } + (authorizations, overlaps) + }); + tokio::time::timeout( + std::time::Duration::from_secs(10), + verify_backend_delegation(source, capabilities), + ) + .await + .unwrap(); + let (authorizations, overlaps) = + tokio::time::timeout(std::time::Duration::from_secs(5), provider) + .await + .unwrap() + .unwrap(); + assert!(authorizations >= 3); + assert!(overlaps >= 1); + } +} + +#[tokio::test] +async fn backend_delegation_reaches_remote_repository_resolution() { + use workdir::http::{ + OpenWorkdirSessionResponse, RemoteWorkdirSession, WorkdirSessionId, + WorkdirSessionOperationRequest, + }; + let root = fixture(); + let source: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new( + manifest::Scope::writable(root.path()).unwrap(), + root.path().to_path_buf(), + )); + let workdir_id = source.workdir().id().clone(); + let response_id = workdir_id.clone(); + let app = Router::new() + .route( + "/v1/working-directories/{id}/sessions", + axum::routing::post(move || { + let workdir_id = response_id.clone(); + async move { + Json(OpenWorkdirSessionResponse { + session_id: WorkdirSessionId::new("delegation-session").unwrap(), + workdir_id, + capabilities: Caps::ALL, + }) + } + }), + ) + .route( + "/v1/workdir-sessions/{id}/operations", + axum::routing::post(move |Json(request): Json| { + let source = source.clone(); + async move { + match workdir::dispatch_workdir_session_operation( + source.as_ref(), + request.operation, + ) + .await + { + Ok(result) => Json(result).into_response(), + Err(error) => { + let error = WorkdirTransportError::from_workdir_error(&error); + ( + StatusCode::from_u16(error.code.http_status()).unwrap(), + Json(error), + ) + .into_response() + } + } + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let (shutdown, finished) = tokio::sync::oneshot::channel::<()>(); + let server = tokio::spawn(async move { + axum::serve(listener, app) + .with_graceful_shutdown(async { + let _ = finished.await; + }) + .await + .unwrap(); + }); + let remote = RemoteWorkdirSession::open( + reqwest::Client::new(), + format!("http://{address}").parse().unwrap(), + "test-session-token", + workdir_id, + Default::default(), + ) + .await + .unwrap(); + tokio::time::timeout( + std::time::Duration::from_secs(10), + verify_backend_delegation(Arc::new(remote), Caps::ALL), + ) + .await + .unwrap(); + shutdown.send(()).unwrap(); + server.await.unwrap(); +} From dc8e586a0bc505a234933844b0aa2a71cf922208 Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 10 Oct 2026 12:04:56 +0900 Subject: [PATCH 4/5] fix: keep runtime responsive during workdir cleanup --- crates/workdir/src/local.rs | 174 ++++- crates/worker-runtime/src/http_server.rs | 430 ++++++++++- crates/worker-runtime/src/runtime.rs | 701 +++++++++++++++++- .../src/runtime/workdir_session.rs | 165 +++++ crates/worker-runtime/src/worker_backend.rs | 42 +- .../worker-runtime/src/working_directory.rs | 369 ++++++++- .../src/working_directory/cleanup.rs | 70 +- .../src/working_directory/occupancy.rs | 196 +++++ docs/development/workdir-removal.md | 24 +- 9 files changed, 2069 insertions(+), 102 deletions(-) create mode 100644 crates/worker-runtime/src/runtime/workdir_session.rs create mode 100644 crates/worker-runtime/src/working_directory/occupancy.rs diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 2d619655a..7d8a7697d 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -1770,6 +1770,9 @@ impl WorkdirSession for LocalWorkdirSession { async fn start_command(&self, request: CommandRequest) -> Result { self.ensure_capability(WorkdirSessionCapability::Command)?; + // Command admission, resource capture and registration must be atomic + // with terminal close, which drains commands and releases resources. + let _admission = self.inner.close_lock.lock().await; self.ensure_open()?; if let Some(spill_dir) = request.spill_dir.as_deref() && !self @@ -1794,6 +1797,15 @@ impl WorkdirSession for LocalWorkdirSession { let telemetry = self.inner.command_telemetry.clone(); let command_environment = self.inner.command_environment.clone(); let (cancel, cancel_rx) = watch::channel(false); + // A session drop only schedules command cancellation. Keep its Runtime + // resources (including cleanup exclusion) alive until the task actually + // finishes or its future is dropped, not just until the handle disappears. + let resources = self + .inner + .resources + .lock() + .unwrap_or_else(|error| error.into_inner()) + .clone(); let task = tokio::spawn(async move { let output = run_command( cwd, @@ -1802,6 +1814,7 @@ impl WorkdirSession for LocalWorkdirSession { telemetry, command_environment, cancel_rx, + resources, ) .await; let _ = completion_tx.send(true); @@ -2131,6 +2144,7 @@ async fn run_command( telemetry: CommandTelemetry, command_environment: BTreeMap, mut cancel: watch::Receiver, + resources: Vec>, ) -> Result { let stdout = tempfile::NamedTempFile::new().map_err(|error| WorkdirError::io(&cwd, error))?; let stderr = tempfile::NamedTempFile::new().map_err(|error| WorkdirError::io(&cwd, error))?; @@ -2250,7 +2264,7 @@ async fn run_command( let stdout_path = stdout_path.to_path_buf(); let stderr_path = stderr_path.to_path_buf(); Some( - tokio::task::spawn_blocking(move || { + spawn_resource_owned_blocking(resources.clone(), move || { persist_command_output(&stdout_path, &stderr_path, &spill_dir) }) .await @@ -2272,6 +2286,18 @@ async fn run_command( }) } +// Blocking spills cannot be aborted with their async waiter. Transfer resource +// ownership into the actual filesystem task so cleanup waits for its completion. +fn spawn_resource_owned_blocking( + resources: Vec>, + effect: impl FnOnce() -> T + Send + 'static, +) -> JoinHandle { + tokio::task::spawn_blocking(move || { + let _resources = resources; + effect() + }) +} + #[derive(Debug, Default)] struct CommandOutputDecoder { read_offset: u64, @@ -4065,6 +4091,152 @@ mod tests { assert_eq!(terminal, Some((handle.0, CommandStatus::TimedOut, None))); } + #[tokio::test(flavor = "current_thread")] + async fn command_admission_serializes_with_close_before_capturing_resources() { + let dir = TempDir::new().unwrap(); + let session = Arc::new(make_fs(&dir)); + let held = session.inner.close_lock.lock().await; + let (closing, close_entered) = tokio::sync::oneshot::channel(); + let close_session = session.clone(); + let close = tokio::spawn(async move { + closing.send(()).unwrap(); + WorkdirSession::close(close_session.as_ref()).await + }); + close_entered.await.unwrap(); + let (starting, start_entered) = tokio::sync::oneshot::channel(); + let start_session = session.clone(); + let start = tokio::spawn(async move { + starting.send(()).unwrap(); + WorkdirSession::start_command( + start_session.as_ref(), + CommandRequest { + command: "true".into(), + timeout_secs: 5, + output_limit: 1024, + cwd: WorkdirPath::root(), + spill_dir: None, + tool_call_id: None, + }, + ) + .await + }); + start_entered.await.unwrap(); + assert!( + !start.is_finished(), + "command started outside terminal-close admission" + ); + drop(held); + tokio::time::timeout(Duration::from_secs(5), close) + .await + .unwrap() + .unwrap() + .unwrap(); + assert!(matches!( + tokio::time::timeout(Duration::from_secs(5), start) + .await + .unwrap() + .unwrap(), + Err(WorkdirError::Unavailable(message)) if message.contains("is closed") + )); + assert!( + session.command_snapshot().is_empty(), + "closed admission launched a command" + ); + } + + #[tokio::test] + async fn blocking_output_spill_retains_resources_after_waiter_is_cancelled() { + #[derive(Debug)] + struct Resource(Option>); + impl Drop for Resource { + fn drop(&mut self) { + let _ = self.0.take().unwrap().send(()); + } + } + let (released, mut release) = tokio::sync::oneshot::channel(); + let resources: Vec> = + vec![Arc::new(Resource(Some(released)))]; + let (entered, started) = tokio::sync::oneshot::channel(); + let (finish, wait) = std::sync::mpsc::channel(); + // Exercise the resource-transfer policy at the same blocking boundary + // used by output persistence, without filesystem timing assumptions. + let waiter = tokio::spawn(async move { + spawn_resource_owned_blocking(resources, move || { + entered.send(()).unwrap(); + wait.recv_timeout(Duration::from_secs(5)).unwrap(); + }) + .await + .unwrap(); + }); + tokio::time::timeout(Duration::from_secs(5), started) + .await + .unwrap() + .unwrap(); + waiter.abort(); + assert!(waiter.await.unwrap_err().is_cancelled()); + assert!( + matches!( + release.try_recv(), + Err(tokio::sync::oneshot::error::TryRecvError::Empty) + ), + "blocking spill lost its resources when the waiter was cancelled" + ); + finish.send(()).unwrap(); + tokio::time::timeout(Duration::from_secs(5), release) + .await + .unwrap() + .unwrap(); + } + + #[tokio::test(flavor = "current_thread")] + async fn session_drop_retains_resources_until_command_task_cancellation_completes() { + #[derive(Debug)] + struct Resource(Option>); + impl Drop for Resource { + fn drop(&mut self) { + let _ = self.0.take().unwrap().send(()); + } + } + let dir = TempDir::new().unwrap(); + let (released, mut release) = tokio::sync::oneshot::channel(); + let session = LocalWorkdirSession::materialized_bound_with_environment( + Workdir::new("command-resource-session"), + dir.path().to_path_buf(), + dir.path().to_path_buf(), + SharedScope::new(Scope::writable(dir.path()).unwrap()), + WorkdirSessionCapabilities::ALL, + BTreeMap::new(), + vec![Arc::new(Resource(Some(released)))], + ); + WorkdirSession::start_command( + &session, + CommandRequest { + command: "true".into(), + timeout_secs: 5, + output_limit: 1024, + cwd: WorkdirPath::root(), + spill_dir: None, + tool_call_id: None, + }, + ) + .await + .unwrap(); + // The sole executor has not polled the command task, so dropping the + // session schedules its abort but cannot yet complete cancellation. + drop(session); + assert!( + matches!( + release.try_recv(), + Err(tokio::sync::oneshot::error::TryRecvError::Empty) + ), + "resources released before the aborted command future was dropped" + ); + tokio::time::timeout(Duration::from_secs(5), release) + .await + .unwrap() + .unwrap(); + } + #[tokio::test] async fn closing_session_releases_runtime_resources() { #[derive(Debug)] diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index df39f7651..336817c51 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -951,10 +951,12 @@ async fn observe_repository_ref( async fn list_working_directories( State(state): State, ) -> RestResult { - let working_directories = state - .runtime - .list_working_directories() - .map_err(RuntimeHttpRestError::runtime)?; + // Inventory providers may inspect the filesystem even for read-only requests. + let working_directories = + tokio::task::spawn_blocking(move || state.runtime.list_working_directories()) + .await + .map_err(RuntimeHttpRestError::working_directory_join)? + .map_err(RuntimeHttpRestError::runtime)?; Ok(Json(RuntimeHttpWorkingDirectoriesResponse { working_directories, })) @@ -989,10 +991,11 @@ async fn get_working_directory( State(state): State, Path(working_directory_id): Path, ) -> RestResult { - let working_directory = state - .runtime - .working_directory(&working_directory_id) - .map_err(RuntimeHttpRestError::runtime)?; + let working_directory = + tokio::task::spawn_blocking(move || state.runtime.working_directory(&working_directory_id)) + .await + .map_err(RuntimeHttpRestError::working_directory_join)? + .map_err(RuntimeHttpRestError::runtime)?; Ok(Json(RuntimeHttpWorkingDirectoryResponse { working_directory, })) @@ -1130,10 +1133,16 @@ async fn cleanup_working_directory( State(state): State, Path(working_directory_id): Path, ) -> RestResult { - let working_directory = state - .runtime - .cleanup_working_directory(&working_directory_id) - .map_err(RuntimeHttpRestError::runtime)?; + // Keep Runtime's occupancy check and deletion together, but do not run + // synchronous filesystem/Git work on the HTTP executor. + let working_directory = tokio::task::spawn_blocking(move || { + state + .runtime + .cleanup_working_directory(&working_directory_id) + }) + .await + .map_err(RuntimeHttpRestError::working_directory_join)? + .map_err(RuntimeHttpRestError::runtime)?; Ok(Json(RuntimeHttpWorkingDirectoryResponse { working_directory, })) @@ -2443,6 +2452,16 @@ impl RuntimeHttpRestError { Self::new(status, code, error.to_string()) } + fn working_directory_join(_error: tokio::task::JoinError) -> Self { + // A panic payload can contain paths or credentials. A missing result + // also does not prove that cleanup made no changes; do not claim rollback. + Self::new( + StatusCode::INTERNAL_SERVER_ERROR, + "working_directory_task_failed", + "Working directory operation failed to return a result", + ) + } + fn json_rejection(error: JsonRejection) -> Self { Self::new( StatusCode::BAD_REQUEST, @@ -4341,34 +4360,379 @@ mod tests { assert!(!error.to_string().contains('/')); } + enum CleanupHttpOutcome { + Removed, + Rejected(&'static str), + Panicked, + } + + struct CleanupHttpGate { + entered: tokio::sync::oneshot::Sender<()>, + release: std::sync::mpsc::Receiver<()>, + } + + struct CleanupHttpBackend { + gate: Mutex>, + outcome: CleanupHttpOutcome, + } + + fn removed_working_directory() -> WorkingDirectoryStatus { + WorkingDirectoryStatus { + summary: crate::catalog::WorkingDirectorySummary { + working_directory_id: "cleanup-test".to_string(), + display_name: None, + repository_id: "repository-test".to_string(), + creation_selector: None, + creation_ref: None, + creation_tree: None, + current_selector: None, + current_ref: None, + current_tree: None, + observed_at_epoch_seconds: None, + materializer_kind: crate::catalog::MaterializerKind::RuntimeGitClone, + cleanup_target: None, + status: crate::catalog::WorkingDirectoryStatusKind::NotFound, + cleanliness: None, + occupied_by: None, + }, + } + } + + impl WorkerExecutionBackend for CleanupHttpBackend { + fn backend_id(&self) -> &str { + "http-cleanup-test" + } + + fn spawn_worker(&self, request: WorkerExecutionSpawnRequest) -> WorkerExecutionSpawnResult { + AcceptingBackend.spawn_worker(request) + } + + fn dispatch_input( + &self, + worker_ref: &WorkerRef, + input: WorkerInput, + ) -> WorkerExecutionResult { + AcceptingBackend.dispatch_input(worker_ref, input) + } + + fn cleanup_working_directory( + &self, + working_directory_id: &str, + ) -> Result + { + assert_eq!(working_directory_id, "cleanup-test"); + let gate = self.gate.lock().unwrap().take(); + if let Some(gate) = gate { + gate.entered.send(()).expect("cleanup entry receiver"); + // A watchdog also bounds a regression that blocks the sole + // Tokio thread, where an async timeout could not make progress. + gate.release + .recv_timeout(std::time::Duration::from_secs(10)) + .expect("cleanup must be released after observations complete"); + } + match self.outcome { + CleanupHttpOutcome::Removed => Ok(removed_working_directory()), + CleanupHttpOutcome::Rejected(code) => Err( + crate::working_directory::WorkingDirectoryDiagnostic::rejected( + code, + "Resolve the current blocker and retry removal.", + ), + ), + CleanupHttpOutcome::Panicked => panic!("private provider path /secret/cleanup"), + } + } + } + + fn cleanup_http_router( + outcome: CleanupHttpOutcome, + gate: Option, + ) -> (Runtime, Router) { + let runtime = Runtime::with_execution_backend( + RuntimeOptions::default(), + Arc::new(CleanupHttpBackend { + gate: Mutex::new(gate), + outcome, + }), + ) + .unwrap(); + let identity = RuntimeIdentityMaterial::generate("runtime-cleanup-test").unwrap(); + let workspace_identity = + RuntimeIdentityMaterial::generate("workspace-cleanup-key").unwrap(); + let workspace_public_key = + crate::auth::decode_public_key(&workspace_identity.public_key).unwrap(); + let workspace_fingerprint = format!( + "sha256:{}", + crate::workspace_issuer::hex_lower(&sha2::Sha256::digest(workspace_public_key)) + ); + let app = runtime_http_router_with_workspace_auth( + runtime.clone(), + Some("cleanup-token".to_string()), + WorkspaceRuntimeHttpAuth { + verifier: WorkspaceCapabilityVerifier::new( + vec![WorkspaceIssuerTrustRecord { + workspace_id: "local".to_string(), + backend_url: "https://backend.test".to_string(), + key_id: "workspace-cleanup-key".to_string(), + algorithm: "ed25519".to_string(), + public_key: workspace_identity.public_key, + public_key_fingerprint: workspace_fingerprint, + identity_revision: 1, + trust_generation: 1, + state: WorkspaceIssuerTrustState::Active, + registered_at_unix: 1, + updated_at_unix: 1, + }], + Arc::new(InMemoryWorkspaceClaimReplayProtection::default()), + ) + .unwrap(), + signer: RuntimeVerificationSigner::from_identity(&identity).unwrap(), + verifications: Arc::new(InMemoryWorkspaceRuntimeVerificationAuthority::default()), + }, + ); + (runtime, app) + } + + #[tokio::test(flavor = "current_thread")] + async fn slow_working_directory_cleanup_does_not_block_http_ping() { + let (entered, started) = tokio::sync::oneshot::channel(); + let (release, wait_for_release) = std::sync::mpsc::channel(); + let (runtime, app) = cleanup_http_router( + CleanupHttpOutcome::Removed, + Some(CleanupHttpGate { + entered, + release: wait_for_release, + }), + ); + runtime + .store_config_bundle(test_bundle(ProfileSelector::Builtin( + "builtin:coder".to_string(), + ))) + .unwrap(); + let scope = RuntimeWorkspaceScope::new("local", "local-token"); + let mut request = task_request("unrelated Worker remains observable during cleanup"); + request.workspace_api = Some(WorkspaceApiRef { + workspace_id: "local".to_string(), + base_url: "https://backend.test".to_string(), + }); + let worker = runtime.create_worker_scoped(&scope, request).unwrap(); + assert!(worker.workdir_attachments.is_empty()); + let worker_path = format!("/v1/workers/{}", worker.worker_id); + let path = "/v1/working-directories/cleanup-test"; + let unauthorized = empty_request(app.clone(), Method::DELETE, path).await; + assert_eq!(unauthorized.status(), StatusCode::UNAUTHORIZED); + let error: RuntimeHttpErrorResponse = read_json(unauthorized).await; + assert_eq!(error.error.code, "unauthorized"); + + // A synchronous RuntimeState lock can stall this sole executor thread, + // including Tokio timeouts. Release from an independent thread on either + // completion, assertion unwind, or watchdog expiry, and report expiry as + // failure even if observations resume after the forced release. + let (observed, wait_for_observations) = std::sync::mpsc::channel(); + let watchdog = std::thread::spawn(move || { + let timed_out = matches!( + wait_for_observations.recv_timeout(std::time::Duration::from_secs(5)), + Err(std::sync::mpsc::RecvTimeoutError::Timeout) + ); + let _ = release.send(()); + timed_out + }); + let cleanup = tokio::spawn(authed_empty_request( + app.clone(), + Method::DELETE, + path, + "cleanup-token", + )); + tokio::time::timeout(std::time::Duration::from_secs(2), started) + .await + .expect("cleanup provider must be entered") + .expect("cleanup entry signal"); + + // Unlike ping, these HTTP routes synchronously acquire RuntimeState. + // Exercise existing Worker paths so authorization/not-found shortcuts + // cannot make a global occupancy-lock regression pass unnoticed. + for observation_path in [ + "/v1/workers".to_string(), + worker_path.clone(), + format!("{worker_path}/session?workspace_id=local"), + ] { + let response = tokio::time::timeout( + std::time::Duration::from_secs(2), + authed_empty_request(app.clone(), Method::GET, &observation_path, "cleanup-token"), + ) + .await + .unwrap_or_else(|_| panic!("{observation_path} must finish while cleanup is gated")); + let status = response.status(); + let body: serde_json::Value = read_json(response).await; + assert_eq!(status, StatusCode::OK, "{observation_path}: {body}"); + if observation_path == "/v1/workers" { + assert_eq!(body["workers"].as_array().unwrap().len(), 1, "{body}"); + assert_eq!( + body["workers"][0]["worker_id"], + worker.worker_id.to_string() + ); + } else if observation_path == worker_path { + assert_eq!(body["worker"]["worker_id"], worker.worker_id.to_string()); + } else { + // This minimal fake has no session store. Its normal, typed + // availability result still requires the real scoped HTTP read. + assert_eq!( + body, + serde_json::to_value(runtime_api::WorkerSessionAvailability::Unavailable { + reason: runtime_api::WorkerSessionUnavailableReason::StorageUnavailable, + message: "retained session storage is unavailable".to_string(), + }) + .unwrap(), + ); + } + } + + // Exercise the same scoped snapshot acquisition used by Runtime WS + // subscriptions, without requiring a socket or the ws-server feature. + let subscription = runtime + .subscribe_event_selector_scoped( + &scope, + protocol::subscription::EventSubscriptionSelector::RuntimeWorkers, + ) + .expect("Runtime subscription snapshot must complete while cleanup is gated"); + let protocol::subscription::SubscriptionSnapshot::Workers { workers } = + subscription.snapshot() + else { + panic!("expected Runtime Worker subscription snapshot"); + }; + assert_eq!(workers.len(), 1); + assert_eq!(workers[0].worker_id.as_str(), worker.worker_id.to_string()); + + let ping = tokio::time::timeout( + std::time::Duration::from_secs(2), + app.oneshot( + Request::builder() + .method(Method::GET) + .uri("/v1/ping") + .header(header::AUTHORIZATION, "Bearer cleanup-token") + .header(RUNTIME_WORKSPACE_SCOPE_HEADER, "local") + .body(Body::empty()) + .unwrap(), + ), + ) + .await + .expect("ping must finish while cleanup is gated") + .unwrap(); + let ping_status = ping.status(); + let ping_body: serde_json::Value = read_json(ping).await; + assert_eq!(ping_status, StatusCode::OK, "{ping_body}"); + assert_eq!(ping_body["runtime_id"], "runtime-cleanup-test"); + assert_eq!(ping_body["protocol_version"], RUNTIME_HTTP_PROTOCOL_VERSION); + assert!( + !cleanup.is_finished(), + "cleanup must still be gated after ping", + ); + // The watchdog owns the release sender; dropping `observed` on an + // assertion failure also releases the provider instead of leaking a task. + let observations_completed = observed.send(()).is_ok(); + let watchdog_expired = watchdog.join().unwrap(); + assert!( + observations_completed && !watchdog_expired, + "Worker observations, subscription snapshot, and ping must finish before cleanup is released", + ); + let response = tokio::time::timeout(std::time::Duration::from_secs(2), cleanup) + .await + .expect("released cleanup must finish") + .unwrap(); + let status = response.status(); + let body: serde_json::Value = read_json(response).await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!( + body, + serde_json::to_value(RuntimeHttpWorkingDirectoryResponse { + working_directory: removed_working_directory(), + }) + .unwrap(), + ); + } + + #[tokio::test(flavor = "current_thread")] + async fn working_directory_cleanup_panic_returns_bounded_http_error() { + let (_, app) = cleanup_http_router(CleanupHttpOutcome::Panicked, None); + let response = authed_empty_request( + app, + Method::DELETE, + "/v1/working-directories/cleanup-test", + "cleanup-token", + ) + .await; + let status = response.status(); + let body: serde_json::Value = read_json(response).await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "{body}"); + assert_eq!( + body, + serde_json::json!({ + "error": { + "code": "working_directory_task_failed", + "message": "Working directory operation failed to return a result", + }, + }), + ); + } + #[tokio::test] async fn cleanup_cause_codes_survive_http_error_serialization() { let cases = [ - ("mount_present", StatusCode::CONFLICT), - ("mount_check_unavailable", StatusCode::SERVICE_UNAVAILABLE), - ("permission_denied", StatusCode::FORBIDDEN), - ("resource_busy", StatusCode::CONFLICT), - ("storage_unavailable", StatusCode::SERVICE_UNAVAILABLE), - ("ownership_unknown", StatusCode::CONFLICT), - ("changes_present", StatusCode::CONFLICT), - ("changes_unknown", StatusCode::CONFLICT), + ("working_directory_not_found", StatusCode::NOT_FOUND), + ( + "working_directory_cleanup_mount_present", + StatusCode::CONFLICT, + ), + ( + "working_directory_cleanup_mount_check_unavailable", + StatusCode::SERVICE_UNAVAILABLE, + ), + ( + "working_directory_cleanup_permission_denied", + StatusCode::FORBIDDEN, + ), + ( + "working_directory_cleanup_resource_busy", + StatusCode::CONFLICT, + ), + ( + "working_directory_cleanup_storage_unavailable", + StatusCode::SERVICE_UNAVAILABLE, + ), + ( + "working_directory_cleanup_ownership_unknown", + StatusCode::CONFLICT, + ), + ( + "working_directory_cleanup_changes_present", + StatusCode::CONFLICT, + ), + ( + "working_directory_cleanup_changes_unknown", + StatusCode::CONFLICT, + ), ]; - for (cause, status) in cases { - let code = format!("working_directory_cleanup_{cause}"); + for (code, expected_status) in cases { + let (_, app) = cleanup_http_router(CleanupHttpOutcome::Rejected(code), None); + let response = authed_empty_request( + app, + Method::DELETE, + "/v1/working-directories/cleanup-test", + "cleanup-token", + ) + .await; + let status = response.status(); + let json: serde_json::Value = read_json(response).await; + assert_eq!(status, expected_status, "{json}"); + assert_eq!(json["error"]["code"], code, "{json}"); let diagnostic = crate::working_directory::WorkingDirectoryDiagnostic::rejected( - &code, + code, "Resolve the current blocker and retry removal.", ); - let response = - RuntimeHttpRestError::runtime(RuntimeError::WorkingDirectory(diagnostic)) - .into_response(); - assert_eq!(response.status(), status); - let body = axum::body::to_bytes(response.into_body(), 2048) - .await - .unwrap(); - let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); - assert_eq!(json["error"]["code"], code, "{json}"); - assert!(json["error"]["message"].as_str().unwrap().contains("retry")); + assert_eq!( + json["error"]["message"], + RuntimeError::WorkingDirectory(diagnostic).to_string(), + "{json}", + ); } } diff --git a/crates/worker-runtime/src/runtime.rs b/crates/worker-runtime/src/runtime.rs index 3489140e9..1c812321c 100644 --- a/crates/worker-runtime/src/runtime.rs +++ b/crates/worker-runtime/src/runtime.rs @@ -59,6 +59,54 @@ use tokio::sync::broadcast; use tokio::sync::mpsc; use uuid::Uuid; +mod workdir_session; + +#[derive(Debug)] +enum WorkdirOperation { + Use(usize), + Cleanup, +} + +/// Transient admission authority, not persisted cleanup or recovery state. +#[derive(Debug)] +struct WorkdirOperationGuard { + state: Weak>, + ids: Vec, + cleanup: bool, +} + +impl Drop for WorkdirOperationGuard { + fn drop(&mut self) { + let Some(state) = self.state.upgrade() else { + return; + }; + let mut state = state + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + for id in &self.ids { + match state.workdir_operations.get_mut(id) { + Some(WorkdirOperation::Use(count)) if !self.cleanup && *count > 1 => *count -= 1, + Some(WorkdirOperation::Use(_)) if !self.cleanup => { + state.workdir_operations.remove(id); + } + Some(WorkdirOperation::Cleanup) if self.cleanup => { + state.workdir_operations.remove(id); + } + _ => {} + } + } + } +} + +fn workdir_operation_busy() -> RuntimeError { + RuntimeError::WorkingDirectory( + crate::working_directory::WorkingDirectoryDiagnostic::rejected( + "working_directory_cleanup_resource_busy", + "Workdir has an in-progress operation; retry after it finishes", + ), + ) +} + /// Workspace-scoped Runtime authorization context supplied by a trusted backend. #[derive(Clone, Debug, Eq, PartialEq)] pub struct RuntimeWorkspaceScope { @@ -296,6 +344,37 @@ impl Drop for WorkerOperationLease { } impl Runtime { + fn reserve_workdir_use<'a>( + &self, + ids: impl IntoIterator, + ) -> Result { + let mut ids: Vec = ids.into_iter().map(str::to_string).collect(); + ids.sort(); + ids.dedup(); + let mut state = self.lock()?; + state.ensure_running()?; + for id in &ids { + state.ensure_workdir_not_removing(id)?; + } + for id in &ids { + match state + .workdir_operations + .entry(id.clone()) + .or_insert(WorkdirOperation::Use(0)) + { + WorkdirOperation::Use(count) => *count += 1, + WorkdirOperation::Cleanup => { + unreachable!("cleanup admission checked under state lock") + } + } + } + Ok(WorkdirOperationGuard { + state: Arc::downgrade(&self.inner), + ids, + cleanup: false, + }) + } + /// Create a memory-backed Runtime with generated identity. pub fn new_memory() -> Self { Self::with_options(RuntimeOptions::default()) @@ -515,8 +594,15 @@ impl Runtime { /// Create a Runtime-owned working directory through the attached execution backend. pub fn create_working_directory( &self, - request: WorkingDirectoryRequest, + mut request: WorkingDirectoryRequest, ) -> Result { + let id = request + .backend_workdir_id + .get_or_insert_with(|| { + crate::working_directory::next_working_directory_id(&request.repository.id) + }) + .clone(); + let _operation = self.reserve_workdir_use([id.as_str()])?; let backend = { let state = self.lock()?; state.ensure_running()?; @@ -581,6 +667,7 @@ impl Runtime { &self, request: WorkingDirectoryRepositoryAccessRequest, ) -> Result<(), RuntimeError> { + let _operation = self.reserve_workdir_use([request.working_directory_id.as_str()])?; let backend = { let state = self.lock()?; state.ensure_running()?; @@ -775,6 +862,7 @@ impl Runtime { working_directory_id: &str, owner_worker_ref: Option<&WorkerRef>, ) -> Result { + let operation = self.reserve_workdir_use([working_directory_id])?; let backend = { let mut state = self.lock()?; state.ensure_running()?; @@ -809,9 +897,10 @@ impl Runtime { backend .working_directory(working_directory_id) .map_err(RuntimeError::WorkingDirectory)?; - backend + let session = backend .open_workdir_session(working_directory_id) - .map_err(RuntimeError::WorkingDirectory) + .map_err(RuntimeError::WorkingDirectory)?; + Ok(workdir_session::retain_operation(session, operation)) } /// Cleanup a Runtime-owned working directory. @@ -819,30 +908,40 @@ impl Runtime { &self, working_directory_id: &str, ) -> Result { - // Hold occupancy authority through the side effect, preventing an - // attachment from entering between the check and physical removal. - // This synchronous provider call must not reenter Runtime. The Git - // materializer does filesystem/Git work only. The global guard also - // delays unrelated state operations during hashing/unlink; releasing it - // would require equivalent per-workdir exclusion on ALL attachment paths, - // not just moving the occupied check before the provider call. - let state = self.lock()?; - state.ensure_running()?; - if let Some(worker_id) = state.worker_id_for_workdir(working_directory_id) { - return Err(RuntimeError::InvalidRequest(format!( - "working directory {working_directory_id} is assigned to worker {worker_id}" - ))); - } - let backend = state.execution_backend.clone().ok_or_else(|| { - RuntimeError::ExecutionBackendUnavailable { - message: "working directory cleanup requires an execution backend".to_string(), + // Admit cleanup atomically with attachment/use admission, but reserve + // only this Workdir while the provider hashes/unlinks. No filesystem + // work or wait for a Workdir operation may hold the global state mutex. + let backend = { + let mut state = self.lock()?; + state.ensure_running()?; + if state.workdir_operations.contains_key(working_directory_id) { + return Err(workdir_operation_busy()); } - })?; - let result = backend + if let Some(worker_id) = state.worker_id_for_workdir(working_directory_id) { + return Err(RuntimeError::InvalidRequest(format!( + "working directory {working_directory_id} is assigned to worker {worker_id}" + ))); + } + let backend = state.execution_backend.clone().ok_or_else(|| { + RuntimeError::ExecutionBackendUnavailable { + message: "working directory cleanup requires an execution backend".to_string(), + } + })?; + state + .workdir_operations + .insert(working_directory_id.to_string(), WorkdirOperation::Cleanup); + backend + }; + // Construct/drop the RAII owner outside the state guard: failure, panic + // and request cancellation must not leave stale exclusion or deadlock. + let _operation = WorkdirOperationGuard { + state: Arc::downgrade(&self.inner), + ids: vec![working_directory_id.to_string()], + cleanup: true, + }; + backend .cleanup_working_directory(working_directory_id) - .map_err(RuntimeError::from); - drop(state); - result + .map_err(RuntimeError::from) } fn annotate_working_directory_statuses( @@ -1041,7 +1140,7 @@ impl Runtime { fn create_worker_with_workspace_inner( &self, - request: CreateWorkerRequest, + mut request: CreateWorkerRequest, scope: Option<&RuntimeWorkspaceScope>, ) -> Result { let operation_lock = self.worker_operation_lock(request.worker_id)?; @@ -1051,6 +1150,17 @@ impl Runtime { if let Some(existing) = self.existing_worker_for_create(&request, scope)? { return Ok(existing); } + // Assign generated materializations before publishing the durable + // request, so cleanup cannot discover an unclaimed creation mid-spawn. + for attachment in &mut request.workdir_attachment_requests { + let directory = &mut attachment.working_directory; + if directory.backend_workdir_id.is_none() { + directory.backend_workdir_id = Some( + crate::working_directory::next_working_directory_id(&directory.repository.id), + ); + } + } + let _workdir_operation = self.reserve_workdir_use(requested_workdir_ids(&request))?; self.refresh_workspace_config(&request)?; let (backend, worker_ref, spawn_request) = { let mut state = self.lock()?; @@ -1565,6 +1675,9 @@ impl Runtime { attachments.sort_by(|left, right| left.alias.cmp(&right.alias)); let mut state = self.lock()?; + for attachment in &attachments { + state.ensure_workdir_not_removing(&attachment.working_directory_id)?; + } let previous = state.worker(worker_ref)?.clone(); { let worker = state.worker_mut(worker_ref)?; @@ -1875,6 +1988,13 @@ impl Runtime { if let Some(receipt) = &owner.receipt { return Ok((Some(receipt.clone()), None)); } + if let Some(preparation) = &owner.request.preparation { + for id in + preparation_workdir_ids(preparation, state.runtime_identity.as_deref()) + { + state.ensure_workdir_not_removing(id)?; + } + } if candidate.restore_guard.active_request_id.as_deref() != Some(&request.request_id) || candidate .pending_restore @@ -2074,6 +2194,19 @@ impl Runtime { state.ensure_workspace_owner(scope, true)?; state.persist_runtime_snapshot()?; } + // Recovery uses the persisted operation, not new preparation. + // Discard it before checking Workdir admission as well as before + // publishing the owner; unrelated cleanup must not reject recovery. + if recovering_pending { + request.preparation = None; + } + if let Some(preparation) = &request.preparation { + for id in + preparation_workdir_ids(preparation, state.runtime_identity.as_deref()) + { + state.ensure_workdir_not_removing(id)?; + } + } let pending = candidate .pending_restore .unwrap_or_else(|| PendingWorkerRestore { @@ -2083,9 +2216,6 @@ impl Runtime { }); let operation_id = pending.operation_id; candidate.pending_restore = Some(pending); - if recovering_pending { - request.preparation = None; - } candidate.restore_guard.rotate(); candidate.restore_guard.active_request_id = Some(request.request_id.clone()); candidate.restore_guard.owners.insert( @@ -2517,6 +2647,28 @@ impl Runtime { ) }; + let _workdir_operation = self.reserve_workdir_use( + requested_workdir_ids(&request.request) + .into_iter() + .chain( + request + .previous_workdir_attachments + .iter() + .map(|attachment| { + attachment + .working_directory + .summary + .working_directory_id + .as_str() + }), + ) + .chain( + request + .logical_workdir_attachments + .iter() + .map(|attachment| attachment.working_directory_id.as_str()), + ), + )?; if !reconcile && let Err(result) = backend.preflight_restore(&request) { // A preflight may attest an already-connected Controller snapshot. // Repeated restore is read-only; do not replace its operation owner. @@ -4853,6 +5005,7 @@ struct RuntimeState { #[cfg(feature = "fs-store")] next_diagnostic_id: u64, workers: BTreeMap, + workdir_operations: BTreeMap, workspace_owners: BTreeMap, config_bundles: BTreeMap, workspace_config_latest: BTreeMap, @@ -4882,6 +5035,7 @@ impl RuntimeState { #[cfg(feature = "fs-store")] next_diagnostic_id: 1, workers: BTreeMap::new(), + workdir_operations: BTreeMap::new(), workspace_owners: BTreeMap::new(), config_bundles: BTreeMap::new(), workspace_config_latest: BTreeMap::new(), @@ -4912,6 +5066,7 @@ impl RuntimeState { #[cfg(feature = "fs-store")] next_diagnostic_id: 1, workers: BTreeMap::new(), + workdir_operations: BTreeMap::new(), workspace_owners: BTreeMap::new(), config_bundles: BTreeMap::new(), workspace_config_latest: BTreeMap::new(), @@ -5056,6 +5211,7 @@ impl RuntimeState { workspace_backend_resource_clients: BTreeMap::new(), next_diagnostic_id, workers, + workdir_operations: BTreeMap::new(), config_bundles: BTreeMap::new(), workspace_config_latest: BTreeMap::new(), workspace_config_fetch_gates: BTreeMap::new(), @@ -5470,6 +5626,16 @@ impl RuntimeState { } } + fn ensure_workdir_not_removing(&self, id: &str) -> Result<(), RuntimeError> { + if matches!( + self.workdir_operations.get(id), + Some(WorkdirOperation::Cleanup) + ) { + return Err(workdir_operation_busy()); + } + Ok(()) + } + fn worker_id_for_workdir(&self, working_directory_id: &str) -> Option { self.workers.values().find_map(|worker| { let has_status = worker.workdir_attachments.iter().any(|attachment| { @@ -5478,7 +5644,23 @@ impl RuntimeState { let has_request = worker.request.as_ref().is_some_and(|request| { requested_workdir_ids(request).contains(&working_directory_id) }); - (has_status || has_request).then_some(worker.worker_id) + let has_logical = worker + .logical_workdir_attachments + .iter() + .any(|attachment| attachment.working_directory_id == working_directory_id); + let has_preparation = worker + .restore_guard + .active_request_id + .as_ref() + .and_then(|id| worker.restore_guard.owners.get(id)) + .filter(|owner| owner.receipt.is_none()) + .and_then(|owner| owner.request.preparation.as_ref()) + .is_some_and(|preparation| { + preparation_workdir_ids(preparation, self.runtime_identity.as_deref()) + .contains(&working_directory_id) + }); + (has_status || has_request || has_logical || has_preparation) + .then_some(worker.worker_id) }) } @@ -6179,6 +6361,33 @@ fn durable_create_worker_request(request: &CreateWorkerRequest) -> CreateWorkerR durable } +fn preparation_workdir_ids<'a>( + preparation: &'a runtime_api::WorkerRestorePreparation, + runtime_id: Option<&str>, +) -> Vec<&'a str> { + let local = |owner: &str| runtime_id.is_none_or(|runtime_id| runtime_id == owner); + preparation + .workdir_attachments + .iter() + .flatten() + .map(|attachment| attachment.working_directory_id.as_str()) + .chain( + preparation + .repository_access_workdirs + .iter() + .filter(|reference| local(&reference.runtime_id)) + .map(|reference| reference.working_directory_id.as_str()), + ) + .chain( + preparation + .repository_access + .iter() + .filter(|access| local(&access.materialization.runtime_id)) + .map(|access| access.working_directory_id.as_str()), + ) + .collect() +} + fn requested_workdir_ids(request: &CreateWorkerRequest) -> Vec<&str> { request .workdir_attachments @@ -7641,6 +7850,35 @@ mod tests { } } + struct WorkdirEffectGate { + entered: std::sync::mpsc::Sender, + release: std::sync::mpsc::Receiver<()>, + } + + impl WorkdirEffectGate { + fn wait(slot: &Mutex>, id: &str) { + let gate = slot.lock().unwrap().take(); + if let Some(gate) = gate { + gate.entered.send(id.to_string()).unwrap(); + gate.release + .recv_timeout(std::time::Duration::from_secs(5)) + .expect("Workdir effect did not receive release before timeout"); + } + } + + fn install( + slot: &Mutex>, + ) -> ( + std::sync::mpsc::Receiver, + std::sync::mpsc::Sender<()>, + ) { + let (entered, receiver) = std::sync::mpsc::channel(); + let (sender, release) = std::sync::mpsc::channel(); + *slot.lock().unwrap() = Some(Self { entered, release }); + (receiver, sender) + } + } + #[derive(Default)] struct TestExecutionBackend { spawn_result: Mutex>, @@ -7673,6 +7911,10 @@ mod tests { repository_access_available: AtomicBool, working_directory_requests: Mutex>, cleanup_occupancy_probe: Mutex>>>, + cleanup_gate: Mutex>, + workdir_use_gate: Mutex>, + cleanup_calls: std::sync::atomic::AtomicUsize, + cleanup_panics: AtomicBool, preserve_submission_acknowledgement_id: AtomicBool, #[cfg(feature = "ws-server")] snapshots: Mutex>, @@ -7717,10 +7959,21 @@ mod tests { &self, _id: &str, ) -> Result { + self.cleanup_calls.fetch_add(1, Ordering::SeqCst); + WorkdirEffectGate::wait(&self.cleanup_gate, _id); + if self.cleanup_panics.load(Ordering::SeqCst) { + panic!("cleanup provider failed after admission"); + } if let Some(state) = self.cleanup_occupancy_probe.lock().unwrap().as_ref() { + let state = state + .try_lock() + .expect("physical cleanup must not hold global Runtime state"); assert!( - matches!(state.try_lock(), Err(std::sync::TryLockError::WouldBlock)), - "occupancy must remain exclusive during physical cleanup" + matches!( + state.workdir_operations.get(_id), + Some(WorkdirOperation::Cleanup) + ), + "the target Workdir must remain reserved during physical cleanup" ); } Err(WorkingDirectoryDiagnostic::rejected( @@ -7749,6 +8002,10 @@ mod tests { &self, request: &WorkingDirectoryRequest, ) -> Result { + WorkdirEffectGate::wait( + &self.workdir_use_gate, + request.backend_workdir_id.as_deref().unwrap(), + ); self.working_directory_requests .lock() .unwrap() @@ -7763,6 +8020,7 @@ mod tests { &self, request: &WorkingDirectoryRepositoryAccessRequest, ) -> Result<(), WorkingDirectoryDiagnostic> { + WorkdirEffectGate::wait(&self.workdir_use_gate, &request.working_directory_id); self.repository_accesses .lock() .unwrap() @@ -8068,7 +8326,7 @@ mod tests { } #[test] - fn workdir_removal_keeps_occupancy_exclusive_during_provider_side_effect() { + fn workdir_removal_reserves_only_its_target_and_releases_on_failure() { let (runtime, backend) = runtime_and_backend(); *backend.cleanup_occupancy_probe.lock().unwrap() = Some(runtime.inner.clone()); let error = runtime @@ -8078,11 +8336,384 @@ mod tests { matches!(error, RuntimeError::WorkingDirectory(ref diagnostic) if diagnostic.code == "working_directory_cleanup_resource_busy") ); - // The lock is released on failure, permitting normal attachment/retry. - assert!(runtime.inner.try_lock().is_ok()); + // Failure releases the per-target reservation for normal use/retry. + assert!(runtime.inner.lock().unwrap().workdir_operations.is_empty()); *backend.cleanup_occupancy_probe.lock().unwrap() = None; } + fn workdir_effect_request(id: Option<&str>) -> WorkingDirectoryRequest { + WorkingDirectoryRequest { + repository: WorkingDirectoryRepository { + id: "repository-1".into(), + provider: "git".into(), + source: server_api::RepositorySource { + kind: server_api::RepositorySourceKind::LocalPath, + uri: "/unused-test-repository".into(), + }, + source_revision: 1, + source_fingerprint: "sha256:test-source".into(), + selector: None, + }, + display_name: None, + materializer: MaterializerKind::RuntimeGitClone, + backend_workdir_id: id.map(str::to_string), + materialization: None, + } + } + + fn workdir_access_request(id: &str) -> WorkingDirectoryRepositoryAccessRequest { + WorkingDirectoryRepositoryAccessRequest { + working_directory_id: id.into(), + materialization: RepositoryMaterializationContext { + workspace_id: "workspace-a".into(), + runtime_id: "runtime-1".into(), + operation_id: "operation-1".into(), + config_revision: 1, + config_projection_digest: "sha256:test-projection".into(), + ssh: None, + }, + } + } + + fn assert_workdir_busy(error: RuntimeError) { + assert!( + matches!(error, RuntimeError::WorkingDirectory(ref diagnostic) + if diagnostic.code == "working_directory_cleanup_resource_busy"), + "{error:?}" + ); + } + + #[test] + fn active_workdir_cleanup_rejects_same_id_admissions_without_blocking_other_workers() { + let (runtime, backend) = runtime_and_backend(); + let owner = scope("workspace-a", "server-a"); + let worker = runtime + .create_worker_scoped(&owner, scoped_task_request("unrelated", "workspace-a")) + .unwrap(); + let id = "workdir-cleanup-active"; + let (entered, release) = WorkdirEffectGate::install(&backend.cleanup_gate); + let cleanup_runtime = runtime.clone(); + let cleanup = std::thread::spawn(move || cleanup_runtime.cleanup_working_directory(id)); + assert_eq!( + entered + .recv_timeout(std::time::Duration::from_secs(5)) + .unwrap(), + id + ); + + assert_eq!(runtime.list_workers_scoped(&owner).unwrap().len(), 1); + assert_workdir_busy(runtime.cleanup_working_directory(id).unwrap_err()); + assert_workdir_busy( + runtime + .create_working_directory(workdir_effect_request(Some(id))) + .unwrap_err(), + ); + assert_workdir_busy( + runtime + .authorize_working_directory_repository_access(workdir_access_request(id)) + .unwrap_err(), + ); + assert_workdir_busy( + runtime + .open_workdir_session_scoped(&owner, id, None) + .unwrap_err(), + ); + let mut create = scoped_task_request("conflicting attachment", "workspace-a"); + create.workdir_attachments = vec![WorkingDirectoryAttachmentClaim { + alias: workdir::WorkdirAttachmentAlias::new("checkout").unwrap(), + working_directory_id: id.into(), + relative_cwd: None, + capabilities: workdir::WorkdirSessionCapabilities::ALL, + }]; + assert_workdir_busy(runtime.create_worker_scoped(&owner, create).unwrap_err()); + let attachment = LogicalWorkdirAttachment { + alias: workdir::WorkdirAttachmentAlias::new("checkout").unwrap(), + working_directory_id: id.into(), + capabilities: workdir::WorkdirSessionCapabilities::ALL, + }; + assert_workdir_busy( + runtime + .replace_worker_workdir_attachments_scoped( + &owner, + &worker.worker_ref, + vec![attachment.clone()], + ) + .unwrap_err(), + ); + runtime + .stop_worker_scoped(&owner, &worker.worker_ref, None) + .unwrap(); + let restore = runtime.test_restore_request(&worker.worker_ref); + assert_workdir_busy( + runtime + .coordinate_worker_restore_operation( + &worker.worker_ref, + runtime_api::WorkerRestoreCoordinationRequest { + expected_observation_token: restore.expected_observation_token, + request_id: restore.request_id, + preparation: Some(runtime_api::WorkerRestorePreparation { + workdir_attachments: Some(vec![ + runtime_api::LogicalWorkdirAttachment { + alias: attachment.alias, + working_directory_id: attachment.working_directory_id, + capabilities: attachment.capabilities, + }, + ]), + ..Default::default() + }), + }, + Some(&owner), + ) + .unwrap_err(), + ); + assert!( + runtime + .lock() + .unwrap() + .worker(&worker.worker_ref) + .unwrap() + .pending_restore + .is_none() + ); + assert_eq!(runtime.list_workers_scoped(&owner).unwrap().len(), 1); + // Different IDs and Worker creation remain available during cleanup. + runtime + .create_worker_scoped( + &owner, + scoped_task_request("another unrelated", "workspace-a"), + ) + .unwrap(); + let other = runtime + .create_working_directory(workdir_effect_request(Some("workdir-other"))) + .unwrap_err(); + assert!( + matches!(other, RuntimeError::WorkingDirectory(ref diagnostic) if diagnostic.code == "working_directory_unsupported") + ); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 1); + release.send(()).unwrap(); + assert_workdir_busy(cleanup.join().unwrap().unwrap_err()); + assert!(runtime.lock().unwrap().workdir_operations.is_empty()); + } + + #[test] + fn in_flight_creation_and_access_exclude_cleanup_including_generated_ids() { + for operation in ["explicit-create", "generated-create", "access"] { + let (runtime, backend) = runtime_and_backend(); + let (entered, release) = WorkdirEffectGate::install(&backend.workdir_use_gate); + let effect_runtime = runtime.clone(); + let effect = std::thread::spawn(move || { + if operation == "access" { + effect_runtime.authorize_working_directory_repository_access( + workdir_access_request("workdir-in-use"), + ) + } else { + effect_runtime + .create_working_directory(workdir_effect_request( + (operation == "explicit-create").then_some("workdir-in-use"), + )) + .map(|_| ()) + } + }); + let id = entered + .recv_timeout(std::time::Duration::from_secs(5)) + .unwrap(); + assert!(!id.is_empty()); + assert_workdir_busy(runtime.cleanup_working_directory(&id).unwrap_err()); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 0); + runtime + .create_worker(task_request("unrelated during effect")) + .unwrap(); + release.send(()).unwrap(); + assert!(effect.join().unwrap().is_err()); + // Failed provider effects release their reservation for ordinary retry. + assert_workdir_busy(runtime.cleanup_working_directory(&id).unwrap_err()); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 1); + } + } + + #[test] + fn logical_attachments_and_unsettled_restore_preparation_exclude_cleanup() { + for pending in [false, true] { + let (runtime, backend) = runtime_and_backend(); + let owner = scope("workspace-a", "server-a"); + let worker = runtime + .create_worker_scoped(&owner, scoped_task_request("logical owner", "workspace-a")) + .unwrap(); + let id = "workdir-logical-owner"; + let attachment = LogicalWorkdirAttachment { + alias: workdir::WorkdirAttachmentAlias::new("checkout").unwrap(), + working_directory_id: id.into(), + capabilities: workdir::WorkdirSessionCapabilities::ALL, + }; + if pending { + runtime + .stop_worker_scoped(&owner, &worker.worker_ref, None) + .unwrap(); + let restore = runtime.test_restore_request(&worker.worker_ref); + let preparation = runtime_api::WorkerRestorePreparation { + workdir_attachments: Some(vec![runtime_api::LogicalWorkdirAttachment { + alias: attachment.alias, + working_directory_id: attachment.working_directory_id, + capabilities: attachment.capabilities, + }]), + ..Default::default() + }; + let coordinated = runtime + .coordinate_worker_restore_operation( + &worker.worker_ref, + runtime_api::WorkerRestoreCoordinationRequest { + expected_observation_token: restore.expected_observation_token, + request_id: restore.request_id, + preparation: Some(preparation.clone()), + }, + Some(&owner), + ) + .unwrap(); + assert_eq!(coordinated, (None, Some(preparation))); + } else { + runtime + .replace_worker_workdir_attachments_scoped( + &owner, + &worker.worker_ref, + vec![attachment], + ) + .unwrap(); + } + assert!( + matches!(runtime.cleanup_working_directory(id).unwrap_err(), RuntimeError::InvalidRequest(message) if message.contains("assigned to worker")) + ); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 0); + } + } + + #[test] + fn unowned_restore_recovery_ignores_new_preparation_for_a_workdir_being_cleaned() { + let (runtime, backend) = runtime_and_backend(); + let owner = scope("workspace-a", "server-a"); + let worker = runtime + .create_worker_scoped( + &owner, + scoped_task_request("recover persisted operation", "workspace-a"), + ) + .unwrap(); + runtime + .stop_worker_scoped(&owner, &worker.worker_ref, None) + .unwrap(); + // Persisted lifecycle recovery may have no request owner (e.g. after an + // interrupted older admission). New preparation is not its authority. + runtime + .lock() + .unwrap() + .worker_mut(&worker.worker_ref) + .unwrap() + .pending_restore = Some(PendingWorkerRestore { + operation_id: WorkerLifecycleOperationId::new(), + mode: WorkerRestoreMode::Explicit, + last_settled_status: WorkerStatus::Stopped, + }); + let id = "workdir-irrelevant-to-recovery"; + let (entered, release) = WorkdirEffectGate::install(&backend.cleanup_gate); + let cleanup_runtime = runtime.clone(); + let cleanup = std::thread::spawn(move || cleanup_runtime.cleanup_working_directory(id)); + entered + .recv_timeout(std::time::Duration::from_secs(5)) + .unwrap(); + let restore = runtime.test_restore_request(&worker.worker_ref); + let (result, preparation) = runtime + .coordinate_worker_restore_operation( + &worker.worker_ref, + runtime_api::WorkerRestoreCoordinationRequest { + expected_observation_token: restore.expected_observation_token, + request_id: restore.request_id, + preparation: Some(runtime_api::WorkerRestorePreparation { + workdir_attachments: Some(vec![runtime_api::LogicalWorkdirAttachment { + alias: workdir::WorkdirAttachmentAlias::new("ignored").unwrap(), + working_directory_id: id.into(), + capabilities: workdir::WorkdirSessionCapabilities::ALL, + }]), + ..Default::default() + }), + }, + Some(&owner), + ) + .unwrap(); + assert_eq!(result.unwrap().state, WorkerRestoreState::Accepted); + assert!(preparation.is_none()); + assert!( + runtime + .lock() + .unwrap() + .worker(&worker.worker_ref) + .unwrap() + .logical_workdir_attachments + .is_empty() + ); + release.send(()).unwrap(); + assert_workdir_busy(cleanup.join().unwrap().unwrap_err()); + } + + #[tokio::test] + async fn live_workdir_session_clones_exclude_cleanup_until_close_or_final_drop() { + for close in [false, true] { + let (runtime, backend) = runtime_and_backend(); + let root = tempfile::tempdir().unwrap(); + let id = "workdir-live-session"; + let provider: workdir::WorkdirSessionHandle = Arc::new( + workdir::LocalWorkdirSession::materialized_bound_with_environment( + workdir::Workdir::new(id), + root.path().to_path_buf(), + root.path().to_path_buf(), + manifest::SharedScope::new(manifest::Scope::writable(root.path()).unwrap()), + workdir::WorkdirSessionCapabilities::ALL, + BTreeMap::new(), + Vec::new(), + ), + ); + let session = workdir_session::retain_operation( + provider, + runtime.reserve_workdir_use([id]).unwrap(), + ); + let clone = session.clone(); + drop(session); + assert_workdir_busy(runtime.cleanup_working_directory(id).unwrap_err()); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 0); + if close { + clone.close().await.unwrap(); + clone.close().await.unwrap(); + // A closed wrapper may remain alive without reserving storage. + assert_workdir_busy(runtime.cleanup_working_directory(id).unwrap_err()); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 1); + } else { + drop(clone); + assert_workdir_busy(runtime.cleanup_working_directory(id).unwrap_err()); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 1); + } + } + } + + #[test] + fn workdir_cleanup_panic_releases_reservation_without_poisoning_runtime() { + let (runtime, backend) = runtime_and_backend(); + backend.cleanup_panics.store(true, Ordering::SeqCst); + assert!( + std::panic::catch_unwind(std::panic::AssertUnwindSafe( + || runtime.cleanup_working_directory("workdir-panic") + )) + .is_err() + ); + assert!(runtime.lock().unwrap().workdir_operations.is_empty()); + backend.cleanup_panics.store(false, Ordering::SeqCst); + runtime + .create_worker(task_request("after cleanup panic")) + .unwrap(); + assert_workdir_busy( + runtime + .cleanup_working_directory("workdir-panic") + .unwrap_err(), + ); + assert_eq!(backend.cleanup_calls.load(Ordering::SeqCst), 2); + } + #[test] fn unknown_typed_intent_is_rejected_before_backend_dispatch() { let (runtime, backend) = runtime_and_backend(); diff --git a/crates/worker-runtime/src/runtime/workdir_session.rs b/crates/worker-runtime/src/runtime/workdir_session.rs new file mode 100644 index 000000000..5a3249c0b --- /dev/null +++ b/crates/worker-runtime/src/runtime/workdir_session.rs @@ -0,0 +1,165 @@ +//! Retain a Runtime active-use reservation for a live provider session. +//! +//! Cloned handles share the reservation. A successful provider close releases +//! it even while closed handles remain; a failed or cancelled close retains it +//! until a later successful close or the final wrapper is dropped. Dropping the +//! wrapper does not close the provider or wait for outstanding commands, so +//! callers must keep a session handle alive while using provider-owned work. + +use super::WorkdirOperationGuard; +use async_trait::async_trait; +use std::sync::{Arc, Mutex}; +use tokio::sync::broadcast; +use workdir::{ + CheckoutObservation, CheckoutRequest, CheckoutResult, CheckoutSearchRequest, + CheckoutSearchResult, CommandEvent, CommandHandle, CommandOutput, CommandOutputRequest, + CommandRequest, CommandSnapshot, CommandStatus, EditRequest, EditResult, GlobRequest, + GlobResult, GrepRequest, GrepResult, ListRequest, ListResult, ReadBytesRequest, + ReadBytesResult, ReadRequest, ReadResult, StatRequest, StatResult, Workdir, WorkdirError, + WorkdirPath, WorkdirScopeAuthorizationRequest, WorkdirScopeOverlapRequest, WorkdirSession, + WorkdirSessionCapabilities, WorkdirSessionHandle, WriteRequest, WriteResult, +}; + +pub(super) fn retain_operation( + session: WorkdirSessionHandle, + operation: WorkdirOperationGuard, +) -> WorkdirSessionHandle { + Arc::new(RetainedWorkdirSession { + inner: session, + operation: Mutex::new(Some(operation)), + }) +} + +struct RetainedWorkdirSession { + inner: WorkdirSessionHandle, + operation: Mutex>, +} + +impl std::fmt::Debug for RetainedWorkdirSession { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Do not expose provider internals or reservation state containing host paths. + f.debug_struct("RetainedWorkdirSession") + .field("workdir", self.inner.workdir()) + .field("capabilities", &self.inner.capabilities()) + .finish_non_exhaustive() + } +} + +#[async_trait] +impl WorkdirSession for RetainedWorkdirSession { + fn workdir(&self) -> &Workdir { + self.inner.workdir() + } + + fn capabilities(&self) -> WorkdirSessionCapabilities { + self.inner.capabilities() + } + + async fn authorize_scope_path( + &self, + request: WorkdirScopeAuthorizationRequest, + ) -> Result<(), WorkdirError> { + self.inner.authorize_scope_path(request).await + } + + async fn scope_rules_overlap( + &self, + request: WorkdirScopeOverlapRequest, + ) -> Result { + self.inner.scope_rules_overlap(request).await + } + + async fn checkout_search( + &self, + request: CheckoutSearchRequest, + ) -> Result { + self.inner.checkout_search(request).await + } + + async fn checkout_observe( + &self, + path: WorkdirPath, + ) -> Result { + self.inner.checkout_observe(path).await + } + + async fn checkout_execute( + &self, + request: CheckoutRequest, + ) -> Result { + self.inner.checkout_execute(request).await + } + + async fn stat(&self, request: StatRequest) -> Result { + self.inner.stat(request).await + } + + async fn read(&self, request: ReadRequest) -> Result { + self.inner.read(request).await + } + + async fn read_bytes(&self, request: ReadBytesRequest) -> Result { + self.inner.read_bytes(request).await + } + + async fn write(&self, request: WriteRequest) -> Result { + self.inner.write(request).await + } + + async fn edit(&self, request: EditRequest) -> Result { + self.inner.edit(request).await + } + + async fn list(&self, request: ListRequest) -> Result { + self.inner.list(request).await + } + + async fn glob(&self, request: GlobRequest) -> Result { + self.inner.glob(request).await + } + + async fn grep(&self, request: GrepRequest) -> Result { + self.inner.grep(request).await + } + + async fn start_command(&self, request: CommandRequest) -> Result { + self.inner.start_command(request).await + } + + async fn command_status(&self, handle: CommandHandle) -> Result { + self.inner.command_status(handle).await + } + + async fn command_output( + &self, + request: CommandOutputRequest, + ) -> Result { + self.inner.command_output(request).await + } + + async fn cancel_command(&self, handle: CommandHandle) -> Result<(), WorkdirError> { + self.inner.cancel_command(handle).await + } + + fn subscribe_command_events(&self) -> Option> { + self.inner.subscribe_command_events() + } + + fn command_snapshot(&self) -> Vec { + self.inner.command_snapshot() + } + + async fn close(&self) -> Result<(), WorkdirError> { + // Provider close must succeed before releasing the active-use reservation. + // No wrapper-local terminal checks may bypass the provider's own checks. + self.inner.close().await?; + let operation = self + .operation + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .take(); + // Release outside the mutex: guard Drop may acquire Runtime state locks. + drop(operation); + Ok(()) + } +} diff --git a/crates/worker-runtime/src/worker_backend.rs b/crates/worker-runtime/src/worker_backend.rs index 9ac79973b..801838b70 100644 --- a/crates/worker-runtime/src/worker_backend.rs +++ b/crates/worker-runtime/src/worker_backend.rs @@ -1025,7 +1025,9 @@ fn runtime_local_workdir_router( command_output_scope.clone(), capabilities, binding.command_environment(), - binding.session_resources(), + binding.session_resources().map_err(|error| { + format!("open Workdir attachment `{alias}` resources: {error}") + })?, ), ) .map_err(|error| format!("bind Workdir attachment `{alias}`: {error}"))?; @@ -3453,7 +3455,7 @@ where scope, WorkdirSessionCapabilities::ALL, binding.command_environment(), - binding.session_resources(), + binding.session_resources()?, )) } @@ -9501,6 +9503,42 @@ mod tests { assert_eq!(status.summary.cleanliness.as_deref(), Some("clean")); } + #[cfg(target_os = "linux")] + #[tokio::test] + async fn local_sessions_exclude_cleanup_until_close_releases_resources() { + let runtime_base = tempfile::tempdir().unwrap(); + let repo = create_clean_repo(); + let materializer = RuntimeGitMaterializer::new(runtime_base.path()); + let binding = materializer + .create(&working_directory_request(repo.path())) + .unwrap(); + let id = &binding.working_directory.id; + let backend = WorkerRuntimeExecutionBackend::new(FailingFactory) + .unwrap() + .with_working_directory_materializer(materializer.clone()); + let session = backend.open_workdir_session(id).unwrap(); + let session_clone = session.clone(); + drop(session); + assert_eq!( + materializer.cleanup_working_directory(id).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + + let alias = WorkdirAttachmentAlias::new("checkout").unwrap(); + let attachments = BTreeMap::from([(alias, binding.clone())]); + let scope = manifest::SharedScope::new(manifest::Scope::writable(binding.root()).unwrap()); + let router = runtime_local_workdir_router(&attachments, &BTreeMap::new(), scope).unwrap(); + session_clone.close().await.unwrap(); + assert_eq!( + materializer.cleanup_working_directory(id).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + router.close_all().await.unwrap(); + // Closed handles, router and passive bindings can remain alive. + materializer.cleanup_working_directory(id).unwrap(); + assert!(!binding.working_directory_root().exists()); + } + #[test] fn spawn_failure_with_existing_working_directory_preserves_workdir() { let runtime_base = tempfile::tempdir().unwrap(); diff --git a/crates/worker-runtime/src/working_directory.rs b/crates/worker-runtime/src/working_directory.rs index 178850a55..dc27551a2 100644 --- a/crates/worker-runtime/src/working_directory.rs +++ b/crates/worker-runtime/src/working_directory.rs @@ -23,6 +23,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use workdir::WorkdirSessionResource; mod cleanup; +mod occupancy; #[cfg(all(test, target_os = "linux"))] pub(crate) use cleanup::tests::fail_once as fail_workdir_cleanup_once; @@ -101,6 +102,9 @@ pub struct WorkingDirectoryBinding { working_directory_root: PathBuf, command_environment: BTreeMap, session_resources: Vec>, + occupancy: occupancy::Occupancy, + occupancy_id: String, + generation: occupancy::Generation, } impl WorkingDirectoryBinding { @@ -120,8 +124,23 @@ impl WorkingDirectoryBinding { self.command_environment.clone() } - pub fn session_resources(&self) -> Vec> { - self.session_resources.clone() + /// Activate resources for a local session. Cloning a binding alone is passive; + /// the returned lease excludes cleanup until the last resource clone drops. + pub fn session_resources( + &self, + ) -> Result>, WorkingDirectoryDiagnostic> { + let lease = self + .occupancy + .acquire_session_use(&self.occupancy_id, &self.generation)?; + if !binding_paths_are_available(self) { + return Err(WorkingDirectoryDiagnostic::new( + "working_directory_not_found", + "Workdir binding is no longer available; acquire a fresh binding", + )); + } + let mut resources = self.session_resources.clone(); + resources.push(Arc::new(lease)); + Ok(resources) } pub fn status(&self) -> WorkingDirectoryStatus { @@ -583,6 +602,7 @@ fn repository_access_now_epoch_seconds() -> u64 { pub struct RuntimeGitMaterializer { runtime_root: PathBuf, repository_access: RepositoryAccessExpiryScheduler, + occupancy: occupancy::Occupancy, } impl RuntimeGitMaterializer { @@ -590,6 +610,7 @@ impl RuntimeGitMaterializer { Self { runtime_root: runtime_root.into(), repository_access: RepositoryAccessExpiryScheduler::new(), + occupancy: occupancy::Occupancy::default(), } } @@ -702,6 +723,9 @@ impl RuntimeGitMaterializer { working_directory_root, command_environment: BTreeMap::new(), session_resources: Vec::new(), + occupancy: self.occupancy.clone(), + occupancy_id: working_directory_id.to_string(), + generation: self.occupancy.generation(&working_directory_id), }) } @@ -904,6 +928,7 @@ impl RuntimeGitMaterializer { request: &WorkingDirectoryRequest, ) -> Result { validate_working_directory_id(&working_directory_id)?; + let _creation_lease = self.occupancy.acquire_creation(&working_directory_id)?; Self::validate_plain_http_source(request)?; let request = self.request_with_authorized_repository_access(&working_directory_id, request)?; @@ -911,20 +936,30 @@ impl RuntimeGitMaterializer { let selector = request.repository.selector.as_deref().unwrap_or("HEAD"); let working_directory_root = self.working_directory_root(&working_directory_id); let checkout_root = working_directory_root.join(CHECKOUT_DIR); - if checkout_root.exists() { + if working_directory_root.symlink_metadata().is_ok() { return Err(WorkingDirectoryDiagnostic::new( "working_directory_exists", "working directory target already exists; cleanup or choose a new working_directory", )); } - fs::create_dir_all(&working_directory_root).map_err(|_| { + fs::create_dir_all(&self.runtime_root).map_err(|_| { + WorkingDirectoryDiagnostic::new( + "working_directory_create_failed", + "failed to create working directory storage", + ) + })?; + // Prepare fallible access before owning a root that must not be adopted + // by a retry after failure. + let access = RepositoryCommandAccess::prepare(&self.runtime_root, &request)?; + // Atomically own this new root before any rollback can remove it. Do + // not adopt an existing partial-cleanup directory or another creator's root. + fs::create_dir(&working_directory_root).map_err(|_| { WorkingDirectoryDiagnostic::new( "working_directory_create_failed", "failed to create working directory; backend-private path details were omitted", ) })?; - let access = RepositoryCommandAccess::prepare(&self.runtime_root, &request)?; let mut clone = repository_git_command(&request, access.as_ref()); clone .args([ @@ -1022,6 +1057,9 @@ impl RuntimeGitMaterializer { working_directory_root: working_directory_root.clone(), command_environment: BTreeMap::new(), session_resources: Vec::new(), + occupancy: self.occupancy.clone(), + occupancy_id: working_directory_id.to_string(), + generation: self.occupancy.generation(&working_directory_id), }; if let Err(error) = self.write_record(&binding) { let _ = fs::remove_dir_all(&working_directory_root); @@ -1046,7 +1084,10 @@ impl WorkingDirectoryMaterializer for RuntimeGitMaterializer { worker_ref: &WorkerRef, request: &WorkingDirectoryRequest, ) -> Result { - let working_directory_id = Self::working_directory_id(worker_ref, &request.repository.id); + let working_directory_id = request + .backend_workdir_id + .clone() + .unwrap_or_else(|| Self::working_directory_id(worker_ref, &request.repository.id)); self.materialize_with_working_directory_id(working_directory_id, request) } @@ -1066,6 +1107,7 @@ impl WorkingDirectoryMaterializer for RuntimeGitMaterializer { request: &WorkingDirectoryRepositoryAccessRequest, ) -> Result<(), WorkingDirectoryDiagnostic> { validate_working_directory_id(&request.working_directory_id)?; + let _use_lease = self.occupancy.acquire_use(&request.working_directory_id)?; let ssh = request.materialization.ssh.as_ref().ok_or_else(|| { WorkingDirectoryDiagnostic::new( "working_directory_remote_repository_access_required", @@ -1187,6 +1229,8 @@ impl WorkingDirectoryMaterializer for RuntimeGitMaterializer { working_directory_id: &str, relative_cwd: Option<&str>, ) -> Result { + validate_working_directory_id(working_directory_id)?; + let _use_lease = self.occupancy.acquire_use(working_directory_id)?; let binding = self.checked_binding(working_directory_id, relative_cwd)?; self.bind_repository_access(working_directory_id, binding) } @@ -1281,7 +1325,10 @@ impl WorkingDirectoryMaterializer for RuntimeGitMaterializer { working_directory_id: &str, ) -> Result { validate_working_directory_id(working_directory_id)?; - cleanup::remove(self, working_directory_id) + let _cleanup_lease = self.occupancy.acquire_cleanup(working_directory_id)?; + let status = cleanup::remove(self, working_directory_id)?; + self.occupancy.invalidate_generation(working_directory_id); + Ok(status) } fn cleanup(&self, binding: &WorkingDirectoryBinding) -> Result<(), WorkingDirectoryDiagnostic> { @@ -2878,7 +2925,7 @@ fn sanitize_path_component(value: &str) -> String { } } -fn next_working_directory_id(_repository_id: &str) -> String { +pub(crate) fn next_working_directory_id(_repository_id: &str) -> String { let now = SystemTime::now() .duration_since(UNIX_EPOCH) .map(|duration| duration.as_millis() as u64) @@ -3933,7 +3980,8 @@ mod tests { #[cfg(target_os = "linux")] { // Real SSH reauthorization updated access evidence repeatedly, but - // did not replace this clean materialization or require manual recovery. + // did not replace this materialization or require manual recovery. + // Status does not inspect content; cleanup still verifies it below. assert_eq!( restored .working_directory_status(&id) @@ -3941,7 +3989,7 @@ mod tests { .summary .cleanliness .as_deref(), - Some("clean") + Some("unknown") ); restored.cleanup_working_directory(&id).unwrap(); assert!(!restored.working_directory_root(&id).exists()); @@ -4443,6 +4491,307 @@ mod tests { assert!(root.exists()); } + fn materialization_context( + ssh: Option, + ) -> crate::catalog::RepositoryMaterializationContext { + crate::catalog::RepositoryMaterializationContext { + workspace_id: "workspace-1".to_string(), + runtime_id: "runtime-1".to_string(), + operation_id: "operation-1".to_string(), + config_revision: 7, + config_projection_digest: "sha256:projection".to_string(), + ssh, + } + } + + #[cfg(target_os = "linux")] + #[test] + fn live_session_resources_block_cleanup_until_last_clone_drops() { + let repo = create_clean_repo(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let binding = materializer.create(&request(repo.path())).unwrap(); + let id = &binding.working_directory.id; + // Read bindings and materializer clones must share the same admission. + let restored = materializer.bind_working_directory(id, None).unwrap(); + let resources = restored.session_resources().unwrap(); + let resource_clone = resources.clone(); + let second_resources = binding.session_resources().unwrap(); + let clone = materializer.clone(); + + for _ in 0..2 { + assert_eq!( + clone.cleanup_working_directory(id).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + } + assert!(binding.root().join("README.md").exists()); + drop(resources); + drop(second_resources); + assert_eq!( + clone.cleanup_working_directory(id).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + drop(resource_clone); + clone.cleanup_working_directory(id).unwrap(); + // Both bindings are still alive and passive after resource release. + assert!(!binding.working_directory_root().exists()); + assert!(!restored.root().exists()); + } + + #[test] + fn active_cleanup_rejects_creation_and_access_before_effects() { + let repo = tempfile::tempdir().unwrap(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let id = "workdir-cleanup-active"; + let cleanup = materializer.occupancy.acquire_cleanup(id).unwrap(); + let mut create_request = request(repo.path()); + create_request.backend_workdir_id = Some(id.to_string()); + let access_request = WorkingDirectoryRepositoryAccessRequest { + working_directory_id: id.to_string(), + materialization: materialization_context(Some(repository_ssh_access())), + }; + + assert_eq!( + materializer.create(&create_request).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + assert_eq!( + materializer + .materialize(&worker_ref(1), &create_request) + .unwrap_err() + .code, + "working_directory_cleanup_resource_busy" + ); + assert_eq!( + materializer + .authorize_repository_access(&access_request) + .unwrap_err() + .code, + "working_directory_cleanup_resource_busy" + ); + assert_eq!( + materializer + .bind_working_directory(id, None) + .unwrap_err() + .code, + "working_directory_cleanup_resource_busy" + ); + assert!(!materializer.working_directory_root(id).exists()); + assert_eq!(materializer.repository_access.pending_count(), 0); + drop(cleanup); + // The cleanup reservation was transient, not a persisted lifecycle. + assert_ne!( + materializer.create(&create_request).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + } + + #[cfg(target_os = "linux")] + #[test] + fn active_cleanup_rejects_existing_binding_resources_but_not_other_ids() { + let repo = create_clean_repo(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let binding = materializer.create(&request(repo.path())).unwrap(); + let id = &binding.working_directory.id; + let cleanup = materializer.occupancy.acquire_cleanup(id).unwrap(); + assert_eq!( + binding.session_resources().unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + assert_eq!( + materializer.cleanup_working_directory(id).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + let other = materializer.create(&request(repo.path())).unwrap(); + let other_resources = other.session_resources().unwrap(); + drop(other_resources); + materializer + .cleanup_working_directory(&other.working_directory.id) + .unwrap(); + drop(cleanup); + let resources = binding.session_resources().unwrap(); + drop(resources); + materializer.cleanup_working_directory(id).unwrap(); + } + + #[cfg(target_os = "linux")] + #[test] + fn cleanup_error_releases_lease_for_session_and_safe_retry() { + let repo = create_clean_repo(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let binding = materializer.create(&request(repo.path())).unwrap(); + let changed = binding.root().join("untracked.txt"); + fs::write(&changed, "preserve me\n").unwrap(); + assert_eq!( + materializer + .cleanup_working_directory(&binding.working_directory.id) + .unwrap_err() + .code, + "working_directory_cleanup_changes_present" + ); + let resources = binding.session_resources().unwrap(); + fs::remove_file(changed).unwrap(); + drop(resources); + materializer + .cleanup_working_directory(&binding.working_directory.id) + .unwrap(); + } + + #[cfg(target_os = "linux")] + #[test] + fn removed_binding_cannot_activate_against_recreated_same_id() { + let repo = create_clean_repo(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let mut create = request(repo.path()); + create.backend_workdir_id = Some("workdir-reused-id".into()); + let old = materializer.create(&create).unwrap(); + materializer + .cleanup_working_directory(&old.working_directory.id) + .unwrap(); + assert_eq!( + old.session_resources().unwrap_err().code, + "working_directory_not_found" + ); + let fresh = materializer.create(&create).unwrap(); + assert_eq!( + old.session_resources().unwrap_err().code, + "working_directory_not_found" + ); + let resources = fresh.session_resources().unwrap(); + assert_eq!( + materializer + .cleanup_working_directory(&fresh.working_directory.id) + .unwrap_err() + .code, + "working_directory_cleanup_resource_busy" + ); + drop(resources); + materializer + .cleanup_working_directory(&fresh.working_directory.id) + .unwrap(); + } + + #[test] + fn same_id_creation_is_exclusive_and_never_adopts_existing_partial_root() { + let repo = tempfile::tempdir().unwrap(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let id = "workdir-creation-owner"; + let mut create = request(repo.path()); + create.backend_workdir_id = Some(id.into()); + let creation = materializer.occupancy.acquire_creation(id).unwrap(); + assert_eq!( + materializer.clone().create(&create).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + assert_eq!( + materializer + .bind_working_directory(id, None) + .unwrap_err() + .code, + "working_directory_cleanup_resource_busy" + ); + assert_eq!( + materializer.cleanup_working_directory(id).unwrap_err().code, + "working_directory_cleanup_resource_busy" + ); + assert!(!materializer.working_directory_root(id).exists()); + drop(creation); + let partial = materializer.working_directory_root(id); + fs::create_dir(&partial).unwrap(); + fs::write( + partial.join("owner-content"), + "preserve partial removal state", + ) + .unwrap(); + assert_eq!( + materializer.create(&create).unwrap_err().code, + "working_directory_exists" + ); + assert_eq!( + fs::read_to_string(partial.join("owner-content")).unwrap(), + "preserve partial removal state" + ); + } + + #[test] + fn failed_access_preparation_does_not_strand_a_creation_root() { + let repo = tempfile::tempdir().unwrap(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let mut create = request(repo.path()); + create.backend_workdir_id = Some("workdir-access-setup-failure".into()); + let mut ssh = repository_ssh_access(); + ssh.known_hosts_entry = + crate::catalog::SensitiveString::new("example.test ssh-ed25519 placeholder"); + create.repository.source = server_api::RepositorySource { + kind: server_api::RepositorySourceKind::Ssh, + uri: ssh.repository_uri.clone(), + }; + create.materialization = Some(materialization_context(Some(ssh))); + // A local storage failure stops access setup before any SSH transport or + // agent can start. The materialization must remain retryable, not adopted. + fs::write( + runtime_root.path().join(REPOSITORY_ACCESS_DIR), + "storage obstruction", + ) + .unwrap(); + assert_eq!( + materializer.create(&create).unwrap_err().code, + "working_directory_repository_access_setup_failed" + ); + assert!( + !materializer + .working_directory_root(create.backend_workdir_id.as_deref().unwrap()) + .exists() + ); + } + + #[test] + fn failed_creation_and_access_release_use_leases() { + let repo = tempfile::tempdir().unwrap(); + let runtime_root = tempfile::tempdir().unwrap(); + let materializer = RuntimeGitMaterializer::new(runtime_root.path()); + let id = "workdir-effect-errors"; + let mut create_request = request(repo.path()); + create_request.backend_workdir_id = Some(id.to_string()); + // Validation fails after admission but before effects. + create_request.repository.provider = "unsupported".to_string(); + assert_eq!( + materializer.create(&create_request).unwrap_err().code, + "working_directory_repository_provider_unsupported" + ); + drop(materializer.occupancy.acquire_cleanup(id).unwrap()); + // A clone failure has already created the target and must also release + // admission after rolling its filesystem effects back. + create_request.repository.provider = "git".to_string(); + assert_eq!( + materializer.create(&create_request).unwrap_err().code, + "working_directory_repository_clone_failed" + ); + assert!(!materializer.working_directory_root(id).exists()); + drop(materializer.occupancy.acquire_cleanup(id).unwrap()); + let access_request = WorkingDirectoryRepositoryAccessRequest { + working_directory_id: id.to_string(), + materialization: materialization_context(None), + }; + assert_eq!( + materializer + .authorize_repository_access(&access_request) + .unwrap_err() + .code, + "working_directory_remote_repository_access_required" + ); + drop(materializer.occupancy.acquire_cleanup(id).unwrap()); + assert!(materializer.bind_working_directory(id, None).is_err()); + drop(materializer.occupancy.acquire_cleanup(id).unwrap()); + } + #[test] fn cleanup_working_directory_removes_clone_and_record() { let repo = create_clean_repo(); diff --git a/crates/worker-runtime/src/working_directory/cleanup.rs b/crates/worker-runtime/src/working_directory/cleanup.rs index 037f01343..a9a2c3a50 100644 --- a/crates/worker-runtime/src/working_directory/cleanup.rs +++ b/crates/worker-runtime/src/working_directory/cleanup.rs @@ -76,19 +76,11 @@ pub(super) fn remaining_status( } let mut summary = saved.working_directory.status_summary(); summary.status = WorkingDirectoryStatusKind::CleanupPending; - summary.cleanliness = Some( - match check_mounts( - &materializer.working_directory_root(id), - &fs::read("/proc/self/mountinfo").ok()?, - ) - .and_then(|()| retry_inventory(materializer, id, &tree, &saved)) - { - Ok(_) => "clean", - Err(error) if error.code == "working_directory_cleanup_changes_present" => "dirty", - Err(_) => "unknown", - } - .to_string(), - ); + // Observation is not removal admission: do not inventory or hash the + // surviving checkout just to list it. The witness can be stale, so neither + // its old cleanliness nor a successful identity check proves current content + // is safe to remove. Actual retries still perform the full safety checks. + summary.cleanliness = Some("unknown".to_string()); Some(WorkingDirectoryStatus { summary }) } @@ -974,7 +966,7 @@ pub(super) mod tests { } #[test] - fn cleanup_pending_rechecks_current_cleanliness_and_retries_without_state_edit() { + fn cleanup_pending_observation_is_unknown_and_retry_checks_safety() { let (_runtime, materializer, binding) = fixture(); let id = &binding.working_directory.id; fail_once(&materializer, id); @@ -983,12 +975,54 @@ pub(super) mod tests { status.summary.status, WorkingDirectoryStatusKind::CleanupPending ); - assert_eq!(status.summary.cleanliness.as_deref(), Some("clean")); + assert_eq!(status.summary.cleanliness.as_deref(), Some("unknown")); let removed = materializer.cleanup_working_directory(id).unwrap(); assert_eq!(removed.summary.status, WorkingDirectoryStatusKind::NotFound); assert!(!binding.working_directory_root.exists()); } + #[test] + fn cleanup_pending_observation_does_not_open_checkout_content() { + use std::os::fd::{AsRawFd, FromRawFd}; + use std::os::unix::ffi::OsStrExt; + + let (_runtime, materializer, binding) = fixture(); + let id = &binding.working_directory.id; + fail_once(&materializer, id); + + // Observe actual file opens rather than a timing threshold or a large + // fixture. Inventory would open this surviving tracked file to hash it. + // SAFETY: inotify_init1 has no pointer arguments. + let fd = unsafe { libc::inotify_init1(libc::IN_NONBLOCK | libc::IN_CLOEXEC) }; + assert!(fd >= 0, "{}", io::Error::last_os_error()); + // SAFETY: fd is a freshly created descriptor, owned only by this File. + let mut events = unsafe { fs::File::from_raw_fd(fd) }; + let path = + std::ffi::CString::new(binding.root.join("README.md").as_os_str().as_bytes()).unwrap(); + // SAFETY: the descriptor is live and path is a valid terminated string. + let watch = + unsafe { libc::inotify_add_watch(events.as_raw_fd(), path.as_ptr(), libc::IN_OPEN) }; + assert!(watch >= 0, "{}", io::Error::last_os_error()); + + let detail = materializer.working_directory_status(id).unwrap(); + let listed = materializer.list_working_directories().unwrap(); + let mut buffer = [0u8; 4096]; + assert_eq!( + events + .read(&mut buffer) + .expect_err("status observation opened surviving checkout content") + .kind(), + io::ErrorKind::WouldBlock + ); + assert_eq!( + detail.summary.status, + WorkingDirectoryStatusKind::CleanupPending + ); + assert_eq!(detail.summary.cleanliness.as_deref(), Some("unknown")); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].summary, detail.summary); + } + #[test] fn partial_deletion_missing_checkout_and_restart_finish_with_durable_content_authority() { for removed in [ @@ -1021,7 +1055,7 @@ pub(super) mod tests { retained.summary.cleanup_target.as_ref(), Some(&binding.working_directory.cleanup_target) ); - assert_eq!(retained.summary.cleanliness.as_deref(), Some("clean")); + assert_eq!(retained.summary.cleanliness.as_deref(), Some("unknown")); assert_eq!( restarted.list_working_directories().unwrap()[0].summary, retained.summary @@ -1074,7 +1108,7 @@ pub(super) mod tests { .summary .cleanliness .as_deref(), - Some("dirty") + Some("unknown") ); assert_eq!( materializer.cleanup_working_directory(id).unwrap_err().code, @@ -1117,7 +1151,7 @@ pub(super) mod tests { .summary .cleanliness .as_deref(), - Some("clean"), + Some("unknown"), "{resolution}" ); // Renewal itself can fail at unlink. Its new witness must survive a diff --git a/crates/worker-runtime/src/working_directory/occupancy.rs b/crates/worker-runtime/src/working_directory/occupancy.rs new file mode 100644 index 000000000..c3e477bb5 --- /dev/null +++ b/crates/worker-runtime/src/working_directory/occupancy.rs @@ -0,0 +1,196 @@ +//! Transient, clone-shared admission for local filesystem effects. Bindings are +//! passive; only effects and activated session resources own leases. The mutex +//! protects admission bookkeeping only, never filesystem work. +use super::WorkingDirectoryDiagnostic; +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; + +#[derive(Clone, Debug, Default)] +pub(super) struct Occupancy { + entries: Arc>>, + generations: Arc>>>, +} + +/// Passive bindings identify a materialization generation, not merely its ID. +#[derive(Clone, Debug)] +pub(super) struct Generation(Arc<()>); + +#[derive(Debug)] +enum Entry { + Use(usize), + Create, + Cleanup, +} + +#[derive(Debug)] +pub(super) struct Lease { + occupancy: Occupancy, + id: String, +} + +fn busy() -> WorkingDirectoryDiagnostic { + WorkingDirectoryDiagnostic::new( + "working_directory_cleanup_resource_busy", + "A filesystem resource is busy or changed during removal. Release the resource and retry.", + ) +} + +impl Occupancy { + pub(super) fn acquire_use(&self, id: &str) -> Result { + let mut entries = self + .entries + .lock() + .unwrap_or_else(|error| error.into_inner()); + match entries.get_mut(id) { + Some(Entry::Cleanup | Entry::Create) => return Err(busy()), + Some(Entry::Use(count)) => *count += 1, + None => { + entries.insert(id.to_string(), Entry::Use(1)); + } + } + Ok(Lease { + occupancy: self.clone(), + id: id.to_string(), + }) + } + + pub(super) fn generation(&self, id: &str) -> Generation { + let mut generations = self + .generations + .lock() + .unwrap_or_else(|error| error.into_inner()); + Generation( + generations + .entry(id.to_string()) + .or_insert_with(|| Arc::new(())) + .clone(), + ) + } + + // Called only after successful removal, while exclusive cleanup is held. + pub(super) fn invalidate_generation(&self, id: &str) { + self.generations + .lock() + .unwrap_or_else(|error| error.into_inner()) + .remove(id); + } + + pub(super) fn acquire_session_use( + &self, + id: &str, + generation: &Generation, + ) -> Result { + let lease = self.acquire_use(id)?; + let valid = self + .generations + .lock() + .unwrap_or_else(|error| error.into_inner()) + .get(id) + .is_some_and(|current| Arc::ptr_eq(current, &generation.0)); + if !valid { + return Err(WorkingDirectoryDiagnostic::new( + "working_directory_not_found", + "Workdir binding is stale; acquire a fresh binding before opening a session", + )); + } + Ok(lease) + } + + pub(super) fn acquire_creation(&self, id: &str) -> Result { + let mut entries = self + .entries + .lock() + .unwrap_or_else(|error| error.into_inner()); + if entries.contains_key(id) { + return Err(busy()); + } + entries.insert(id.to_string(), Entry::Create); + Ok(Lease { + occupancy: self.clone(), + id: id.to_string(), + }) + } + + pub(super) fn acquire_cleanup(&self, id: &str) -> Result { + let mut entries = self + .entries + .lock() + .unwrap_or_else(|error| error.into_inner()); + if entries.contains_key(id) { + return Err(busy()); + } + entries.insert(id.to_string(), Entry::Cleanup); + Ok(Lease { + occupancy: self.clone(), + id: id.to_string(), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn use_and_cleanup_leases_release_during_unwind() { + let occupancy = Occupancy::default(); + for operation in ["use", "create", "cleanup"] { + let result = std::panic::catch_unwind(|| { + let _lease = match operation { + "cleanup" => occupancy.acquire_cleanup("workdir-panic").unwrap(), + "create" => occupancy.acquire_creation("workdir-panic").unwrap(), + _ => occupancy.acquire_use("workdir-panic").unwrap(), + }; + panic!("effect failed after admission"); + }); + assert!(result.is_err()); + drop(occupancy.acquire_use("workdir-panic").unwrap()); + drop(occupancy.acquire_cleanup("workdir-panic").unwrap()); + } + } + + #[test] + fn simultaneous_use_and_cleanup_admit_only_one_direction() { + let occupancy = Occupancy::default(); + let barrier = Arc::new(std::sync::Barrier::new(2)); + std::thread::scope(|scope| { + let use_occupancy = occupancy.clone(); + let use_barrier = barrier.clone(); + let use_thread = scope.spawn(move || { + use_barrier.wait(); + let result = use_occupancy.acquire_use("workdir-race"); + // Keep the winner's lease until both admission attempts finish. + use_barrier.wait(); + result + }); + barrier.wait(); + let cleanup = occupancy.acquire_cleanup("workdir-race"); + barrier.wait(); + let use_result = use_thread.join().unwrap(); + assert_ne!(use_result.is_ok(), cleanup.is_ok()); + let error = if let Err(error) = use_result { + error + } else { + cleanup.unwrap_err() + }; + assert_eq!(error.code, "working_directory_cleanup_resource_busy"); + }); + drop(occupancy.acquire_cleanup("workdir-race").unwrap()); + } +} + +impl Drop for Lease { + fn drop(&mut self) { + let mut entries = self + .occupancy + .entries + .lock() + .unwrap_or_else(|error| error.into_inner()); + match entries.get_mut(&self.id) { + Some(Entry::Use(count)) if *count > 1 => *count -= 1, + _ => { + entries.remove(&self.id); + } + } + } +} diff --git a/docs/development/workdir-removal.md b/docs/development/workdir-removal.md index 450d0ac5f..d0f6f6a9f 100644 --- a/docs/development/workdir-removal.md +++ b/docs/development/workdir-removal.md @@ -44,6 +44,11 @@ removal commits registry deletion only after Runtime confirms physical deletion or authoritative not-found. An unknown result leaves the registry in place so retry can reconcile the provider's current state. +A retained partial-removal Workdir is listed as `cleanup_pending` with cleanliness +`unknown`. List/detail observation checks its saved identity but does not read or +hash surviving checkout content. The saved removal witness is not a current +cleanliness assertion; only an actual removal retry runs the full content checks. + ## Runtime safety boundary The Runtime keeps removal content/identity evidence outside the tree being @@ -55,9 +60,22 @@ ignored/untracked content. Do not edit this witness manually. Mount-safe removal currently requires Linux `openat2` and `/proc` inspection. Unavailable inspection fails closed with `mount_check_unavailable`; there is no fallback to recursive deletion across an unchecked filesystem boundary. -Removal retains the Runtime occupancy mutex through its filesystem effect. -Unrelated Runtime state operations can therefore wait during hashing/unlink; -releasing it would require equivalent exclusion on all attachment paths. +Runtime HTTP list, detail and removal requests run synchronous provider work on +blocking workers rather than the HTTP executor. Removal reserves only the target +Workdir under the Runtime state mutex, then releases that mutex before content +inspection and unlink. Unrelated Worker list/detail, session observation and +subscription snapshots remain available while removal runs. Conflicting use, +attachment, creation or removal of the same Workdir is rejected promptly rather +than waiting under the global mutex. + +Provider leases protect in-flight creation/access and live session resources, +including their command tasks. Cloned sessions retain protection until safe close +or final resource release; passive bindings alone do not prevent removal. Removed +bindings cannot activate against a recreated Workdir with the same ID. Creation +owns a fresh root exclusively so failed creation cannot roll back another +creator's checkout or adopt an existing partial-removal directory. Reservations +are transient and released on provider success, error or unwind; they do not +replace the durable cleanup witness or attachment authority. ## Diagnostics From fc577163448a6bbac5500a57d3ab34bb4aaa9d3b Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 10 Oct 2026 12:04:56 +0900 Subject: [PATCH 5/5] docs: record workdir tooling and runtime feature blockers --- docs/report/2026-10-09-parallel-workdir-read-busy.md | 7 +++++++ ...026-10-09-runtime-http-only-feature-dependencies.md | 10 ++++++++++ 2 files changed, 17 insertions(+) create mode 100644 docs/report/2026-10-09-parallel-workdir-read-busy.md create mode 100644 docs/report/2026-10-09-runtime-http-only-feature-dependencies.md diff --git a/docs/report/2026-10-09-parallel-workdir-read-busy.md b/docs/report/2026-10-09-parallel-workdir-read-busy.md new file mode 100644 index 000000000..25c116f73 --- /dev/null +++ b/docs/report/2026-10-09-parallel-workdir-read-busy.md @@ -0,0 +1,7 @@ +# Parallel Workdir reads return transient provider-busy errors + +During the Workdir cleanup fix, batches of independent `Read` calls against this checkout repeatedly returned `Workdir session is unavailable: filesystem provider busy` for several calls, while one read in the same batch succeeded. Serial retries against the same paths succeeded. No live Runtime or storage repair was attempted. + +This obstructed source inspection and made parallel tool batches less useful. The observations do not establish whether contention is in the filesystem provider, tool broker, or session admission layer. + +Suggested investigation: reproduce two concurrent read-only tool calls on one attachment, inspect the admission/lease boundary, and distinguish ordinary read concurrency from destructive-operation exclusion. If temporary rejection is intentional, document which tool combinations may overlap and give a bounded retryable diagnostic. Do not relax cleanup or scope safety to accommodate parallel reads. diff --git a/docs/report/2026-10-09-runtime-http-only-feature-dependencies.md b/docs/report/2026-10-09-runtime-http-only-feature-dependencies.md new file mode 100644 index 000000000..ea7378d71 --- /dev/null +++ b/docs/report/2026-10-09-runtime-http-only-feature-dependencies.md @@ -0,0 +1,10 @@ +# Runtime HTTP-only production feature dependencies + +While validating the Workdir cleanup fix, `cargo check -p worker-runtime --no-default-features --features http-server` failed on dependencies outside the cleanup diff: + +- `http_server.rs` and `http_server/runtime_management_api.rs` use retention APIs gated by `fs-store`. +- `workspace_request.rs` imports `futures::StreamExt`, and an HTTP-enabled path in `worker_backend.rs` uses `futures::executor::block_on`, but `dep:futures` is activated by `ws-server`, not `http-server`. + +The HTTP-only **test** build with `fs-store,http-server` succeeds because `futures` is also a dev-dependency; it does not prove that configuration's production compile closure. Root `cargo check` and the default Runtime build cover the normal `ws-server,fs-store` configuration. + +This dependency mismatch was not repaired as part of the cleanup concurrency fix. Decide the supported feature surface explicitly, then align dependency ownership and retention route gating with it. Keep a production `cargo check` for each supported non-default configuration so dev-dependencies cannot mask this boundary.