fix: resolve COM memory instability and ensure graceful Excel teardown

BatchExcel.Tests/ExcelProcessTrackerTests.cs

@@ -0,0 +1,101 @@
+using System.Diagnostics;
+using BatchExcel.Services;
+using Xunit;
+
+namespace BatchExcel.Tests;
+
+// 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
+    {
+        public HashSet<int> RunningPids { get; } = new();
+        public List<int> 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";
+        }
+    }
+
+    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()
+    {
+        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(1, killed);
+        Assert.Contains(42, mock.KilledPids);
+        Assert.DoesNotContain(0, mock.KilledPids);
+    }
+
+    [Fact]
+    public void KillAllTracked_KillsOnlyRunningProcesses()
+    {
+        var mock = new MockProcessInterop();
+        mock.RunningPids.Add(123);
+        mock.RunningPids.Add(456);
+
+        ExcelProcessTracker.InteropProvider = mock;
+        ExcelProcessTracker.Track(123);
+        ExcelProcessTracker.Track(456);
+        ExcelProcessTracker.Track(789); // Not running according to mock
+
+        int killed = ExcelProcessTracker.KillAllTracked();
+
+        Assert.Equal(2, killed);
+        Assert.Contains(123, mock.KilledPids);
+        Assert.Contains(456, mock.KilledPids);
+        Assert.DoesNotContain(789, mock.KilledPids);
+    }
+
+    [Fact]
+    public void SafeQuitExcel_FallsBackToKill_IfProcessStaysRunning()
+    {
+        var mock = new MockProcessInterop();
+        uint pid = 999;
+        mock.RunningPids.Add((int)pid);
+
+        ExcelProcessTracker.InteropProvider = mock;
+        ExcelProcessTracker.Track(pid);
+
+        // 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
+        Assert.Contains((int)pid, mock.KilledPids);
+        Assert.Equal(0, ExcelProcessTracker.KillAllTracked()); // Should be untracked
+    }
+}

BatchExcel/Services/ExcelProcessTracker.cs

@@ -1,8 +1,51 @@
-using System.Diagnostics;
+using System.Diagnostics;
 using System.Runtime.InteropServices;
 
 namespace BatchExcel.Services;
 
+/// <summary>
+/// Abstraction for OS process interactions to enable deterministic testing.
+/// </summary>
+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; }
+    }
+}
+
 /// <summary>
 /// Tracks Excel process IDs to enable reliable cleanup of zombie processes.
 /// Uses the Excel Application HWND to resolve the actual OS process ID.
@@ -15,21 +58,34 @@ public static class ExcelProcessTracker
     private static readonly object Lock = new();
     private static readonly HashSet<uint> TrackedPids = new();
 
+    internal static IProcessInterop InteropProvider { get; set; } = new DefaultProcessInterop();
+
     /// <summary>
     /// Gets the OS process ID for an Excel Application COM object using its window handle.
+    /// Returns 0 if the process ID cannot be resolved.
     /// </summary>
     public static uint GetExcelProcessId(dynamic excelApp)
     {
-        IntPtr hwnd = new IntPtr((int)excelApp.Hwnd);
-        GetWindowThreadProcessId(hwnd, out uint pid);
-        return pid;
+        try
+        {
+            // 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;
+        }
+        catch (Exception ex)
+        {
+            Debug.WriteLine($"[ExcelProcessTracker] Failed to resolve PID: {ex.Message}");
+            return 0;
+        }
     }
 
     /// <summary>
     /// Registers an Excel process ID for tracking.
     /// </summary>
     public static void Track(uint pid)
     {
+        if (pid == 0) return;
         lock (Lock)
         {
             TrackedPids.Add(pid);
@@ -41,6 +97,7 @@ public static void Track(uint pid)
     /// </summary>
     public static void Untrack(uint pid)
     {
+        if (pid == 0) return;
         lock (Lock)
         {
             TrackedPids.Remove(pid);
@@ -65,19 +122,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
             }
@@ -92,31 +144,31 @@ public static int KillAllTracked()
     /// </summary>
     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
         {
             // Ignore quit errors
         }
         finally
         {
-            try
-            {
-                Marshal.FinalReleaseComObject(excelApp);
-            }
-            catch
-            {
-                // Ignore
-            }
+            // Ensure the local dynamic reference is cleared from the stack before collecting,
+            // otherwise the GC might still see it as a root.
+            excelApp = null;
 
-            // 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();
 
@@ -125,25 +177,23 @@ 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
                 }
             }
 
             Untrack(pid);
         }
     }
 }
-