Skip to content

Remove 1.1s wall-clock sleep from model-path cache test #15

Description

@BumpyClock

Problem

resolve_model_path_uses_cache_and_does_not_rescan in
crates/core/src/transcription/model/tests.rs (lines 329–359) contains a
hard-coded 1 100 ms sleep at line 341:

// tests.rs:338–342
// Sleep so snapshot B gets an unambiguously newer mtime than A; a fresh
// `resolve_model_path_in_dir` scan sorts by mtime desc and would pick B. If
// the cache is honored, we still get A — proving no rescan (TR-3).
std::thread::sleep(std::time::Duration::from_millis(1100));
let snapshot_b = create_parakeet_snapshot(tmp.path(), "snap-b");

The sleep exists solely to guarantee that the filesystem assigns a strictly
newer mtime to snap-b than to snap-a, satisfying the mtime-descending
sort inside resolve_model_path_in_dir (catalog.rs lines 144–152):

// catalog.rs:144–152
let modified = entry
    .metadata()
    .and_then(|meta| meta.modified())
    .unwrap_or(std::time::SystemTime::UNIX_EPOCH);
candidates.push((modified, path));
// ...
candidates.sort_by(|a, b| b.0.cmp(&a.0));

Without the sleep, snap-a and snap-b can share the same 1-second-resolution
mtime on HFS+/APFS (and many Linux filesystems), making sort order undefined and
the assertion unreliable.


Why it matters

  • CI latency — this single test adds ≥1.1 s of wall-clock time unconditionally;
    it runs on every cargo test invocation of ansible-core.
  • Flake risk — the approach depends on filesystem mtime granularity. On
    filesystems with sub-second precision (ext4, btrfs, APFS) the sleep is usually
    sufficient, but any CI environment whose clock is throttled or where tmpfs
    coalesces timestamps can still produce equal mtimes, causing a non-deterministic
    test failure.
  • Coupling to a private implementation detail — the test verifies cache
    correctness (a public-ish invariant) by exploiting mtime ordering (a private
    scan heuristic). If the scan strategy changes, the test breaks for the wrong
    reason.

Proposed change

Preferred approach — inject deterministic mtimes (no sleep):

  1. Add filetime as a [dev-dependencies] entry in crates/core/Cargo.toml.
  2. After creating snap-a, call
    filetime::set_file_mtime(&snap_a, filetime::FileTime::from_unix_time(1_000_000, 0)).
  3. After creating snap-b, set its mtime one second later:
    filetime::FileTime::from_unix_time(1_000_001, 0).
  4. Delete std::thread::sleep(...) at line 341.
  5. The rest of the test body (lines 343–358) is unchanged.

Alternative — assert cache hit through an observable signal (no mtime dependency):

If pinning filetime as a dependency is undesirable, the cache invariant can be
asserted without relying on mtime ordering at all:

  • After refresh_download_status() caches snap-a, delete the snap-b
    directory's parent from the filesystem (so a rescan would return
    Err/None), then assert resolve_model_path still returns snap-a. This
    proves the cache is consulted without requiring any ordering guarantee.
  • The existing resolve_model_path_falls_back_when_cache_stale test (lines
    361–380) already covers the inverse path (stale cache → fallback rescan), so
    the test suite stays complete either way.

Tests that must change: only resolve_model_path_uses_cache_and_does_not_rescan
(lines 329–359). No other tests touch the sleep; no production code changes.


Recommendation / triage

DO — pure test improvement, removes CI latency + flake. Prefer setting
deterministic mtimes (filetime crate or set_file_mtime) over sleeping; or
assert cache behavior through an observable (no rescan) signal.


Acceptance criteria

  • std::thread::sleep removed from resolve_model_path_uses_cache_and_does_not_rescan.
  • Test still asserts: cached path (snap-a) wins over a newer unscanned snapshot (snap-b).
  • Test still asserts: after refresh_download_status(), the newer snapshot is picked.
  • cargo test -p ansible-core passes with no flaky failures on CI.
  • Wall-clock runtime of the test drops below 50 ms (from ≥1 100 ms).
  • No production code changed; behavior of resolve_model_path and refresh_download_status unchanged.
  • Existing neighboring tests (refresh_populates_resolved_path_cache, resolve_model_path_falls_back_when_cache_stale, delete_model_invalidates_resolved_path_cache) continue to pass unmodified.

Traceability

  • clawpatch finding ID: fnd_sig-feat-service-60d65539ae-3efc_ddc627fe81
  • Revalidate command:
    clawpatch revalidate --finding fnd_sig-feat-service-60d65539ae-3efc_ddc627fe81 --reasoning-effort high
    
  • Verified against current code at crates/core/src/transcription/model/tests.rs:341 and crates/core/src/transcription/model/catalog.rs:144–152 (no line drift from the original finding).
  • No coupled issues at this time.

Metadata

Metadata

Assignees

No one assigned

    Labels

    deslopifyFound by clawpatch deslopify reviewperformanceCPU/memory/latency efficiencytech-debtCode quality / maintainability cleanup

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions