From da65753d8196e0d2f384dcb6b8bd152d2f366dba Mon Sep 17 00:00:00 2001 From: lanyue-llk <270302213+lanyue-llk@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:09:25 +0800 Subject: [PATCH] fix(reliability): harden diagnostics and test infrastructure --- .github/actions/use-ci-release/action.yml | 15 ++++ .github/scripts/unpack-ci-release.sh | 3 + .github/scripts/unpack-ci-release.test.cjs | 60 +++++++++++++++ moli-fetch/src/tests/support.rs | 73 +++++++++++++++++- moli-layout/tests/phase4_layout_contract.rs | 13 ++++ moli-layout/tests/phase5_output_contract.rs | 85 +++++++++++++++++++-- moli/src/telemetry.rs | 28 +++++++ moli/tests/fetch_cli.rs | 67 ++++++++++++++++ 8 files changed, 336 insertions(+), 8 deletions(-) create mode 100644 .github/scripts/unpack-ci-release.test.cjs diff --git a/.github/actions/use-ci-release/action.yml b/.github/actions/use-ci-release/action.yml index 9b4582de7..5c305e988 100644 --- a/.github/actions/use-ci-release/action.yml +++ b/.github/actions/use-ci-release/action.yml @@ -13,6 +13,21 @@ runs: using: composite steps: - name: Download release artifact + id: download + continue-on-error: true + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 + with: + name: moli-release-${{ inputs.side }} + path: target/ci-artifacts/${{ inputs.side }} + - name: Check downloaded archive completeness + id: archive + continue-on-error: true + shell: bash + env: + RELEASE_SIDE: ${{ inputs.side }} + run: gzip -t "target/ci-artifacts/$RELEASE_SIDE/moli-release-$RELEASE_SIDE.tar.gz" + - name: Retry incomplete release download + if: steps.download.outcome != 'success' || steps.archive.outcome != 'success' uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: name: moli-release-${{ inputs.side }} diff --git a/.github/scripts/unpack-ci-release.sh b/.github/scripts/unpack-ci-release.sh index 1311d6fc4..6f734de81 100755 --- a/.github/scripts/unpack-ci-release.sh +++ b/.github/scripts/unpack-ci-release.sh @@ -16,6 +16,9 @@ esac release_name="moli-release-$side" archive="target/ci-artifacts/$side/$release_name.tar.gz" test -f "$archive" +# A download action can terminate with a partial extracted archive. Reject it +# before touching the executable directory, including after the bounded retry. +gzip -t "$archive" mkdir -p target/ci-bin tar -xzf "$archive" -C target/ci-bin (cd "target/ci-bin/$release_name" && sha256sum --check SHA256SUMS) diff --git a/.github/scripts/unpack-ci-release.test.cjs b/.github/scripts/unpack-ci-release.test.cjs new file mode 100644 index 000000000..4a18b374c --- /dev/null +++ b/.github/scripts/unpack-ci-release.test.cjs @@ -0,0 +1,60 @@ +'use strict'; + +const assert = require('node:assert/strict'); +const { spawnSync } = require('node:child_process'); +const { createHash } = require('node:crypto'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const test = require('node:test'); + +const script = path.join(__dirname, 'unpack-ci-release.sh'); + +function fixture(t) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'moli-ci-release-test-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const packageDir = path.join(root, 'package', 'moli-release-head'); + const archive = path.join(root, 'target', 'ci-artifacts', 'head', 'moli-release-head.tar.gz'); + fs.mkdirSync(packageDir, { recursive: true }); + fs.mkdirSync(path.dirname(archive), { recursive: true }); + const binary = Buffer.from('test release binary'); + fs.writeFileSync(path.join(packageDir, 'moli'), binary); + fs.writeFileSync(path.join(packageDir, 'revision.txt'), 'expected-revision\n'); + fs.writeFileSync(path.join(packageDir, 'SHA256SUMS'), + `${createHash('sha256').update(binary).digest('hex')} moli\n`); + function pack() { + const result = spawnSync('tar', ['-czf', archive, '-C', path.join(root, 'package'), 'moli-release-head']); + assert.equal(result.status, 0, String(result.stderr)); + } + pack(); + return { root, archive, packageDir, pack }; +} + +function unpack(root, revision = 'expected-revision') { + return spawnSync('bash', [script, 'head', revision], { cwd: root, encoding: 'utf8' }); +} + +test('valid archive passes payload and exact revision verification', (t) => { + const { root } = fixture(t); + const result = unpack(root); + assert.equal(result.status, 0, result.stderr); + assert.equal(fs.readFileSync(path.join(root, 'target/ci-bin/moli-release-head/moli'), 'utf8'), 'test release binary'); +}); + +test('truncated download cannot write a partial executable; redownload recovers', (t) => { + const { root, archive } = fixture(t); + const complete = fs.readFileSync(archive); + fs.writeFileSync(archive, complete.subarray(0, complete.length - 16)); + assert.notEqual(unpack(root).status, 0); + assert.equal(fs.existsSync(path.join(root, 'target/ci-bin')), false); + fs.writeFileSync(archive, complete); + assert.equal(unpack(root).status, 0); +}); + +test('complete downloads still reject wrong revisions and altered payloads', (t) => { + const { root, packageDir, pack } = fixture(t); + assert.notEqual(unpack(root, 'wrong-revision').status, 0); + fs.writeFileSync(path.join(packageDir, 'moli'), 'tampered payload'); + pack(); + assert.notEqual(unpack(root).status, 0); +}); diff --git a/moli-fetch/src/tests/support.rs b/moli-fetch/src/tests/support.rs index 4f7d02611..92f5c7b09 100644 --- a/moli-fetch/src/tests/support.rs +++ b/moli-fetch/src/tests/support.rs @@ -1003,14 +1003,85 @@ impl ScriptedHttpServer { } } +#[test] +fn scripted_server_waits_for_request_on_nonblocking_accepted_socket() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let mut client = TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + client + .set_read_timeout(Some(Duration::from_secs(2))) + .unwrap(); + let (stream, _) = listener.accept().unwrap(); + // macOS may inherit this mode from the listening socket. + stream.set_nonblocking(true).unwrap(); + let hits = Arc::new(AtomicUsize::new(0)); + let requests = Arc::new(Mutex::new(Vec::new())); + let responses = Arc::new(Mutex::new(VecDeque::from([ScriptedResponse::ok("ready")]))); + let (done_tx, done_rx) = std_mpsc::channel(); + let handler = { + let hits = Arc::clone(&hits); + let requests = Arc::clone(&requests); + thread::spawn(move || { + handle_scripted_connection(stream, hits, requests, responses); + done_tx.send(()).unwrap(); + }) + }; + assert!( + matches!( + done_rx.recv_timeout(Duration::from_millis(100)), + Err(std_mpsc::RecvTimeoutError::Timeout) + ), + "a connection without HTTP bytes must not receive a scripted response" + ); + client + .write_all(b"GET /delayed HTTP/1.1\r\nHost: localhost\r\n\r\n") + .unwrap(); + let mut response = String::new(); + client.read_to_string(&mut response).unwrap(); + handler.join().unwrap(); + assert!(response.ends_with("ready")); + assert_eq!(hits.load(Ordering::SeqCst), 1); + assert!(requests.lock()[0].starts_with("GET /delayed HTTP/1.1\r\n")); +} + +#[test] +fn scripted_server_does_not_consume_response_for_closed_unused_connection() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let client = TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + let (stream, _) = listener.accept().unwrap(); + drop(client); + let hits = Arc::new(AtomicUsize::new(0)); + let requests = Arc::new(Mutex::new(Vec::new())); + let responses = Arc::new(Mutex::new(VecDeque::from([ScriptedResponse::ok("ready")]))); + handle_scripted_connection( + stream, + Arc::clone(&hits), + Arc::clone(&requests), + Arc::clone(&responses), + ); + assert_eq!(hits.load(Ordering::SeqCst), 0); + assert!(requests.lock().is_empty()); + assert_eq!(responses.lock().len(), 1); +} + fn handle_scripted_connection( mut stream: std::net::TcpStream, hits: Arc, requests: Arc>>, responses: Arc>>, ) { + stream + .set_nonblocking(false) + .expect("scripted HTTP connection should use blocking reads"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("scripted HTTP connection should have a bounded read"); let mut request = [0; 1024]; - let bytes_read = stream.read(&mut request).unwrap_or(0); + let bytes_read = stream + .read(&mut request) + .expect("read scripted HTTP request"); + if bytes_read == 0 { + return; + } let request_text = String::from_utf8_lossy(&request[..bytes_read]).into_owned(); requests.lock().push(request_text); let _ = hits.fetch_add(1, Ordering::SeqCst) + 1; diff --git a/moli-layout/tests/phase4_layout_contract.rs b/moli-layout/tests/phase4_layout_contract.rs index f77a572fa..728800a5a 100644 --- a/moli-layout/tests/phase4_layout_contract.rs +++ b/moli-layout/tests/phase4_layout_contract.rs @@ -1586,6 +1586,19 @@ fn inline_blocks_use_their_internal_last_line_baseline_and_overflow_fallback() { ); assert_close(baselines[0], more.y + more.height); assert_close(baselines[1], more.y + more.height); + + // Extra block-end padding moves the text-backed peers by the same amount + // while leaving the clipped inline-block's top edge fixed. + let fallback_news = rect(&fallback, RED); + let fallback_hao = rect(&fallback, GREEN); + styles + .primary + .insert(5, nav_item_style(BLUE, 29.0, Overflow::Hidden)); + let deeper_fallback = render(&source, &mut styles, 300, 100); + assert_close(rect(&deeper_fallback, RED).y, fallback_news.y + 10.0); + assert_close(rect(&deeper_fallback, GREEN).y, fallback_hao.y + 10.0); + assert_close(rect(&deeper_fallback, BLUE).y, more.y); + assert_close(rect(&deeper_fallback, BLUE).height, more.height + 10.0); } #[test] diff --git a/moli-layout/tests/phase5_output_contract.rs b/moli-layout/tests/phase5_output_contract.rs index 408f34afe..c79e0cda7 100644 --- a/moli-layout/tests/phase5_output_contract.rs +++ b/moli-layout/tests/phase5_output_contract.rs @@ -766,12 +766,48 @@ fn auto_scrollbar_feedback_reveals_the_perpendicular_axis() { assert!(extent.vertical_scrollbar.is_some()); } +fn fixed_inline_font() -> (ResolvedLayoutStyle, DocumentLayoutServices) { + use style::values::computed::font::{ + FamilyName, FontFamily, FontFamilyList, FontFamilyNameSyntax, SingleFontFamily, + }; + let mut font = style::properties::style_structs::Font::initial_values(); + font.set_font_family(FontFamily { + families: FontFamilyList { + list: style::ArcSlice::from_iter(std::iter::once(SingleFontFamily::FamilyName( + FamilyName { + name: Atom::from("Moli Ahem"), + syntax: FontFamilyNameSyntax::Quoted, + }, + ))), + }, + is_system_font: false, + is_initial: false, + }); + let style = ResolvedLayoutStyle::from_stylo( + style::properties::ComputedValues::initial_values_with_font_override(font), + ); + let mut services = + DocumentLayoutServices::with_system_font_policy(moli_layout::SystemFontPolicy::Disabled); + services + .register_web_font(moli_layout::WebFontRegistration::new( + "fixed", + moli_layout::WebFontFace::new("Moli Ahem"), + include_bytes!("fixtures/moli-ahem.ttf").to_vec(), + )) + .unwrap(); + (style, services) +} + #[test] fn scrollbar_feedback_rebreaks_the_reused_inline_layout_at_its_final_width() { - const TEXT: &str = "alpha beta gamma delta epsilon zeta eta theta iota kappa"; + const TEXT: &str = concat!( + "alpha beta gamma delta epsilon zeta eta theta iota kappa ", + "supercalifragilisticexpialidocious", + ); let source = Source(vec![ Node::element("root", vec![1]), Node::element("scroller", vec![2]), + Node::element("fixed-font", vec![3]), Node::text("text", TEXT), ]); let mut styles = Styles::default(); @@ -788,8 +824,9 @@ fn scrollbar_feedback_rebreaks_the_reused_inline_layout_at_its_final_width() { height: length(40.0), }, overflow: Point { - // Only vertical feedback is needed to narrow the text. - // Horizontal overflow depends on platform font advances. + // Only the vertical scrollbar narrows the text. Keep the + // long word's horizontal overflow without showing a + // horizontal scrollbar. x: Overflow::Hidden, y: Overflow::Scroll, }, @@ -798,13 +835,32 @@ fn scrollbar_feedback_rebreaks_the_reused_inline_layout_at_its_final_width() { ), ); - let feedback = build(&source, &mut styles); + // The old system-font fixture happened to fit on macOS but exposed + // hanging-space overflow on Linux. Bind shaping to the same test face. + let (font_style, mut fixed_services) = fixed_inline_font(); + styles.0.insert(2, font_style); + let feedback = build_layout_pass( + &source, + &mut styles, + &mut fixed_services, + LayoutPassRequest::new(LayoutViewport::new(320, 240, 1.0), LayoutFlushReason::Test), + ) + .unwrap(); assert_eq!(feedback.metrics.numeric_layout_pass_count, 2); assert_eq!( feedback.element_metrics_for_source(1).unwrap().client_size, moli_layout::LayoutSize::new(85.0, 40.0), ); - let feedback_text = feedback.client_rects_for_source(2); + let scroller_box = feedback.source_output(1).unwrap().principal_box.unwrap(); + let extent = feedback.scroll_extent(scroller_box).unwrap(); + assert!(extent.vertical_scrollbar.is_some()); + assert!(extent.horizontal_scrollbar.is_none()); + assert!(!extent.allows_user_scroll_x); + let feedback_text = feedback.text_range_rects(3, 0..TEXT.encode_utf16().count()); + assert!( + !feedback_text.is_empty(), + "compare real text fragments, not element-only client rects" + ); // Lay out the same paragraph directly at the converged 85px content // width. Its line fragments must match the scrollbar-corrected result; @@ -822,8 +878,23 @@ fn scrollbar_feedback_rebreaks_the_reused_inline_layout_at_its_final_width() { }, ), ); - let direct = build(&source, &mut styles); - assert_eq!(feedback_text, direct.client_rects_for_source(2)); + let direct = build_layout_pass( + &source, + &mut styles, + &mut fixed_services, + LayoutPassRequest::new(LayoutViewport::new(320, 240, 1.0), LayoutFlushReason::Test), + ) + .unwrap(); + assert_eq!( + feedback_text, + direct.text_range_rects(3, 0..TEXT.encode_utf16().count()) + ); + assert!( + feedback_text + .iter() + .any(|quad| quad.points.iter().any(|point| point.x > 85.0)), + "the unbreakable final word should retain horizontal overflow" + ); } #[test] diff --git a/moli/src/telemetry.rs b/moli/src/telemetry.rs index 830307bc7..626b89d53 100644 --- a/moli/src/telemetry.rs +++ b/moli/src/telemetry.rs @@ -19,6 +19,7 @@ pub fn init(log_filter: &str) { .with_env_filter(filter) .with_target(false) .with_writer(std::io::stderr) + .log_internal_errors(false) .try_init(); } @@ -107,4 +108,31 @@ mod tests { assert!(output.contains("fallback-info")); assert!(!output.contains("dependency-warning")); } + + #[test] + fn broken_diagnostic_writer_does_not_panic() { + const CHILD: &str = "MOLI_TEST_BROKEN_DIAGNOSTIC_SINK"; + if std::env::var_os(CHILD).is_some() { + super::init("error"); + tracing::error!("the diagnostic receiver has already exited"); + return; + } + let (reader, writer) = std::io::pipe().unwrap(); + drop(reader); + let status = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "telemetry::tests::broken_diagnostic_writer_does_not_panic", + "--nocapture", + ]) + .env(CHILD, "1") + .stdout(std::process::Stdio::null()) + .stderr(writer) + .status() + .unwrap(); + assert!( + status.success(), + "logging to a closed diagnostic pipe terminated the process: {status}" + ); + } } diff --git a/moli/tests/fetch_cli.rs b/moli/tests/fetch_cli.rs index 663aca3f5..9dae576c9 100644 --- a/moli/tests/fetch_cli.rs +++ b/moli/tests/fetch_cli.rs @@ -1315,6 +1315,73 @@ fn cli_dump_html_keeps_uncaught_script_errors_out_of_stdout() -> Result<()> { Ok(()) } +#[test] +fn cli_uncaught_script_error_is_logged_to_writable_stderr() -> Result<()> { + let output = Command::new(env!("CARGO_BIN_EXE_moli")) + .args([ + "fetch", + "--log-level", + "error", + "--wait-until", + "load", + "--timeout", + "5000", + "--dump", + "html", + "data:text/html,after", + ]) + .output()?; + + let stdout = clean_output(&output.stdout); + let stderr = clean_output(&output.stderr); + assert!(output.status.success(), "stdout={stdout}\nstderr={stderr}"); + assert!(stdout.contains("
after
")); + assert!(stderr.contains("Uncaught Error: stderr-control")); + Ok(()) +} + +#[cfg(unix)] +#[test] +fn cli_uncaught_script_error_survives_broken_stderr_pipe() -> Result<()> { + use std::{fs::File, os::fd::FromRawFd, process::Stdio}; + + let mut pipe_fds = [0; 2]; + // SAFETY: `pipe_fds` points to two writable integers. Each returned file + // descriptor is transferred to exactly one owner or closed below. + if unsafe { libc::pipe(pipe_fds.as_mut_ptr()) } != 0 { + return Err(std::io::Error::last_os_error().into()); + } + // Closing the read endpoint makes the child's first diagnostic write + // return BrokenPipe, matching a launcher whose stderr reader disappeared. + unsafe { libc::close(pipe_fds[0]) }; + // SAFETY: the write descriptor is valid and ownership moves into `File`. + let broken_stderr = unsafe { File::from_raw_fd(pipe_fds[1]) }; + let output = Command::new(env!("CARGO_BIN_EXE_moli")) + .args([ + "fetch", + "--log-level", + "error", + "--wait-until", + "load", + "--timeout", + "5000", + "--dump", + "html", + "data:text/html,after", + ]) + .stderr(Stdio::from(broken_stderr)) + .output()?; + + let stdout = clean_output(&output.stdout); + assert_eq!( + output.status.code(), + Some(0), + "broken diagnostic pipe must not abort the browser: stdout={stdout}" + ); + assert!(stdout.contains("
after
")); + Ok(()) +} + #[test] fn cli_http_cache_dir_reuses_cached_response_across_processes() -> Result<()> { let runtime = tokio::runtime::Runtime::new()?;