diff --git a/crates/op-host-services/src/mcp_serve/tool_profile.rs b/crates/op-host-services/src/mcp_serve/tool_profile.rs index 8246ed62c..733ca48d7 100644 --- a/crates/op-host-services/src/mcp_serve/tool_profile.rs +++ b/crates/op-host-services/src/mcp_serve/tool_profile.rs @@ -262,6 +262,16 @@ impl Default for McpAccessProfile { } } +/// Whether calling `name` will mutate the document. +/// +/// Used by the connection tier to decide, BEFORE dispatch, whether a `/mcp` +/// call needs a shutdown write pass. Unclassified names default to `Write` +/// (see [`access_of`]), which is the safe direction here too: an unknown tool +/// is admitted through the barrier rather than slipping past it. +pub fn tool_writes(name: &str) -> bool { + matches!(access_of(name), ToolAccess::Write) +} + /// The classification for `name`, if it is in the static catalog. pub fn profile_for(name: &str) -> Option<&'static ToolProfile> { TOOL_PROFILES.iter().find(|profile| profile.name == name) diff --git a/crates/op-host-services/src/web_canvas_server/collab_state.rs b/crates/op-host-services/src/web_canvas_server/collab_state.rs index 940875b28..d2c8232d9 100644 --- a/crates/op-host-services/src/web_canvas_server/collab_state.rs +++ b/crates/op-host-services/src/web_canvas_server/collab_state.rs @@ -349,6 +349,13 @@ impl WebCanvasState { jian_ops_schema::image_thumbs::discard_for_document(prepared.document()); return IngestOutcome::Rejected; } + // Captured before the install activates the incoming document's + // thumbnails. If the session then REJECTS the edit, rolling the + // document back does not roll these back: the previous document's + // pending seed was consumed by its own activation, so re-activating it + // is a no-op and the refused document's thumbnails keep resolving live + // ids. Restoring the snapshot on the rejection path is what undoes it. + let thumbnails_before = jian_ops_schema::image_thumbs::capture_snapshot(); // The capture is now open and MUST be closed on every path. The guard // is what makes that true even if the install below panics: an // unclosed capture leaves the session permanently unable to accept @@ -365,8 +372,16 @@ impl WebCanvasState { match capture.finish() { op_collab_host::LocalEditOutcome::Committed => IngestOutcome::Committed, op_collab_host::LocalEditOutcome::NoChange => IngestOutcome::NoChange, - op_collab_host::LocalEditOutcome::Rejected => IngestOutcome::Rejected, - op_collab_host::LocalEditOutcome::Failed => IngestOutcome::Failed, + // The document was rolled back, so the thumbnails must roll back + // with it — otherwise the refused document's images stay active. + op_collab_host::LocalEditOutcome::Rejected => { + jian_ops_schema::image_thumbs::restore_snapshot(thumbnails_before); + IngestOutcome::Rejected + } + op_collab_host::LocalEditOutcome::Failed => { + jian_ops_schema::image_thumbs::restore_snapshot(thumbnails_before); + IngestOutcome::Failed + } } } } diff --git a/crates/op-host-services/src/web_canvas_server/collab_state_tests.rs b/crates/op-host-services/src/web_canvas_server/collab_state_tests.rs index 1a2c531a7..62928bc78 100644 --- a/crates/op-host-services/src/web_canvas_server/collab_state_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/collab_state_tests.rs @@ -378,3 +378,51 @@ fn a_gated_push_still_owns_its_document_so_the_seed_is_released() { let refused = state.apply_prepared_document_push(push, None); assert!(refused.is_err(), "a viewer's push must be refused"); } + +#[test] +fn a_rejected_ingest_rolls_the_thumbnail_registry_back_too() { + // The document rolls back on a rejection, but the thumbnails did not: the + // previous document's pending seed was consumed by its own activation, so + // re-activating it is a no-op and the REFUSED document's images kept + // resolving live ids. + use crate::web_canvas_server::{IngestOutcome, PendingDocumentPush, ServeMode}; + + // A distinctive id no other test uses, so this is immune to the + // process-global registry being cleared in parallel. + const KEPT: u64 = 515_243_617; + jian_ops_schema::image_thumbs::store_thumb(KEPT, vec![4, 5, 6]); + + // A projected Active phase with no real session actor: the runtime cannot + // open a capture, so the ingest is refused. + let mut state = daemon(); + in_session( + &mut state, + CollabConnectionPhase::Active, + CollabUiRole::Owner, + ); + + let body = serde_json::json!({ + "document": { + "version": "1.0.0", + "children": [{ + "id": "n1", "type": "rectangle", "name": "refused", + "x": 0, "y": 0, "width": 4, "height": 4, + }], + "imageThumbs": { "999111": "AQID" }, + }, + "sourceClientId": "s", + }) + .to_string(); + let mut push = PendingDocumentPush::parse(&body, ServeMode::Local).expect("parses"); + let prepared = push.prepared.take().expect("a document push"); + + assert_eq!( + state.ingest_document_in_session(prepared), + IngestOutcome::Rejected + ); + assert_eq!( + jian_ops_schema::image_thumbs::thumb_for(KEPT).as_deref(), + Some(&[4u8, 5, 6][..]), + "a refused ingest must leave the registry as it found it" + ); +} diff --git a/crates/op-host-services/src/web_canvas_server/connection.rs b/crates/op-host-services/src/web_canvas_server/connection.rs index 7bcf84a31..117b2ba0c 100644 --- a/crates/op-host-services/src/web_canvas_server/connection.rs +++ b/crates/op-host-services/src/web_canvas_server/connection.rs @@ -338,20 +338,18 @@ pub(super) fn dispatch( // A document write must hold a pass for its whole locked segment, so // shutdown cannot snapshot the document between the pass being taken // and the commit landing. - let write_pass = match (ctx.write_barrier, pending_push.is_some()) { - (Some(barrier), true) => match barrier.enter() { - Some(pass) => Some(pass), - None => { - crate::mcp_serve::write_mcp_http_response_with_origin( - stream, - "503 Service Unavailable", - r#"{"ok":false,"error":"shutting-down","message":"this daemon is stopping and cannot accept writes"}"#, - cors_origin, - )?; - return Ok(false); - } - }, - _ => None, + let write_pass = match admit_mutation(ctx, pending_push.is_some()) { + MutationAdmission::NotAWrite => None, + MutationAdmission::Admitted(pass) => pass, + MutationAdmission::ShuttingDown => { + crate::mcp_serve::write_mcp_http_response_with_origin( + stream, + "503 Service Unavailable", + SHUTTING_DOWN_REST_BODY, + cors_origin, + )?; + return Ok(false); + } }; let reply = { let mut guard = state.lock().unwrap_or_else(|p| p.into_inner()); @@ -472,6 +470,36 @@ pub(super) fn dispatch( )?; return Ok(false); } + // A `/mcp` call that will mutate the document is admitted through the same + // barrier the REST push uses. `tool_profile` already knows which tools + // write, so the decision is made BEFORE dispatch — the alternative is + // discovering it from an `EditorCommand` the tool has already produced, + // which is after the point where refusing is still honest. + let mcp_write = op_mcp::parse_tool_call(&req.body) + .is_some_and(|call| crate::mcp_serve::tool_profile::tool_writes(&call.tool)); + let mcp_write_pass = match admit_mutation(ctx, mcp_write) { + MutationAdmission::NotAWrite => None, + MutationAdmission::Admitted(pass) => pass, + MutationAdmission::ShuttingDown => { + // A tools/call error envelope, not a transport failure: an MCP + // client must be able to read the refusal and keep its session. + let refusal = op_mcp::parse_tool_call(&req.body).map(|call| { + op_mcp::tool_response_to_json(&op_mcp::ToolResponse::Err { + id: call.id.clone(), + code: op_mcp::ToolErrorCode::ToolFailed, + message: "shutting-down: this daemon is stopping and cannot accept writes" + .to_string(), + }) + }); + crate::mcp_serve::write_mcp_http_response_with_origin( + stream, + "200 OK", + refusal.as_deref().unwrap_or(SHUTTING_DOWN_REST_BODY), + cors_origin, + )?; + return Ok(false); + } + }; // JSON-RPC `/mcp` dispatch against the in-memory document. A mutating apply // bumps the sync version, broadcast to SSE subscribers so the browser shell // sees JSON-RPC-driven changes too. @@ -531,6 +559,8 @@ pub(super) fn dispatch( } response }; + // Released only once the commit is visible in the shared state. + drop(mcp_write_pass); let status = if response.is_empty() { "202 Accepted" } else { @@ -540,6 +570,39 @@ pub(super) fn dispatch( Ok(false) } +/// The body a REST write gets once shutdown has closed the barrier. +const SHUTTING_DOWN_REST_BODY: &str = r#"{"ok":false,"error":"shutting-down","message":"this daemon is stopping and cannot accept writes"}"#; + +/// Whether a mutation may proceed, and the pass that proves it is in flight. +pub(super) enum MutationAdmission<'a> { + /// Not a document mutation — nothing to admit. + NotAWrite, + /// Admitted. `None` when this deployment runs no barrier (local/managed). + Admitted(Option>), + /// The daemon is stopping and will not durably accept this write. + ShuttingDown, +} + +/// The single admission point for every document mutation. +/// +/// Both the REST push and the `/mcp` write dispatch go through here, so a +/// mutation route added later cannot quietly skip the barrier — which is +/// exactly how `/mcp` came to be writing during shutdown while REST was +/// refused. +pub(super) fn admit_mutation<'a>(ctx: &ConnCtx<'a>, is_write: bool) -> MutationAdmission<'a> { + if !is_write { + return MutationAdmission::NotAWrite; + } + let Some(barrier) = ctx.write_barrier else { + // Local and managed daemons have no flush to protect. + return MutationAdmission::Admitted(None); + }; + match barrier.enter() { + Some(pass) => MutationAdmission::Admitted(Some(pass)), + None => MutationAdmission::ShuttingDown, + } +} + /// Stream Server-Sent Events to a subscribed client: write the SSE headers, /// emit the current tick immediately (initial sync), then forward each /// bump from `rx` as a `data: {"version":N,"collabSeq":M}` event. A periodic diff --git a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs index fee0c45c1..14fdb55da 100644 --- a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs +++ b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs @@ -44,6 +44,14 @@ use super::*; /// enough that a deploy is not held up by a long-lived SSE stream. const SHUTDOWN_DRAIN_SECS: u64 = 10; +/// How often the write drain reports the blocked-writer count. +const WRITE_DRAIN_REPORT_SECS: u64 = 5; + +/// The ceiling on waiting for in-flight writes. Flushing over a live writer +/// loses acked work, so this is deliberately far longer than the connection +/// drain — and `stop_grace_period` must exceed it. +const WRITE_DRAIN_HARD_CAP_SECS: u64 = 60; + /// Wait for the active-connection count to reach zero, up to the bound. /// /// Returns whether it actually drained. SSE streams routinely outlive this, @@ -60,19 +68,47 @@ fn drain_connections(conn_count: &Arc) -> bool { conn_count.load(Ordering::Acquire) == 0 } -/// Wait for in-flight document writes to finish, up to the bound. +/// Wait for in-flight document writes to finish. /// /// Separate from the connection drain: an SSE stream holds a connection for -/// minutes and is safe to flush past, while a write in progress is not. +/// minutes and is safe to flush past, while a write in progress is not — +/// flushing over one loses work a client was already told had landed. +/// +/// So this does NOT give up after the ordinary drain window. It keeps waiting +/// to a hard ceiling, reporting the blocked-writer count every +/// [`WRITE_DRAIN_REPORT_SECS`] so an operator can see why the stop is slow. +/// Reaching the ceiling is an error, not a routine outcome: `stop_grace_period` +/// must exceed [`WRITE_DRAIN_HARD_CAP_SECS`] or the container is killed +/// mid-flush regardless of what this does. fn drain_write_barrier(barrier: &Arc) -> bool { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(SHUTDOWN_DRAIN_SECS); - while std::time::Instant::now() < deadline { + let started = std::time::Instant::now(); + let mut next_report = std::time::Duration::from_secs(WRITE_DRAIN_REPORT_SECS); + loop { if barrier.active() == 0 { return true; } + let waited = started.elapsed(); + if waited >= std::time::Duration::from_secs(WRITE_DRAIN_HARD_CAP_SECS) { + eprintln!( + "openpencil --serve-web --online: ERROR {} write(s) still in flight after {}s; \ + flushing anyway — those clients were acked and may not be persisted. Raise \ + stop_grace_period above {}s.", + barrier.active(), + WRITE_DRAIN_HARD_CAP_SECS, + WRITE_DRAIN_HARD_CAP_SECS + ); + return false; + } + if waited >= next_report { + eprintln!( + "openpencil --serve-web --online: waiting on {} in-flight write(s) ({}s)", + barrier.active(), + waited.as_secs() + ); + next_report += std::time::Duration::from_secs(WRITE_DRAIN_REPORT_SECS); + } std::thread::sleep(std::time::Duration::from_millis(20)); } - barrier.active() == 0 } /// Longest gap between idle sweeps, however long the idle deadline is. @@ -233,15 +269,8 @@ pub fn run_online_web_canvas(options: ServeWebOptions) -> Result<()> { // holding a pass would otherwise commit after the flush snapshotted the // document, having already answered 200. write_barrier.close(); - let writes_settled = drain_write_barrier(&write_barrier); - if !writes_settled { - eprintln!( - "openpencil --serve-web --online: {} write(s) still in flight after {}s; \ - flushing anyway — those clients were acked but may not be persisted", - write_barrier.active(), - SHUTDOWN_DRAIN_SECS - ); - } + // Reports and escalates internally; a `false` means the hard cap was hit. + let _writes_settled = drain_write_barrier(&write_barrier); let drained = drain_connections(&conn_count); let flush_started = std::time::Instant::now(); let flushed = registry.flush_all(); diff --git a/crates/op-host-services/src/web_canvas_server/online_shutdown_tests.rs b/crates/op-host-services/src/web_canvas_server/online_shutdown_tests.rs index 313e84710..1064e3c13 100644 --- a/crates/op-host-services/src/web_canvas_server/online_shutdown_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/online_shutdown_tests.rs @@ -81,3 +81,79 @@ fn a_write_during_shutdown_is_refused_rather_than_acked() { ); assert_eq!(body(&response)["error"], "shutting-down"); } + +#[test] +fn an_mcp_write_tool_is_refused_once_the_barrier_closes() { + // `/mcp` used to apply straight to the editor with no barrier, so a write + // tool could commit after the flush snapshot — the exact window the REST + // path was already protected from. + use crate::web_canvas_server::tenant::WriteBarrier; + let registry = registry(); + let verifier = verifier(); + let barrier = WriteBarrier::default(); + barrier.close(); + + let call = r#"{"jsonrpc":"2.0","id":7,"method":"tools/call","params":{"name":"add_page","arguments":{"name":"x"}}}"#; + let request = Request::json("POST", "/mcp", call).with_bearer("tokA"); + let mut stream = MockStream { + input: std::io::Cursor::new(request.wire().into_bytes()), + output: Vec::new(), + }; + serve_one_online(&mut stream, ®istry, &verifier, &barrier).expect("serve"); + let response = String::from_utf8_lossy(&stream.output).into_owned(); + // A tools/call error envelope, not a transport failure: the client keeps + // its session and can read why. + assert_eq!(status_line(&response), "HTTP/1.1 200 OK", "{response}"); + let payload = body(&response); + assert_eq!(payload["result"]["isError"], true, "{response}"); + assert!( + payload["result"]["content"][0]["text"] + .as_str() + .unwrap_or_default() + .contains("shutting-down"), + "{response}" + ); +} + +#[test] +fn an_mcp_read_tool_still_works_while_shutting_down() { + // Only writes are refused: a read cannot be lost by the flush. + use crate::web_canvas_server::tenant::WriteBarrier; + let barrier = WriteBarrier::default(); + barrier.close(); + + let call = r#"{"jsonrpc":"2.0","id":8,"method":"tools/call","params":{"name":"get_document_info","arguments":{}}}"#; + let request = Request::json("POST", "/mcp", call).with_bearer("tokA"); + let mut stream = MockStream { + input: std::io::Cursor::new(request.wire().into_bytes()), + output: Vec::new(), + }; + serve_one_online(&mut stream, ®istry(), &verifier(), &barrier).expect("serve"); + let response = String::from_utf8_lossy(&stream.output).into_owned(); + assert_ne!(body(&response)["result"]["isError"], true, "{response}"); +} + +#[test] +fn the_write_classification_matches_the_tool_catalog() { + // The barrier decision is made before dispatch, from this metadata. + use crate::mcp_serve::tool_profile::tool_writes; + for write in [ + "add_page", + "insert_node", + "delete_node", + "undo", + "batch_design", + ] { + assert!(tool_writes(write), "{write}"); + } + for read in [ + "get_node", + "list_pages", + "get_document_info", + "snapshot_layout", + ] { + assert!(!tool_writes(read), "{read}"); + } + // Unclassified tools are admitted through the barrier rather than past it. + assert!(tool_writes("add_some_dynamic_kit_component")); +} diff --git a/crates/op-host-web/src/canvaskit/inner.rs b/crates/op-host-web/src/canvaskit/inner.rs index 143b3eddb..31bdbc873 100644 --- a/crates/op-host-web/src/canvaskit/inner.rs +++ b/crates/op-host-web/src/canvaskit/inner.rs @@ -178,15 +178,13 @@ impl crate::repaint_ctx::RepaintContext for CkInner { &mut self.host } - fn reset_persistence_baselines(&mut self) { - // Recomputed from the account's own freshly loaded state on the next - // comparison, rather than carried over from the previous account's. - self.settings_fingerprint = None; - // Rebuilt from the state the account's own partition just loaded, so - // the next comparison measures against THIS account rather than - // reporting the whole partition as a change. - self.credential_fingerprint = - crate::web_settings::credential_fingerprint(self.host.editor_state()); + fn reset_persistence_baselines(&mut self, load: &crate::web_settings::CredentialLoad) { + // Through the SAME constructors mount uses. A bare recompute set + // `settings_fingerprint` to `None` — which the save gate reads as + // "never save" — and dropped the credential write-pending retry and + // the fail-closed `write_disabled` an unsupported snapshot sets. + self.settings_fingerprint = load.initial_settings_fingerprint(self.host.editor_state()); + self.credential_fingerprint = load.initial_fingerprint(self.host.editor_state()); } fn viewport_size(&self) -> (f32, f32) { self.backend.logical_size() diff --git a/crates/op-host-web/src/live_sync_identity.rs b/crates/op-host-web/src/live_sync_identity.rs index ab6cf37cc..6ad9889e7 100644 --- a/crates/op-host-web/src/live_sync_identity.rs +++ b/crates/op-host-web/src/live_sync_identity.rs @@ -63,6 +63,11 @@ pub(crate) fn reset_for_new_identity(inner: &Rc &WidgetHost; fn host_mut(&mut self) -> &mut WidgetHost; - /// Forget the settings/credential change-detection baselines. + /// Rebuild the settings/credential change-detection baselines from the + /// partition that was just loaded. /// /// Called when the tab switches accounts. The baselines were computed /// against the PREVIOUS account's state, so keeping them means the first - /// comparison after the switch reports a spurious change and writes — or - /// uploads — the new account's partition against the old one's shape. + /// comparison after the switch reports a spurious change and writes the + /// new account's partition against the old one's shape. + /// + /// `load` carries the semantics a bare recompute would throw away — the + /// write-pending retry, and the fail-closed `write_disabled` an + /// unsupported snapshot sets — so it is rebuilt through the same + /// `initial_*` constructors mount uses, not from the state alone. /// /// Defaulted to a no-op: a backend with no persistence baselines (the - /// smoke harness) has nothing to forget. - fn reset_persistence_baselines(&mut self) {} + /// smoke harness) has nothing to rebuild. + fn reset_persistence_baselines(&mut self, load: &crate::web_settings::CredentialLoad) { + let _ = load; + } /// Logical viewport size in CSS pixels. File-open/import flows fit the /// loaded content to this size after replacing the document. fn viewport_size(&self) -> (f32, f32); diff --git a/crates/op-host-web/src/web_credential_sync.rs b/crates/op-host-web/src/web_credential_sync.rs index 9123dcc96..bc7e5d99b 100644 --- a/crates/op-host-web/src/web_credential_sync.rs +++ b/crates/op-host-web/src/web_credential_sync.rs @@ -186,6 +186,25 @@ thread_local! { /// request, so it is safe to run ahead of the postMessage bridge init gate. pub(crate) fn reset() { SYNC_STATE.with(|state| *state.borrow_mut() = CredentialSyncState::default()); + // Any callback already in flight was issued for the PREVIOUS account. + // Clearing the queue cannot recall it, so the epoch is what makes it inert + // when it lands — see `identity_is_current`. + ISSUED_EPOCH.with(|epoch| epoch.set(crate::identity_epoch::epoch())); +} + +thread_local! { + /// The identity epoch the in-flight requests were issued under. + static ISSUED_EPOCH: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// Whether a completing callback still belongs to the account that issued it. +/// +/// An XHR cannot be un-issued: `reset` empties the queue, but a POST already +/// on the wire will still complete and its callback would fold the previous +/// account's result — a success, a retry schedule, an error banner — into the +/// new account's state. Comparing epochs at completion is what discards it. +fn identity_is_current() -> bool { + ISSUED_EPOCH.with(std::cell::Cell::get) == crate::identity_epoch::epoch() } /// Begin credential-policy discovery against the daemon. This issues a daemon @@ -241,6 +260,9 @@ fn request_repaint() { fn fetch_policy() { let base = crate::daemon_base::daemon_base(); let on_response: Rc = Rc::new(|status, body| { + if !identity_is_current() { + return; // issued for a previous account + } let policy = parse_policy_response(status, &body); if policy.is_none() { report_sync_failure(Some(status)); @@ -263,6 +285,11 @@ fn fetch_policy() { fn post_credentials(body: &str) { let base = crate::daemon_base::daemon_base(); let on_response: Rc = Rc::new(|status, _| { + if !identity_is_current() { + // Issued for a previous account: its result must not become the + // new account's success, retry schedule or error banner. + return; + } if !(200..300).contains(&status) { report_sync_failure(Some(status)); } @@ -290,6 +317,11 @@ fn schedule_retry(delay_ms: i32, generation: u64) { use wasm_bindgen::JsCast; let callback = wasm_bindgen::closure::Closure::::once_into_js(move || { + if !identity_is_current() { + // A timer armed for a previous account: firing it would resend + // that account's credential payload into the new tenant. + return; + } let action = SYNC_STATE.with(|state| state.borrow_mut().retry_due(generation)); dispatch(action); }); diff --git a/crates/op-host-web/src/web_settings.rs b/crates/op-host-web/src/web_settings.rs index 28e0120ba..569a50fe5 100644 --- a/crates/op-host-web/src/web_settings.rs +++ b/crates/op-host-web/src/web_settings.rs @@ -30,6 +30,42 @@ const CREDENTIAL_PAYLOAD_VERSION: u32 = 2; const STORAGE_KEY: &str = "openpencil-rust-web-settings"; const CREDENTIAL_STORAGE_KEY: &str = "openpencil-rust-web-credentials"; +/// Restore every setting the partition snapshot owns to its default. +/// +/// `apply_payload` only writes fields the stored blob actually carries, so an +/// EMPTY partition left the previous account's values in place: B inherited +/// A's locale, recent files, MCP port and provider profiles. Resetting first +/// makes "absent from the blob" mean "default" rather than "keep whatever was +/// there". +/// +/// The field set is exactly what `apply_payload` writes, so the two cannot +/// drift into a field that is saved per-account but never reset. +/// +/// `theme_mode` is included: it is stored IN the per-account blob, so it is +/// account-scoped by the same definition as everything else here. (A device +/// preference that ought to survive an account switch would have to move out +/// of the settings payload first — a product decision, not one this reset can +/// make unilaterally.) +pub(super) fn reset_account_scoped_settings(state: &mut EditorState) { + let defaults = op_editor_core::AgentSettings::default(); + let eui = &mut state.editor_ui; + eui.theme_mode = op_editor_core::EditorUiState::default().theme_mode; + eui.locale = op_editor_core::EditorUiState::default().locale; + eui.recent_files.clear(); + eui.agent_settings.mcp_server.port = defaults.mcp_server.port; + eui.agent_settings.mcp_cli_enabled = defaults.mcp_cli_enabled; + eui.agent_settings.images_advanced_open = defaults.images_advanced_open; + eui.agent_settings.openverse_client_id = defaults.openverse_client_id; + eui.agent_settings.openverse_client_secret = defaults.openverse_client_secret; + eui.agent_settings.auto_update_enabled = defaults.auto_update_enabled; + eui.agent_settings.experimental_features_enabled = defaults.experimental_features_enabled; + eui.agent_settings.builtin_agents = defaults.builtin_agents; + eui.agent_settings.next_builtin_agent_id = defaults.next_builtin_agent_id; + eui.agent_settings.image_gen_profiles = defaults.image_gen_profiles; + eui.agent_settings.next_image_gen_profile_id = defaults.next_image_gen_profile_id; + eui.agent_settings.active_image_gen_profile_id = defaults.active_image_gen_profile_id; +} + /// Re-read account-scoped storage after the tab's partition changed. /// /// The shell loads settings and credentials at mount, before any @@ -43,12 +79,15 @@ pub(crate) fn reload_for_active_partition let Ok(mut context) = inner.try_borrow_mut() else { return; }; - let _ = storage::load_into(context.host_mut().editor_state_mut()); + // Defaults first, then the target partition on top: without this an empty + // partition silently inherits the previous account's settings. + reset_account_scoped_settings(context.host_mut().editor_state_mut()); + let load = storage::load_into(context.host_mut().editor_state_mut()); // AFTER the load, not before: the baselines must measure against the state // this partition just produced. Rebuilding them from the previous // account's state would make the very next comparison report the whole // partition as a change and write it back under the wrong key. - context.reset_persistence_baselines(); + context.reset_persistence_baselines(&load); context.host_mut().mark_editor_state_dirty(); let _ = context.repaint(); } @@ -92,6 +131,7 @@ use storage::{ }; pub(crate) use storage::{ credential_migration_pending, load_into, save_credentials_if_changed, save_if_changed, + CredentialLoad, }; #[derive(Debug, Clone, PartialEq, Eq)] @@ -106,6 +146,15 @@ pub(crate) struct Fingerprint { recent_files: Vec, } +impl CredentialFingerprint { + /// The fail-closed flag, for tests that assert an unsupported snapshot + /// keeps writes disabled. + #[cfg(test)] + pub(crate) fn write_disabled_for_test(&self) -> bool { + self.write_disabled + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct CredentialFingerprint { builtin_agents: Vec, diff --git a/crates/op-host-web/src/web_settings_partition_tests.rs b/crates/op-host-web/src/web_settings_partition_tests.rs new file mode 100644 index 000000000..dd3e12a40 --- /dev/null +++ b/crates/op-host-web/src/web_settings_partition_tests.rs @@ -0,0 +1,115 @@ +//! Account-partition isolation: what an empty partition must clear, and what +//! the rebuilt persistence baselines must preserve. +//! +//! Split out of `web_settings_tests.rs` at the 800-line cap; nested under it +//! so `use super::*` still reaches the shared helpers. + +use super::*; + +#[test] +fn an_empty_partition_clears_the_previous_accounts_credentials() { + // The account-switch leak: after switching, the in-memory state still held + // account A's API keys, and an empty partition for B meant "keep whatever + // was there" instead of "no keys". + let mut state = EditorState::new(); + let mut writes = Vec::new(); + // A signs in and stores a key. + let credential_json = r#"{"version":2,"builtin_agents":[{"id":"a1","preset":"custom", + "display_name":"A's model","kind":"openai-compat","api_key":"sk-account-a", + "model":"m","base_url":"https://api.example.com/v1","enabled":true}], + "image_gen_profiles":[],"active_image_gen_profile_id":null,"openverse_oauth":null}"#; + super::storage::load_into_with(&mut state, None, Some(credential_json), |k, v| { + writes.push((k.to_string(), v.to_string())); + true + }); + assert!( + !state.editor_ui.agent_settings.builtin_agents.is_empty(), + "A's credential must load in the first place" + ); + + // B signs in: their partition is empty. + super::storage::load_into_with(&mut state, None, None, |_, _| true); + assert!( + state.editor_ui.agent_settings.builtin_agents.is_empty(), + "B must not inherit A's API keys from an empty partition" + ); +} + +#[test] +fn an_empty_partition_resets_account_scoped_settings_to_defaults() { + // `apply_payload` only writes fields the blob carries, so an empty + // partition used to leave A's locale, recent files and provider config in + // place for B. + let mut state = EditorState::new(); + state.editor_ui.locale = Locale::Ja; + state.editor_ui.recent_files = vec![RecentFile { + path: "/a/secret-project.op".into(), + modified_at: 1, + }]; + state.editor_ui.agent_settings.mcp_server.port = 4242; + state.editor_ui.agent_settings.openverse_client_id = "a-client".into(); + + super::reset_account_scoped_settings(&mut state); + super::storage::load_into_with(&mut state, None, None, |_, _| true); + + let defaults = op_editor_core::EditorUiState::default(); + let default_agents = op_editor_core::AgentSettings::default(); + assert_eq!(state.editor_ui.locale, defaults.locale, "locale must reset"); + assert!( + state.editor_ui.recent_files.is_empty(), + "B must not see A's recent files" + ); + assert_eq!( + state.editor_ui.agent_settings.mcp_server.port, + default_agents.mcp_server.port + ); + assert!(state + .editor_ui + .agent_settings + .openverse_client_id + .is_empty()); +} + +#[test] +fn a_populated_partition_still_wins_over_the_defaults() { + // The reset must not erase the partition being loaded — defaults first, + // then the target snapshot on top. + let mut state = EditorState::new(); + state.editor_ui.locale = Locale::Ja; + let settings = r#"{"version":1,"locale":"fr","mcp_port":5150}"#; + + super::reset_account_scoped_settings(&mut state); + super::storage::load_into_with(&mut state, Some(settings), None, |_, _| true); + + assert_eq!(state.editor_ui.locale, Locale::Fr); + assert_eq!(state.editor_ui.agent_settings.mcp_server.port, 5150); +} + +#[test] +fn rebuilt_baselines_keep_saving_and_keep_failing_closed() { + // The regression this pins: setting `settings_fingerprint` to `None` + // makes the save gate (`if let Some(..)`) skip forever, so nothing was + // ever persisted again after an account switch. + let mut state = EditorState::new(); + let healthy = super::storage::load_into_with(&mut state, None, None, |_, _| true); + assert!( + healthy.initial_settings_fingerprint(&state).is_some(), + "a healthy partition must leave the save path enabled" + ); + assert!(!healthy + .initial_fingerprint(&state) + .write_disabled_for_test()); + + // …and an unsupported snapshot must still fail closed rather than being + // "reset" into a writable baseline. + let unsupported = r#"{"version":9999}"#; + let mut state = EditorState::new(); + let blocked = super::storage::load_into_with(&mut state, Some(unsupported), None, |_, _| true); + assert!( + blocked.initial_settings_fingerprint(&state).is_none(), + "an unsupported snapshot must keep settings writes disabled" + ); + assert!(blocked + .initial_fingerprint(&state) + .write_disabled_for_test()); +} diff --git a/crates/op-host-web/src/web_settings_tests.rs b/crates/op-host-web/src/web_settings_tests.rs index 16982fd22..c01eb7ab1 100644 --- a/crates/op-host-web/src/web_settings_tests.rs +++ b/crates/op-host-web/src/web_settings_tests.rs @@ -738,31 +738,5 @@ fn pending_legacy_migration_blocks_an_ordinary_settings_write() { assert_ne!(settings_baseline, fingerprint(&state)); } -#[test] -fn an_empty_partition_clears_the_previous_accounts_credentials() { - // The account-switch leak: after switching, the in-memory state still held - // account A's API keys, and an empty partition for B meant "keep whatever - // was there" instead of "no keys". - let mut state = EditorState::new(); - let mut writes = Vec::new(); - // A signs in and stores a key. - let credential_json = r#"{"version":2,"builtin_agents":[{"id":"a1","preset":"custom", - "display_name":"A's model","kind":"openai-compat","api_key":"sk-account-a", - "model":"m","base_url":"https://api.example.com/v1","enabled":true}], - "image_gen_profiles":[],"active_image_gen_profile_id":null,"openverse_oauth":null}"#; - super::storage::load_into_with(&mut state, None, Some(credential_json), |k, v| { - writes.push((k.to_string(), v.to_string())); - true - }); - assert!( - !state.editor_ui.agent_settings.builtin_agents.is_empty(), - "A's credential must load in the first place" - ); - - // B signs in: their partition is empty. - super::storage::load_into_with(&mut state, None, None, |_, _| true); - assert!( - state.editor_ui.agent_settings.builtin_agents.is_empty(), - "B must not inherit A's API keys from an empty partition" - ); -} +#[path = "web_settings_partition_tests.rs"] +mod partition_tests; diff --git a/vendor/jian b/vendor/jian index af3b7bfe3..f0c24707d 160000 --- a/vendor/jian +++ b/vendor/jian @@ -1 +1 @@ -Subproject commit af3b7bfe3dbd95baaa5886b43b17f2d70d7fc18f +Subproject commit f0c24707d9cebca63b6b24a89bca41b769889989