executor: fix context-cancellation leaks, local copy bugs, drop dead options

- sshRun: buffer the done channel so the command goroutine can exit
  after a ctx-cancel return instead of blocking forever (goroutine leak).
- sftpUpload/sftpDownload: build the sftp client over an ssh session we
  hold (newSftpSession), and on ctx-cancel close that session to abort
  the in-flight transfer from outside the sftp mutex, then drain the copy
  goroutine before returning. newSftpSession drains the session stderr
  pipe in a goroutine (RequestSubsystem never starts the copy that
  Session.Stderr would use, so unread stderr would stall the channel).
  The cancel drain is bounded by a grace timeout so an app-level wedge
  cannot hang the deploy; a transport-level wedge is left to a follow-up.
- sftpDownload now downloads into a temp file in the destination dir and
  renames over the destination only on success, so a canceled or failed
  download no longer truncates or destroys an existing local file (newly
  reachable now that a download actually aborts mid-copy).
- Local.Upload: multi-file glob + mkdir creates the destination directory
  itself, and honors context cancellation between files.
- Local.Download: pass the caller context through.
- Connector.String: cap the private-key slice with min().
- Remove dead options never read by any executor: Checksum from
  UpDownOpts and SyncOpts, and Force from SyncOpts, plus the config
  sync 'force' field (undocumented no-op) and its schema entry. Behavior
  change: under strict YAML a playbook that set 'sync: {force: true}'
  will now fail to parse.

Author: paskal

pkg/config/command.go

@@ -68,7 +68,6 @@ type SyncInternal struct {
 	Dest    string   `yaml:"dst" toml:"dst"`         // destination must be a directory
 	Delete  bool     `yaml:"delete" toml:"delete"`   // delete files in destination that are not in source
 	Exclude []string `yaml:"exclude" toml:"exclude"` // exclude files matching these patterns
-	Force   bool     `yaml:"force" toml:"force"`     // force sync even if source and destination are the same
 }
 
 // DeleteInternal defines delete command, implemented internally

pkg/executor/connector.go

@@ -177,5 +177,7 @@ func (c *Connector) sshConfig(user, privateKeyPath string) (*ssh.ClientConfig, n
 }
 
 func (c *Connector) String() string {
-	return fmt.Sprintf("ssh connector with private key %s.., timeout %v, agent %v", c.privateKey[:8], c.timeout, c.enableAgent)
+	// cap the slice so it does not run past the end for short or empty (agent-only) keys
+	key := c.privateKey[:min(len(c.privateKey), 8)]
+	return fmt.Sprintf("ssh connector with private key %s.., timeout %v, agent %v", key, c.timeout, c.enableAgent)
 }

pkg/executor/connector_test.go

