Skip to content

Fix end-of-dream busyloop and harden resume/settings against bad values - #666

Open
scottdraves wants to merge 1 commit into
masterfrom
fix-resume-busyloop-streaming-fallback
Open

scottdraves wants to merge 1 commit into
masterfrom
fix-resume-busyloop-streaming-fallback

Conversation

@scottdraves

Copy link
Copy Markdown
Contributor

Problem

When a dream ended with an empty cache and streaming disallowed (0 quota, fresh install), preflightNextDream(false) returned nullopt every frame and the player re-preflighted at ~60fps with no way to advance — a CPU/log busyloop.

The field trigger was a saved last_played_frame sitting on the clip's last frame: the resumed dream decoded ~1 frame, "finished" on arrival, and dropped straight into the non-streaming natural transition, which then spun.

Changes

  • PlaylistManager::preflightNextDream — the actual fix. When nothing is cached and streaming was disallowed, stream the next dream as a last resort instead of returning nullopt. Cached content is still preferred above this; it only fires when the cache is empty.
  • Player::SetPlaylistAtDream — clamp the resume frame to [0, frames-1] using the dream's metadata frame count, replacing the bogus 24h@60fps ceiling that let past-end values through. A far-past-end seek otherwise decodes zero frames and wedges the decoder (no frames available spin).
  • JSONStorage::GetOrSetValue — wrap the boost::json as_*() extraction in try/catch so a malformed or out-of-range setting (an integer too large for uint64 is parsed as a double) falls back to the default instead of throwing and terminating the process.

Verification

Tested against the live Windows client with the exact field repro and a robustness battery on last_played_frame:

last_played_frame Before After
7225 (last frame, 0 quota, empty cache) 60fps busyloop streams next dream, 1 preflight
-100 safe (→0) safe (→0)
50000 (past 7226-frame end) decoder hang (no frames available ×655) clamp→7225, instant-finish, streaming fallback
~1e18 safe (fake guard) clamp→last frame
~1e23 (exceeds uint64) crash 0xC0000409 settings falls back to default

Confirmed end-to-end (empty cache): resume past end → clamping to last frameDecoder ended with only 1 framesPreflight : no cached dreams available; streaming next dream at position 1 → next dream streams. No spin, no crash.

🤖 Generated with Claude Code

When a dream ended with an empty cache and streaming disallowed (0 quota,
fresh install), preflightNextDream(false) returned nullopt every frame and
the player re-preflighted at 60fps with no way to advance — a CPU/log
busyloop. The field trigger was a saved last_played_frame sitting on the
clip's last frame, so the resumed dream "finished" on arrival and dropped
straight into that non-streaming transition.

- PlaylistManager::preflightNextDream: when nothing is cached and streaming
  was disallowed, stream the next dream as a last resort instead of returning
  nullopt. Cached content is still preferred; this only fires when the cache
  is empty.
- Player::SetPlaylistAtDream: clamp the resume frame to [0, frames-1] using
  the dream's metadata frame count, replacing the bogus 24h@60fps ceiling that
  let past-end values through. A far-past-end seek otherwise decodes zero
  frames and wedges the decoder ("no frames available" spin); a value too
  large is parsed as a double and crashes the settings read (see below).
- JSONStorage::GetOrSetValue: wrap the boost::json as_*() extraction in
  try/catch so a malformed or out-of-range setting (an integer too large for
  uint64 is parsed as a double) falls back to the default instead of throwing
  and terminating the process.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alansley

Copy link
Copy Markdown
Contributor

@scottdraves - for whenever you come back to this, I had GPT 5.6 Sol review the PR w/ high effort and it suggests some minor adjustments:

The busy-loop diagnosis looks sound, and the resume clamping plus JSON exception handling are good improvements. One concern: the new fallback returns an uncached dream when canStream == false. Some callers derive that value from remainingQuota > 0, while PlayClip/PreloadClip can subsequently call GetDreamDownloadLink without another quota check. This could bypass cache-only/zero-quota behavior. Could “prefer cached” and “stream permitted” be represented separately, with last-resort streaming requiring explicit permission? It would also be good to retain a safe fallback when metadata frames <= 0; otherwise a corrupt large seek remains unbounded. With those addressed, this looks good.

@alansley

Copy link
Copy Markdown
Contributor

Reviewed this branch and found three follow-ups. Fixes are up as #683, based directly on 47f25e9 and targeting this branch, so merging it folds them straight into this PR.

  • Player.cpp — the resume clamp now only applies when the metadata frame count is known, but Dream::frames is an uninitialised int that CacheManager leaves at 0 whenever the payload omits "frames". A wrapped last_played_frame on such a dream goes through unclamped and still hangs the decoder at startup — the case the removed 24h bound covered.
  • JSONStorage.cpp — the new try/catch wraps GetOrSetValue's shared body, so it catches writes as well as reads. A wrong-kind value makes Set a silent no-op that still returns true, and Settings::Get's self-repair (Set + Commit) fails silently with it, so the bad value can't be corrected from inside the app.
  • PlaylistManager.cpp — the streaming fallback returns a dream that's neither cached nor has a streaming URL, so the caller resolves one synchronously while holding the update mutex. Offline — the usual reason the cache is empty — that fails and repeats every update, trading the busyloop for a blocking one.

Full reasoning and the fixes in #683. Builds clean on Linux; the offline/empty-cache path is reasoned about rather than runtime-tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants