fix(#652): reset shared stall timer on completed working rep#653
Closed
9thLevelSoftware wants to merge 1 commit into
Closed
fix(#652): reset shared stall timer on completed working rep#6539thLevelSoftware wants to merge 1 commit into
9thLevelSoftware wants to merge 1 commit into
Conversation
The shared stall countdown (WorkoutCoordinator.stallStartTime) is armed by both low-velocity motion and firmware DELOAD_OCCURRED, but until now was cancelled only by a metric above 10 mm/s. PR #650 cleared the verbal-cue defer deadline at the completed-rep boundary on the same premise: a completed working rep is authoritative proof the athlete is back in motion. Without resetting the stall timer at that boundary, a countdown started at a turnaround, brief pause, or firmware de-load could survive a subsequent valid rep and later triggerAutoStop() -> handleSetCompletion(), prematurely ending a set the athlete is still performing. Add resetStallTimer() alongside deferAutoStopDeadlineMs = 0L inside the repCountAfter > repCountBefore branch of handleRepNotification. This is the narrow shared fix for both stall arms and preserves issue #256: a pending first working rep has not completed, so its timer remains armed and the existing 5-second stall guard still fires. TDD evidence: - RED characterization (Issue 652 test) reproduced on main@0f00f4b: stallStartTime remained set after a completed second working rep. - GREEN after the fix; full :shared:testAndroidHostTest suite (2442 tests) and issue #256 tests stay green. No thresholds, setting semantics, AMRAP position auto-stop, VBT Auto-End, or unrelated code changed. Stall Detection remains independent from autoEndOnVelocityLoss. Fixes #652
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses Issue #652 by ensuring that a completed rep boundary resets the stall timer in ActiveSessionEngine. This prevents stale countdowns from surviving subsequent valid reps and incorrectly auto-completing the set. A corresponding unit test has been added to DWSMWorkoutLifecycleTest to verify this behavior. There are no review comments to address, so I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Contributor
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. Verdict: Approve | One-line fix at the right boundary, uses the existing Correctness / Safety Findings Ponytail Review Ponytail net: 0 lines. Suggested Minimal Patch Final Merge Guidance 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 23.6K · Output: 1.5K · Cached: 108.5K |
9thLevelSoftware
pushed a commit
that referenced
this pull request
Jul 14, 2026
- F1: ignore DELOAD_OCCURRED in Echo mode — Echo levels are defined by the firmware's deload window, so the event fires routinely mid-set and must not arm the 5s auto-stop stall timer (root cause of sets force-ending at 6/3 reps on Harder/Hardest). - #652/#653 absorbed: a completed working rep resets the shared stall timer in handleRepNotification (supersedes PR #653). - F3: startWorkout() resets biomechanics engine, VBT one-shot flags, and rep boundary timestamps at every set start — the Phase 35C variable warm-up fast path bypassed handleSetCompletion's reset block, leaking the warm-up set's velocity baseline into the working set. - F4: velocity-armed stall countdowns now require handles in use (maxPosition > STALL_MIN_POSITION) to arm and to continue, re-checked per sample — a racked pause between reps no longer ends a standard set. New stallArmedByDeload flag exempts deload-armed countdowns (genuine cable release retracts to ~0mm and must still auto-stop); a deload upgrades an existing velocity-armed countdown. - F7: deferAutoStopDeadlineMs moved to WorkoutCoordinator (@volatile); WorkoutCoordinator.resetAutoStopState() is the single reset for all auto-stop/stall/defer fields; ASE and RFM delegate to it. - F8: releasing the handles during the verbal-cue defer window clears the deadline so auto-stop resumes promptly instead of waiting out 30s. Regression tests: Echo deload ignored / Echo velocity stall kept, Issue 652 rep-cancels-stall, F3 set-start reset, 4x F4 arm-source scenarios, F8 clear-on-release, coordinator reset unit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq
9thLevelSoftware
pushed a commit
that referenced
this pull request
Jul 14, 2026
F2 and F9 withdrawn (totalReps excludes warm-up reps; stall fields already @volatile), F5 resolved via #653 absorb + F4 continuation check, and per-finding resolution status for the fixes on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq
Owner
Author
|
Rolled into #654 |
9thLevelSoftware
added a commit
that referenced
this pull request
Jul 14, 2026
…ed pauses safe, VBT baseline leak closed (e2e audit + fixes) (#654) * docs: e2e audit of stall detection / auto-stop pipeline Follow-up to #652 / PR #653 and the 2026-07-14 Echo session report (sets force-ended at 6 and ~3 reps on Harder/Hardest with VBT auto-end disabled). Nine findings, ranked. Root cause of the report: DELOAD_OCCURRED is a routine firmware event in Echo mode (Echo levels are defined by deload windows) but is treated as cable release and arms the 5s auto-stop stall timer (F1). Compounding: VBT velocity-loss baseline contaminated by firmware warm-up reps (F2), Phase 35C warm-up-set early return skips biomech/VBT resets (F3), global stall-detection toggle does not govern routine sets (F6). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq * fix: stall-detection audit fixes F1/F3/F4/F7/F8 + absorb #653 - F1: ignore DELOAD_OCCURRED in Echo mode — Echo levels are defined by the firmware's deload window, so the event fires routinely mid-set and must not arm the 5s auto-stop stall timer (root cause of sets force-ending at 6/3 reps on Harder/Hardest). - #652/#653 absorbed: a completed working rep resets the shared stall timer in handleRepNotification (supersedes PR #653). - F3: startWorkout() resets biomechanics engine, VBT one-shot flags, and rep boundary timestamps at every set start — the Phase 35C variable warm-up fast path bypassed handleSetCompletion's reset block, leaking the warm-up set's velocity baseline into the working set. - F4: velocity-armed stall countdowns now require handles in use (maxPosition > STALL_MIN_POSITION) to arm and to continue, re-checked per sample — a racked pause between reps no longer ends a standard set. New stallArmedByDeload flag exempts deload-armed countdowns (genuine cable release retracts to ~0mm and must still auto-stop); a deload upgrades an existing velocity-armed countdown. - F7: deferAutoStopDeadlineMs moved to WorkoutCoordinator (@volatile); WorkoutCoordinator.resetAutoStopState() is the single reset for all auto-stop/stall/defer fields; ASE and RFM delegate to it. - F8: releasing the handles during the verbal-cue defer window clears the deadline so auto-stop resumes promptly instead of waiting out 30s. Regression tests: Echo deload ignored / Echo velocity stall kept, Issue 652 rep-cancels-stall, F3 set-start reset, 4x F4 arm-source scenarios, F8 clear-on-release, coordinator reset unit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq * refactor(F6): remove dead global stallDetectionEnabled preference The global UserPreferences.stallDetectionEnabled was consumed by no workout path and had no wired UI toggle — every workout honors the per-exercise RoutineExercise.stallDetectionEnabled or the Just Lift per-session toggle. Its only effect was incorrectly gating the 'Auto-End on Velocity Loss' row in Settings, implying a global control that did not exist. Removed: the preference field, persistence key/load/setter (PreferencesManager), SettingsManager/MainViewModel passthroughs, the SettingsTab parameter and gating (auto-end row is now always enabled), and the fake's override. JustLiftDefaults.stallDetectionEnabled and the per-exercise flag are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq * docs: audit corrections & resolutions addendum F2 and F9 withdrawn (totalReps excludes warm-up reps; stall fields already @volatile), F5 resolved via #653 absorb + F4 continuation check, and per-finding resolution status for the fixes on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq * fix: remove comma from test name — Kotlin/Native forbids ',' in backticked identifiers The iOS Test Target Compile CI job failed on WorkoutCoordinatorAutoStopResetTest with 'Name contains illegal characters: ","'. JVM targets accept commas in backticked test names; Kotlin/Native does not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ee4mJkzUkJTbwEUvMCSSaq --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Fixes #652
Root cause
ActiveSessionEngine.handleRepNotificationalready treats a completed working rep as authoritative proof of resumed motion (PR #650) and zeroesdeferAutoStopDeadlineMsat that boundary. The sharedWorkoutCoordinator.stallStartTimecountdown, armed by both low-velocity motion and firmwareDELOAD_OCCURRED, was never reset at the same boundary. It was cancelled only when a later sample exceededSTALL_VELOCITY_HIGH(10 mm/s), so a countdown started at a turnaround, brief pause, or firmware de-load could survive a subsequent valid rep and later callrequestAutoStop()->triggerAutoStop()->handleSetCompletion(), prematurely ending a set the athlete is still performing.Fix
In
ActiveSessionEngine.handleRepNotification(), insideif (repCountAfter > repCountBefore), callresetStallTimer()immediately alongsidedeferAutoStopDeadlineMs = 0L. One shared reset covers both stall arms because both write to the samecoordinator.stallStartTime.TDD evidence
Issue 652 - completed Echo rep cancels an armed stall timerinDWSMWorkoutLifecycleTestfailed withexpected:<null> but was:<1783989200633>.:shared:testAndroidHostTestsuite: 2442 tests, 0 failures, 0 errors, 0 skipped.deload starts stall timer even with pending rep,velocity stall auto-stops with pending rep after timer expires) remain GREEN.What did NOT change
shouldRunPositionBasedAutoStop).autoEndOnVelocityLoss).ActiveSessionEngine.ktandDWSMWorkoutLifecycleTest.kt.Issue #256 protection is preserved: a pending first working rep has not completed, so its timer remains armed and the existing 5-second stall guard still fires. Stall Detection remains independent from
autoEndOnVelocityLoss- no new VBT/stall coupling introduced.Files
shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt- production fix (one newresetStallTimer()call).shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/DWSMWorkoutLifecycleTest.kt- new regression test (RED characterization preserved from the RCA artifact).Audited baseline:
origin/main@0f00f4b28856bfc3dd6536340390ec01b2c3ea03RCA: #652 (comment)