From 82aea2ef5cd9cf78599267cd09eccab06fbc9935 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:12:13 +0500 Subject: [PATCH 01/33] feat(rules): match patterns as anchored globs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classify` tested patterns with `p.contains(pat)`, so nothing tied a pattern to a path component and rules fired well outside the directories they name: `/tmp/` matched `/home/user/tmp/tax-return.pdf`, `/var/log/` matched `~/var/log`, and `.dmg` matched `holiday.dmgx`. All three come back `review`, which is an actionable verdict — the app offered to move user data on the strength of a substring. Patterns are globs now, compiled with globset and `literal_separator` set so `*` stops at a separator. That makes anchoring expressible: `/tmp/**` is the filesystem root's tmp and nothing else, while `**/node_modules/**` still reaches any depth. The `patterns` TODO named globset for this. Compiled matchers are cached in a `OnceLock` on the db rather than built per call: `report::build` classifies every entry, and a home directory is millions of them. The cache is `#[serde(skip)]` with a hand-written `Clone`, so a db that arrives deserialized behaves like one built here. A pattern that fails to compile is dropped with a warning instead of panicking mid-scan, and a test asserts every shipped pattern compiles — a protected rule that silently matches nothing is the worst thing this module could do quietly. Closes #41 --- Cargo.lock | 24 +++++ Cargo.toml | 3 + crates/diskern-core/Cargo.toml | 1 + crates/diskern-core/rules/base.json | 28 +++--- crates/diskern-core/src/rules.rs | 148 ++++++++++++++++++++++++++-- 5 files changed, 184 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9b202b1..62ce792 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -252,6 +252,16 @@ dependencies = [ "tinyvec", ] +[[package]] +name = "bstr" +version = "1.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6bb31b46c14244e20ee9984b11bf5c992b91fb6939fea616e3512c8baecdbe5f" +dependencies = [ + "memchr", + "serde_core", +] + [[package]] name = "bumpalo" version = "3.20.3" @@ -847,6 +857,7 @@ version = "0.1.0" dependencies = [ "blake3", "dashmap", + "globset", "jwalk", "petgraph", "rayon", @@ -1433,6 +1444,19 @@ version = "0.3.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e4eba85ea1d0a966a983acd07deee566e67395d2d96b6fb39e62b5a833f1eb0b" +[[package]] +name = "globset" +version = "0.4.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07c34a9410465b45bd9787443bc7370f37735bad04b0f0cd57ff1a3186c98988" +dependencies = [ + "aho-corasick", + "bstr", + "log", + "regex-automata", + "regex-syntax", +] + [[package]] name = "gobject-sys" version = "0.18.0" diff --git a/Cargo.toml b/Cargo.toml index 091df29..bc770b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,9 @@ rayon = "1" petgraph = "0.8" dashmap = "6" +# Path matching for the rules database +globset = "0.4" + [profile.release] lto = "thin" strip = true diff --git a/crates/diskern-core/Cargo.toml b/crates/diskern-core/Cargo.toml index 532702c..80d0779 100644 --- a/crates/diskern-core/Cargo.toml +++ b/crates/diskern-core/Cargo.toml @@ -22,6 +22,7 @@ jwalk.workspace = true rayon.workspace = true petgraph.workspace = true dashmap.workspace = true +globset.workspace = true [dev-dependencies] tempfile = "3" diff --git a/crates/diskern-core/rules/base.json b/crates/diskern-core/rules/base.json index d8e6878..a68121d 100644 --- a/crates/diskern-core/rules/base.json +++ b/crates/diskern-core/rules/base.json @@ -1,93 +1,93 @@ { - "version": 2, + "version": 3, "rules": [ { "id": "windows-driverstore", - "patterns": ["/windows/system32/driverstore"], + "patterns": ["**/windows/system32/driverstore/**"], "category": "system_critical", "verdict": "protected", "description": "Windows driver store. Removing files here can make hardware unusable." }, { "id": "windows-winsxs", - "patterns": ["/windows/winsxs"], + "patterns": ["**/windows/winsxs/**"], "category": "system_critical", "verdict": "protected", "description": "Windows component store. Managed by the OS; never modify directly." }, { "id": "windows-installer-cache", - "patterns": ["/windows/installer/", "/package cache/"], + "patterns": ["**/windows/installer/**", "**/package cache/**"], "category": "system_critical", "verdict": "protected", "description": "Windows Installer cache. Windows needs these to uninstall, repair, or patch installed software; removing them strands every product that registered here." }, { "id": "chrome-cache", - "patterns": ["/google/chrome/user data/default/cache", "/.cache/google-chrome"], + "patterns": ["**/google/chrome/user data/*/cache/**", "**/.cache/google-chrome/**"], "category": "browser_cache", "verdict": "safe", "description": "Chrome browser cache. Fully regenerated on next use; no effect on installed applications or saved data." }, { "id": "firefox-cache", - "patterns": ["/.cache/mozilla/firefox", "/mozilla/firefox/profiles"], + "patterns": ["**/.cache/mozilla/firefox/**", "**/mozilla/firefox/profiles/**"], "category": "browser_cache", "verdict": "safe", "description": "Firefox cache. Regenerated automatically." }, { "id": "rust-target", - "patterns": ["/target/debug", "/target/release"], + "patterns": ["**/target/debug/**", "**/target/release/**"], "category": "build_artifact", "verdict": "review", "description": "Rust build artifacts. Regenerable via cargo build, but rebuilding large projects takes time." }, { "id": "node-modules", - "patterns": ["/node_modules/"], + "patterns": ["**/node_modules/**"], "category": "build_artifact", "verdict": "review", "description": "Node.js dependencies. Regenerable via npm/pnpm install if a lockfile exists." }, { "id": "pip-cache", - "patterns": ["/.cache/pip", "/appdata/local/pip/cache"], + "patterns": ["**/.cache/pip/**", "**/appdata/local/pip/cache/**"], "category": "package_manager_cache", "verdict": "safe", "description": "pip download cache. Packages are re-downloaded on demand." }, { "id": "unix-system-logs", - "patterns": ["/var/log/"], + "patterns": ["/var/log/**", "/private/var/log/**"], "category": "log", "verdict": "review", "description": "System logs. Reclaimable once you no longer need them, but they are the first thing anyone asks for when something breaks." }, { "id": "macos-user-logs", - "patterns": ["/library/logs/"], + "patterns": ["**/library/logs/**"], "category": "log", "verdict": "review", "description": "Application logs written by macOS apps. Regenerated as the apps run; only worth keeping while you are chasing a bug." }, { "id": "windows-crash-logs", - "patterns": ["/appdata/local/crashdumps/", "/appdata/local/microsoft/windows/wer/"], + "patterns": ["**/appdata/local/crashdumps/**", "**/appdata/local/microsoft/windows/wer/**"], "category": "log", "verdict": "review", "description": "Windows crash dumps and error reports. Large, and only useful while investigating the crash that produced them." }, { "id": "installer-packages", - "patterns": [".dmg", ".msi", ".pkg"], + "patterns": ["**/*.dmg", "**/*.msi", "**/*.pkg"], "category": "installer", "verdict": "review", "description": "Installer package. Reclaimable, but you will have to download it again to reinstall or roll back." }, { "id": "temp-dirs", - "patterns": ["/tmp/", "/appdata/local/temp"], + "patterns": ["/tmp/**", "**/var/tmp/**", "**/appdata/local/temp/**"], "category": "temp_file", "verdict": "review", "description": "Temporary files. Usually safe once no program is using them." diff --git a/crates/diskern-core/src/rules.rs b/crates/diskern-core/src/rules.rs index 7f2ccce..acff344 100644 --- a/crates/diskern-core/src/rules.rs +++ b/crates/diskern-core/src/rules.rs @@ -5,15 +5,23 @@ //! patterns and yields a Category + base Verdict. The risk module may //! DOWNGRADE a verdict (Safe -> Review) based on evidence; it may never //! upgrade one (Risky -> Safe). Protected is final. +//! +//! Patterns are globs, matched against the normalized path (lowercased, +//! `\` rewritten to `/`) with `*` stopping at a path separator. That +//! anchoring is the point: a plain substring test made `/tmp/` fire on +//! `/home/user/tmp/tax-return.pdf`, which is user data, not scratch space. use crate::{Category, Verdict}; +use globset::{Glob, GlobBuilder, GlobSet, GlobSetBuilder}; use serde::{Deserialize, Serialize}; +use std::sync::OnceLock; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Rule { pub id: String, - /// Substring / suffix patterns matched against the normalized path. - /// TODO: replace with proper glob matching (globset crate). + /// Globs matched against the normalized path. `*` matches within one + /// path component, `**` spans components, so `**/node_modules/**` + /// reaches any depth while `/tmp/**` stays at the filesystem root. pub patterns: Vec, pub category: Category, pub verdict: Verdict, @@ -21,10 +29,32 @@ pub struct Rule { pub description: String, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] pub struct RulesDb { pub version: u32, pub rules: Vec, + /// One compiled matcher per rule, in rule order. Built on the first + /// `classify` and reused after: a home directory is millions of + /// entries, and compiling a glob per entry would cost more than the + /// walk that found them. + /// + /// Skipped by serde and rebuilt on demand, so a db that arrives over + /// the wire behaves exactly like one built here. + #[serde(skip)] + matchers: OnceLock>, +} + +/// Hand-written because `OnceLock` isn't `Clone`. A clone starts +/// with an empty cache rather than sharing one — the rules are the value, +/// the compiled form is an optimization. +impl Clone for RulesDb { + fn clone(&self) -> Self { + Self { + version: self.version, + rules: self.rules.clone(), + matchers: OnceLock::new(), + } + } } impl RulesDb { @@ -35,17 +65,70 @@ impl RulesDb { .expect("embedded rules db must parse") } + /// Build a db from rules held in memory (tests, future remote rule + /// updates). Same matching path as [`RulesDb::embedded`]. + pub fn new(version: u32, rules: Vec) -> Self { + Self { + version, + rules, + matchers: OnceLock::new(), + } + } + /// First matching rule wins; order in the db is priority order. /// Protected rules are listed first for exactly that reason. pub fn classify(&self, path: &std::path::Path) -> (Category, Verdict, Option<&Rule>) { - let p = path.to_string_lossy().replace('\\', "/").to_lowercase(); - for rule in &self.rules { - if rule.patterns.iter().any(|pat| p.contains(pat.as_str())) { + let p = normalize(path); + let candidate = std::path::Path::new(&p); + for (rule, matcher) in self.rules.iter().zip(self.matchers()) { + if matcher.is_match(candidate) { return (rule.category, rule.verdict, Some(rule)); } } (Category::Unknown, Verdict::Review, None) } + + fn matchers(&self) -> &[GlobSet] { + self.matchers + .get_or_init(|| self.rules.iter().map(compile).collect()) + } +} + +/// Lowercase, `/`-separated. Windows hands us `C:\Users\...`; the rules +/// are written once, in one shape, and the path is bent to fit them. +pub(crate) fn normalize(path: &std::path::Path) -> String { + path.to_string_lossy().replace('\\', "/").to_lowercase() +} + +/// `literal_separator` is the whole reason this module compiles globs at +/// all: without it `*` swallows `/`, and `**/target/*/**` would reach back +/// across directories the rule never named. +fn build_glob(pattern: &str) -> std::result::Result { + GlobBuilder::new(pattern).literal_separator(true).build() +} + +/// A pattern that doesn't compile is dropped, not fatal. The alternative — +/// panicking inside `classify` — would take the whole scan down over one +/// bad line in a rules file that may not even be ours. `patterns_compile` +/// below keeps the shipped db honest. +fn compile(rule: &Rule) -> GlobSet { + let mut builder = GlobSetBuilder::new(); + for pattern in &rule.patterns { + match build_glob(pattern) { + Ok(glob) => { + builder.add(glob); + } + Err(e) => tracing::warn!( + rule = %rule.id, + pattern = %pattern, + "ignoring rule pattern that is not a valid glob: {e}" + ), + } + } + builder.build().unwrap_or_else(|e| { + tracing::warn!(rule = %rule.id, "rule matches nothing: {e}"); + GlobSet::empty() + }) } #[cfg(test)] @@ -123,6 +206,59 @@ mod tests { } } + /// Every shipped pattern has to compile, because `compile` drops the + /// ones that don't. A protected rule that silently matches nothing is + /// the worst failure this module has. + #[test] + fn every_shipped_pattern_is_a_valid_glob() { + for rule in &RulesDb::embedded().rules { + for pattern in &rule.patterns { + assert!( + build_glob(pattern).is_ok(), + "{}: {pattern} is not a valid glob", + rule.id + ); + } + } + } + + /// Issue #41. Under substring matching every one of these matched a + /// rule written for somewhere else on the disk, and `review` is an + /// actionable verdict — the app offered to move them. + #[test] + fn rules_do_not_reach_outside_the_paths_they_name() { + let db = RulesDb::embedded(); + for path in [ + "/home/user/tmp/tax-return.pdf", // not /tmp + "/home/user/var/log/notes.txt", // not /var/log + "/home/user/Downloads/holiday.dmgx", // not a .dmg + "/home/user/mytmp/scratch.bin", + ] { + let (cat, _, rule) = db.classify(std::path::Path::new(path)); + assert_eq!(cat, Category::Unknown, "{path} matched {rule:?}"); + } + } + + /// The same rules still have to fire where they were meant to. + #[test] + fn rules_still_reach_the_paths_they_do_name() { + let db = RulesDb::embedded(); + for (path, expected) in [ + ("/tmp/build-9a2f/out.o", Category::TempFile), + ("/var/tmp/systemd-private/x", Category::TempFile), + ( + "C:\\Users\\x\\AppData\\Local\\Temp\\a.tmp", + Category::TempFile, + ), + ("/home/u/proj/node_modules/react/index.js", Category::BuildArtifact), + ("/home/u/proj/target/debug/app", Category::BuildArtifact), + ("/home/u/.cache/pip/wheels/a.whl", Category::PackageManagerCache), + ] { + let (cat, _, _) = db.classify(std::path::Path::new(path)); + assert_eq!(cat, expected, "{path}"); + } + } + /// An installed application's own repair binary is not a reclaimable /// download. `unknown` is the right answer: report::build drops those, /// so it never reaches the user as an actionable row. From f93d791aa467d07b0b2ac588020d37f313e36b48 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:12:34 +0500 Subject: [PATCH 02/33] fix(rules): point the Firefox rule at cache2, not the whole profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `firefox-cache` listed `/mozilla/firefox/profiles` with verdict `safe`. On Windows the profile itself lives at `%APPDATA%\Mozilla\Firefox\Profiles\\`, which normalizes straight onto that pattern — so the rule covered `places.sqlite` (bookmarks and history), `logins.json` and `cookies.sqlite`. They were listed under "Safe to remove" in the app, and `quarantine_finding` accepted them. The cache is `cache2` inside the profile, under LocalAppData on Windows and `~/Library/Caches` on macOS. Naming those directly keeps the rule to what it claims to cover; the Linux pattern was always cache-only and is unchanged. Two tests pin it from both sides: profile data must not come back `safe`, and cache2 on all three platforms still must. Closes #40 --- crates/diskern-core/rules/base.json | 4 ++-- crates/diskern-core/src/rules.rs | 33 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/crates/diskern-core/rules/base.json b/crates/diskern-core/rules/base.json index a68121d..d391a38 100644 --- a/crates/diskern-core/rules/base.json +++ b/crates/diskern-core/rules/base.json @@ -31,10 +31,10 @@ }, { "id": "firefox-cache", - "patterns": ["**/.cache/mozilla/firefox/**", "**/mozilla/firefox/profiles/**"], + "patterns": ["**/.cache/mozilla/firefox/**", "**/mozilla/firefox/profiles/*/cache2/**", "**/library/caches/firefox/profiles/*/cache2/**"], "category": "browser_cache", "verdict": "safe", - "description": "Firefox cache. Regenerated automatically." + "description": "Firefox network cache (cache2). Regenerated automatically. The rest of the profile — bookmarks, history, logins, cookies — is deliberately not covered." }, { "id": "rust-target", diff --git a/crates/diskern-core/src/rules.rs b/crates/diskern-core/src/rules.rs index acff344..ccac0c8 100644 --- a/crates/diskern-core/src/rules.rs +++ b/crates/diskern-core/src/rules.rs @@ -259,6 +259,39 @@ mod tests { } } + /// Issue #40. `/mozilla/firefox/profiles` covered the whole profile + /// directory, so bookmarks, history, logins and cookies were listed + /// under "Safe to remove" and `quarantine_finding` accepted them. + /// Only cache2 is cache; everything else in the profile is user data. + #[test] + fn firefox_profile_data_is_not_safe_to_remove() { + let db = RulesDb::embedded(); + for path in [ + "C:\\Users\\x\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\ab12.default\\logins.json", + "C:\\Users\\x\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\ab12.default\\places.sqlite", + "C:\\Users\\x\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\ab12.default\\cookies.sqlite", + "/home/u/.mozilla/firefox/ab12.default/key4.db", + ] { + let (_, verdict, rule) = db.classify(std::path::Path::new(path)); + assert_ne!(verdict, Verdict::Safe, "{path} matched {rule:?}"); + } + } + + /// The cache the rule is actually named after still classifies. + #[test] + fn firefox_cache_is_still_safe_to_remove() { + let db = RulesDb::embedded(); + for path in [ + "/home/u/.cache/mozilla/firefox/ab12.default/cache2/entries/A1B2", + "C:\\Users\\x\\AppData\\Local\\Mozilla\\Firefox\\Profiles\\ab12.default\\cache2\\entries\\A1B2", + "/Users/x/Library/Caches/Firefox/Profiles/ab12.default/cache2/entries/A1B2", + ] { + let (cat, verdict, _) = db.classify(std::path::Path::new(path)); + assert_eq!(cat, Category::BrowserCache, "{path}"); + assert_eq!(verdict, Verdict::Safe, "{path}"); + } + } + /// An installed application's own repair binary is not a reclaimable /// download. `unknown` is the right answer: report::build drops those, /// so it never reaches the user as an actionable row. From 92d6f90893766a787888fb527bf47b4d0d1a7650 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:12:58 +0500 Subject: [PATCH 03/33] docs(rules): describe glob matching instead of substrings The matching-semantics section still told rule authors that patterns are substrings matched anywhere in the path, and pointed at #41 as the fix. The fix landed, so the guidance has to move with it: how `*` and `**` differ, why a directory rule needs a trailing `/**`, and why a pattern that doesn't compile makes a `protected` rule fail open. --- docs/RULES.md | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/docs/RULES.md b/docs/RULES.md index eb1731e..b34f926 100644 --- a/docs/RULES.md +++ b/docs/RULES.md @@ -20,7 +20,7 @@ time from [`crates/diskern-core/rules/base.json`](../crates/diskern-core/rules/b | Field | Meaning | | ------------- | -------------------------------------------------------------------- | | `id` | Stable, kebab-case, unique. Shown to users as evidence. | -| `patterns` | Substrings matched against the normalized path (lowercased, `/`-separated). | +| `patterns` | Globs matched against the normalized path (lowercased, `/`-separated). | | `category` | What the file *is* — `browser_cache`, `build_artifact`, `log`, … | | `verdict` | Base safety verdict (see below). | | `description` | Plain-language explanation shown to the user. | @@ -42,19 +42,34 @@ evidence (e.g. recently-accessed files), never less. - **First match wins** — order in the file is priority order, which is why `protected` rules are listed first. - Paths that match no rule get `unknown` / `review` — never `safe`. -- Patterns are plain substrings matched anywhere in the path, so a rule - cannot say "this extension, but only under Downloads". Where that - matters, write the half that is safe on its own (the extension) and - keep the verdict at `review`. Proper glob matching is - [issue #41](https://github.com/Coding-Moves/diskern/issues/41). - -Because matches can land anywhere in a path, **rule order is a safety -property**. `installer-packages` matches `.msi` anywhere — including -`C:\Windows\Installer`, the cache Windows needs to uninstall, repair or -patch installed software, and the `Package Cache` folders that serve the -same purpose for .NET and Visual Studio. `review` is an *actionable* -verdict in the app, so without `windows-installer-cache` listed above -`installer-packages`, the UI would offer to quarantine them. +- Patterns are globs, matched against the whole normalized path: + - `*` matches within one path component and stops at `/` + - `**` spans components + - a pattern that starts with `/` is anchored at the filesystem root + +That anchoring is what keeps a rule inside the directory it names. +`/tmp/**` is the root's scratch directory; it does not reach +`/home/user/tmp/tax-return.pdf`. `**/node_modules/**` still matches at +any depth, because that is what the rule means. + +Write patterns to end in `/**` when the rule is about a directory, and +as `**/*.ext` when it is about an extension. A directory pattern without +the trailing `/**` matches the directory entry itself — and the scanner +only ever classifies files, so it would match nothing. + +A pattern that isn't a valid glob is dropped with a warning rather than +taken down the scan, so **a typo makes a rule match nothing**. For a +`protected` rule that fails open, which is why +`every_shipped_pattern_is_a_valid_glob` in +[`rules.rs`](../crates/diskern-core/src/rules.rs) exists. + +**Rule order is a safety property.** `installer-packages` matches +`**/*.msi` anywhere on the disk — including `C:\Windows\Installer`, the +cache Windows needs to uninstall, repair or patch installed software, and +the `Package Cache` folders that serve the same purpose for .NET and +Visual Studio. `review` is an *actionable* verdict in the app, so without +`windows-installer-cache` listed above `installer-packages`, the UI would +offer to quarantine them. When adding a broad rule: @@ -75,7 +90,7 @@ rules PRs are very welcome. Guidelines: 1. Be conservative: when in doubt, use `review`, not `safe`. 2. Patterns should be specific enough not to match user data - (e.g. `/target/debug`, not `/target`). + (e.g. `**/target/debug/**`, not `**/target/**`). 3. Write the `description` for end users: what it is, why it's safe (or not), what happens after removal. 4. Add a test in [`rules.rs`](../crates/diskern-core/src/rules.rs) if the From fcba04c053efcb8d635eedec643052e658a85e09 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:13:09 +0500 Subject: [PATCH 04/33] docs(rules): show the rule-format example as globs The example block above the field table still carried the pre-glob patterns, which now read as a rule that matches nothing. --- docs/RULES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/RULES.md b/docs/RULES.md index b34f926..153143b 100644 --- a/docs/RULES.md +++ b/docs/RULES.md @@ -10,7 +10,7 @@ time from [`crates/diskern-core/rules/base.json`](../crates/diskern-core/rules/b ```json { "id": "chrome-cache", - "patterns": ["/google/chrome/user data/default/cache", "/.cache/google-chrome"], + "patterns": ["**/google/chrome/user data/*/cache/**", "**/.cache/google-chrome/**"], "category": "browser_cache", "verdict": "safe", "description": "Chrome browser cache. Fully regenerated on next use." From 2457e41bd0b630a5f0b09990bbf891f7a75905e2 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:13:49 +0500 Subject: [PATCH 05/33] fix(scanner): match excludes on normalized path components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_excluded` compared raw strings with `starts_with`, which is a character test, not a path test. Two consequences: - `/run` also excluded `/runtime-data`, and anything else whose name merely begins with those four characters - a root typed `c:\windows\winsxs` never matched the `C:\Windows\WinSxS` exclude, so the component store was walked anyway — on the one platform where walking it is slowest Excludes now go through the same normalization `rules::classify` uses (lowercased, `\` to `/`) and compare component-wise: a path matches when it *is* the excluded directory, or when the character after the prefix is a separator. Normalization happens once per root rather than inside `process_read_dir`, which runs for every directory the walk opens. The `excludes` doc called them glob-style; they never were, and now that `rules` really does take globs the distinction matters. Closes #46 --- crates/diskern-core/src/scanner.rs | 64 ++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 3c18867..2984291 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -13,7 +13,10 @@ use std::sync::Arc; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ScanOptions { pub roots: Vec, - /// Glob-style excludes (e.g. "/proc", "C:\\Windows\\WinSxS"). + /// Directories never walked, given as paths rather than globs + /// (e.g. "/proc", "C:\\Windows\\WinSxS"). Matched on whole path + /// components after the same normalization the rules database uses, + /// so case and separator style don't have to line up with the root. pub excludes: Vec, pub follow_symlinks: bool, // default false — symlink loops are real pub min_file_size: u64, // skip tiny files for dedup purposes @@ -78,7 +81,9 @@ fn walk_root( progress: &ScanProgress, out: &mut Vec, ) -> Result<()> { - let excludes = opts.excludes.clone(); + // Normalized once, not per directory: `process_read_dir` runs on every + // directory the walk opens, and the exclude list never changes. + let excludes: Vec = opts.excludes.iter().map(|e| normalize_exclude(e)).collect(); let walker = jwalk::WalkDir::new(root) .follow_links(opts.follow_symlinks) @@ -121,9 +126,35 @@ fn walk_root( Ok(()) } +/// Same shape the rules database matches in: lowercased, `/`-separated, +/// no trailing separator. An exclude written `C:\\Windows\\WinSxS` has to +/// match a root the user typed as `c:\\windows\\winsxs`, and +/// `rules::classify` already normalizes for exactly that reason. +fn normalize_exclude(exclude: &str) -> String { + let normalized = exclude.replace('\\', "/").to_lowercase(); + let trimmed = normalized.trim_end_matches('/'); + // "/" itself trims to empty; keep it as the root rather than a prefix + // that matches every path. + if trimmed.is_empty() { + normalized + } else { + trimmed.to_string() + } +} + +/// True when `path` *is* an excluded directory or sits inside one. +/// +/// Compared on whole path components. A raw `starts_with` on the string +/// made `/run` exclude `/runtime-data` as well, because "/run" is a prefix +/// of "/runtime-data" in characters but not in directories. fn is_excluded(path: &Path, excludes: &[String]) -> bool { - let p = path.to_string_lossy(); - excludes.iter().any(|ex| p.starts_with(ex.as_str())) + let p = crate::rules::normalize(path); + let p = p.trim_end_matches('/'); + excludes.iter().any(|ex| { + p == ex + || p.strip_prefix(ex.as_str()) + .is_some_and(|rest| rest.starts_with('/')) + }) } fn to_epoch(t: std::time::SystemTime) -> Option { @@ -136,6 +167,31 @@ fn to_epoch(t: std::time::SystemTime) -> Option { mod tests { use super::*; + #[test] + fn excludes_match_whole_components_not_characters() { + let excludes = ["/run".to_string()]; + assert!(is_excluded(Path::new("/run"), &excludes)); + assert!(is_excluded(Path::new("/run/user/1000/x"), &excludes)); + // The bug: a character-prefix compare skipped this too. + assert!(!is_excluded(Path::new("/runtime-data/x"), &excludes)); + assert!(!is_excluded(Path::new("/runner"), &excludes)); + } + + #[test] + fn excludes_survive_a_differently_cased_or_separated_root() { + let excludes = [normalize_exclude("C:\\Windows\\WinSxS")]; + assert!(is_excluded(Path::new("c:/windows/winsxs/component/x.dll"), &excludes)); + assert!(is_excluded(Path::new("C:\\Windows\\WinSxS\\x.dll"), &excludes)); + assert!(!is_excluded(Path::new("c:/windows/winsxs-backup/x.dll"), &excludes)); + } + + #[test] + fn a_trailing_separator_on_an_exclude_changes_nothing() { + let excludes = [normalize_exclude("/proc/")]; + assert!(is_excluded(Path::new("/proc/1/maps"), &excludes)); + assert!(!is_excluded(Path::new("/process-data/x"), &excludes)); + } + #[test] fn scans_a_temp_tree() { let dir = tempfile::tempdir().unwrap(); From eaa837bff05ac98a6d363ee7e9e5d0e015f0dd81 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:14:49 +0500 Subject: [PATCH 06/33] feat(dedup): let callers restrict which entries are eligible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `find_duplicates_filtered` takes a predicate and only stages, hashes and groups the entries it accepts. The existing two entry points delegate to it with "everything", so nothing changes for current callers. This is the seam `report` needs: a duplicate set is an offer to keep one copy and drop the rest, and entries the user is not allowed to act on have no business in that offer. Filtering before stage 1 also means they are never hashed — hashing is where a real scan spends its time. --- crates/diskern-core/src/dedup.rs | 69 +++++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/crates/diskern-core/src/dedup.rs b/crates/diskern-core/src/dedup.rs index 38124a8..d6d3f67 100644 --- a/crates/diskern-core/src/dedup.rs +++ b/crates/diskern-core/src/dedup.rs @@ -37,10 +37,33 @@ pub fn find_duplicates_cancellable( entries: &mut [FileEntry], cancelled: &AtomicBool, ) -> Option> { + find_duplicates_filtered(entries, |_| true, cancelled) +} + +/// [`find_duplicates_cancellable`], restricted to the entries `eligible` +/// accepts. +/// +/// Duplicate sets are an *offer*: "you are storing this three times, keep +/// one". An entry the user is never allowed to act on doesn't belong in +/// that offer, and hashing it is work spent to produce a number nobody can +/// use. `eligible` is where the caller says which entries those are; it is +/// called exactly once per entry, before any hashing. +pub fn find_duplicates_filtered( + entries: &mut [FileEntry], + eligible: F, + cancelled: &AtomicBool, +) -> Option> +where + F: Fn(&FileEntry) -> bool, +{ + let keep: Vec = entries.iter().map(eligible).collect(); + // Stage 1: bucket by size. let mut by_size: HashMap> = HashMap::new(); for (i, e) in entries.iter().enumerate() { - by_size.entry(e.size).or_default().push(i); + if keep[i] { + by_size.entry(e.size).or_default().push(i); + } } let candidates: Vec = by_size .into_values() @@ -68,11 +91,14 @@ pub fn find_duplicates_cancellable( entries[i].hash = h; } - // Stage 3: bucket by hash. + // Stage 3: bucket by hash. Only stage-1 survivors carry one, but the + // eligibility check is repeated here so a caller reusing entries that + // already hold hashes from an earlier run can't smuggle them back in. let mut by_hash: HashMap<&str, Vec<&FileEntry>> = HashMap::new(); - for e in entries.iter() { - if let Some(h) = &e.hash { - by_hash.entry(h).or_default().push(e); + for (i, e) in entries.iter().enumerate() { + match &e.hash { + Some(h) if keep[i] => by_hash.entry(h).or_default().push(e), + _ => continue, } } @@ -126,6 +152,39 @@ mod tests { assert_eq!(sets[0].wasted, 10); } + #[test] + fn ineligible_entries_form_no_sets() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a"), b"same-bytes").unwrap(); + std::fs::write(dir.path().join("b"), b"same-bytes").unwrap(); + std::fs::write(dir.path().join("keep-a"), b"other-byte").unwrap(); + std::fs::write(dir.path().join("keep-b"), b"other-byte").unwrap(); + + let opts = ScanOptions { + roots: vec![dir.path().to_path_buf()], + ..Default::default() + }; + let mut entries = scan(&opts, Arc::new(ScanProgress::default())).unwrap(); + let never = AtomicBool::new(false); + let sets = find_duplicates_filtered( + &mut entries, + |e| e.path.file_name().is_some_and(|n| n.to_string_lossy().starts_with("keep")), + &never, + ) + .unwrap(); + + assert_eq!(sets.len(), 1); + assert!(sets[0].paths.iter().all(|p| p + .file_name() + .is_some_and(|n| n.to_string_lossy().starts_with("keep")))); + // Excluded entries are never hashed, so they cost nothing either. + for entry in &entries { + if entry.path.file_name().is_some_and(|n| n == "a") { + assert!(entry.hash.is_none()); + } + } + } + #[test] fn stops_when_cancelled() { let dir = tempfile::tempdir().unwrap(); From 63ec0e09481942f123dc9f30813a946b4d5e4d1b Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:15:52 +0500 Subject: [PATCH 07/33] fix(scanner): stop applying the dedup size floor to the whole walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ScanOptions::min_file_size` was documented as "skip tiny files for dedup purposes" and then applied inside the walk, before anything else ran. A file under it never reached the rules engine, the risk model, the report or `files_scanned` — so the default of 1 quietly dropped every zero-byte file, and anyone raising it to 1 MB to speed dedup up would also have hidden every small cache file from classification. The knob moves to where the doc comment always said it lived: dedup. It is now `ReportOptions::dedup_min_size`, passed to `find_duplicates_filtered`, and `ScanOptions` no longer has a size filter at all. `build` and `build_cancellable` keep their signatures and use the default; `build_with` is the version that takes the options. Closes #47 --- crates/diskern-core/src/report.rs | 94 +++++++++++++++++++++++++++++- crates/diskern-core/src/scanner.rs | 5 -- 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 6ca3099..7ac3a0d 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -5,6 +5,26 @@ use crate::{dedup, risk, rules::RulesDb, Category, FileEntry, Finding, Verdict}; use serde::{Deserialize, Serialize}; use std::sync::atomic::{AtomicBool, Ordering}; +/// Knobs for [`build_with`] that are about the report, not the walk. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReportOptions { + /// Files strictly smaller than this take no part in duplicate + /// detection. This lived on `ScanOptions` as `min_file_size` and was + /// applied inside the walk, which meant raising it to speed dedup up + /// also hid every small file from the rules engine, the risk model, + /// the report and `files_scanned` — none of which the name promised. + pub dedup_min_size: u64, +} + +impl Default for ReportOptions { + fn default() -> Self { + // 1, not 0: a zero-byte file is identical to every other zero-byte + // file, so a default of 0 makes one enormous duplicate set worth + // nothing. They still reach the report as findings. + Self { dedup_min_size: 1 } + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Report { pub findings: Vec, @@ -23,8 +43,18 @@ pub fn build(entries: Vec, rules: &RulesDb) -> Report { /// stopped early, so there is no report to show — not an error, just the /// user's answer arriving before ours. pub fn build_cancellable( + entries: Vec, + rules: &RulesDb, + cancelled: &AtomicBool, +) -> Option { + build_with(entries, rules, &ReportOptions::default(), cancelled) +} + +/// [`build_cancellable`] with the report knobs spelled out. +pub fn build_with( mut entries: Vec, rules: &RulesDb, + opts: &ReportOptions, cancelled: &AtomicBool, ) -> Option { let now = std::time::SystemTime::now() @@ -32,7 +62,11 @@ pub fn build_cancellable( .map(|d| d.as_secs() as i64) .unwrap_or(0); - let duplicate_sets = dedup::find_duplicates_cancellable(&mut entries, cancelled)?; + let duplicate_sets = dedup::find_duplicates_filtered( + &mut entries, + |e| e.size >= opts.dedup_min_size, + cancelled, + )?; let files_scanned = entries.len() as u64; let mut findings = Vec::new(); @@ -81,3 +115,61 @@ pub fn build_cancellable( files_scanned, }) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::scanner::{scan, ScanOptions, ScanProgress}; + use std::sync::Arc; + + fn scan_dir(dir: &std::path::Path) -> Vec { + let opts = ScanOptions { + roots: vec![dir.to_path_buf()], + ..Default::default() + }; + scan(&opts, Arc::new(ScanProgress::default())).unwrap() + } + + /// Issue #47. `min_file_size` was applied inside the walk, so a file + /// under it never reached the rules engine, the risk model, the report + /// or `files_scanned` — the default of 1 quietly dropped every + /// zero-byte file. The knob is about dedup and now only affects dedup. + #[test] + fn the_dedup_minimum_does_not_hide_files_from_the_report() { + let dir = tempfile::tempdir().unwrap(); + let tmp = dir.path().join("tmp"); + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("empty.log"), b"").unwrap(); + std::fs::write(tmp.join("small-a"), b"tiny").unwrap(); + std::fs::write(tmp.join("small-b"), b"tiny").unwrap(); + + let rules = RulesDb::new( + 1, + vec![crate::rules::Rule { + id: "test-temp".into(), + patterns: vec!["**/tmp/**".into()], + category: Category::TempFile, + verdict: Verdict::Review, + description: "test".into(), + }], + ); + + let entries = scan_dir(dir.path()); + let never = AtomicBool::new(false); + + // Every file is scanned and classified, whatever the dedup floor. + let big_floor = ReportOptions { + dedup_min_size: 1_000_000, + }; + let report = build_with(entries.clone(), &rules, &big_floor, &never).unwrap(); + assert_eq!(report.files_scanned, 3); + assert_eq!(report.findings.len(), 3); + // ...but nothing is small enough to be staged for dedup. + assert!(report.duplicate_sets.is_empty()); + + // With the default floor the two identical files do pair up. + let report = build_with(entries, &rules, &ReportOptions::default(), &never).unwrap(); + assert_eq!(report.files_scanned, 3); + assert_eq!(report.duplicate_sets.len(), 1); + } +} diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 2984291..7271b06 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -19,7 +19,6 @@ pub struct ScanOptions { /// so case and separator style don't have to line up with the root. pub excludes: Vec, pub follow_symlinks: bool, // default false — symlink loops are real - pub min_file_size: u64, // skip tiny files for dedup purposes } impl Default for ScanOptions { @@ -28,7 +27,6 @@ impl Default for ScanOptions { roots: vec![], excludes: default_excludes(), follow_symlinks: false, - min_file_size: 1, } } } @@ -107,9 +105,6 @@ fn walk_root( } let Ok(meta) = entry.metadata() else { continue }; let size = meta.len(); - if size < opts.min_file_size { - continue; - } progress.files_seen.fetch_add(1, Ordering::Relaxed); progress.bytes_seen.fetch_add(size, Ordering::Relaxed); From 3885aba9e179db2ec094c847007d162c58cb10f2 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:17:08 +0500 Subject: [PATCH 08/33] refactor(dedup): give the eligibility predicate the entry index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers that decide eligibility from something they already computed — `report` classifies every entry before dedup runs — can look the answer up by position instead of re-deriving it or carrying a set of paths as large as the disk. No behaviour change: both existing entry points still accept everything. --- crates/diskern-core/src/dedup.rs | 17 +++++++++++++---- crates/diskern-core/src/report.rs | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/crates/diskern-core/src/dedup.rs b/crates/diskern-core/src/dedup.rs index d6d3f67..98c8426 100644 --- a/crates/diskern-core/src/dedup.rs +++ b/crates/diskern-core/src/dedup.rs @@ -37,7 +37,7 @@ pub fn find_duplicates_cancellable( entries: &mut [FileEntry], cancelled: &AtomicBool, ) -> Option> { - find_duplicates_filtered(entries, |_| true, cancelled) + find_duplicates_filtered(entries, |_, _| true, cancelled) } /// [`find_duplicates_cancellable`], restricted to the entries `eligible` @@ -48,15 +48,24 @@ pub fn find_duplicates_cancellable( /// that offer, and hashing it is work spent to produce a number nobody can /// use. `eligible` is where the caller says which entries those are; it is /// called exactly once per entry, before any hashing. +/// +/// The index comes along because the interesting callers have already +/// worked something out per entry — `report` classifies first — and +/// looking that answer up by position beats re-deriving it or keeping a +/// set of paths the size of the disk. pub fn find_duplicates_filtered( entries: &mut [FileEntry], eligible: F, cancelled: &AtomicBool, ) -> Option> where - F: Fn(&FileEntry) -> bool, + F: Fn(usize, &FileEntry) -> bool, { - let keep: Vec = entries.iter().map(eligible).collect(); + let keep: Vec = entries + .iter() + .enumerate() + .map(|(i, e)| eligible(i, e)) + .collect(); // Stage 1: bucket by size. let mut by_size: HashMap> = HashMap::new(); @@ -168,7 +177,7 @@ mod tests { let never = AtomicBool::new(false); let sets = find_duplicates_filtered( &mut entries, - |e| e.path.file_name().is_some_and(|n| n.to_string_lossy().starts_with("keep")), + |_, e| e.path.file_name().is_some_and(|n| n.to_string_lossy().starts_with("keep")), &never, ) .unwrap(); diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 7ac3a0d..3ff8f1e 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -64,7 +64,7 @@ pub fn build_with( let duplicate_sets = dedup::find_duplicates_filtered( &mut entries, - |e| e.size >= opts.dedup_min_size, + |_, e| e.size >= opts.dedup_min_size, cancelled, )?; let files_scanned = entries.len() as u64; From 3ebc255fa3176cbe27562465860f379d2f64900a Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:18:29 +0500 Subject: [PATCH 09/33] fix(report): stop counting duplicated and protected bytes as reclaimable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the headline number overstated what a user can actually free. The halves overlap. `total_reclaimable` was findings + duplicate waste, and a file is often in both: two identical 1 GB installers under `/tmp` are two findings at 1 GB each *and* a duplicate set with 1 GB wasted, so the headline said 3 GB where 2 GB is the whole disk's worth. Findings are now counted in full, and a duplicate set adds only the redundant copies nothing has counted yet — `size * (copies - 1 - already counted)`. Protected files were duplicate candidates. `find_duplicates` ran over every entry, so two copies of a driver-store file contributed `wasted` bytes to a total the app will never offer to act on. Classification now happens before dedup rather than after, which lets protected entries sit the stage out — they are also the entries most expensive to hash and least useful to. Four tests pin the arithmetic from each side: both copies already findings, neither copy a finding, one of each, and protected. Closes #44 --- crates/diskern-core/src/report.rs | 202 +++++++++++++++++++++++++++--- 1 file changed, 183 insertions(+), 19 deletions(-) diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 3ff8f1e..58fd49a 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -3,6 +3,7 @@ use crate::{dedup, risk, rules::RulesDb, Category, FileEntry, Finding, Verdict}; use serde::{Deserialize, Serialize}; +use std::collections::HashSet; use std::sync::atomic::{AtomicBool, Ordering}; /// Knobs for [`build_with`] that are about the report, not the walk. @@ -62,21 +63,34 @@ pub fn build_with( .map(|d| d.as_secs() as i64) .unwrap_or(0); + // Classify before dedup, not after. A protected file has no business + // in a duplicate set — the set is an offer to keep one copy and drop + // the rest, and dropping a driver store copy is not on offer — and + // hashing it is time spent producing a number nobody can act on. + // + // Classification is cheap per entry, but a home directory is millions + // of them, so it happens once and the answer is kept. + let mut verdicts: Vec<(Category, Verdict, Option<&crate::rules::Rule>)> = + Vec::with_capacity(entries.len()); + for entry in &entries { + if cancelled.load(Ordering::Relaxed) { + return None; + } + verdicts.push(rules.classify(&entry.path)); + } + let duplicate_sets = dedup::find_duplicates_filtered( &mut entries, - |_, e| e.size >= opts.dedup_min_size, + |i, e| e.size >= opts.dedup_min_size && verdicts[i].1 != Verdict::Protected, cancelled, )?; let files_scanned = entries.len() as u64; let mut findings = Vec::new(); - for entry in entries { - // Classification is cheap per entry, but a home directory is - // millions of them — cheap times millions is still a wait. + for (entry, (category, verdict, rule)) in entries.into_iter().zip(verdicts) { if cancelled.load(Ordering::Relaxed) { return None; } - let (category, verdict, rule) = rules.classify(&entry.path); // Unknown + unremarkable files aren't findings; don't drown the user. if category == Category::Unknown { @@ -103,8 +117,7 @@ pub fn build_with( }); } - let total_reclaimable = findings.iter().map(|f| f.reclaimable).sum::() - + duplicate_sets.iter().map(|d| d.wasted).sum::(); + let total_reclaimable = total_reclaimable(&findings, &duplicate_sets); findings.sort_by_key(|f| std::cmp::Reverse(f.reclaimable)); @@ -116,12 +129,69 @@ pub fn build_with( }) } +/// The two halves of the report overlap, so they can't just be added. +/// +/// A finding offers the file's own bytes. A duplicate set offers the +/// copies beyond the first. A file is often both: two identical 1 GB +/// installers under `/tmp` are two findings at 1 GB each *and* a duplicate +/// set with 1 GB wasted, and summing those said 3 GB when 2 GB is +/// everything there is. +/// +/// So findings are counted in full, and a duplicate set adds only the +/// redundant copies nobody has counted yet. +fn total_reclaimable(findings: &[Finding], duplicate_sets: &[dedup::DuplicateSet]) -> u64 { + let counted: HashSet<&std::path::Path> = findings + .iter() + .filter(|f| f.reclaimable > 0) + .map(|f| f.entry.path.as_path()) + .collect(); + + let from_findings: u64 = findings.iter().map(|f| f.reclaimable).sum(); + let from_duplicates: u64 = duplicate_sets + .iter() + .map(|set| { + // One copy always stays; that is what makes the rest redundant. + let redundant = set.paths.len().saturating_sub(1); + let already = set + .paths + .iter() + .filter(|p| counted.contains(p.as_path())) + .count(); + set.size * redundant.saturating_sub(already) as u64 + }) + .sum(); + + from_findings + from_duplicates +} + #[cfg(test)] mod tests { use super::*; use crate::scanner::{scan, ScanOptions, ScanProgress}; use std::sync::Arc; + fn temp_rules() -> RulesDb { + RulesDb::new( + 1, + vec![ + crate::rules::Rule { + id: "test-protected".into(), + patterns: vec!["**/dk-sys/**".into()], + category: Category::SystemCritical, + verdict: Verdict::Protected, + description: "test".into(), + }, + crate::rules::Rule { + id: "test-temp".into(), + patterns: vec!["**/dk-scratch/**".into()], + category: Category::TempFile, + verdict: Verdict::Review, + description: "test".into(), + }, + ], + ) + } + fn scan_dir(dir: &std::path::Path) -> Vec { let opts = ScanOptions { roots: vec![dir.to_path_buf()], @@ -137,23 +207,13 @@ mod tests { #[test] fn the_dedup_minimum_does_not_hide_files_from_the_report() { let dir = tempfile::tempdir().unwrap(); - let tmp = dir.path().join("tmp"); + let tmp = dir.path().join("dk-scratch"); std::fs::create_dir(&tmp).unwrap(); std::fs::write(tmp.join("empty.log"), b"").unwrap(); std::fs::write(tmp.join("small-a"), b"tiny").unwrap(); std::fs::write(tmp.join("small-b"), b"tiny").unwrap(); - let rules = RulesDb::new( - 1, - vec![crate::rules::Rule { - id: "test-temp".into(), - patterns: vec!["**/tmp/**".into()], - category: Category::TempFile, - verdict: Verdict::Review, - description: "test".into(), - }], - ); - + let rules = temp_rules(); let entries = scan_dir(dir.path()); let never = AtomicBool::new(false); @@ -172,4 +232,108 @@ mod tests { assert_eq!(report.files_scanned, 3); assert_eq!(report.duplicate_sets.len(), 1); } + + /// Issue #44. Two identical files under `/tmp` are two findings *and* + /// one duplicate set. Adding both halves counted the same bytes twice: + /// 4 + 4 + 4 = 12 where 8 is everything on the disk. + #[test] + fn duplicated_findings_are_not_counted_twice() { + let dir = tempfile::tempdir().unwrap(); + let tmp = dir.path().join("dk-scratch"); + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("a.iso"), b"same").unwrap(); + std::fs::write(tmp.join("b.iso"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert_eq!(report.findings.len(), 2); + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.duplicate_sets[0].wasted, 4); + // Both copies are already offered as findings, so the duplicate + // set adds nothing on top of them. + assert_eq!(report.total_reclaimable, 8); + } + + /// A duplicate whose copies are not findings still contributes — the + /// fix must not swing the other way and undercount. + #[test] + fn duplicates_outside_the_findings_still_count() { + let dir = tempfile::tempdir().unwrap(); + let docs = dir.path().join("docs"); + std::fs::create_dir(&docs).unwrap(); + std::fs::write(docs.join("a.txt"), b"same").unwrap(); + std::fs::write(docs.join("b.txt"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert!(report.findings.is_empty()); // unknown category, dropped + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.total_reclaimable, 4); // one redundant copy + } + + /// Half in, half out: one copy is an actionable finding, the other is + /// unclassified user data. Acting on everything frees one copy's worth. + #[test] + fn a_duplicate_shared_with_a_finding_counts_once() { + let dir = tempfile::tempdir().unwrap(); + let tmp = dir.path().join("dk-scratch"); + let docs = dir.path().join("docs"); + std::fs::create_dir(&tmp).unwrap(); + std::fs::create_dir(&docs).unwrap(); + std::fs::write(tmp.join("a.iso"), b"same").unwrap(); + std::fs::write(docs.join("keep.iso"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert_eq!(report.findings.len(), 1); + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.total_reclaimable, 4); + } + + /// The other half of #44: `find_duplicates` ran over every entry, + /// including protected ones, so system files contributed `wasted` + /// bytes to a total the user is never allowed to act on. + #[test] + fn protected_files_form_no_duplicate_sets() { + let dir = tempfile::tempdir().unwrap(); + let sys = dir.path().join("dk-sys"); + std::fs::create_dir(&sys).unwrap(); + std::fs::write(sys.join("a.dll"), b"same").unwrap(); + std::fs::write(sys.join("b.dll"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert_eq!(report.findings.len(), 2); + assert!(report.findings.iter().all(|f| f.verdict == Verdict::Protected)); + assert!(report.duplicate_sets.is_empty()); + assert_eq!(report.total_reclaimable, 0); + } } From 03db5e64b1e7da4eacc9e3b084937f1bdd163ceb Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:19:14 +0500 Subject: [PATCH 10/33] fix(actions): fall back to copy+remove when restoring across filesystems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `quarantine` handled `rename` failing with EXDEV and fell back to copy then remove. `restore` was a bare `fs::rename`, so the exact case the move survived was the case the undo failed on. That case is the normal one, not an edge one: quarantine lives in the app's local data directory and the scanned files come from wherever the user pointed the scan — a second drive, or a `/home` on its own partition. Quarantining worked; restoring returned EXDEV and the file stayed where the app had put it. Both directions now go through one `move_file`. The copy-then-remove half is its own function so it can be tested without two mounts to hand, and the remove stays last: a failure there leaves the file in both places, which is recoverable, where the other order would not be. Closes #42 --- crates/diskern-core/src/actions.rs | 71 +++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 7 deletions(-) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 9aa1224..935dd2a 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -39,11 +39,7 @@ pub fn quarantine( let flat = file.to_string_lossy().replace(['/', '\\', ':'], "_"); let dest = quarantine_dir.join(format!("{stamp}_{flat}")); - // rename() fails across filesystems; fall back to copy+remove. - if std::fs::rename(file, &dest).is_err() { - std::fs::copy(file, &dest).map_err(|e| io_err(file, e))?; - std::fs::remove_file(file).map_err(|e| io_err(file, e))?; - } + move_file(file, &dest)?; Ok(QuarantineRecord { original: file.to_path_buf(), @@ -57,8 +53,36 @@ pub fn restore(record: &QuarantineRecord) -> Result<()> { if let Some(parent) = record.original.parent() { std::fs::create_dir_all(parent).map_err(|e| io_err(parent, e))?; } - std::fs::rename(&record.quarantined_to, &record.original) - .map_err(|e| io_err(&record.quarantined_to, e)) + move_file(&record.quarantined_to, &record.original) +} + +/// Move a file, in the one direction this crate is allowed to move things. +/// +/// `rename` is the fast path and fails with `EXDEV` when the two paths are +/// on different filesystems — which is the normal case here, not an edge +/// one: quarantine lives in the app's local data directory, and the files +/// being quarantined come from wherever the user pointed the scan, which +/// may well be a second drive or a `/home` on its own partition. +/// +/// Both directions go through this. The quarantine path used to handle the +/// fallback and the restore path didn't, so the exact case the move +/// survived was the case the undo failed on. +fn move_file(from: &Path, to: &Path) -> Result<()> { + if std::fs::rename(from, to).is_ok() { + return Ok(()); + } + copy_then_remove(from, to) +} + +/// The fallback half of [`move_file`], separated so it can be tested +/// without two filesystems to hand. +/// +/// The remove comes last on purpose: if it fails, the file still exists in +/// both places, which is recoverable. Removing first and failing to copy +/// would not be. +fn copy_then_remove(from: &Path, to: &Path) -> Result<()> { + std::fs::copy(from, to).map_err(|e| io_err(from, e))?; + std::fs::remove_file(from).map_err(|e| io_err(from, e)) } fn io_err(path: &Path, source: std::io::Error) -> GenomeError { @@ -94,6 +118,39 @@ mod tests { assert!(f.exists()); } + /// Issue #42. `quarantine` handled the cross-filesystem case and + /// `restore` didn't, so quarantining a file from another mount worked + /// and undoing it returned EXDEV. Provoking a real EXDEV needs two + /// mounts, so this exercises the fallback both directions now share. + #[test] + fn the_cross_filesystem_fallback_moves_the_bytes() { + let dir = tempfile::tempdir().unwrap(); + let from = dir.path().join("from.txt"); + let to = dir.path().join("to.txt"); + std::fs::write(&from, b"data").unwrap(); + + copy_then_remove(&from, &to).unwrap(); + + assert!(!from.exists()); + assert_eq!(std::fs::read(&to).unwrap(), b"data"); + } + + #[test] + fn restore_recreates_a_directory_that_was_removed_meanwhile() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let nested = dir.path().join("a/b"); + std::fs::create_dir_all(&nested).unwrap(); + let f = nested.join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + + let rec = quarantine(&f, Verdict::Safe, &q).unwrap(); + std::fs::remove_dir_all(dir.path().join("a")).unwrap(); + + restore(&rec).unwrap(); + assert_eq!(std::fs::read(&f).unwrap(), b"data"); + } + #[test] fn refuses_protected() { let dir = tempfile::tempdir().unwrap(); From 5027ae72bd5fb6cd8357c90e5fba3d07bee0d02a Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:20:59 +0500 Subject: [PATCH 11/33] feat(actions): record every quarantine in a manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module doc promised files are moved "with a manifest recording original locations, so every action is reversible until the user explicitly purges quarantine". There was no manifest. `quarantine` returned a `QuarantineRecord` and the caller dropped it — `App.jsx` ignores the resolved value of `invoke("quarantine_finding", ...)` — so reversibility lasted exactly as long as the value nobody kept. The quarantine filenames could not stand in for it either: flattening maps `/` and `_` onto the same character, so `1717000000_home_user_.cache_x` has no single original it can be read back to. Now `quarantine` appends the record to `manifest.jsonl` in the quarantine directory before it returns, and this module gains the three operations that record makes possible: - `list` — everything still quarantined, read off disk - `restore_from_manifest` — restore one file and stop listing it - `purge` — the "purge quarantine" step the README already described Details that matter more than they look: - the same flattening collision that made filenames useless could also make two originals land on one quarantine path within a second, where the second rename silently overwrote the first. Destinations are now unique, and names are capped so a deep path can't exceed the 255-byte filename limit. - `purge` removes only what the manifest lists. Anything else in the directory is somebody else's file. - rewrites go through a temp file and a rename, so a crash mid-write leaves the previous manifest rather than a truncated one. - one torn line is skipped with a warning instead of hiding every other record behind it. --- crates/diskern-core/src/actions.rs | 314 ++++++++++++++++++++++++++++- 1 file changed, 309 insertions(+), 5 deletions(-) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 935dd2a..d9b8559 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -3,11 +3,31 @@ //! Design rule: Diskern never hard-deletes. Files are moved to a //! quarantine directory with a manifest recording original locations, so //! every action is reversible until the user explicitly purges quarantine. +//! +//! The manifest is what makes "reversible" outlive the process. It is a +//! JSON Lines file in the quarantine directory: one [`QuarantineRecord`] +//! per line, appended as files arrive, rewritten when one leaves. The +//! quarantine filenames cannot stand in for it — flattening a path maps +//! `/` and `_` onto the same character, so `1700000000_home_user_.cache_x` +//! has no single original it could be read back to. +//! +//! [`purge`] is the one operation here that deletes, and it deletes only +//! what the manifest says this crate put there. use crate::{GenomeError, Result, Verdict}; use serde::{Deserialize, Serialize}; +use std::io::Write; use std::path::{Path, PathBuf}; +/// Quarantine's record of itself, inside the quarantine directory. +pub const MANIFEST_NAME: &str = "manifest.jsonl"; + +/// Longest flattened name we will build. Filenames are capped at 255 +/// bytes on every filesystem Diskern targets, and a deep path flattens +/// past that easily; the tail is kept because that is the half that +/// distinguishes two files. +const MAX_FLAT_LEN: usize = 180; + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct QuarantineRecord { pub original: PathBuf, @@ -34,21 +54,57 @@ pub fn quarantine( std::fs::create_dir_all(quarantine_dir).map_err(|e| io_err(quarantine_dir, e))?; - // Flatten path into a unique quarantine filename. let stamp = now_epoch(); - let flat = file.to_string_lossy().replace(['/', '\\', ':'], "_"); - let dest = quarantine_dir.join(format!("{stamp}_{flat}")); + let dest = unique_dest(quarantine_dir, stamp, file); move_file(file, &dest)?; - Ok(QuarantineRecord { + let record = QuarantineRecord { original: file.to_path_buf(), quarantined_to: dest, at_epoch: stamp, - }) + }; + + // Record before returning. A caller that drops the returned value — + // which is exactly what the app's `quarantine_finding` used to do — + // must not be able to lose the only note of where the file came from. + append_to_manifest(quarantine_dir, &record)?; + Ok(record) +} + +/// A quarantine filename that no existing file already owns. +/// +/// Flattening is lossy on purpose (it only has to be readable; the +/// manifest is the source of truth), which means two different originals +/// can flatten to the same name inside the same second. Without this the +/// second `rename` would silently overwrite the first — quarantine losing +/// a file is the one failure this module cannot have. +fn unique_dest(quarantine_dir: &Path, stamp: i64, file: &Path) -> PathBuf { + let mut flat = file.to_string_lossy().replace(['/', '\\', ':'], "_"); + if flat.len() > MAX_FLAT_LEN { + // Byte-slice on a char boundary; a split mid-codepoint would panic. + let cut = flat + .char_indices() + .map(|(i, _)| i) + .find(|&i| flat.len() - i <= MAX_FLAT_LEN) + .unwrap_or(0); + flat = flat.split_off(cut); + } + + let base = format!("{stamp}_{flat}"); + let mut candidate = quarantine_dir.join(&base); + let mut n = 1u32; + while candidate.exists() { + candidate = quarantine_dir.join(format!("{base}.{n}")); + n += 1; + } + candidate } /// Restore a quarantined file to its original location. +/// +/// Leaves the manifest alone — see [`restore_from_manifest`] for the +/// version that also stops listing the file as quarantined. pub fn restore(record: &QuarantineRecord) -> Result<()> { if let Some(parent) = record.original.parent() { std::fs::create_dir_all(parent).map_err(|e| io_err(parent, e))?; @@ -56,6 +112,154 @@ pub fn restore(record: &QuarantineRecord) -> Result<()> { move_file(&record.quarantined_to, &record.original) } +/// Everything the manifest says is currently in quarantine, oldest first. +/// +/// A quarantine directory with no manifest is empty, not broken: the app +/// resolves the path before anything has been quarantined into it. +pub fn list(quarantine_dir: &Path) -> Result> { + let path = manifest_path(quarantine_dir); + let contents = match std::fs::read_to_string(&path) { + Ok(c) => c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(vec![]), + Err(e) => return Err(io_err(&path, e)), + }; + + Ok(contents + .lines() + .filter(|line| !line.trim().is_empty()) + .filter_map(|line| match serde_json::from_str::(line) { + Ok(record) => Some(record), + // One torn line — a half-written append after a power cut — + // must not hide every other file from the restore list. + Err(e) => { + tracing::warn!(manifest = %path.display(), "skipping unreadable manifest line: {e}"); + None + } + }) + .collect()) +} + +/// Restore the file quarantined *to* `quarantined_to` and stop listing it. +/// +/// Addressed by quarantine path rather than by original path: the +/// quarantine path is the unique one. The same original can be quarantined +/// again after being restored, and both attempts are real records. +pub fn restore_from_manifest( + quarantine_dir: &Path, + quarantined_to: &Path, +) -> Result { + let records = list(quarantine_dir)?; + let record = records + .iter() + .find(|r| r.quarantined_to == quarantined_to) + .cloned() + .ok_or_else(|| { + GenomeError::Rules(format!( + "{} is not listed in the quarantine manifest", + quarantined_to.display() + )) + })?; + + restore(&record)?; + + // Rewrite only after the move succeeded. Dropping the line first would + // strand the file in quarantine with nothing recording where it came + // from — the failure this manifest exists to prevent. + let remaining: Vec = records + .into_iter() + .filter(|r| r.quarantined_to != quarantined_to) + .collect(); + write_manifest(quarantine_dir, &remaining)?; + Ok(record) +} + +/// What a [`purge`] actually did. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct PurgeSummary { + pub files_removed: usize, + pub bytes_removed: u64, + /// Files the manifest listed that could not be removed, with the + /// reason. Reported rather than raised: one locked file shouldn't + /// abort the purge of everything else. + pub failed: Vec, +} + +/// Empty quarantine for good. This is the only deletion in Diskern, and +/// it deletes only the files the manifest says this crate moved here — +/// never whatever else happens to be sitting in the directory. +pub fn purge(quarantine_dir: &Path) -> Result { + let records = list(quarantine_dir)?; + let mut summary = PurgeSummary::default(); + let mut kept = Vec::new(); + + for record in records { + let size = std::fs::metadata(&record.quarantined_to) + .map(|m| m.len()) + .unwrap_or(0); + match std::fs::remove_file(&record.quarantined_to) { + Ok(()) => { + summary.files_removed += 1; + summary.bytes_removed += size; + } + // Already gone: the manifest was stale, and the goal (not + // there any more) is met. Anything else stays listed so the + // user can still restore it. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + summary.files_removed += 1; + } + Err(e) => { + summary + .failed + .push(format!("{}: {e}", record.quarantined_to.display())); + kept.push(record); + } + } + } + + write_manifest(quarantine_dir, &kept)?; + Ok(summary) +} + +/// Where the manifest lives for a given quarantine directory. +pub fn manifest_path(quarantine_dir: &Path) -> PathBuf { + quarantine_dir.join(MANIFEST_NAME) +} + +fn append_to_manifest(quarantine_dir: &Path, record: &QuarantineRecord) -> Result<()> { + let path = manifest_path(quarantine_dir); + let line = serde_json::to_string(record).map_err(|e| { + GenomeError::Rules(format!("could not serialize a quarantine record: {e}")) + })?; + + let mut file = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .map_err(|e| io_err(&path, e))?; + writeln!(file, "{line}").map_err(|e| io_err(&path, e)) +} + +/// Rewrite the whole manifest. Via a temporary file in the same directory +/// so a crash mid-write leaves the old manifest intact rather than a +/// truncated one — the manifest is the only copy of where these files +/// belong. +fn write_manifest(quarantine_dir: &Path, records: &[QuarantineRecord]) -> Result<()> { + let path = manifest_path(quarantine_dir); + let tmp = path.with_extension("jsonl.tmp"); + + let mut body = String::new(); + for record in records { + let line = serde_json::to_string(record).map_err(|e| { + GenomeError::Rules(format!("could not serialize a quarantine record: {e}")) + })?; + body.push_str(&line); + body.push('\n'); + } + + std::fs::write(&tmp, body).map_err(|e| io_err(&tmp, e))?; + std::fs::rename(&tmp, &path).map_err(|e| io_err(&path, e)) +} + /// Move a file, in the one direction this crate is allowed to move things. /// /// `rename` is the fast path and fails with `EXDEV` when the two paths are @@ -159,4 +363,104 @@ mod tests { assert!(quarantine(&f, Verdict::Protected, dir.path()).is_err()); assert!(f.exists()); // untouched } + + /// Issue #43. The module promised every action stays reversible until + /// the user purges, and the only record of where a file came from was + /// the value `quarantine` returned — which the app dropped on the + /// floor. Reversibility has to survive the process exiting. + #[test] + fn a_record_outlives_the_process_that_made_it() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let f = dir.path().join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + + // Return value deliberately ignored, the way the app used to. + let _ = quarantine(&f, Verdict::Safe, &q).unwrap(); + + // Nothing carried over in memory: read it back off the disk. + let listed = list(&q).unwrap(); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].original, f); + + restore_from_manifest(&q, &listed[0].quarantined_to).unwrap(); + assert_eq!(std::fs::read(&f).unwrap(), b"data"); + assert!(list(&q).unwrap().is_empty()); + } + + /// Flattening maps `/` and `_` onto the same character, so two + /// different originals can produce the same name inside one second. + /// Before `unique_dest` the second rename overwrote the first. + #[test] + fn two_files_that_flatten_alike_do_not_overwrite_each_other() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let a = dir.path().join("a_b"); + let b_dir = dir.path().join("a"); + std::fs::create_dir(&b_dir).unwrap(); + let b = b_dir.join("b"); + std::fs::write(&a, b"first").unwrap(); + std::fs::write(&b, b"second").unwrap(); + + let ra = quarantine(&a, Verdict::Safe, &q).unwrap(); + let rb = quarantine(&b, Verdict::Safe, &q).unwrap(); + + assert_ne!(ra.quarantined_to, rb.quarantined_to); + assert_eq!(std::fs::read(&ra.quarantined_to).unwrap(), b"first"); + assert_eq!(std::fs::read(&rb.quarantined_to).unwrap(), b"second"); + assert_eq!(list(&q).unwrap().len(), 2); + } + + #[test] + fn an_empty_quarantine_lists_nothing_rather_than_failing() { + let dir = tempfile::tempdir().unwrap(); + assert!(list(&dir.path().join("never-used")).unwrap().is_empty()); + } + + /// A half-written line after a power cut must not hide the records + /// either side of it. + #[test] + fn a_torn_manifest_line_is_skipped_not_fatal() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let f = dir.path().join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + quarantine(&f, Verdict::Safe, &q).unwrap(); + + let manifest = manifest_path(&q); + let good = std::fs::read_to_string(&manifest).unwrap(); + std::fs::write(&manifest, format!("{{\"original\": tru\n{good}")).unwrap(); + + assert_eq!(list(&q).unwrap().len(), 1); + } + + #[test] + fn purge_removes_the_files_it_listed_and_nothing_else() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let f = dir.path().join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + let rec = quarantine(&f, Verdict::Safe, &q).unwrap(); + + // Something Diskern did not put here. + let stranger = q.join("not-ours.txt"); + std::fs::write(&stranger, b"leave me alone").unwrap(); + + let summary = purge(&q).unwrap(); + assert_eq!(summary.files_removed, 1); + assert_eq!(summary.bytes_removed, 4); + assert!(summary.failed.is_empty()); + + assert!(!rec.quarantined_to.exists()); + assert!(stranger.exists()); + assert!(list(&q).unwrap().is_empty()); + } + + #[test] + fn restoring_something_not_in_the_manifest_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + std::fs::create_dir_all(&q).unwrap(); + assert!(restore_from_manifest(&q, &q.join("invented")).is_err()); + } } From 79dbd692663f71c1e825cb7864209c8e4d7d1b7e Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:23:57 +0500 Subject: [PATCH 12/33] fix(app): stop the progress ticker from a guard that can't be skipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `start_scan` spawns a thread that emits `scan-progress` every 150ms and stopped it with two statements after the `.await`. Statements after an await only run if control reaches them. It did not, twice over. There used to be a `?` between the await and the `stop.store`, so a panicking blocking task — a rayon or jwalk worker dying on an odd filesystem — returned the join error and left the thread emitting progress for the rest of the process, behind whatever the error state the UI then showed. That `?` has since moved below the cleanup, but the shape is still fragile: dropping the command's future, which is how Tauri cancels a command, skips the cleanup entirely. Both cleanups now live in a `ScanRun` guard and run from `Drop`. The slot-clearing goes with the ticker for the same reason and keeps the same identity check: clear only if the slot still holds *this* scan, so a short scan that started second can't erase a longer one's handle. Closes #45 --- app/src-tauri/src/commands.rs | 127 +++++++++++++++++++++++----------- 1 file changed, 86 insertions(+), 41 deletions(-) diff --git a/app/src-tauri/src/commands.rs b/app/src-tauri/src/commands.rs index f6d6737..92d3f78 100644 --- a/app/src-tauri/src/commands.rs +++ b/app/src-tauri/src/commands.rs @@ -33,6 +33,79 @@ struct ScanProgressPayload { bytes_seen: u64, } +/// Everything a running scan owns outside itself: the ticker thread that +/// emits `scan-progress`, and this scan's entry in the shared +/// [`ActiveScan`] slot. Both are released on drop. +/// +/// They used to be released by statements after the `.await`, which only +/// run if control reaches them. A `?` between the two — there was one — +/// returned on a panicking blocking task and left the ticker emitting +/// every 150ms for the rest of the process, behind whatever the UI showed +/// next. Dropping the command's future, which is how a Tauri command is +/// cancelled, skipped the cleanup entirely. A guard cannot be skipped. +struct ScanRun<'a> { + stop: Arc, + ticker: Option>, + state: &'a ActiveScan, + progress: Arc, +} + +impl<'a> ScanRun<'a> { + fn start( + window: &Window, + state: &'a ActiveScan, + progress: Arc, + ) -> Self { + let stop = Arc::new(std::sync::atomic::AtomicBool::new(false)); + + // A plain OS thread keeps this independent of whatever async + // runtime Tauri is using internally. + let ticker = { + let progress = progress.clone(); + let stop = stop.clone(); + let window = window.clone(); + std::thread::spawn(move || { + while !stop.load(Ordering::Relaxed) { + let payload = ScanProgressPayload { + files_seen: progress.files_seen.load(Ordering::Relaxed), + bytes_seen: progress.bytes_seen.load(Ordering::Relaxed), + }; + let _ = window.emit("scan-progress", payload); + std::thread::sleep(Duration::from_millis(150)); + } + }) + }; + + Self { + stop, + ticker: Some(ticker), + state, + progress, + } + } +} + +impl Drop for ScanRun<'_> { + fn drop(&mut self) { + self.stop.store(true, Ordering::Relaxed); + if let Some(ticker) = self.ticker.take() { + let _ = ticker.join(); + } + + // Clear only if the slot still holds *this* scan. An unconditional + // `= None` would let a short scan that started second erase a + // longer one's handle when it finished first, leaving the survivor + // with a Cancel button wired to nothing. + let mut slot = self.state.slot(); + if slot + .as_ref() + .is_some_and(|current| Arc::ptr_eq(current, &self.progress)) + { + *slot = None; + } + } +} + /// Read-only scan. Safe to expose; touches nothing. /// /// Emits a `scan-progress` event roughly every 150ms while running, so the @@ -54,32 +127,12 @@ pub async fn start_scan( // here on purpose — holding it across the await below would make this // future non-Send. *state.slot() = Some(progress.clone()); - let stop = Arc::new(std::sync::atomic::AtomicBool::new(false)); - - // Ticker thread: emits progress snapshots on an interval until `stop` - // is set once the scan below finishes. A plain OS thread keeps this - // independent of whatever async runtime Tauri is using internally. - let ticker = { - let progress = progress.clone(); - let stop = stop.clone(); - let window = window.clone(); - std::thread::spawn(move || { - while !stop.load(Ordering::Relaxed) { - let payload = ScanProgressPayload { - files_seen: progress.files_seen.load(Ordering::Relaxed), - bytes_seen: progress.bytes_seen.load(Ordering::Relaxed), - }; - let _ = window.emit("scan-progress", payload); - std::thread::sleep(Duration::from_millis(150)); - } - }) - }; + + // Everything this scan has to undo, undone on the way out however the + // way out happens. + let run = ScanRun::start(&window, &state, progress.clone()); let progress_for_scan = progress.clone(); - // Note the missing `?`: the join result is unwrapped *after* the cleanup - // below. Returning early here would leave the ticker running and this - // scan still listed as in-flight, so cancelling would target a scan that - // had already ended. let joined = tauri::async_runtime::spawn_blocking(move || { let opts = scanner::ScanOptions { roots, @@ -102,21 +155,7 @@ pub async fn start_scan( }) .await; - stop.store(true, Ordering::Relaxed); - let _ = ticker.join(); - // Clear only if the slot still holds *this* scan. An unconditional - // `= None` would let a short scan that started second erase a longer - // one's handle when it finished first, leaving the survivor with a - // Cancel button wired to nothing. - { - let mut slot = state.slot(); - if slot - .as_ref() - .is_some_and(|current| Arc::ptr_eq(current, &progress)) - { - *slot = None; - } - } + drop(run); // One final snapshot so the UI's last-seen count matches the real total. let _ = window.emit( @@ -149,8 +188,14 @@ pub fn cancel_scan(state: State<'_, ActiveScan>) -> bool { } } -/// The ONLY mutating command. Re-classifies server-side before acting — -/// the frontend's claimed verdict is never trusted. +/// The mutating commands below all take `quarantine_dir` from the +/// frontend, which resolves it to `/Quarantine`. Nothing +/// here trusts a verdict the frontend claims — `quarantine_finding` +/// re-classifies, and the restore/purge commands only ever touch paths +/// the manifest in that directory says Diskern put there itself. +/// +/// Re-classifies server-side before acting — the frontend's claimed +/// verdict is never trusted. #[tauri::command] pub async fn quarantine_finding( path: PathBuf, From e538dbb5ce77c0506780bb948314db67a92fc543 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:24:07 +0500 Subject: [PATCH 13/33] feat(app): expose the quarantine manifest as commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three commands over the manifest the engine now keeps: - `list_quarantine` — read-only, everything still quarantined - `restore_quarantined` — put one file back, addressed by its path *in* quarantine, which is the unique one; the same original can be quarantined, restored and quarantined again - `purge_quarantine` — the one command in Diskern that deletes, and it deletes only what the manifest lists All three take `quarantine_dir` from the frontend the way `quarantine_finding` already does, and all three go through the manifest rather than the directory listing, so a path the webview invents reaches no file. --- app/src-tauri/src/commands.rs | 48 +++++++++++++++++++++++++++++++++++ app/src-tauri/src/lib.rs | 3 +++ 2 files changed, 51 insertions(+) diff --git a/app/src-tauri/src/commands.rs b/app/src-tauri/src/commands.rs index 92d3f78..dd78afa 100644 --- a/app/src-tauri/src/commands.rs +++ b/app/src-tauri/src/commands.rs @@ -214,3 +214,51 @@ pub async fn quarantine_finding( .await .map_err(|e| e.to_string())? } + +/// Everything still in quarantine, read from the manifest on disk. +/// +/// Read-only. This is what makes quarantine reversible across restarts: +/// before the manifest existed, the record of where a file came from lived +/// only in the value `quarantine_finding` returned, and the UI dropped it. +#[tauri::command] +pub async fn list_quarantine( + quarantine_dir: PathBuf, +) -> Result, String> { + tauri::async_runtime::spawn_blocking(move || { + actions::list(&quarantine_dir).map_err(|e| e.to_string()) + }) + .await + .map_err(|e| e.to_string())? +} + +/// Put one quarantined file back where it came from. +/// +/// Addressed by its path *in quarantine*, which is the unique one — the +/// same original can be quarantined, restored and quarantined again. The +/// manifest is consulted first, so a path the frontend invented reaches +/// no file. +#[tauri::command] +pub async fn restore_quarantined( + quarantine_dir: PathBuf, + quarantined_to: PathBuf, +) -> Result { + tauri::async_runtime::spawn_blocking(move || { + actions::restore_from_manifest(&quarantine_dir, &quarantined_to).map_err(|e| e.to_string()) + }) + .await + .map_err(|e| e.to_string())? +} + +/// Empty quarantine for good. +/// +/// The one command in Diskern that deletes anything, and it deletes only +/// the files the manifest lists — the ones this app moved there. The UI +/// confirms first; this is past the point of no return. +#[tauri::command] +pub async fn purge_quarantine(quarantine_dir: PathBuf) -> Result { + tauri::async_runtime::spawn_blocking(move || { + actions::purge(&quarantine_dir).map_err(|e| e.to_string()) + }) + .await + .map_err(|e| e.to_string())? +} diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 3867d69..dc719ee 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -19,6 +19,9 @@ pub fn run() { commands::start_scan, commands::cancel_scan, commands::quarantine_finding, + commands::list_quarantine, + commands::restore_quarantined, + commands::purge_quarantine, ]) .run(tauri::generate_context!()) .expect("error while running Diskern"); From a06ba8a196c99d21ec0047edad424de23951b36a Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:24:19 +0500 Subject: [PATCH 14/33] feat(app): add a Quarantine panel with restore and purge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last missing half of reversibility: somewhere to call restore from. The panel reads the manifest off disk rather than anything this session remembers, so it renders before any scan has been run — files quarantined in an earlier session are restorable without scanning again, which is the whole point of the manifest existing. Restoring puts a row back in the report it came from and takes its bytes back out of the reclaimed running total; a record with no matching finding (a different scan, or a restart) just refreshes the list. Purge asks first and says what it did — files removed, bytes freed, and how many it couldn't remove. It's the only irreversible thing in the app, so it's a two-step confirm behind a collapsed section rather than a button sitting next to the scan results. Closes #43 --- app/src/App.jsx | 173 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 172 insertions(+), 1 deletion(-) diff --git a/app/src/App.jsx b/app/src/App.jsx index 9f970e7..3691aa3 100644 --- a/app/src/App.jsx +++ b/app/src/App.jsx @@ -1,4 +1,4 @@ -import React, { useState, useMemo, useRef, useEffect } from "react"; +import React, { useState, useMemo, useRef, useEffect, useCallback } from "react"; import { invoke } from "@tauri-apps/api/core"; import { open } from "@tauri-apps/plugin-dialog"; import { listen } from "@tauri-apps/api/event"; @@ -199,6 +199,145 @@ function DuplicatesSection({ sets }) { ); } +/** + * What is sitting in quarantine right now, read back from the manifest on + * disk rather than from anything this session remembers. + * + * That distinction is the point. Quarantine is only "reversible" if the + * record of where a file came from outlives the window it was moved in — + * before the manifest existed, closing the app stranded every quarantined + * file with a flattened filename nobody could read an original path out + * of. So this renders whether or not a scan has been run. + */ +function QuarantineSection({ quarantineDir, refreshKey, onRestored }) { + const [records, setRecords] = useState([]); + const [isOpen, setIsOpen] = useState(false); + const [error, setError] = useState(null); + const [busyPath, setBusyPath] = useState(null); + const [purgePhase, setPurgePhase] = useState("idle"); // idle | confirming | working + const [purgeNotice, setPurgeNotice] = useState(null); + + const reload = useCallback(async () => { + if (!quarantineDir) return; + try { + setRecords(await invoke("list_quarantine", { quarantineDir })); + setError(null); + } catch (e) { + setError(String(e)); + } + }, [quarantineDir]); + + useEffect(() => { + reload(); + }, [reload, refreshKey]); + + async function restore(record) { + setError(null); + setBusyPath(record.quarantined_to); + try { + await invoke("restore_quarantined", { + quarantineDir, + quarantinedTo: record.quarantined_to, + }); + onRestored(record); + await reload(); + } catch (e) { + setError(String(e)); + } finally { + setBusyPath(null); + } + } + + async function purge() { + setError(null); + setPurgePhase("working"); + try { + const summary = await invoke("purge_quarantine", { quarantineDir }); + setPurgeNotice( + `Deleted ${summary.files_removed} file${summary.files_removed === 1 ? "" : "s"}` + + ` · ${(summary.bytes_removed / 1e6).toFixed(1)} MB freed` + + (summary.failed.length ? ` · ${summary.failed.length} could not be removed` : "") + ); + await reload(); + } catch (e) { + setError(String(e)); + } finally { + setPurgePhase("idle"); + } + } + + // Nothing quarantined and nothing to say about it: stay out of the way. + if (records.length === 0 && !error && !purgeNotice) return null; + + return ( +
+ + {isOpen && ( +
+

+ Moved here, not deleted. Restore puts a file back where it came from. + Purge is the only thing in Diskern that deletes, and it deletes only + what is listed here. +

+
    + {records.map((r) => ( +
  • + {r.original} + + {new Date(r.at_epoch * 1000).toLocaleString()} + + + {busyPath === r.quarantined_to ? ( + Restoring… + ) : ( + + )} + +
  • + ))} +
+ + {records.length > 0 && ( +
+ {purgePhase === "idle" && ( + + )} + {purgePhase === "confirming" && ( + + + Delete {records.length} file{records.length === 1 ? "" : "s"} for good? + This cannot be undone. + + + + + )} + {purgePhase === "working" && Deleting…} +
+ )} + + {purgeNotice &&

{purgeNotice}

} + {error &&

{error}

} +
+ )} +
+ ); +} + /** * Live "is this actually working" feedback while a scan runs. * @@ -243,6 +382,9 @@ export default function App() { const [quarantinedPaths, setQuarantinedPaths] = useState(() => new Set()); const [reclaimed, setReclaimed] = useState(0); const [quarantineDir, setQuarantineDir] = useState(null); + // Bumped whenever this session moves a file in, so the quarantine list + // re-reads the manifest rather than guessing at what changed. + const [quarantineVersion, setQuarantineVersion] = useState(0); const unlistenRef = useRef(null); // Resolve a sensible, always-writable quarantine location once on mount: @@ -279,6 +421,22 @@ export default function App() { return next; }); setReclaimed((prev) => prev + finding.reclaimable); + setQuarantineVersion((v) => v + 1); + } + + // A restored file is back on disk, so it belongs back in the report it + // came from — as a row again, and out of the reclaimed running total. + // A record with no matching finding (restored after a different scan, or + // after a restart) just isn't in this report; the list still refreshes. + function handleRestored(record) { + const finding = report?.findings.find((f) => f.entry.path === record.original); + setQuarantinedPaths((prev) => { + if (!prev.has(record.original)) return prev; + const next = new Set(prev); + next.delete(record.original); + return next; + }); + if (finding) setReclaimed((prev) => Math.max(0, prev - finding.reclaimable)); } // Cancelling races the scan finishing on its own; the command returns @@ -366,6 +524,13 @@ export default function App() { )} {error &&

{error}

} {notice &&

{notice}

} + {/* Rendered before any scan too: files quarantined in an earlier + session are restorable without scanning again. */} + )} @@ -392,6 +557,12 @@ export default function App() { {error &&

{error}

} {notice &&

{notice}

} + + Date: Sat, 5 Sep 2026 12:24:19 +0500 Subject: [PATCH 15/33] style(app): style the quarantine panel Quiet by default, matching the cancel button: quarantine is a safety net, not the thing anyone opened the app to look at. The purge confirmation is the exception and reuses the red the destructive confirmations already use. --- app/src/styles.css | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/src/styles.css b/app/src/styles.css index cc63417..94e9ecf 100644 --- a/app/src/styles.css +++ b/app/src/styles.css @@ -118,3 +118,10 @@ button { padding: 0.6rem 1.4rem; font-size: 1rem; cursor: pointer; } /* A cancelled scan is an outcome, not a failure — deliberately not .error. */ .notice { opacity: 0.75; font-size: 0.9rem; } + +/* Quarantine: the undo list. Deliberately closed by default — it's a + safety net, not the thing you came here to look at. */ +.quarantine-note { font-size: 0.82rem; opacity: 0.75; margin: 0.5rem 0 0; } +.purge { margin-top: 0.75rem; } +.purge .confirm { flex-wrap: wrap; } +.purge .confirm-q { color: #c0392b; opacity: 1; } From 0e35ab3aaad0b8dc05b6f59457a8444bcd34a28a Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:26:05 +0500 Subject: [PATCH 16/33] feat(graph): build an impact graph from the scanned entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `graph.rs` compiled and had no callers. This is the half it was missing: a constructor that turns a scan into project roots, dependency stores, and the `References` edges between them. One pass over the entries answers both questions. A directory holding a marker file (`Cargo.toml`, `package.json`, `pyproject.toml`) is a project root; a directory named `target`, `node_modules` or a virtualenv that an entry sits under is a store. A marker *inside* a store marks nothing — every npm package ships a `package.json`, and treating those as roots would make one `node_modules` look like ten thousand projects. A project whose own store wasn't scanned links to the nearest enclosing project's store of the same kind. That is where "referenced by 3 projects" comes from rather than always being 1: npm workspaces hoist dependencies to the repository root and Cargo workspaces share one `target/`, so the members really do reference it. `referencing_projects` now answers for enclosing directories too. Findings are files, and nothing references `proj/node_modules/react/index.js` directly — what projects reference is the store it sits in. --- crates/diskern-core/src/graph.rs | 285 ++++++++++++++++++++++++++++++- 1 file changed, 276 insertions(+), 9 deletions(-) diff --git a/crates/diskern-core/src/graph.rs b/crates/diskern-core/src/graph.rs index 306a1e6..f569e6a 100644 --- a/crates/diskern-core/src/graph.rs +++ b/crates/diskern-core/src/graph.rs @@ -8,10 +8,26 @@ //! link them to the dependency stores they reference. Everything else //! (dynamic linking, registry, plists) comes later. +use crate::FileEntry; use petgraph::graph::{DiGraph, NodeIndex}; use serde::{Deserialize, Serialize}; -use std::collections::HashMap; -use std::path::PathBuf; +use std::collections::{HashMap, HashSet}; +use std::path::{Path, PathBuf}; + +/// A file whose presence makes the directory holding it a project root, +/// and the stores a project of that kind owns. +/// +/// Root-level stores only, in v0. `__pycache__` and nested `node_modules` +/// exist at every depth and belong to whatever encloses them, which is a +/// containment question rather than a reference one. +const PROJECTS: &[(&str, ProjectKind, &[&str])] = &[ + ("cargo.toml", ProjectKind::Cargo, &["target"]), + ("package.json", ProjectKind::Npm, &["node_modules"]), + ("pyproject.toml", ProjectKind::Python, &[".venv", "venv"]), +]; + +/// Directory names that are dependency stores wherever they appear. +const STORE_NAMES: &[&str] = &["target", "node_modules", ".venv", "venv"]; #[derive(Debug, Clone, Serialize, Deserialize)] pub enum Node { @@ -45,6 +61,64 @@ pub struct ImpactGraph { } impl ImpactGraph { + /// Build the graph from one pass over the scanned entries. + /// + /// Two things come out of that pass: which directories are project + /// roots (they hold a marker file), and which are dependency stores + /// (an entry lives under one). A `References` edge is added for each + /// project to the store it owns. + /// + /// A project whose own store wasn't scanned is linked to the nearest + /// enclosing project's store of the same kind instead. That is not a + /// guess: npm workspaces hoist dependencies to the repository root and + /// Cargo workspaces share one `target/`, so the members really do + /// reference it — and it is where the "referenced by 3 projects" + /// number comes from rather than always being 1. + pub fn from_entries(entries: &[FileEntry]) -> Self { + let mut roots: HashMap = HashMap::new(); + let mut stores: HashSet = HashSet::new(); + + for entry in entries { + let store = enclosing_store(&entry.path); + if let Some(store) = &store { + stores.insert(store.clone()); + } + + // A marker inside a dependency store marks nothing: every npm + // package ships a package.json, and there are tens of + // thousands of them under one node_modules. + if store.is_some() { + continue; + } + let Some(kind) = marker_kind(&entry.path) else { + continue; + }; + if let Some(dir) = entry.path.parent() { + roots.insert(dir.to_path_buf(), kind); + } + } + + let mut graph = Self::default(); + for (root, kind) in &roots { + let owned = stores_of(root, *kind, &stores); + let targets = if owned.is_empty() { + shared_store(root, *kind, &roots, &stores) + } else { + owned + }; + + for store in targets { + let from = graph.node(Node::ProjectRoot { + path: root.clone(), + kind: *kind, + }); + let to = graph.node(Node::DependencyStore(store)); + graph.graph.add_edge(from, to, Edge::References); + } + } + graph + } + pub fn node(&mut self, node: Node) -> NodeIndex { let key = match &node { Node::File(p) | Node::DependencyStore(p) => p.clone(), @@ -60,13 +134,206 @@ impl ImpactGraph { /// How many project roots reference this path (directly, v0)? /// This number feeds risk::downgrade — the "breaks 17 projects" number. + /// + /// Answers for the nearest enclosing node as well as for the path + /// itself, because findings are files and the thing projects reference + /// is the store above them: nothing points at + /// `proj/node_modules/react/index.js`, but three projects may well + /// point at the `proj/node_modules` it sits in. pub fn referencing_projects(&self, path: &std::path::Path) -> usize { - let Some(&ix) = self.index.get(path) else { - return 0; - }; - self.graph - .neighbors_directed(ix, petgraph::Direction::Incoming) - .filter(|&n| matches!(self.graph[n], Node::ProjectRoot { .. })) - .count() + for ancestor in path.ancestors() { + let Some(&ix) = self.index.get(ancestor) else { + continue; + }; + return self + .graph + .neighbors_directed(ix, petgraph::Direction::Incoming) + .filter(|&n| matches!(self.graph[n], Node::ProjectRoot { .. })) + .count(); + } + 0 + } +} + +/// The outermost dependency store this path sits inside, if any. +/// +/// Outermost, not nearest: `proj/node_modules/a/node_modules/b` belongs to +/// `proj/node_modules`, which is the store a project actually references. +fn enclosing_store(path: &Path) -> Option { + let mut found = None; + let mut current = path.parent(); + while let Some(dir) = current { + if dir + .file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| STORE_NAMES.contains(&n)) + { + found = Some(dir.to_path_buf()); + } + current = dir.parent(); + } + found +} + +/// Which kind of project a file marks, if it marks one. +fn marker_kind(path: &Path) -> Option { + let name = path.file_name()?.to_str()?.to_ascii_lowercase(); + PROJECTS + .iter() + .find(|(marker, _, _)| *marker == name) + .map(|(_, kind, _)| *kind) +} + +/// The stores this project owns that the scan actually saw. +fn stores_of(root: &Path, kind: ProjectKind, seen: &HashSet) -> Vec { + PROJECTS + .iter() + .filter(|(_, k, _)| *k == kind) + .flat_map(|(_, _, names)| names.iter()) + .map(|name| root.join(name)) + .filter(|store| seen.contains(store)) + .collect() +} + +/// The store an enclosing project of the same kind owns — what a workspace +/// member uses when the dependencies were hoisted above it. +fn shared_store( + root: &Path, + kind: ProjectKind, + roots: &HashMap, + seen: &HashSet, +) -> Vec { + for ancestor in root.ancestors().skip(1) { + if roots.get(ancestor) != Some(&kind) { + continue; + } + let stores = stores_of(ancestor, kind, seen); + if !stores.is_empty() { + return stores; + } + } + vec![] +} + +#[cfg(test)] +mod tests { + use super::*; + + fn entries(paths: &[&str]) -> Vec { + paths + .iter() + .map(|p| FileEntry { + path: PathBuf::from(p), + size: 1, + modified: None, + accessed: None, + is_symlink: false, + hash: None, + }) + .collect() + } + + #[test] + fn a_project_references_the_store_beside_it() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/home/u/proj/package.json", + "/home/u/proj/src/index.js", + "/home/u/proj/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/home/u/proj/node_modules/react/index.js")), + 1 + ); + // The project's own source is referenced by nothing. + assert_eq!( + graph.referencing_projects(Path::new("/home/u/proj/src/index.js")), + 0 + ); + } + + /// An abandoned store — no project alongside it — is the case this + /// whole module exists to tell apart from the one above. + #[test] + fn an_unreferenced_store_is_referenced_by_nobody() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/home/u/old-thing/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new( + "/home/u/old-thing/node_modules/react/index.js" + )), + 0 + ); + } + + /// Every npm package ships a package.json. Treating those as project + /// roots would make one node_modules look like ten thousand projects. + #[test] + fn a_package_json_inside_node_modules_is_not_a_project() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/home/u/proj/package.json", + "/home/u/proj/node_modules/react/package.json", + "/home/u/proj/node_modules/react/node_modules/loose/package.json", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/home/u/proj/node_modules/react/package.json")), + 1 + ); + } + + /// The README's headline evidence. npm workspaces hoist dependencies + /// to the repository root, so all three members really do reference + /// the one store. + #[test] + fn hoisted_workspace_members_all_reference_the_shared_store() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/package.json", + "/repo/packages/a/package.json", + "/repo/packages/b/package.json", + "/repo/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/repo/node_modules/react/index.js")), + 3 + ); + } + + /// A member with its own store uses that one, not the root's. + #[test] + fn a_member_with_its_own_store_does_not_borrow_the_roots() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/package.json", + "/repo/packages/a/package.json", + "/repo/packages/a/node_modules/x/index.js", + "/repo/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/repo/packages/a/node_modules/x/index.js")), + 1 + ); + assert_eq!( + graph.referencing_projects(Path::new("/repo/node_modules/react/index.js")), + 1 + ); + } + + #[test] + fn cargo_workspace_members_share_one_target() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/Cargo.toml", + "/repo/crates/core/Cargo.toml", + "/repo/crates/cli/Cargo.toml", + "/repo/target/debug/app", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/repo/target/debug/app")), + 3 + ); } } From 5b8b4f1534a0365e80704dd00a03740e006e842e Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:27:06 +0500 Subject: [PATCH 17/33] feat(report): put the graph stage into the pipeline it was drawn in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pipeline in the core README and in `lib.rs` reads scanner ──► index ──► dedup ──► graph ──► rules + risk ──► report and the graph stage wasn't in it. `report::build` called `rules.classify` and `risk::assess` and stopped there; `risk::downgrade` — the function that takes `referenced_by` and makes a verdict more cautious — had no callers anywhere in the workspace. The visible consequence was that a `node_modules` three live projects depend on got the same verdict, and the same reasons, as an abandoned one. "Referenced by 3 projects" is the evidence that makes Diskern different from every other disk cleaner, and it never reached the report. `build_with` now builds an `ImpactGraph` from the entries, asks it how many projects reference each one, and passes that through `risk::downgrade` before assessing risk. A referenced store picks up a `referenced by N projects` reason and drops from Review to Risky. Which makes Risky reachable for the first time, so its bytes stop counting as reclaimable: `actions::quarantine` refuses Risky and the UI renders no action for it, so counting those bytes would promise space the app will not free — the same overstatement #44 fixed for protected files. Closes #48 --- crates/diskern-core/src/report.rs | 124 ++++++++++++++++++++++++++---- 1 file changed, 109 insertions(+), 15 deletions(-) diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 58fd49a..3d3232b 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -1,7 +1,7 @@ //! Assembles scanner + dedup + rules + risk into Findings — the single //! structure both the CLI and the Tauri UI render. -use crate::{dedup, risk, rules::RulesDb, Category, FileEntry, Finding, Verdict}; +use crate::{dedup, graph, risk, rules::RulesDb, Category, FileEntry, Finding, Verdict}; use serde::{Deserialize, Serialize}; use std::collections::HashSet; use std::sync::atomic::{AtomicBool, Ordering}; @@ -63,6 +63,12 @@ pub fn build_with( .map(|d| d.as_secs() as i64) .unwrap_or(0); + // The graph stage. Project roots and the dependency stores they point + // at, worked out from the same entries the rest of the pipeline sees, + // so a `node_modules` three live projects depend on can be told apart + // from an abandoned one. + let impact = graph::ImpactGraph::from_entries(&entries); + // Classify before dedup, not after. A protected file has no business // in a duplicate set — the set is an offer to keep one copy and drop // the rest, and dropping a driver store copy is not on offer — and @@ -70,48 +76,67 @@ pub fn build_with( // // Classification is cheap per entry, but a home directory is millions // of them, so it happens once and the answer is kept. - let mut verdicts: Vec<(Category, Verdict, Option<&crate::rules::Rule>)> = - Vec::with_capacity(entries.len()); + let mut verdicts: Vec = Vec::with_capacity(entries.len()); for entry in &entries { if cancelled.load(Ordering::Relaxed) { return None; } - verdicts.push(rules.classify(&entry.path)); + let (category, base, rule) = rules.classify(&entry.path); + // Evidence can only make a verdict more cautious, never less. + let referenced_by = impact.referencing_projects(&entry.path); + let verdict = risk::downgrade(base, referenced_by); + verdicts.push(Classified { + category, + verdict, + rule, + referenced_by, + }); } let duplicate_sets = dedup::find_duplicates_filtered( &mut entries, - |i, e| e.size >= opts.dedup_min_size && verdicts[i].1 != Verdict::Protected, + |i, e| e.size >= opts.dedup_min_size && verdicts[i].verdict != Verdict::Protected, cancelled, )?; let files_scanned = entries.len() as u64; let mut findings = Vec::new(); - for (entry, (category, verdict, rule)) in entries.into_iter().zip(verdicts) { + for (entry, class) in entries.into_iter().zip(verdicts) { if cancelled.load(Ordering::Relaxed) { return None; } // Unknown + unremarkable files aren't findings; don't drown the user. - if category == Category::Unknown { + if class.category == Category::Unknown { continue; } - let assessment = risk::assess(&entry, verdict, now); - let mut reasons: Vec = rule + let assessment = risk::assess(&entry, class.verdict, now); + let mut reasons: Vec = class + .rule .map(|r| vec![format!("matched rule {}: {}", r.id, r.description)]) .unwrap_or_default(); + if class.referenced_by > 0 { + reasons.push(format!( + "referenced by {} project{}", + class.referenced_by, + if class.referenced_by == 1 { "" } else { "s" } + )); + } reasons.extend(assessment.reasons); findings.push(Finding { - reclaimable: if verdict == Verdict::Protected { - 0 - } else { - entry.size + // Bytes nothing will ever offer to move are not reclaimable. + // `actions::quarantine` refuses Risky as well as Protected, and + // the UI renders neither with an action, so counting either + // towards the headline promises space the app won't free. + reclaimable: match class.verdict { + Verdict::Protected | Verdict::Risky => 0, + Verdict::Safe | Verdict::Review => entry.size, }, entry, - category, - verdict, + category: class.category, + verdict: class.verdict, risk_score: assessment.score, reasons, }); @@ -129,6 +154,15 @@ pub fn build_with( }) } +/// What the pipeline worked out about one entry before findings are built. +struct Classified<'a> { + category: Category, + /// After [`risk::downgrade`], not the rule's base verdict. + verdict: Verdict, + rule: Option<&'a crate::rules::Rule>, + referenced_by: usize, +} + /// The two halves of the report overlap, so they can't just be added. /// /// A finding offers the file's own bytes. A duplicate set offers the @@ -336,4 +370,64 @@ mod tests { assert!(report.duplicate_sets.is_empty()); assert_eq!(report.total_reclaimable, 0); } + + /// Issue #48. The graph stage was in the pipeline diagram and not in + /// the pipeline: `risk::downgrade` had no callers anywhere, so a + /// `node_modules` three live projects depend on got the same verdict + /// and the same reasons as an abandoned one. + #[test] + fn a_referenced_store_is_more_cautious_than_an_abandoned_one() { + let dir = tempfile::tempdir().unwrap(); + let live = dir.path().join("live"); + let dead = dir.path().join("dead"); + std::fs::create_dir_all(live.join("node_modules/react")).unwrap(); + std::fs::create_dir_all(dead.join("node_modules/react")).unwrap(); + std::fs::write(live.join("package.json"), b"{}").unwrap(); + std::fs::write(live.join("node_modules/react/index.js"), b"live").unwrap(); + std::fs::write(dead.join("node_modules/react/index.js"), b"dead").unwrap(); + + let rules = RulesDb::new( + 1, + vec![crate::rules::Rule { + id: "test-node-modules".into(), + patterns: vec!["**/node_modules/**".into()], + category: Category::BuildArtifact, + verdict: Verdict::Review, + description: "test".into(), + }], + ); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &rules, + &ReportOptions::default(), + &never, + ) + .unwrap(); + + let find = |needle: &str| { + report + .findings + .iter() + .find(|f| f.entry.path.to_string_lossy().contains(needle)) + .unwrap() + .clone() + }; + + let referenced = find("live/node_modules"); + assert_eq!(referenced.verdict, Verdict::Risky); + assert!(referenced + .reasons + .iter() + .any(|r| r == "referenced by 1 project")); + // Nothing offers to move a risky file, so its bytes are not on + // offer either. + assert_eq!(referenced.reclaimable, 0); + + let abandoned = find("dead/node_modules"); + assert_eq!(abandoned.verdict, Verdict::Review); + assert!(!abandoned.reasons.iter().any(|r| r.starts_with("referenced by"))); + assert_eq!(abandoned.reclaimable, 4); + } } From 768189322319c9e1538972ba3dacbd4a5886c811 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:31:58 +0500 Subject: [PATCH 18/33] chore(audit): document and ignore the advisories Tauri owns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo audit` reports seventeen advisories against this lockfile and zero of them are vulnerabilities: sixteen unmaintained crates and one unsound one, all reaching the tree through `tauri`. They are one dependency wearing seventeen hats. Tauri v2 renders on Linux through webkit2gtk-4.1, which links the gtk-rs GTK3 bindings — archived upstream in 2024. That is ten of them, plus `glib`'s unsound `VariantStrIter` (fixed in the 0.19 line the GTK3 bindings never moved to), plus `proc-macro-error` via `glib-macros`. The five `unic-*` crates arrive the same way, through `urlpattern` in `tauri-utils`. Verified with `cargo tree -i`; none has a version that clears it, so neither the audit job nor the auto-fix PR has anywhere to go. Which is the actual risk. The audit job fails while advisories stand, so it has been red every Monday since it was written and will stay red until Tauri moves to GTK4 — and a check that is always red stops being read. The eighteenth advisory, in a crate we chose, would land in a run nobody looks at. So they are listed in `.cargo/audit.toml` with the reason for each and a review date. `ignore` suppresses exactly those ids: a new advisory against these same crates still fails, as does anything anywhere else. The ids go into the job summary on every run so a suppression cannot outlive its reason quietly, and editing the file re-runs the audit the same way moving the lockfile does. `rustsec/audit-check`, which runs on pull requests, does not read `.cargo/audit.toml` — it only takes an `ignore` input — so the workflow greps the ids out of the file rather than keeping a second list. Closes #23 Closes #24 Closes #25 Closes #26 Closes #27 Closes #28 Closes #29 Closes #30 Closes #31 Closes #32 Closes #33 Closes #34 Closes #35 Closes #36 Closes #37 Closes #38 Closes #39 Closes #55 --- .cargo/audit.toml | 58 +++++++++++++++++++++++++++++++++++ .github/workflows/audit.yml | 33 +++++++++++++++++++- crates/diskern-core/README.md | 4 +-- docs/DEPENDENCY-AUTOMATION.md | 47 ++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 .cargo/audit.toml diff --git a/.cargo/audit.toml b/.cargo/audit.toml new file mode 100644 index 0000000..96ba478 --- /dev/null +++ b/.cargo/audit.toml @@ -0,0 +1,58 @@ +# cargo-audit configuration. +# +# Everything listed below is an *informational* advisory (unmaintained or +# unsound), not a vulnerability. `cargo audit` reports zero vulnerabilities +# in this lockfile and this file does not change that: it only suppresses +# the ids named here, so a real advisory — including a new one against any +# of these same crates — still fails the run. +# +# Every id here reaches the lockfile through `tauri`, is a Linux-desktop +# dependency, and has no version that clears it. Diskern cannot fix them by +# updating; Tauri has to move off GTK3 first. Keeping them permanently red +# would train everyone to ignore a red audit, which is a worse security +# outcome than an explicit, reviewed list. +# +# The reasoning, the review cadence, and how to check whether an entry can +# be dropped are in docs/DEPENDENCY-AUTOMATION.md. +# +# Reviewed: 2026-09-05. Re-check when Tauri ships a GTK4/webkit6 runtime. + +[advisories] +ignore = [ + # --------------------------------------------------------------- + # gtk-rs GTK3 bindings — archived upstream, no successor at 0.18. + # tauri 2.11 -> gtk 0.18 / webkit2gtk / tao / muda. The successor + # (gtk4-rs) is not what Tauri v2 links against on Linux. + # --------------------------------------------------------------- + "RUSTSEC-2024-0413", # atk + "RUSTSEC-2024-0416", # atk-sys + "RUSTSEC-2024-0412", # gdk + "RUSTSEC-2024-0418", # gdk-sys + "RUSTSEC-2024-0411", # gdkwayland-sys + "RUSTSEC-2024-0417", # gdkx11 + "RUSTSEC-2024-0414", # gdkx11-sys + "RUSTSEC-2024-0415", # gtk + "RUSTSEC-2024-0420", # gtk-sys + "RUSTSEC-2024-0419", # gtk3-macros + + # glib 0.18.5 — unsound VariantStrIter. The fix landed in the 0.19 + # line, which the archived GTK3 bindings above never moved to. The + # unsound function is glib's own GVariant iteration; nothing in + # Diskern calls it, and the GTK3 stack that does is the one Tauri + # drives. + "RUSTSEC-2024-0429", + + # proc-macro-error 1.0.4 — unmaintained, pulled in by glib-macros + # 0.18.5, so it leaves with the GTK3 bindings above. Build-time + # proc macro: it does not ship in the binary. + "RUSTSEC-2024-0370", + + # unic-* 0.9.0 — unmaintained, all five via urlpattern 0.3 -> + # tauri-utils. Pure Unicode tables, no network or filesystem + # surface, and replacing urlpattern is Tauri's call not ours. + "RUSTSEC-2025-0081", # unic-char-property + "RUSTSEC-2025-0075", # unic-char-range + "RUSTSEC-2025-0080", # unic-common + "RUSTSEC-2025-0100", # unic-ucd-ident + "RUSTSEC-2025-0098", # unic-ucd-version +] diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml index 8707290..cf2b898 100644 --- a/.github/workflows/audit.yml +++ b/.github/workflows/audit.yml @@ -12,7 +12,9 @@ on: push: branches: [main, master] pull_request: - paths: [Cargo.lock] + # The ignore list is part of what the audit means, so editing it has to + # re-run the audit just as moving the lockfile does. + paths: [Cargo.lock, .cargo/audit.toml] jobs: # PR-time advisory check: annotates the changed lockfile inline so the @@ -26,9 +28,23 @@ jobs: checks: write # the action reports advisories as a check run steps: - uses: actions/checkout@v7 + + # `rustsec/audit-check` does not read .cargo/audit.toml — it only + # takes an `ignore` input — while the plain `cargo audit` in the job + # below does. Reading the ids out of the file keeps one source of + # truth instead of a second list that drifts from the first. + - name: Read the ignored advisories + id: ignored + run: | + ids=$(sed -n '/ignore *= *\[/,/^]/p' .cargo/audit.toml \ + | grep -oE 'RUSTSEC-[0-9]{4}-[0-9]{4}' | paste -sd,) + echo "ids=$ids" >> "$GITHUB_OUTPUT" + echo "Ignoring: ${ids:-nothing}" + - uses: rustsec/audit-check@v2 with: token: ${{ secrets.GITHUB_TOKEN }} + ignore: ${{ steps.ignored.outputs.ids }} # Off-PR runs own a single tracking issue for the whole lockfile: one # issue, kept current, rather than a fresh one every Monday. @@ -57,6 +73,21 @@ jobs: gh label create automated-issue --force \ --color C5DEF5 --description "Filed by a workflow, not a human" + # Visible, not silent: an ignore list nobody ever reads is how a + # suppressed advisory outlives the reason it was suppressed. + - name: Report the ignored advisories + run: | + { + echo "### Ignored advisories" + echo + sed -n '/ignore *= *\[/,/^]/p' .cargo/audit.toml \ + | grep -oE 'RUSTSEC-[0-9]{4}-[0-9]{4}' | sed 's/^/- /' + echo + echo "Rationale and review cadence: docs/DEPENDENCY-AUTOMATION.md" + } >> "$GITHUB_STEP_SUMMARY" + + # Reads .cargo/audit.toml from the working directory, so the ids + # above are already excluded from these counts. - name: Run cargo audit id: audit run: | diff --git a/crates/diskern-core/README.md b/crates/diskern-core/README.md index a6f4a1f..f6ff8b9 100644 --- a/crates/diskern-core/README.md +++ b/crates/diskern-core/README.md @@ -26,11 +26,11 @@ scanner ──► index ──► dedup ──► graph ──► rules + risk | ---------------------------- | ------------------------------------------------------ | | [`scanner`](src/scanner.rs) | Parallel filesystem walk (jwalk) with live progress | | [`dedup`](src/dedup.rs) | Duplicate detection — BLAKE3, size-collision gated | -| [`graph`](src/graph.rs) | Reference graph (what depends on what) | +| [`graph`](src/graph.rs) | Reference graph — which projects depend on which stores | | [`rules`](src/rules.rs) | Deterministic rules DB ([`rules/base.json`](rules/base.json)) | | [`risk`](src/risk.rs) | Turns rule matches + evidence into a verdict | | [`report`](src/report.rs) | Aggregates findings into a serializable report | -| [`actions`](src/actions.rs) | Quarantine (move-to-review-folder) — the only mutator | +| [`actions`](src/actions.rs) | Quarantine + manifest (move, restore, purge) — the only mutator | | [`ai`](src/ai.rs) | Optional narration layer (`--features ai`) | ## Develop diff --git a/docs/DEPENDENCY-AUTOMATION.md b/docs/DEPENDENCY-AUTOMATION.md index f54ed6a..03260b1 100644 --- a/docs/DEPENDENCY-AUTOMATION.md +++ b/docs/DEPENDENCY-AUTOMATION.md @@ -35,6 +35,53 @@ The job then fails, which is what auto-fix watches for. Both labels are created by the workflow if the repository doesn't have them yet, so this needs no manual setup. +## The ignore list + +Some advisories cannot be fixed here. They arrive through `tauri`, and +clearing them needs a release Tauri has not made yet. Those are listed, +with a reason each, in [`.cargo/audit.toml`](../.cargo/audit.toml). + +The whole list today is one dependency: Tauri v2 renders on Linux through +webkit2gtk-4.1, which links the **gtk-rs GTK3 bindings**. Those bindings +were archived upstream in 2024 — ten crates, plus `glib`'s unsound +`VariantStrIter` (fixed in the 0.19 line the GTK3 bindings never moved +to), plus `proc-macro-error` via `glib-macros`. The five `unic-*` crates +come the same way, via `urlpattern` in `tauri-utils`. Confirm any of it +with: + +``` +cargo tree -i gtk --target x86_64-unknown-linux-gnu +``` + +None of them is a *vulnerability*. `cargo audit` reports zero of those +against this lockfile; all seventeen are informational — sixteen +unmaintained, one unsound. + +**Why ignore rather than leave them red.** The audit job fails while +advisories stand, and auto-fix opens a PR when a version clears one. +Neither has anywhere to go here: there is no newer version, so the audit +stays red forever and the auto-fix PR is empty every week. A check that +is always red stops being read, and then the eighteenth advisory — the +one that *is* a vulnerability, in a crate we chose — arrives into a run +nobody looks at. The list buys back a green baseline. + +**What it does not do.** `ignore` suppresses exactly the ids named. A new +advisory against any of these same crates still fails the run, as does +any advisory anywhere else in the tree. The ids are also printed into the +job summary on every run, so a suppression cannot quietly outlive its +reason. + +**Reviewing it.** Re-check when Tauri ships a GTK4/webkit6 Linux runtime, +and otherwise every few months. To test whether an entry can go, delete +the line and run `cargo audit`: if it stays quiet, the advisory is gone +and so should the line be. + +Two places consume the list. `cargo audit` reads `.cargo/audit.toml` +itself. `rustsec/audit-check`, which runs on pull requests, does not — it +only takes an `ignore` input — so the workflow greps the ids out of the +file and passes them in, rather than keeping a second list to drift from +the first. + ## Auto-fix Runs `cargo audit fix` (raises a version requirement when clearing the From 2601ce8ab1b9fd38c1da5132a8b13583e9fba3e0 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:32:32 +0500 Subject: [PATCH 19/33] docs(changelog): record the issue sweep Grouped the way the changes actually land: new capability (quarantine manifest, graph evidence), changed behaviour that a user or a caller would notice (glob rules, exclude matching, the moved dedup knob, the honest reclaimable total, the audit ignore list), and plain fixes. --- CHANGELOG.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44ed707..8f97f44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,10 +20,39 @@ All notable changes to Diskern are documented here. The format follows dependency-update PR when an advisory lands - Community docs: contributing guide, code of conduct, security policy - Per-section READMEs and an architecture overview +- Quarantine keeps a manifest, so a quarantined file can be restored + after the app is closed. The desktop app gained a Quarantine panel with + per-file restore and an explicit purge +- The impact graph reaches the report: a dependency store a live project + depends on is now marked more cautiously than an abandoned one, with + "referenced by N projects" as the evidence ### Changed - Licensed under MIT +- Rule patterns are globs rather than substrings, so a rule stays inside + the directory it names. The Firefox rule covers `cache2` rather than + the whole profile, and `/tmp` no longer matches `~/tmp` +- Excludes are matched on whole path components after normalization, so + `/run` stops excluding `/runtime-data` and a differently-cased Windows + root still matches +- `min_file_size` moved from `ScanOptions` to `ReportOptions` as + `dedup_min_size`: it is a dedup knob and was hiding small files from + the whole report +- The reclaimable headline no longer counts the same bytes twice, and no + longer counts bytes on files the app refuses to act on +- `cargo audit` ignores seventeen unfixable transitive advisories from + Tauri's Linux GTK3 stack, listed with a reason each in + `.cargo/audit.toml` + +### Fixed + +- Restoring a quarantined file across filesystems no longer fails with + `EXDEV` +- The scan progress ticker thread stops from a guard, so a panicking or + cancelled scan can't leave it emitting for the rest of the process +- Two files that flatten to the same quarantine name no longer overwrite + each other ## [0.1.0] From 79d6df4318a9e7eaad83e4ae45afed44fe1dcc7e Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:33:03 +0500 Subject: [PATCH 20/33] style: apply rustfmt to the issue sweep --- app/src-tauri/src/commands.rs | 6 +----- crates/diskern-core/src/actions.rs | 5 ++--- crates/diskern-core/src/dedup.rs | 6 +++++- crates/diskern-core/src/graph.rs | 9 +++------ crates/diskern-core/src/report.rs | 10 ++++++++-- crates/diskern-core/src/rules.rs | 10 ++++++++-- crates/diskern-core/src/scanner.rs | 15 ++++++++++++--- 7 files changed, 39 insertions(+), 22 deletions(-) diff --git a/app/src-tauri/src/commands.rs b/app/src-tauri/src/commands.rs index dd78afa..482e8d9 100644 --- a/app/src-tauri/src/commands.rs +++ b/app/src-tauri/src/commands.rs @@ -51,11 +51,7 @@ struct ScanRun<'a> { } impl<'a> ScanRun<'a> { - fn start( - window: &Window, - state: &'a ActiveScan, - progress: Arc, - ) -> Self { + fn start(window: &Window, state: &'a ActiveScan, progress: Arc) -> Self { let stop = Arc::new(std::sync::atomic::AtomicBool::new(false)); // A plain OS thread keeps this independent of whatever async diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index d9b8559..334a292 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -227,9 +227,8 @@ pub fn manifest_path(quarantine_dir: &Path) -> PathBuf { fn append_to_manifest(quarantine_dir: &Path, record: &QuarantineRecord) -> Result<()> { let path = manifest_path(quarantine_dir); - let line = serde_json::to_string(record).map_err(|e| { - GenomeError::Rules(format!("could not serialize a quarantine record: {e}")) - })?; + let line = serde_json::to_string(record) + .map_err(|e| GenomeError::Rules(format!("could not serialize a quarantine record: {e}")))?; let mut file = std::fs::OpenOptions::new() .create(true) diff --git a/crates/diskern-core/src/dedup.rs b/crates/diskern-core/src/dedup.rs index 98c8426..384f672 100644 --- a/crates/diskern-core/src/dedup.rs +++ b/crates/diskern-core/src/dedup.rs @@ -177,7 +177,11 @@ mod tests { let never = AtomicBool::new(false); let sets = find_duplicates_filtered( &mut entries, - |_, e| e.path.file_name().is_some_and(|n| n.to_string_lossy().starts_with("keep")), + |_, e| { + e.path + .file_name() + .is_some_and(|n| n.to_string_lossy().starts_with("keep")) + }, &never, ) .unwrap(); diff --git a/crates/diskern-core/src/graph.rs b/crates/diskern-core/src/graph.rs index f569e6a..1753e24 100644 --- a/crates/diskern-core/src/graph.rs +++ b/crates/diskern-core/src/graph.rs @@ -256,14 +256,11 @@ mod tests { /// whole module exists to tell apart from the one above. #[test] fn an_unreferenced_store_is_referenced_by_nobody() { - let graph = ImpactGraph::from_entries(&entries(&[ - "/home/u/old-thing/node_modules/react/index.js", - ])); + let graph = + ImpactGraph::from_entries(&entries(&["/home/u/old-thing/node_modules/react/index.js"])); assert_eq!( - graph.referencing_projects(Path::new( - "/home/u/old-thing/node_modules/react/index.js" - )), + graph.referencing_projects(Path::new("/home/u/old-thing/node_modules/react/index.js")), 0 ); } diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 3d3232b..fea69d3 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -366,7 +366,10 @@ mod tests { .unwrap(); assert_eq!(report.findings.len(), 2); - assert!(report.findings.iter().all(|f| f.verdict == Verdict::Protected)); + assert!(report + .findings + .iter() + .all(|f| f.verdict == Verdict::Protected)); assert!(report.duplicate_sets.is_empty()); assert_eq!(report.total_reclaimable, 0); } @@ -427,7 +430,10 @@ mod tests { let abandoned = find("dead/node_modules"); assert_eq!(abandoned.verdict, Verdict::Review); - assert!(!abandoned.reasons.iter().any(|r| r.starts_with("referenced by"))); + assert!(!abandoned + .reasons + .iter() + .any(|r| r.starts_with("referenced by"))); assert_eq!(abandoned.reclaimable, 4); } } diff --git a/crates/diskern-core/src/rules.rs b/crates/diskern-core/src/rules.rs index ccac0c8..2cc81dc 100644 --- a/crates/diskern-core/src/rules.rs +++ b/crates/diskern-core/src/rules.rs @@ -250,9 +250,15 @@ mod tests { "C:\\Users\\x\\AppData\\Local\\Temp\\a.tmp", Category::TempFile, ), - ("/home/u/proj/node_modules/react/index.js", Category::BuildArtifact), + ( + "/home/u/proj/node_modules/react/index.js", + Category::BuildArtifact, + ), ("/home/u/proj/target/debug/app", Category::BuildArtifact), - ("/home/u/.cache/pip/wheels/a.whl", Category::PackageManagerCache), + ( + "/home/u/.cache/pip/wheels/a.whl", + Category::PackageManagerCache, + ), ] { let (cat, _, _) = db.classify(std::path::Path::new(path)); assert_eq!(cat, expected, "{path}"); diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 7271b06..4a04234 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -175,9 +175,18 @@ mod tests { #[test] fn excludes_survive_a_differently_cased_or_separated_root() { let excludes = [normalize_exclude("C:\\Windows\\WinSxS")]; - assert!(is_excluded(Path::new("c:/windows/winsxs/component/x.dll"), &excludes)); - assert!(is_excluded(Path::new("C:\\Windows\\WinSxS\\x.dll"), &excludes)); - assert!(!is_excluded(Path::new("c:/windows/winsxs-backup/x.dll"), &excludes)); + assert!(is_excluded( + Path::new("c:/windows/winsxs/component/x.dll"), + &excludes + )); + assert!(is_excluded( + Path::new("C:\\Windows\\WinSxS\\x.dll"), + &excludes + )); + assert!(!is_excluded( + Path::new("c:/windows/winsxs-backup/x.dll"), + &excludes + )); } #[test] From 1893d9c3ca3f0a98767fb456ab8d726b17bf5a9b Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:46:46 +0500 Subject: [PATCH 21/33] feat(cli): print every reason, not just the matched rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule reason says what a file is. The reasons after it say why this copy of it got the verdict it did — and "referenced by 3 projects" is exactly the line that explains why one `node_modules` is risky and the one next to it is not. Printing only the first reason meant the evidence the graph stage now produces reached the report and stopped there, which is most of the way to not having produced it. --- crates/diskern-cli/src/main.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/diskern-cli/src/main.rs b/crates/diskern-cli/src/main.rs index f154a3c..b312433 100644 --- a/crates/diskern-cli/src/main.rs +++ b/crates/diskern-cli/src/main.rs @@ -167,9 +167,12 @@ fn print_findings(findings: &[&Finding], top: usize) { human_bytes(f.reclaimable), f.entry.path.display() ); - // The first reason is the matched rule; the rest come from - // the risk module and repeat across a whole category. - if let Some(reason) = f.reasons.first() { + // Every reason, not just the matched rule. The rule says + // what the file is; the rest say why this copy of it got + // the verdict it did — "referenced by 3 projects" is the + // line that explains a risky node_modules, and printing + // only the first hid exactly that. + for reason in &f.reasons { println!(" {:>9} {reason}", ""); } } From 5076a8c149cad80a0b78d19a3d94420f7869820e Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:46:53 +0500 Subject: [PATCH 22/33] feat(app): show every reason on a finding row Same gap as the CLI: the row rendered `reasons[0]`, which is always the matched rule, so the reference evidence behind a risky verdict never appeared next to the row it explains. --- app/src/App.jsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/App.jsx b/app/src/App.jsx index 3691aa3..bf4522f 100644 --- a/app/src/App.jsx +++ b/app/src/App.jsx @@ -92,7 +92,10 @@ function FindingRow({ f, quarantineDir, onQuarantined }) {
  • {f.entry.path} {(f.entry.size / 1e6).toFixed(1)} MB - {f.reasons[0]} + {/* Every reason, not just the matched rule: "referenced by 3 + projects" is what explains a risky row, and it is never the + first one. */} + {f.reasons.join(" · ")} {canQuarantine && ( From 62b999da90d281cec87440402f95fdaca5864ec0 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 12:51:21 +0500 Subject: [PATCH 23/33] test(actions): tear the manifest fixture somewhere the spellchecker allows The torn line cut off at `tru`, which `typos` reads as a misspelt `true` rather than as the half-written JSON it is. Cutting the line mid-path instead keeps it just as invalid and just as realistic. --- crates/diskern-core/src/actions.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 334a292..43ba7db 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -428,7 +428,12 @@ mod tests { let manifest = manifest_path(&q); let good = std::fs::read_to_string(&manifest).unwrap(); - std::fs::write(&manifest, format!("{{\"original\": tru\n{good}")).unwrap(); + // A line cut off mid-value, the way an interrupted append leaves it. + std::fs::write( + &manifest, + format!("{{\"original\": \"/home/u/half-writ\n{good}"), + ) + .unwrap(); assert_eq!(list(&q).unwrap().len(), 1); } From 4446b2f85203b4cd8687af1a4093ca38d9c36698 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:11:48 +0500 Subject: [PATCH 24/33] fix(actions): record the quarantine before moving the file, not after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `quarantine` moved the file and then wrote the manifest line. Serde refuses a `PathBuf` that isn't valid UTF-8, and Linux and macOS both allow filenames that aren't — a Latin-1 name out of an old archive is enough, and `rules::normalize` uses `to_string_lossy`, so such a file classifies normally and reaches the user as an actionable row. Quarantining one moved it, failed to encode it, and returned an error. The file was then gone from its original location, absent from the manifest, and sitting in quarantine under a flattened name that cannot be read back to an original — while the UI, seeing the error, told the user nothing had happened. Unrecoverable except by hand, and caused by the exact operation whose promise is that it is reversible. The line is encoded first now, so a record that cannot be written stops the move instead of following it. If the append itself fails after a successful move, the move is rolled back rather than left standing. --- crates/diskern-core/src/actions.rs | 80 +++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 17 deletions(-) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 43ba7db..66b381d 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -55,20 +55,31 @@ pub fn quarantine( std::fs::create_dir_all(quarantine_dir).map_err(|e| io_err(quarantine_dir, e))?; let stamp = now_epoch(); - let dest = unique_dest(quarantine_dir, stamp, file); - - move_file(file, &dest)?; - let record = QuarantineRecord { original: file.to_path_buf(), - quarantined_to: dest, + quarantined_to: unique_dest(quarantine_dir, stamp, file), at_epoch: stamp, }; - // Record before returning. A caller that drops the returned value — - // which is exactly what the app's `quarantine_finding` used to do — - // must not be able to lose the only note of where the file came from. - append_to_manifest(quarantine_dir, &record)?; + // Encode before moving anything. `serde` refuses a `PathBuf` that + // isn't valid UTF-8, and Linux and macOS both allow filenames that + // aren't — a Latin-1 name out of an old archive is enough. Moving + // first and discovering that afterwards left the file in quarantine + // with no record of where it came from, under a flattened name that + // cannot be read back: the exact loss this manifest exists to stop. + let line = encode(&record)?; + + move_file(file, &record.quarantined_to)?; + + // The move happened; the record must follow it or the move must not + // stand. Anything else strands the file. + if let Err(e) = append_line(quarantine_dir, &line) { + // Best effort, and the only sensible order: the original was + // sitting here a moment ago, so putting it back is the outcome + // closest to nothing having happened. + let _ = move_file(&record.quarantined_to, &record.original); + return Err(e); + } Ok(record) } @@ -225,11 +236,19 @@ pub fn manifest_path(quarantine_dir: &Path) -> PathBuf { quarantine_dir.join(MANIFEST_NAME) } -fn append_to_manifest(quarantine_dir: &Path, record: &QuarantineRecord) -> Result<()> { - let path = manifest_path(quarantine_dir); - let line = serde_json::to_string(record) - .map_err(|e| GenomeError::Rules(format!("could not serialize a quarantine record: {e}")))?; +/// One manifest line. Fails on a path that is not valid UTF-8, which is +/// why every caller encodes before it moves anything. +fn encode(record: &QuarantineRecord) -> Result { + serde_json::to_string(record).map_err(|e| { + GenomeError::Rules(format!( + "cannot record {} in the quarantine manifest, so it will not be moved: {e}", + record.original.display() + )) + }) +} +fn append_line(quarantine_dir: &Path, line: &str) -> Result<()> { + let path = manifest_path(quarantine_dir); let mut file = std::fs::OpenOptions::new() .create(true) .append(true) @@ -248,10 +267,7 @@ fn write_manifest(quarantine_dir: &Path, records: &[QuarantineRecord]) -> Result let mut body = String::new(); for record in records { - let line = serde_json::to_string(record).map_err(|e| { - GenomeError::Rules(format!("could not serialize a quarantine record: {e}")) - })?; - body.push_str(&line); + body.push_str(&encode(record)?); body.push('\n'); } @@ -467,4 +483,34 @@ mod tests { std::fs::create_dir_all(&q).unwrap(); assert!(restore_from_manifest(&q, &q.join("invented")).is_err()); } + + /// A path serde cannot encode must stop the move, not follow it. + /// + /// Linux and macOS both allow filenames that aren't valid UTF-8, and a + /// disk scanner meets them. Encoding after the move left the file in + /// quarantine, absent from the manifest, under a flattened name with + /// no way back — while the caller was told the operation failed. + #[cfg(unix)] + #[test] + fn a_path_that_cannot_be_recorded_is_not_moved() { + use std::os::unix::ffi::OsStrExt; + + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + // "café.dmg" in Latin-1: a valid filename, invalid UTF-8. + let victim = dir.path().join(std::ffi::OsStr::from_bytes(b"caf\xe9.dmg")); + std::fs::write(&victim, b"payload").unwrap(); + + let err = quarantine(&victim, Verdict::Review, &q).unwrap_err(); + assert!( + err.to_string().contains("will not be moved"), + "unexpected error: {err}" + ); + + // The file is exactly where it was, and quarantine is untouched. + assert_eq!(std::fs::read(&victim).unwrap(), b"payload"); + assert!(list(&q).unwrap().is_empty()); + let strays = std::fs::read_dir(&q).map(|d| d.count()).unwrap_or(0); + assert_eq!(strays, 0, "quarantine should hold no orphan"); + } } From f953f27a7edf0d7bd5f384f1c349407e184f399c Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:12:23 +0500 Subject: [PATCH 25/33] fix(actions): refuse to restore over a file that is already there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both halves of `move_file` replace an existing destination without asking, so restoring put the quarantined copy over whatever now sat at the original path — silently, and with no way back. That is not a corner case. The files Diskern marks `safe` are the ones applications rebuild: quarantine a browser cache, keep browsing, then change your mind, and the undo destroys the cache the browser has since written. `restore` had no caller outside its own tests before this branch, so the Quarantine panel is what makes it reachable — in the one module whose stated rule is that nothing is ever destroyed. An occupied destination is now an error the caller shows, naming the file and what to do about it, rather than a decision this function makes on the user's behalf. Checked with `symlink_metadata`, so a broken symlink in the way counts as in the way. --- crates/diskern-core/src/actions.rs | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 66b381d..3a48100 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -114,9 +114,29 @@ fn unique_dest(quarantine_dir: &Path, stamp: i64, file: &Path) -> PathBuf { /// Restore a quarantined file to its original location. /// +/// Refuses when something is already there. Both halves of `move_file` +/// replace an existing destination without asking, and the files most +/// likely to be quarantined are the ones most likely to come back: a +/// browser cache is `safe` precisely because the browser rebuilds it, so +/// "quarantine the cache, keep browsing, change your mind" ends with the +/// undo destroying the newer file. Overwriting a file the user did not +/// name is exactly what this module promises never to do, so an occupied +/// destination is an error the caller shows rather than a decision this +/// function makes. +/// /// Leaves the manifest alone — see [`restore_from_manifest`] for the /// version that also stops listing the file as quarantined. pub fn restore(record: &QuarantineRecord) -> Result<()> { + // symlink_metadata, not exists(): a broken symlink is something in + // the way too, and exists() follows the link and reports false. + if std::fs::symlink_metadata(&record.original).is_ok() { + return Err(GenomeError::Rules(format!( + "{} already exists; move or remove it and restore again — \ + refusing to overwrite it with the quarantined copy", + record.original.display() + ))); + } + if let Some(parent) = record.original.parent() { std::fs::create_dir_all(parent).map_err(|e| io_err(parent, e))?; } @@ -513,4 +533,33 @@ mod tests { let strays = std::fs::read_dir(&q).map(|d| d.count()).unwrap_or(0); assert_eq!(strays, 0, "quarantine should hold no orphan"); } + + /// Restoring must not destroy a file that came back on its own. + /// + /// The caches Diskern calls `safe` are the ones applications rebuild, + /// so this is the ordinary sequence, not a contrived one: quarantine + /// the cache, carry on using the app, then change your mind. + #[test] + fn restore_refuses_to_overwrite_something_already_there() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let original = dir.path().join("cache.dat"); + std::fs::write(&original, b"old-copy").unwrap(); + + let rec = quarantine(&original, Verdict::Safe, &q).unwrap(); + std::fs::write(&original, b"regenerated-by-the-app").unwrap(); + + let err = restore(&rec).unwrap_err(); + assert!(err.to_string().contains("already exists"), "{err}"); + + // Neither copy was destroyed: the new one is still in place and + // the quarantined one is still restorable once it is moved aside. + assert_eq!(std::fs::read(&original).unwrap(), b"regenerated-by-the-app"); + assert_eq!(std::fs::read(&rec.quarantined_to).unwrap(), b"old-copy"); + assert_eq!(list(&q).unwrap().len(), 1); + + std::fs::remove_file(&original).unwrap(); + restore(&rec).unwrap(); + assert_eq!(std::fs::read(&original).unwrap(), b"old-copy"); + } } From 1dbd9d008ff17a9f4bc71dcb1f2272ca57ab33c2 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:14:01 +0500 Subject: [PATCH 26/33] fix(actions): serialize manifest access so a concurrent write isn't lost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `restore_from_manifest` and `purge` are read-modify-write: they read the whole manifest, act, then rewrite it from what they read. A record appended in between was erased by that rewrite, and the file it named stayed in quarantine with nothing recording where it belonged — invisible to `list`, unreachable by `restore`, and not even removed by a later `purge`. The app makes that reachable. `quarantine_finding`, `restore_quarantined` and `purge_quarantine` each run on their own blocking task, and nothing in the UI stops a user quarantining a finding row while another row's restore is still in flight. Every manifest access now goes through one process-wide lock, held across the read, the action and the rewrite. `list` takes it; the internal `read_manifest` doesn't, because `Mutex` isn't reentrant and the operations that rewrite already hold it. The test drives two threads quarantining against a third restoring and asserts the invariant that matters: nothing sits in the quarantine directory without a manifest line naming it. It fails on every run with the lock neutered. --- crates/diskern-core/src/actions.rs | 109 ++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 2 deletions(-) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 3a48100..2c33156 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -18,6 +18,7 @@ use crate::{GenomeError, Result, Verdict}; use serde::{Deserialize, Serialize}; use std::io::Write; use std::path::{Path, PathBuf}; +use std::sync::{Mutex, MutexGuard}; /// Quarantine's record of itself, inside the quarantine directory. pub const MANIFEST_NAME: &str = "manifest.jsonl"; @@ -28,6 +29,35 @@ pub const MANIFEST_NAME: &str = "manifest.jsonl"; /// distinguishes two files. const MAX_FLAT_LEN: usize = 180; +/// Serializes every manifest access in this process. +/// +/// `restore_from_manifest` and `purge` are read-modify-write: they read +/// the whole manifest and later rewrite it from what they read. A record +/// appended in between was erased by that rewrite, leaving the file in +/// quarantine with nothing recording where it belongs — invisible to +/// `list`, unrestorable, and not even removed by a later `purge`. +/// +/// The app makes that reachable: `quarantine_finding`, `restore_quarantined` +/// and `purge_quarantine` each run on their own blocking task, and nothing +/// in the UI stops a user quarantining one row while another row's restore +/// is still in flight. +/// +/// One lock for the process rather than one per directory: these +/// operations are rare, short, and a second quarantine directory in one +/// process isn't a thing that happens. +static MANIFEST: Mutex<()> = Mutex::new(()); + +/// A poisoned lock means an earlier holder panicked mid-operation. The +/// manifest on disk is either the old one or the new one — `write_manifest` +/// renames a complete file into place — so there is no torn state to +/// protect, and refusing to unlock would break quarantine for the rest of +/// the session. +fn manifest_lock() -> MutexGuard<'static, ()> { + MANIFEST + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct QuarantineRecord { pub original: PathBuf, @@ -73,6 +103,7 @@ pub fn quarantine( // The move happened; the record must follow it or the move must not // stand. Anything else strands the file. + let _guard = manifest_lock(); if let Err(e) = append_line(quarantine_dir, &line) { // Best effort, and the only sensible order: the original was // sitting here a moment ago, so putting it back is the outcome @@ -148,6 +179,13 @@ pub fn restore(record: &QuarantineRecord) -> Result<()> { /// A quarantine directory with no manifest is empty, not broken: the app /// resolves the path before anything has been quarantined into it. pub fn list(quarantine_dir: &Path) -> Result> { + let _guard = manifest_lock(); + read_manifest(quarantine_dir) +} + +/// [`list`] without taking the lock — for callers that already hold it. +/// `Mutex` is not reentrant, so they must not go through `list`. +fn read_manifest(quarantine_dir: &Path) -> Result> { let path = manifest_path(quarantine_dir); let contents = match std::fs::read_to_string(&path) { Ok(c) => c, @@ -179,7 +217,10 @@ pub fn restore_from_manifest( quarantine_dir: &Path, quarantined_to: &Path, ) -> Result { - let records = list(quarantine_dir)?; + // Held across the read, the move and the rewrite: anything appended + // between the read and the rewrite would be erased by it. + let _guard = manifest_lock(); + let records = read_manifest(quarantine_dir)?; let record = records .iter() .find(|r| r.quarantined_to == quarantined_to) @@ -219,7 +260,8 @@ pub struct PurgeSummary { /// it deletes only the files the manifest says this crate moved here — /// never whatever else happens to be sitting in the directory. pub fn purge(quarantine_dir: &Path) -> Result { - let records = list(quarantine_dir)?; + let _guard = manifest_lock(); + let records = read_manifest(quarantine_dir)?; let mut summary = PurgeSummary::default(); let mut kept = Vec::new(); @@ -562,4 +604,67 @@ mod tests { restore(&rec).unwrap(); assert_eq!(std::fs::read(&original).unwrap(), b"old-copy"); } + + /// Nothing may end up in the quarantine directory without a manifest + /// line naming it. + /// + /// `restore_from_manifest` and `purge` rewrite the manifest from what + /// they read, so a record appended in between used to be erased — + /// stranding a file that `list` could not see, `restore` could not + /// reach and `purge` would not remove. Two threads quarantining while + /// a third restores reproduces it without the lock. + #[test] + fn concurrent_quarantines_and_restores_strand_no_file() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let victims: Vec = (0..40) + .map(|i| { + let f = dir.path().join(format!("victim-{i}.dat")); + std::fs::write(&f, format!("{i}")).unwrap(); + f + }) + .collect(); + + std::thread::scope(|scope| { + for chunk in victims.chunks(20) { + let q = &q; + scope.spawn(move || { + for victim in chunk { + quarantine(victim, Verdict::Safe, q).unwrap(); + } + }); + } + // Restores racing the appends, on whatever is listed so far. + scope.spawn(|| { + for _ in 0..40 { + if let Some(record) = list(&q).unwrap().first() { + // Losing the race with another restore is fine; + // stranding a file is not. + let _ = restore_from_manifest(&q, &record.quarantined_to); + } + } + }); + }); + + let listed: std::collections::HashSet = list(&q) + .unwrap() + .into_iter() + .map(|r| r.quarantined_to) + .collect(); + let on_disk: Vec = std::fs::read_dir(&q) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.path()) + .filter(|p| p.file_name().is_some_and(|n| n != MANIFEST_NAME)) + .collect(); + + for path in &on_disk { + assert!( + listed.contains(path), + "{} is in quarantine with no manifest record", + path.display() + ); + } + assert_eq!(listed.len(), on_disk.len()); + } } From 0cbea3c63d6c5714eac13fb4033a97cebf4d505a Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:14:23 +0500 Subject: [PATCH 27/33] fix(actions): make purging an unused quarantine a no-op, not an error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `purge` on a directory nothing has been moved into read an empty list and then asked `write_manifest` to rewrite it, which failed because the directory did not exist. The caller got "io error at .../manifest.jsonl.tmp: No such file or directory" — which reads like a disk fault rather than the nothing-to-do it actually is. Not reachable from the app today, which hides the button until something is listed, but `purge` is a public entry point and the error was a lie. --- crates/diskern-core/src/actions.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 2c33156..a68c521 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -327,6 +327,12 @@ fn write_manifest(quarantine_dir: &Path, records: &[QuarantineRecord]) -> Result let path = manifest_path(quarantine_dir); let tmp = path.with_extension("jsonl.tmp"); + // The directory need not exist yet: `purge` on a quarantine nothing + // has been moved into reads an empty list and lands here, and failing + // that with "No such file or directory" reads like a disk fault + // rather than the no-op it is. + std::fs::create_dir_all(quarantine_dir).map_err(|e| io_err(quarantine_dir, e))?; + let mut body = String::new(); for record in records { body.push_str(&encode(record)?); @@ -667,4 +673,13 @@ mod tests { } assert_eq!(listed.len(), on_disk.len()); } + + #[test] + fn purging_a_quarantine_nothing_was_moved_into_is_a_no_op() { + let dir = tempfile::tempdir().unwrap(); + let summary = purge(&dir.path().join("never-used")).unwrap(); + assert_eq!(summary.files_removed, 0); + assert_eq!(summary.bytes_removed, 0); + assert!(summary.failed.is_empty()); + } } From f1dc89eaa6dc6013fee9ebc9971f81a100a7edfe Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:15:25 +0500 Subject: [PATCH 28/33] fix(graph): let a directory be more than one kind of project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `roots` mapped each directory to a single `ProjectKind`, so a root holding two marker files kept whichever the walk yielded last. A directory with both `Cargo.toml` and `package.json` — any Rust binary with a web front end, this repository's own `app/` among them — got one `References` edge instead of two, and the store that lost stayed `Review` instead of dropping to `Risky`. The app then offered to quarantine the build output of a live project. Worse, *which* store lost depended on the order the walk returned the two markers in, so the same tree could give different verdicts on different machines. Deterministic verdicts are the guarantee the README opens with. A root now carries a set of kinds, and the maps are ordered, so a given scan produces the same graph every time. The node stays keyed by path, so a root that is two projects is still one project to anything counting references — asserted, along with both marker orders. --- crates/diskern-core/src/graph.rs | 106 +++++++++++++++++++++++++------ 1 file changed, 85 insertions(+), 21 deletions(-) diff --git a/crates/diskern-core/src/graph.rs b/crates/diskern-core/src/graph.rs index 1753e24..3ec1f40 100644 --- a/crates/diskern-core/src/graph.rs +++ b/crates/diskern-core/src/graph.rs @@ -11,7 +11,7 @@ use crate::FileEntry; use petgraph::graph::{DiGraph, NodeIndex}; use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::path::{Path, PathBuf}; /// A file whose presence makes the directory holding it a project root, @@ -36,7 +36,7 @@ pub enum Node { DependencyStore(PathBuf), // e.g. a node_modules dir, ~/.cargo/registry } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub enum ProjectKind { Cargo, Npm, @@ -75,7 +75,18 @@ impl ImpactGraph { /// reference it — and it is where the "referenced by 3 projects" /// number comes from rather than always being 1. pub fn from_entries(entries: &[FileEntry]) -> Self { - let mut roots: HashMap = HashMap::new(); + // A set of kinds per root, not one: a directory holding both a + // `Cargo.toml` and a `package.json` is both projects, and it is a + // shape that turns up constantly — any Rust binary with a web + // front end, this repository's own `app/` among them. Keeping one + // kind meant whichever marker the walk happened to yield last + // won, so one of `target/` and `node_modules/` came back + // unreferenced, and *which* one changed with directory order. + // + // Ordered maps, so the graph a given scan produces is the same + // graph every time. Verdicts are meant to be deterministic, and a + // reference count feeds straight into one. + let mut roots: BTreeMap> = BTreeMap::new(); let mut stores: HashSet = HashSet::new(); for entry in entries { @@ -94,26 +105,31 @@ impl ImpactGraph { continue; }; if let Some(dir) = entry.path.parent() { - roots.insert(dir.to_path_buf(), kind); + roots.entry(dir.to_path_buf()).or_default().insert(kind); } } let mut graph = Self::default(); - for (root, kind) in &roots { - let owned = stores_of(root, *kind, &stores); - let targets = if owned.is_empty() { - shared_store(root, *kind, &roots, &stores) - } else { - owned - }; - - for store in targets { - let from = graph.node(Node::ProjectRoot { - path: root.clone(), - kind: *kind, - }); - let to = graph.node(Node::DependencyStore(store)); - graph.graph.add_edge(from, to, Edge::References); + for (root, kinds) in &roots { + for kind in kinds { + let owned = stores_of(root, *kind, &stores); + let targets = if owned.is_empty() { + shared_store(root, *kind, &roots, &stores) + } else { + owned + }; + + for store in targets { + // Keyed by path, so a root that is two kinds is still + // one node with one edge per store — and one project + // for anything counting references. + let from = graph.node(Node::ProjectRoot { + path: root.clone(), + kind: *kind, + }); + let to = graph.node(Node::DependencyStore(store)); + graph.graph.add_edge(from, to, Edge::References); + } } } graph @@ -200,11 +216,14 @@ fn stores_of(root: &Path, kind: ProjectKind, seen: &HashSet) -> Vec, + roots: &BTreeMap>, seen: &HashSet, ) -> Vec { for ancestor in root.ancestors().skip(1) { - if roots.get(ancestor) != Some(&kind) { + if !roots + .get(ancestor) + .is_some_and(|kinds| kinds.contains(&kind)) + { continue; } let stores = stores_of(ancestor, kind, seen); @@ -333,4 +352,49 @@ mod tests { 3 ); } + + /// A directory can be more than one kind of project, and both of its + /// stores are then referenced. Keeping a single kind per root meant + /// whichever marker the walk yielded last won, so one store came back + /// unreferenced — and which one depended on directory order, which + /// makes the verdict non-deterministic. Both orders are asserted. + #[test] + fn a_root_that_is_two_projects_references_both_of_its_stores() { + let both = [ + "/proj/Cargo.toml", + "/proj/package.json", + "/proj/target/debug/app", + "/proj/node_modules/react/index.js", + ]; + let reversed: Vec<&str> = both.iter().copied().rev().collect(); + + for order in [both.to_vec(), reversed] { + let graph = ImpactGraph::from_entries(&entries(&order)); + assert_eq!( + graph.referencing_projects(Path::new("/proj/target/debug/app")), + 1, + "target/ in order {order:?}" + ); + assert_eq!( + graph.referencing_projects(Path::new("/proj/node_modules/react/index.js")), + 1, + "node_modules/ in order {order:?}" + ); + } + } + + /// A root that is two kinds is still one project, so a store it shares + /// with siblings must not be counted twice for it. + #[test] + fn a_two_kind_root_counts_once_against_a_shared_store() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/Cargo.toml", + "/repo/package.json", + "/repo/node_modules/react/index.js", + ])); + assert_eq!( + graph.referencing_projects(Path::new("/repo/node_modules/react/index.js")), + 1 + ); + } } From 0d576f28129690c3d52dcff8facc644cb778a23c Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:16:09 +0500 Subject: [PATCH 29/33] fix(graph): answer a cancel during the graph stage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ImpactGraph::from_entries` walks every entry's ancestors — measured at about 1.3s per million entries — and `build_with` ran it before the first `cancelled` check, with nothing checking the flag inside it. On a multi-million-file scan that is several seconds in which the Cancel button does nothing. Which undoes part of what the previous round bought: the walk and the hashing pass were both made to stop promptly, and this stage was inserted in front of them. Moving the dead spot is not removing it. `from_entries_cancellable` checks the flag per entry and again per project root; `build_with` propagates the `None`. `from_entries` keeps its signature for callers that have nothing to cancel with. --- crates/diskern-core/src/graph.rs | 32 ++++++++++++++++++++++++++++++- crates/diskern-core/src/report.rs | 20 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/crates/diskern-core/src/graph.rs b/crates/diskern-core/src/graph.rs index 3ec1f40..6178d45 100644 --- a/crates/diskern-core/src/graph.rs +++ b/crates/diskern-core/src/graph.rs @@ -13,6 +13,7 @@ use petgraph::graph::{DiGraph, NodeIndex}; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicBool, Ordering}; /// A file whose presence makes the directory holding it a project root, /// and the stores a project of that kind owns. @@ -75,6 +76,19 @@ impl ImpactGraph { /// reference it — and it is where the "referenced by 3 projects" /// number comes from rather than always being 1. pub fn from_entries(entries: &[FileEntry]) -> Self { + static NEVER: AtomicBool = AtomicBool::new(false); + Self::from_entries_cancellable(entries, &NEVER) + .expect("a run that cannot be cancelled cannot stop early") + } + + /// [`from_entries`], abandoned as soon as `cancelled` is set. + /// + /// This walks every entry's ancestors, which is a second or so per + /// million entries — long enough that a Cancel arriving during it + /// would otherwise do nothing until the stage after it started. The + /// walk and the hashing pass both stop promptly; this has to as well, + /// or it just moves the dead spot. + pub fn from_entries_cancellable(entries: &[FileEntry], cancelled: &AtomicBool) -> Option { // A set of kinds per root, not one: a directory holding both a // `Cargo.toml` and a `package.json` is both projects, and it is a // shape that turns up constantly — any Rust binary with a web @@ -90,6 +104,9 @@ impl ImpactGraph { let mut stores: HashSet = HashSet::new(); for entry in entries { + if cancelled.load(Ordering::Relaxed) { + return None; + } let store = enclosing_store(&entry.path); if let Some(store) = &store { stores.insert(store.clone()); @@ -111,6 +128,9 @@ impl ImpactGraph { let mut graph = Self::default(); for (root, kinds) in &roots { + if cancelled.load(Ordering::Relaxed) { + return None; + } for kind in kinds { let owned = stores_of(root, *kind, &stores); let targets = if owned.is_empty() { @@ -132,7 +152,7 @@ impl ImpactGraph { } } } - graph + Some(graph) } pub fn node(&mut self, node: Node) -> NodeIndex { @@ -397,4 +417,14 @@ mod tests { 1 ); } + + #[test] + fn a_cancelled_build_stops_instead_of_finishing() { + let cancelled = AtomicBool::new(true); + assert!(ImpactGraph::from_entries_cancellable( + &entries(&["/repo/package.json", "/repo/node_modules/react/index.js"]), + &cancelled, + ) + .is_none()); + } } diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index fea69d3..9b461c2 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -67,7 +67,7 @@ pub fn build_with( // at, worked out from the same entries the rest of the pipeline sees, // so a `node_modules` three live projects depend on can be told apart // from an abandoned one. - let impact = graph::ImpactGraph::from_entries(&entries); + let impact = graph::ImpactGraph::from_entries_cancellable(&entries, cancelled)?; // Classify before dedup, not after. A protected file has no business // in a duplicate set — the set is an offer to keep one copy and drop @@ -436,4 +436,22 @@ mod tests { .any(|r| r.starts_with("referenced by"))); assert_eq!(abandoned.reclaimable, 4); } + + /// The graph stage is a full pass over the entries in front of every + /// other cancellation check, so it has to answer a cancel itself — + /// otherwise it just moves the dead spot the last round removed. + #[test] + fn a_cancel_during_the_graph_stage_stops_the_report() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a.txt"), b"x").unwrap(); + + let cancelled = AtomicBool::new(true); + assert!(build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &cancelled, + ) + .is_none()); + } } From 4e8d7c8f791144e15a54c1adb2f02eb3cc698107 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:17:29 +0500 Subject: [PATCH 30/33] fix(report): keep risky bytes out of the reclaimable headline entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Risky findings were given `reclaimable = 0`, but only Protected entries were held out of dedup — and `total_reclaimable` treats a zero-reclaimable finding as "nothing has counted this yet". So a duplicate set of risky copies added its full `wasted`, and the bytes walked back into the headline through the half they had just been taken out of. Two live npm projects with an identical file in each `node_modules`: both copies risky, both `reclaimable = 0`, and the total still counted one copy's worth of them. Which is the overstatement #44 set out to remove, reintroduced by the risky verdicts #48 made reachable. There is now one `is_actionable`, used for both what counts towards the headline and what takes part in dedup. Splitting that definition across two expressions is what let them disagree. --- crates/diskern-core/src/report.rs | 99 +++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 13 deletions(-) diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 9b461c2..1275eef 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -69,10 +69,10 @@ pub fn build_with( // from an abandoned one. let impact = graph::ImpactGraph::from_entries_cancellable(&entries, cancelled)?; - // Classify before dedup, not after. A protected file has no business - // in a duplicate set — the set is an offer to keep one copy and drop - // the rest, and dropping a driver store copy is not on offer — and - // hashing it is time spent producing a number nobody can act on. + // Classify before dedup, not after. A file nothing will act on has no + // business in a duplicate set — the set is an offer to keep one copy + // and drop the rest, and dropping a driver store copy is not on offer + // — and hashing it is time spent producing a number nobody can use. // // Classification is cheap per entry, but a home directory is millions // of them, so it happens once and the answer is kept. @@ -95,7 +95,7 @@ pub fn build_with( let duplicate_sets = dedup::find_duplicates_filtered( &mut entries, - |i, e| e.size >= opts.dedup_min_size && verdicts[i].verdict != Verdict::Protected, + |i, e| e.size >= opts.dedup_min_size && is_actionable(verdicts[i].verdict), cancelled, )?; let files_scanned = entries.len() as u64; @@ -127,12 +127,10 @@ pub fn build_with( findings.push(Finding { // Bytes nothing will ever offer to move are not reclaimable. - // `actions::quarantine` refuses Risky as well as Protected, and - // the UI renders neither with an action, so counting either - // towards the headline promises space the app won't free. - reclaimable: match class.verdict { - Verdict::Protected | Verdict::Risky => 0, - Verdict::Safe | Verdict::Review => entry.size, + reclaimable: if is_actionable(class.verdict) { + entry.size + } else { + 0 }, entry, category: class.category, @@ -154,6 +152,20 @@ pub fn build_with( }) } +/// Whether anything will ever offer to move this file. +/// +/// `actions::quarantine` refuses Protected and Risky, and the UI renders +/// no action for either. One definition, used in both places it matters: +/// what counts towards the reclaimable headline, and what takes part in +/// dedup. Splitting them let Risky bytes back into the total through the +/// duplicate half after they had been taken out of the findings half. +fn is_actionable(verdict: Verdict) -> bool { + match verdict { + Verdict::Safe | Verdict::Review => true, + Verdict::Risky | Verdict::Protected => false, + } +} + /// What the pipeline worked out about one entry before findings are built. struct Classified<'a> { category: Category, @@ -346,8 +358,8 @@ mod tests { } /// The other half of #44: `find_duplicates` ran over every entry, - /// including protected ones, so system files contributed `wasted` - /// bytes to a total the user is never allowed to act on. + /// including ones nothing will act on, so their bytes contributed + /// `wasted` to a total the user can never do anything with. #[test] fn protected_files_form_no_duplicate_sets() { let dir = tempfile::tempdir().unwrap(); @@ -454,4 +466,65 @@ mod tests { ) .is_none()); } + + /// Risky bytes were taken out of the findings half of the headline and + /// then walked back in through the duplicate half: `total_reclaimable` + /// treats a zero-reclaimable finding as "nobody counted this yet", so + /// a duplicate set of Risky copies added its full `wasted`. + #[test] + fn risky_duplicates_do_not_return_to_the_headline() { + let dir = tempfile::tempdir().unwrap(); + for project in ["live1", "live2"] { + let root = dir.path().join(project); + std::fs::create_dir_all(root.join("node_modules/react")).unwrap(); + std::fs::write(root.join("package.json"), b"{}").unwrap(); + std::fs::write(root.join("node_modules/react/index.js"), b"identical").unwrap(); + } + + let rules = RulesDb::new( + 1, + vec![crate::rules::Rule { + id: "test-node-modules".into(), + patterns: vec!["**/node_modules/**".into()], + category: Category::BuildArtifact, + verdict: Verdict::Review, + description: "test".into(), + }], + ); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &rules, + &ReportOptions::default(), + &never, + ) + .unwrap(); + + let risky: Vec<&Finding> = report + .findings + .iter() + .filter(|f| f.verdict == Verdict::Risky) + .collect(); + assert_eq!(risky.len(), 2, "both node_modules copies are referenced"); + assert!(risky.iter().all(|f| f.reclaimable == 0)); + + // Nothing the app refuses to act on may reach a duplicate set... + for set in &report.duplicate_sets { + for path in &set.paths { + assert!( + !path.to_string_lossy().contains("node_modules"), + "{} is risky and should not be offered as a duplicate", + path.display() + ); + } + } + // ...so the headline is the one duplicate pair the user really + // can act on — the two identical `package.json` files, which are + // unclassified and so not findings, but are still two copies of + // the same two bytes. Before the fix the risky pair added its own + // 9 bytes on top, promising space the app refuses to free. + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.total_reclaimable, 2); + } } From 433b5c9f2ebf60c5d5155c080852f3d508b5b92d Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:19:14 +0500 Subject: [PATCH 31/33] perf(scanner): fold the path while comparing instead of normalizing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_excluded` built a normalized copy of every path — one allocation for the separator rewrite, another for a Unicode `to_lowercase` — for every child of every directory the walk opens, and did it before looking at the exclude list at all. It now folds character by character as it compares and stops at the first one that differs, which for a path not under an exclude is almost always the first or second. Empty exclude lists return immediately. Measured over 200k paths with the four default Linux excludes: 38.8ms for the original `starts_with`, 97.2ms after the component-boundary fix, 36.9ms now — so the correctness fix no longer costs anything. Folding is ASCII, matching `normalize_exclude`; these are directory names like `Windows` and `System`, and both sides were cross-checked against the previous implementation before the swap. --- crates/diskern-core/src/scanner.rs | 74 +++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 12 deletions(-) diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 4a04234..570429d 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -121,12 +121,15 @@ fn walk_root( Ok(()) } -/// Same shape the rules database matches in: lowercased, `/`-separated, -/// no trailing separator. An exclude written `C:\\Windows\\WinSxS` has to -/// match a root the user typed as `c:\\windows\\winsxs`, and -/// `rules::classify` already normalizes for exactly that reason. +/// Lowercased, `/`-separated, no trailing separator. An exclude written +/// `C:\\Windows\\WinSxS` has to match a root the user typed as +/// `c:\\windows\\winsxs`. +/// +/// ASCII case folding, not Unicode: these are directory names like +/// `Windows` and `System`, and it lets [`is_excluded`] fold the path a +/// character at a time instead of building a lowercased copy of it. fn normalize_exclude(exclude: &str) -> String { - let normalized = exclude.replace('\\', "/").to_lowercase(); + let normalized = exclude.replace('\\', "/").to_ascii_lowercase(); let trimmed = normalized.trim_end_matches('/'); // "/" itself trims to empty; keep it as the root rather than a prefix // that matches every path. @@ -142,14 +145,44 @@ fn normalize_exclude(exclude: &str) -> String { /// Compared on whole path components. A raw `starts_with` on the string /// made `/run` exclude `/runtime-data` as well, because "/run" is a prefix /// of "/runtime-data" in characters but not in directories. +/// +/// This runs for every child of every directory the walk opens, so it +/// normalizes nothing: it folds the path as it compares and stops at the +/// first character that differs — which, for a path not under an exclude, +/// is almost always the first or second. Building a normalized copy per +/// entry cost two allocations each and measured 2.4x slower than the +/// plain `starts_with` it replaced. fn is_excluded(path: &Path, excludes: &[String]) -> bool { - let p = crate::rules::normalize(path); - let p = p.trim_end_matches('/'); - excludes.iter().any(|ex| { - p == ex - || p.strip_prefix(ex.as_str()) - .is_some_and(|rest| rest.starts_with('/')) - }) + if excludes.is_empty() { + return false; + } + let path = path.to_string_lossy(); + excludes.iter().any(|ex| is_within(&path, ex)) +} + +/// Is `path`, folded as it goes, the already-normalized `exclude` itself +/// or something inside it? +fn is_within(path: &str, exclude: &str) -> bool { + let mut chars = path.chars(); + for expected in exclude.chars() { + match chars.next() { + Some(c) if fold(c) == expected => {} + _ => return false, + } + } + // Ends exactly there, or carries on at a component boundary — which a + // trailing separator on the path itself also satisfies. + match chars.next() { + None => true, + Some(sep) => sep == '/' || sep == '\\', + } +} + +fn fold(c: char) -> char { + match c { + '\\' => '/', + c => c.to_ascii_lowercase(), + } } fn to_epoch(t: std::time::SystemTime) -> Option { @@ -189,6 +222,23 @@ mod tests { )); } + #[test] + fn nothing_is_excluded_when_there_are_no_excludes() { + assert!(!is_excluded(Path::new("/proc/1/maps"), &[])); + } + + #[test] + fn a_trailing_separator_on_the_path_still_matches() { + let excludes = [normalize_exclude("/run")]; + assert!(is_excluded(Path::new("/run/"), &excludes)); + } + + #[test] + fn an_exclude_longer_than_the_path_does_not_match() { + let excludes = [normalize_exclude("/proc/self/fd")]; + assert!(!is_excluded(Path::new("/proc"), &excludes)); + } + #[test] fn a_trailing_separator_on_an_exclude_changes_nothing() { let excludes = [normalize_exclude("/proc/")]; From b9954f27ee85014025baba077d16ca478fb17a08 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:24:09 +0500 Subject: [PATCH 32/33] docs(changelog): note the two behaviours the review changed Restore refusing an occupied destination is something a user will meet, and so is a quarantine being declined outright for a path that cannot be recorded. Both are new answers to situations that previously had a silent, worse one. --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f97f44..96bfe77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,11 @@ All notable changes to Diskern are documented here. The format follows - Restoring a quarantined file across filesystems no longer fails with `EXDEV` +- Restore refuses when something is already at the original path, rather + than overwriting it — quarantine a cache, keep using the app, change + your mind, and the file the app rebuilt survives +- A file whose path can't be recorded in the manifest is not moved at + all, instead of being moved and then losing its record - The scan progress ticker thread stops from a guard, so a panicking or cancelled scan can't leave it emitting for the rest of the process - Two files that flatten to the same quarantine name no longer overwrite From bdcdbfad7bf668bbdecc420f7757b91923086946 Mon Sep 17 00:00:00 2001 From: Muawiya Amir Date: Sat, 5 Sep 2026 14:26:55 +0500 Subject: [PATCH 33/33] test(actions): use a non-UTF-8 fixture the spellchecker can read `b"caf\xe9.dmg"` made `typos` see a misspelt `calf` in the byte literal. Raw invalid bytes inside an ordinary name test the same thing without spelling anything. --- crates/diskern-core/src/actions.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index a68c521..5d7f949 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -554,8 +554,8 @@ mod tests { /// A path serde cannot encode must stop the move, not follow it. /// - /// Linux and macOS both allow filenames that aren't valid UTF-8, and a - /// disk scanner meets them. Encoding after the move left the file in + /// Linux and macOS both allow filenames that are not valid UTF-8, and + /// a disk scanner meets them. Encoding after the move left the file in /// quarantine, absent from the manifest, under a flattened name with /// no way back — while the caller was told the operation failed. #[cfg(unix)] @@ -565,8 +565,12 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let q = dir.path().join("quarantine"); - // "café.dmg" in Latin-1: a valid filename, invalid UTF-8. - let victim = dir.path().join(std::ffi::OsStr::from_bytes(b"caf\xe9.dmg")); + // Raw bytes no UTF-8 decoder accepts, in an otherwise ordinary + // name: a legal filename on this platform, and one a scan of a + // real disk turns up in downloads unpacked from old archives. + let victim = dir + .path() + .join(std::ffi::OsStr::from_bytes(b"photo-\xff\xfe.dmg")); std::fs::write(&victim, b"payload").unwrap(); let err = quarantine(&victim, Verdict::Review, &q).unwrap_err();