feat(command): Add configurable timeout options for command execution

[thomhurst]

Summary

• Adds Timeout property (TimeSpan?) to CommandLineOptions for setting maximum command execution time
• Adds GracefulShutdownTimeout property (TimeSpan) with 30-second default for configuring graceful termination wait time
• Updates Command.cs to use these configurable timeout values instead of hardcoded 30-second graceful shutdown

Closes #1485

Test plan

• Verify build succeeds
• Test command execution with Timeout set to a value shorter than command duration - should cancel
• Test command execution with custom GracefulShutdownTimeout value
• Test that existing behavior (no timeout) still works when Timeout is null

🤖 Generated with Claude Code

[thomhurst]

Summary

Adds configurable timeout options for command execution, replacing the hardcoded 30-second graceful shutdown timeout with configurable Timeout and GracefulShutdownTimeout properties.

Critical Issues

None found ✅

Suggestions

1. Resource Disposal Order (Minor)

In Command.cs:271-278, the disposal order could potentially cause issues. The timeoutCancellationToken is disposed before linkedCancellationToken, but linkedCancellationToken references it. While this likely works due to implementation details, it would be more defensive to reverse the disposal order by declaring linkedCancellationToken first.

2. Redundant CancellationTokenSource Creation

When Timeout is null, a new CancellationTokenSource is created (Command.cs:274) but never used for cancellation - it just gets linked with the existing cancellationToken. This creates unnecessary allocations when no timeout is specified. However, the current approach favors code clarity over micro-optimization, which is reasonable.

Previous Review Status

Unable to retrieve previous comments due to API token scope limitations.

Verdict

✅ APPROVE - No critical issues. The implementation is sound and addresses the feature request properly. The suggestions are minor optimizations that don't affect correctness.

[Copilot]

Pull request overview

This PR adds configurable timeout options for command execution to ModularPipelines. It introduces two new properties to CommandLineOptions that allow users to control command execution timeouts and graceful shutdown behavior, replacing the previously hardcoded 30-second graceful shutdown timeout.

Key changes:

• Adds Timeout property (nullable TimeSpan) to specify maximum command execution time
• Adds GracefulShutdownTimeout property (TimeSpan) with a 30-second default for configurable graceful termination
• Updates command execution logic to use these configurable values with linked cancellation token sources

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/ModularPipelines/Options/CommandLineOptions.cs	Adds two new timeout-related properties with comprehensive XML documentation
src/ModularPipelines/Context/Command.cs	Implements timeout functionality using linked cancellation token sources and applies configurable graceful shutdown timeout
.gitignore	Updates gitignore entry for worktrees directory (cosmetic change)

[Copilot]

When Timeout is not set (null), this creates a CancellationTokenSource that never cancels. This unnecessarily creates a linked token source that combines the never-cancelling timeout token with the actual cancellation token.

Consider only creating the timeout cancellation token source when Timeout has a value, and only creating the linked token source when needed. This simplifies the logic and avoids unnecessary object allocations when no timeout is specified.

Suggested change

	// Create a timeout cancellation token if Timeout is specified
	using var timeoutCancellationToken = options.Timeout.HasValue
	? new CancellationTokenSource(options.Timeout.Value)
	: new CancellationTokenSource();
	// Link the timeout token with the passed cancellation token
	using var linkedCancellationToken = CancellationTokenSource.CreateLinkedTokenSource(
	timeoutCancellationToken.Token, cancellationToken);
	// Create a timeout CancellationTokenSource only if Timeout is specified
	using var timeoutCancellationToken = options.Timeout.HasValue
	? new CancellationTokenSource(options.Timeout.Value)
	: null;
	// Link the timeout token (if any) with the passed cancellation token
	using var linkedCancellationToken = timeoutCancellationToken != null
	? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken.Token, cancellationToken)
	: CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

[Copilot]

The new Timeout and GracefulShutdownTimeout properties lack test coverage. Based on the test structure in this repository (e.g., CommandTests.cs), tests should be added to verify:

1. Command execution respects the Timeout value and cancels when exceeded
2. Command execution with custom GracefulShutdownTimeout terminates appropriately
3. Command execution without Timeout (null) runs to completion normally

The existing CommandEchoTimeoutModule test doesn't actually test the new Timeout property on CommandLineOptions.

[thomhurst]

Summary

This PR adds configurable timeout options to command execution, replacing the hardcoded 30-second graceful shutdown timeout with configurable values.

