Audit: completion inferred from zero-crossing — explicit didComplete signal + tick guards in TimerCoordinator

[wuersch]

Severity: Low · Audit findings 14, 15, 17 · Suggested branch: fix/timer-completion-signal

Problem (architectural)

Workout completion is inferred from observed values rather than signalled by the model: TimerLifecycleModifier watches totalTimeRemaining cross >0 → ==0 and fires the completion path (confetti, workout log). Any code path that can make remaining hit zero — a canonical pause clamp, a stray post-pause tick — accidentally "completes" the workout. The clean fix is an explicit didComplete signal set only inside the model's update(), which also de-fangs the tick-lifetime issues below.

Findings

• (14, Low) Kraftli Timers/Features/Timer/AMRAP/AMRAPTimerModel.swift:89 — pause(at:) can clamp totalTimeRemaining to exactly 0 when the canonical HK transition date lands at/after the natural end; TimerLifecycleModifier.swift:61-70 treats the crossing as completion → logs and celebrates a workout the user just paused, while the Watch sits paused with fractions of a second left. (Dissent: the ~60 Hz tick almost always completes first; window practically tiny.)
• (15, Low) Kraftli Timers/Features/Timer/Shared/TimerCoordinator.swift:61 — each tick is an unstructured Task { @MainActor }; invalidating the timer in stop() can't cancel an already-enqueued Task, the models never clear their start anchor on pause/reset, and update() has no isRunning guard. A stray tick recomputes the display from the local clock after pause(at:) — undoing the anchored freeze for the whole pause — and near boundaries can fire onIntervalComplete/onWarning/the completion branch on a paused timer.
• (17, Low) Kraftli Timers/Features/Timer/Shared/TimerCoordinator.swift:86 — deinit { stop() } calls MainActor-isolated stop() from a nonisolated deinit. The app targets set SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor with Swift 5 language mode, so this compiles as a warning, but invalidating a main-run-loop Timer/CADisplayLink from whatever thread drops the last reference violates the Timer API contract. In practice the coordinator is owned by view-held models released on the main thread, so the risk is theoretical today. (Verified manually — the workflow verifiers for this finding timed out.)

Fix direction

Model-owned didComplete signal set only in update() (or require isRunning at the zero-crossing); guard update() on isRunning or clear the start anchor in pause/pause(at:)/reset; address the deinit isolation opportunistically (e.g. isolated deinit or ensuring the timer is stopped before release).