Skip to content

fix: return error on non-ErrNotExist stat failures in Tar.Sync()#13684

Open
Lidang-Jiang wants to merge 1 commit intodocker:mainfrom
Lidang-Jiang:fix/13654-tar-sync-stat-error
Open

fix: return error on non-ErrNotExist stat failures in Tar.Sync()#13684
Lidang-Jiang wants to merge 1 commit intodocker:mainfrom
Lidang-Jiang:fix/13654-tar-sync-stat-error

Conversation

@Lidang-Jiang
Copy link
Copy Markdown

Summary

  • Fix silent misclassification of non-ErrNotExist stat errors in Tar.Sync() (internal/sync/tar.go)
  • Add unit tests for the three stat outcomes: exists, not-exists, and permission error

The previous two-way branch only checked for fs.ErrNotExist. Any other os.Stat error (e.g. EACCES, EIO) fell through to the else clause, silently treating the path as copyable. This masked real errors and caused failures downstream with the original cause lost.

The fix restructures the condition into a three-way branch, following the pattern already used by entriesForPath() in the same file.

Fixes #13654

Before / After

Before (buggy behavior)

Non-ErrNotExist stat errors are silently treated as "copy":

=== Old (buggy) logic ===
  /tmp/before_test_existing.txt → COPY (err=<nil>)
  /no/such/path → DELETE (not exist)
  /tmp/before_test_noaccess/secret.txt → COPY (err=stat /tmp/before_test_noaccess/secret.txt: permission denied)

The path with a permission error is incorrectly added to the copy list instead of surfacing the error.

After (fixed behavior)

Permission errors are now properly returned:

=== New (fixed) logic ===
  /tmp/before_test_existing.txt → COPY
  /no/such/path → DELETE
  /tmp/before_test_noaccess/secret.txt → ERROR: stat /tmp/before_test_noaccess/secret.txt: permission denied

Unit test output:

=== RUN   TestSync_ExistingPath
--- PASS: TestSync_ExistingPath (0.00s)
=== RUN   TestSync_NonExistentPath
--- PASS: TestSync_NonExistentPath (0.00s)
=== RUN   TestSync_StatPermissionError
--- PASS: TestSync_StatPermissionError (0.00s)
=== RUN   TestSync_MixedPaths
--- PASS: TestSync_MixedPaths (0.00s)
PASS
ok  	github.com/docker/compose/v5/internal/sync	0.005s

Lint:

$ golangci-lint run ./internal/sync/...
0 issues.

Test plan

  • TestSync_ExistingPath — existing host path is correctly added to copy list
  • TestSync_NonExistentPath — missing host path triggers rm -rf delete command
  • TestSync_StatPermissionErrorEACCES error is returned immediately (skipped on root/Windows)
  • TestSync_MixedPaths — mix of existing and missing paths handled correctly in one call
  • golangci-lint run ./internal/sync/... — 0 issues

Previously, Sync() only checked for fs.ErrNotExist when classifying
paths into copy vs delete. Non-NotExist stat errors (e.g. EACCES,
EIO) caused the condition to be false, falling through to the else
clause which incorrectly treated the path as copyable. This masked
real errors and led to cryptic failures downstream.

Restructure the condition into a three-way branch:
- err == nil → copy
- ErrNotExist → delete
- other errors → return immediately with context

This follows the pattern already used by entriesForPath() in the
same file.

Fixes docker#13654

Signed-off-by: Lidang Jiang <lidangjiang@gmail.com>
Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
@Lidang-Jiang Lidang-Jiang requested a review from a team as a code owner March 29, 2026 02:27
@Lidang-Jiang Lidang-Jiang requested review from glours and ndeloof March 29, 2026 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sync: non-ErrNotExist stat errors silently treated as "copy" in Tar.Sync()

1 participant