Critical Issues

1. Resource Leak and Unnecessary Allocation (src/ModularPipelines/Context/Command.cs)

Issue: The code creates a CancellationTokenSource even when Timeout is null, which wastes resources and serves no purpose.

Current code:

using var timeoutCancellationToken = options.Timeout.HasValue
    ? new CancellationTokenSource(options.Timeout.Value)
    : new CancellationTokenSource();  // ← Creates unnecessary object when no timeout

Problem: When Timeout is null, this creates a CancellationTokenSource that is never used for timing purposes. We then link it with the original cancellation token, which is redundant.

Recommended fix:

// Only create timeout token if Timeout is specified
CancellationTokenSource? timeoutCancellationToken = options.Timeout.HasValue
    ? new CancellationTokenSource(options.Timeout.Value)
    : null;

// Link tokens only if we have a timeout, otherwise use original token
using var linkedCancellationToken = timeoutCancellationToken != null
    ? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken.Token, cancellationToken)
    : null;

var effectiveToken = linkedCancellationToken?.Token ?? cancellationToken;

// Dispose timeout token if created
timeoutCancellationToken?.Dispose();

Or even simpler, if timeout exists:

CancellationToken effectiveToken = cancellationToken;
CancellationTokenSource? timeoutCts = null;
CancellationTokenSource? linkedCts = null;

if (options.Timeout.HasValue)
{
    timeoutCts = new CancellationTokenSource(options.Timeout.Value);
    linkedCts = CancellationTokenSource.CreateLinkedTokenSource(timeoutCts.Token, cancellationToken);
    effectiveToken = linkedCts.Token;
}

try
{
    // ... use effectiveToken ...
}
finally
{
    linkedCts?.Dispose();
    timeoutCts?.Dispose();
}

2. Disposal Pattern Issue

Issue: The current using declarations may not guarantee proper disposal order. When both timeoutCancellationToken and linkedCancellationToken are created, they need to be disposed in the correct order (linked first, then timeout).

The current code has the using statements in the right order, but it's fragile. If Timeout is null, we're still creating and disposing unnecessary objects.

Suggestions

1. Add XML documentation example

Consider adding a usage example in the XML docs for the Timeout property to show developers how to use it:

/// <example>
/// <code>
/// var result = await context.Command.ExecuteCommandLineTool(
///     new CommandLineToolOptions("long-running-command")
///     {
///         Timeout = TimeSpan.FromMinutes(5)
///     });
/// </code>
/// </example>

2. Consider adding tests

The PR test plan mentions testing timeout scenarios, but I don't see test files in the diff. Consider adding unit tests for:

• Command execution that completes before timeout
• Command execution that exceeds timeout (should throw OperationCanceledException)
• Custom GracefulShutdownTimeout behavior
• Null timeout (existing behavior preserved)

Verdict

⚠️ REQUEST CHANGES - Critical resource leak/allocation issue must be fixed

[thomhurst]

Summary

This PR adds configurable timeout options for command execution: ExecutionTimeout for maximum command duration and GracefulShutdownTimeout for graceful termination wait time.

Critical Issues

1. Resource leak - CancellationTokenSource not properly disposed

In Command.cs:271-276, the timeoutCancellationToken is created with a using statement, but when ExecutionTimeout is null, a new CancellationTokenSource is created that will never be disposed:

using var timeoutCancellationToken = options.ExecutionTimeout.HasValue
    ? new CancellationTokenSource(options.ExecutionTimeout.Value)
    : new CancellationTokenSource();

The problem: When ExecutionTimeout is null, the CancellationTokenSource is created but never canceled, and the using statement will attempt to dispose it. However, this creates unnecessary allocations for every command execution when timeouts aren't used.

Solution: Only create the timeout token when needed:

using var timeoutCancellationToken = options.ExecutionTimeout.HasValue
    ? new CancellationTokenSource(options.ExecutionTimeout.Value)
    : null;

using var linkedCancellationToken = timeoutCancellationToken != null
    ? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken.Token, cancellationToken)
    : CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

2. Inconsistent property naming

The PR description mentions a Timeout property, but the implementation uses ExecutionTimeout. The XML documentation in CommandLineOptions.cs:70 also references "Timeout" in the remarks:

/// <para>When a command is cancelled (either via <see cref="Timeout"/> or an external cancellation token),

