Skip to content

Commit 035fdb1

Browse files
authored
Merge pull request #1 from alexeyu/fix/must-fix-issues
Fix/must fix issues
2 parents ff89bfe + fd85a25 commit 035fdb1

17 files changed

Lines changed: 393 additions & 17 deletions

File tree

.githooks/pre-commit

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
#!/usr/bin/env bash
2+
# Pre-commit hook: keep commits formatted and vet-clean so CI's
3+
# golangci-lint pass never fails on something a local check would catch.
4+
#
5+
# Enable it once per clone with: make hooks
6+
# (which runs: git config core.hooksPath .githooks)
7+
set -euo pipefail
8+
9+
cd "$(git rev-parse --show-toplevel)"
10+
11+
# Full CI parity when the linter is installed: golangci-lint@v2.12 runs
12+
# gofmt + goimports + lll, matching .github/workflows/ci.yml.
13+
if command -v golangci-lint >/dev/null 2>&1; then
14+
exec golangci-lint run
15+
fi
16+
17+
# Fallback when golangci-lint isn't installed. gofmt is the check that
18+
# would have caught the formatting miss; goimports/lll are only enforced
19+
# by the linter, so install golangci-lint for complete coverage.
20+
unformatted="$(gofmt -l .)"
21+
if [ -n "$unformatted" ]; then
22+
echo "pre-commit: these files are not gofmt-clean:" >&2
23+
echo "$unformatted" >&2
24+
echo "fix with: gofmt -w ." >&2
25+
exit 1
26+
fi
27+
28+
go vet ./...

.github/workflows/ci.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ jobs:
1717
with:
1818
go-version-file: go.mod
1919
- run: go build ./...
20+
- name: go.mod is tidy
21+
run: go mod tidy -diff
2022
- run: go vet ./...
2123
- run: go test ./... -race
2224
- name: Integration tests (real FTP/FTPS/SFTP servers via Docker)

Makefile

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
BINARY := uploadfun
22
VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev)
33

4-
.PHONY: build test test-integration lint vet run clean
4+
.PHONY: build test test-integration lint vet run clean hooks
55

66
build:
77
go build -ldflags "-X main.version=$(VERSION)" -o $(BINARY) ./cmd/uploadfun
@@ -23,3 +23,7 @@ run: build
2323

2424
clean:
2525
rm -f $(BINARY)
26+
27+
# Enable the tracked git hooks (runs lint/format before each commit).
28+
hooks:
29+
git config core.hooksPath .githooks

cmd/uploadfun/main.go

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ func parseArgs(args []string, stdout, stderr io.Writer) (*cliOptions, int) {
6464
fs.PrintDefaults()
6565
}
6666

