Skip to content

Clarify DefaultNodeConnectionTimeout#13216

Draft
rainersigwald wants to merge 1 commit intomainfrom
clearer-timeout-comment
Draft

Clarify DefaultNodeConnectionTimeout#13216
rainersigwald wants to merge 1 commit intomainfrom
clearer-timeout-comment

Conversation

@rainersigwald
Copy link
Member

@baronfel and I both looked around this and were confused by the description of this variable, which is actually just the post-build node idle timeout.

Chet and I both looked around this and were confused by the description of this variable, which is actually just the post-build node idle timeout.
@rainersigwald rainersigwald self-assigned this Feb 6, 2026
Copilot AI review requested due to automatic review settings February 6, 2026 19:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Clarifies the documentation for DefaultNodeConnectionTimeout in CommunicationsUtilities to reduce confusion about what the timeout represents in the node infrastructure.

Changes:

  • Updates the XML doc summary for DefaultNodeConnectionTimeout.
  • Adds remarks explaining the tradeoff between node reuse and resource usage.

Comment on lines +394 to +396
/// Wait time between when a node goes idle and its self-termination.
/// </summary>
/// <remarks>
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated XML docs describe this as an idle/self-termination timeout, but this value is also used as a general connection/handshake wait timeout (e.g., host-side wait in NodeProviderInProc and server-side wait for initial connection in NodePipeServer/NodeEndpointOutOfProcBase). Consider rewording to cover both usages (max time to establish/wait for a connection, which also effectively bounds how long an idle node waits for the next build before exiting), otherwise readers may be misled.

Suggested change
/// Wait time between when a node goes idle and its self-termination.
/// </summary>
/// <remarks>
/// Default maximum time (in milliseconds) to wait for a node connection/handshake and,
/// once connected, the maximum time a node may remain idle before self-terminating.
/// </summary>
/// <remarks>
/// This value bounds both how long the host or server will wait for a node to establish a
/// connection/handshake and how long an idle node will wait for the next build before exiting.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good feedback but to me it looks like the other uses are flatly wrong and using 15 minute timeouts in those cases is bananas. Need to look deeper, so going back to draft here.

@rainersigwald rainersigwald marked this pull request as draft February 9, 2026 14:55
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.

3 participants

Comments