fix(native-engine): reuse cached server and restore saved sessions#6606
Merged
Conversation
matthewevans
enabled auto-merge
July 24, 2026 20:32
Contributor
📝 WalkthroughWalkthroughChangesNative engine provisioning and diagnostics
Persisted session restoration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ensure_native_engine_sync
participant provision_binary
participant native_server_process
ensure_native_engine_sync->>provision_binary: provision verified binary
provision_binary->>provision_binary: validate cached binary and minisign
ensure_native_engine_sync->>native_server_process: spawn with log directory and startup log
native_server_process-->>ensure_native_engine_sync: write startup stderr
sequenceDiagram
participant persisted_restore_loop
participant restore_persisted_session
participant GameSession
persisted_restore_loop->>restore_persisted_session: restore persisted session JSON
restore_persisted_session->>GameSession: deserialize and build session
GameSession-->>persisted_restore_loop: return lobby metadata and started state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@client/src-tauri/src/native_engine.rs`:
- Around line 378-385: Scope log paths by NativeEngineKey instead of sharing
base/logs: update log_directory and its callers, including
ensure_native_engine_sync, to use the current key consistently with
key_directory, binary, and data_directory. Extend gc_after_successful_spawn to
prune stale or aged rolling and per-game logs for the current key while
preserving active logs.
In `@crates/phase-server/src/main.rs`:
- Around line 116-139: Update restore_persisted_session to distinguish invalid
persisted-session data from operational failures such as thread creation, join,
or panic errors, using a typed error or equivalent classification instead of
collapsing all failures into String. Adjust its caller to delete the persisted
row only for confirmed-invalid snapshots, while retaining it for recoverable
operational restore failures.
🪄 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: 0cbd306f-b25d-49dd-95e9-a2d153f9b716
📒 Files selected for processing (2)
client/src-tauri/src/native_engine.rscrates/phase-server/src/main.rs
Comment on lines
+378
to
+385
| fn log_directory(&self) -> PathBuf { | ||
| self.base.join(LOG_DIRECTORY) | ||
| } | ||
|
|
||
| fn startup_log(&self) -> PathBuf { | ||
| self.log_directory().join("server-startup.log") | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Log directory isn't keyed by engine version/channel and is never pruned.
log_directory()/startup_log() return a single shared path (base/logs) for every NativeEngineKey, unlike key_directory, binary, binary_signature, and data_directory, which are all scoped per key. This is the first PR wiring --log-dir into the spawned server (previously stderr/stdout were discarded), so it's a newly introduced gap, not pre-existing:
- Switching preview/release channels interleaves
phase-server.logandgames/<code>.login the same directory, muddying diagnosis across channels. - This directory sits outside
gc_after_successful_spawn's scope (which only pruneskey_directoryentries and cache blobs per the GC test), so the daily-rolling main log and per-game logs accumulate indefinitely with no retention policy — undermining the very "diagnosable without a console" goal this feature is meant to serve once disk fills up.
As per path instructions, "Surface GAPS (missing or wrong behavior)" is exactly this kind of missing lifecycle management for a newly-introduced persistent resource.
♻️ Proposed fix: scope logs per key and add retention
- fn log_directory(&self) -> PathBuf {
- self.base.join(LOG_DIRECTORY)
- }
-
- fn startup_log(&self) -> PathBuf {
- self.log_directory().join("server-startup.log")
- }
+ fn log_directory(&self, key: &NativeEngineKey) -> PathBuf {
+ self.key_directory(key).join(LOG_DIRECTORY)
+ }
+
+ fn startup_log(&self, key: &NativeEngineKey) -> PathBuf {
+ self.log_directory(key).join("server-startup.log")
+ }Update the call site in ensure_native_engine_sync (already has key in scope) accordingly, and add pruning of stale/aged rolling and per-game log files to gc_after_successful_spawn so the current key's logs don't grow unbounded either.
🤖 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 `@client/src-tauri/src/native_engine.rs` around lines 378 - 385, Scope log
paths by NativeEngineKey instead of sharing base/logs: update log_directory and
its callers, including ensure_native_engine_sync, to use the current key
consistently with key_directory, binary, and data_directory. Extend
gc_after_successful_spawn to prune stale or aged rolling and per-game logs for
the current key while preserving active logs.
Source: Path instructions
Comment on lines
+116
to
+139
| #[cfg(windows)] | ||
| fn restore_persisted_session(json: &str, db: SharedDb) -> Result<GameSession, String> { | ||
| let json = json.to_owned(); | ||
| let restore = std::thread::Builder::new() | ||
| .name("phase-session-restore".to_owned()) | ||
| .stack_size(PERSISTED_SESSION_RESTORE_STACK_BYTES) | ||
| .spawn(move || { | ||
| let persisted = serde_json::from_str::<server_core::PersistedSession>(&json) | ||
| .map_err(|error| error.to_string())?; | ||
| Ok(GameSession::from_persisted(persisted, db.as_ref())) | ||
| }) | ||
| .map_err(|error| format!("could not start restore thread: {error}"))?; | ||
| restore | ||
| .join() | ||
| .map_err(|_| "persisted session restore thread panicked".to_owned())? | ||
| } | ||
|
|
||
| #[cfg(not(windows))] | ||
| fn restore_persisted_session(json: &str, db: SharedDb) -> Result<GameSession, String> { | ||
| let persisted = serde_json::from_str::<server_core::PersistedSession>(json) | ||
| .map_err(|error| error.to_string())?; | ||
| Ok(GameSession::from_persisted(persisted, db.as_ref())) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the restore helper and its caller.
rg -n "restore_persisted_session|delete_session|PersistedSession|from_persisted" crates/phase-server/src/main.rs
# Show the relevant surrounding lines for the restore helper.
sed -n '100,170p' crates/phase-server/src/main.rs
# Show the caller area around the delete_session logic.
sed -n '1020,1060p' crates/phase-server/src/main.rsRepository: phase-rs/phase
Length of output: 6075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1028,1095p' crates/phase-server/src/main.rsRepository: phase-rs/phase
Length of output: 3727
Don’t delete persisted sessions on generic restore errors. restore_persisted_session collapses JSON parse, thread spawn/join, and panic failures into Err(String), but the caller deletes the row for every Err. A transient restore failure can permanently erase a recoverable session; only delete confirmed-invalid snapshots and keep operational failures.
🤖 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 `@crates/phase-server/src/main.rs` around lines 116 - 139, Update
restore_persisted_session to distinguish invalid persisted-session data from
operational failures such as thread creation, join, or panic errors, using a
typed error or equivalent classification instead of collapsing all failures into
String. Adjust its caller to delete the persisted row only for confirmed-invalid
snapshots, while retaining it for recoverable operational restore failures.
Source: Path instructions
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
1 participant
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.
Summary by CodeRabbit
Performance
Bug Fixes
Diagnostics