diff --git a/phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift b/phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift index 527f54c2..d2a39e56 100644 --- a/phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift +++ b/phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift @@ -1,7 +1,64 @@ import Foundation enum AutotuneRecommendationRunner { - private static let processTimeout: TimeInterval = 30 + /// Wall-clock budget for `macprovider-cli autotune --recommend --json`. + /// + /// Autotune runs Stage-1 probes against every non-blocked, RAM-eligible + /// row in the catalog. Each probe spawns a subprocess of + /// `macprovider-cli serve`, waits for MLX to load the model, then does a + /// prewarm HTTP call followed by measured TTFT + tokens/sec replicates. + /// + /// Per-candidate strict worst case (see `Stage1Prober` in + /// `Stage1Iterator.swift`): + /// * `readyTimeoutSec = 120s` — MLX model load + /// * prewarm `probeOnce` = `probeIdleTimeoutSec = 300s` (Track A3, + /// `Stage1Iterator.swift:474`) + /// * measured replicate `probeOnce` = `probeIdleTimeoutSec = 300s` + /// (default `stage1Replicates = 1`, `AutotuneCommand.swift:35-37`) + /// = 720s per candidate. + /// + /// **The App-side cannot assume a fixed candidate count.** The recommend + /// path in `AutotuneRecommend.benchmarks()` iterates every catalog row + /// filtered only by `runtime_status == "blocked"` and RAM/tier gates. + /// The set of admitted rows scales with `safetyMarginGB = 4` and the + /// user's Mac tier, so a 36GB machine may probe 4 rows and larger + /// machines can hit 5+. Compounded with 720s per candidate this + /// exceeds any small App-side guess. + /// + /// **This timeout is the App-side authoritative ceiling.** The CLI's + /// declared `AutotuneCommand.maxDuration = 7200s` at + /// `phase3-binary/Sources/macprovider-cli/AutotuneCommand.swift:47-48` + /// is only enforced by the non-recommend path (deadline created at + /// `AutotuneCommand.swift:157-161`). `runAutotuneRecommend()` at + /// `AutotuneCommand.swift:131-139` returns before that deadline is + /// installed and calls `AutotuneRecommendationBenchmarker().benchmarks()` + /// with no deadline / cancellation input. So this cap is not "CLI cap + + /// grace"; it IS the outer bound of autotune runtime, chosen by the + /// App. + /// + /// Value: 7260s = 2h1m. Rationale: + /// 1. Above any per-candidate math ceiling for reasonable N (up to + /// ~10 candidates × 720s = 7200s worst case). + /// 2. Well above the empirical 2-5 min median so the UX is never + /// affected in the healthy case. + /// 3. Aligned with the CLI's declared `maxDuration` constant so if + /// the recommend path is later fixed to enforce maxDuration + /// (see follow-up items in the PR body), the two ceilings + /// naturally coincide. + /// 4. Below 2.5h so a truly wedged subprocess doesn't trap the user + /// in a multi-hour spinner. + /// + /// Median UX is unaffected because autotune completes in 2-5 min. Only + /// a pathologically wedged run reaches this ceiling, at which point + /// the user has almost certainly already quit the app. + /// + /// Prior wrong values in this file's history: + /// * 30s (v1.8.3): ~240× too short; every fresh install failed at + /// `.autotuning` — see 2026-07-05 smoke report at + /// `/private/tmp/claude-501/.../scratchpad/smoke-v183/`. + /// * 1800s / 2700s (R1/R2 audit rounds): under-counted candidate + /// cardinality and per-candidate steps. + static let processTimeout: TimeInterval = 7260 private final class ProcessOutputBuffer: @unchecked Sendable { private let lock = NSLock() diff --git a/phase3-binary/app/Tests/MalibuTests/AutotuneRecommendationRunnerTimeoutTests.swift b/phase3-binary/app/Tests/MalibuTests/AutotuneRecommendationRunnerTimeoutTests.swift new file mode 100644 index 00000000..4e8eaa2e --- /dev/null +++ b/phase3-binary/app/Tests/MalibuTests/AutotuneRecommendationRunnerTimeoutTests.swift @@ -0,0 +1,85 @@ +import XCTest +@testable import Malibu + +/// Pins the wall-clock budget the App gives `macprovider-cli autotune +/// --recommend --json` on fresh-install onboarding. +/// +/// **Design decision (R3)**: this timeout is the App-side authoritative +/// ceiling for autotune runtime. The CLI's declared +/// `AutotuneCommand.maxDuration = 7200s` is NOT enforced on the +/// `--recommend` code path — `runAutotuneRecommend()` at +/// `phase3-binary/Sources/macprovider-cli/AutotuneCommand.swift:131-139` +/// returns before the deadline is created at :157-161, and +/// `AutotuneRecommendationBenchmarker.benchmarks()` runs with no +/// deadline / cancellation input. So the App cannot rely on a +/// CLI-enforced upper bound firing first. +/// +/// Given that, the App-side timeout is NOT derived from per-candidate +/// math (which under-counts on higher-tier Macs because `--recommend` +/// iterates every non-blocked RAM-eligible catalog row, count varies). +/// It IS bounded to a defensible range that survives realistic +/// worst-case candidate cardinality while remaining a UX-tolerable +/// ceiling. +/// +/// The 2026-07-05 v1.8.3 smoke shows `processTimeout = 30` killed +/// autotune ~4 minutes before completion on this Mac. Any App-side +/// value shorter than realistic per-machine autotune runtime will +/// reproduce that failure on some tier. +final class AutotuneRecommendationRunnerTimeoutTests: XCTestCase { + /// Realistic worst-case autotune runtime the App must budget for. + /// Rationale: per-candidate strict worst case is 720s + /// (`readyTimeoutSec = 120s` + prewarm `probeOnce = 300s` + + /// measured `probeOnce = 300s` per SPEC-023 v1.7.5). Catalog + /// cardinality after RAM/tier filtering scales with hardware; a + /// 10-candidate ceiling covers current catalog size with headroom. + /// 10 × 720s = 7200s. If either per-candidate math or catalog size + /// changes materially, revisit this floor. + private static let realisticWorstCaseAutotuneSec: TimeInterval = 7200 + + /// Above this, the user is in a multi-hour spinner and has almost + /// certainly quit the app. Beyond legitimate autotune runtime by + /// any reasonable interpretation. + private static let untenableSpinnerCeilingSec: TimeInterval = 2.5 * 60 * 60 + + func testProcessTimeoutCoversRealisticWorstCaseAutotune() { + // The App-side deadline MUST cover the realistic worst-case + // autotune runtime for the highest-tier hardware in the wild, + // because the CLI's --recommend path does NOT enforce its + // own declared maxDuration and cannot fail-fast on its own. + // If the App fires below this floor, healthy fresh installs + // on beefier Macs get killed mid-benchmark — that is the + // 2026-07-05 v1.8.3 smoke failure mode. + XCTAssertGreaterThanOrEqual( + AutotuneRecommendationRunner.processTimeout, + Self.realisticWorstCaseAutotuneSec, + """ + processTimeout=\(AutotuneRecommendationRunner.processTimeout)s \ + is below the realistic worst-case autotune runtime of \ + \(Self.realisticWorstCaseAutotuneSec)s (10 candidates × 720s \ + per candidate). Higher-tier Macs will hit .timedOut before \ + autotune converges. If per-candidate math or catalog size \ + has changed, update realisticWorstCaseAutotuneSec. + """ + ) + } + + func testProcessTimeoutIsNotUntenable() { + // Belt-and-suspenders: if the constant is ever set to some + // multi-hour value, the user is trapped in an indefinite + // spinner because the CLI has no independent way to give up + // (recommend path doesn't enforce maxDuration). Catch that + // before it ships. + XCTAssertLessThanOrEqual( + AutotuneRecommendationRunner.processTimeout, + Self.untenableSpinnerCeilingSec, + """ + processTimeout=\(AutotuneRecommendationRunner.processTimeout)s \ + exceeds \(Self.untenableSpinnerCeilingSec)s (2.5h). The CLI \ + --recommend path does not enforce its own maxDuration, so \ + this ceiling IS the outer bound. Beyond 2.5h the user has \ + given up. Introduce a heartbeat / progress protocol rather \ + than raise this further. + """ + ) + } +} diff --git a/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_ARCHITECT_PROMPT.md b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_ARCHITECT_PROMPT.md new file mode 100644 index 00000000..7f9398c4 --- /dev/null +++ b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_ARCHITECT_PROMPT.md @@ -0,0 +1,70 @@ +# AUDIT_FIX_AUTOTUNE_TIMEOUT — ARCHITECT lane (R5, final value) + +You are auditing PR `fix/autotune-timeout-progress` (commit `ea4f6c0`) +from the ARCHITECT lane. Round 5 refire because the final timeout +value is materially different from what R1 audited. + +## Value history for context + +R1 audited `processTimeout = 1800` (30 min). Convergence rounds +raised this through 2700 → 7260s. Final value: **7260s (2h1m)**. + +CODE lane discovered during convergence that the CLI's `--recommend` +path does NOT enforce its declared `maxDuration=7200s` (only the +non-recommend path installs a deadline at +`phase3-binary/Sources/macprovider-cli/AutotuneCommand.swift:157-161`). +So 7260s is the App-side authoritative ceiling, NOT a fallback under +a CLI-enforced cap. + +## Focus this round + +- Is 7260s (2h1m) the RIGHT App-side ceiling given the CLI has no + independent bound protecting the user? Or does 2h push the user + past spinner-fatigue territory where they'll quit the app before + autotune returns, making the value effectively useless? +- Should this PR ALSO wire `maxDuration` enforcement into the CLI + `--recommend` path so both ends have a bound, or is that + correctly deferred as follow-up? Consider the SPEC-026 §6.1 + state-machine contract: is `.autotuning → .failed(timedOut)` a + reasonable terminal for 2h1m, or should the copy be updated to + reflect the longer wait? +- The rationale comment + (`phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift:4-51`) + cites "10 candidates × 720s = 7200s worst case" as the floor + rationale. Is 10 candidates a defensible upper bound on catalog + cardinality, or should the App instead depend on a CLI-signaled + progress heartbeat (deferred)? +- The tests + (`phase3-binary/app/Tests/MalibuTests/AutotuneRecommendationRunnerTimeoutTests.swift`) + pin `realisticWorstCaseAutotuneSec = 7200s` and + `untenableSpinnerCeilingSec = 2.5h`. Are those constants named + well enough that a future dev bumping either can see the impact? +- The BUILD prompt scope-out named "heartbeat protocol on CLI + stderr" as follow-up. Now that we've iterated through 4 rounds, + is this PR the right shape to ship (bump-the-constant) or should + it evolve to also add a `runAutotune(timeout:)` injection point + for the future heartbeat work? +- Should the 2.5h ceiling really allow a 2.5h spinner, or is that + now just permission to defer building progress UI indefinitely? + +Do NOT recommend new coordinator surface or new SPECs. +Do NOT flag R2 CODE-M-2 (orphan child subprocess) — deferred to +CLI-side follow-up. + +## Referenced context + +Common context: `specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_COMMON.md`. + +## Output format + +Start with exactly one summary line: + +`VERDICT: READY | COUNTS: C=0 H=0 M=0 L=` + +or: + +`VERDICT: NEEDS REVISION | COUNTS: C= H= M= L=` + +Then list ID-prefixed findings, ordered by severity: `ARCH-C-1`, +`ARCH-H-1`, `ARCH-M-1`, `ARCH-L-1`, etc. Each finding must cite +the file:line and concrete evidence. diff --git a/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_CODE_PROMPT.md b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_CODE_PROMPT.md new file mode 100644 index 00000000..87df2b80 --- /dev/null +++ b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_CODE_PROMPT.md @@ -0,0 +1,43 @@ +# AUDIT_FIX_AUTOTUNE_TIMEOUT — CODE lane (R5, final) + +You are auditing PR `fix/autotune-timeout-progress` (commit `ea4f6c0`) +from the CODE lane. Round 5 sanity re-check after R4 CODE-L-1 wording +fix. + +## R4 finding recap + +- **R4 CODE-L-1**: LOW wording typo — rationale said "Below 10-candidate + × 720s worst case" but 7260 > 7200. Fixed in commit `ea4f6c0` to + say "Above". + +## R5 focus + +- Verify the typo fix at + `phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift:39` + and confirm no other inversions in the rationale comment. +- Sanity-check the final state — 91 tests pass, no new callers of + `AutotuneRecommendationRunner.processTimeout`, no drift in + Stage1Prober constants that would invalidate the 720s / 10-candidate + math cited in the rationale. + +Do NOT flag R2 CODE-M-2 (orphan child subprocess) — deferred. +Do NOT re-audit visibility. +Do NOT recommend progress UI or stderr tailing. + +## Referenced context + +Common context: `specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_COMMON.md`. + +## Output format + +Start with exactly one summary line: + +`VERDICT: READY | COUNTS: C=0 H=0 M=0 L=` + +or: + +`VERDICT: NEEDS REVISION | COUNTS: C= H= M= L=` + +Then list ID-prefixed findings, ordered by severity: `CODE-C-1`, +`CODE-H-1`, `CODE-M-1`, `CODE-L-1`, etc. Each finding must cite the +file:line and concrete evidence. diff --git a/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_COMMON.md b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_COMMON.md new file mode 100644 index 00000000..3dacaadd --- /dev/null +++ b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_COMMON.md @@ -0,0 +1,31 @@ +Branch: `fix/autotune-timeout-progress` (commit `ae23d48`). + +Context: fresh-install onboarding was 100% failing at `.autotuning` +because `AutotuneRecommendationRunner.processTimeout` was 30 seconds +but autotune actually needs 2-20 minutes on cold installs (real +model-load + prefill benchmarks). See: +- `phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift` + (bumped 30 → 1800s, added rationale comment) +- `phase3-binary/app/Tests/MalibuTests/AutotuneRecommendationRunnerTimeoutTests.swift` + (3 new tests pinning the invariant against CLI's own budgets) +- `phase3-binary/Sources/macprovider-cli/Stage1Iterator.swift:379-420` + (`Stage1Prober` with `readyTimeoutSec=120`, `probeIdleTimeoutSec=300`) +- Smoke evidence: + `/private/tmp/claude-501/.../scratchpad/smoke-v183/` (v1.8.3 timing + traces showing autotune manual run for 90+ seconds) + +Scope of THIS PR is strictly the timeout bump + tests. Progress UI +(tail stderr for `[warn] spec-023 probe:` messages) is deferred to a +follow-up. Do NOT recommend scope expansion — just audit what's here. + +Lock bar: 0 CRITICAL / 0 HIGH / 0 MEDIUM. + +Output format (per repo convention): + +Start with: +`VERDICT: READY | COUNTS: C=0 H=0 M=0 L=` +or: +`VERDICT: NEEDS REVISION | COUNTS: C= H= M= L=` + +Then ID-prefixed findings (severity-first). Each finding must cite the +file:line and concrete evidence. diff --git a/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_SECURITY_PROMPT.md b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_SECURITY_PROMPT.md new file mode 100644 index 00000000..6fbf4a6d --- /dev/null +++ b/specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_SECURITY_PROMPT.md @@ -0,0 +1,64 @@ +# AUDIT_FIX_AUTOTUNE_TIMEOUT — SECURITY lane (R5, final value) + +You are auditing PR `fix/autotune-timeout-progress` (commit `ea4f6c0`) +from the SECURITY lane. Round 5 refire because the final timeout value +is materially different from what R1 audited. + +## Value history for context + +R1 audited `processTimeout = 1800` (30 min). Convergence rounds +raised this through 2700 → 7260s. Final value: **7260s (2h1m)**. + +## Focus this round + +- Does the extended 2h1m subprocess lifetime introduce any new + credential / secret / Keychain-session TTL concern that the + original 30-min audit didn't cover? +- The R1 SEC-L-1 finding flagged unbounded `ProcessOutputBuffer` + (`phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift:33-50`) + as a local DoS/OOM hardening gap under a 30-min cap. Under a + 2h1m cap, is this still a LOW or does it graduate to MEDIUM + because a chatty stderr / stdout can accumulate more data? + Consider realistic autotune output volume (line-per-probe JSON + events, ~500 bytes/event × ~10 probes/candidate × ~10 candidates + = single-digit MB; not GB). +- `sanitizedProcessEnvironment` ( + `phase3-binary/app/Sources/Malibu/Onboarding/AutotuneRecommendationRunner.swift:126-128`) + filters env at spawn time. Does the 2h1m runtime give any window + for env variables to be re-introduced via a temp file / DYLD + path / plist read? Verify against `ProcessEnvironmentSanitizer` + behavior. +- On timeout at 7260s, `process.terminate()` sends SIGTERM. Does + half-finished autotune leave any partial keychain slot or + partial `config.yaml` write? (Note: onboarding state file + hardening is out of scope.) +- Is the raised timeout an amplification of any spoofed / compromised + `macprovider-cli` binary risk? At 30s an attacker had 30s to + exfil; at 7260s they have 2h. Is this materially worse or is + the code-signing check upstream (bundled CLI path) enough + mitigation? +- Do the new tests (`AutotuneRecommendationRunnerTimeoutTests`) + expose any private Malibu state beyond `@testable import Malibu` + that already exposed `processTimeout` at R1? + +Do NOT flag R2 CODE-M-2 (orphan child subprocess) — that is the +deferred CLI-side follow-up. +Do NOT expand scope beyond the timeout value + tests. + +## Referenced context + +Common context: `specs/AUDIT_FIX_AUTOTUNE_TIMEOUT_COMMON.md`. + +## Output format + +Start with exactly one summary line: + +`VERDICT: READY | COUNTS: C=0 H=0 M=0 L=` + +or: + +`VERDICT: NEEDS REVISION | COUNTS: C= H= M= L=` + +Then list ID-prefixed findings, ordered by severity: `SEC-C-1`, +`SEC-H-1`, `SEC-M-1`, `SEC-L-1`, etc. Each finding must cite the +file:line and concrete evidence.