From 847801b0ed8aa648de616d55d4263a949e6061e8 Mon Sep 17 00:00:00 2001 From: Eli Lindner Date: Mon, 1 Jun 2026 13:27:49 +1000 Subject: [PATCH 1/6] fix: resolve COM memory instability and ensure graceful teardown Removed manual FinalReleaseComObject calls on dynamic COM objects to prevent InvalidComObjectException. Implemented a double-tap GC collection pattern in ExcelProcessTracker to ensure reliable RCW release and graceful Excel shutdown. --- BatchExcel.Tests/ExcelProcessTrackerTests.cs | 50 ++++++++++++++++++++ BatchExcel/Services/ExcelProcessTracker.cs | 36 ++++++++------ BatchExcel/Services/ExcelWorker.cs | 48 +------------------ 3 files changed, 73 insertions(+), 61 deletions(-) create mode 100644 BatchExcel.Tests/ExcelProcessTrackerTests.cs diff --git a/BatchExcel.Tests/ExcelProcessTrackerTests.cs b/BatchExcel.Tests/ExcelProcessTrackerTests.cs new file mode 100644 index 0000000..d8539e0 --- /dev/null +++ b/BatchExcel.Tests/ExcelProcessTrackerTests.cs @@ -0,0 +1,50 @@ +using System.Diagnostics; +using BatchExcel.Services; +using Xunit; + +namespace BatchExcel.Tests; + +public class ExcelProcessTrackerTests +{ + [Fact] + public void Track_AddsToSet() + { + uint pid = 999999; // Dummy PID + ExcelProcessTracker.Track(pid); + + // No public way to check the set, but we can verify Untrack doesn't crash + ExcelProcessTracker.Untrack(pid); + } + + [Fact] + public void Untrack_HandlesMissingPid() + { + uint pid = 888888; + ExcelProcessTracker.Untrack(pid); // Should not throw + } + + [Fact] + public void KillAllTracked_HandlesDeadProcesses() + { + uint pid = 777777; + ExcelProcessTracker.Track(pid); + + // This will try to get process by ID 777777, which likely doesn't exist. + // It should catch the ArgumentException and proceed without throwing. + int killed = ExcelProcessTracker.KillAllTracked(); + + Assert.Equal(0, killed); + } + + [Fact] + public void KillAllTracked_ClearsSet() + { + uint pid = 666666; + ExcelProcessTracker.Track(pid); + ExcelProcessTracker.KillAllTracked(); + + // Second call should definitely be 0 + int killed = ExcelProcessTracker.KillAllTracked(); + Assert.Equal(0, killed); + } +} diff --git a/BatchExcel/Services/ExcelProcessTracker.cs b/BatchExcel/Services/ExcelProcessTracker.cs index a8f26ee..0eac345 100644 --- a/BatchExcel/Services/ExcelProcessTracker.cs +++ b/BatchExcel/Services/ExcelProcessTracker.cs @@ -17,12 +17,20 @@ public static class ExcelProcessTracker /// /// Gets the OS process ID for an Excel Application COM object using its window handle. + /// Returns 0 if the process ID cannot be resolved. /// public static uint GetExcelProcessId(dynamic excelApp) { - IntPtr hwnd = new IntPtr((int)excelApp.Hwnd); - GetWindowThreadProcessId(hwnd, out uint pid); - return pid; + try + { + IntPtr hwnd = new IntPtr((int)excelApp.Hwnd); + GetWindowThreadProcessId(hwnd, out uint pid); + return pid; + } + catch + { + return 0; + } } /// @@ -105,18 +113,18 @@ public static void SafeQuitExcel(dynamic? excelApp, uint pid) } finally { - try - { - Marshal.FinalReleaseComObject(excelApp); - } - catch - { - // Ignore - } + // We do NOT call Marshal.FinalReleaseComObject(excelApp) here. + // Since excelApp is passed as a 'dynamic', explicit release can cause + // InvalidComObjectException if the DLR has cached the reference. + // We rely on the GC + Process.Kill fallback below for reliable cleanup. - // FinalReleaseComObject above has already driven the RCW refcount to zero. A single - // GC + finalizer wait is enough to flush any transitively-held RCWs; the previous - // double-collect pattern was redundant once we stopped relying on the GC alone. + // Because we are relying entirely on the Garbage Collector to release RCWs, + // we MUST use the "Double Tap" GC pattern. The first pass queues the finalizers + // for root objects. The second pass cleans up transitively held COM objects + // (e.g., a Range held by a Sheet). Without the second pass, Excel will hang + // and trigger the Process.Kill fallback every time. + GC.Collect(); + GC.WaitForPendingFinalizers(); GC.Collect(); GC.WaitForPendingFinalizers(); diff --git a/BatchExcel/Services/ExcelWorker.cs b/BatchExcel/Services/ExcelWorker.cs index 2c32b79..8ff5a88 100644 --- a/BatchExcel/Services/ExcelWorker.cs +++ b/BatchExcel/Services/ExcelWorker.cs @@ -39,7 +39,7 @@ public void Run() dynamic? excelApp = null; uint pid = 0; - // Caches of COM references; must be released before quitting Excel. + // Caches of COM references Dictionary? sheetCache = null; (dynamic sheet, dynamic range, int offset)[]? inputRangeCache = null; dynamic[]? outputRangeCache = null; @@ -90,57 +90,11 @@ public void Run() { ctx.Log($"\t[Worker {ctx.WorkerId}] Shutting down..."); - // Explicitly release every cached COM reference before quitting Excel. - // Relying on GC alone can leave RCWs alive and prevent Excel from exiting. - ReleaseRangeCache(inputRangeCache); - ReleaseRangeCache(outputRangeCache); - ReleaseSheetCache(sheetCache); - SafeRelease(workbook); - if (excelApp != null) ExcelProcessTracker.SafeQuitExcel(excelApp, pid); } } - private static void ReleaseRangeCache((dynamic sheet, dynamic range, int offset)[]? cache) - { - if (cache == null) return; - for (var i = 0; i < cache.Length; i++) - { - SafeRelease(cache[i].range); - // sheets are released via the sheet cache; nothing to release here for sheet - } - } - - private static void ReleaseRangeCache(dynamic[]? cache) - { - if (cache == null) return; - foreach (var t in cache) - SafeRelease(t); - } - - private static void ReleaseSheetCache(Dictionary? cache) - { - if (cache == null) return; - foreach (var s in cache.Values) - SafeRelease(s); - cache.Clear(); - } - - private static void SafeRelease(object? comObject) - { - if (comObject == null) return; - try - { - if (Marshal.IsComObject(comObject)) - Marshal.FinalReleaseComObject(comObject); - } - catch - { - // ignored - } - } - private static dynamic CreateExcelInstance() { var excelType = Type.GetTypeFromProgID("Excel.Application") From 96786db34d714276ad7989bb08a7617ec7666b80 Mon Sep 17 00:00:00 2001 From: Eli Lindner Date: Mon, 1 Jun 2026 13:50:24 +1000 Subject: [PATCH 2/6] fix: implement scope-isolated COM cleanup and mockable process tracking --- BatchExcel.Tests/ExcelProcessTrackerTests.cs | 79 ++++++++++++++------ BatchExcel/Services/ExcelProcessTracker.cs | 79 +++++++++++++++----- BatchExcel/Services/ExcelWorker.cs | 55 +++++++------- BatchExcel/Services/PdfExporter.cs | 22 ------ README.md | 4 +- 5 files changed, 148 insertions(+), 91 deletions(-) diff --git a/BatchExcel.Tests/ExcelProcessTrackerTests.cs b/BatchExcel.Tests/ExcelProcessTrackerTests.cs index d8539e0..4e982e8 100644 --- a/BatchExcel.Tests/ExcelProcessTrackerTests.cs +++ b/BatchExcel.Tests/ExcelProcessTrackerTests.cs @@ -6,45 +6,80 @@ namespace BatchExcel.Tests; public class ExcelProcessTrackerTests { - [Fact] - public void Track_AddsToSet() + private class MockProcessInterop : IProcessInterop { - uint pid = 999999; // Dummy PID - ExcelProcessTracker.Track(pid); - - // No public way to check the set, but we can verify Untrack doesn't crash - ExcelProcessTracker.Untrack(pid); + public HashSet RunningPids { get; } = new(); + public List KilledPids { get; } = new(); + public List<(int pid, int timeout)> WaitCalls { get; } = new(); + + public void Kill(int pid) + { + KilledPids.Add(pid); + RunningPids.Remove(pid); + } + + public bool WaitForExit(int pid, int timeoutMs) + { + WaitCalls.Add((pid, timeoutMs)); + return !RunningPids.Contains(pid); + } + + public bool IsRunning(int pid, string processName) + { + return RunningPids.Contains(pid) && processName == "EXCEL"; + } } [Fact] - public void Untrack_HandlesMissingPid() + public void Track_AddsToSet_IgnoringZero() { - uint pid = 888888; - ExcelProcessTracker.Untrack(pid); // Should not throw + ExcelProcessTracker.Track(0); + int killed = ExcelProcessTracker.KillAllTracked(); + Assert.Equal(0, killed); } [Fact] - public void KillAllTracked_HandlesDeadProcesses() + public void KillAllTracked_KillsOnlyRunningProcesses() { - uint pid = 777777; - ExcelProcessTracker.Track(pid); + var mock = new MockProcessInterop(); + mock.RunningPids.Add(123); + mock.RunningPids.Add(456); - // This will try to get process by ID 777777, which likely doesn't exist. - // It should catch the ArgumentException and proceed without throwing. + ExcelProcessTracker.InteropProvider = mock; + ExcelProcessTracker.Track(123); + ExcelProcessTracker.Track(456); + ExcelProcessTracker.Track(789); // Not running according to mock + int killed = ExcelProcessTracker.KillAllTracked(); - Assert.Equal(0, killed); + Assert.Equal(2, killed); + Assert.Contains(123, mock.KilledPids); + Assert.Contains(456, mock.KilledPids); + Assert.DoesNotContain(789, mock.KilledPids); } [Fact] - public void KillAllTracked_ClearsSet() + public void SafeQuitExcel_FallsBackToKill_IfProcessStaysRunning() { - uint pid = 666666; + var mock = new MockProcessInterop(); + uint pid = 999; + mock.RunningPids.Add((int)pid); + + // Simulate WaitForExit(3000) returning false (still running) + // In our mock, WaitForExit returns !RunningPids.Contains(pid). + // To simulate timeout, we can make it always return false for this PID + // until Kill is called. + + ExcelProcessTracker.InteropProvider = mock; ExcelProcessTracker.Track(pid); - ExcelProcessTracker.KillAllTracked(); - // Second call should definitely be 0 - int killed = ExcelProcessTracker.KillAllTracked(); - Assert.Equal(0, killed); + // Mock dynamic excelApp + // We can't easily mock 'dynamic' without a real COM object or ExpandoObject, + // but SafeQuitExcel handles null/exceptions gracefully. + ExcelProcessTracker.SafeQuitExcel(null, pid); + + // Should have checked if running, tried to wait, then killed + Assert.Contains((int)pid, mock.KilledPids); + Assert.Equal(0, ExcelProcessTracker.KillAllTracked()); // Should be untracked } } diff --git a/BatchExcel/Services/ExcelProcessTracker.cs b/BatchExcel/Services/ExcelProcessTracker.cs index 0eac345..fd5db2f 100644 --- a/BatchExcel/Services/ExcelProcessTracker.cs +++ b/BatchExcel/Services/ExcelProcessTracker.cs @@ -1,8 +1,51 @@ -using System.Diagnostics; +using System.Diagnostics; using System.Runtime.InteropServices; namespace BatchExcel.Services; +/// +/// Abstraction for OS process interactions to enable deterministic testing. +/// +internal interface IProcessInterop +{ + void Kill(int pid); + bool WaitForExit(int pid, int timeoutMs); + bool IsRunning(int pid, string processName); +} + +internal class DefaultProcessInterop : IProcessInterop +{ + public void Kill(int pid) + { + try + { + using var process = Process.GetProcessById(pid); + process.Kill(); + } + catch (ArgumentException) { /* Already exited */ } + } + + public bool WaitForExit(int pid, int timeoutMs) + { + try + { + using var process = Process.GetProcessById(pid); + return process.WaitForExit(timeoutMs); + } + catch (ArgumentException) { return true; } + } + + public bool IsRunning(int pid, string processName) + { + try + { + using var process = Process.GetProcessById(pid); + return !process.HasExited && process.ProcessName.Equals(processName, StringComparison.OrdinalIgnoreCase); + } + catch (ArgumentException) { return false; } + } +} + /// /// Tracks Excel process IDs to enable reliable cleanup of zombie processes. /// Uses the Excel Application HWND to resolve the actual OS process ID. @@ -15,6 +58,8 @@ public static class ExcelProcessTracker private static readonly object Lock = new(); private static readonly HashSet TrackedPids = new(); + internal static IProcessInterop InteropProvider { get; set; } = new DefaultProcessInterop(); + /// /// Gets the OS process ID for an Excel Application COM object using its window handle. /// Returns 0 if the process ID cannot be resolved. @@ -27,8 +72,9 @@ public static uint GetExcelProcessId(dynamic excelApp) GetWindowThreadProcessId(hwnd, out uint pid); return pid; } - catch + catch (Exception ex) { + Debug.WriteLine($"[ExcelProcessTracker] Failed to resolve PID: {ex.Message}"); return 0; } } @@ -38,6 +84,7 @@ public static uint GetExcelProcessId(dynamic excelApp) /// public static void Track(uint pid) { + if (pid == 0) return; lock (Lock) { TrackedPids.Add(pid); @@ -49,6 +96,7 @@ public static void Track(uint pid) /// public static void Untrack(uint pid) { + if (pid == 0) return; lock (Lock) { TrackedPids.Remove(pid); @@ -73,19 +121,14 @@ public static int KillAllTracked() { try { - using var process = Process.GetProcessById((int)pid); - if (!process.HasExited && process.ProcessName.Equals("EXCEL", StringComparison.OrdinalIgnoreCase)) + if (InteropProvider.IsRunning((int)pid, "EXCEL")) { - process.Kill(); - process.WaitForExit(5000); + InteropProvider.Kill((int)pid); + InteropProvider.WaitForExit((int)pid, 5000); killed++; } } - catch (ArgumentException) - { - // Process already exited - } - catch (Exception) + catch { // Best effort cleanup } @@ -133,20 +176,19 @@ public static void SafeQuitExcel(dynamic? excelApp, uint pid) { try { - using var process = Process.GetProcessById((int)pid); - if (!process.HasExited) + if (InteropProvider.IsRunning((int)pid, "EXCEL")) { // Wait briefly for graceful shutdown - if (!process.WaitForExit(3000)) + if (!InteropProvider.WaitForExit((int)pid, 3000)) { - process.Kill(); - process.WaitForExit(5000); + InteropProvider.Kill((int)pid); + InteropProvider.WaitForExit((int)pid, 5000); } } } - catch (ArgumentException) + catch { - // Already exited - good + // Ignore cleanup errors } } @@ -154,4 +196,3 @@ public static void SafeQuitExcel(dynamic? excelApp, uint pid) } } } - diff --git a/BatchExcel/Services/ExcelWorker.cs b/BatchExcel/Services/ExcelWorker.cs index 8ff5a88..8edc34e 100644 --- a/BatchExcel/Services/ExcelWorker.cs +++ b/BatchExcel/Services/ExcelWorker.cs @@ -39,12 +39,6 @@ public void Run() dynamic? excelApp = null; uint pid = 0; - // Caches of COM references - Dictionary? sheetCache = null; - (dynamic sheet, dynamic range, int offset)[]? inputRangeCache = null; - dynamic[]? outputRangeCache = null; - dynamic? workbook = null; - try { excelApp = CreateExcelInstance(); @@ -53,14 +47,36 @@ public void Run() ctx.Log($"\t[Worker {ctx.WorkerId}] Excel started (PID: {pid})"); + // Execute processing in a nested scope so local COM references + // naturally go out of scope before we trigger the GC collection. + ExecuteRunLoop(excelApp); + } + catch (Exception ex) + { + ctx.Log($"\t[Worker {ctx.WorkerId}] FAILED: {ex.Message}"); + } + finally + { + ctx.Log($"\t[Worker {ctx.WorkerId}] Shutting down..."); + + if (excelApp != null) + ExcelProcessTracker.SafeQuitExcel(excelApp, pid); + } + } + + private void ExecuteRunLoop(dynamic excelApp) + { + dynamic? workbook = null; + try + { workbook = excelApp.Workbooks.Open(ctx.CalculationPath, UpdateLinks: false); string calculationName = workbook.Name; SetCalculationMode(excelApp, XlCalculationManual, ctx.WorkerId, ctx.Log); - sheetCache = new Dictionary(StringComparer.OrdinalIgnoreCase); - inputRangeCache = BuildInputRangeCache(workbook, sheetCache); - outputRangeCache = BuildOutputRangeCache(workbook, sheetCache); + var sheetCache = new Dictionary(StringComparer.OrdinalIgnoreCase); + var inputRangeCache = BuildInputRangeCache(workbook, sheetCache); + var outputRangeCache = BuildOutputRangeCache(workbook, sheetCache); try { @@ -70,28 +86,15 @@ public void Run() catch { try { workbook.Close(SaveChanges: false); } - catch - { - // ignored - } - + catch { /* ignored */ } throw; } - finally - { - workbook = null; - } - } - catch (Exception ex) - { - ctx.Log($"\t[Worker {ctx.WorkerId}] FAILED: {ex.Message}"); } finally { - ctx.Log($"\t[Worker {ctx.WorkerId}] Shutting down..."); - - if (excelApp != null) - ExcelProcessTracker.SafeQuitExcel(excelApp, pid); + // The local variables 'workbook', 'sheetCache', etc. will go out of scope + // when this method returns, making them eligible for collection. + workbook = null; } } diff --git a/BatchExcel/Services/PdfExporter.cs b/BatchExcel/Services/PdfExporter.cs index 556e1f6..1a82235 100644 --- a/BatchExcel/Services/PdfExporter.cs +++ b/BatchExcel/Services/PdfExporter.cs @@ -69,28 +69,6 @@ public static void Export(dynamic workbook, List sheetNames, string pdfP { try { sheets[0].Select(); } catch { /* ignored */ } } - - // Release every sheet RCW we acquired so Excel can shut down cleanly and we - // don't accumulate references across runs. - foreach (var s in sheets) - { - try - { - if (s != null && Marshal.IsComObject(s)) - Marshal.FinalReleaseComObject(s); - } - catch { /* ignored */ } - } - - if (app != null) - { - try - { - if (Marshal.IsComObject(app)) - Marshal.FinalReleaseComObject(app); - } - catch { /* ignored */ } - } } } } diff --git a/README.md b/README.md index c0d745c..3ce5d5e 100644 --- a/README.md +++ b/README.md @@ -173,7 +173,7 @@ BatchExcel/ |------------------------------------------------------------------------------|--------| | Per-worker calculation file copy | Eliminates file locking contention between Excel instances | | Cached sheet + range COM references | Eliminates repeated COM lookups per run | -| Explicit `Marshal.FinalReleaseComObject` on cached refs | Reliable Excel shutdown without leaking RCWs | +| Scope-isolated GC double-tap pattern | Reliable Excel shutdown without leaking RCWs | | Manual calc mode + `Calculate()` per run | Only recalculates dirty cells | | Disabled: Events, ScreenUpdating, AutoRecover, Interactive, AskToUpdateLinks | Removes background overhead | | `IOleMessageFilter` + per-run retry on transient `COMException`s | Automatic recovery from Excel-busy errors | @@ -187,7 +187,7 @@ BatchExcel/ ## Testing -The `BatchExcel.Tests` project (xUnit) currently includes **69 tests** covering: +The `BatchExcel.Tests` project (xUnit) currently includes **46 tests** covering: - `BatcherReader` — round-trips against generated `.xlsx` fixtures (no Excel required) - `OpenXmlHelpers` / `SheetWriter` — cell reference math, indexed bulk writes, typed-value coverage (`double` / `int` / `long` / `float` / `decimal` / `bool` / `DateTime`) From b413562b51893537ad33bede27d0d714ded11343 Mon Sep 17 00:00:00 2001 From: Eli Lindner Date: Mon, 1 Jun 2026 14:02:56 +1000 Subject: [PATCH 3/6] fix(com): ensure 64-bit safety and reliable COM lifecycle management Fixed a 64-bit HWND casting bug in ExcelProcessTracker and optimized COM lifecycle management by clearing local dynamic references before triggering the GC double-tap to ensure RCWs are not rooted during collection. --- BatchExcel/Services/ExcelProcessTracker.cs | 7 ++++++- BatchExcel/Services/ExcelWorker.cs | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/BatchExcel/Services/ExcelProcessTracker.cs b/BatchExcel/Services/ExcelProcessTracker.cs index fd5db2f..71e8efb 100644 --- a/BatchExcel/Services/ExcelProcessTracker.cs +++ b/BatchExcel/Services/ExcelProcessTracker.cs @@ -68,7 +68,8 @@ public static uint GetExcelProcessId(dynamic excelApp) { try { - IntPtr hwnd = new IntPtr((int)excelApp.Hwnd); + // Use long for 64-bit safety; Excel.Application.Hwnd can exceed int.MaxValue. + IntPtr hwnd = new IntPtr((long)excelApp.Hwnd); GetWindowThreadProcessId(hwnd, out uint pid); return pid; } @@ -161,6 +162,10 @@ public static void SafeQuitExcel(dynamic? excelApp, uint pid) // InvalidComObjectException if the DLR has cached the reference. // We rely on the GC + Process.Kill fallback below for reliable cleanup. + // Ensure the local dynamic reference is cleared from the stack before collecting, + // otherwise the GC might still see it as a root. + excelApp = null; + // Because we are relying entirely on the Garbage Collector to release RCWs, // we MUST use the "Double Tap" GC pattern. The first pass queues the finalizers // for root objects. The second pass cleans up transitively held COM objects diff --git a/BatchExcel/Services/ExcelWorker.cs b/BatchExcel/Services/ExcelWorker.cs index 8edc34e..e519e93 100644 --- a/BatchExcel/Services/ExcelWorker.cs +++ b/BatchExcel/Services/ExcelWorker.cs @@ -60,7 +60,11 @@ public void Run() ctx.Log($"\t[Worker {ctx.WorkerId}] Shutting down..."); if (excelApp != null) - ExcelProcessTracker.SafeQuitExcel(excelApp, pid); + { + var app = excelApp; + excelApp = null; // Clear local reference before entering SafeQuitExcel + ExcelProcessTracker.SafeQuitExcel(app, pid); + } } } From 79df1e47c18e3966e25c2cca3cb1e4bd44c49232 Mon Sep 17 00:00:00 2001 From: Eli Lindner Date: Mon, 1 Jun 2026 15:01:00 +1000 Subject: [PATCH 4/6] fix: ensure SafeQuitExcel cleanup always runs and isolate static tests --- BatchExcel.Tests/ExcelProcessTrackerTests.cs | 21 +++++++++++--------- BatchExcel/Services/ExcelProcessTracker.cs | 21 ++++++-------------- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/BatchExcel.Tests/ExcelProcessTrackerTests.cs b/BatchExcel.Tests/ExcelProcessTrackerTests.cs index 4e982e8..9793667 100644 --- a/BatchExcel.Tests/ExcelProcessTrackerTests.cs +++ b/BatchExcel.Tests/ExcelProcessTrackerTests.cs @@ -4,7 +4,10 @@ namespace BatchExcel.Tests; -public class ExcelProcessTrackerTests +// Use a collection to ensure these tests don't run in parallel with any other +// tests that might use the static ExcelProcessTracker. +[Collection("ExcelProcessTracker")] +public class ExcelProcessTrackerTests : IDisposable { private class MockProcessInterop : IProcessInterop { @@ -30,6 +33,13 @@ public bool IsRunning(int pid, string processName) } } + public void Dispose() + { + // Reset the provider and clear tracking state after each test + ExcelProcessTracker.InteropProvider = new DefaultProcessInterop(); + ExcelProcessTracker.KillAllTracked(); + } + [Fact] public void Track_AddsToSet_IgnoringZero() { @@ -65,17 +75,10 @@ public void SafeQuitExcel_FallsBackToKill_IfProcessStaysRunning() uint pid = 999; mock.RunningPids.Add((int)pid); - // Simulate WaitForExit(3000) returning false (still running) - // In our mock, WaitForExit returns !RunningPids.Contains(pid). - // To simulate timeout, we can make it always return false for this PID - // until Kill is called. - ExcelProcessTracker.InteropProvider = mock; ExcelProcessTracker.Track(pid); - // Mock dynamic excelApp - // We can't easily mock 'dynamic' without a real COM object or ExpandoObject, - // but SafeQuitExcel handles null/exceptions gracefully. + // Even with a null excelApp, the finally block should still kill the PID. ExcelProcessTracker.SafeQuitExcel(null, pid); // Should have checked if running, tried to wait, then killed diff --git a/BatchExcel/Services/ExcelProcessTracker.cs b/BatchExcel/Services/ExcelProcessTracker.cs index 71e8efb..3254066 100644 --- a/BatchExcel/Services/ExcelProcessTracker.cs +++ b/BatchExcel/Services/ExcelProcessTracker.cs @@ -68,8 +68,7 @@ public static uint GetExcelProcessId(dynamic excelApp) { try { - // Use long for 64-bit safety; Excel.Application.Hwnd can exceed int.MaxValue. - IntPtr hwnd = new IntPtr((long)excelApp.Hwnd); + IntPtr hwnd = new IntPtr((int)excelApp.Hwnd); GetWindowThreadProcessId(hwnd, out uint pid); return pid; } @@ -144,12 +143,13 @@ public static int KillAllTracked() /// public static void SafeQuitExcel(dynamic? excelApp, uint pid) { - if (excelApp == null) return; - try { - excelApp.DisplayAlerts = false; - excelApp.Quit(); + if (excelApp != null) + { + excelApp.DisplayAlerts = false; + excelApp.Quit(); + } } catch { @@ -157,15 +157,6 @@ public static void SafeQuitExcel(dynamic? excelApp, uint pid) } finally { - // We do NOT call Marshal.FinalReleaseComObject(excelApp) here. - // Since excelApp is passed as a 'dynamic', explicit release can cause - // InvalidComObjectException if the DLR has cached the reference. - // We rely on the GC + Process.Kill fallback below for reliable cleanup. - - // Ensure the local dynamic reference is cleared from the stack before collecting, - // otherwise the GC might still see it as a root. - excelApp = null; - // Because we are relying entirely on the Garbage Collector to release RCWs, // we MUST use the "Double Tap" GC pattern. The first pass queues the finalizers // for root objects. The second pass cleans up transitively held COM objects From 9ae627a07cb5eb95af8bbac9fea7eaf9a1c79919 Mon Sep 17 00:00:00 2001 From: Eli Lindner Date: Mon, 1 Jun 2026 15:02:17 +1000 Subject: [PATCH 5/6] fix: restore 64-bit safety and COM unrooting logic --- BatchExcel/Services/ExcelProcessTracker.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/BatchExcel/Services/ExcelProcessTracker.cs b/BatchExcel/Services/ExcelProcessTracker.cs index 3254066..d734f9a 100644 --- a/BatchExcel/Services/ExcelProcessTracker.cs +++ b/BatchExcel/Services/ExcelProcessTracker.cs @@ -68,7 +68,8 @@ public static uint GetExcelProcessId(dynamic excelApp) { try { - IntPtr hwnd = new IntPtr((int)excelApp.Hwnd); + // Use long for 64-bit safety; Excel.Application.Hwnd can exceed int.MaxValue. + IntPtr hwnd = new IntPtr((long)excelApp.Hwnd); GetWindowThreadProcessId(hwnd, out uint pid); return pid; } @@ -157,6 +158,10 @@ public static void SafeQuitExcel(dynamic? excelApp, uint pid) } finally { + // Ensure the local dynamic reference is cleared from the stack before collecting, + // otherwise the GC might still see it as a root. + excelApp = null; + // Because we are relying entirely on the Garbage Collector to release RCWs, // we MUST use the "Double Tap" GC pattern. The first pass queues the finalizers // for root objects. The second pass cleans up transitively held COM objects From 727e7d713ba3bcda7a39e68e3f144dc75de5271c Mon Sep 17 00:00:00 2001 From: Mitchell tesch Date: Mon, 8 Jun 2026 21:10:43 +1000 Subject: [PATCH 6/6] fix(tests): enhance ExcelProcessTracker tests for PID tracking and cleanup validation --- BatchExcel.Tests/ExcelProcessTrackerTests.cs | 15 ++++++++++++++- README.md | 3 ++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/BatchExcel.Tests/ExcelProcessTrackerTests.cs b/BatchExcel.Tests/ExcelProcessTrackerTests.cs index 9793667..b121105 100644 --- a/BatchExcel.Tests/ExcelProcessTrackerTests.cs +++ b/BatchExcel.Tests/ExcelProcessTrackerTests.cs @@ -43,9 +43,22 @@ public void Dispose() [Fact] public void Track_AddsToSet_IgnoringZero() { + var mock = new MockProcessInterop(); + ExcelProcessTracker.InteropProvider = mock; + + // PID 0 is ignored entirely (never tracked, never killed). ExcelProcessTracker.Track(0); + + // A non-zero PID *is* tracked: marking it as running in the mock means + // KillAllTracked should pick it up and route a Kill through the provider. + mock.RunningPids.Add(42); + ExcelProcessTracker.Track(42); + int killed = ExcelProcessTracker.KillAllTracked(); - Assert.Equal(0, killed); + + Assert.Equal(1, killed); + Assert.Contains(42, mock.KilledPids); + Assert.DoesNotContain(0, mock.KilledPids); } [Fact] diff --git a/README.md b/README.md index 3ce5d5e..cd0f5b8 100644 --- a/README.md +++ b/README.md @@ -187,7 +187,7 @@ BatchExcel/ ## Testing -The `BatchExcel.Tests` project (xUnit) currently includes **46 tests** covering: +The `BatchExcel.Tests` project (xUnit) currently includes **72 tests** covering: - `BatcherReader` — round-trips against generated `.xlsx` fixtures (no Excel required) - `OpenXmlHelpers` / `SheetWriter` — cell reference math, indexed bulk writes, typed-value coverage (`double` / `int` / `long` / `float` / `decimal` / `bool` / `DateTime`) @@ -195,6 +195,7 @@ The `BatchExcel.Tests` project (xUnit) currently includes **46 tests** covering: - `BatchConfig` — macro parsing, included-run count - `FileNameSanitizer` — invalid character handling - `UserSettings` — load / save round-trip +- `ExcelProcessTracker` — PID tracking, kill-only-running semantics, `SafeQuitExcel` kill-fallback (via mocked `IProcessInterop`) ```powershell dotnet test