updates some features and fix an issue with program crashing#6
updates some features and fix an issue with program crashing#6
Conversation
…erly, adjust some robrowser commands and change compile mode to recompile on msbuild
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| try | ||
| { | ||
| var p = Process.GetProcessById(pid); | ||
| if (!p.HasExited) | ||
| KillAll(pid, type); | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
🔴 KillOrphanProcesses kills arbitrary processes due to no process identity validation
KillOrphanProcesses() at IProcess.cs:90-96 uses Process.GetProcessById(pid) and immediately kills the process if !p.HasExited, without verifying that the process is actually one of the expected server executables. Even in the crash scenario this feature is designed for, PIDs can be reused by the OS between sessions. A system reboot (common after a crash) guarantees PID reuse. The method should verify the process name matches the expected server executable (e.g., by comparing p.ProcessName against configured paths like Configuration.LoginPath, Configuration.MapPath, etc.) before calling KillAll.
| try | |
| { | |
| var p = Process.GetProcessById(pid); | |
| if (!p.HasExited) | |
| KillAll(pid, type); | |
| } | |
| catch { } | |
| try | |
| { | |
| var p = Process.GetProcessById(pid); | |
| if (!p.HasExited) | |
| { | |
| var expectedType = GetProcessType(p); | |
| if (expectedType == type) | |
| KillAll(pid, type); | |
| } | |
| } | |
| catch { } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (Properties.Settings.Default.DevMode) | ||
| cmd = @"npm run serve"; | ||
| else | ||
| cmd = @"npm run live"; | ||
| else | ||
| cmd = @"npm run serve"; |
There was a problem hiding this comment.
🚩 Swapped npm run serve/live commands for ROBrowser may be intentionally reversed
The PR swaps the npm run serve and npm run live commands for the ROBrowser case at MainWindow.xaml.cs:242-245. Previously DevMode used serve and non-DevMode used live; now it's reversed. Conventionally, serve is associated with development (hot reload, source maps) and live with production, so the original mapping (DevMode→serve) made more semantic sense. However, this may be project-specific and intentional — the commit message says 'adjust some robrowser commands'. Worth confirming with the author whether this swap was deliberate.
Was this helpful? React with 👍 or 👎 to provide feedback.
AoShinhoServ-Monitor.csproj
Outdated
| <PlatformTarget>x64</PlatformTarget> | ||
| <LangVersion>7.3</LangVersion> | ||
| <ErrorReport>prompt</ErrorReport> | ||
| <Prefer32Bit>true</Prefer32Bit> |
There was a problem hiding this comment.
📝 Info: Prefer32Bit inconsistency between Debug and Release x64 configurations
In the .csproj, Debug|x64 has <Prefer32Bit>false</Prefer32Bit> (line 49) while Release|x64 has <Prefer32Bit>true</Prefer32Bit> (line 58). While Prefer32Bit has no practical effect when PlatformTarget is explicitly set to x64 (it only applies to AnyCPU), the inconsistency between configurations is likely an oversight and could cause confusion.
Was this helpful? React with 👍 or 👎 to provide feedback.
| cmd = $@"msbuild {projectname} -t:rebuild -property:Configuration=Release /p:DefineConstants=""PRERE"""; | ||
| else | ||
| cmd = $@"msbuild {projectname} -t:build -property:Configuration=Release"; | ||
| cmd = $@"msbuild {projectname} -t:rebuild -property:Configuration=Release"; |
There was a problem hiding this comment.
📝 Info: msbuild -t:rebuild is a deliberate behavioral change from -t:build
The compile commands at MainWindow.xaml.cs:295-297 changed from -t:build (incremental) to -t:rebuild (clean + build). This is a significant behavioral change — rebuild always starts from scratch, which is slower but avoids stale object file issues. The commit message says 'change compile mode to recompile on msbuild', confirming this is intentional. Just noting that this will meaningfully increase compile times for users.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try | ||
| { | ||
| var p = Process.GetProcessById(pid); | ||
| if (!p.HasExited) | ||
| KillAll(pid, type); | ||
| } | ||
| catch { } | ||
| } |
There was a problem hiding this comment.
🚩 KillOrphanProcesses does not verify process identity before killing
Even in the crash-recovery scenario (the intended use case), KillOrphanProcesses() at IProcess.cs:92-94 calls Process.GetProcessById(pid) and checks !p.HasExited, but never verifies that the process at that PID is actually the expected server process (e.g., by comparing p.ProcessName). On Windows, PIDs are recycled, so after a crash + reboot, the saved PID could belong to an entirely unrelated process. A simple p.ProcessName check against the expected executable name would substantially reduce the risk of killing innocent processes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Debug|x64 = Debug|x64 | ||
| Release|x64 = Release|x64 |
There was a problem hiding this comment.
🚩 Default Platform is still AnyCPU but only x64 configurations are defined
The .csproj at line 6 still has <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform> as the default platform, but the PR removed all AnyCPU build configurations and only defines Debug|x64 and Release|x64. If someone builds without explicitly specifying the platform (e.g., msbuild with no /p:Platform flag), MSBuild will default to AnyCPU and find no matching configuration PropertyGroup, falling back to unconfigured defaults. The .sln file is updated to only list x64 configurations, so builds through Visual Studio should be fine, but command-line builds could behave unexpectedly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| case "build": | ||
| cmd = @"npm run build -- -O -T"; | ||
| cmd = @"npm run build -- -O"; |
There was a problem hiding this comment.
🚩 Removal of -T flag from npm build command
At MainWindow.xaml.cs:234, the npm build command changed from npm run build -- -O -T to npm run build -- -O, dropping the -T flag. Without knowing what -T does in the specific ROBrowser build tool, this is a silent behavioral change. If -T enabled tree-shaking, type-checking, or another optimization, its removal could produce larger/less-optimized output. The commit message doesn't explain this change.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ILogging.processesInfos.Add(info); | ||
| return; |
There was a problem hiding this comment.
📝 Info: CMake code path skips SaveTrackedPIDs
In RunWithRedirectCmdAsync, the CMake code path (MainWindow.xaml.cs:268-290) adds processes to ILogging.processesInfos via CreateCMake() and directly, then returns at line 290 before reaching IProcess.SaveTrackedPIDs() at line 332. This means CMake build processes won't be persisted to settings for orphan cleanup. This is a minor inconsistency in the new tracking feature — though CMake builds are typically short-lived, a long make install could become orphaned if the monitor crashes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }; | ||
|
|
||
| await Task.WhenAll(tasks); | ||
| IProcess.SaveTrackedPIDs(); |
There was a problem hiding this comment.
🚩 SaveTrackedPIDs not called after individual server process starts, only after all four complete
In the latest commit (3aa0391), SaveTrackedPIDs() was removed from RunWithRedirectAsync (server process launcher) and moved to Do_Run_All at MainWindow.xaml.cs:170, which only executes after all 4 server processes have started. This means if the monitor crashes while starting the 4 servers (e.g., process 1-3 started, process 4 fails/hangs), none of the PIDs will be persisted. In contrast, RunWithRedirectCmdAsync at MainWindow.xaml.cs:332 still calls SaveTrackedPIDs() per-process. This inconsistency creates a window where a crash during startup leaves orphan server processes untracked. The tradeoff was presumably to avoid a race condition noted in the commit message, but it weakens crash recovery for the primary use case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| public static void SaveTrackedPIDs() | ||
| { | ||
| lock (_trackLock) | ||
| { | ||
| var pids = string.Join(",", ILogging.processesInfos.ToArray().Select(p => $"{p.pID}:{p.type}")); | ||
| Properties.Settings.Default.TrackedPIDs = pids; | ||
| Properties.Settings.Default.Save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 SaveTrackedPIDs lock does not protect against concurrent List modifications
The _trackLock in SaveTrackedPIDs (IProcess.cs:75) protects the save operation, but ILogging.processesInfos is a plain List<T> that is concurrently modified from multiple threads: Parallel.ForEach in Do_Kill_All removes items, RunWithRedirectAsync adds items from background threads, and Proc_HasExited removes items via the dispatcher. The .ToArray() call at line 77 provides some protection against enumeration-during-modification, but List<T>.ToArray() itself is not thread-safe if another thread is modifying the list simultaneously. This is a pre-existing issue not introduced by this PR, but the new SaveTrackedPIDs method adds another concurrent access point.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR makes several changes to the RO Server Monitor application:
Orphan process cleanup: Adds a mechanism to persist spawned process PIDs to user settings (TrackedPIDs) and kill them on next startup if they were left running (e.g., after a crash).
Rename rAthena → ROServers: Renames the constants class and all references across the codebase, along with method/parameter renames (SaverAthenaFilePath → SaveServerFilePath, rAthenaProcess → ROProcess).
Project config: Downgrades target framework from 4.8.1 → 4.8, switches platform from AnyCPU → x64, adds ClickOnce publish settings, and bumps VS version to 18.
Build command tweaks: Changes msbuild -t:build to -t:rebuild and removes the -T flag from npm run build.
Version bump: 1.0.5 → 1.0.6 with branding update to "RO Server Monitor".