Skip to content

fix: deliver a completed response after the request timeout expires - #151

Draft
AndreiSingeorzan wants to merge 1 commit into
masterfrom
fix/deliver-completed-response
Draft

fix: deliver a completed response after the request timeout expires#151
AndreiSingeorzan wants to merge 1 commit into
masterfrom
fix/deliver-completed-response

Conversation

@AndreiSingeorzan

Copy link
Copy Markdown

Draft: opening early for discussion on scope. The fix and its regression test are complete and green; see "Not included" for the two follow-ups I deliberately left out.

Problem

A request handler that outlives the server's request timeout still runs to completion, but its response is then silently discarded. The connection stays healthy, no error frame is sent, and the caller is told nothing — so a client without a RequestTimeout waits forever.

This is not hypothetical. It is the root cause of a UiPath Studio customer issue where opening a project hung indefinitely on "Loading project…". Studio's IProjectProcessControlService.OpenProject takes no CancellationToken and, on a cold machine, loads 681 dependency assemblies plus Roslyn compilations — comfortably past the 60 s request timeout that pipe used. Paired memory dumps showed the project process holding a completed Task<AssemblyLoadResult[]> with all 681 results while the shell still had the request pending on a Connected pipe.

Root cause

Server.OnRequestReceived:

var token = timeoutHelper.Token;
response = await HandleRequest(method, route, request, token);
await SendResponse(response, token);          // token may already be canceled
}
catch (Exception ex) when (response is null)  // false - the handler SUCCEEDED
{
    await OnError(request, timeoutHelper.CheckTimeout(ex, request.MethodName));
}
  1. The timeout fires while the handler runs. If the handler takes no CancellationToken, it never notices and keeps working — the timeout stops nothing.
  2. The handler completes, so response is non-null. The answer exists.
  3. SendResponse(response, token) is called with the already-canceled token. Connection.SendMessage observes it at _sendLock.WaitAsync(cancellationToken) and throws before a byte reaches the wire.
  4. The when (response is null) filter does not match, so OnError — which would have sent a Response.Fail on an uncancelable token — never runs.
  5. The outer catch traces the exception, the request is removed, and the server carries on healthily.

Note the asymmetry this creates: a handler that does take a CancellationToken throws at step 1, leaves response null, matches the filter, and the caller gets a clean TimeoutException. Identical timeout, opposite outcome, decided purely by whether the contract method happens to declare the parameter.

Fix

await SendResponse(response, default);

Once the handler has produced a response, the peer is owed it — cancelling delivery discards work that is already done. This makes the success path consistent with OnError, which has always sent on default for exactly this reason.

Worth noting how narrow the behavioural change is: Connection.SendMessage already writes with CancellationToken.None once it holds the send lock. The token only ever gated acquiring that lock. So this does not make the write less interruptible than it already was.

Test

RequestTimeoutTests + RequestTimeoutTestsOverNamedPipes, with a new ISlowService.EchoAfterIgnoringCancellation that deliberately declares no CancellationToken — the shape of the contracts that hit this. No existing test service had that shape, which is why the bug had no coverage.

Server timeout 1 s, handler 3 s, client sends no timeout.

Before the fix — fails at the 10 s lease:

Failed CompletedHandler_OutlivingTheRequestTimeout_StillGetsItsResponse [10 s]
  .EchoAfterIgnoringCancellation("payload", HandlerWork) should complete in 00:00:10 but did not.

with the library logging its own failure:

info: ServerConnection ... sending response for ISlowService EchoAfterIgnoringCancellation 0.
fail: ServerConnection ... # System.Threading.Tasks.TaskCanceledException: A task was canceled.
         at UiPath.Ipc.Server.OnRequestReceived(Request request) in Server.cs:line 105

After the fix — passes in 3 s, i.e. the moment the handler finishes:

t+ 0.00s  CLIENT  [3] sending request, awaiting the response with a 10s test lease
t+ 0.05s  SERVER  [4] handler entered - will work 3s, takes no CancellationToken parameter
t+ 1.07s  SERVER  [5] *** request timeout FIRED - token canceled, but nobody is observing it ***
t+ 3.06s  SERVER  [6] handler RAN TO COMPLETION - the answer now exists
t+ 3.06s  CLIENT  [7] response received: "payload"

Steps 1-6 are identical in both runs — the timeout still fires at 1 s and the handler still ignores it. Only the delivery changes.

The test keeps its step tracing deliberately: the failure mode is a silence, and the trace is what makes the chronology legible to the next person who reads it. On the failure path it also probes the connection afterwards and shows a second call on the same pipe succeeding, which is the part that makes this bug so confusing in the field.

Existing coverage confirms timeouts are not weakened — SystemTests.cs:73, SystemTests.cs:80 and ComputingTests.cs:84 still assert TimeoutException, and the full suite is green: 99 passed on net6.0, 97 on net461.

Not included, deliberately

  • The when (response is null) filter is still wrong in the general case. Any send failure after a successful handler — not just a timeout — is swallowed the same way. The robust form is a sent flag so no path can consume a request without answering it. That needs its own test with an induced send failure, so it belongs in a separate PR rather than riding on this one.
  • Only the named-pipe transport variant. The defect is in Server, above the transport, so one variant demonstrates it, but the repo convention is three. Happy to add Tcp and WebSockets here if preferred.

For reviewers

  • The download-stream path behaves differently and no test covers it: SendStream passes the token to the write and registers cancellationToken.UnsafeRegister(… Connection.Dispose()). Today a timed-out streaming response tears down the whole connection; under this change it is delivered normally. I believe that is the correct and consistent outcome, but it is a real behavioural change for Stream-returning contracts and deserves a second opinion.
  • Unrelated but adjacent, spotted while reading Request.GetTimeout: a client-supplied TimeoutInSeconds overrides the server's RequestTimeout with no ceiling, so a peer can pin a server-side request slot for as long as it likes. Worth its own issue.

🤖 Generated with Claude Code

A handler that takes no CancellationToken keeps running past the request
timeout, so `response` is non-null by the time SendResponse is reached. Sending
it on the already-canceled token throws in Connection.SendMessage before a byte
reaches the wire, and the `when (response is null)` filter does not match, so no
Response.Fail is sent either. The caller is told nothing and waits forever on a
healthy connection.

Send the completed response on `default` instead, as OnError already does. This
is narrow: Connection.SendMessage already writes with CancellationToken.None
once it holds the send lock, so the token only ever gated acquiring that lock.

Adds RequestTimeoutTests over named pipes, plus an ISlowService whose method
deliberately declares no CancellationToken - no existing test service had that
shape, which is why this had no coverage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant