fix: per-song cloud sync silently disabled by a losing try_lock

[LargeModGames]

Summary

Per-song listening-history sync to spotatui.com was silently never firing, so every session's records were uploaded in one batch during the awaited exit sync, making quit slow.

Root cause: spawn_history_collector snapshotted user_config.behavior.sync_token once via app.try_lock() into a non-mut binding. Startup pre-work added later (recap check + spawn_blocking(load_listens)) pushed that read into the live render loop, and tokio's fair mutex hands a freed permit straight to a queued waiter, so the try_lock intermittently lost and pinned the token to None for the whole process. The exit path reads the same field with a real lock().await, which is why exit sync worked and per-song did not.

Six changes across src/infra/history.rs and src/tui/runner.rs:

• take the app lock properly at collector startup instead of try_lock
• refresh the token from the guard already held each tick, so pasting or clearing it in Settings takes effect mid-session
• log whether cloud sync is enabled at startup and whenever the token changes
• move the cursor read and listens-file parse into spawn_blocking, keeping ever-growing blocking IO off a runtime worker
• advance the sync cursor by max() rather than last(), so an out-of-order listens.jsonl cannot regress the cursor and re-POST synced records
• restore the terminal before the exit network calls, and bound each call separately (2s history, 1s now-playing clear) instead of inheriting the shared client's 10s connect + 30s request

Ordering note: the now-playing clear stays strictly the last network call. The collector keeps ticking after pause_native_playback_before_exit flips is_playing and re-pushes now-playing; the website route is an onConflictDoUpdate upsert, so a clear that landed first would be undone and leave a stale "paused" card until the 5-minute online threshold expires. Running the two under tokio::join! was tried and reverted for this reason. Separate timeout budgets also mean a slow history upload cannot starve the clear.

Testing

• cargo fmt --all --check - clean
• cargo clippy --no-default-features --features telemetry -- -D warnings - clean
• cargo test --no-default-features --features telemetry - 476 passed, 0 failed

Manually smoke tested on the full cargo run build rather than slim, since pause_native_playback_before_exit is #[cfg(feature = "streaming")]. Checked:

• per-song successfully synchronized listening history to cloud (1 tracks) lines appear mid-session rather than one large batch at exit
• quitting mid-track leaves the public widget showing offline within ~10s
• an offline quit finishes quickly instead of stalling up to a minute

Additional notes

None.

Summary by CodeRabbit

• Bug Fixes
  • Improved cloud history synchronization to safely handle token changes during active sessions.
  • Updated cloud cursor tracking for more reliable progress, including recovery when older records arrive out of order.
  • Prevented unmanaged background sync work by cleanly coordinating shutdown of the history collector.
  • Ensured the terminal interface is restored promptly on exit.
  • Added per-step time limits for exit-time history syncing and now-playing cloud cleanup, preserving correct ordering.

[coderabbitai]

📝 Walkthrough

Walkthrough

Cloud history synchronization now tracks network tasks, handles shutdown through a returned collector handle, and persists append-position cursors with legacy timestamp compatibility. Application exit shuts down collection, restores the terminal, then runs bounded history sync and now-playing cleanup.

Changes

History synchronization and graceful exit

Layer / File(s)	Summary
Collector lifecycle and token state src/infra/history.rs	The collector returns a shutdown handle, tracks cloud-sync tasks in a shared JoinSet, reads sync tokens under a lock, and drains pending network work.
Append-offset history synchronization src/infra/history.rs	Cursor loading supports offset:<u64> and legacy timestamp formats, blocking tasks handle JSONL loading, successful syncs persist the next append offset, and out-of-order appends are tested.
Terminal restoration and exit sync src/runtime.rs, src/tui/runner.rs	Runtime passes the collector handle to the UI, which shuts down collection, restores terminal state, then applies separate timeouts to history synchronization and final now-playing cleanup.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant start_ui
  participant HistoryCollectorHandle
  participant ratatui
  participant sync_history_to_cloud
  participant clear_now_playing_from_cloud
  start_ui->>HistoryCollectorHandle: shutdown collector
  start_ui->>ratatui: restore terminal state
  start_ui->>sync_history_to_cloud: run with 2-second timeout
  sync_history_to_cloud-->>start_ui: return or timeout
  start_ui->>clear_now_playing_from_cloud: run with 1-second timeout
  clear_now_playing_from_cloud-->>start_ui: return or timeout