@@ -5,9 +5,31 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func TestConnector_String(t *testing.T) {
+	tests := []struct {
+		name       string
+		key        string
+		wantSubstr string
+	}{
+		{"agent-only empty key", "", "private key .., "},
+		{"short key", "abc", "private key abc.., "},
+		{"exactly eight", "12345678", "private key 12345678.., "},
+		{"long key path", "/home/user/.ssh/id_rsa", "private key /home/us.., "},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			c := &Connector{privateKey: tc.key, timeout: time.Second}
+			var got string
+			assert.NotPanics(t, func() { got = c.String() }, "String must not panic on short or empty keys")
+			assert.Contains(t, got, tc.wantSubstr, "String must show at most the first 8 key chars")
+		})
+	}
+}
+
 func TestConnector_Connect(t *testing.T) {
 	ctx := context.Background()
 	hostAndPort, teardown := startTestContainer(t)

pkg/executor/executor.go

@@ -27,18 +27,15 @@ type RunOpts struct {
 
 // UpDownOpts is a struct for upload and download options.
 type UpDownOpts struct {
-	Mkdir    bool     // create remote directory if it does not exist
-	Checksum bool     // compare checksums of local and remote files, default is size and modtime
-	Force    bool     // overwrite existing files on remote
-	Exclude  []string // exclude files matching the given patterns
+	Mkdir   bool     // create remote directory if it does not exist
+	Force   bool     // overwrite existing files on remote
+	Exclude []string // exclude files matching the given patterns
 }
 
 // SyncOpts is a struct for sync options.
 type SyncOpts struct {
-	Delete   bool     // delete extra files on remote
-	Exclude  []string // exclude files matching the given patterns
-	Checksum bool     // compare checksums of local and remote files, default is size and modtime
-	Force    bool     // overwrite existing files on remote
+	Delete  bool     // delete extra files on remote
+	Exclude []string // exclude files matching the given patterns
 }
 
 // DeleteOpts is a struct for delete options.

pkg/executor/local.go

@@ -67,7 +67,7 @@ func (l *Local) Run(ctx context.Context, cmd string, _ *RunOpts) (out []string,
 }
 
 // Upload just copy file from one place to another
-func (l *Local) Upload(_ context.Context, src, dst string, opts *UpDownOpts) (err error) {
+func (l *Local) Upload(ctx context.Context, src, dst string, opts *UpDownOpts) (err error) {
 
 	// check if the local parameter contains a glob pattern
 	matches, err := filepath.Glob(src)
@@ -88,12 +88,20 @@ func (l *Local) Upload(_ context.Context, src, dst string, opts *UpDownOpts) (er
 	}
 
 	if mkdir {
-		if err = os.MkdirAll(filepath.Dir(dst), 0o750); err != nil {
-			return fmt.Errorf("can't create local dir %s: %w", filepath.Dir(dst), err)
+		// with multiple matches dst is treated as a directory, so create it; otherwise create its parent
+		mkdirTarget := filepath.Dir(dst)
+		if len(matches) > 1 {
+			mkdirTarget = dst
+		}
+		if err = os.MkdirAll(mkdirTarget, 0o750); err != nil {
+			return fmt.Errorf("can't create local dir %s: %w", mkdirTarget, err)
 		}
 	}
 
 	for _, match := range matches {
+		if err := ctx.Err(); err != nil { // honor cancellation between files
+			return err
+		}
 		relPath, e := filepath.Rel(filepath.Dir(src), match)
 		if e != nil {
 			return fmt.Errorf("failed to build relative path for %s: %w", match, e)
@@ -139,8 +147,8 @@ func (l *Local) Upload(_ context.Context, src, dst string, opts *UpDownOpts) (er
 }
 
 // Download just copy file from one place to another
-func (l *Local) Download(_ context.Context, src, dst string, opts *UpDownOpts) (err error) {
-	return l.Upload(context.Background(), src, dst, opts) // same as upload for local
+func (l *Local) Download(ctx context.Context, src, dst string, opts *UpDownOpts) (err error) {
+	return l.Upload(ctx, src, dst, opts) // same as upload for local
 }
 
 // Sync directories from src to dst

pkg/executor/local_test.go

@@ -866,6 +866,52 @@ func TestDeleteWithExclude(t *testing.T) {
 	}
 }
 
+func TestLocal_UploadMultiGlobMkdir(t *testing.T) {
+	srcDir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(srcDir, "a.txt"), []byte("a"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(srcDir, "b.txt"), []byte("b"), 0o644))
+
+	// dst directory does not exist yet; with multiple matches it must be created as a directory
+	dst := filepath.Join(t.TempDir(), "sub", "dest")
+	l := &Local{}
+	err := l.Upload(context.Background(), filepath.Join(srcDir, "*.txt"), dst, &UpDownOpts{Mkdir: true})
+	require.NoError(t, err)
+
+	assert.FileExists(t, filepath.Join(dst, "a.txt"))
+	assert.FileExists(t, filepath.Join(dst, "b.txt"))
+}
+
+func TestLocal_UploadCanceledContext(t *testing.T) {
+	srcDir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(srcDir, "a.txt"), []byte("a"), 0o644))
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // already canceled
+
+	l := &Local{}
+	err := l.Upload(ctx, filepath.Join(srcDir, "*.txt"), t.TempDir(), &UpDownOpts{Mkdir: true})
+	require.ErrorIs(t, err, context.Canceled, "upload should honor a canceled context")
+}
+
+func TestLocal_UploadMultiFileCanceled(t *testing.T) {
+	srcDir := t.TempDir()
+	for _, n := range []string{"a.txt", "b.txt", "c.txt"} {
+		require.NoError(t, os.WriteFile(filepath.Join(srcDir, n), []byte(n), 0o644))
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // canceled before the per-file loop runs
+
+	dst := t.TempDir()
+	l := &Local{}
+	err := l.Upload(ctx, filepath.Join(srcDir, "*.txt"), dst, &UpDownOpts{Mkdir: true})
+	require.ErrorIs(t, err, context.Canceled, "multi-file upload should honor a canceled context")
+
+	entries, err := os.ReadDir(dst)
+	require.NoError(t, err)
+	assert.Empty(t, entries, "no files should be copied once the context is canceled")
+}
+
 func TestClose(t *testing.T) {
 	l := &Local{}
 	err := l.Close()