BUG: Completed reps logged as one short when ROM reps precede working set

[9thLevelSoftware]

Bug Report

Type: Bug
Reporter: shredness_10860
Device: Pixel 9 XL Pro
OS: Android v17
App Version: 0.9.6 (post AMRAP fix)
Repo: Mobile (project-phoenix-mp)

Summary

Completed reps are logged as one short of actually completed. When a user completes ROM reps followed by a working set, the final rep count in the log is off by one.

Steps to Reproduce

1. Select any exercise
2. Set reps to 7
3. Start exercise
4. Complete first 3 ROM reps
5. Complete the working set of 7 reps
6. Review log

Expected Behavior

7 reps should be recorded in the workout log.

Actual Behavior

Only 6 reps are recorded in the workout log.

Impact

Rep counting is inaccurate for exercises with ROM reps followed by working sets. This affects workout tracking accuracy and could impact user progress records. The issue appears to be an off-by-one error in rep counting logic, possibly related to how ROM reps and working set reps are tallied.

Notes

• Reporter mentions this is "post AMRAP fix" — may be a regression from recent AMRAP-related changes
• Affects any exercise when ROM reps are completed before the working set

---

Original Discord Report

<@1502654170031849644>  Completed reps are logged as one short of actually completed.

Device: Pixel 9 XL Pro
OS: Android, v17
App: Version 0.9.6 (post AMRAP fix)

steps to reproduce:
-select any exercise, set reps to 7.
-start exercise, complete first 3 ROM reps, complete the working set of 7 reps.
-review log, only 6 reps recorded.

[9thLevelSoftware]

Reporter clarification requested (2026-08-17)

RCA research for #703 traced the rep-counting path in RepCounterFromMachine.processModern() and RepNotificationFreshnessGate.evaluate() against commit 14fddb2 (post-PR #701 "AMRAP fix"). The app code is correct for any valid BLE stream — workingReps = repsSetCount (line 576) is straight-through, the freshness gate stays Armed during working reps, and compareAndSet in handleSetCompletion prevents duplicate teardown. PR #701 (AMRAP fix) does not change non-AMRAP finite-set behavior, so the "post AMRAP fix" framing is temporal context, not a code-level regression.

The bug must originate either on the BLE delivery boundary (last-rep packet lost between the machine and the phone) or in firmware-side semantics (repsSetCount indexing). Before opening a code RCA comment, I've asked the reporter two triage questions in the Discord thread to distinguish "lost on the way" vs "never sent":

1. Is the off-by-one every set, sometimes, or only at specific rep counts?
2. Does the on-screen rep counter momentarily show the correct count (e.g., 7) and then drop on save, or is it already wrong on screen?

Answers will land in the Discord thread and be appended to the ticket state on the next bridge tick. A full RCA + code-side fix recommendation will follow once we know which side of the wire the count is being lost on.

[9thLevelSoftware]