Loading

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title uses an allowed conventional-commit prefix and clearly describes the core cloud-sync bug fix.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/per-song-cloud-sync

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/per-song-cloud-sync

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/infra/history.rs`:
- Around line 2261-2271: The cursor fallback in the `spawn_blocking` closure
still uses non-test `unwrap()` calls. Create the epoch timestamp as a fallible
value, propagate errors with `?`, and reuse that value for both the read and
parse fallback branches while preserving the existing `load_listens` result
flow.
- Around line 2291-2295: Update the sync cursor logic around new_listens and
last_ended_at so concurrent out-of-order appends cannot be skipped by the
ended_at > last_synced_at filter. Replace the timestamp-only advancement with an
append sequence, or use a replay overlap backed by server-side idempotency, and
add coverage for an out-of-order append occurring during an in-flight sync.

In `@src/tui/runner.rs`:
- Around line 1378-1399: Update the exit flow in start_ui around the
history_sync and clear_now_playing calls to signal spawn_history_collector
shutdown and await all outstanding now-playing work before invoking
clear_now_playing_from_cloud. Ensure the final clear occurs only after the
collector and its pause-triggered uploads have stopped, while preserving the
existing independent timeouts and warning handling.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ce42960-a36d-49bf-a597-319c6f23d7c6

📥 Commits

Reviewing files that changed from the base of the PR and between ae21eab and 6a36df8.

📒 Files selected for processing (2)

• src/infra/history.rs
• src/tui/runner.rs

[coderabbitai]

🧹 Nitpick comments (2)

src/tui/runner.rs (1)

1362-1405: 🩺 Stability & Availability | 🔵 Trivial

Residual risk: a push already in flight when shutdown aborts can still land at the server after the final clear.

Aborting the collector's JoinSet stops further client-side processing, but if a push request's bytes were already flushed to the socket before the abort took effect, the server can still apply that push after this clear runs (these are upserts with no ordering guarantee). Not fixable cheaply client-side; would need a sequence number or timestamp on the now-playing payload that the server uses to reject stale updates.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tui/runner.rs` around lines 1362 - 1405, Add ordering metadata to the
now-playing payload and update the server-side upsert handling so stale updates
are rejected based on sequence number or timestamp. Ensure the final clear
cannot be overwritten by an already in-flight push, while preserving current
collector shutdown and exit-clear flow.

src/infra/history.rs (1)

354-372: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

No test for the shutdown timeout/abort path.

This is the exact mechanism that fixes the previously flagged now-playing/exit-clear race. A regression test would catch future changes that break the "abort collector -> JoinSet drop -> child tasks cancelled" chain, e.g. by adding a slow fake task to the collector's JoinSet and asserting shutdown() returns within ~1s and no further completion is observed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/infra/history.rs` around lines 354 - 372, Add a regression test covering
HistoryCollectorHandle::shutdown when the collector task remains blocked:
include a slow fake child task owned by its JoinSet, invoke shutdown, assert it
returns within roughly one second, and verify no child completion occurs
afterward. Ensure the test exercises the timeout, task abort, and JoinSet
cancellation chain without changing normal shutdown behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/infra/history.rs`:
- Around line 354-372: Add a regression test covering
HistoryCollectorHandle::shutdown when the collector task remains blocked:
include a slow fake child task owned by its JoinSet, invoke shutdown, assert it
returns within roughly one second, and verify no child completion occurs
afterward. Ensure the test exercises the timeout, task abort, and JoinSet
cancellation chain without changing normal shutdown behavior.

In `@src/tui/runner.rs`:
- Around line 1362-1405: Add ordering metadata to the now-playing payload and
update the server-side upsert handling so stale updates are rejected based on
sequence number or timestamp. Ensure the final clear cannot be overwritten by an
already in-flight push, while preserving current collector shutdown and
exit-clear flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c8d91685-4c89-4cbd-9074-9fa5a5b6b197

📥 Commits

Reviewing files that changed from the base of the PR and between 6a36df8 and c23cf54.

📒 Files selected for processing (3)

• src/infra/history.rs
• src/runtime.rs
• src/tui/runner.rs