arch-audit v0.6.x — checkpoint: Batches F→J landed#136
Merged
Conversation
Two structural guards layered on top of the rusqlite 0.39 / SQLite 3.51.3 upstream fix for the second-Connection bug class (GH #131, closed alongside). clippy.toml (new): disallowed-methods on rusqlite::Connection::open. RW second Connections must go through SqliteWriter; RO inspection uses Connection::open_with_flags(SQLITE_OPEN_READ_ONLY | SQLITE_OPEN_NO_MUTEX). Header is honest about scope: RO opens are defense-in-depth + intent signal, NOT immunity from the upstream lock-order-inversion close race (that comes from the rusqlite version pin alone). The open_with_flags(RW|CREATE) loophole is acknowledged; legit RW exceptions are #[allow]-annotated with a WHY-comment. deny.toml: [bans] for rusqlite + libsqlite3-sys with wrappers scoped to perima-db (prod) + perima/perima-app (dev-deps) + r2d2_sqlite/refinery-core (transitive linkers required by cargo-deny's wrappers semantics). Adding rusqlite to a 4th first-party crate is now a hard CI failure. Call-site adjustments: - crates/cli/tests/{manifest_created,scan_persists, scan_with_metadata_test,scan_with_volumes}.rs: 6 read-only sites converted to open_with_flags(RO|NO_MUTEX); 1 UPDATE-injection site in scan_with_volumes::sentinel_rows_migrated keeps RW with #[allow] (the perima scan subprocess writer has already exited by the time the test injects, so no concurrent second writable Connection). - crates/db/src/manifest.rs: production write_manifest #[allow] (separate manifest.db file on the user's volume, not the main perima.db; different unixInodeInfo => no inversion risk); 3 unit-test sites RO-converted. - crates/db/src/search_repo.rs: seed_conn proptest helper #[allow] (#124 backlog; post-3.51.2 safe). - crates/db/src/connection.rs: open_and_migrate #[allow] (this IS the SqliteWriter's single Connection entry point; the lint exists to push callers INTO this function). - crates/app/src/search.rs: seed_via_conn test FTS seed #[allow] (post-3.51.2 safe but fragile pattern; structural fix would route via writer). Verified: just clippy clean, cargo deny check bans ok, full nextest green (240/240 + 12/12 desktop).
…gic-drop teardown Pre-fix shutdown depended on every cloned Sender<WriteCmd> (held by repos / handlers) being dropped before SqliteWriterHandle::join(). Forgetting one parked the writer thread in flume::Receiver::recv forever and hung pthread_join. The bug class produced "magic-drop" callsites that listed N explicit drops matching how many senders the surrounding code had cloned (e.g. GH #131's 3-of-3 fix in run_scan_inner_with_metadata; the magic number went 1 -> 4 -> 3 across two commits in two days). This change replaces the implicit "drop all senders" contract with an explicit Shutdown signal: - crates/db/src/cmd.rs: add WriteCmd::Shutdown variant (no payload). WriteCmd is #[non_exhaustive] so this is non-breaking. - crates/db/src/writer/mod.rs: * run_writer_loop matches Shutdown and returns before dispatch. * dispatch gets unreachable!() for Shutdown to keep the existing no-_-arm exhaustiveness pressure on future variants. * SqliteWriterHandle::join sends Shutdown via try_send before joining the thread. Surviving sender clones become inert (next try_send -> Disconnected) instead of blocking shutdown. * NO Drop impl on the handle. Adding Drop would Shutdown-trigger on every handle scope-exit, breaking the CLI's deliberate "drop handle, let sender clones in repos extend the writer's life" pattern (crates/cli/src/main.rs::build_container). Tested and confirmed during development: Drop broke 6 CLI integration tests with "sending on a closed channel". * Regression test writer_shuts_down_with_outstanding_sender_clones spawns the writer, clones a sender, calls handle.join() WITHOUT dropping the clone, and asserts the join completes in <5s + the surviving clone's try_send returns Err(Disconnected). - crates/desktop/src/commands.rs::run_scan_inner_with_metadata: 5 lines (drop(file_repo); drop(vol_repo); drop(sentinel_repo); writer.join(); + WHY-comment) collapse to 1 + a one-paragraph WHY-comment pointing at the new pattern. The ~15 other "drop(repo); writer.join();" sites in test fixtures across crates/db/tests/ and crates/desktop/tests/ keep their explicit drops; they are now no-ops but harmless. A future cleanup sweep can remove them. Verified: just clippy clean, full nextest green (240/240 + 12/12 desktop). The Shutdown regression test passes in 93ms.
WHY: Batch F replaces 16 hand-written FTS5 triggers with one
template + Rust spec. minijinja 2.x is the audit-§4.6 pick (small
runtime, jinja-familiar, supports {% macro %} blocks for shared
aggregation patterns). default-features=false + explicit
[macros, loader] features keeps the dep slim.
WHY: Batch F's single source of truth for the 16 FTS5 sync triggers. Spec entries replace the hand-copied trigger SQL across V006/V007/V008. LEGACY_TRIGGER_NAMES preserves V007-only names so existing dev DBs converge after the boot-time install lands. Template + render in next two tasks consume FTS_AGGREGATIONS.
Closes 8 clippy::doc_markdown errors flagged by Task 2 code-quality review. SQL table/column names (search_content, search_index, deleted_at) and one helper-fn name (representative_path) plus the proper noun SQLite were used bare inside doc comments. Workspace clippy lint level is -D warnings, so these were CI-gate-blocking. No semantic change.
WHY: Single template + 4 shared aggregation macros encode the V007->V008 lesson (every tag/metadata aggregation filters deleted_at on BOTH the link and entity tables) once. Per-BodyKind macros expand to the V008 trigger bodies. Render fn lands in next task; this commit just introduces the template asset + a parse smoke test.
WHY: render_fts_triggers() reads spec.rs + template, returns the full install SQL (LEGACY DROPs + current DROPs + 16 CREATE TRIGGER bodies). install_fts_triggers(&conn) execute_batch'es it. Per-source-table snapshots pin the rendered SQL byte-for-byte; reviewer-verified to match V008-final form semantics. Wired into writer in next task. Snapshots land at crates/db/src/schema/snapshots/ (insta default for unit tests in src/, NOT the plan-misstated tests/snapshots/ path). Adds insta dev-dep to perima-db (workspace pin already present).
WHY: Closes the V006->V007->V008 trigger drift bug class. SqliteWriter::start and ::start_in_memory both call install_fts_triggers after their refinery migration apply. Idempotent — runs every boot, DROPs V006/V007/V008 trigger names + LEGACY V007-only names, then CREATEs codegen-rendered bodies. start_in_memory_installs_fts_triggers smoke test verifies the in-memory path; the file-backed path is exercised by every existing FTS-touching integration test.
Code-quality reviewer minors on commit fb2c422: add WHY comments at both install_fts_triggers call sites (sibling sites in the file all carry inline WHY blocks for boundary decisions, per CLAUDE.md), and swap drop(h) → h.join() in the new smoke test so it matches sibling writer tests' shutdown discipline (SqliteWriterHandle has no Drop impl, so drop() leaks the writer thread until process teardown).
WHY: pins the boot-time install contract. Second call on the same conn is a verified no-op on sqlite_master rows. Catches any future template edit that accidentally introduces non-deterministic SQL output.
WHY: pins the LEGACY_TRIGGER_NAMES contract. Existing dev DBs that ever ran on V007-only state carry 'search_after_location_hash_change'; this test pre-seeds it and asserts install_fts_triggers DROPs it. Removing or shrinking LEGACY_TRIGGER_NAMES without a paired add to FTS_AGGREGATIONS fails this test.
Code-quality reviewer minor on commit 56c1e16: the test was hard-coded to one legacy name ('search_after_location_hash_change') but its plural name promised coverage of every entry. Switch to a loop over LEGACY_TRIGGER_NAMES so adding a 2nd entry (e.g. when V010+ retires another V008-era trigger) automatically gets covered. Adds a trigger_count helper to dedupe the two query_row sites and a WHY comment explaining why the pre-seed body is a minimal stub.
Code-quality reviewer minors on commit 18cc96c: replace the magic 3 across the strategy / Model / seed loop with a single SLOTS const (commented to flag what bumping it requires); add a debug_assert in shadow_hash that enforces the slot < 128 single-byte encoding bound; extend attach_tag_raw's WHY comment to spell out the idempotency contract that mirrors the model's BTreeSet::insert.
WHY: Batch G inventory pass landed all ~22 raw-SQL test helpers + cross- cluster consts in a single shared module per spec §5.2. Tests still live in src/search_repo.rs this commit; subsequent commits move tests cluster- by-cluster + delete the in-source mod tests block. The #![allow(dead_code)] is load-bearing — helpers are consumed by 3 sibling integration-test binaries each compiling common/mod.rs with only a subset used per binary (workaround per rust-lang/rust#46379).
WHY: Batch G cluster 1 of 3. Tests that exercise search behavior without trigger-side assertions land in search_semantics.rs. Helpers consumed from common/mod.rs (Task 2). Test count unchanged (118 in-crate tests; 11 tests now in separate search_semantics integration-test binary). Also fixes common/mod.rs: adds #![allow(unreachable_pub)] to suppress clippy -D warnings for pub helpers in test-binary-local mod.
…_post_v007 WHY: spec-review nit. Doc comment "I5: calling SearchRepository::rebuild() twice..." was dropped during the verbatim move in 2e65ae0. Single-line fix preserves the original byte-identical-move invariant.
WHY: Batch G cluster 2 of 3. T22/T40-T48 + trigger_sync_on_* + multi-location-rename + soft-delete/restore + tag-rename-propagates land in search_triggers.rs. Helpers consumed from common/mod.rs. Also folds in 3 style NITs from Task 3 code review: - //! before #![allow] in search_semantics.rs (matches dominant convention). - Inline WHY on #![allow] attrs in common/mod.rs (matches Task 2 style). - No blank line between use common and use perima_core (single import group). Adds #[allow(dead_code, unused_imports)] to src/search_repo.rs::tests to silence -D warnings on helpers now only used by the 2 remaining proptests; Task 5 deletes the mod wholesale. Test count unchanged (118).
WHY: Batch G cluster 3 of 3 (final). Both proptests (fts_consistent_under_tag_churn + fts_matches_ground_truth_under_soft_delete_churn) move to search_proptests.rs with case counts unchanged (#124 cap preserved). Now-empty #[cfg(test)] mod tests block deleted from src/search_repo.rs. Stale comments at fts_codegen_round_trip.rs:~57,~205 updated to point at common::seed_conn / common::test_db. Long dead_code WHY in common/mod.rs wrapped to ≤100-char lines (Task 4 review NIT). Production search_repo.rs settles at ~120 LOC; audit §A9's literal 4-way split (mod.rs/filters.rs/query.rs/rows.rs) declared SUPERSEDED by Batch C — no meaningful production split surface remains. Test count unchanged (118).
WHY: code-review NIT. Multi-line WHY between two #![allow] attrs read ambiguously. Shortened comment fits inline (92 chars) so attribute and its WHY are visually adjacent like the unwrap_used allow on line 1.
…ingleton WHY: Batch H Task 1. Three new prod deps (@tanstack/react-query@^5, zustand@^5, @tanstack/react-router@^1) + queryClient singleton with desktop-calibrated defaults (5min staleTime, no refetchOnWindowFocus). Register module augmentation makes useQuery error type = CoreError without per-call generics (which would break T inference per v5 TS docs). QueryClientProvider wraps App; Router added in Task 6. No behavior change yet.
Reviewer nit: gcTime: 30min was the only default option missing a WHY block. The other four defaults all explain their non-default choice; gcTime should too — it diverges from v5's 5min default deliberately (native desktop, ample RAM, route-switch cache warmth).
…lumes/search WHY: Batch H Task 2. Factory pattern per spec §5.4. Each domain exports xxxKeys namespace + xxxQueryOptions(args) factory + use<Domain>(args) hook. Bridge to neverthrow::ResultAsync via .match(ok, err => throw err). Search query gates on `enabled: query.length >= MIN_QUERY_LEN` (= 2); empty input fires no IPC. eslint-disable on throw sites (with WHY block) because CoreError is the Register-augmented defaultError type — wrapping in Error would lose the typed discriminant. No consumers yet; tests untouched.
WHY: Batch H Task 3. Single store, slice pattern per Zustand 5 docs. Slices: View (viewMode + selectedTagId), Search (raw + debounced query), Scan (status + lastReport), Notifications (toast queue). notifyError(err: CoreError) is convenience for error paths. No persist middleware — intentional per-restart reset matches today. No Provider needed (Zustand singleton). No consumers yet.
WHY: Batch H Task 4. Single subscription site for app-event channel. Per-reason surgical invalidation (TagsChanged → tagsKeys; FilesChanged + MetadataChanged → filesKeys debounced 300ms; SearchIndexRebuilt → searchKeys). Upgrades Batch E's TODO Batch H coarse-refetch placeholder. TS-exhaustive switch on event.kind AND event.data.reason — adding a new variant becomes a compile error. No consumer yet.
WHY: Batch H Task 5. Renders useUiStore.notifications. info kind auto-dismisses after 5s; error persists until user clicks ×. Replaces ad-hoc error + watcherError state in App.tsx (deferred from Batch D D-11). Not yet mounted; mounts in Task 7.
…der IndexRoute WHY: Batch H Task 6. Code-based routes (NOT file-based) — single route in v0.6.x. createHashHistory matches Tauri static-served dist/index.html. App.tsx becomes the root-route component (mounts inside <RouterProvider> via the rootRoute.component). IndexRoute is a placeholder — Task 7 fills with the file/tag/search composition. KNOWN: App.test/App.compose/ScanButton tests fail this commit — they assert against pre-router App content. Task 10 rewrites them (see spec §4.11 for the per-file rewrite scope). NOTE: All 78 tests pass this commit (no actual failures); the KNOWN note is pre-documented for Task 7 which will cause the failures.
WHY: Batch H Task 7. App.tsx shrinks from 322 LOC to 42 LOC: provider- free root-route component (useDomainEvents mount + layout chrome). 12 useStates + 2 useEffects in App.tsx → 0. ViewModeToggle extracted to its own component (reads viewMode from store). IndexRoute (routes/index.tsx) holds the file/tag/search composition, reading from useFiles/useTags/useSearch + useUiStore. No manual useMemo on composeVisible/sortByRank/computeFacets — React Compiler 1.0 handles. Interim shape: SearchBar/ScanButton/StatusBar accept all props as optional with defaults so App.tsx can call them prop-less; their internals stay prop-driven until Tasks 8a/8b/9 migrate them to store-driven. Test rewrites land in Task 10.
WHY: Batch H Task 8a. ScanButton owns its own mutation. onSuccess
mirrors result into useUiStore.scan (StatusBar reads from there);
notify('info', ...) gives transient UX confirmation; invalidateQueries
on files + tags; startWatch fires async. busy state derived from
both store status AND mutation.isPending so the button stays
disabled across re-renders.
tsconfig.app.json added (excludes src/__tests__) so tsc -b in the
build script type-checks only production source — test files are
owned by vitest's own TS pass. build script updated to
`tsc -b tsconfig.app.json`. tsconfig.json unchanged (ESLint still
uses it for type-aware rules across all files).
ScanButton.test failure is Task 10.
WHY: Batch H Task 8b. SearchBar owns timing + sanitisation; dispatches setSearchQuery(raw) immediately + setDebouncedQuery(sanitised) on 300ms debounce. IndexRoute keys useSearch on debouncedQuery — IPC fires once per debounce, not per keystroke. MIN_QUERY_LEN/buildFtsQuery/ clearedRef logic preserved verbatim. SearchBar.test failure is Task 10.
Reviewer noted the dual-field rewrite dropped aria-label="Search files" that the prop-driven SearchBar had. Restoring as a single-line addition so screen-readers announce the field correctly.
…rBanner WHY: Batch H Task 9. StatusBar reads scan slice + uses useShallow for multi-property selector (Zustand v5 crash safety). TagSidebar drops selectedTagId/onSelect props in favor of direct store reads. WatcherBanner deleted — watcher errors now flow as notifyError → NotificationStack toast.
WHY: Batch H Task 10. App.test → hooks/useDomainEvents.test (assertions on queryClient.invalidateQueries spy via renderHook + mocked subscribeToAppEvents capture). App.compose.test → routes/index.test (renders IndexRoute via renderWithProviders, mocks api.listFilesWithTags + api.listTags + api.search, asserts sidebar All-count under each case 1-5 of the #25 composition pin). StatusBar/SearchBar/ScanButton tests rewritten to mock store + Query providers via the new __tests__/test-utils.tsx helper (renderWithProviders + makeFreshQueryClient + resetUiStore + defaultUiState). TagSidebar tests also updated to drop the removed onSelect/selectedTagId props (Task 9 left them broken; Task 10 closes that gap). All 81 tests green; pre-Batch-H baseline was 78. WHY fireEvent.change (not userEvent.type) in SearchBar tests: userEvent v14 + vi.useFakeTimers + React 19 controlled-input rerender hangs on microtask-flush dependency. fireEvent.change is synchronous + same React-controlled-input path our prod code hits. WHY vi.spyOn(useUiStore, "setState") (not on individual action funcs) for the C1 regression test: spying on getState().setX swaps the function identity, which makes consuming components see a "new" useEffect dep on next render and re-run the effect (false positive). setState is the single funnel every Zustand action passes through.
Reviewer flagged inconsistency between TagSidebar's spyOn(useUiStore.getState(), "setSelectedTagId") and SearchBar's spyOn(useUiStore, "setState") funnel. Both are valid; TagSidebar's form is safe ONLY because the component has no useEffect deps on the action. Documented the safety condition + the upgrade path.
WHY: Batch I needs rolling-file log appender + cross-platform log-dir resolver in crates/app::telemetry (where CLI + desktop both call into). tracing-appender 0.2.5 (latest stable; same tokio-rs org as tracing-subscriber). directories v6 already in workspace from earlier work; just adding the workspace = true line in crates/app.
WHY: Batch I Task 2. Hoists CLI's logging::init to crates/app/telemetry.rs so desktop can share. Adds rolling-file layer (hourly rotation) alongside stderr; JSON-by-default in release, pretty in debug; PERIMA_LOG_JSON override stays. Log dir resolved via directories::ProjectDirs (state_dir on Linux, data_dir fallback elsewhere — cache_dir rejected because macOS auto-purges). Returns WorkerGuard which the caller MUST hold for process lifetime (dropping terminates the non-blocking flush thread). Helper truncated() keeps span fields bounded for user-input strings (used in Task 5's SearchUseCase instrument). 6 new unit tests. CLI's logging.rs untouched yet (Task 3 deletes it). tracing-subscriber added to crates/app/Cargo.toml (required by init_subscriber layered-subscriber construction; was CLI-only before).
WHY: Batch I Task 3. Removes crates/cli/src/logging.rs (functionality hoisted to perima_app::telemetry in Task 2). main() holds the returned WorkerGuard for process lifetime via _log_guard binding — dropping early loses pending log lines from the non-blocking appender. Also creates ~/.local/state/perima/logs/perima.log on first run via the new file appender (was stderr-only before).
WHY: Batch I Task 4. Desktop binary has had no tracing subscriber init until now — handler-side tracing::*! macros were silently dropping events. Calls init_subscriber at the top of run() BEFORE tauri::Builder::default() so handler events emitted during .setup are captured. AppState gains _log_guard field (holds the WorkerGuard for app lifetime). AppState::new loses const because WorkerGuard is not const-constructible — non-load-bearing change. After this task, desktop logs to the same OS-resolved log dir as CLI (~/.local/state/perima/logs/ on Linux).
WHY: Batch I Task 5. 6 instrumentation sites: 5 UseCase::execute (Scan, Search, Tag, Volume, Metadata) + writer dispatch. err(level="warn", Display) auto-logs Err returns at WARN (not default ERROR — over-promotes user-input failures). Adds kind_str() accessor on ScanCommand/TagCommand/VolumeCommand/ MetadataCommand/WriteCmd; path_display() on ScanCommand. NO hlc field on dispatch span — HLC is per-row, generated in handlers (per Batch C constraints). truncated() helper from app::telemetry bounds the SearchUseCase query field at 64 chars to prevent span bloat from long user queries; #[allow(dead_code)] on truncated removed (Task 5 IS the consumer). 4 continue-path tracing::warn! calls in scan.rs::execute_full preserved (per spec §6.6 — err(...) only catches outer execute Err returns, doesn't conflict with inner-loop continue warns).
…pture WHY: Batch I Task 6. perima debug-report bundles the active rolling log + last K rotated files + env context (perima version, GIT_SHA, OS, PERIMA_LOG/PERIMA_LOG_JSON values) into a single file for attaching to bug reports. Falls back to "(no active log file present)" if first run. build.rs captures `git rev-parse --short HEAD` into the GIT_SHA rustc-env at compile; falls back to "unknown" if git/.git absent (vendored builds). cargo:rerun-if-changed=.git/HEAD picks up new commits during dev. Integration test seeds an isolated XDG_STATE_HOME and asserts the report-file contains the expected divider structure.
WHY: tracing-appender 0.2.5 with Rotation::HOURLY + prefix "perima" writes filenames like "perima.YYYY-MM-DD-HH" (NOT "perima.log" or "perima.log.YYYY..."). I-6's initial impl hardcoded the legacy plan filenames so the active-log section always showed "(no active log file present)" and the rotated section was always empty. Fix: glob log_dir for files starting with "perima.", lex-sort descending (== chronological descending given the timestamp suffix), use the lex- greatest as the active log and skip-1-take-K for rotated. Test assertion relaxed from "=== active log: perima.log ===" to "=== active log: " (the dynamic filename suffix shifts hour-by-hour). Smoke confirmed: report now contains "=== active log: perima.2026-04-24-11 ===".
…er iterations WHY: GH #111. Previously ThumbnailGenerator::resize_image called Resizer::new() on every iteration, discarding scratch buffers + the CPU-extension dispatch cache. Worker-owned Resizer per spec D-1 (Approach A in 3-way alternatives table) keeps zero synchronisation overhead, future-multi-worker-friendly, and avoids the work-stealing hazard that would defeat thread_local!. Resizer: Send (fast_image_resize 6.0 supertrait); owning it inside the tokio::spawn'd async move future is sound under the multi-thread runtime. ThumbnailGenerator::generate + resize_image gain &mut Resizer last positional; MetadataQueue::spawn's spawned closure allocates one Resizer per worker task lifetime; process() forwards. 10 unit tests adapt for new arity (one let mut resizer = Resizer::new() at test top each). resize_image's local `resized` renamed to `output_img` and generate's to `scaled` to satisfy clippy::similar_names (-D warnings). resize_only_bench rewrite + read_exif log-level bump land in J-2 + J-3.
WHY: J-1 (46a8a26) added `let mut resizer = Resizer::new();` at the top of 5 pixel-coercion tests adjacent to the existing `let resized = ...` binding. clippy::similar_names fires under `--all-targets` (the `just clippy` invocation) on the `resizer` vs `resized` one-character diff. The implementer's lib-only `cargo clippy -p perima-media` did not surface these because it skips the test module. Renamed the 5 test-side `resized` bindings to `output_img` (matches the production-side rename in resize_image:221). Also moved `use fast_image_resize::Resizer;` in queue.rs above the crate-internal `use crate::thumbnail::ThumbnailGenerator;` to match third-party-then- crate convention. Mechanical fix; no functional change. Verifies clean under `cargo clippy -p perima-media --all-targets -- -D warnings` + `cargo nextest run -p perima-media` + `cargo doc -p perima-media`.
WHY: GH #110. Previously read_exif's MediaSource::file_path failure arm logged at debug — silently invisible at the default RUST_LOG=info level. Permission errors / missing files / symlink loops were indistinguishable from "file has no EXIF block" (the normal case for PNGs and many camera-exported JPEGs). Per spec D-2 (3-way option table; the typed-CoreError-variant + Err- propagation alternatives rejected as over-spec): only the log-level changes. Outer extract() still returns metadata with empty EXIF fields; behavior preserved. Parse-error arm UNCHANGED at debug — parse failures are normal for many containers and would noise-spam logs at warn.
WHY: GH #111 Batch J acceptance gate. resize_only_bench was #[ignore]-d (skipped on every nextest run) and its message body referenced `cargo test` which is banned per CLAUDE.md (scripts/no-cargo-test.sh). Renamed to resize_only_bench_baseline_vs_reused_proves_amortization; runs BOTH baseline (fresh Resizer per iter) AND reused (one Resizer) loops in the same test invocation; asserts reused is ≥5% faster. WHY cfg_attr(debug_assertions, ignore): in unoptimized builds the scratch-buffer + CPU-dispatch-cache savings are invisible against the ~450ms-per-iter pixel-processing cost (measured 1.01x debug vs 1.06x release). The 5% threshold only holds when SIMD is active. Release CI covers this via: cargo nextest run --release -p perima-media. Audit acceptance is ≥10% throughput; 5% lower bound chosen for CI flake margin per spec D-3. Image size: 1920×1080 (avoids nextest slow-timeout in debug). ITERS=20 provides stable signal in release.
…135 WHY: spec reviewer empirically reproduced 27% flake rate (15-run release sample) against the ≥1.05 speedup assertion added in 3811586. Per-run ratio swings 0.890x — 1.049x; mean ~1.02x. Signal is dominated by DynamicImage::clone() (~6 MB memcpy/iter) + WebP encode in resize_image() — Resizer-internal scratch + dispatch-cache savings are a tiny fraction. The previous cfg_attr(debug_assertions, ignore = ...) gating violated the plan's "runs on every cargo nextest" requirement AND empirically failed in release at 27%. Per Batch J spec §7 option B + §8 risk #4: convert to print-only + file follow-up. Removed the cfg_attr ignore so the test runs in BOTH debug and release; removed the assert! so flake floor doesn't tank CI; preserved the eprintln so a human reading CI logs can still spot regressions by eye across runs. GH #135 tracks the restructuring needed to re-introduce a stable assertion (mitigations: bypass DynamicImage::clone, use Image<&[u8]> directly, or adopt criterion).
clippy 1.95's missing_const_for_fn correctly identifies AppState::new as a pure field-assignment body. The Batch I WHY comment about WorkerGuard blocking const was wrong — WorkerGuard construction happens in the caller, not inside new().
Windows runners share C:\.perima\manifest.db across parallel test binaries. Filter manifest_files COUNT by tempdir basename so concurrent scans don't inflate this test's row count. Linux/macOS unaffected (volume root /.perima/ is root-only; manifest write silently fails).
Workaround for #137. taiki-e/install-action's PowerShell wrapper aborts on windows-latest when the runner image leaks BASH_FUNC_* env vars; their 10-iter retry doesn't catch it (verified 2/2 runs failed). The defensive Shellshock check is correct security behavior — fix is to bypass bash entirely on Windows. Linux/macOS unchanged.
Concurrent CLI test binaries write to shared C:\.perima\manifest.db on GHA windows-latest runners. Basename-filter fix in 19d68c1 confirmed the 3 fixture rows are stripped by a write race before the assertion. Linux/macOS unaffected — they exercise the else-branch graceful- degradation path which is the more interesting invariant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Architecture-audit branch checkpoint #2 — continues from #130 (Batches A→E). Branch stays open; test-architecture work (#101 + #108) lands on a follow-up branch.
Bundles five batches plus #131 hardening:
crates/db/src/schema/(Rust + minijinja templates) owns FTS5 trigger DDL; refinery owns table DDL.install_fts_triggersruns at every writer start (sub-ms idempotent). Snapshot-tested + 32-case proptest. Closes research: should perima drop SQLite migrations entirely (post-v1 reopen) #134.crates/db/src/search_repo.rstest extraction. Moves 22 raw-SQL helpers + 5 test files tocrates/db/tests/{common,search_semantics,search_triggers,search_proptests}.rs. Productionsearch_repo.rscollapses to 117 LOC.useState(component-local). TanStack Router withcreateHashHistory().apps/desktop/src/App.tsx322 → 42 LOC. `Register { defaultError: CoreError }` makes typed errors flow without per-call generics. `useDomainEvents` is the sole `subscribeToAppEvents` consumer. `WatcherBanner` deleted (errors now toast).#131 hardening (bundled inline): `clippy.toml` bans `rusqlite::Connection::open` outside writer code; `WriteCmd::Shutdown` variant for clean writer-actor termination.
Test plan
Closes
Filed during this push
Merge plan
Squash-merge. Branch is not deleted — work continues on `architecture-audit/v0.6.x`.