chore: Handle sftp reconnections #4075

javier-aliaga · 2025-10-27T11:55:53Z

Description

This PR introduces a re-connection mechanism to the sftp binding.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #4061

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation
- Created the dapr/docs PR:

Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.

tests/utils/sftpproxy/proxy.go

bindings/sftp/client.go

JoshVanL · 2025-11-06T15:13:03Z

bindings/sftp/client.go

+	c.rLock.Lock()
+	defer c.rLock.Unlock()


It is hard to follow the logic when we have 2 locks

Added comment to explain why is each lock used, let me know what do you think

JoshVanL · 2025-11-06T15:16:25Z

bindings/sftp/client.go

+	msg := strings.ToLower(err.Error())
+	switch {
+	case strings.Contains(msg, "use of closed network connection"),
+		strings.Contains(msg, "connection reset by peer"),


Can we not use typed errors here? syscall.ECONNRESET

I will add to the previous if(L230), but I will keep the message here just in case.

I modified the method, we will not reconnect if it is one of this errors:

if errors.Is(err, sftpClient.ErrSSHFxPermissionDenied) || errors.Is(err, sftpClient.ErrSSHFxNoSuchFile) || errors.Is(err, sftpClient.ErrSSHFxOpUnsupported) { return false }

Any other error we will try to reconnect

bindings/sftp/client.go

bindings/sftp/sftp_integration_test.go

JoshVanL

I am still finding it hard to follow the logic here. We are not locking at the top of functions and we seem to be setting and unsetting need re-connection in multiple places.

bindings/sftp/client.go

JoshVanL · 2025-11-18T12:49:39Z

bindings/sftp/client.go

+	if !c.needsReconnect.Load() {
+		return nil
+	}


Move below lock

JoshVanL · 2025-11-18T12:50:18Z

bindings/sftp/client.go

+	pErr := c.ping()
+	if pErr != nil {


Suggested change

pErr := c.ping()

if pErr != nil {

err := c.ping()

if err == nil {

return nil

}

what if the c.ping works? should we set needsReconnect to false?

Yep, I think so :)

JoshVanL · 2025-11-18T12:51:18Z

bindings/sftp/client.go

+		oldSftp := c.sftpClient
+		oldSSH := c.sshClient


No need for these temp variables.

Suggested change

oldSftp := c.sftpClient

oldSSH := c.sshClient

if c.sftpClient != nil {

c.sftpClient.Close()

}

if c.sshClient != nil {

c.sshClient.Close()

}

JoshVanL · 2025-11-18T12:52:17Z

bindings/sftp/client.go

+	if c.shouldReconnect(err) {
+		c.log.Debugf("sftp binding error: %s", err)
+		c.needsReconnect.Store(true)
+	} else {
+		c.lock.RUnlock()
+		return err
+	}


Suggested change

if c.shouldReconnect(err) {

c.log.Debugf("sftp binding error: %s", err)

c.needsReconnect.Store(true)

} else {

c.lock.RUnlock()

return err

}

if !c.shouldReconnect(err) {

c.lock.RUnlock()

return err

}

c.log.Debugf("sftp binding error: %s", err)

c.needsReconnect.Store(true)

JoshVanL · 2025-11-18T12:52:41Z

bindings/sftp/client.go

+	}
+	c.lock.RUnlock()
+
+	if c.needsReconnect.Load() {


Suggested change

if c.needsReconnect.Load() {

JoshVanL · 2025-11-18T12:53:12Z

bindings/sftp/client.go

+	c.lock.RUnlock()
+
+	if c.needsReconnect.Load() {
+		c.log.Debugf("sftp binding: reconnecting to %s", c.address)


Move log line inside doReconnect, and only after we have locked and checked nedsReconnect.

cicoyle

few things from me. Thx!

bindings/sftp/client.go

cicoyle · 2025-11-19T16:57:32Z

bindings/sftp/client.go

+}
+
+func withReconnection(c *Client, fn func() error) error {
+	c.lock.RLock()


This locking then calling to reconnect then locking again feels like maybe we should have an abstraction function with a for loop where we only lock in that one place versus in a few places. Maybe it could be cleaner if we had a client.do func that had a for loop where we check the client, then do the shouldReconnect and reconnect calls - this way we try reconnecting more than once otherwise, as is we only try once I believe.

I did not want to put more complex mechanism with retries and backoffs, I think this component it may need to be rewritten as some customers requested different improvements. I did a small refactor to introduce a do func where we lock the action 51c4e94

what do you think?

I agree it can be rewritten and improved further. plz open an issue so we dont forget and it can be prioritized at a later date 👍🏻

cicoyle

lgtm, thx Javi!

Signed-off-by: Javier Aliaga <[email protected]>

javier-aliaga force-pushed the sftp-connections branch 19 times, most recently from a331e1d to f79c833 Compare November 3, 2025 12:52

javier-aliaga marked this pull request as ready for review November 3, 2025 12:53

javier-aliaga requested review from a team as code owners November 3, 2025 12:53

javier-aliaga force-pushed the sftp-connections branch 2 times, most recently from d211516 to d43bfab Compare November 3, 2025 13:24

JoshVanL requested changes Nov 6, 2025

View reviewed changes

javier-aliaga requested a review from JoshVanL November 7, 2025 09:33

javier-aliaga force-pushed the sftp-connections branch from 55f5a6a to 90f6d6b Compare November 7, 2025 09:38

mikeee reviewed Nov 7, 2025

View reviewed changes

bindings/sftp/client.go Outdated Show resolved Hide resolved

bindings/sftp/sftp_integration_test.go Show resolved Hide resolved

javier-aliaga force-pushed the sftp-connections branch 2 times, most recently from 69d8f53 to c3594d7 Compare November 12, 2025 10:44

javier-aliaga force-pushed the sftp-connections branch from 781c3fc to edd95a3 Compare November 12, 2025 11:17

JoshVanL requested changes Nov 12, 2025

View reviewed changes

bindings/sftp/client.go Outdated Show resolved Hide resolved

bindings/sftp/client.go Outdated Show resolved Hide resolved

bindings/sftp/client.go Outdated Show resolved Hide resolved

bindings/sftp/client.go Outdated Show resolved Hide resolved

javier-aliaga force-pushed the sftp-connections branch 5 times, most recently from 7353dde to a5af6f4 Compare November 18, 2025 12:23

JoshVanL requested changes Nov 18, 2025

View reviewed changes

javier-aliaga force-pushed the sftp-connections branch 3 times, most recently from 78a33bd to cc88c05 Compare November 18, 2025 14:07

JoshVanL previously approved these changes Nov 18, 2025

View reviewed changes

cicoyle requested changes Nov 19, 2025

View reviewed changes

javier-aliaga dismissed JoshVanL’s stale review via 1270d9b November 19, 2025 17:36

javier-aliaga force-pushed the sftp-connections branch 2 times, most recently from 1afe19b to 72d44bf Compare November 19, 2025 17:58

cicoyle approved these changes Nov 19, 2025

View reviewed changes

cicoyle added the automerge label Nov 19, 2025

javier-aliaga closed this Nov 20, 2025

javier-aliaga reopened this Nov 20, 2025

javier-aliaga closed this Nov 20, 2025

This was referenced Nov 20, 2025

chore: Handle sftp reconnections #4096

Closed

chore: Handle sftp reconnections #4097

Closed

javier-aliaga reopened this Nov 20, 2025

javier-aliaga force-pushed the sftp-connections branch from 72d44bf to a9de6e7 Compare November 20, 2025 17:12

chore: Handle sftp reconnections

b83457d

Signed-off-by: Javier Aliaga <[email protected]>

javier-aliaga force-pushed the sftp-connections branch from a9de6e7 to b83457d Compare November 20, 2025 17:12

cicoyle merged commit b4481d2 into dapr:main Nov 20, 2025
124 of 125 checks passed

cicoyle added this to the v1.17 milestone Nov 20, 2025

javier-aliaga added a commit to javier-aliaga/components-contrib that referenced this pull request Nov 20, 2025

chore: Handle sftp reconnections (dapr#4075)

29f9279

Signed-off-by: Javier Aliaga <[email protected]>

-		oldSftp := c.sftpClient
-		oldSSH := c.sshClient
+		if c.sftpClient != nil {
+		c.sftpClient.Close()
+		}
+		if c.sshClient != nil {
+		  c.sshClient.Close()
+		}

chore: Handle sftp reconnections #4075

chore: Handle sftp reconnections #4075

Uh oh!

Conversation

javier-aliaga commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javier-aliaga Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoshVanL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cicoyle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cicoyle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

javier-aliaga commented Oct 27, 2025 •

edited

Loading

javier-aliaga Nov 7, 2025 •

edited

Loading