fix(#714): per-snapshot ownership marker prevents stale async Just Lift write

[9thLevelSoftware]

Summary

Fix Project Phoenix issue #714 (post-#716 P1 follow-up): an older captured Just Lift snapshots async persistSnapshot coroutine could grab the settingsManager.mutateWorkout(profileId) mutex AFTER a newer synchronous completion-job write had already persisted fresh defaults, then clobber them with stale captured values.

See PR #716 review thread discussion_r3837465213. PR #716 (commit f9c0defa) added the Just Lift defaults capture + persist pipeline; this PR closes the remaining P1 ordering regression identified by the post-merge audit.

Root cause

Both writers serialise through the same settingsManager.mutateWorkout mutex, but mutateWorkout does not impose recency. When two Just Lift sets complete before the first persistSnapshot reaches its Just Lift write, the second synchronous write flips preferences to Old School, then the older async coroutine resumes and re-writes TUT over the fresh value.

Fix — smallest ownership/ordering mechanism

Per-snapshot ownership marker on WorkoutExitSnapshot:

• WorkoutExitSnapshot gains a @Volatile var justLiftDefaultsPersisted: Boolean = false body field (with markJustLiftDefaultsPersisted() / clearJustLiftDefaultsPersisted() helpers) so the marker survives the existing getOrCapture { .copy(terminalPath = …) } hot path that re-issues a snapshot for an already-captured key.
• WorkoutExitSnapshot loses the data class modifier; the two .copy(terminalPath = …) callsites in WorkoutExitSnapshotStore mutate the existing terminalPath in place instead. No other classes or equals/hashCode usages are affected (equality of WorkoutExitSnapshot is not used outside the lease-keyed map).
• persistCapturedJustLiftDefaultsSnapshot(snapshot, claimIfNeeded) now distinguishes the synchronous completion-job write (the canonical owner; flips the marker on success, clears on throw) from the async persistSnapshot / retained-snapshot retry paths (skip if the marker is already true, otherwise write — preserving the durable backstop the test plan requires).
• Sync call site in handleSetCompletion → claimIfNeeded = true (with the existing try/catch wrapper).
• Async call site in persistSnapshot → claimIfNeeded = false.

The synchronous failure path explicitly clears the marker so the async retry can take ownership. Process-recovery retry paths (retryRetainedWorkoutExitPersistence / retryWorkoutExitPersistence) re-use the async path and inherit the same fallback semantics.

Scope

• Touched files only: ActiveSessionEngine.kt (helper + two call sites, plus narrowed comments addressing PR fix: persist Just Lift mode through automatic completion snapshot (#714) #716 style-review threads within the issue-714 touched region), WorkoutExitSnapshot.kt (data class → class + marker + body), WorkoutExitPersistenceTest.kt (the pre-existing test file for issue [Bug]: Just Lift mode resets to TUT at end of every set instead of preserving the user's selected mode (iOS) #714).
• No schema changes, no portal/Supabase migration, no TUT picker UX change, no machine-resistance or iOS autostart changes.

PR #716 review-thread cleanup

• discussion_r3837465213 (the audits open P1) — fixed by this commit.
• discussion_r3837465215 (failure containment: continue completion when saving Just Lift defaults fails) — already addressed by PR fix: persist Just Lift mode through automatic completion snapshot (#714) #716 52a80788 (try/catch at line 11283-11294 of ActiveSessionEngine.kt); verified intact and will be resolved with an explanation.
• discussion_r3837511295 (pre-summary ordering: persist defaults before publishing the summary) — already addressed by the same 52a80788 fix (write happens before the WorkoutState.Idle flip / SetSummary publish in the synchronous completion job); verified intact and will be resolved with an explanation.
• kilo-code style comments (3837532484, 3837532489, 3837532491, 3837560397, 3837582668, 3837451173, 3837532493): narrowed where they fall within the issue-714 touched files (comment-block collapse, fully-qualified WorkoutParameters uses the existing import, added JustLiftDefaultsDocument / RepCountTiming imports to the test file).

Regression evidence

Three new deterministic tests in WorkoutExitPersistenceTest (Issue714 successful synchronous write makes the async fallback path no-op for the same snapshot, Issue714 sync failure preserves the async fallback so the snapshot can still persist, Issue714 interleaved completion — older async TUT snapshots late write does not overwrite newer Old School sync write). Red-green verified by temporarily reverting the marker check: tests fail before the fix, pass after the fix.

Test evidence (./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true)

WorkoutExitPersistenceTest       tests=34 failures=0 errors=0 skipped=0
Issue593BodyweightRepEntryTest    tests=4 failures=0 errors=0 skipped=0
DropSetRuntimeRecoveryTest       tests=65 failures=0 errors=0 skipped=0
ActiveSessionEngineIntegrationTest tests=14 failures=0 errors=0 skipped=0
Issue673SetEndReasonLifecycleTest tests=22 failures=0 errors=0 skipped=0
Issue687StaleWorkSuppressionTest  tests=18 failures=0 errors=0 skipped=0
DWSMWorkoutLifecycleTest         tests=224 failures=0 errors=0 skipped=0

Fixes #714

🤖 Generated with Hermes Agent

Co-Authored-By: Hermes hermes@nousresearch.com

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-30T02:16:03.994885Z	7bd53af	New commits
🔒 Security Review	✅ Completed 2026-08-30T00:56:44.586083Z	1a79afc	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a79afceea

ℹ️ 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".

[kilo-code-bot]

Code Review Roast 🔥

Verdict: 1 Issue Found | Recommendation: Add back one integration test before merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	1
💡 suggestion	0
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
WorkoutExecutionGuardTest.kt	619	Guard-level tests pass even if the wiring between persistCapturedJustLiftDefaultsSnapshot and executionGuard.persistJustLiftDefaultsIfNewer breaks — the deleted integration tests in WorkoutExitPersistenceTest covered this

---

Verdict

Comment only — the refactor is sound. The profile-wide execution gate correctly prevents older async writes from clobbering newer sync writes, and the new design is materially simpler than the per-snapshot marker approach.

Correctness / Safety Findings

No correctness or safety findings. Mutex.withLock ensures the check-and-act is atomic, the executionId comparison correctly rejects stale writes, and failed writes don't update the map (enabling retry). The previous P2 concern from Codex (older unclaimed snapshot superseding a newer successful write) is fully addressed by the new design.

Ponytail Review

• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExecutionGuardTest.kt:619 — test-shrink: the three new tests verify the guard in isolation. The previous PR's integration tests in WorkoutExitPersistenceTest (deleted in this diff) verified the full path through persistCapturedJustLiftDefaultsSnapshot → executionGuard.persistJustLiftDefaultsIfNewer → settingsManager.mutateWorkout. Port at least one back — the sync-failure-then-async-retry path is the most valuable.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExecutionGuard.kt:1224 — yagni: persistJustLiftDefaultsIfNewer returns Boolean but the only production caller ignores it. The return is only used in tests, which could verify the side effect (writes list) instead.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt:431 — minor: getOrCapture returns .copy(terminalPath = terminalPath) but doesn't store the copy back in the map. Subsequent retainedSnapshots() returns the stale version. Benign since terminalPath doesn't affect persistence behavior.

Ponytail net: -5 lines (Boolean return removal) + ~200 lines (one ported integration test) = net +195.

Suggested Minimal Patch

1. Port one integration test from the deleted set back to WorkoutExitPersistenceTest. The sync-failure-then-async-retry path exercises the catch (e: Throwable) at ActiveSessionEngine.kt:11267 and confirms the retry can still take ownership after the synchronous write threw.
2. Optional cleanup: drop the Boolean return from persistJustLiftDefaultsIfNewer and update tests to verify side effects.

Final Merge Guidance

Can merge after one integration test is ported back. The guard-level tests are necessary but not sufficient — the wiring between the engine and the guard is the integration risk the PR was supposed to close.

---

🏆 Best part: The profile-wide execution gate is genuinely simpler than the per-snapshot marker. One mutex, one map, one comparison. The previous design had a marker that had to survive getOrCapture { .copy(terminalPath = …) } — that complexity is gone.

💀 Worst part: Deleting the integration tests while claiming "red-green verified" is a coverage gap dressed as a verification claim. The new tests verify the guard's logic, not the engine's wiring to the guard.

📊 Overall: Like replacing a Rube Goldberg machine with a light switch — cleaner, simpler, and harder to break. Just don't forget to test that the switch is actually wired to the light.

Fix these issues in Kilo Cloud

Previous Review Summaries (2 snapshots, latest commit e200b33)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit e200b33)

Verdict: 1 Issue Found | Recommendation: Add back one integration test before merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	1
💡 suggestion	0
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
WorkoutExecutionGuardTest.kt	619	Guard-level tests pass even if the wiring between persistCapturedJustLiftDefaultsSnapshot and executionGuard.persistJustLiftDefaultsIfNewer breaks — the deleted integration tests in WorkoutExitPersistenceTest covered this

---

Verdict

Comment only — the refactor is sound. The profile-wide execution gate correctly prevents older async writes from clobbering newer sync writes, and the new design is materially simpler than the per-snapshot marker approach.

Correctness / Safety Findings

No correctness or safety findings. Mutex.withLock ensures the check-and-act is atomic, the executionId comparison correctly rejects stale writes, and failed writes don't update the map (enabling retry). The previous P2 concern from Codex (older unclaimed snapshot superseding a newer successful write) is fully addressed by the new design.

Ponytail Review

• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExecutionGuardTest.kt:619 — test-shrink: the three new tests verify the guard in isolation. The previous PR's integration tests in WorkoutExitPersistenceTest (deleted in this diff) verified the full path through persistCapturedJustLiftDefaultsSnapshot → executionGuard.persistJustLiftDefaultsIfNewer → settingsManager.mutateWorkout. Port at least one back — the sync-failure-then-async-retry path is the most valuable.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExecutionGuard.kt:1224 — yagni: persistJustLiftDefaultsIfNewer returns Boolean but the only production caller ignores it. The return is only used in tests, which could verify the side effect (writes list) instead.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt:431 — minor: getOrCapture returns .copy(terminalPath = terminalPath) but doesn't store the copy back in the map. Subsequent retainedSnapshots() returns the stale version. Benign since terminalPath doesn't affect persistence behavior.

Ponytail net: -5 lines (Boolean return removal) + ~200 lines (one ported integration test) = net +195.

Suggested Minimal Patch

1. Port one integration test from the deleted set back to WorkoutExitPersistenceTest. The sync-failure-then-async-retry path exercises the catch (e: Throwable) at ActiveSessionEngine.kt:11267 and confirms the retry can still take ownership after the synchronous write threw.
2. Optional cleanup: drop the Boolean return from persistJustLiftDefaultsIfNewer and update tests to verify side effects.

Final Merge Guidance

Can merge after one integration test is ported back. The guard-level tests are necessary but not sufficient — the wiring between the engine and the guard is the integration risk the PR was supposed to close.

---

🏆 Best part: The profile-wide execution gate is genuinely simpler than the per-snapshot marker. One mutex, one map, one comparison. The previous design had a marker that had to survive getOrCapture { .copy(terminalPath = …) } — that complexity is gone.

💀 Worst part: Deleting the integration tests while claiming "red-green verified" is a coverage gap dressed as a verification claim. The new tests verify the guard's logic, not the engine's wiring to the guard.

📊 Overall: Like replacing a Rube Goldberg machine with a light switch — cleaner, simpler, and harder to break. Just don't forget to test that the switch is actually wired to the light.

Fix these issues in Kilo Cloud

Previous review (commit 1a79afc)

Verdict: 6 Issues Found | Recommendation: Address test gap before merge; rest is optional cleanup

Overview

Severity	Count
🚨 critical	0
⚠️ warning	1
💡 suggestion	5
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
WorkoutExitPersistenceTest.kt	1824	Test claims cross-snapshot late-write race but runs olderAsync before newerSync — doesn't exercise the actual bug
ActiveSessionEngine.kt	9036	10-line comment for a 25-line function, repeated 3 more times elsewhere
ActiveSessionEngine.kt	9066	Two catch blocks with identical bodies — split for no behavioral reason
ActiveSessionEngine.kt	9055	Defensive pre-clear for a sync-after-sync scenario with no code path
WorkoutExitSnapshot.kt	351	15-line class docstring + 12-line property kdoc that say the same thing
WorkoutExitSnapshot.kt	415	Three wrappers around a single boolean — private set + mark/clear helpers

---

Verdict

Comment only — the fix logic is sound (per-snapshot marker prevents same-snapshot duplicate writes, and the invariant that sync runs before its own async for each snapshot handles the cross-snapshot case). The warning is a test coverage gap, not a code bug.

Correctness / Safety Findings

No correctness or safety findings. The @Volatile marker, try/catch with marker-clearing on throw, and per-snapshot isolation are all correct. The invariant the fix relies on (sync write runs before its own async write for each snapshot) holds given the call sites in handleSetCompletion (line 11298) and persistSnapshot (line 9132).

Ponytail Review

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:9036 — shrink: 10-line function docstring repeated at property kdoc, class docstring, and two call-site comments. Keep one ~3-line summary.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:9055 — yagni: pre-clear of marker for claimIfNeeded = true. No code path produces a true marker at that point.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:9066 — shrink: two catch blocks with identical bodies. Collapse to one.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt:351 — shrink: class docstring re-explains the bug fix instead of documenting the class. Move reasoning to the call site.
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt:415 — shrink: mark/clear helpers around a field with private set. Two callers in one file — just expose the setter.
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt:1824 — test-shrink: reorder so olderAsync runs after newerSync; otherwise the test passes even without the fix.

Ponytail net: -50 lines.

Suggested Minimal Patch

1. Reorder test 3 so olderSnapshot async runs AFTER newerSnapshot sync.
2. Delete the comment at lines 9056-9058 and the if (claimIfNeeded) { ... } block (lines 9055-9060).
3. Collapse the two catch blocks at lines 9066-9071 into one.
4. Remove the private set and replace mark/clear calls with direct field assignment.
5. Trim the class docstring (lines 351-365) and property kdoc (lines 399-410) to one-liners.

Final Merge Guidance

Can merge after the test gap is closed (reorder test 3). The Ponytail cleanup is optional — the current code works, it's just verbose.

---

🏆 Best part: The per-snapshot marker design is actually clever — it's the smallest mechanism that solves the problem without introducing a global lock or sequence number. The @Volatile + clear-on-throw pattern is textbook ownership transfer.

💀 Worst part: The third test claims to catch a regression it can't actually catch. Red-green verification only works if the test exercises the failing path.

📊 Overall: Like a seatbelt that's installed correctly but not actually buckled.

Files Reviewed (3 files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt - 3 issues
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt - 2 issues
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt - 1 issue

Fix these issues in Kilo Cloud

---

_{Reviewed by minimax-m3 · Input: 45.8K · Output: 8.1K · Cached: 650.2K}

_{Review guidance: REVIEW.md from base branch main}