67-
if err := fs.Parse(args); err != nil {
67+
paths, err := parseInterleaved(fs, args)
68+
if err != nil {
6869
if errors.Is(err, flag.ErrHelp) {
6970
return nil, exitOK
7071
}
@@ -87,7 +88,7 @@ func parseArgs(args []string, stdout, stderr io.Writer) (*cliOptions, int) {
8788
return nil, exitUsageError
8889
}
8990

90-
opts.paths = fs.Args()
91+
opts.paths = paths
9192
if len(opts.paths) == 0 {
9293
_, _ = fmt.Fprintln(stderr, "uploadfun: at least one file or directory argument is required")
9394
fs.Usage()
@@ -97,6 +98,27 @@ func parseArgs(args []string, stdout, stderr io.Writer) (*cliOptions, int) {
9798
return opts, exitOK
9899
}
99100

101+
// parseInterleaved runs fs.Parse repeatedly so flags and positional
102+
// arguments may appear in any order — the documented invocation puts
103+
// paths before --config, but stdlib flag otherwise stops parsing at the
104+
// first positional. It returns the collected positionals, or the first
105+
// fs.Parse error (including flag.ErrHelp).
106+
func parseInterleaved(fs *flag.FlagSet, args []string) ([]string, error) {
107+
var positionals []string
108+
rest := args
109+
for {
110+
if err := fs.Parse(rest); err != nil {
111+
return nil, err
112+
}
113+
rest = fs.Args()
114+
if len(rest) == 0 {
115+
return positionals, nil
116+
}
117+
positionals = append(positionals, rest[0])
118+
rest = rest[1:]
119+
}
120+
}
121+
100122
// expandPaths turns the positional file/dir arguments into a flat file
101123
// list: directories expand non-recursively, every regular file included,
102124
// no extension filtering, subdirectories and hidden/dotfiles silently
@@ -131,9 +153,31 @@ func expandPaths(paths []string) ([]string, error) {
131153
files = append(files, filepath.Join(p, entry.Name()))
132154
}
133155
}
156+
if err := checkBasenameCollisions(files); err != nil {
157+
return nil, err
158+
}
134159
return files, nil
135160
}
136161

162+
// checkBasenameCollisions rejects inputs that would map to the same remote
163+
// filename. The remote name is a file's basename (see dispatch's
164+
// remoteName), so two inputs sharing a basename — e.g. a/img.jpg and
165+
// b/img.jpg, or the same path passed twice — would, under the default
166+
// delete-first overwrite, have one silently clobber the other. Catch it up
167+
// front rather than reporting success for a file that was overwritten.
168+
func checkBasenameCollisions(files []string) error {
169+
seen := make(map[string]string, len(files))
170+
for _, f := range files {
171+
base := filepath.Base(f)
172+
if prev, ok := seen[base]; ok {
173+
return fmt.Errorf(
174+
"inputs %q and %q both upload to remote name %q", prev, f, base)
175+
}
176+
seen[base] = f
177+
}
178+
return nil
179+
}
180+
137181
func run(ctx context.Context, args []string, stdout, stderr io.Writer) int {
138182
opts, code := parseArgs(args, stdout, stderr)
139183
if opts == nil {

cmd/uploadfun/main_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,30 @@ func TestParseArgsValid(t *testing.T) {
6363
}
6464
}
6565

66+
func TestParseArgsInterleavedPathsAndFlags(t *testing.T) {
67+
// The documented invocation puts paths before --config; flags and
68+
// positionals must be accepted in any order.
69+
cases := [][]string{
70+
{"a.jpg", "dir/", "--config", "x.yaml"},
71+
{"a.jpg", "--config", "x.yaml", "dir/"},
72+
{"--config", "x.yaml", "a.jpg", "dir/"},
73+
{"a.jpg", "--verbose", "dir/", "--config", "x.yaml"},
74+
}
75+
for _, args := range cases {
76+
var stdout, stderr bytes.Buffer
77+
opts, code := parseArgs(args, &stdout, &stderr)
78+
if opts == nil {
79+
t.Fatalf("args %v: expected valid opts (code=%d, stderr=%q)", args, code, stderr.String())
80+
}
81+
if opts.configPath != "x.yaml" {
82+
t.Errorf("args %v: expected config x.yaml, got %q", args, opts.configPath)
83+
}
84+
if len(opts.paths) != 2 || opts.paths[0] != "a.jpg" || opts.paths[1] != "dir/" {
85+
t.Errorf("args %v: unexpected paths %v", args, opts.paths)
86+
}
87+
}
88+
}
89+
6690
func TestParseArgsDryRunAndNoVerify(t *testing.T) {
6791
var stdout, stderr bytes.Buffer
6892
opts, code := parseArgs(
@@ -134,6 +158,20 @@ func TestExpandPaths(t *testing.T) {
134158
}
135159
}
136160

161+
func TestExpandPathsRejectsBasenameCollision(t *testing.T) {
162+
dirA, dirB := t.TempDir(), t.TempDir()
163+
writeFile(t, filepath.Join(dirA, "img.jpg"), "a")
164+
writeFile(t, filepath.Join(dirB, "img.jpg"), "b")
165+
166+
_, err := expandPaths([]string{filepath.Join(dirA, "img.jpg"), filepath.Join(dirB, "img.jpg")})
167+
if err == nil {
168+
t.Fatal("expected an error when two inputs share a remote basename")
169+
}
170+
if !strings.Contains(err.Error(), "img.jpg") {
171+
t.Errorf("expected the colliding name in the error, got %q", err.Error())
172+
}
173+
}
174+
137175
func TestExpandPathsNonexistent(t *testing.T) {
138176
if _, err := expandPaths([]string{"/nonexistent/path/xyz"}); err == nil {
139177
t.Fatal("expected an error for a nonexistent path")

dispatch.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,11 +88,22 @@ func runEndpoint(
8888
remoteName := filepath.Base(file)
8989
ok := false
9090
for attempt := 1; attempt <= ep.Attempts; attempt++ {
91+
// Stop retrying once canceled instead of burning the remaining
92+
// budget on connects that fail instantly against a dead ctx,
93+
// each emitting a misleading "connect" FileErrorEvent.
94+
if ctx.Err() != nil {
95+
break
96+
}
9197
if !connected {
9298
connectCtx, cancel := context.WithTimeout(ctx, ep.ConnectTimeout)
9399
connErr := up.Connect(connectCtx, ep)
94100
cancel()
95101
if connErr != nil {
102+
// A connect that failed only because ctx was canceled
103+
// isn't a real endpoint failure — don't report it.
104+
if ctx.Err() != nil {
105+
break
106+
}
96107
events <- FileErrorEvent{
97108
Endpoint: ep.Name, File: file, Attempt: attempt, Reason: "connect", Err: connErr,
98109
}

dispatch_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,18 @@ type fakeUploader struct {
3030
listResult []string
3131
listErr error
3232
listCalls int
33+
34+
// beforeUpload, if set, runs at the start of each Upload with the
35+
// 1-based call number — a hook for tests to cancel mid-transfer.
36+
beforeUpload func(call int)
3337
}
3438

3539
func (f *fakeUploader) Connect(ctx context.Context, ep Endpoint) error {
40+
// Real transports honor ctx at connect time; mirror that so tests can
41+
// exercise cancellation during the retry loop's reconnect.
42+
if ctx.Err() != nil {
43+
return ctx.Err()
44+
}
3645
f.mu.Lock()
3746
defer f.mu.Unlock()
3847
f.connectCalls++
@@ -66,6 +75,9 @@ func (f *fakeUploader) Upload(
6675
f.uploadCalls++
6776
calls := f.uploadCalls
6877
f.mu.Unlock()
78+
if f.beforeUpload != nil {
79+
f.beforeUpload(calls)
80+
}
6981
progress(50, 100)
7082
progress(100, 100)
7183
if calls <= f.failUploadN {
@@ -345,6 +357,46 @@ func TestDispatchContextCanceledBeforeStart(t *testing.T) {
345357
}
346358
}
347359

360+
func TestDispatchCancelDuringRetryEmitsNoSpuriousErrors(t *testing.T) {
361+
ctx, cancel := context.WithCancel(context.Background())
362+
363+
// Every upload fails, so each attempt disconnects and reconnects —
364+
// the retry path. Cancel while the first attempt is mid-upload.
365+
f := &fakeUploader{failUploadN: 1000}
366+
f.beforeUpload = func(call int) {
367+
if call == 1 {
368+
cancel()
369+
}
370+
}
371+
withFakeUploader(t, f)
372+
373+
ep := testEndpoint("ep1")
374+
ep.Attempts = 5
375+
events := collectEvents(Upload(ctx, []string{"a.jpg"}, []Endpoint{ep}, Options{NoVerify: true}))
376+
377+
counts := countByType(events)
378+
// Only the first attempt's genuine upload failure should be reported;
379+
// the remaining attempts must not run once canceled.
380+
if counts["error"] > 1 {
381+
t.Errorf("expected at most one error event after cancellation, got %d", counts["error"])
382+
}
383+
for _, e := range events {
384+
if fe, ok := e.(FileErrorEvent); ok && fe.Reason == "connect" {
385+
t.Errorf("unexpected spurious connect error after cancellation: %+v", fe)
386+
}
387+
}
388+
389+
var done EndpointDoneEvent
390+
for _, e := range events {
391+
if d, ok := e.(EndpointDoneEvent); ok {
392+
done = d
393+
}
394+
}
395+
if done.Succeeded != 0 || done.Failed != 1 {
396+
t.Errorf("expected the canceled file counted as failed, got %+v", done)
397+
}
398+
}
399+
348400
func TestDispatchDryRunSuccess(t *testing.T) {
349401
f := &fakeUploader{listResult: []string{"existing1.jpg", "existing2.jpg"}}
350402
withFakeUploader(t, f)

go.mod

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ module github.com/alexeyu/uploadfun
22

33
go 1.26
44

5-
require gopkg.in/yaml.v3 v3.0.1
5+
require (
6+
github.com/jlaffaye/ftp v0.2.1
7+
github.com/pkg/sftp v1.13.10
8+
golang.org/x/crypto v0.54.0
9+
gopkg.in/yaml.v3 v3.0.1
10+
)
611

712
require (
8-
github.com/jlaffaye/ftp v0.2.1 // indirect
913
github.com/kr/fs v0.1.0 // indirect
10-
github.com/pkg/sftp v1.13.10 // indirect
11-
golang.org/x/crypto v0.54.0 // indirect
1214
golang.org/x/sys v0.47.0 // indirect
1315
)

go.sum

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,21 @@
1+
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
2+
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
13
github.com/jlaffaye/ftp v0.2.1 h1:AICcTYPMkaXlmjLMm9I+lB36f6jXCsCvBqVQc6EfC1Y=
24
github.com/jlaffaye/ftp v0.2.1/go.mod h1:gXSIr1pA9NhynDNigiFHs4+yL7o7I6bGF9Za9wi9tcE=
35
github.com/kr/fs v0.1.0 h1:Jskdu9ieNAYnjxsi0LbQp1ulIKZV1LAFgK1tWhpZgl8=
46
github.com/kr/fs v0.1.0/go.mod h1:FFnZGqtBN9Gxj7eW1uZ42v5BccTP0vu6NEaFoC2HwRg=
57
github.com/pkg/sftp v1.13.10 h1:+5FbKNTe5Z9aspU88DPIKJ9z2KZoaGCu6Sr6kKR/5mU=
68
github.com/pkg/sftp v1.13.10/go.mod h1:bJ1a7uDhrX/4OII+agvy28lzRvQrmIQuaHrcI1HbeGA=
9+
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
10+
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
11+
github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
12+
github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
713
golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw=
814
golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk=
915
golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs=
1016
golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw=
17+
golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0=
18+
golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w=
1119
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
1220
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
1321
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

internal/transport/conn.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package transport
2+
3+
import (
4+
"net"
5+
"sync/atomic"
6+
"time"
7+
)
8+
9+
// stallGuard is shared by every connection opened for one endpoint dial
10+
// (the control connection and, for FTP, each data connection). It flips
11+
// once from connecting to established, which switches the guardedConns it
12+
// wraps from passive to idle-timeout enforcement.
13+
type stallGuard struct {
14+
stallTimeout time.Duration
15+
established atomic.Bool
16+
}
17+
18+
// wrap returns c wrapped so its reads and writes enforce the stall timeout
19+
// once the guard is established.
20+
func (g *stallGuard) wrap(c net.Conn) *guardedConn {
21+
return &guardedConn{Conn: c, guard: g}
22+
}
23+
24+
// markEstablished switches wrapped connections from connect-phase (passive,
25+
// so the dialer's connect deadline governs) to transfer-phase idle-timeout
26+
// enforcement. Called once the session is fully up.
27+
func (g *stallGuard) markEstablished() { g.established.Store(true) }
28+
29+
// guardedConn enforces an idle (stall) timeout: once established, every
30+
// Read/Write first pushes the deadline to now+stallTimeout, so a transfer
31+
// that makes no forward progress for that long fails instead of blocking
32+
// forever. Before the guard is established it leaves the deadline alone, so
33+
// whatever connect deadline the dialer set stays in force.
34+
type guardedConn struct {
35+
net.Conn
36+
guard *stallGuard
37+
}
38+
39+
func (c *guardedConn) Read(b []byte) (int, error) {
40+
c.arm(c.SetReadDeadline)
41+
return c.Conn.Read(b)
42+
}
43+
44+
func (c *guardedConn) Write(b []byte) (int, error) {
45+
c.arm(c.SetWriteDeadline)
46+
return c.Conn.Write(b)
47+
}
48+
49+
func (c *guardedConn) arm(setDeadline func(time.Time) error) {
50+
if !c.guard.established.Load() || c.guard.stallTimeout <= 0 {
51+
return
52+
}
53+
_ = setDeadline(time.Now().Add(c.guard.stallTimeout))
54+
}

0 commit comments

Comments
 (0)