From b7efa3356bc3bb56e915223de327171ea29f652b Mon Sep 17 00:00:00 2001 From: Muhannad Al-Khatib Date: Tue, 25 Aug 2026 16:55:34 +0300 Subject: [PATCH 1/2] Kill child process on CommandTimeout to prevent stale-result mix-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The process is reused across sequential commands within one invocation (e.g. a create-shipment command followed by a log-fetch command in a finally block). taskCompletionSource, its reflection helpers, and invocationTimeoutTimer are single mutable fields with no per-invocation correlation, so if a command times out but its process keeps running, a late stdout line is applied to whichever invocation is current by then and silently resolves it with stale/wrong data — or, if nothing ever arrives, the caller can hang indefinitely with no logged error at all. Root-caused against a live production incident: a downstream consumer had a call sit unacknowledged in its message queue for 11+ minutes past both the configured CommandTimeout and IdleTimeout, with zero log output anywhere in the call chain — consistent with this class of orphaned-TCS hang rather than a carrier- or endpoint-specific bug. A retry through a separate code path succeeded immediately and appears to have also resolved the original stuck invocation, consistent with a stale/orphaned completion rather than a genuine downstream failure. Now the timed-out process is killed immediately. No further output can arrive to mis-resolve a later invocation, and the next InvokeAsync on this instance fails fast via the existing "process not started or terminated" guard instead of hanging or resolving to a stale result. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 5 --- SW.Serverless/Services/ServerlessService.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/SW.Serverless/Services/ServerlessService.cs b/SW.Serverless/Services/ServerlessService.cs index 10ef172..2afb987 100644 --- a/SW.Serverless/Services/ServerlessService.cs +++ b/SW.Serverless/Services/ServerlessService.cs @@ -173,6 +173,25 @@ void InvocationTimeoutTimerCallback(object state) { invocationTimeoutTimer.Dispose(); trySetTrySetExceptionMethod.Invoke(taskCompletionSource, new object[] { new TimeoutException() }); + + // The process is reused for multiple sequential commands on the same invocation + // (e.g. CreateShipment followed by GetLogs in a finally block). If we leave a + // timed-out process running, its eventual late output is delivered to whichever + // taskCompletionSource is current by then - a later, unrelated invocation - and + // silently resolves it with the wrong (stale) result instead of its own. Killing + // the process here removes that possibility entirely: no further output can ever + // arrive, and the next InvokeAsync on this instance fails fast via the + // "process not started or terminated" guard instead of hanging or being + // mis-resolved. + try + { + if (processStarted && !process.HasExited) + process.Kill(); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Failed to kill timed-out adapter process."); + } } void ErrorDataReceived(object sender, DataReceivedEventArgs args) From 796afebd22f33f200869df31abea666fd1bf2898 Mon Sep 17 00:00:00 2001 From: Muhannad Al-Khatib Date: Tue, 25 Aug 2026 17:03:44 +0300 Subject: [PATCH 2/2] Fix ordering race: kill the process before completing the timeout Task CodeRabbit caught a real gap in the previous commit: it completed the shared taskCompletionSource before killing the process. That let the awaiter's continuation (e.g. the very finally-block follow-up call this fix exists to protect) start a new InvokeAsync and overwrite taskCompletionSource while the old process was still alive, racing the kill. And if process.Kill() itself threw, processStarted stayed true, silently permitting reuse of an instance whose process might still be running. Now: kill first, then complete the Task in a finally so the exception still surfaces even if Kill() throws. A new sticky `timedOut` field is set unconditionally before the kill attempt and checked alongside the existing HasExited guard, so a failed Kill() no longer leaves the instance falsely reusable. Co-Authored-By: Claude Sonnet 5 --- SW.Serverless/Services/ServerlessService.cs | 24 +++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/SW.Serverless/Services/ServerlessService.cs b/SW.Serverless/Services/ServerlessService.cs index 2afb987..fc36518 100644 --- a/SW.Serverless/Services/ServerlessService.cs +++ b/SW.Serverless/Services/ServerlessService.cs @@ -30,6 +30,7 @@ public class ServerlessService : IServerlessService, IDisposable private MethodInfo trySetTrySetExceptionMethod; private Timer invocationTimeoutTimer; private bool processStarted; + private volatile bool timedOut; private ILogger adapterLogger; private readonly ICloudFilesService cloudFilesService; public ServerlessService(ServerlessOptions serverlessOptions, IMemoryCache memoryCache, ILoggerFactory loggerFactory, IServiceProvider serviceProvider, ICloudFilesService cloudFilesService) @@ -141,7 +142,7 @@ async public Task InvokeAsync(string command, object input, in throw new ArgumentException("Invalid name.", nameof(command)); } - if (!processStarted || process.HasExited) + if (!processStarted || process.HasExited || timedOut) throw new Exception("Process not started or terminated."); taskCompletionSource = new TaskCompletionSource(); @@ -172,17 +173,22 @@ async public Task InvokeAsync(string command, object input, in void InvocationTimeoutTimerCallback(object state) { invocationTimeoutTimer.Dispose(); - trySetTrySetExceptionMethod.Invoke(taskCompletionSource, new object[] { new TimeoutException() }); // The process is reused for multiple sequential commands on the same invocation // (e.g. CreateShipment followed by GetLogs in a finally block). If we leave a // timed-out process running, its eventual late output is delivered to whichever // taskCompletionSource is current by then - a later, unrelated invocation - and - // silently resolves it with the wrong (stale) result instead of its own. Killing - // the process here removes that possibility entirely: no further output can ever - // arrive, and the next InvokeAsync on this instance fails fast via the - // "process not started or terminated" guard instead of hanging or being - // mis-resolved. + // silently resolves it with the wrong (stale) result instead of its own. + // + // Order matters here: kill (and mark this instance permanently dead) BEFORE + // completing the caller's Task. Completing the Task first would let the awaiter's + // continuation - e.g. that same finally-block GetLogs follow-up - start a new + // InvokeAsync and overwrite taskCompletionSource while the old process is still + // alive, racing the kill below. Killing first, and sticking `timedOut` regardless + // of whether Kill() itself throws, guarantees no further output can ever arrive and + // that the next InvokeAsync on this instance fails fast via the "process not + // started or terminated" guard instead of hanging or being mis-resolved. + timedOut = true; try { if (processStarted && !process.HasExited) @@ -192,6 +198,10 @@ void InvocationTimeoutTimerCallback(object state) { logger.LogWarning(ex, "Failed to kill timed-out adapter process."); } + finally + { + trySetTrySetExceptionMethod.Invoke(taskCompletionSource, new object[] { new TimeoutException() }); + } } void ErrorDataReceived(object sender, DataReceivedEventArgs args)