Repository navigation
Conversation
Confetti3
left a comment
There was a problem hiding this comment.
Thanks for this. An explicit Ready/Failed result instead of a bare event, unnamed handles duplicated only into the game with restricted access, one cleanup path for every failure after process creation, and refusing old payloads are all good changes, and the tests use real child processes. Reviewed at c775922. A few things before this can go in:
1. A crash during startup now loses its crash handling. Before this PR, a game that exited before the ready signal still came back as a handle, so the exit-code path at src/launcher/launcher.cxx:771-781 ran: the exit code was logged by name, NoteWindowsCrashDump pointed at any Windows dump, and the player got launcher.game_crashed, which asks for the crash files. Now Await returns ERROR_PROCESS_ABORTED and wWinMain (launcher.cxx:751-756) only special-cases 0xC0000139 and Steam running as administrator, so a hook crash before fMain::Initialize shows the generic launcher.start_failed that points only at launcher.log, even though the game wrote sf4e-crash-*.dmp. Please classify an early exit with the same rules as a normal exit (one shared helper for IsCrashExit, the name log, the dump note and the message choice) so both paths agree.
2. Cleanup does not check that the game stopped. The fail lambda (launcher.cxx:337-347) ignores the results of TerminateProcess and the 5-second wait, closes the handle and returns, and the new messages then say "The failed launch was stopped." This is already better than before (the old code did not wait at all), but if the wait does not return WAIT_OBJECT_0 the launcher should log that and not tell the player the launch was stopped, since a retry could start while the old process is still there.
3. Please share one file hasher instead of adding another. HashFile (src/common/GameCompatibility.hxx:14) is the fourth SHA-256 file hasher, next to GetHash (src/sf4e/sf4e.cxx:44), ComputeFileSha256Hex (src/launcher/update/github_release_download.cxx:12) and HashFile (src/launcher/update/PackageInstaller.cxx:101), each with its own buffer size, cleanup and error handling. Since yours takes an already-open handle, it is the right shape to become the shared helper: move it to its own small header, use it for the compatibility check, and replace Sidecar's 175-line GetHash with it (open the DLL, hash the handle). Keep the size check, fingerprint and locking in GameFile. Do not switch to the existing path-based helpers here, since reopening the path would give up the point of hashing the handle you hold.
Related: Sidecar re-hashes the 7 MB executable inside DLL_PROCESS_ATTACH (src/sidecar/sidecar.cxx:124). That is the same BCryptOpenAlgorithmProvider call GetHash already makes from the same DllMain, so it does not add a new kind of risk, but it is work under loader lock that the launcher has already done on the same path while holding the file against writes until the handshake ends. Consider dropping the Sidecar re-check; if you prefer to keep it as a second line, at least have it use the shared hasher.
4. Suggestion: return one launch outcome instead of rebuilding it from Win32 codes. Await folds a Sidecar-reported failure, a wait failure, a timeout, an incomplete report and an early exit into {error, processExit} (src/common/StartupHandshake.hxx:9), and CreateSF4Process then mixes those with validation and process-creation errors through two out-parameters. wWinMain has to guess the cause from the code, so for example a Sidecar hashing failure (ERROR_INVALID_DATA) or a sharing violation on the executable ends up as the generic "startup or injection failed". A small result type with an explicit outcome (unsupported game, validation I/O error, Sidecar failed with code, timeout, early exit with code, create failed with code) plus one launcher-side function that picks the message would remove the conditional chain, fix item 1 naturally, and give the handshake test a stable contract to check.
Still to check in the game once these are in: a normal launch with the executable handle held, a forced hook failure and a forced compatibility failure reaching the right message, a slow first start against the new 60-second limit, and a crash before the ready signal.
Extract this focused change from the locally tested development build. Keep other features out of the upstream review; the pull request records implementation details, prior integration evidence, and outstanding isolated validation.
c775922 to
ec1ce4f
Compare
Keep every key and translated value intact while avoiding unrelated PR additions at the shared country-list tail.
This rejects unsupported game executables before injection and makes startup success or failure explicit. A launcher no longer treats every signaled event as a successful hook installation, silently continues after a failed Sidecar hash, or offers a retry while its previous child may still be running.
Compatibility and startup protocol
Review changes
Rebased on release cd4daa5, preserving current replay handoff, crash reporting, controller initialization and runtime changes. The Ready publication remains at the established platform initialization point; it does not claim the full game or a match has finished loading.
Validation
Remaining acceptance checks
A full Discord-SDK-enabled build and real injected-game first launch, forced hook failure, slow startup/timeout and native early-crash recovery remain manual acceptance checks. Synthetic child tests and the owned executable check are not substitutes for those. The fingerprint intentionally accepts only the documented executable; additional versions need verified evidence before adding them.
Compatibility update (17 October 2026)
Moved this feature's localization entries beside the existing launcher diagnostics in all 14 catalogs. Keys and translations are unchanged; unrelated PRs no longer append competing blocks at the same location.
Checked all 16 open contribution PRs against
releaseatcd4daa5c2fb19ee5a3ecd14d0e2d9cbee753aa4f. All 120 PR pairs merge cleanly; three complete merge orders produce the same source tree. The four dependent PRs document the additional branch refresh required after a prerequisite squash merge. These checks cover the exact branch heads linked in this update, not future upstream edits.Validation of the combined tree:
relay_unreachable; its full rerun passed on unchanged code. Both logs are retained.The current pass did not run the full Discord SDK, Inno Setup installer integration, a physical-controller session inside SSFIV, or a real match between two PCs. No new release assets or private test package were published. Earlier feature-specific results above remain identified separately where applicable.