RCA: Completed reps logged one short when ROM reps precede working set (issue #703)

Recreation status

/Users/christopherwilloughby/.hermes/phoenix-bug-recreations/703/bug-recreation.md
(RCA readiness: READY — code-path analysis on commit 14fddb2. Live BLE reproduction was not possible; no physical Vitruvian machine or Pixel 9 XL Pro was available.)

Reporter clarification used

Discord reply 1538887584623763516 (captured by the RCA research stage):

• "began post AMRAP fix" is temporal context, not a code-level regression. PR fix: exempt AMRAP leases from rep freshness gate (#700) #701 (57d47d48) leaves the standard finite-set path of RepNotificationFreshnessGate identical — targetMatches and the terminal check are unchanged when isJustLift=false && isAmrap=false.
• The on-screen rep counter reaches 7 and then the set auto-completes. AnimatedRepCounter fires showingCelebration = 7 (line 110) only when confirmedReps advances, so the in-memory repCounter.workingReps == 7 at the moment of completion is provable.
• The persisted WorkoutSession.workingReps field is 6, not 7. Only the snapshot path is wrong.

This eliminates the recreation-stage Hypotheses 1 (firmware 0-indexed repsSetCount) and 2 (BLE packet loss on the final rep) — both would also corrupt the live counter. PR #701 (Hypothesis 4) is also eliminated by diff inspection: the gate's behavior for standard finite sets is byte-for-byte unchanged.

Root cause

A lifecycle race in ActiveSessionEngine: coordinator._repCount.value is written at the bottom of handleRepNotification, after the synchronous repCounter.process(...) has already fired the WORKOUT_COMPLETE RepEvent. Because the RepEvent callback dispatches handleSetCompletion through scope.launch (non-blocking), the launched coroutine can run before the write on line 1334 executes. The snapshot path therefore reads a one-rep-stale _repCount.value.

Trace (commit 14fddb2):

1. ActiveSessionEngine.handleRepNotification — shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:1319
repCounter.process(repsSetCount=7, …) is called.
2. RepCounterFromMachine.processModern — shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/usecase/RepCounterFromMachine.kt:562-606
   • Line 576: workingReps = repsSetCount (= 7).
   • Line 597: shouldStop = true.
   • Line 598-604: onRepEvent?.invoke(RepEvent(WORKOUT_COMPLETE, workingCount=7)).
3. ActiveSessionEngine.repCounter.onRepEvent callback — ActiveSessionEngine.kt:464-538
   • Line 466: scope.launch { … } (queues handleSetCompletion).
   • Line 534: handleSetCompletion(eventLease) runs inside the launched coroutine.
4. coordinator._repCount.value write — ActiveSessionEngine.kt:1334 — only happens after process() returns.

The launched coroutine on step 3 is non-blocking. The scheduler can run it before line 1334 executes. When it does, handleSetCompletion → captureExitSnapshot (ActiveSessionEngine.kt:4727 → 3437) reads _repCount.value whose workingReps is still 6 from the previous notification, and persists that into WorkoutSession.workingReps (ActiveSessionEngine.kt:3504) and CompletedSet.actualReps (:3547).

The user sees "7" because AnimatedRepCounter.showingCelebration = 7 is driven directly from the repCounter.getRepCount() snapshot that WorkoutHud composes, and because the celebration fires synchronously during process() (before the race). The persistence layer reads the StateFlow, which is one tick behind.

A second, weaker trigger path exists at handleMonitorMetric (ActiveSessionEngine.kt:1682-1684); it is less likely to win because line 1666 writes _repCount.value before line 1682 in the same function — but it can still observe a stale value on the very first metric tick after shouldStopWorkout() flips. The primary path is the scope.launch race above.

The atomic compareAndSet guard at ActiveSessionEngine.kt:4687 correctly prevents duplicate completions but does not address read-after-write ordering — handleSetCompletion is allowed to run with a stale _repCount.value snapshot as long as it is the only one to set setCompletionInProgress=true.

Affected code

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt
  • line 466 — scope.launch from onRepEvent callback (WORKOUT_COMPLETE arm)
  • line 1334 — _repCount.value write (post-process())
  • line 1682-1684 — handleMonitorMetric arm (secondary)
  • line 3437 — captureExitSnapshot reads stale _repCount.value
  • line 3504 / 3547 — WorkoutSession.workingReps / CompletedSet.actualReps persisted
• shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/usecase/RepCounterFromMachine.kt
  • line 562-606 — processModern increments and fires WORKOUT_COMPLETE inside one synchronous call

Fix direction (binding for the downstream fixer)

The snapshot must reflect the final repCounter state of the rep that triggered completion, not a one-tick-stale StateFlow.

Recommended approach (smallest behavioral change, preserves existing logging/PR_TRACK):

1. In ActiveSessionEngine.handleRepNotification, write _repCount.value before repCounter.process(...) so the StateFlow is up-to-date for any RepEvent callbacks process fires synchronously. This is the order other handleRepNotification-style sites use, and it preserves the existing repCountBefore/repCountAfter invariant by snapshotting _repCount.value into a local before process(). Specifically: hoist val repCountBefore = repCounter.getRepCount().totalReps (line 1311) into a _repCountBefore = coordinator._repCount.value read, then coordinator._repCount.value = repCounter.getRepCount() immediately after repCounter.process(...) returns, before any side-effects. More directly: write coordinator._repCount.value = repCounter.getRepCount() at the start of handleRepNotification (after the early-return guards at 1302-1305) so that any synchronous RepEvent callback in process() already sees the post-previous-tick state; or equivalently, flush the StateFlow write before the WORKOUT_COMPLETE onRepEvent callback returns. A cleaner alternative is to move the WORKOUT_COMPLETE arm out of the synchronous callback and dispatch it through the same coordinator.repEventsCollectionJob collector (line 674) so it runs on a known-serial dispatcher.

2. In captureExitSnapshot (line 3437), if the _repCount.value read disagrees with repCounter.getRepCount() for a non-AMRAP, non-JustLift finite set, prefer repCounter.getRepCount().workingReps (read directly from repCounter, not the StateFlow) when repCounter.shouldStopWorkout() is true. This is the source-of-truth read; the StateFlow is a cache.

3. Belt-and-braces: in the WORKOUT_COMPLETE arm of the onRepEvent callback (line 527-536), call coordinator._repCount.value = repCounter.getRepCount() before handleSetCompletion(eventLease) so the launched coroutine cannot see a stale value.

Any of (1), (2), or (3) closes the race. (1) is the recommended fix — single-line ordering change, no new code paths, no behavioral change for the monitor path. Add (3) as a defensive flush if a regression test still flakes after (1).

Acceptance criteria

• Given a finite-set, non-AMRAP, non-JustLift set with reps = 7 and warmupReps = 3,
  when the BLE rep notification with repsSetCount = 7 arrives and shouldStopWorkout() becomes true,
  then WorkoutSession.workingReps == 7 and CompletedSet.actualReps == 7 are persisted.
• handleSetCompletion's PR_TRACK log (ActiveSessionEngine.kt:4696) reports workingReps=7 for the same instant as the snapshot's workingReps=7.
• AnimatedRepCounter.showingCelebration == 7 (regression guard — existing on-screen behavior unchanged).

Regression test target

Add a commonTest that:

1. Boots the ActiveSessionEngineIntegrationTest harness (or DWSMWorkoutLifecycleTest harness) with WorkoutParameters(reps=7, warmupReps=3, isJustLift=false, isAMRAP=false).
2. Simulates 3 warmup notifications then 7 working notifications via acceptRepNotification(...) (or handleRepNotification if it is testable from commonTest).
3. Asserts the persisted WorkoutExitSnapshot.session.workingReps == 7 and CompletedSet.actualReps == 7.

If direct handleRepNotification is not exposed from commonTest, drive it through BleRepository notifications + advanceUntilIdle() on testScope. The test must fail on 14fddb2 and pass after the fix.

A second test should cover the monitor-metric secondary path: inject a WorkoutMetric after shouldStopWorkout() flips in repCounter but before _repCount.value is updated by handleRepNotification, and assert the snapshot is correct.

Explicit non-goals

• Do not touch RepNotificationFreshnessGate. The gate is not the cause; PR fix: exempt AMRAP leases from rep freshness gate (#700) #701 is innocent.
• Do not change RepCounterFromMachine.processModern. The rep counter is the source of truth.
• Do not alter the compareAndSet guard at line 4687 or the duplicate-suppression semantics. The race is read-after-write, not duplicate-completion.
• Do not remove scope.launch from the onRepEvent callback to "fix" timing — handleSetCompletion legitimately needs to be on a coroutine because it suspends. The fix is to ensure _repCount.value is current before the launch dispatches.

Files inspected

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt (lines 460-540, 1280-1370, 1620-1690, 3420-3560, 3617-3645, 4680-4730)
• shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/usecase/RepCounterFromMachine.kt (lines 308-606)
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGate.kt (lines 48-98) — verified identical pre/post PR fix: exempt AMRAP leases from rep freshness gate (#700) #701 for finite non-AMRAP sets
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/screen/WorkoutHud.kt (lines 654-672)
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/components/AnimatedRepCounter.kt (lines 99-160) — celebration driven by confirmedReps, proving in-memory workingReps == 7
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt (test harness + persistence path)

[9thLevelSoftware]

{
"body": "## Architecture Review: APPROVED\n\nReviewer: phoenixarchitect (GPT-5.6 Terra xhigh)\nCommit inspected: 14fddb2 (matches RCA)\n\n### RCA Validation\n\nRoot cause confirmed by source inspection:\n- handleRepNotification (line 1319) calls repCounter.process() which synchronously fires onRepEvent?.invoke(WORKOUT_COMPLETE) at RepCounterFromMachine.kt:598-604 when workingReps >= workingTarget.\n- The onRepEvent callback at ActiveSessionEngine.kt:466 dispatches handleSetCompletion via scope.launch { ... } (non-blocking).\n- coordinator._repCount.value = repCounter.getRepCount() is written at line 1334, AFTER process() returns.\n- handleSetCompletion → captureExitSnapshot → buildExitSnapshot reads coordinator._repCount.value at line 3437 and persists it into WorkoutSession.workingReps (line 3504) and CompletedSet.actualReps (line 3547).\n- On-screen counter is unaffected because WorkoutHud (line 658) reads repCount.workingReps from the same StateFlow, and AnimatedRepCounter (line 110) fires showingCelebration = confirmedReps via a LaunchedEffect — both observe the Compose snapshot, which updates on the next recomposition after the StateFlow write.\n\n### Architectural Risk Assessment\n\nBreaking changes: None. The fix is a pure ordering change — moving the StateFlow write before process() or flushing it in the WORKOUT_COMPLETE arm. No API surface, schema, or data model changes.\n\nCompatibility: Mobile-only. No portal/Supabase/edge function impact. The WorkoutSession and CompletedSet schemas are unchanged; only the value persisted for workingReps changes from stale (N-1) to correct (N).\n\nExisting patterns: The fix aligns with the existing pattern at handleMonitorMetric (line 1666), which writes _repCount.value before calling handleSetCompletion at line 1683. The recommended fix makes handleRepNotification follow the same write-before-act ordering.\n\nScope: Minimal and focused. The RCA's non-goals are correct:\n- Do NOT touch RepNotificationFreshnessGate (innocent, PR #701 verified identical for finite sets)\n- Do NOT touch RepCounterFromMachine.processModern (source of truth)\n- Do NOT alter the compareAndSet guard (prevents duplicates, not the cause)\n- Do NOT remove scope.launch (handleSetCompletion legitimately suspends)\n\nCross-repo risks: None. This is a self-contained lifecycle race in ActiveSessionEngine on the mobile side.\n\n### Fix Direction Assessment\n\nThe RCA recommends three options, any of which closes the race:\n1. (Recommended) Hoist _repCount.value write before process() — single-line ordering change, no new code paths.\n2. In captureExitSnapshot, prefer repCounter.getRepCount().workingReps over the StateFlow when shouldStopWorkout() is true.\n3. Belt-and-braces: flush _repCount.value in the WORKOUT_COMPLETE arm before handleSetCompletion.\n\nOption 1 is architecturally cleanest — it ensures the StateFlow is always current before any synchronous callback fires, which is the invariant the code should have had from the start. It also protects against future callback arms that might read the StateFlow.\n\nOption 2 is a good defensive complement — reading directly from repCounter (the source of truth) in the snapshot path is more robust than relying on a cache. Consider adding it as a belt-and-braces measure even with option 1.\n\nOption 3 is redundant if option 1 is applied — but harmless as a defensive flush.\n\n### Test Target Assessment\n\nThe RCA's regression test target is sound:\n- The DWSMTestHarness already provides modernRepPacket() and startCableSet() helpers.\n- The WorkoutExitPersistenceTest pattern shows how to drive handleSetCompletion and assert saveSessionAttempts / saved counts.\n- A test that simulates 3 warmup + 7 working reps via fakeBleRepo.repEvents emissions + advanceUntilIdle() and asserts fakeWorkoutRepo.saveSessionAttempts[0].workingReps == 7 will fail on 14fddb2 and pass after the fix.\n- The StandardTestDispatcher in the test harness will deterministically interleave the scope.launch coroutine, making the race reproducible in tests.\n\n### Verdict\n\nAPPROVED. The fix direction is technically sound, minimal in scope, introduces no breaking changes or migration needs, aligns with existing codebase patterns, and has no cross-repo or schema risks. The bugfix agent may proceed with implementation."
}

[9thLevelSoftware]

Additional info from Discord thread:

to illustrate my point, here's an export of the last few back squat workouts. note the target reps. when set to "0" that's AMRAP, but i consistently hit 3 on those. before today's session (8/27/2026), working reps match target reps. because i wasnt sure the AMRAP fix was put in place, i opted to define the last set today. hence my theory that the AMRAP fix is causing this issue.

Attachments:

• workout_history_1786970655144.csv: https://cdn.discordapp.com/attachments/1538869232312516638/1538892918490796112/workout_history_1786970655144.csv?ex=6a845537&is=6a8303b7&hm=257cecb2ff64ebe4e2e696ddefcf922c46f8f7da3b5802c629d7eac1599735ae&

— added by ticket bot