Don't hide cancellation reason

[fkorotkov]

Otherwise, we assume a task has timeout when in reality it could've been externally interrupted.

[fkorotkov]

I'll cherry pick changes to the new CLI after this change is merged.

[edigaryev]

We already know this because we have the log.Printf("Captured %v...", sig) below.

[edigaryev]

Can we not change the error message to avoid changing tests and potentially breaking things for people expecting "Timed out" string in the instruction's logs?

[fkorotkov]

Linting will fail I think. Errors should be lowercase

[edigaryev]

Let's add a //nolint: then.

[fkorotkov]

Can do something like this though 967a6a5

[edigaryev]

Perhaps we can skip using the WithTimeoutCause() if we're never using the context.Cause()?

[fkorotkov]

Didn't read docs that deep and thought ctx.Err() will have it set. See 349540a

[edigaryev]

Sorry, but I'm still not sure why we need it if we already have context.DeadlineExceeded that serves the same purpose.

[edigaryev]

It feels like a proper approach here would be branch on ctx.Err():

diff --git a/internal/executor/shell.go b/internal/executor/shell.go
index 4545003..9e419cd 100644
--- a/internal/executor/shell.go
+++ b/internal/executor/shell.go
@@ -61,7 +61,15 @@ func ShellCommandsAndWait(
 
        select {
        case <-ctx.Done():
-               handler([]byte("\nTimed out!"))
+               var message string
+
+               if errors.Is(ctx.Err(), context.DeadlineExceeded) {
+                       message = "Timed out!"
+               } else {
+                       message = fmt.Sprintf("Context terminated unexpectedly: %v", ctx.Err())
+               }
+
+               handler([]byte("\n" + message))
 
                processdumper.Dump()
 
@@ -69,7 +77,7 @@ func ShellCommandsAndWait(
                        handler([]byte(fmt.Sprintf("\nFailed to kill a timed out shell session: %s", err)))
                }
 
-               return cmd, TimeOutError
+               return cmd, ctx.Err()
        case <-done:
                var forcePiperClosure bool

This will keep the old behavior in place and won't munge the error message.

[fkorotkov]

IMO the cause approach is better. It's clear what was the cause. DeadlineExceeded might come from somehting else in the future, for example.

[fkorotkov]

Plus what kind of error we need to return? ctx.Err() or TimeOutError?

[edigaryev]

What do you think about also printing the value of response.TimeoutInSeconds and/or timeoutCtx.Deadline() to the agent's log?

This should provide some additional clues.

[fkorotkov]

Don't think the issue is with TimeoutInSeconds since we saw "Capruted terminated..." log message. "Timed out!" message is just misleading in that case.

[edigaryev]

You can probably use cases.Title() here to skip the gory details of string manipulation.