Skip to content

Commit dfbb2c7

Browse files
TideGearclaude
andcommitted
fix: address sixth round of PR review findings
VcRedistStep: per-year markers now also encode architecture (`$year-x86` / `$year-x64`, plus `legacy-x86` / `legacy-x64` for yearless paths). Without this, an x86-only install (e.g. vc_redist.x86.exe for 2015) wrote a year-only marker that incorrectly suppressed the matching x64 install on a later game. Yearless redists (A:\redist\vcredist_*, A:\_CommonRedist\VC_redist.*) are similarly distinguished. archKey() inspects the win path for x64/amd64/wow64/ x86_64 hints (and explicit .x64./.x86. filenames) and defaults to x86 for ambiguous paths. Round-5 archless markers are migrated as `_$year-x86` only — conservative: any actual x64 install will re-run once. appliesTo no longer short-circuits on the coarse game-dir VCREDIST_INSTALLED marker when per-year/ arch markers indicate a missing (year, arch) pair; the game-dir marker is now only a fallback when no per-year markers exist or containerRoot is null. WinHandler.GET_PROCESS dispatch: each listener invocation (slot listener and each multi-listener entry) is now wrapped in try/catch so one bad listener can't kill the worker thread. Same fix applied to the listProcesses() send- failure broadcast. WinHandler.exec(String): handle quoted paths. A leading `"` triggers quoted- path parsing (filename = quoted substring; parameters = trimmed remainder); unquoted commands still split on the first whitespace. Without this, "C:\Program Files (x86)\Steam\steam.exe" -shutdown shredded into a bogus filename of "C:\Program". XServerScreen.requestWineProcessSnapshot: distinguish request/send failure from a legitimate empty process list. WinHandler.listProcesses() emits (0, 0, null) only when sendPacket() fails — Wine itself never broadcasts empty. Added ProcessSnapshotException, listener completes deferred exceptionally on (count==0 && info==null), await maps it to null so isSteamExeAlive(null) == true keeps the polling loop alive instead of falsely concluding Steam exited. WinHandler + XServerScreen: add a snapshot-collection mutex (processSnapshotMutex). Both Kotlin pollers (requestWineProcessSnapshot and startExitWatchForUnmappedGameWindow) now serialize their add-listener → listProcesses → await → remove-listener sequence so concurrent listProcesses calls don't cross-pollute each other's deferreds. Multi-listener support is preserved for long-running watchers; only the per-iteration snapshot is serialized. LudusaviRegistry.ensureLoaded: extend the empty-set guard to the disk-cache fallback. A poisoned `{}` cache file (e.g., one written before the round-5 empty-fetch guard shipped) used to populate memoryCache with emptyMap(), after which the fast path short-circuited forever. Now: if the parsed disk cache is empty, log and delete the poisoned file (best-effort), fall through to null so the next ensureLoaded retries the fetch. Parser exceptions still leave the file alone. WindowManager.reapLeakedClientWindows: add the 1x1 parent-size predicate to match the compositor's full orphan-chain marker (dd3987b). Without it, a legitimate window whose parent happens to be blank-class+pid==0 but is NOT a 1x1 orphanage stub could still be reaped. PluviaMain SDK bridge prompt: drop the dead `persisted` boolean computed in onConfirmClick / onActionClick (round-5 changed every relaunch(...) to relaunch(true), so the Boolean is no longer read). Side effects and the existing onFailure Timber.w log are preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent ffba233 commit dfbb2c7

7 files changed

Lines changed: 262 additions & 66 deletions

File tree

