[Agent] fix(crash): synchronous stopEmulation in crash handler; atomic JPEG write#2888
Conversation
github-actions
bot
added
agent-work
|
🤖 PR created. AI review starting automatically. Changes in #2888:
|
| // guaranteed to run here — the Swift concurrency runtime cannot schedule them | ||
| // before the process is killed. | ||
| // | ||
| // autoSaveState() is intentionally omitted: it requires async Realm database |
There was a problem hiding this comment.
saving on crashing was working in realm before, is there other code path that was/is covering it?
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
06d536a to
40b96a2
Compare
Ready for Human ReviewCompleted
|
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
40b96a2 to
ccd40e6
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review — Cycle 1/3
Reviewer: claude-opus-4-6
Summary: This PR correctly fixes two issues from #2766: (1) the uncaughtExceptionHandler previously used Task.detached which is dead code in a crash handler since the Swift concurrency runtime cannot schedule tasks before process termination — now replaced with a synchronous stopEmulation() call, and (2) save-state screenshot JPEG writes now use .atomic to prevent partial/corrupt files on interruption. Both changes are well-reasoned, minimal, and improve crash-time safety.
🔴 CRITICAL — none | 🟠 MAJOR — none
🟡 MINOR — PVEmulatorViewController.swift:52 — stopEmulation() is annotated @mainactor (see PVEmulatorCore+RunLoop.swift:38-39), but the crash handler runs on the faulting thread which may not be the main thread. In normal code this would be a concurrency violation, but in a crash handler context where the process is dying, this is the correct pragmatic trade-off. Worth a brief inline comment noting the deliberate off-main-actor call.
⚪ NIT — none
Verdict: APPROVE ✅
|
No description provided. |
github-actions
bot
added
ready-for-review
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
ccd40e6 to
fcb2c5c
Compare
There was a problem hiding this comment.
AI Code Review Cycle 1/3 - Reviewer: claude-opus-4-6 - APPROVE. This PR correctly fixes two issues from PR 2766: (1) uncaughtExceptionHandler previously wrapped stopEmulation() in a Task.detached that could never execute during crash recovery, now calls it synchronously; (2) save-state JPEG writes now use .atomic to prevent partial files. MINOR: stopEmulation() is @mainactor but the crash handler runs on the faulting thread - correct trade-off since old async code was a no-op, but may warn under strict concurrency. No CRITICAL or MAJOR issues.
|
No description provided. |
|
@claude saving on crashing was working in realm before, is there other code path that was/is covering it? i noticed you removed the async call, but will this break saving on quit/crash? it's currently working actually, double check this will still work. also make sure it works with the future swiftdata backend so it needs to be abstracted like the other database migration process #2510 |
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
fcb2c5c to
4e88547
Compare
There was a problem hiding this comment.
AI Code Review Cycle 1/3 - Reviewer: claude-opus-4-6 -- Summary: Well-reasoned correctness fix for the crash handler in uncaughtExceptionHandler. The previous Task.detached was dead code that would never run during crash recovery. The fix correctly replaces this with a synchronous stopEmulation() call. The .atomic write for save-state screenshots is a straightforward durability improvement. -- CRITICAL: none. MAJOR: none. MINOR: stopEmulation() is declared @mainactor on PVEmulatorCore, but uncaughtExceptionHandler may be called on any thread. In a crash handler this is the correct tradeoff. A defensive comment noting the intentional @mainactor bypass would help future readers. NIT: none. -- Verdict: APPROVE
|
No description provided. |
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
4e88547 to
b907793
Compare
Analysis + ChangesSave-on-quit/crash code pathsThere were multiple separate code paths covering saves — the normal quit path was never touched by this PR:
Key point: The SwiftData abstraction (#2510)Extracted the Realm write from public protocol SaveStatePersistenceServiceProtocol: AnyObject {
func registerSaveState(
gameID: String, coreIdentifier: String,
file: PVFile, imageFile: PVImageFile?, isAutosave: Bool
) async throws -> String
}
To swap in SwiftData when #2510 lands: create |
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
1c2a900 to
9818c9a
Compare
|
AI review approved — auto-merge enabled, @JoeMatt assigned for final review. |
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
9818c9a to
cdbcb42
Compare
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
cdbcb42 to
efcfd7a
Compare
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
efcfd7a to
59430a1
Compare
…rite
- uncaughtExceptionHandler: replace Task.detached { @mainactor } with a
direct synchronous stopEmulation() call. Async tasks are never scheduled
after an uncaught exception — the process terminates immediately after
the handler returns, so any Task created here is a dead letter.
autoSaveState() is intentionally dropped from the crash path: it requires
async Realm writes that cannot complete safely during crash recovery and
risk database corruption if interrupted mid-write.
- createNewSaveState: write the save-state screenshot JPEG with the .atomic
option (write-to-temp + rename) to prevent a partial/corrupt image file
if the process is interrupted during the write.
Part of #2766
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ta compat Extract the Realm database write out of createNewSaveState (PVUI) into a new SaveStatePersistenceServiceProtocol, with RomDatabase as the Realm- backed conformance. - New SaveStatePersistenceService.swift: defines the protocol so PVUI emulator code no longer calls RomDatabase/Realm directly - RomDatabase+Saves.swift: implements registerSaveState() using withCheckedThrowingContinuation to bridge asyncWriteTransaction into proper async/await; freeze()s PVSaveState before off-thread serialise - PVEmulatorControllerProtocol: exposes saveStatePersistenceService (defaulting to RomDatabase.sharedInstance) and calls it from createNewSaveState; cleanupOldAutoSaves moved to RomDatabase layer - Normal quit() path (autoSaveState) is unchanged and unaffected - Crash handler (uncaughtExceptionHandler) intentionally calls only synchronous stopEmulation() — async saves cannot complete before the process terminates after an uncaught exception Swapping in a SwiftData backend (#2510) now only requires a new conformance to SaveStatePersistenceServiceProtocol with no PVUI changes. Part of #2766 / #2510 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
force-pushed
the
fix/crash-save-mainactor-atomic
branch
from
59430a1 to
f0309d7
Compare
There was a problem hiding this comment.
AI Code Review Cycle 1/3. Reviewer: claude-opus-4-6. CRITICAL: RomDatabase+Saves.swift:33-38 Continuation leak if asyncWriteTransaction fails to obtain a Realm instance. The registerSaveState method wraps asyncWriteTransaction in withCheckedThrowingContinuation. If asyncWriteTransaction cannot create a Realm instance (RomDatabase.swift lines 711-713), it returns without calling the block, meaning the continuation is never resumed and the calling Task hangs indefinitely. Fix by guarding against this before entering the continuation. MINOR: SaveStatePersistenceService.swift:14 imports PVRealm coupling the abstraction to Realm types. RomDatabase+Saves.swift:89 cleanupOldAutoSaves creates new Realm() relying on thread-local write transaction. NIT: RomDatabase+Saves.swift:13 old typo recoovery remains. Verdict: REQUEST_CHANGES.
|
No description provided. |
- RomDatabase+Saves: pre-obtain Realm before withCheckedThrowingContinuation so asyncWriteTransaction's silent early-return can never leave the continuation dangling (CRITICAL) - RomDatabase+Saves: pass realm instance to cleanupOldAutoSaves instead of creating a new Realm() inside the write block (MINOR) - RomDatabase+Saves: fix typo "recoovery" → "recovery" (NIT) - SaveStatePersistenceService: add explanatory comment about PVRealm import coupling (MINOR) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
✅ Fixes pushed — addressed all AI review issues: CRITICAL fixed: MINOR fixed: NIT fixed: Typo MINOR addressed: Added an explanatory comment in |
There was a problem hiding this comment.
AI Code Review - Cycle 1/3 | Reviewer: claude-opus-4-6
Summary: Solid correctness fixes for crash-time safety and save-state data integrity. The uncaughtExceptionHandler change correctly removes a Task.detached that could never execute in a crash handler context - only the synchronous stopEmulation() (which flushes battery/SRAM) belongs here. The .atomic JPEG write prevents partial screenshot files. The SaveStatePersistenceServiceProtocol abstraction is a reasonable preparatory step for SwiftData migration, cleanly decoupling PVUI from direct Realm calls.
CRITICAL - none | MAJOR - none
MINOR - RomDatabase+Saves.swift:55-80 - The continuation is resumed in defer inside the realm.writeAsync block, which executes before the Realm commit completes. If the commit fails, the caller sees success and the error is silently lost. In practice Realm commits very rarely fail and the original asyncWriteTransaction pattern had the same fire-and-forget behavior, so this is not a regression - but worth noting for the eventual SwiftData migration.
NIT - SaveStatePersistenceService.swift:29 - Typo in doc comment PVEmualatorControllerProtocol (missing l in Emulator) - mirrors the existing typo in the protocol name.
Verdict: APPROVE
|
No description provided. |
1 participant
Summary
Follow-up to #2766, fixing two correctness issues introduced by that PR:
uncaughtExceptionHandler— removedTask.detached { @MainActor }(dead letter):An NSException handler is called on the faulting thread immediately before the process
terminates. Any
Taskcreated here is never scheduled — the Swift concurrency runtimecannot dispatch it before the process dies. The
@MainActorhop makes it even less likelyto run if the crash occurred on the main thread. The fix calls
core.stopEmulation()synchronously and drops
autoSaveState()from the crash path entirely: that functionrequires async Realm writes that (a) cannot complete before termination and (b) risk
Realm database corruption if interrupted mid-write.
createNewSaveState— atomic JPEG write for save-state screenshot:The save-state thumbnail was written with
data.write(to: url)(no options), which writesthe file in place. If the process is killed between opening the file and finishing the write,
the result is a partial/corrupt image. Using
.atomic(write-to-temp + rename) ensures thefile is either fully written or not present — no partial state.
Changed files
PVEmulatorViewController.swiftuncaughtExceptionHandler: replaced asyncTask.detached { @MainActor }with synchronousstopEmulation()PVEmulatorControllerProtocol.swiftcreateNewSaveState: screenshot JPEG uses.atomicwrite optionCHANGELOG.md### FixedWhy
autoSaveState()is not called in the crash handlerautoSaveState()ultimately callscore.saveState(toFileAtPath:)(async, core-level) andRomDatabase.sharedInstance.asyncWriteTransaction { … }(Realm async write). Neither cancomplete in a signal/exception handler context. Attempting them risks:
.svsfile left on disk (truncated/corrupt save state)The battery (SRAM) data is the critical save — it is flushed synchronously by
stopEmulation(). The save-state snapshot is a convenience feature that cannot besafely created at crash time.
Test plan
Part of #2766
🤖 Generated with Claude Code