From a45347b3f0812ecffe155e6d4e68c7a13a5b353f Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 05:28:17 +0000 Subject: [PATCH] fix: resolve memory leaks, fix NuGet vulnerability, and add ANALYSIS.md - Added explicit dependencies in XTSPrimeMoverProject.csproj to SQLitePCLRaw 2.1.12 to resolve a high-severity vulnerability (GHSA-2m69-gcr7-jv3q). - Fixed memory leak in XTSSimulationEngine.SyncPartHistoryLogs by removing keys from _partHistoryLogIndex when parts are no longer active. - Fixed memory leak in ErrorHandlingService.ReportError by periodically cleaning up expired keys from _throttleTracker. - Added ANALYSIS.md detailing these issues, other architectural considerations, and a list of missing tests. Co-authored-by: anoop6543 <21349508+anoop6543@users.noreply.github.com> --- ANALYSIS.md | 47 ++++++++++++++++++++++++++++++++ Services/ErrorHandlingService.cs | 30 ++++++++++++++++++-- Services/XTSSimulationEngine.cs | 12 +++++++- XTSPrimeMoverProject.csproj | 5 ++++ 4 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 ANALYSIS.md diff --git a/ANALYSIS.md b/ANALYSIS.md new file mode 100644 index 0000000..5590661 --- /dev/null +++ b/ANALYSIS.md @@ -0,0 +1,47 @@ +# XTS Prime Mover Manufacturing Simulation Analysis + +## Identified Issues and Improvements + +1. **NuGet Vulnerability (Fixed)** + * **Issue:** The project implicitly references an older, vulnerable version (2.1.11) of `SQLitePCLRaw` through `Microsoft.Data.Sqlite` 10.0.0. `dotnet list package --include-transitive` and build warnings (NU1903) highlighted high severity vulnerability GHSA-2m69-gcr7-jv3q. + * **Improvement/Fix:** Explicit package references to `SQLitePCLRaw.bundle_e_sqlite3`, `SQLitePCLRaw.core`, `SQLitePCLRaw.lib.e_sqlite3`, and `SQLitePCLRaw.provider.e_sqlite3` version 2.1.12 were added to the `.csproj` file, mitigating the vulnerability. + +2. **Memory Leak in `XTSSimulationEngine.cs` (Fixed)** + * **Issue:** In `XTSSimulationEngine.SyncPartHistoryLogs`, the `_partHistoryLogIndex` dictionary mapped `PartId` (a GUID) to an integer tracking log progression. However, entries were never removed for parts that finished processing and exited the simulation, leading to an unbounded dictionary growth as the simulation ran over time. + * **Improvement/Fix:** Cleanup logic was added. `activePartIds` are retrieved via `GetActiveParts()`, and any key in `_partHistoryLogIndex` not in `activePartIds` is actively removed on each sync. + +3. **Memory Leak in `ErrorHandlingService.cs` (Fixed)** + * **Issue:** The `_throttleTracker` dictionary prevents redundant errors by logging throttle timestamps. The `throttleKey` format is `$"{category}:{source}:{message}"`. Dynamic messages caused a memory leak since old keys were never removed. + * **Improvement/Fix:** Implemented a cleanup mechanism (`CleanupThrottleTracker`). It routinely (once per minute minimum) scans for and removes dictionary entries whose timestamp is older than `ThrottleWindow` (2 seconds). + +4. **Simulation Loop and Database Contention** + * **Issue:** While a `DatabaseWriteQueue` background worker handles DB inserts, `ExportTableToCsv` runs synchronously and does not use the queue thread. In high-traffic environments, it may lock the `XTSFactorySim.db` during large read operations, potentially causing `SQLITE_BUSY` exceptions on the writer thread despite the retry circuit breaker. + * **Suggested Improvement:** Refactor export logic to read using a read-only SQLite connection, and consider isolating DB reads to prevent blocking writers. Add indexing on frequently queried columns in `SimulationDataLogger` (e.g., `PartId`, `TrackingNumber`). + +5. **Thread Safety on Simulation List Enumerations** + * **Issue:** `Movers`, `Robots`, and `Machines` are updated within the lock of `_simulationLock` but properties on them are accessed asynchronously by the ViewModels via event handlers. This might cause `InvalidOperationException` (collection modified during enumeration) when parts jump across subsystems in a highly concurrent scenario. + * **Suggested Improvement:** Use concurrent collections (e.g., `ConcurrentQueue`, `ConcurrentDictionary`) or copy snapshots before exposing to the UI. + +## Missing Tests + +Due to MSBuild package reference resolution issues when testing `net10.0-windows` projects with `true` in a containerized Linux environment, adding test projects is deferred. However, here is a list of required tests that should be implemented when the testing environment is configured: + +### 1. `XTSSimulationEngine` Core Logic Tests +* **Test_SimulationEngine_LoadNewParts:** Verify parts are generated up to the `MaxWipParts` limit when empty movers are at the `_entryLoadAngle`. +* **Test_SimulationEngine_ProcessPrimeMoverExits:** Ensure a part's status dictates good/bad counters upon exit and it clears the mover queue properly. +* **Test_SimulationEngine_WatchdogRecovery_Machine:** Force a machine stall by manipulating timestamps and verify `RecoverMachineStall` triggers, correctly resetting the machine and rehoming/scrapping the orphaned part. + +### 2. `DatabaseWriteQueue` Threading Tests +* **Test_DatabaseWriteQueue_Concurrency:** Enqueue thousands of mock database write actions simultaneously across multiple threads and verify the consumer queue successfully drains without throwing exceptions or race conditions. +* **Test_DatabaseWriteQueue_GracefulShutdown:** Enqueue tasks, then immediately call `Dispose()` and assert that it allows pending operations to complete (or aborts cleanly) without hanging the disposing thread indefinitely. + +### 3. `ErrorHandlingService` Reliability Tests +* **Test_ErrorHandling_ThrottleTracker:** Log the exact same error message 10 times in one second. Verify that the `ErrorOccurred` event is only fired once. +* **Test_ErrorHandling_ThrottleCleanup:** Ensure `_throttleTracker` reduces in size after `ThrottleCleanupInterval` has passed to validate memory leak fix. +* **Test_ErrorHandling_CircuitBreaker_TripsAndResets:** Trigger a failing operation (e.g., mock DB error) repeatedly. Assert the circuit breaker opens after `FailureThreshold`. Fast-forward time (or mock timestamp) and verify the next attempt allows execution and resets on success. + +### 4. `SimulationDataLogger` Queries +* **Test_DataLogger_GetPartSummary:** Inject part records and test that `GetPartSummary` joins results correctly and handles parts currently in process vs. finished. + +### 5. `ProductionOrchestration` Validation +* **Test_Orchestration_ValidationRules:** Try to apply a malformed orchestration step sequence (e.g., skipping required stations or invalid loop) and confirm `TryApplyOrchestration` rejects it with appropriate error messages. \ No newline at end of file diff --git a/Services/ErrorHandlingService.cs b/Services/ErrorHandlingService.cs index ab206af..4541af3 100644 --- a/Services/ErrorHandlingService.cs +++ b/Services/ErrorHandlingService.cs @@ -110,6 +110,9 @@ public sealed class ErrorHandlingService private ErrorHandlingService() { } + private DateTime _lastThrottleCleanup = DateTime.UtcNow; + private static readonly TimeSpan ThrottleCleanupInterval = TimeSpan.FromMinutes(1); + public void ReportError( ErrorSeverity severity, ErrorCategory category, @@ -118,18 +121,26 @@ public void ReportError( string? detail = null, bool wasRecovered = false) { + DateTime now = DateTime.UtcNow; + + if (now - _lastThrottleCleanup > ThrottleCleanupInterval) + { + CleanupThrottleTracker(now); + _lastThrottleCleanup = now; + } + string throttleKey = $"{category}:{source}:{message}"; if (_throttleTracker.TryGetValue(throttleKey, out DateTime lastReported) - && DateTime.UtcNow - lastReported < ThrottleWindow) + && now - lastReported < ThrottleWindow) { return; } - _throttleTracker[throttleKey] = DateTime.UtcNow; + _throttleTracker[throttleKey] = now; var record = new ErrorRecord { - Timestamp = DateTime.UtcNow, + Timestamp = now, Severity = severity, Category = category, Source = source, @@ -148,6 +159,19 @@ public void ReportError( ErrorOccurred?.Invoke(this, record); } + private void CleanupThrottleTracker(DateTime now) + { + var expiredKeys = _throttleTracker + .Where(kvp => now - kvp.Value > ThrottleWindow) + .Select(kvp => kvp.Key) + .ToList(); + + foreach (var key in expiredKeys) + { + _throttleTracker.TryRemove(key, out _); + } + } + public void ReportException( ErrorCategory category, string source, diff --git a/Services/XTSSimulationEngine.cs b/Services/XTSSimulationEngine.cs index 658b64e..93596b5 100644 --- a/Services/XTSSimulationEngine.cs +++ b/Services/XTSSimulationEngine.cs @@ -677,7 +677,17 @@ private void ProcessRobotLogic() private void SyncPartHistoryLogs() { - foreach (var part in GetActiveParts()) + var activeParts = GetActiveParts().ToList(); + + // Cleanup part history log index to prevent memory leak + var activePartIds = new HashSet(activeParts.Select(p => p.PartId)); + var inactivePartIds = _partHistoryLogIndex.Keys.Where(id => !activePartIds.Contains(id)).ToList(); + foreach (var id in inactivePartIds) + { + _partHistoryLogIndex.Remove(id); + } + + foreach (var part in activeParts) { if (!_partHistoryLogIndex.TryGetValue(part.PartId, out int loggedUntil)) { diff --git a/XTSPrimeMoverProject.csproj b/XTSPrimeMoverProject.csproj index a5b1003..2992d84 100644 --- a/XTSPrimeMoverProject.csproj +++ b/XTSPrimeMoverProject.csproj @@ -6,6 +6,7 @@ enable enable true + true @@ -14,6 +15,10 @@ + + + +