app/src/main/java/app/gamenative/ui/PluviaMain.kt

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -821,12 +821,13 @@ fun PluviaMain(
821821
val rec = runCatching {
822822
SteamUtils.getRecommendedSdkCloudSaveSubdirAsync(context, gameId)
823823
}.getOrNull()
824-
val persisted = rec != null && runCatching {
825-
val container = ContainerUtils.getContainer(context, state.launchedAppId)
826-
container.sdkCloudSaveSubdir = rec.subdir
827-
container.saveData()
828-
}.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") }
829-
.isSuccess
824+
if (rec != null) {
825+
runCatching {
826+
val container = ContainerUtils.getContainer(context, state.launchedAppId)
827+
container.sdkCloudSaveSubdir = rec.subdir
828+
container.saveData()
829+
}.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") }
830+
}
830831
withContext(Dispatchers.Main) { relaunch(true) }
831832
}
832833
}
@@ -841,12 +842,11 @@ fun PluviaMain(
841842
// Don't ask again for this game.
842843
msgDialogState = MessageDialogState(false)
843844
CoroutineScope(Dispatchers.IO).launch {
844-
val persisted = runCatching {
845+
runCatching {
845846
val container = ContainerUtils.getContainer(context, state.launchedAppId)
846847
container.putExtra("sdkCloudBridgePromptDismissed", "1")
847848
container.saveData()
848849
}.onFailure { Timber.w(it, "Failed to persist sdkCloudBridgePromptDismissed") }
849-
.isSuccess
850850
withContext(Dispatchers.Main) { relaunch(true) }
851851
}
852852
}

app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt

Lines changed: 39 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ import kotlinx.coroutines.Job
177177
import kotlinx.coroutines.delay
178178
import kotlinx.coroutines.launch
179179
import kotlinx.coroutines.runBlocking
180+
import kotlinx.coroutines.sync.withLock
180181
import kotlinx.coroutines.withContext
181182
import kotlinx.coroutines.withTimeoutOrNull
182183
import org.json.JSONException
@@ -310,20 +311,27 @@ private fun buildEssentialProcessAllowlist(): Set<String> {
310311
return (essentialServices + CORE_WINE_PROCESSES).toSet()
311312
}
312313

314+
private class ProcessSnapshotException(msg: String) : Exception(msg)
315+
313316
private suspend fun requestWineProcessSnapshot(winHandler: WinHandler): List<ProcessInfo>? {
314317
val lock = Any()
315318
var currentList = mutableListOf<ProcessInfo>()
316319
var expectedCount = 0
317320
val deferred = CompletableDeferred<List<ProcessInfo>?>()
318321

319-
// Register via add/removeOnGetProcessInfoListener so we don't clobber
320-
// other concurrent watchers (e.g. startExitWatchForUnmappedGameWindow,
321-
// or another awaitSteamShutdown poll on the same WinHandler) that also
322-
// need process-info events.
323322
val listener = OnGetProcessInfoListener { index, count, processInfo ->
324323
synchronized(lock) {
324+
// (0, 0, null) is only emitted by WinHandler.listProcesses() when
325+
// the underlying UDP send fails; wine itself never broadcasts an
326+
// empty list. Treating it as a successful empty snapshot would let
327+
// awaitSteamShutdown conclude Steam exited on a transient send
328+
// failure, so surface it as an exception instead.
325329
if (count == 0 && processInfo == null) {
326-
if (!deferred.isCompleted) deferred.complete(emptyList())
330+
if (!deferred.isCompleted) {
331+
deferred.completeExceptionally(
332+
ProcessSnapshotException("request/send failure"),
333+
)
334+
}
327335
return@synchronized
328336
}
329337
if (index == 0) {
@@ -343,14 +351,23 @@ private suspend fun requestWineProcessSnapshot(winHandler: WinHandler): List<Pro
343351
}
344352
}
345353

346-
return try {
354+
// Serialize the add/list/await/remove sequence against any other concurrent
355+
// snapshot caller; the wire protocol broadcasts GET_PROCESS responses to
356+
// every registered listener with no request id to disambiguate.
357+
return winHandler.processSnapshotMutex.withLock {
347358
winHandler.addOnGetProcessInfoListener(listener)
348-
winHandler.listProcesses()
349-
withTimeoutOrNull(EXIT_PROCESS_RESPONSE_TIMEOUT_MS) {
350-
deferred.await()
359+
try {
360+
winHandler.listProcesses()
361+
withTimeoutOrNull(EXIT_PROCESS_RESPONSE_TIMEOUT_MS) {
362+
try {
363+
deferred.await()
364+
} catch (e: ProcessSnapshotException) {
365+
null
366+
}
367+
}
368+
} finally {
369+
winHandler.removeOnGetProcessInfoListener(listener)
351370
}
352-
} finally {
353-
winHandler.removeOnGetProcessInfoListener(listener)
354371
}
355372
}
356373

@@ -859,12 +876,17 @@ fun XServerScreen(
859876
val startTime = System.currentTimeMillis()
860877
while (System.currentTimeMillis() - startTime < EXIT_PROCESS_TIMEOUT_MS) {
861878
val deferred = CompletableDeferred<List<ProcessInfo>?>()
862-
synchronized(lock) {
863-
pendingSnapshot = deferred
864-
}
865-
winHandler.listProcesses()
866-
val snapshot = withTimeoutOrNull(EXIT_PROCESS_RESPONSE_TIMEOUT_MS) {
867-
deferred.await()
879+
// Serialize against any other listProcesses-driven caller so a
880+
// concurrent awaitSteamShutdown poll doesn't deliver its
881+
// GET_PROCESS responses into this watcher's pending snapshot.
882+
val snapshot = winHandler.processSnapshotMutex.withLock {
883+
synchronized(lock) {
884+
pendingSnapshot = deferred
885+
}
886+
winHandler.listProcesses()
887+
withTimeoutOrNull(EXIT_PROCESS_RESPONSE_TIMEOUT_MS) {
888+
deferred.await()
889+
}
868890
}
869891
if (snapshot != null) {
870892
val hasNonEssential = snapshot.any {

app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,19 @@ object LudusaviRegistry {
9393
if (cacheFile.isFile) {
9494
runCatching { parseCacheJson(cacheFile.readText()) }
9595
.onSuccess {
96-
memoryCache = it
97-
Timber.w("LudusaviRegistry: using stale disk cache (${it.size} entries) after fetch failure")
98-
return@withLock it
96+
// Mirror the empty-fetch guard above: a 0-entry disk cache is a
97+
// poisoned/stale artifact (e.g. a `{}` file from before this guard
98+
// shipped). Promoting it to memoryCache would let the fast path
99+
// short-circuit forever. Delete the file so a future fetch can
100+
// replace it, and fall through to return null.
101+
if (it.isEmpty()) {
102+
Timber.w("LudusaviRegistry: disk cache parsed to 0 entries; deleting poisoned file")
103+
runCatching { cacheFile.delete() }
104+
} else {
105+
memoryCache = it
106+
Timber.w("LudusaviRegistry: using stale disk cache (${it.size} entries) after fetch failure")
107+
return@withLock it
108+
}
99109
}
100110
}
101111
return@withLock null

app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt

Lines changed: 61 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -69,26 +69,32 @@ object VcRedistStep : PreInstallStep {
6969
gameDirPath: String,
7070
): Boolean {
7171
// vcredist installs system-wide into the Wine prefix, not per-game. We
72-
// track installed years at the container root (one marker per year)
73-
// so reinstalling the game (which wipes the game dir) doesn't force a
74-
// redundant re-run, while still detecting when a *different* game
75-
// bundles a year we have not yet installed.
72+
// track installed years/arches at the container root (one marker per
73+
// year+arch) so reinstalling the game (which wipes the game dir)
74+
// doesn't force a redundant re-run, while still detecting when a
75+
// *different* game bundles a year/arch we have not yet installed.
7676
val gameDir = File(gameDirPath)
7777
val required = requiredVersions(gameDir)
7878
if (required.isEmpty()) {
79-
// Nothing to install: treat the step as satisfied.
8079
return false
8180
}
8281
val containerRoot = container.rootDir?.absolutePath
8382
migrateLegacyContainerMarker(containerRoot, required)
83+
migrateLegacyArchlessMarkers(containerRoot)
8484
val installed = installedVersions(containerRoot)
85+
if (containerRoot != null && installed.isNotEmpty()) {
86+
// Per-year/arch markers are the source of truth here. The coarse
87+
// game-dir marker is ignored — if anything required is missing we
88+
// must run buildCommand for the missing set.
89+
return required.any { it !in installed }
90+
}
8591
val missing = required - installed
8692
if (missing.isEmpty()) {
87-
// All required years already covered by container-level markers.
8893
return false
8994
}
90-
// Fall back to the legacy game-dir marker so an in-flight install
91-
// that already ran for this exact game directory still short-circuits.
95+
// No per-year markers to consult: fall back to the legacy game-dir
96+
// marker so an in-flight install that already ran for this exact game
97+
// directory still short-circuits.
9298
return !MarkerUtils.hasMarker(gameDirPath, Marker.VCREDIST_INSTALLED)
9399
}
94100

@@ -125,6 +131,7 @@ object VcRedistStep : PreInstallStep {
125131
gameDirPath: String,
126132
): String? {
127133
val containerRoot = container.rootDir?.absolutePath
134+
migrateLegacyArchlessMarkers(containerRoot)
128135
val installed = installedVersions(containerRoot)
129136
val parts = mutableListOf<String>()
130137
for ((winPath, args) in vcRedistMap) {
@@ -134,11 +141,11 @@ object VcRedistStep : PreInstallStep {
134141
if (lastSep < 0) continue
135142
val hostFile = File(gameDir, rest.replace('\\', '/'))
136143
if (!hostFile.isFile) continue
137-
// Skip installer entries for years already installed system-wide
138-
// so we don't pop a fresh installer window per launch (the
139-
// 963d7999 fix). Years we haven't seen still run.
144+
// Skip installer entries for year+arch combos already installed
145+
// system-wide so we don't pop a fresh installer window per launch
146+
// (the 963d7999 fix). Year+arch combos we haven't seen still run.
140147
val version = versionKey(winPath)
141-
if (version != null && installed.contains(version)) continue
148+
if (installed.contains(version)) continue
142149
parts.add(if (args.isEmpty()) winPath else "$winPath $args")
143150
}
144151
return if (parts.isEmpty()) null else parts.joinToString(" & ")
@@ -170,7 +177,7 @@ object VcRedistStep : PreInstallStep {
170177
if (rest.lastIndexOf('\\') < 0) continue
171178
val hostFile = File(gameDir, rest.replace('\\', '/'))
172179
if (!hostFile.isFile) continue
173-
versionKey(winPath)?.let { out.add(it) }
180+
out.add(versionKey(winPath))
174181
}
175182
return out
176183
}
@@ -192,23 +199,55 @@ object VcRedistStep : PreInstallStep {
192199
}
193200

194201
/**
195-
* Map an installer's Windows path to a stable version key. Recognises
196-
* year-suffixed folders ("2005".."2022"). Paths with no clear year
197-
* (legacy `A:\redist\…`, root-level `A:\_CommonRedist\VC_redist.*`) map
198-
* to "legacy" so they are still tracked, just with a single shared key.
202+
* Map an installer's Windows path to a stable "$year-$arch" key (or
203+
* "legacy-$arch" when no year is present). x86 and x64 installers from
204+
* the same year get distinct keys so installing one does not mask the
205+
* other. When no architecture hint is detectable we default to x86,
206+
* matching the most common older redistributable layout.
199207
*/
200-
private fun versionKey(winPath: String): String? {
208+
private fun versionKey(winPath: String): String {
201209
val lower = winPath.lowercase()
210+
val arch = archKey(lower)
202211
for (year in YEAR_KEYS) {
203212
if (lower.contains("\\$year\\") || lower.contains("\\msvc$year\\") ||
204213
lower.contains("\\msvc${year}_x64\\")
205214
) {
206-
return year
215+
return "$year-$arch"
216+
}
217+
}
218+
return "legacy-$arch"
219+
}
220+
221+
private fun archKey(lowerWinPath: String): String {
222+
val x64Hints = listOf("x64", "amd64", "wow64", "x86_64", "_x64\\", ".x64.")
223+
if (x64Hints.any { lowerWinPath.contains(it) }) return "x64"
224+
if (lowerWinPath.contains("x86") || lowerWinPath.contains(".x86.")) return "x86"
225+
return "x86"
226+
}
227+
228+
/**
229+
* One-time upgrade for markers written by the prior round-5 fix that were
230+
* keyed by year only (e.g. `.vcredist_installed_2005`, `.vcredist_installed_legacy`).
231+
* We don't know which arch was actually installed back then, so we
232+
* conservatively claim only x86 coverage; if x64 was also installed it
233+
* will be re-run on the next launch (one redundant installer at most).
234+
*/
235+
private fun migrateLegacyArchlessMarkers(containerRoot: String?) {
236+
if (containerRoot == null) return
237+
val dir = File(containerRoot)
238+
val files = dir.listFiles() ?: return
239+
for (f in files) {
240+
if (!f.isFile) continue
241+
val name = f.name
242+
if (!name.startsWith(PER_VERSION_MARKER_PREFIX)) continue
243+
val suffix = name.substring(PER_VERSION_MARKER_PREFIX.length)
244+
if (suffix.contains('-')) continue
245+
val migrated = File(containerRoot, "$PER_VERSION_MARKER_PREFIX$suffix-x86")
246+
if (!migrated.exists()) {
247+
runCatching { migrated.createNewFile() }
207248
}
249+
runCatching { f.delete() }
208250
}
209-
// Generic / yearless installers — track under a shared key so they
210-
// don't all collapse to "no key" and bypass the marker check.
211-
return "legacy"
212251
}
213252

214253
private val YEAR_KEYS = listOf(

app/src/main/java/com/winlator/winhandler/WinHandler.java

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -293,9 +293,24 @@ public void exec(String command) {
293293
if (command2.isEmpty()) {
294294
return;
295295
}
296-
String[] cmdList = command2.split(" ", 2);
297-
final String filename = cmdList[0];
298-
final String parameters = cmdList.length > 1 ? cmdList[1] : "";
296+
final String filename;
297+
final String parameters;
298+
// A naive split(" ", 2) would shred Windows paths like
299+
// "C:\Program Files (x86)\Steam\steam.exe" -shutdown.
300+
if (command2.charAt(0) == '"') {
301+
int closing = command2.indexOf('"', 1);
302+
if (closing > 0) {
303+
filename = command2.substring(1, closing);
304+
parameters = command2.substring(closing + 1).trim();
305+
} else {
306+
filename = command2.substring(1);
307+
parameters = "";
308+
}
309+
} else {
310+
String[] cmdList = command2.split(" ", 2);
311+
filename = cmdList[0];
312+
parameters = cmdList.length > 1 ? cmdList[1] : "";
313+
}
299314
exec(filename, parameters);
300315
}
301316

@@ -345,10 +360,18 @@ public void listProcesses() {
345360
if (!sendPacket(CLIENT_PORT)) {
346361
OnGetProcessInfoListener slotListener = this.onGetProcessInfoListener;
347362
if (slotListener != null) {
348-
slotListener.onGetProcessInfo(0, 0, null);
363+
try {
364+
slotListener.onGetProcessInfo(0, 0, null);
365+
} catch (Throwable t) {
366+
Log.w(TAG, "process info listener threw", t);
367+
}
349368
}
350369
for (OnGetProcessInfoListener l : extraProcessInfoListeners) {
351-
l.onGetProcessInfo(0, 0, null);
370+
try {
371+
l.onGetProcessInfo(0, 0, null);
372+
} catch (Throwable t) {
373+
Log.w(TAG, "process info listener threw", t);
374+
}
352375
}
353376
}
354377
});
@@ -470,6 +493,26 @@ public void removeOnGetProcessInfoListener(OnGetProcessInfoListener listener) {
470493
extraProcessInfoListeners.remove(listener);
471494
}
472495

496+
/**
497+
* Coordinates listProcesses-driven snapshot collection so events from one
498+
* snapshot don't bleed into another concurrent snapshot's listener. The
499+
* wire protocol has no request id to disambiguate broadcast responses, so
500+
* a serializing primitive is the cheapest fix that preserves the existing
501+
* GET_PROCESS packet format.
502+
*
503+
* <p>Callers that wrap an {@code addOnGetProcessInfoListener} /
504+
* {@code listProcesses} / await / {@code removeOnGetProcessInfoListener}
505+
* sequence should hold this lock for the duration of the sequence.
506+
* Long-running listeners that don't drive listProcesses themselves don't
507+
* need to acquire it.
508+
*
509+
* <p>Exposed as a {@link kotlinx.coroutines.sync.Mutex} so Kotlin callers
510+
* can suspend on it without binding to a thread, which a JVM monitor would
511+
* forbid across coroutine suspension points.
512+
*/
513+
public final kotlinx.coroutines.sync.Mutex processSnapshotMutex =
514+
kotlinx.coroutines.sync.MutexKt.Mutex(false);
515+
473516
private void startSendThread() {
474517
Executors.newSingleThreadExecutor().execute(() -> {
475518
while (this.running) {
@@ -532,11 +575,21 @@ private void handleRequest(byte requestCode, final int port) throws IOException
532575
this.receiveData.get(bytes);
533576
String name = StringUtils.fromANSIString(bytes);
534577
ProcessInfo info = new ProcessInfo(pid, name, memoryUsage, affinityMask, wow64Process);
578+
// Isolate each listener so a misbehaving consumer can't kill the
579+
// receive thread and break future polling for everyone else.
535580
if (this.onGetProcessInfoListener != null) {
536-
this.onGetProcessInfoListener.onGetProcessInfo(index, numProcesses, info);
581+
try {
582+
this.onGetProcessInfoListener.onGetProcessInfo(index, numProcesses, info);
583+
} catch (Throwable t) {
584+
Log.w(TAG, "process info listener threw", t);
585+
}
537586
}
538587
for (OnGetProcessInfoListener l : extraProcessInfoListeners) {
539-
l.onGetProcessInfo(index, numProcesses, info);
588+
try {
589+
l.onGetProcessInfo(index, numProcesses, info);
590+
} catch (Throwable t) {
591+
Log.w(TAG, "process info listener threw", t);
592+
}
540593
}
541594
return;
542595
case RequestCodes.GET_GAMEPAD:

0 commit comments

Comments
 (0)