Skip to content

[Correctness] ProcessWrapper.WaitUntilRunningAsync — always waits the full timeout, never returns 'running' early #858

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Warning

File: src/Servy.Service/ProcessManagement/ProcessWrapper.cs, lines 183-200

The method name promises an early-return as soon as the child becomes "running". It does not — it loops sleeping 500ms until the full timeout elapses, only checking for HasExited:

public async Task<bool> WaitUntilRunningAsync(TimeSpan timeout, CancellationToken cancellationToken = default)
{
    ThrowIfDisposed();
    var start = DateTime.UtcNow;
    while (DateTime.UtcNow - start < timeout)
    {
        cancellationToken.ThrowIfCancellationRequested();
        if (_process.HasExited)
            return false; // process exited before becoming healthy
        await Task.Delay(500, cancellationToken);
    }
    return !_process.HasExited;
}

There is no path that returns true before timeout elapses. The method's semantics in practice are "sleep for timeout, then report whether the process is still alive", not "wait until running".

The single caller (Service.cs:1208) uses the return value to gate the post-launch hook:

if (await _childProcess.WaitUntilRunningAsync(TimeSpan.FromSeconds(_options!.StartTimeout), cts.Token))
{
    StartPostLaunchProcess();
}

Consequence: with StartTimeout = 30 (default) the post-launch hook is delayed a full 30 seconds even when the child started successfully in 100 ms. Users configure StartTimeout as an upper bound for slow startups, not as a deliberate post-launch delay — so the post-launch script is paying the worst-case wait every start.

Suggested fix:
Either rename to WaitForExitOrTimeoutAsync (so the contract matches the behavior — useful as a "survived first N seconds" probe), or actually implement an early-exit signal:

  • Add a real readiness check (e.g., process responds to a no-op, or an internal "started" flag has been set), and break out of the loop the moment that fires.
  • Trigger post-launch as soon as Process.Start() returns and _process.HasExited is false — the existing 500ms sleep + full-timeout pattern adds latency without adding signal.

If the goal is just "wait a tiny bit so the child is past initial-fault territory before we run post-launch", a fixed 1-2 second delay would be both honest and faster than the current behavior.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions