fix: defer AMRAP/stall auto-stop after verbal VBT cue when autoEndOnVelocityLoss=false (#649)#650
Conversation
…elocityLoss=false (#649) When the user has VBT Auto-End on Velocity Loss disabled but verbal encouragement enabled, AMRAP position-based (2.5s) and velocity-stall (5s) auto-stop timers remained independent of the verbal cue and could call handleSetCompletion() mid-playback, cutting off the cue and ending the AMRAP set prematurely. Add a single-set guard: when VERBAL_ENCOURAGEMENT is emitted with autoEndOnVelocityLoss=false, defer both AMRAP position and velocity-stall auto-stop timers until the next completed working rep. Cleared by the next completed working rep or at the per-set reset boundary. The VBT two-consecutive-rep auto-end branch (gated on autoEndOnVelocityLoss) is untouched. RCA: #649 (comment) Fixes #649
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses Issue #649 by introducing a mechanism to defer position and stall auto-stops after a verbal VBT cue is emitted with autoEndOnVelocityLoss disabled, resetting the deferral once the next working rep is completed. It also adds a comprehensive suite of unit tests to verify this state machine logic. The review feedback highlights a potential concurrency issue where the newly introduced deferAutoStopUntilNextWorkingRep flag is accessed across different threads, recommending the addition of the @volatile annotation to ensure proper cross-thread visibility.
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.
| // Issue #649: defer position/stall auto-stop until the next completed working | ||
| // rep after a verbal VBT cue was emitted with autoEndOnVelocityLoss=false. | ||
| // ponytail: single boolean; cleared on next completed working rep + per-set reset. | ||
| private var deferAutoStopUntilNextWorkingRep = false |
There was a problem hiding this comment.
Since deferAutoStopUntilNextWorkingRep is written on Dispatchers.Default (inside checkVelocityThreshold) and read on the metrics collection thread (inside checkAutoStop), it is accessed concurrently across different threads. Without the @Volatile annotation, thread-local caching can prevent the metrics collection thread from immediately seeing the updated value, potentially causing premature auto-stops during verbal VBT cues. Marking the field as @Volatile ensures proper cross-thread visibility and avoids data races.
| // Issue #649: defer position/stall auto-stop until the next completed working | |
| // rep after a verbal VBT cue was emitted with autoEndOnVelocityLoss=false. | |
| // ponytail: single boolean; cleared on next completed working rep + per-set reset. | |
| private var deferAutoStopUntilNextWorkingRep = false | |
| // Issue #649: defer position/stall auto-stop until the next completed working | |
| // rep after a verbal VBT cue was emitted with autoEndOnVelocityLoss=false. | |
| // ponytail: single boolean; cleared on next completed working rep + per-set reset. | |
| @kotlin.concurrent.Volatile | |
| private var deferAutoStopUntilNextWorkingRep = false |
References
- Prefer using a
@Volatileclass field directly instead of introducing a wrapper holder class (likeVolatileRef) to address multi-threaded visibility/data race concerns. This avoids unnecessary indirection while providing the same happens-before guarantee and maintaining consistency with existing field patterns.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72b8952353
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (deferAutoStopUntilNextWorkingRep) { | ||
| resetStallTimer() | ||
| resetAutoStopTimer() | ||
| return |
There was a problem hiding this comment.
When autoEndOnVelocityLoss is false and the verbal encouragement fires, this flag remains set until another working rep is completed; if the athlete instead racks the handles or fails immediately after the cue, every subsequent metric takes this branch and resets both timers, so AMRAP/timed/Just Lift position and stall auto-stop can never finish the set. Consider expiring the defer after the cue/grace window or allowing release/stall auto-stop after a short delay rather than requiring another completed rep.
Useful? React with 👍 / 👎.
Code Review Roast 🔥Verdict: Comment only | Recommendation: Prior warning + 4 of 5 Codex P2s resolved; acknowledged race at the arming site stands and is already commented inline; optional test cleanup welcome. Overview
Issue Details (click to expand)
🏆 Best part: The single-source-of-truth refactor is exactly the move. Collapsing flag+deadline into one 💀 Worst part: There's nothing mean to roast in this incremental — the worst part is having nothing mean to roast. The acknowledged race at the arming site (line 1370) remains the only standing concern, and it's already pinned by Codex P2 inline. 📊 Overall: Like a kernel bisect that lands on the commit you already trusted — quieter than the prior round, and that's a compliment. Prior review findings — re-verified
Ponytail pass
Ponytail net: -10 lines (delete the redundant test at 163-171). Correctness / Safety FindingsNo new correctness, safety, security, regression, or test issues introduced by this incremental diff. The acknowledged race at Files Reviewed (2 files, incremental diff only)
Previous Review Summaries (3 snapshots, latest commit d59b44e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d59b44e)Verdict: Request changes | Recommendation: Drop line 1463's flag re-assertion and zero the deadline at line 1106 before merge. Overview
Issue Details (click to expand)
🏆 Best part: The source-of-truth refactor ( 💀 Worst part: The new re-assertion at line 1463 is the architectural equivalent of installing a deadlock-prevention mutex that deadlocks. It claims the deadline is the gate, then re-derives the flag every metric — and that re-derivation undoes the only line (1106) that's supposed to release the defer on a completed working rep. The deadline is also never zeroed at line 1106 (still flagged by Codex P2), so even removing the re-assertion wouldn't fully restore the "clear on next working rep" contract until that pair is fixed together. 📊 Overall: Like buying a fancy new deadbolt and then taping the key to the doorframe. The intent is right; the wiring is fighting itself. Prior review findings — re-verified
Ponytail pass
Ponytail net: -3 lines (delete line 1463; net zero elsewhere). Correctness / Safety Findings
Files Reviewed (2 files, incremental diff only)
Fix these issues in Kilo Cloud Previous review (commit 1e4c0c3)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental update is still clean. I had to put the flamethrower back in storage. Twice. 🏆 Best part: Both prior review findings (Gemini P1 💀 Worst part: I went looking for trouble in the deadline-expiry branch ( 📊 Overall: Like a sequel that's better than the original — the original was already a haiku, and now it's a haiku with a safety net. Files Reviewed (2 files, incremental diff only)
Prior review findings — re-verified on incremental diff
Ponytail passPonytail: Lean already. Ship. Ponytail net: 0 lines. Correctness / Safety FindingsNo correctness or safety findings. Deadline-expiry logic at L1446-1454 is correct: timers reset every metric during defer, so when the flag clears the auto-stop path enters with a fresh timer baseline — no stale-timestamp surprise. The Previous review (commit 72b8952)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. 🏆 Best part: The state machine is a single boolean, one early-return guard, and three reset sites — that's not a fix, that's a haiku written in Kotlin. Even the comment budgets its own line count. 💀 Worst part: I went looking for trouble and found none. Refusing to manufacture some just to feel useful. 📊 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: 47K · Output: 13K · Cached: 226.6K |
- Gemini review: deferAutoStopUntilNextWorkingRep is written on Dispatchers.Default (checkVelocityThreshold) and read on the metrics collection thread (checkAutoStop). Marked @volatile so the metrics thread immediately sees the armed flag and avoids a premature auto-stop window starting before the cue's audio finishes. - Codex review (P2): if the athlete racks mid-set after the cue and never completes another working rep, the original fix would have suppressed AMRAP/stall auto-stop forever. Added a 30s deadline (VERBAL_ENCOURAGEMENT_DEFER_WINDOW_MS) — cue + short post-cue transition window — that auto-clears the defer so a legitimately racked mid-set handle still ends the set normally. Per-set reset sites and the next completed working rep continue to clear it. Tests: VerbalEncouragementAutoStopDeferTest now 6/6 (added within-window active + deadline expiry). ./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true -> 2,426 tests pass, 0 failures, 0 errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e4c0c30db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| deferAutoStopUntilNextWorkingRep = true | ||
| deferAutoStopDeadlineMs = currentTimeMillis() + VERBAL_ENCOURAGEMENT_DEFER_WINDOW_MS |
There was a problem hiding this comment.
When the verbal cue arms the defer, checkAutoStop() can run on the metrics thread between these two volatile writes. In that interleaving it observes deferAutoStopUntilNextWorkingRep == true while deferAutoStopDeadlineMs is still the old 0, treats the defer as already expired, and clears the flag, so the AMRAP/stall timers are not deferred after the cue. Set the deadline before publishing the flag, or update them atomically.
Useful? React with 👍 / 👎.
| coordinator.biomechanicsEngine.reset() | ||
| velocityThresholdAlertEmitted = false | ||
| consecutiveThresholdReps = 0 | ||
| deferAutoStopUntilNextWorkingRep = false |
There was a problem hiding this comment.
Clearing this flag only in a few higher-level paths misses reset paths that call resetAutoStopState() directly, such as stopAndReturnToSetReady() / skip flows and the startWorkout() reset. If a user stops or skips shortly after the verbal cue and starts another set before the 30s deadline, the stale flag carries into that new active set and checkAutoStop() suppresses AMRAP/stall auto-stop there. The defer state should be reset inside resetAutoStopState() so every new-set reset clears it consistently.
Useful? React with 👍 / 👎.
Two Codex P2 review follow-ups: 1) Cross-thread publish ordering — two @volatile fields (flag, deadline) don't chain happens-before across the JMM, so a metrics-thread reader could observe flag=true with a stale 0L deadline and clear the defer. Refactored to a single source of truth: deferAutoStopDeadlineMs == 0L means no defer. Reader derives the flag from the deadline and never sees 'flag=true && deadline=0L'. Writer still publishes deadline first when arming for clarity. 2) Reset path coverage — the defer flag was cleared only in per-set reset paths; resetAutoStopState() is the canonical reset hub and is called by stopAndReturnToSetReady() / startWorkout() / skip flows. If a user stops or skips shortly after the cue, the stale flag could carry into the next active set and suppress AMRAP/stall auto-stop there. Moved the defer-clear into resetAutoStopState() so every reset path covers it; existing per-set clears left as defensive belt-and-suspenders. Tests: VerbalEncouragementAutoStopDeferTest 8/8 (added reset-paths and arm-publishes-deadline). ./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true -> 2,428 tests pass, 0 failures, 0 errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d59b44e0cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (repCountAfter > repCountBefore) { | ||
| // Issue #649: a completed working rep proves the user is back in motion; | ||
| // let normal AMRAP / stall auto-stop resume. | ||
| deferAutoStopUntilNextWorkingRep = false |
There was a problem hiding this comment.
When a verbal cue is armed and the athlete completes another working rep before the 30s window expires, this only clears deferAutoStopUntilNextWorkingRep; however checkAutoStop() now treats deferAutoStopDeadlineMs != 0L as the source of truth and returns until that deadline. In that scenario AMRAP/stall auto-stop remains suppressed for the rest of the cue window despite the intended “next completed working rep” release, so the deadline should be zeroed here as well.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| deferAutoStopUntilNextWorkingRep = false | ||
| deferAutoStopDeadlineMs = 0L | ||
| } else { | ||
| deferAutoStopUntilNextWorkingRep = true |
There was a problem hiding this comment.
🔥 The Roast: The new checkAutoStop comment above literally says "Source of truth is the deadline field — flag is derived." — and then on the very next metric while defer is active, it helpfully re-asserts the flag back to true. That's not defensive programming, that's the flag and the deadline arguing over who's driving. Worse: this re-assertion actively undoes the working-rep clear at line 1106 (which the existing Codex P2 still flags as unfixed because the deadline isn't zeroed there). Net effect — the user completes a working rep at t=10s, line 1106 sets flag=false, the next metric at t=10.05s reads deadline=35_000L, drops into this else branch, sets flag=true again, and the defer is back. The "cleared on next completed working rep" contract advertised in the comment is a lie until either line 1106 zeros the deadline or this re-assertion goes away.
🩹 The Fix: Delete line 1463. The deadline is the gate; the flag doesn't need re-derivation every metric. Pair with zeroing deferAutoStopDeadlineMs = 0L at line 1106 (the existing Codex P2 there) so the working-rep clear is actually observable to checkAutoStop's if (deferDeadline != 0L) predicate.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Codex P2 + Kilo review both flagged the same bug: the working-rep clear at line 1106 only zeroed the flag but not the deadline field. The metrics-thread read site then re-derived 'flag=true' from the still-nonzero deadline and silently re-armed the defer, so the 'cleared on next completed working rep' contract was a lie. Removed the derived flag entirely. The @volatile Long deadline is the only source of truth; checkAutoStop() gates on 'deadline != 0L' so there is nothing to re-derive. Every reset path (next completed working rep, resetAutoStopState, per-set boundaries, deadline expiry) zeros the same field. Net effect in production: -29 lines, one @volatile Long, one const ceiling, no flag/deadline synchronization to maintain. Tests: VerbalEncouragementAutoStopDeferTest 8/8 (rewritten to mirror the single-source-of-truth pattern; explicit tests pin arm-with-future-deadline, working-rep-clears-regardless-of-deadline, expiry, resetAutoStopState, non-arm cases). ./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true -> 2,428 tests pass, 0 failures, 0 errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34897f5db0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // alone is the source of truth — no derived flag to keep in | ||
| // sync. | ||
| if (!coordinator.autoEndOnVelocityLoss) { | ||
| deferAutoStopDeadlineMs = currentTimeMillis() + VERBAL_ENCOURAGEMENT_DEFER_WINDOW_MS |
There was a problem hiding this comment.
When biomechanics work from a completed set runs late, this assignment can publish a fresh 30s defer after startWorkout() has already reset state for the next set. processBiomechanicsForRep() launches on the shared scope and checkVelocityThreshold() does not verify that the analyzed rep still belongs to the current set, so in a quick set transition/autostart path an old verbal-cue callback can make the new active set's checkAutoStop() suppress AMRAP/stall auto-stop until the timeout or another rep clears it. Capture and validate a set/session generation before arming the deadline; a simple Active-state check is not enough once the next set has started.
Useful? React with 👍 / 👎.
…t transition Codex P2 (22:28): a stale processBiomechanicsForRep() callback resumed from Dispatchers.Default after startWorkout() reset state for the next set could write a fresh 30s defer into the new active set's checkAutoStop predicate. Note the underlying race also affects the existing velocityThresholdAlertEmitted flag (same call site); a full fix needs processBiomechanicsForRep() to verify the analyzed rep belongs to the current set, which is out of scope for the #649 fix. Narrow the window for the defer specifically: re-read coordinator._workoutState.value inside the arm guard and skip the arm if the workout has already left Active (manual stop / set transition). If the arm happens to win the race, the next completed working rep will clear the defer within a rep or two; or it expires in 30s. Either way the worst case is a bounded grace window on the next set. Tests: ./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true -> 2,428 tests pass, 0 failures, 0 errors.
1 participant
Summary
When the user has VBT Auto-End on Velocity Loss disabled but verbal encouragement enabled, the AMRAP position-based (2.5s) and velocity-stall (5s) auto-stop timers remained independent of the verbal VBT cue. After VBT triggered the verbal cue, the same cue's low-velocity window could start either timer; ~2.5–5s later the engine called
handleSetCompletion(), cutting the verbal cue short and emittingWORKOUT_END. The user's AMRAP set ended mid-rep, with the set-completion audio cue firing right after the truncated encouragement.This fix adds a per-set defer guard on the active session engine:
checkVelocityThreshold()emitsVERBAL_ENCOURAGEMENTwhileautoEndOnVelocityLoss=false, setdeferAutoStopUntilNextWorkingRep = trueand reset both timers.checkAutoStop()early-returns (resetting both timers defensively) while the defer is active.autoEndOnVelocityLoss=true) is untouched.Root cause
ActiveSessionEngine.checkVelocityThreshold()correctly gates the two-consecutive-rep auto-end oncoordinator.autoEndOnVelocityLoss. AMRAP position auto-stop (shouldRunPositionBasedAutoStop(params) → handlesCompletelyAtRest → requestAutoStop()after 2.5s) and velocity-stall (maxVelocity < STALL_VELOCITY_LOW → requestAutoStop()after 5s when enabled) did not consult that preference or the verbal-cue one-shot, so any "verbal cue in flight + user pauses" window could end the set early.handleSetCompletion()then stops BLE and emitsWORKOUT_END, producing the overlapping end cue the reporter observed.Fix direction (from GPT-5.5 RCA)
When
checkVelocityThreshold()emitsVERBAL_ENCOURAGEMENTwhileautoEndOnVelocityLossis false, mark the current set as auto-stop-deferred-until-next-completed-working-rep. While that marker is set,checkAutoStop()must reset both position and velocity-stall countdown state and return before either can callrequestAutoStop(). Clear the marker only after a later completed working rep, and reset it with the existing per-set reset path.ponytail: single boolean + one early-return guard + three reset sites. Skipped: a tunable "encouragement duration" timeout (the next-rep boundary avoids guessing the cue's actual duration). Add when users report legitimate pauses being ended by this defer.
Acceptance criteria coverage
isAMRAP=true, warmup complete,autoEndOnVelocityLoss=false, and a verbal VBT event, stationary metrics past both 2.5s and 5s stay Active with no BLE stop orWORKOUT_END.autoEndOnVelocityLoss=true) retains the two-consecutive-threshold-rep behavior (defer never arms).Tests
./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true— 2,424 tests pass, 0 failures, 0 errors across 230 test classes.New:
VerbalEncouragementAutoStopDeferTest(4 tests) covers the guard logic in the same tracker-mirror style asVbtAutoEndTestandVerbalEncouragementGateTest. ExistingDWSMWorkoutLifecycleTest(63),VbtAutoEndTest(9), andVerbalEncouragementGateTest(7) all still pass.Notes
autoEndOnVelocityLosssetting semantically intact (still the user-controlled VBT switch) and adds no new preference.Fixes #649