diff --git a/scripts/checkedReadMigration.test.ts b/scripts/checkedReadMigration.test.ts index 183edb9..bfaaad4 100644 --- a/scripts/checkedReadMigration.test.ts +++ b/scripts/checkedReadMigration.test.ts @@ -2,6 +2,8 @@ import assert from 'node:assert/strict'; import { readFileSync } from 'node:fs'; import test from 'node:test'; +import { readSourceFiles } from './sourceTree.js'; + // Runes and the Tauri bridge, shimmed the way truncatedBufferGuard.test.ts // shims them: the stores are runes modules, and Node's test runner gives every // file its own process, so this cannot leak into another suite. @@ -152,14 +154,23 @@ test('the completed buffer is refused or accepted according to that verdict', as const viewer = readFileSync('src/lib/MarkdownViewer.svelte', 'utf8'); -test('no writable buffer in the app is filled by the unchecked command', () => { - for (const [name, source] of [ - ['MarkdownViewer.svelte', viewer], - ['documentSession.svelte.ts', readFileSync('src/lib/sessions/documentSession.svelte.ts', 'utf8')], - ['windowSession.svelte.ts', readFileSync('src/lib/sessions/windowSession.svelte.ts', 'utf8')], - ] as const) { - assert.doesNotMatch(source, /invoke\('read_file_content'/, `${name} still uses the unchecked command`); - } +test('the unchecked read command is gone, and nothing calls it', () => { + // This was a three-file allowlist — the files #379 migrated. That is the + // wrong shape for a rule that means "nobody, anywhere": a fourth file + // added the call and the test still passed. Two changes fix it. The scan + // below covers the whole tree, so a new file cannot slip under it. And the + // Rust command itself is deleted, which is what turns the rule from a + // convention into a fact: `read_file_content` is not registered any more, + // so invoking it fails loudly instead of quietly filling a buffer with + // mojibake nothing flagged. + const offenders = readSourceFiles('src') + .filter(({ text }) => /invoke\('read_file_content'/.test(text)) + .map(({ path }) => path); + assert.deepEqual(offenders, [], 'read_file_content no longer exists; read_file_content_checked is the read'); + + const rust = readFileSync('src-tauri/src/lib.rs', 'utf8'); + assert.doesNotMatch(rust, /\basync fn read_file_content\(/, 'the unchecked command must stay deleted'); + assert.doesNotMatch(rust, /^\s*read_file_content,\s*$/m, 'and must not be registered again'); }); test('entering the editor reads the fidelity and stores it', () => { diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 3b2ba9c..ab848ea 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1072,6 +1072,82 @@ mod tests { assert!(out.contains("[jump](#real)"), "got: {out}"); } + /// A document that has BOTH kinds of code region, with the inline span at + /// a lower offset than the fence. + /// + /// `in_code_region` is a binary search, so `code_region_ranges` has to + /// emit its regions in document order. The two kinds are found by + /// different parts of the scan — fences by the line walk, inline spans by + /// `push_inline_code_spans` over the text between fences — and a build + /// order that appends all of one kind after all of the other leaves the + /// vector unsorted for exactly this shape of document. The binary search + /// then walks straight past the fence and every marker inside it is + /// reported as ordinary prose (#375 / #389 all over again). + /// + /// Every marker below is checked, not just one. `process_wikilinks` runs + /// a separate pass per marker kind and each probes `in_code_region` at its + /// own offset, so a probe that happens to land inside the region does not + /// say anything about the probes beside it. + const FENCE_AFTER_INLINE_CODE: &str = concat!( + "Prose with `a code span` in it.\n", + "\n", + "```text\n", + "![[embed.md]]\n", + "[[wikilink]]\n", + "==highlight==\n", + "^[footnote]\n", + "```\n", + ); + + #[test] + fn an_inline_span_before_a_fence_does_not_expose_the_fence_to_embeds() { + let out = process_internal_embeds(FENCE_AFTER_INLINE_CODE); + assert!( + out.contains("![[embed.md]]") && !out.contains(" String { + let source = include_str!("lib.rs").replace("\r\n", "\n"); + let needle = format!("\nfn {}(content: &str) -> String {{", "convert_markdown"); + let start = source + .find(&needle) + .expect("convert_markdown must keep its `&str -> String` signature"); + let rest = &source[start + needle.len()..]; + rest[..rest + .find("\n}\n") + .expect("convert_markdown must be terminated")] + .to_string() + } + + #[test] + fn convert_markdown_hands_the_fail_safe_the_raw_buffer() { + // `annotate_task_checkboxes` is a fail-safe only while what reaches it + // is the buffer the command was called with. The hazard is not the + // call — it is the *name*: adding a step the obvious way, + // + // let content = process_new_thing(content); + // + // near the top rebinds the parameter, and the unchanged call at the + // bottom starts handing over preprocessed text. Nothing about that + // edit looks wrong and no behavioural test can see it, because the two + // sides it is supposed to cross-check now agree by definition. + // + // "This string is the one the caller passed in" is provenance, not a + // type, so the compiler cannot be made to check it. What can be made + // structural is the shadowing: `convert_markdown` copies its input to + // `raw_buffer` before anything else runs, which turns the shadowing + // edit above into a harmless one. This test pins the three properties + // that copy depends on. + let body = convert_markdown_body(); + + let capture = "let raw_buffer = content;"; + let first_let = body + .find("\n let ") + .map(|i| i + "\n ".len()) + .expect("convert_markdown must bind something"); + assert!( + body[first_let..].starts_with(capture), + "the raw buffer must be captured before the first preprocessing \ + step, or the step can shadow `content` above it:\n{body}", + ); + assert_eq!( + body.matches(capture).count(), + 1, + "`raw_buffer` is bound more than once — a second binding is the \ + same hole under a new name:\n{body}", + ); + assert!( + Regex::new(r"annotate_task_checkboxes\([^;]*,\s*raw_buffer\s*\)") + .unwrap() + .is_match(&body), + "the fail-safe is no longer handed `raw_buffer`; whatever it now \ + receives can agree with the HTML by construction:\n{body}", + ); + } + #[test] fn every_convert_markdown_preprocessing_step_is_registered() { // Re-reads this file so that a fifth preprocessing step cannot be @@ -2295,10 +2438,19 @@ fn remove_pinned_tag(app: AppHandle, name: String) -> Result<(), String> { /// N. One mismatched pairing (e.g. a 4-backtick inline sample, or a ~~~ /// fence, which the old pattern did not know at all) desynchronized the /// protection for the entire rest of the document. +/// +/// The result is in ascending document order, which is not cosmetic: +/// `in_code_region` binary-searches it. That order is produced by +/// construction rather than by a sort at the end — the scan alternates +/// between the two kinds of region (the text before a fence, then the fence, +/// then the text after it), so each push is at a higher offset than the last. +/// The previous shape collected fences here and appended every inline span in +/// a second pass afterwards, which left the vector unsorted for any document +/// containing both, and made a single `sort_unstable()` call the only thing +/// standing between the search and a wrong answer. fn code_region_ranges(content: &str) -> Vec<(usize, usize)> { let len = content.len(); let mut regions: Vec<(usize, usize)> = Vec::new(); - let mut plain_segments: Vec<(usize, usize)> = Vec::new(); // (fence char, opener run length, region start) let mut fence: Option<(u8, usize, usize)> = None; let mut seg_start = 0usize; @@ -2338,8 +2490,10 @@ fn code_region_ranges(content: &str) -> Vec<(usize, usize)> { // The info string of a backtick fence may not contain backticks. let info_ok = marker != Some(b'`') || !trimmed[run_len..].contains('`'); if is_fence_line && info_ok { + // Before the fence region itself, so the two kinds of + // region stay interleaved in document order. if seg_start < line_start { - plain_segments.push((seg_start, line_start)); + push_inline_code_spans(content, seg_start, line_start, &mut regions); } fence = Some((marker.unwrap(), run_len, line_start)); } @@ -2352,36 +2506,47 @@ fn code_region_ranges(content: &str) -> Vec<(usize, usize)> { Some((_, _, start)) => regions.push((start, len)), None => { if seg_start < len { - plain_segments.push((seg_start, len)); + push_inline_code_spans(content, seg_start, len, &mut regions); } } } - // Inline code spans in the text between fences. CommonMark parses inline - // elements one block at a time and a blank line ends a block, so pairing - // is confined to each blank-line-delimited chunk: a stray backtick in - // prose must not open a span that runs on until the opening backtick of a - // real code span paragraphs later, suppressing every embed, wikilink and - // highlight in between. - for (seg_s, seg_e) in plain_segments { - let mut chunk_start = seg_s; - let mut line_start = seg_s; - while line_start < seg_e { - let line_end = content[line_start..seg_e] - .find('\n') - .map(|i| line_start + i + 1) - .unwrap_or(seg_e); - if content[line_start..line_end].trim().is_empty() { - pair_inline_code_runs(content, chunk_start, line_start, &mut regions); - chunk_start = line_end; - } - line_start = line_end; + debug_assert!( + regions.windows(2).all(|pair| pair[0] <= pair[1]), + "code_region_ranges emitted regions out of document order, which \ + makes in_code_region's binary search miss them: {regions:?}", + ); + regions +} + +/// Splits `content[start..end]` — a stretch of text between fences — into +/// blocks and records the inline code spans of each. +/// +/// CommonMark parses inline elements one block at a time and a blank line +/// ends a block, so pairing is confined to each blank-line-delimited chunk: a +/// stray backtick in prose must not open a span that runs on until the +/// opening backtick of a real code span paragraphs later, suppressing every +/// embed, wikilink and highlight in between. +fn push_inline_code_spans( + content: &str, + start: usize, + end: usize, + regions: &mut Vec<(usize, usize)>, +) { + let mut chunk_start = start; + let mut line_start = start; + while line_start < end { + let line_end = content[line_start..end] + .find('\n') + .map(|i| line_start + i + 1) + .unwrap_or(end); + if content[line_start..line_end].trim().is_empty() { + pair_inline_code_runs(content, chunk_start, line_start, regions); + chunk_start = line_end; } - pair_inline_code_runs(content, chunk_start, seg_e, &mut regions); + line_start = line_end; } - - regions.sort_unstable(); - regions + pair_inline_code_runs(content, chunk_start, end, regions); } /// Records the inline code spans inside `content[start..end]`, which must be @@ -3287,6 +3452,14 @@ fn escape_html_text(text: &str) -> String { /// a new step here must also be registered in `line_preserving_transforms()`. #[tauri::command] fn convert_markdown(content: &str) -> String { + // The buffer this command was called with, captured before anything runs + // and never rebound. What `annotate_task_checkboxes` is handed at the end + // has to be this rather than the parameter name: a new step written as a + // `let content = ...` shadow would rebind that name and silently retarget + // the call without touching it. See that function's doc comment and + // `convert_markdown_hands_the_fail_safe_the_raw_buffer`. + let raw_buffer = content; + let processed_autolinks = process_parenthesized_autolinks(content); let processed_embeds = process_internal_embeds(&processed_autolinks); let processed_links = process_wikilinks(&processed_embeds); @@ -3309,7 +3482,7 @@ fn convert_markdown(content: &str) -> String { options.render.sourcepos = true; let html = markdown_to_html(&masked_math.text, &options); - annotate_task_checkboxes(restore_math_spans(&html, &masked_math), content) + annotate_task_checkboxes(restore_math_spans(&html, &masked_math), raw_buffer) } /// Marks the rendered task checkboxes the frontend is allowed to toggle. @@ -3322,18 +3495,35 @@ fn convert_markdown(content: &str) -> String { /// number of the *raw* buffer (`documentSession.toggleTaskCheckbox`). The two /// only agree while every preprocessing step preserves line numbers. Checking /// the raw buffer here is what turns a broken step into "the checkbox stays -/// disabled" instead of "the checkbox writes a `- [x]` marker into whatever -/// happens to sit on that line" — the P0 that issue #352 fixed, where the -/// marker could land inside a fenced code block. +/// disabled" instead of a write aimed at the wrong line of the user's +/// document — the P0 that issue #352 fixed. +/// +/// What a wrong line costs is narrower than it was, and worth stating +/// precisely, because an overstated reason invites the next reader to check +/// it, find it false, and delete the guard as theatre. Since #352 the +/// frontend rewrites only lines that already match +/// `/^(\s*(?:>\s*)*(?:[-+*]|\d+[.)])\s+)\[( |x|X)\]/` +/// (`documentSession.toggleTaskCheckbox`), so a wrong line that is ordinary +/// prose is a no-op and the toggle reports failure — it does NOT write a +/// `- [x]` marker into whatever happens to sit there, as this comment used to +/// claim of the pre-#352 frontend. What still corrupts is a wrong line that +/// is itself task-shaped, and neither spelling of that is exotic: a task list +/// quoted inside a fenced code block is ordinary content in a notes app, and +/// a real task elsewhere in the same document means the user clicks one +/// checkbox and a different one silently flips. /// /// So do NOT "unify" this with the preprocessed text that produced the HTML. /// Passing `&processed_links` here would make the two sides agree by /// definition, delete the guard, and turn every future line-count regression -/// straight into document corruption. Keep the contract honest in the +/// straight into a mis-aimed write. Nor is passing the *parameter name* +/// enough at the call site: `convert_markdown` captures its input as +/// `raw_buffer` first precisely so that a later `let content = …` cannot +/// retarget the call without touching it. Keep the contract honest in the /// transforms instead; `every_preprocessing_step_preserves_source_line_numbers` -/// is what enforces it, and +/// is what enforces it, /// `task_checkboxes_stay_inert_when_the_html_and_the_buffer_disagree` pins -/// this guard. +/// this guard, and `convert_markdown_hands_the_fail_safe_the_raw_buffer` pins +/// the argument it is given. fn annotate_task_checkboxes(html: String, markdown: &str) -> String { let markdown_lines = markdown.lines().collect::>(); @@ -3455,6 +3645,19 @@ async fn render_markdown(content: String) -> Result { .unwrap_or_else(|e| Err(e.to_string())) } +/// Reads a file, with the fidelity of the decode: returns `(content, lossy)`. +/// Since every read path decodes leniently, a caller that puts the text into +/// an EDITABLE buffer must carry `lossy` onto the tab — otherwise the first +/// auto-save writes U+FFFD over a file that was merely in another encoding. +/// +/// This is now the only read-to-string command. Its sibling +/// `read_file_content` returned the text and dropped the verdict; it survived +/// #379 for callers that re-read a file whose tab was already flagged, then +/// lost its last call site and stayed registered — a command whose defining +/// property is that it hides the flag, one `invoke` away from any new caller. +/// Deleting it makes "which command should this use" a question with one +/// answer rather than a convention. +/// /// Deliberately async, like every other file-touching command here. A /// synchronous `#[tauri::command]` runs on the main thread, so a read from a /// slow volume (SMB, iCloud, a failing USB stick) freezes the whole @@ -3462,23 +3665,6 @@ async fn render_markdown(content: String) -> Result { /// returns. `spawn_blocking` moves the wait onto the blocking pool, which is /// what `tauri::async_runtime` provides it for. #[tauri::command] -async fn read_file_content(path: String) -> Result { - tauri::async_runtime::spawn_blocking(move || { - read_to_string_lossy(&path) - .map(|decoded| decoded.content) - .map_err(|e| e.to_string()) - }) - .await - .unwrap_or_else(|e| Err(e.to_string())) -} - -/// `read_file_content` plus the fidelity of the decode: returns -/// `(content, lossy)`. Since every read path decodes leniently, a caller that -/// puts the text into an EDITABLE buffer must use this one and carry `lossy` -/// onto the tab — otherwise the first auto-save writes U+FFFD over a file -/// that was merely in another encoding. The bare `read_file_content` remains -/// for callers that re-read a file whose tab is already flagged. -#[tauri::command] async fn read_file_content_checked(path: String) -> Result<(String, bool), String> { tauri::async_runtime::spawn_blocking(move || { read_to_string_lossy(&path) @@ -4459,7 +4645,6 @@ pub fn run() { open_markdown_preview, render_markdown, send_markdown_path, - read_file_content, read_file_content_checked, canonicalize_path, read_file_as_data_url, diff --git a/src-tauri/src/window_runtime.rs b/src-tauri/src/window_runtime.rs index 4bdcdd3..fe38e2c 100644 --- a/src-tauri/src/window_runtime.rs +++ b/src-tauri/src/window_runtime.rs @@ -140,10 +140,41 @@ fn read_pinned_tags(app: &AppHandle) -> Vec { /// its pinned tag from its own close handler. /// /// This is the same defect the frontend fixed in #405 for the recent-files -/// list. That fix was a re-read alone, which is sufficient there because -/// `localStorage` is per-document and single-threaded, making an RMW cycle -/// atomic by construction. Rust commands have no such property, so here the -/// cycle needs an explicit lock. +/// list, where a re-read alone was enough. The previous revision of this +/// comment — written here, by #424, the change that added this lock — +/// explained why by saying that `localStorage` is per-document and +/// single-threaded, +/// "making an RMW cycle atomic by construction". That is false, and it is +/// corrected here rather than softened, because someone reasoning from it +/// about some other shared `localStorage` key would conclude they need no +/// synchronisation at all. Each *document* is single-threaded; two Markpad +/// windows are two documents sharing one origin's storage area. The storage +/// mutex the HTML standard describes for exactly this case is not implemented +/// by any shipping engine, WebKit and WebView2 included, so a `getItem` … +/// `setItem` pair in one window can interleave with the other window's and +/// lose precisely the update drawn above. +/// +/// What the re-read buys is a narrower window, not atomicity: +/// +/// - Before it, the exposure was the whole lifetime of a window's in-memory +/// copy — from the last time that window looked at the list until it next +/// wrote, which is minutes. After it, the cycle is one synchronous turn of +/// the event loop containing no `await`: `getItem`, a `JSON.parse` of at +/// most nine short strings, `setItem`. +/// - The writes happen on discrete user actions (open a file, remove an +/// entry, rename), so colliding means two windows landing inside those +/// microseconds. +/// - What a collision costs is one entry of a recent-file list, which the +/// next open puts back. +/// +/// It is a residual race, accepted on those three grounds — not a guarantee. +/// None of the three holds here. This cycle is a file read, a parse, a +/// serialize and an `atomic_write`: milliseconds of I/O on a preemptively +/// scheduled thread pool, not microseconds of straight-line JS. The collision +/// is not a coincidence but the ordinary shape of quitting, since ⌘Q makes +/// every window write from its own close handler at once. And a dropped pin is +/// a thing the user made, with nothing to recreate it from. Unlocked, this +/// cycle was measured losing updates; hence the lock. /// /// Serialising also keeps two `atomic_write` calls off the same target at /// once, which is not something `atomic_write` handles either: its temp file diff --git a/src/lib/utils/recentFiles.ts b/src/lib/utils/recentFiles.ts index dc40a1d..d7b5bfa 100644 --- a/src/lib/utils/recentFiles.ts +++ b/src/lib/utils/recentFiles.ts @@ -58,6 +58,15 @@ export function readStoredRecentFiles(): string[] { * makes each change a change to the *stored* list rather than to a stale copy * of it. * + * It narrows the race rather than closing it. Two windows are two documents + * over one storage area and the spec's storage mutex is not implemented + * anywhere, so the `getItem`/`setItem` pair below can still interleave with + * another window's. It is accepted here because the cycle is one synchronous + * turn of the event loop, it runs only on discrete user actions, and the worst + * loss is one recent-file entry the next open restores — three things that are + * NOT true of every shared key. `update_pinned_tags` in `window_runtime.rs` + * documents the contrast and takes a real lock. + * * The write goes through {@link writeStoredSetting} (#370) so that a write * which changes nothing does not fire a `storage` event in the other windows. * That is what keeps the listener below from bouncing an update back and