feat: streaming library sync, memory cards, and save state history - #3856
Open
LoneAngelFayt wants to merge 21 commits into
Open
feat: streaming library sync, memory cards, and save state history#3856LoneAngelFayt wants to merge 21 commits into
LoneAngelFayt wants to merge 21 commits into
Conversation
Extend the streaming session layer with library-backed asset sync and per-user memory cards. State library sync: manual and autosave states are pulled from the container into the user's library, with PCSX2 screenshots extracted from the .p2s archive as state thumbnails. A session can be claimed with a state id to resume directly from a library state. Save file sync: save files are hydrated into the container at claim and pulled back on release, deduplicated by content hash. Memory cards: a new per-user, per-emulator whole-card model with version history (memory_cards, memory_card_versions). Opt-in per platform, currently PCSX2 only. Cards are hydrated at claim (wipe-then-replace) and evacuated before the session is released. Playtime: session start and end are ingested as play sessions, skipping runs shorter than five seconds. Session hardening: heartbeats mark liveness, and a session whose heartbeat has gone stale can be taken over after the previous container is torn down. Frontend: resume-from-state picker, memory card picker and manager, asset previews, and an admin streaming panel on the activity view.
…session An admin ending a session from the activity panel used to leave the player's tab stranded on a dead stream with no indication anything had happened. The player's tab now learns of this near-instantly via a socket push to a per-user room, with a slower REST poll (30s) as a fallback for a dropped event or a throttled background tab, exits fullscreen, and shows a dialog naming the admin and the reason given before returning to the game's details page. Backend: a 15-minute Redis tombstone records who ended the session and why, read by both the new side-effect-free status endpoint and the existing heartbeat, and pushed live over Socket.IO the moment it is written. The self-release guard is corrected to key off whether a reason was supplied rather than off user identity, since an admin can be logged into the same account they are evicting. The admin panel's release action now prompts for an optional reason that is shown to the displaced player alongside the admin's name.
Pulled states overwrote the slot they came from, so a player who saves often could only ever resume from the last capture in each slot. Every capture is now its own library asset, and the resume picker is the way back to any earlier point. The container filename encodes the emulator slot, so the library name and the container name have to diverge. A capture stamp is inserted directly before the slot token, which keeps both slot patterns matching at the end of the name: states written before this resolve unchanged, and the container-side name is recovered by dropping the stamp. Retention is capped per ROM, emulator and user by STREAMING_STATE_HISTORY_LIMIT (default 50), pruning oldest first, and a capture identical to the previous one is dropped rather than taking a slot in the history. Hydration now pushes only the newest state: every history entry collapses to the same container-side name, so pushing more would just overwrite in place. A resume pick already sent at claim time suppresses the push entirely, since it would land before the broker's deferred load fires. With the library holding the history, slots stop being a user-facing concept and become the register the emulator writes through. The player pins to the autosave slot, which is also what hydration targets and what save-and-exit already used, so in-game quick-load lands on the same file the picker last sent down. That made the slot picker and the separate load-autosave button redundant, and required save-state to accept the autosave slot: the two coarse slot bounds collapse into one, and _assert_valid_slot no longer takes an allow_autosave flag. Frontend: the state strip gains grid and list layouts for histories the horizontal row buries, persisted per user. The pre-game screen is one layout instead of two, with the resume panel always present so a first run and a long history read as the same screen, and the memory card picker split into its own section. Dolphin inherits all of this. xemu does not: it has no slot convention to place a stamp against, so its captures still update in place.
PCSX2 embeds a PNG inside every .p2s savestate, so a pulled state gets its resume-picker thumbnail for free. Dolphin savestates carry no frame, so its broker captures one at save time and serves it from GET /state-screenshot. Move the sourcing decision out of _store_state_asset and into _pull_state_to_library, so extract-or-fetch is resolved in one place and the image is passed down. A 404 from the broker is the normal "this one captures no frames" answer, so it is not logged. Also drop the PCSX2-only wording from the memory-card comments, since Dolphin now shares those paths.
Neither screenshot source validated what it handed over. The zip path trusted an entry that only claims to be a PNG by name, and the broker path trusted whatever came back over HTTP, so an error page from a proxy in front of a broker would have been written into the user's screenshots directory and bound to a state as its thumbnail. Check the magic bytes in _store_state_screenshot rather than at each source, since both feed that one function.
The seven hand-rolled urllib blocks for state files, save archives and memory cards become three primitives: a raising binary GET, a best-effort GET that swallows the routine 404, and a best-effort PUT. The nosec annotation and the secret header now live in one place each.
Contributor
Author
|
migrations might need a look through, some of this work is older and we've moved past the current numbers, something we may have to adjust at point of merge |
LoneAngelFayt
marked this pull request as ready for review
July 24, 2026 05:26
Contributor
Author
|
@gantoine This should be ready I updated the migrations so they should they should be clean this would be great to get into 5.1.0 to make the streaming feel more feature complete instead of a tech demo lol |
gantoine
removed
the
on-hold
gantoine
self-requested a review
July 24, 2026 11:45
Member
Contributor
Greptile SummaryThis PR substantially extends native streaming persistence and lifecycle management.
Confidence Score: 2/5The PR is not safe to merge until release errors relinquish session claims, interrupted adoption can recover, and public cards can be selected for streaming. Detached teardown leaves Redis claims intact after intermediate failures, separately committed adoption state can trap retries in a permanent 502 response, and the claim picker plus backend ownership check make public cards unreachable. backend/endpoints/streaming.py, frontend/src/v2/components/Player/MemoryCardPicker.vue, backend/endpoints/memory_cards.py Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
backend/endpoints/streaming.py:2577-2578
**Failed teardown retains session claim**
When broker shutdown, card evacuation, wiping, or notification raises during detached release, this handler only logs the exception and skips the Redis deletion at line 2564. The API has already reported a successful release, but subsequent players continue to see the container as occupied until stale takeover or the six-hour TTL clears it.
### Issue 2 of 3
frontend/src/v2/components/Player/MemoryCardPicker.vue:53-54
**Public cards remain unselectable**
When another user publishes a card for this emulator, the claim picker calls the owner-only listing instead of `getSharedMemoryCards`, so the public card never appears. Supplying its ID directly also fails the backend ownership check, leaving the new sharing endpoint and visibility control unusable for streaming.
### Issue 3 of 3
backend/endpoints/streaming.py:2179-2186
**Adoption retry rejects stored card**
When the card version was committed but recording the separate adoption outcome failed, the next claim reads the same container card and deduplication finds the existing version. This branch treats that idempotent result as an import failure, deletes the claim, and returns 502 on every retry instead of recording the outstanding adoption decision.
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2577
to
+2578
| except Exception: | ||
| log.exception("session teardown failed, platform=%s", platform) |
Contributor
There was a problem hiding this comment.
When broker shutdown, card evacuation, wiping, or notification raises during detached release, this handler only logs the exception and skips the Redis deletion at line 2564. The API has already reported a successful release, but subsequent players continue to see the container as occupied until stale takeover or the six-hour TTL clears it.
Knowledge Base Used: Streaming and Emulation
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/endpoints/streaming.py
Line: 2577-2578
Comment:
**Failed teardown retains session claim**
When broker shutdown, card evacuation, wiping, or notification raises during detached release, this handler only logs the exception and skips the Redis deletion at line 2564. The API has already reported a successful release, but subsequent players continue to see the container as occupied until stale takeover or the six-hour TTL clears it.
**Knowledge Base Used:** [Streaming and Emulation](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/streaming-and-emulation.md)
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+53
to
+54
| const { data } = await memoryCardApi.getMemoryCards({ emulator }); | ||
| cards.value = data; |
Contributor
There was a problem hiding this comment.
When another user publishes a card for this emulator, the claim picker calls the owner-only listing instead of getSharedMemoryCards, so the public card never appears. Supplying its ID directly also fails the backend ownership check, leaving the new sharing endpoint and visibility control unusable for streaming.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/v2/components/Player/MemoryCardPicker.vue
Line: 53-54
Comment:
**Public cards remain unselectable**
When another user publishes a card for this emulator, the claim picker calls the owner-only listing instead of `getSharedMemoryCards`, so the public card never appears. Supplying its ID directly also fails the backend ownership check, leaving the new sharing endpoint and visibility control unusable for streaming.
**Knowledge Base Used:**
- [Streaming and Emulation](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/streaming-and-emulation.md)
- [Frontend App Structure (v1)](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/frontend-app.md)
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment.
This one is intentional, and the fix is to the docstrings rather than the behavior.
Public cards are view-only on purpose. A card mounted at claim is written back as a new version on release, and that version belongs to the card's owner, so hydrating another user's public card would mean one player's session silently appending versions to someone else's card. Supporting it properly needs copy-on-mount (fork the public card into a claimer-owned copy at claim time), which is a feature rather than a fix, so it is deliberately out of scope for this PR. _resolve_memory_card has enforced owner-scoping the whole time; the picker calling getMemoryCards matches that.
You were right that the codebase contradicted itself, though. Two docstrings claimed the claim picker already mounted shared cards:
GET /memory-cards/sharedsaid "Used by the claim picker so a user can hydrate a shared card"get_card_by_idsaid "Used when hydrating a card shared by another user"
Neither was true of any caller. Both now describe what actually happens (browsing and download for /shared; unscoped reads plus session-owned lookups for get_card_by_id), and _resolve_memory_card spells out why mounting is owner-scoped instead of pointing at internal planning shorthand.
The other two P1s are fixed in 440a367, both with regression tests:
- Failed teardown retains session claim - the Redis delete moves into a
finally, so a raise from broker shutdown, evacuation, wipe or the force-release note can no longer strand the claim after the API has already reported the release. - Adoption retry rejects stored card - a dedup match against the card's latest version is now treated as the idempotent retry it is (hydrate would push back the bytes already on the container), so the outstanding decision gets recorded instead of returning 502 forever. A match against a non-latest version still aborts, since hydrate would push different bytes over the adopted card.
Worth flagging: test_adopt_deduplicated_against_older_version_aborts asserted the trapped 502, but its fixture stored only one version, which was therefore the latest, i.e. exactly the recoverable case. It is replaced by two tests that separate the retry from a genuine older-version match.
Contributor
There was a problem hiding this comment.
That's a solid explanation — the copy-on-mount requirement makes the ownership constraint the right default, and fixing the misleading docstrings is exactly the right resolution here. The original comment was pointing at a real contradiction between the docs and the behavior; you've resolved it at the source.
The teardown finally fix and the adoption retry correction both sound well-scoped. The test fixture issue (test_adopt_deduplicated_against_older_version_aborts asserting the wrong case because its single version happened to be latest) is exactly the kind of thing that passes green but proves nothing — good catch replacing it with the two separated cases.
No further action needed on this thread.
Comment on lines
+2179
to
+2186
| log.error( | ||
| "adopted memory card matched an existing version of card %d", | ||
| memory_card.id, | ||
| ) | ||
| await async_cache.delete(_session_redis_key(session_key)) | ||
| if created_blank_card_id is not None: | ||
| db_memory_card_handler.delete_card(created_blank_card_id) | ||
| raise HTTPException(status_code=502, detail=_CARD_IMPORT_FAILED_DETAIL) |
Contributor
There was a problem hiding this comment.
When the card version was committed but recording the separate adoption outcome failed, the next claim reads the same container card and deduplication finds the existing version. This branch treats that idempotent result as an import failure, deletes the claim, and returns 502 on every retry instead of recording the outstanding adoption decision.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/endpoints/streaming.py
Line: 2179-2186
Comment:
**Adoption retry rejects stored card**
When the card version was committed but recording the separate adoption outcome failed, the next claim reads the same container card and deduplication finds the existing version. This branch treats that idempotent result as an import failure, deletes the claim, and returns 502 on every retry instead of recording the outstanding adoption decision.
**Knowledge Base Used:**
- [Streaming and Emulation](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/streaming-and-emulation.md)
- [Database Models and Migrations](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/database-models.md)
How can I resolve this? If you propose a fix, please make it concise.…eaming-additions Conflicts were confined to files upstream barely touched: - streaming.py / test_streaming.py: upstream moved every mutating session route from ROMS_READ to ROMS_USER_WRITE so KIOSK_MODE visitors, who all share one synthetic user, cannot claim sessions or overwrite each other's save states. Applied the same rule to the routes this branch adds, so heartbeat now gates on ROMS_USER_WRITE while the side-effect-free status route stays on ROMS_READ, and added heartbeat to the kiosk test matrix. - Stream.vue: this branch replaced the launch screen with a pre-game config panel, so its structure wins; ported upstream's usePageTitle call and the GameCover style-context="player" prop onto it. - locales/*/play.json: kept both sides' keys. Also renumbered this branch's migrations onto the new upstream head: 0104_memory_cards and 0105_container_adoptions collided with upstream's 0104/0105 and left two alembic heads, so they become 0108 and 0109 chained onto 0107_roms_dedup_cover_index.
…pted adoptions Detached teardown deleted the Redis claim inside the try block, so a raise from broker shutdown, card evacuation, wiping or the force-release note skipped it. The API had already answered "released", leaving the container occupied to everyone else until stale takeover or the six-hour TTL. The delete moves to a finally: a card left un-evacuated is recoverable, since the next claim prompts to adopt whatever is still on the container, but a phantom claim is not. Adoption recorded the card version and the decision row in separate steps. When the version committed and the decision did not, the retry read the same container card, dedup refused a second copy, and the claim aborted 502 on every attempt. That case is idempotent, since hydrate would push back the bytes already on the container, so it now records the outstanding decision and continues. A dedup match against a version that is not the latest still aborts, because hydrate would push different bytes over the adopted card. test_adopt_deduplicated_against_older_version_aborts asserted the trapped 502, but its fixture stored only one version, which was therefore the latest and the recoverable case. It is replaced by two tests covering the retry and a genuine older-version match. Public memory cards stay view-only at claim: a mounted card is written back as a new version belonging to its owner, so hydrating someone else's card needs copy-on-mount semantics that do not exist yet. Docstrings on the shared-card listing and get_card_by_id claimed the claim picker already mounted shared cards; they now describe the owner-scoped behavior.
The merge brought in a useCanPlay mock that predates the streaming branch, so the composable destructured an undefined ref.
LoneAngelFayt
added a commit
to LoneAngelFayt/pcsx2-romm-integration
that referenced
this pull request
Aug 3, 2026
The gate 8e3777b added locks out every user of released RomM. Upstream rommapp/romm#3211 is merged and is what people run; its claim response returns the operator's configured host with nothing appended. The RomM half of the gate, which carries the stream token into the iframe URL, is rommapp/romm#3856 and is still open. So the browser loads the stream bare, /verify sees no token, and the document, every asset and the WebSocket upgrade are all refused. Specs STREAM_GATE=off|token, defaulted off until #3856 merges. Deciding enforcement in the broker rather than in the nginx config keeps a mode switch to a restart: as 1736c84 recorded, the nginx patch lands in the writable layer behind its own marker grep, so a gate decided there would need a recreate in each direction. Also records the accepted risk. Defaulted off, :latest serves the interactive desktop with the ROM library mounted again, which the README will state plainly rather than bury.
LoneAngelFayt
added a commit
to LoneAngelFayt/pcsx2-romm-integration
that referenced
this pull request
Aug 3, 2026
8e3777b gates the desktop on a token RomM cannot send. rommapp/romm#3211 is merged and is what people run, and its claim response returns the operator's configured host with nothing appended; the half that carries the token is rommapp/romm#3856, still open. So the browser loads the stream bare, /verify sees no token, and the document, every asset and the WebSocket upgrade are all refused. That is a lockout, not a gate, and it is live for everyone on the published image. The decision is made here rather than in the nginx config on purpose. As 1736c84 recorded, that patch lands in the container's writable layer behind its own marker grep, so a gate decided there needs a recreate in each direction; decided in the broker, switching modes is a restart. The token machinery is untouched: /launch still mints, /status still reports, release still clears, and the TTL and grace still tick. Only enforcement is bypassed, so turning the gate back on is one variable. The seven existing gate tests are pinned to STREAM_GATE=token, or every 403 assertion in them would have started passing for the wrong reason under the new default.
3 tasks
LoneAngelFayt
added a commit
to LoneAngelFayt/pcsx2-romm-integration
that referenced
this pull request
Aug 3, 2026
The Security section stated the token gate as settled fact, which stopped being true the moment STREAM_GATE landed. It now names the exposure in its own paragraph rather than burying it in a variable table: with the gate off, anyone reaching 3000 or 3001 gets the desktop and the ROM library at /files with no credential. It also names why, with both upstream PRs linked, because an operator weighing that risk needs to know it ends when rommapp/romm#3856 merges and that turning the gate on is one variable and a restart. The init.sh comment block claimed unconditional enforcement too. It now records the split: the gate is always injected, the broker decides whether to enforce, and that is what keeps a mode switch from needing a recreate in each direction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #3211.
Closes #3968: the streaming container is now gated by a broker-minted, session-bound stream token. The broker mints the token at launch, RomM carries it in the claim host URL, and the container's nginx vhost enforces it on every request (document, assets, and the WebSocket upgrade) via
auth_request. The RomM-side half of that gate (carrying the token through to the iframe host URL) lands in this PR.Library sync, memory cards, session hardening
Save states and save files now sync between the container and the user's library, so a session can be resumed from any machine. State thumbnails come from whichever source the emulator offers: PCSX2 embeds a frame in the
.p2sarchive, while Dolphin's broker captures one alongside the save and serves it from/state-screenshot. Adds per-user memory cards with version history (opt-in per platform, PCSX2 and GameCube), hydrated at claim and evacuated at release. Sessions get heartbeats, so a stale one can be taken over instead of wedging the container.Importing a card that's already there
Turning on
memory_card_syncused to wipe whatever the container was already holding on the first claim. The first claim now stops and asks: adopt that card as version one, or start clean. The answer is recorded per user and container, so it's asked once. The flag is also ignored, with a warning, on platforms that have no memory card (Wii, Wii U, Xbox), where whole-card sync would have quietly replaced the per-file save sync those platforms actually rely on.Force-release notifications
Ending someone's session from the admin panel used to leave their tab on a dead stream with no explanation. They now get a dialog naming the admin and the reason, then go back to the game's details page.
Save state history
Pulled states used to overwrite the slot they came from, so saving often meant only the newest capture per slot survived. Every capture is now its own library asset. Retention is capped by
STREAMING_STATE_HISTORY_LIMIT(default 50 per ROM/emulator/user) and identical back-to-back captures are dropped.Since the library holds the history, slots stop being a user-facing concept and become just the register the emulator writes through, so the slot picker and load-autosave button are gone. Older states resolve unchanged, no migration needed.
Dolphin and xemu both get this. xemu's snapshots do carry a slot in the filename, so timestamps hang off it the same way.
Known follow-up: the state list grows to ~50 per user per emulator, so the picker wants pagination. Not in this PR.
AI assistance
Written with substantial AI assistance (Claude Code). Design, review, and end-to-end testing against real PCSX2 and Dolphin containers were mine; the implementation was largely AI-authored under that direction and read through before committing.
Checklist