This should reference ExecutionTimeout instead.

Suggestions

1. Consider testing: No tests were added for this new functionality. Consider adding unit tests to verify:

   • Commands time out correctly when ExecutionTimeout is set
   • GracefulShutdownTimeout works as expected
   • Existing behavior is unchanged when ExecutionTimeout is null

2. Allocation concern: Creating a linked cancellation token for every command (even when timeout isn't needed) adds overhead. The allocation-conscious approach would be to only create the linked token when ExecutionTimeout has a value.

Verdict

⚠️ REQUEST CHANGES - Critical issues found (resource handling and documentation inconsistency)

[thomhurst]

Summary

Adds configurable execution timeout and graceful shutdown timeout to command execution.

Critical Issues

1. Resource leak in conditional CancellationTokenSource creation
In Command.cs:271-279, when ExecutionTimeout is not specified, CancellationTokenSource.CreateLinkedTokenSource(cancellationToken) is still created and disposed. This is unnecessary - you should only create the linked token source if there's a timeout OR the cancellation token can be cancelled.

// Current implementation (line 278-279):
using var linkedCancellationToken = timeoutCancellationToken != null
    ? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken.Token, cancellationToken)
    : CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);  // ← Unnecessary allocation

Suggested fix:

using var linkedCancellationToken = options.ExecutionTimeout.HasValue
    ? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken!.Token, cancellationToken)
    : null;

var effectiveCancellationToken = linkedCancellationToken?.Token ?? cancellationToken;

Then use effectiveCancellationToken in the registration and ExecuteAsync calls.

2. Potential ObjectDisposedException
In Command.cs:283, the registration callback uses linkedCancellationToken.Token which could be disposed before the callback executes in edge cases. While unlikely, this should be defensive.

Suggestions

Minor: .gitignore comment clarity
The comment "# Git worktrees directory" is good, but consider adding why this specific path (e.g., "# Local git worktrees directory used by development tools") to provide more context for future maintainers.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 should be addressed to avoid unnecessary allocations in the common case (no timeout specified).

[thomhurst]

Summary

Adds configurable timeout options for command execution with ExecutionTimeout and GracefulShutdownTimeout properties.

Critical Issues

Unnecessary CancellationTokenSource allocation when no timeout is set

Location: src/ModularPipelines/Context/Command.cs:310-312

The current implementation always creates a linkedCancellationToken even when timeoutCancellationToken is null:

using var linkedCancellationToken = timeoutCancellationToken != null
    ? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken.Token, cancellationToken)
    : CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

When there's no timeout, this creates an unnecessary CancellationTokenSource that just wraps the original cancellationToken. This defeats the purpose of the optimization comment on line 304 ("avoid unnecessary allocations").

Suggested fix: Only create the linked token when a timeout is specified, otherwise use the original cancellationToken directly:

// Only create timeout token if ExecutionTimeout is specified to avoid unnecessary allocations
using var timeoutCancellationToken = options.ExecutionTimeout.HasValue
    ? new CancellationTokenSource(options.ExecutionTimeout.Value)
    : null;

// Link the timeout token with the passed cancellation token if timeout is specified
using var linkedCancellationToken = timeoutCancellationToken != null
    ? CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellationToken.Token, cancellationToken)
    : null;

var effectiveCancellationToken = linkedCancellationToken?.Token ?? cancellationToken;

using var forcefulCancellationToken = new CancellationTokenSource();

await using var registration = effectiveCancellationToken.Register(() =>
{
    try
    {
        if (forcefulCancellationToken.Token.CanBeCanceled)
        {
            forcefulCancellationToken.CancelAfter(options.GracefulShutdownTimeout);
        }
    }
    catch (ObjectDisposedException)
    {
        // Ignored
    }
});

try
{
    var result = await command
        .WithStandardOutputPipe(PipeTarget.ToStringBuilder(standardOutputStringBuilder))
        .WithStandardErrorPipe(PipeTarget.ToStringBuilder(standardErrorStringBuilder))
        .WithValidation(CommandResultValidation.None)
        .ExecuteAsync(forcefulCancellationToken.Token, effectiveCancellationToken).ConfigureAwait(false);
    // ... rest of the code
}

This ensures that when ExecutionTimeout is null (the default/common case), no extra allocations are made.

Verdict

⚠️ REQUEST CHANGES - Critical performance issue with unnecessary allocations in the default (no timeout) path