classify error as retryable when pipe closed in deadlineCappableConn#4567
Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| } | ||
|
|
||
| if _, ok := errors.AsType[*exceptions.SSHTunnelClosedError](err); ok { | ||
| return ErrorRetryRecoverable, ErrorInfo{ |
There was a problem hiding this comment.
This is a comment just for my own understanding.
Do these errors need to be notified to telemetry?
From the PR description and the original PR it seems that they only happen in a certain situation where the pipe is going to be terminated anyway.
No action is required from us or the customer. It seems to me that we would only pay attention to them if we were inspecting logs manually, right?
There was a problem hiding this comment.
It could be argued to be overly cautious, but if the error is only to expected to occur once, recoverable allows us to still notice if it persists in case there's a bug in some of our logic. The "pipe closed" refers to the io.Pipe used for SSH, not the ClickPipe, and repeatedly hitting this error on read could mean the ClickPipe becomes blocked.
There was a problem hiding this comment.
echo @ilidemi that this leans overly cautious. Because this net.Pipe() solution is somewhat of an unconventional (and creative) workaround, this provides a peace of mind knowing we'd get notified if error persists (vs. unconditionally ignore it in the Read/Write methods).
| } | ||
|
|
||
| if _, ok := errors.AsType[*exceptions.SSHTunnelClosedError](err); ok { | ||
| return ErrorRetryRecoverable, ErrorInfo{ |
There was a problem hiding this comment.
It could be argued to be overly cautious, but if the error is only to expected to occur once, recoverable allows us to still notice if it persists in case there's a bug in some of our logic. The "pipe closed" refers to the io.Pipe used for SSH, not the ClickPipe, and repeatedly hitting this error on read could mean the ClickPipe becomes blocked.
3 participants
Classify a transient error that can happen with DeadlineCapableConn. In #4380 we introduced an in-memory transport between connector and ssh using net.Pipe() to allow it to respect deadlines. This introduced a scenario where if the ssh tunnel is processing read/write when the pipe is closed, we get an
io.ErrClosedPipethat ended up unhandled. The error is wrapped in anSSHTunnelClosedErrorto be precise, to avoid blindly handle allio.ErrClosedPipethat could hide future unrelated bugs.Fixes: DBI-896