diff --git a/crates/kernel/src/kernel.rs b/crates/kernel/src/kernel.rs index 360847b4..78482254 100644 --- a/crates/kernel/src/kernel.rs +++ b/crates/kernel/src/kernel.rs @@ -60,6 +60,9 @@ struct Io { /// host-managed server stops when it reaches EOF, which also happens /// when this process dies, so a crashed shell leaves no kernel behind. lifeline: Option, + /// The task copying the child's stderr into the [`Tail`]; it ends at + /// the end of the pipe, after the kernel's last line. + stderr: Option>, } /// Keeps the last lines the kernel wrote to stderr, to say why it exited. @@ -116,7 +119,7 @@ fn start(launch: &Launch, network: &Network, log: &LogSink, tail: &Tail) -> Resu return Err("the kernel's stdin/stdout are not piped".into()); }; // The kernel logs to stderr; it never carries protocol frames. - if let Some(stderr) = child.stderr.take() { + let stderr = child.stderr.take().map(|stderr| { let log = log.clone(); let tail = tail.clone(); tokio::spawn(async move { @@ -126,14 +129,14 @@ fn start(launch: &Launch, network: &Network, log: &LogSink, tail: &Tail) -> Resu (log)(&format!("octos: {line}")); tail.push(line); } - }); - } + }) + }); let lines = BufReader::new(Box::pin(stdout) as Pin>).lines(); Ok(if shared { // Frames go over the WebSocket once it is up (`prepare_network`). - Io { writer: Box::pin(tokio::io::sink()), lines, running: Running::Child(child), lifeline: Some(stdin) } + Io { writer: Box::pin(tokio::io::sink()), lines, running: Running::Child(child), lifeline: Some(stdin), stderr } } else { - Io { writer: Box::pin(stdin), lines, running: Running::Child(child), lifeline: None } + Io { writer: Box::pin(stdin), lines, running: Running::Child(child), lifeline: None, stderr } }) } #[cfg(target_env = "ohos")] @@ -153,6 +156,7 @@ fn start(launch: &Launch, network: &Network, log: &LogSink, tail: &Tail) -> Resu lines: BufReader::new(Box::pin(reader) as Pin>).lines(), running: Running::Embedded(task), lifeline: None, + stderr: None, }) } #[cfg(not(target_env = "ohos"))] @@ -266,7 +270,7 @@ async fn exited(running: &mut Running) -> String { async fn stop(io: Io) { // Closing stdin stops either kind of child: the stdio pipe drains owned // turns on EOF; a host-managed server drains its connections and exits. - let Io { writer, lines, mut running, lifeline } = io; + let Io { writer, lines, mut running, lifeline, .. } = io; let grace = if lifeline.is_some() { SHARED_GRACE } else { STDIO_GRACE }; drop(writer); drop(lines); @@ -401,16 +405,18 @@ pub(crate) async fn supervise( } } } - Ok(None) => break CloseReason::Exited(with_tail("the kernel closed its output", &tail)), + Ok(None) => break CloseReason::Exited("the kernel closed its output".into()), Err(e) => break CloseReason::Exited(format!("reading from the kernel failed: {e}")), }, - why = exited(&mut io.running) => break CloseReason::Exited(with_tail(&why, &tail)), + why = exited(&mut io.running) => break CloseReason::Exited(why), } }}; (log)(&format!("octos-core: stopping kernel {generation}: {reason}")); + let stderr = io.stderr.take(); stop(io).await; match reason { - CloseReason::Failed(why) => CloseReason::Failed(with_tail(&why, &tail)), + CloseReason::Failed(why) => CloseReason::Failed(last_words(&why, stderr, &tail).await), + CloseReason::Exited(why) => CloseReason::Exited(last_words(&why, stderr, &tail).await), other => other, } } @@ -464,6 +470,24 @@ fn tool_list_reply(text: &str) -> Option { }) } +/// How long a stopped kernel's stderr may take to reach its end. The pipe +/// closes with the kernel; only a process it started that inherited the +/// pipe can hold it open longer. +const LAST_WORDS_GRACE: Duration = Duration::from_secs(2); + +/// `why` with the kernel's last words (the [`Tail`]), once `stderr`, the +/// task reading them, has reached the end of the pipe. That task runs +/// beside the supervisor, so lines the kernel wrote just before its stdout +/// closed or it exited can still be on their way to the tail: read at once, +/// the reason said "the kernel closed its output: fake kernel up" without +/// the "asked to exit" the kernel wrote last. +async fn last_words(why: &str, stderr: Option>, tail: &Tail) -> String { + if let Some(reader) = stderr { + let _ = tokio::time::timeout(LAST_WORDS_GRACE, reader).await; + } + with_tail(why, tail) +} + fn with_tail(why: &str, tail: &Tail) -> String { let tail = tail.text(); if tail.is_empty() { diff --git a/crates/kernel/tests/core.rs b/crates/kernel/tests/core.rs index 2a74ba25..43d9d545 100644 --- a/crates/kernel/tests/core.rs +++ b/crates/kernel/tests/core.rs @@ -238,7 +238,9 @@ async fn a_kernel_that_dies_closes_its_connections_with_its_last_words() { call(&mut a, "1", "session/list", json!({})).await; a.send(json!({"jsonrpc": "2.0", "method": "test/exit", "params": {}}).to_string()).unwrap(); match next(&mut a).await { - Err(CloseReason::Exited(why)) => assert!(why.contains("asked to exit") || why.contains("exit"), "{why}"), + // The line it wrote to stderr last, whichever end the core saw + // first: its output closing or its exit status. + Err(CloseReason::Exited(why)) => assert!(why.contains("asked to exit"), "{why}"), other => panic!("expected Exited, got {other:?}"), } for _ in 0..100 { diff --git a/crates/shell/src/apps.rs b/crates/shell/src/apps.rs index 5cba914d..64b30f32 100644 --- a/crates/shell/src/apps.rs +++ b/crates/shell/src/apps.rs @@ -141,6 +141,32 @@ pub fn system_card_apps() -> Vec { Vec::new() } +/// System apps a test registers for itself (the glance tile tests' +/// probes). They join App Hub's registry, the one [`system_card_apps`] +/// lists, which is process-wide and never forgets an app, so a test of the +/// build's own catalog saw them or not depending on which tests had run +/// before it in the same process. A test registers its app here, and the +/// catalog tests leave these out. +#[cfg(test)] +pub(crate) mod test_system_apps { + use std::sync::Mutex; + + static IDS: Mutex> = Mutex::new(Vec::new()); + + /// Register `app` for a test. It is noted first, so a catalog that + /// lists it was read after it was noted. + #[cfg(feature = "app-hub")] + pub(crate) fn register(app: octosense_appstore::system::SystemApp) { + IDS.lock().unwrap_or_else(|e| e.into_inner()).push(app.id); + octosense_appstore::system::register_system_app(app); + } + + /// Whether a test registered the system app `id` (an `os.` id). + pub(crate) fn is_test_app(id: &str) -> bool { + IDS.lock().unwrap_or_else(|e| e.into_inner()).contains(&id) + } +} + /// The services contained apps call through `host.request` (ADR 0004) /// that are not the assistant's, registered once, before the first system /// app can open: `mail` keeps accounts and passwords for the Mail app; @@ -658,12 +684,19 @@ mod tests { .collect() } + /// The build's own rows among `rows`: without the system apps other + /// tests registered for themselves ([`super::test_system_apps`]). + /// `rows` must be read before this looks, as an argument is. + fn build_rows(rows: Vec) -> Vec { + rows.into_iter().filter(|row| !card_manifest_id(row).is_some_and(super::test_system_apps::is_test_app)).collect() + } + #[cfg(feature = "mobile-apps")] #[test] fn bundled_apps_open_without_catalog_files_or_child_processes() { use makepad_widgets::*; let _one_rinx = crate::module_host::RINX_INSTANCE_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let catalog = bundled_catalog(); + let catalog = build_rows(bundled_catalog()); // The linked modules in link order (AppCard is opt-in, `app-appcard`, // not part of `mobile-apps`; `settings` is the phone product's), then // the system apps no native module of the same id replaces. @@ -718,7 +751,7 @@ mod tests { let mut cx = Cx::new(Box::new(|_, _| {})); cx.with_vm(makepad_widgets::script_mod); let mut host = crate::module_host::ModuleHost::default(); - for (index, app) in bundled_catalog().iter().enumerate() { + for (index, app) in build_rows(bundled_catalog()).iter().enumerate() { let module = registry.module(&app.id).unwrap(); let client = index as u64 + 1; host.create(&mut cx, client, module, module_open(module, app).unwrap(), dvec2(400.0, 700.0)).unwrap(); @@ -768,7 +801,7 @@ mod tests { fn the_system_apps_ship_as_card_apps() { let registry = AppRegistry::default(); let native = registry.linked_ids(); - let ids: Vec = system_card_apps().into_iter().map(|app| app.id).collect(); + let ids: Vec = build_rows(system_card_apps()).into_iter().map(|app| app.id).collect(); let expected: Vec<&str> = system_app_ids().iter().copied().filter(|id| !native.contains(id)).collect(); assert_eq!(ids, expected); for id in &ids { diff --git a/crates/shell/src/glance_card.rs b/crates/shell/src/glance_card.rs index 7716a342..6906a252 100644 --- a/crates/shell/src/glance_card.rs +++ b/crates/shell/src/glance_card.rs @@ -839,7 +839,8 @@ mod tests { } /// A system app, registered from a pack made on the fly, whose manifest - /// grants `glance` and `storage` and nothing else. + /// grants `glance` and `storage` and nothing else. It is registered as + /// a test's, so the catalog tests running beside these leave it out. #[cfg(feature = "app-hub")] fn register_test_app(id: &'static str) { let dir = std::env::temp_dir().join(format!("glance-tile-app-{id}-{}", std::process::id())); @@ -852,7 +853,7 @@ mod tests { ); std::fs::write(dir.join("manifest.json"), manifest).unwrap(); let pack = serde_json::to_string(&octosense_app_hub::pack::pack_dir(&dir).unwrap()).unwrap(); - octosense_appstore::system::register_system_app(octosense_appstore::system::SystemApp { id, name: "Glance test", pack: Box::leak(pack.into_boxed_str()), assets: &[] }); + crate::apps::test_system_apps::register(octosense_appstore::system::SystemApp { id, name: "Glance test", pack: Box::leak(pack.into_boxed_str()), assets: &[] }); } /// A Cx with the widgets and the tile frame registered, for driving