diff --git a/.github/workflows/test-migrations.yml b/.github/workflows/test-migrations.yml new file mode 100644 index 000000000..1def94ff7 --- /dev/null +++ b/.github/workflows/test-migrations.yml @@ -0,0 +1,85 @@ +name: Migrations + +on: + workflow_dispatch: + pull_request: + paths: + # Migration implementation files + - 'repo/fsrepo/migrations/**' + - 'test/cli/migrations/**' + # Config and repo handling + - 'repo/fsrepo/**' + # This workflow file itself + - '.github/workflows/test-migrations.yml' + push: + branches: + - 'master' + - 'release-*' + paths: + - 'repo/fsrepo/migrations/**' + - 'test/cli/migrations/**' + - 'repo/fsrepo/**' + - '.github/workflows/test-migrations.yml' + +concurrency: + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event_name == 'push' && github.sha || github.ref }} + cancel-in-progress: true + +jobs: + test: + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest, macos-latest] + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + env: + TEST_VERBOSE: 1 + IPFS_CHECK_RCMGR_DEFAULTS: 1 + defaults: + run: + shell: bash + steps: + - name: Check out Kubo + uses: actions/checkout@v5 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version-file: 'go.mod' + + - name: Build kubo binary + run: | + make build + echo "Built ipfs binary at $(pwd)/cmd/ipfs/" + + - name: Add kubo to PATH + run: | + echo "$(pwd)/cmd/ipfs" >> $GITHUB_PATH + + - name: Verify ipfs in PATH + run: | + which ipfs || echo "ipfs not in PATH" + ipfs version || echo "Failed to run ipfs version" + + - name: Run migration unit tests + run: | + go test ./repo/fsrepo/migrations/... + + - name: Run CLI migration tests + env: + IPFS_PATH: ${{ runner.temp }}/ipfs-test + run: | + export PATH="${{ github.workspace }}/cmd/ipfs:$PATH" + which ipfs || echo "ipfs not found in PATH" + ipfs version || echo "Failed to run ipfs version" + go test ./test/cli/migrations/... + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: ${{ matrix.os }}-test-results + path: | + test/**/*.log + ${{ runner.temp }}/ipfs-test/ diff --git a/bin/check_go_version b/bin/check_go_version deleted file mode 100755 index 74320010b..000000000 --- a/bin/check_go_version +++ /dev/null @@ -1,44 +0,0 @@ -#!/bin/sh -# -# Check that the go version is at least equal to a minimum version -# number. -# -# Call it for example like this: -# -# $ check_go_version "1.5.2" -# - -USAGE="$0 GO_MIN_VERSION" - -die() { - printf >&2 "fatal: %s\n" "$@" - exit 1 -} - -# Get arguments - -test "$#" -eq "1" || die "This program must be passed exactly 1 arguments" "Usage: $USAGE" - -GO_MIN_VERSION="$1" - -UPGRADE_MSG="Please take a look at https://golang.org/doc/install to install or upgrade go." - -# Get path to the directory containing this file -# If $0 has no slashes, uses "./" -PREFIX=$(expr "$0" : "\(.*\/\)") || PREFIX='./' -# Include the 'check_at_least_version' function -. ${PREFIX}check_version - -# Check that the go binary exists and is in the path - -GOCC=${GOCC="go"} - -type ${GOCC} >/dev/null 2>&1 || die_upgrade "go is not installed or not in the PATH!" - -# Check the go binary version - -VERS_STR=$(${GOCC} version 2>&1) || die "'go version' failed with output: $VERS_STR" - -GO_CUR_VERSION=$(expr "$VERS_STR" : ".*go version.* go\([^[:space:]]*\) .*") || die "Invalid 'go version' output: $VERS_STR" - -check_at_least_version "$GO_MIN_VERSION" "$GO_CUR_VERSION" "${GOCC}" diff --git a/bin/check_version b/bin/check_version deleted file mode 100755 index 25007002c..000000000 --- a/bin/check_version +++ /dev/null @@ -1,77 +0,0 @@ -#!/bin/sh - -if test "x$UPGRADE_MSG" = "x"; then - printf >&2 "fatal: Please set '"'$UPGRADE_MSG'"' before sourcing this script\n" - exit 1 -fi - -die_upgrade() { - printf >&2 "fatal: %s\n" "$@" - printf >&2 "=> %s\n" "$UPGRADE_MSG" - exit 1 -} - -major_number() { - vers="$1" - - # Hack around 'expr' exiting with code 1 when it outputs 0 - case "$vers" in - 0) echo "0" ;; - 0.*) echo "0" ;; - *) expr "$vers" : "\([^.]*\).*" || return 1 - esac -} - -check_at_least_version() { - MIN_VERS="$1" - CUR_VERS="$2" - PROG_NAME="$3" - - # Get major, minor and fix numbers for each version - MIN_MAJ=$(major_number "$MIN_VERS") || die "No major version number in '$MIN_VERS' for '$PROG_NAME'" - CUR_MAJ=$(major_number "$CUR_VERS") || die "No major version number in '$CUR_VERS' for '$PROG_NAME'" - - # We expect a version to be of form X.X.X - # if the second dot doesn't match, we consider it a prerelease - - if MIN_MIN=$(expr "$MIN_VERS" : "[^.]*\.\([0-9][0-9]*\)"); then - # this captured digit is necessary, since expr returns code 1 if the output is empty - if expr "$MIN_VERS" : "[^.]*\.[0-9]*\([0-9]\.\|[0-9]\$\)" >/dev/null; then - MIN_PRERELEASE="0" - else - MIN_PRERELEASE="1" - fi - MIN_FIX=$(expr "$MIN_VERS" : "[^.]*\.[0-9][0-9]*[^0-9][^0-9]*\([0-9][0-9]*\)") || MIN_FIX="0" - else - MIN_MIN="0" - MIN_PRERELEASE="0" - MIN_FIX="0" - fi - if CUR_MIN=$(expr "$CUR_VERS" : "[^.]*\.\([0-9][0-9]*\)"); then - # this captured digit is necessary, since expr returns code 1 if the output is empty - if expr "$CUR_VERS" : "[^.]*\.[0-9]*\([0-9]\.\|[0-9]\$\)" >/dev/null; then - CUR_PRERELEASE="0" - else - CUR_PRERELEASE="1" - fi - CUR_FIX=$(expr "$CUR_VERS" : "[^.]*\.[0-9][0-9]*[^0-9][^0-9]*\([0-9][0-9]*\)") || CUR_FIX="0" - else - CUR_MIN="0" - CUR_PRERELEASE="0" - CUR_FIX="0" - fi - - # Compare versions - VERS_LEAST="$PROG_NAME version '$CUR_VERS' should be at least '$MIN_VERS'" - test "$CUR_MAJ" -lt "$MIN_MAJ" && die_upgrade "$VERS_LEAST" - test "$CUR_MAJ" -gt "$MIN_MAJ" || { - test "$CUR_MIN" -lt "$MIN_MIN" && die_upgrade "$VERS_LEAST" - test "$CUR_MIN" -gt "$MIN_MIN" || { - test "$CUR_PRERELEASE" -gt "$MIN_PRERELEASE" && die_upgrade "$VERS_LEAST" - test "$CUR_PRERELEASE" -lt "$MIN_PRERELEASE" || { - test "$CUR_FIX" -lt "$MIN_FIX" && die_upgrade "$VERS_LEAST" - true - } - } - } -} diff --git a/cmd/ipfs/kubo/daemon.go b/cmd/ipfs/kubo/daemon.go index 7daa66ee7..fa89bf632 100644 --- a/cmd/ipfs/kubo/daemon.go +++ b/cmd/ipfs/kubo/daemon.go @@ -334,7 +334,8 @@ func daemonFunc(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment } // Use hybrid migration strategy that intelligently combines external and embedded migrations - err = migrations.RunHybridMigrations(cctx.Context(), version.RepoVersion, cctx.ConfigRoot, false) + // Use req.Context instead of cctx.Context() to avoid attempting repo open before migrations complete + err = migrations.RunHybridMigrations(req.Context, version.RepoVersion, cctx.ConfigRoot, false) if err != nil { fmt.Println("Repository migration failed:") fmt.Printf(" %s\n", err) @@ -387,7 +388,8 @@ func daemonFunc(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment log.Errorf("failed to create autoconf client: %v", err) } else { // Start primes cache and starts background updater - if _, err := client.Start(cctx.Context()); err != nil { + // Use req.Context for background updater lifecycle (node doesn't exist yet) + if _, err := client.Start(req.Context); err != nil { log.Errorf("failed to start autoconf updater: %v", err) } } diff --git a/core/commands/repo.go b/core/commands/repo.go index 017143127..622e92d7e 100644 --- a/core/commands/repo.go +++ b/core/commands/repo.go @@ -423,19 +423,12 @@ migration. Versions below 16 require external migration tools. return fmt.Errorf("downgrade from version %d to %d requires --allow-downgrade flag", currentVersion, targetVersion) } - // Check if repo is locked by daemon before running migration - locked, err := fsrepo.LockedByOtherProcess(cctx.ConfigRoot) - if err != nil { - return fmt.Errorf("could not check repo lock: %w", err) - } - if locked { - return fmt.Errorf("cannot run migration while daemon is running (repo.lock exists)") - } - fmt.Printf("Migrating repository from version %d to %d...\n", currentVersion, targetVersion) // Use hybrid migration strategy that intelligently combines external and embedded migrations - err = migrations.RunHybridMigrations(cctx.Context(), targetVersion, cctx.ConfigRoot, allowDowngrade) + // Use req.Context instead of cctx.Context() to avoid opening the repo before migrations run, + // which would acquire the lock that migrations need + err = migrations.RunHybridMigrations(req.Context, targetVersion, cctx.ConfigRoot, allowDowngrade) if err != nil { fmt.Println("Repository migration failed:") fmt.Printf(" %s\n", err) diff --git a/docs/changelogs/v0.38.md b/docs/changelogs/v0.38.md index 2edb31adf..a03424380 100644 --- a/docs/changelogs/v0.38.md +++ b/docs/changelogs/v0.38.md @@ -5,6 +5,7 @@ This release was brought to you by the [Shipyard](https://ipshipyard.com/) team. - [v0.38.0](#v0380) +- [v0.38.1](#v0381) ## v0.38.0 @@ -290,3 +291,20 @@ The new [`Internal.MFSNoFlushLimit`](https://github.com/ipfs/kubo/blob/master/do | Jakub Sztandera | 1 | +67/-15 | 3 | | Masih H. Derkani | 1 | +1/-2 | 2 | | Dominic Della Valle | 1 | +2/-1 | 1 | + +## v0.38.1 + +Fixes migration panic on Windows when upgrading from v0.37 to v0.38 ("panic: error can't be dealt with transactionally: Access is denied"). + +### Changelog + +
+Full Changelog + +
+ +### ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ Contributors + +| Contributor | Commits | Lines ยฑ | Files Changed | +|-------------|---------|---------|---------------| +| TBD | | | | diff --git a/mk/golang.mk b/mk/golang.mk index 4f4cd4fed..b50179a0a 100644 --- a/mk/golang.mk +++ b/mk/golang.mk @@ -1,5 +1,4 @@ # golang utilities -GO_MIN_VERSION = 1.25 export GO111MODULE=on @@ -74,11 +73,8 @@ test_go_lint: test/bin/golangci-lint test_go: $(TEST_GO) -check_go_version: - @$(GOCC) version - bin/check_go_version $(GO_MIN_VERSION) +# Version check is no longer needed - go.mod enforces minimum version .PHONY: check_go_version -DEPS_GO += check_go_version TEST += $(TEST_GO) TEST_SHORT += test_go_fmt test_go_short diff --git a/repo/fsrepo/migrations/atomicfile/atomicfile.go b/repo/fsrepo/migrations/atomicfile/atomicfile.go index 87704196d..209b8c368 100644 --- a/repo/fsrepo/migrations/atomicfile/atomicfile.go +++ b/repo/fsrepo/migrations/atomicfile/atomicfile.go @@ -1,6 +1,7 @@ package atomicfile import ( + "fmt" "io" "os" "path/filepath" @@ -34,23 +35,27 @@ func New(path string, mode os.FileMode) (*File, error) { // Close atomically replaces the target file with the temporary file func (f *File) Close() error { - if err := f.File.Close(); err != nil { - os.Remove(f.File.Name()) - return err + closeErr := f.File.Close() + if closeErr != nil { + // Try to cleanup temp file, but prioritize close error + _ = os.Remove(f.File.Name()) + return closeErr } - - if err := os.Rename(f.File.Name(), f.path); err != nil { - os.Remove(f.File.Name()) - return err - } - - return nil + return os.Rename(f.File.Name(), f.path) } // Abort removes the temporary file without replacing the target func (f *File) Abort() error { - f.File.Close() - return os.Remove(f.File.Name()) + closeErr := f.File.Close() + removeErr := os.Remove(f.File.Name()) + + if closeErr != nil && removeErr != nil { + return fmt.Errorf("abort failed: close: %w, remove: %v", closeErr, removeErr) + } + if closeErr != nil { + return closeErr + } + return removeErr } // ReadFrom reads from the given reader into the atomic file diff --git a/repo/fsrepo/migrations/atomicfile/atomicfile_test.go b/repo/fsrepo/migrations/atomicfile/atomicfile_test.go new file mode 100644 index 000000000..668045d12 --- /dev/null +++ b/repo/fsrepo/migrations/atomicfile/atomicfile_test.go @@ -0,0 +1,208 @@ +package atomicfile + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestNew_Success verifies atomic file creation +func TestNew_Success(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + defer func() { _ = af.Abort() }() + + // Verify temp file exists + assert.FileExists(t, af.File.Name()) + + // Verify temp file is in same directory + assert.Equal(t, dir, filepath.Dir(af.File.Name())) +} + +// TestClose_Success verifies atomic replacement +func TestClose_Success(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + + content := []byte("test content") + _, err = af.Write(content) + require.NoError(t, err) + + tempName := af.File.Name() + + require.NoError(t, af.Close()) + + // Verify target file exists with correct content + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, content, data) + + // Verify temp file removed + assert.NoFileExists(t, tempName) +} + +// TestAbort_Success verifies cleanup +func TestAbort_Success(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + + tempName := af.File.Name() + + require.NoError(t, af.Abort()) + + // Verify temp file removed + assert.NoFileExists(t, tempName) + + // Verify target not created + assert.NoFileExists(t, path) +} + +// TestAbort_ErrorHandling tests error capture +func TestAbort_ErrorHandling(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + + // Close file to force close error + af.File.Close() + + // Remove temp file to force remove error + os.Remove(af.File.Name()) + + err = af.Abort() + // Should get both errors + require.Error(t, err) + assert.Contains(t, err.Error(), "abort failed") +} + +// TestClose_CloseError verifies cleanup on close failure +func TestClose_CloseError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + + tempName := af.File.Name() + + // Close file to force close error + af.File.Close() + + err = af.Close() + require.Error(t, err) + + // Verify temp file cleaned up even on error + assert.NoFileExists(t, tempName) +} + +// TestReadFrom verifies io.Copy integration +func TestReadFrom(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + defer func() { _ = af.Abort() }() + + content := []byte("test content from reader") + n, err := af.ReadFrom(bytes.NewReader(content)) + require.NoError(t, err) + assert.Equal(t, int64(len(content)), n) +} + +// TestFilePermissions verifies mode is set correctly +func TestFilePermissions(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0600) + require.NoError(t, err) + + _, err = af.Write([]byte("test")) + require.NoError(t, err) + + require.NoError(t, af.Close()) + + info, err := os.Stat(path) + require.NoError(t, err) + + // On Unix, check exact permissions + if runtime.GOOS != "windows" { + mode := info.Mode().Perm() + assert.Equal(t, os.FileMode(0600), mode) + } +} + +// TestMultipleAbortsSafe verifies calling Abort multiple times is safe +func TestMultipleAbortsSafe(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.txt") + + af, err := New(path, 0644) + require.NoError(t, err) + + tempName := af.File.Name() + + // First abort should succeed + require.NoError(t, af.Abort()) + assert.NoFileExists(t, tempName, "temp file should be removed after first abort") + + // Second abort should handle gracefully (file already gone) + err = af.Abort() + // Error is acceptable since file is already removed, but it should not panic + t.Logf("Second Abort() returned: %v", err) +} + +// TestNoTempFilesAfterOperations verifies no .tmp-* files remain after operations +func TestNoTempFilesAfterOperations(t *testing.T) { + const testIterations = 5 + + tests := []struct { + name string + operation func(*File) error + }{ + {"close", (*File).Close}, + {"abort", (*File).Abort}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + + // Perform multiple operations + for i := 0; i < testIterations; i++ { + path := filepath.Join(dir, fmt.Sprintf("test%d.txt", i)) + + af, err := New(path, 0644) + require.NoError(t, err) + + _, err = af.Write([]byte("test data")) + require.NoError(t, err) + + require.NoError(t, tt.operation(af)) + } + + // Check for any .tmp-* files + tmpFiles, err := filepath.Glob(filepath.Join(dir, ".tmp-*")) + require.NoError(t, err) + assert.Empty(t, tmpFiles, "should be no temp files after %s", tt.name) + }) + } +} diff --git a/repo/fsrepo/migrations/common/utils.go b/repo/fsrepo/migrations/common/utils.go index 217da609f..e7d704dad 100644 --- a/repo/fsrepo/migrations/common/utils.go +++ b/repo/fsrepo/migrations/common/utils.go @@ -1,6 +1,7 @@ package common import ( + "bytes" "encoding/json" "fmt" "io" @@ -40,47 +41,51 @@ func Must(err error) { // WithBackup performs a config file operation with automatic backup and rollback on error func WithBackup(configPath string, backupSuffix string, fn func(in io.ReadSeeker, out io.Writer) error) error { - in, err := os.Open(configPath) + // Read the entire file into memory first + // This allows us to close the file before doing atomic operations, + // which is necessary on Windows where open files can't be renamed + data, err := os.ReadFile(configPath) if err != nil { - return err + return fmt.Errorf("failed to read config file %s: %w", configPath, err) } - defer in.Close() - // Create backup - backup, err := atomicfile.New(configPath+backupSuffix, 0600) + // Create an in-memory reader for the data + in := bytes.NewReader(data) + + // Create backup atomically to prevent partial backup on interruption + backupPath := configPath + backupSuffix + backup, err := atomicfile.New(backupPath, 0600) if err != nil { - return err + return fmt.Errorf("failed to create backup file for %s: %w", backupPath, err) } - - // Copy to backup - if _, err := backup.ReadFrom(in); err != nil { + if _, err := backup.Write(data); err != nil { Must(backup.Abort()) - return err + return fmt.Errorf("failed to write backup data: %w", err) } - - // Reset input for reading - if _, err := in.Seek(0, io.SeekStart); err != nil { + if err := backup.Close(); err != nil { Must(backup.Abort()) - return err + return fmt.Errorf("failed to finalize backup: %w", err) } - // Create output file + // Create output file atomically out, err := atomicfile.New(configPath, 0600) if err != nil { - Must(backup.Abort()) - return err + // Clean up backup on error + os.Remove(backupPath) + return fmt.Errorf("failed to create atomic file for %s: %w", configPath, err) } // Run the conversion function if err := fn(in, out); err != nil { Must(out.Abort()) - Must(backup.Abort()) - return err + // Clean up backup on error + os.Remove(backupPath) + return fmt.Errorf("config conversion failed: %w", err) } - // Close everything on success + // Close the output file atomically Must(out.Close()) - Must(backup.Close()) + // Backup remains for potential revert return nil } diff --git a/repo/fsrepo/migrations/embedded.go b/repo/fsrepo/migrations/embedded.go index a2aa4d252..a8218be63 100644 --- a/repo/fsrepo/migrations/embedded.go +++ b/repo/fsrepo/migrations/embedded.go @@ -6,6 +6,7 @@ import ( "log" "os" + lockfile "github.com/ipfs/go-fs-lock" "github.com/ipfs/kubo/repo/fsrepo/migrations/common" mg16 "github.com/ipfs/kubo/repo/fsrepo/migrations/fs-repo-16-to-17/migration" mg17 "github.com/ipfs/kubo/repo/fsrepo/migrations/fs-repo-17-to-18/migration" @@ -109,6 +110,13 @@ func RunEmbeddedMigrations(ctx context.Context, targetVer int, ipfsDir string, a return err } + // Acquire lock once for all embedded migrations to prevent concurrent access + lk, err := lockfile.Lock(ipfsDir, "repo.lock") + if err != nil { + return fmt.Errorf("failed to acquire repo lock: %w", err) + } + defer lk.Close() + fromVer, err := RepoVersion(ipfsDir) if err != nil { return fmt.Errorf("could not get repo version: %w", err) diff --git a/repo/fsrepo/migrations/ipfsdir_test.go b/repo/fsrepo/migrations/ipfsdir_test.go index c94ebc586..c18721bae 100644 --- a/repo/fsrepo/migrations/ipfsdir_test.go +++ b/repo/fsrepo/migrations/ipfsdir_test.go @@ -11,6 +11,8 @@ import ( func TestRepoDir(t *testing.T) { fakeHome := t.TempDir() t.Setenv("HOME", fakeHome) + // On Windows, os.UserHomeDir() uses USERPROFILE, not HOME + t.Setenv("USERPROFILE", fakeHome) fakeIpfs := filepath.Join(fakeHome, ".ipfs") t.Setenv(config.EnvDir, fakeIpfs) diff --git a/test/cli/migrations/migration_16_to_latest_test.go b/test/cli/migrations/migration_16_to_latest_test.go index 521b31646..97a1ec1ff 100644 --- a/test/cli/migrations/migration_16_to_latest_test.go +++ b/test/cli/migrations/migration_16_to_latest_test.go @@ -21,6 +21,7 @@ import ( ipfs "github.com/ipfs/kubo" "github.com/ipfs/kubo/test/cli/harness" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -52,6 +53,13 @@ func TestMigration16ToLatest(t *testing.T) { // Comparison tests using 'ipfs repo migrate' command t.Run("repo migrate: forward migration with auto values", testRepoMigrationWithAuto) t.Run("repo migrate: backward migration", testRepoBackwardMigration) + + // Temp file and backup cleanup tests + t.Run("daemon migrate: no temp files after successful migration", testNoTempFilesAfterSuccessfulMigration) + t.Run("daemon migrate: no temp files after failed migration", testNoTempFilesAfterFailedMigration) + t.Run("daemon migrate: backup files persist after successful migration", testBackupFilesPersistAfterSuccessfulMigration) + t.Run("repo migrate: backup files can revert migration", testBackupFilesCanRevertMigration) + t.Run("repo migrate: conversion failure cleans up temp files", testConversionFailureCleanup) } // ============================================================================= @@ -392,10 +400,15 @@ func setupStaticV16Repo(t *testing.T) *harness.Node { v16FixturePath := "testdata/v16-repo" // Create a temporary test directory - each test gets its own copy - // Use ./tmp.DELETEME/ as requested by user instead of /tmp/ - tmpDir := filepath.Join("tmp.DELETEME", "migration-test-"+t.Name()) + // Sanitize test name for Windows - replace invalid characters + sanitizedName := strings.Map(func(r rune) rune { + if strings.ContainsRune(`<>:"/\|?*`, r) { + return '_' + } + return r + }, t.Name()) + tmpDir := filepath.Join(t.TempDir(), "migration-test-"+sanitizedName) require.NoError(t, os.MkdirAll(tmpDir, 0755)) - t.Cleanup(func() { os.RemoveAll(tmpDir) }) // Convert to absolute path for harness absTmpDir, err := filepath.Abs(tmpDir) @@ -559,6 +572,8 @@ func testRepoBackwardMigration(t *testing.T) { // First run forward migration to get to v17 result := node.RunIPFS("repo", "migrate") + t.Logf("Forward migration stdout:\n%s", result.Stdout.String()) + t.Logf("Forward migration stderr:\n%s", result.Stderr.String()) require.Empty(t, result.Stderr.String(), "Forward migration should succeed") // Verify we're at the latest version @@ -569,6 +584,8 @@ func testRepoBackwardMigration(t *testing.T) { // Now run reverse migration back to v16 result = node.RunIPFS("repo", "migrate", "--to=16", "--allow-downgrade") + t.Logf("Backward migration stdout:\n%s", result.Stdout.String()) + t.Logf("Backward migration stderr:\n%s", result.Stderr.String()) require.Empty(t, result.Stderr.String(), "Reverse migration should succeed") // Verify version was downgraded to 16 @@ -753,3 +770,149 @@ func runDaemonWithMultipleMigrationMonitoring(t *testing.T, node *harness.Node, } } } + +// ============================================================================= +// TEMP FILE AND BACKUP CLEANUP TESTS +// ============================================================================= + +// Helper functions for test cleanup assertions +func assertNoTempFiles(t *testing.T, dir string, msgAndArgs ...interface{}) { + t.Helper() + tmpFiles, err := filepath.Glob(filepath.Join(dir, ".tmp-*")) + require.NoError(t, err) + assert.Empty(t, tmpFiles, msgAndArgs...) +} + +func backupPath(configPath string, fromVer, toVer int) string { + return fmt.Sprintf("%s.%d-to-%d.bak", configPath, fromVer, toVer) +} + +func setupDaemonCmd(ctx context.Context, node *harness.Node, args ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, node.IPFSBin, args...) + cmd.Dir = node.Dir + for k, v := range node.Runner.Env { + cmd.Env = append(cmd.Env, k+"="+v) + } + return cmd +} + +func testNoTempFilesAfterSuccessfulMigration(t *testing.T) { + node := setupStaticV16Repo(t) + + // Run successful migration + _, migrationSuccess := runDaemonMigrationWithMonitoring(t, node) + require.True(t, migrationSuccess, "migration should succeed") + + assertNoTempFiles(t, node.Dir, "no temp files should remain after successful migration") +} + +func testNoTempFilesAfterFailedMigration(t *testing.T) { + node := setupStaticV16Repo(t) + + // Corrupt config to force migration failure + configPath := filepath.Join(node.Dir, "config") + corruptedJson := `{"Bootstrap": ["auto",` // Invalid JSON + require.NoError(t, os.WriteFile(configPath, []byte(corruptedJson), 0644)) + + // Attempt migration (should fail) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + cmd := setupDaemonCmd(ctx, node, "daemon", "--migrate") + output, _ := cmd.CombinedOutput() + t.Logf("Failed migration output: %s", output) + + assertNoTempFiles(t, node.Dir, "no temp files should remain after failed migration") +} + +func testBackupFilesPersistAfterSuccessfulMigration(t *testing.T) { + node := setupStaticV16Repo(t) + + // Run migration from v16 to latest (v18) + _, migrationSuccess := runDaemonMigrationWithMonitoring(t, node) + require.True(t, migrationSuccess, "migration should succeed") + + // Check for backup files from each migration step + configPath := filepath.Join(node.Dir, "config") + backup16to17 := backupPath(configPath, 16, 17) + backup17to18 := backupPath(configPath, 17, 18) + + // Both backup files should exist + assert.FileExists(t, backup16to17, "16-to-17 backup should exist") + assert.FileExists(t, backup17to18, "17-to-18 backup should exist") + + // Verify backup files contain valid JSON + data16to17, err := os.ReadFile(backup16to17) + require.NoError(t, err) + var config16to17 map[string]interface{} + require.NoError(t, json.Unmarshal(data16to17, &config16to17), "16-to-17 backup should be valid JSON") + + data17to18, err := os.ReadFile(backup17to18) + require.NoError(t, err) + var config17to18 map[string]interface{} + require.NoError(t, json.Unmarshal(data17to18, &config17to18), "17-to-18 backup should be valid JSON") +} + +func testBackupFilesCanRevertMigration(t *testing.T) { + node := setupStaticV16Repo(t) + + configPath := filepath.Join(node.Dir, "config") + versionPath := filepath.Join(node.Dir, "version") + + // Read original v16 config + originalConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + + // Migrate to v17 only + result := node.RunIPFS("repo", "migrate", "--to=17") + require.Empty(t, result.Stderr.String(), "migration to v17 should succeed") + + // Verify backup exists + backup16to17 := backupPath(configPath, 16, 17) + assert.FileExists(t, backup16to17, "16-to-17 backup should exist") + + // Manually revert using backup + backupData, err := os.ReadFile(backup16to17) + require.NoError(t, err) + require.NoError(t, os.WriteFile(configPath, backupData, 0600)) + require.NoError(t, os.WriteFile(versionPath, []byte("16"), 0644)) + + // Verify config matches original + revertedConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + assert.JSONEq(t, string(originalConfig), string(revertedConfig), "reverted config should match original") + + // Verify version is back to 16 + versionData, err := os.ReadFile(versionPath) + require.NoError(t, err) + assert.Equal(t, "16", strings.TrimSpace(string(versionData)), "version should be reverted to 16") +} + +func testConversionFailureCleanup(t *testing.T) { + // This test verifies that when a migration's conversion function fails, + // all temporary files are cleaned up properly + node := setupStaticV16Repo(t) + + configPath := filepath.Join(node.Dir, "config") + + // Create a corrupted config that will cause conversion to fail during JSON parsing + // The migration will read this, attempt to parse as JSON, and fail + corruptedJson := `{"Bootstrap": ["auto",` // Invalid JSON - missing closing bracket + require.NoError(t, os.WriteFile(configPath, []byte(corruptedJson), 0644)) + + // Attempt migration (should fail during conversion) + result := node.RunIPFS("repo", "migrate") + require.NotEmpty(t, result.Stderr.String(), "migration should fail with error") + + assertNoTempFiles(t, node.Dir, "no temp files should remain after conversion failure") + + // Verify no backup files were created (failure happened before backup) + backupFiles, err := filepath.Glob(filepath.Join(node.Dir, "config.*.bak")) + require.NoError(t, err) + assert.Empty(t, backupFiles, "no backup files should be created on conversion failure") + + // Verify corrupted config is unchanged (atomic operations prevented overwrite) + currentConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + assert.Equal(t, corruptedJson, string(currentConfig), "corrupted config should remain unchanged") +} diff --git a/test/cli/migrations/migration_concurrent_test.go b/test/cli/migrations/migration_concurrent_test.go new file mode 100644 index 000000000..8c716f51c --- /dev/null +++ b/test/cli/migrations/migration_concurrent_test.go @@ -0,0 +1,55 @@ +package migrations + +// NOTE: These concurrent migration tests require the local Kubo binary (built with 'make build') to be in PATH. +// +// To run these tests successfully: +// export PATH="$(pwd)/cmd/ipfs:$PATH" +// go test ./test/cli/migrations/ + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const daemonStartupWait = 2 * time.Second + +// TestConcurrentMigrations tests concurrent daemon --migrate attempts +func TestConcurrentMigrations(t *testing.T) { + t.Parallel() + + t.Run("concurrent daemon migrations prevented by lock", testConcurrentDaemonMigrations) +} + +func testConcurrentDaemonMigrations(t *testing.T) { + node := setupStaticV16Repo(t) + + // Start first daemon --migrate in background (holds repo.lock) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + firstDaemon := setupDaemonCmd(ctx, node, "daemon", "--migrate") + require.NoError(t, firstDaemon.Start()) + defer func() { + // Shutdown first daemon + shutdownCmd := setupDaemonCmd(context.Background(), node, "shutdown") + _ = shutdownCmd.Run() + _ = firstDaemon.Wait() + }() + + // Wait for first daemon to start and acquire lock + time.Sleep(daemonStartupWait) + + // Attempt second daemon --migrate (should fail due to lock) + secondDaemon := setupDaemonCmd(context.Background(), node, "daemon", "--migrate") + output, err := secondDaemon.CombinedOutput() + t.Logf("Second daemon output: %s", output) + + // Should fail with lock error + require.Error(t, err, "second daemon should fail when first daemon holds lock") + require.Contains(t, string(output), "lock", "error should mention lock") + + assertNoTempFiles(t, node.Dir, "no temp files should be created when lock fails") +} diff --git a/test/cli/migrations/migration_mixed_15_to_latest_test.go b/test/cli/migrations/migration_mixed_15_to_latest_test.go index 9f1a482f8..6ee96b939 100644 --- a/test/cli/migrations/migration_mixed_15_to_latest_test.go +++ b/test/cli/migrations/migration_mixed_15_to_latest_test.go @@ -23,8 +23,9 @@ import ( "os" "os/exec" "path/filepath" + "runtime" + "slices" "strings" - "syscall" "testing" "time" @@ -61,7 +62,8 @@ func testDaemonMigration15ToLatest(t *testing.T) { node := setupStaticV15Repo(t) // Create mock migration binary for 15โ†’16 (16โ†’17 will use embedded migration) - createMockMigrationBinary(t, "15", "16") + mockBinDir := createMockMigrationBinary(t, "15", "16") + customPath := buildCustomPath(mockBinDir) configPath := filepath.Join(node.Dir, "config") versionPath := filepath.Join(node.Dir, "version") @@ -80,7 +82,7 @@ func testDaemonMigration15ToLatest(t *testing.T) { originalPeerID := getNestedValue(originalConfig, "Identity.PeerID") // Run dual migration using daemon --migrate - stdoutOutput, migrationSuccess := runDaemonWithLegacyMigrationMonitoring(t, node) + stdoutOutput, migrationSuccess := runDaemonWithLegacyMigrationMonitoring(t, node, customPath) // Debug output t.Logf("Daemon output:\n%s", stdoutOutput) @@ -124,7 +126,8 @@ func testRepoMigration15ToLatest(t *testing.T) { node := setupStaticV15Repo(t) // Create mock migration binary for 15โ†’16 (16โ†’17 will use embedded migration) - createMockMigrationBinary(t, "15", "16") + mockBinDir := createMockMigrationBinary(t, "15", "16") + customPath := buildCustomPath(mockBinDir) configPath := filepath.Join(node.Dir, "config") versionPath := filepath.Join(node.Dir, "version") @@ -135,16 +138,7 @@ func testRepoMigration15ToLatest(t *testing.T) { require.Equal(t, "15", strings.TrimSpace(string(versionData)), "Should start at version 15") // Run migration using 'ipfs repo migrate' with custom PATH - result := node.Runner.Run(harness.RunRequest{ - Path: node.IPFSBin, - Args: []string{"repo", "migrate"}, - CmdOpts: []harness.CmdOpt{ - func(cmd *exec.Cmd) { - // Ensure the command inherits our modified PATH with mock binaries - cmd.Env = append(cmd.Env, "PATH="+os.Getenv("PATH")) - }, - }, - }) + result := runMigrationWithCustomPath(node, customPath, "repo", "migrate") require.Empty(t, result.Stderr.String(), "Migration should succeed without errors") // Verify final version is latest @@ -184,10 +178,10 @@ func setupStaticV15Repo(t *testing.T) *harness.Node { } // runDaemonWithLegacyMigrationMonitoring monitors for hybrid migration patterns -func runDaemonWithLegacyMigrationMonitoring(t *testing.T, node *harness.Node) (string, bool) { +func runDaemonWithLegacyMigrationMonitoring(t *testing.T, node *harness.Node, customPath string) (string, bool) { // Monitor for hybrid migration completion - use "Hybrid migration completed successfully" as success pattern stdoutOutput, daemonStarted := runDaemonWithMigrationMonitoringCustomEnv(t, node, "Using hybrid migration strategy", "Hybrid migration completed successfully", map[string]string{ - "PATH": os.Getenv("PATH"), // Pass current PATH which includes our mock binaries + "PATH": customPath, // Pass custom PATH with our mock binaries }) // Check for hybrid migration patterns in output @@ -271,17 +265,59 @@ func runDaemonWithMigrationMonitoringCustomEnv(t *testing.T, node *harness.Node, t.Log("Daemon startup timed out") } - // Stop the daemon + // Stop the daemon using ipfs shutdown command for graceful shutdown if cmd.Process != nil { - _ = cmd.Process.Signal(syscall.SIGTERM) + shutdownCmd := exec.Command(node.IPFSBin, "shutdown") + shutdownCmd.Dir = node.Dir + for k, v := range node.Runner.Env { + shutdownCmd.Env = append(shutdownCmd.Env, k+"="+v) + } + + if err := shutdownCmd.Run(); err != nil { + // If graceful shutdown fails, force kill + _ = cmd.Process.Kill() + } + + // Wait for process to exit _ = cmd.Wait() } return outputBuffer.String(), daemonReady && migrationStarted && migrationCompleted } -// createMockMigrationBinary creates a platform-agnostic Go binary for migration on PATH -func createMockMigrationBinary(t *testing.T, fromVer, toVer string) { +// buildCustomPath creates a custom PATH with mock migration binaries prepended. +// This is necessary for test isolation when running tests in parallel with t.Parallel(). +// Without isolated PATH handling, parallel tests can interfere with each other through +// global PATH modifications, causing tests to download real migration binaries instead +// of using the test mocks. +func buildCustomPath(mockBinDirs ...string) string { + // Prepend mock directories to ensure they're found first + pathElements := append(mockBinDirs, os.Getenv("PATH")) + return strings.Join(pathElements, string(filepath.ListSeparator)) +} + +// runMigrationWithCustomPath runs a migration command with a custom PATH environment. +// This ensures the migration uses our mock binaries instead of downloading real ones. +func runMigrationWithCustomPath(node *harness.Node, customPath string, args ...string) *harness.RunResult { + return node.Runner.Run(harness.RunRequest{ + Path: node.IPFSBin, + Args: args, + CmdOpts: []harness.CmdOpt{ + func(cmd *exec.Cmd) { + // Remove existing PATH entries using slices.DeleteFunc + cmd.Env = slices.DeleteFunc(cmd.Env, func(s string) bool { + return strings.HasPrefix(s, "PATH=") + }) + // Add custom PATH + cmd.Env = append(cmd.Env, "PATH="+customPath) + }, + }, + }) +} + +// createMockMigrationBinary creates a platform-agnostic Go binary for migration testing. +// Returns the directory containing the binary to be added to PATH. +func createMockMigrationBinary(t *testing.T, fromVer, toVer string) string { // Create bin directory for migration binaries binDir := t.TempDir() @@ -289,73 +325,60 @@ func createMockMigrationBinary(t *testing.T, fromVer, toVer string) { scriptName := fmt.Sprintf("fs-repo-%s-to-%s", fromVer, toVer) sourceFile := filepath.Join(binDir, scriptName+".go") binaryPath := filepath.Join(binDir, scriptName) + if runtime.GOOS == "windows" { + binaryPath += ".exe" + } + // Generate minimal mock migration binary code goSource := fmt.Sprintf(`package main - -import ( - "fmt" - "os" - "path/filepath" - "strings" -) - +import ("fmt"; "os"; "path/filepath"; "strings"; "time") func main() { - // Parse command line arguments - real migration binaries expect -path= - var repoPath string + var path string var revert bool - for _, arg := range os.Args[1:] { - if strings.HasPrefix(arg, "-path=") { - repoPath = strings.TrimPrefix(arg, "-path=") - } else if arg == "-revert" { - revert = true - } + for _, a := range os.Args[1:] { + if strings.HasPrefix(a, "-path=") { path = a[6:] } + if a == "-revert" { revert = true } } - - if repoPath == "" { - fmt.Fprintf(os.Stderr, "Usage: %%s -path= [-verbose=true] [-revert]\n", os.Args[0]) + if path == "" { fmt.Fprintln(os.Stderr, "missing -path="); os.Exit(1) } + + from, to := "%s", "%s" + if revert { from, to = to, from } + fmt.Printf("fake applying %%s-to-%%s repo migration\n", from, to) + + // Create and immediately remove lock file to simulate proper locking behavior + lockPath := filepath.Join(path, "repo.lock") + lockFile, err := os.Create(lockPath) + if err != nil && !os.IsExist(err) { + fmt.Fprintf(os.Stderr, "Error creating lock: %%v\n", err) os.Exit(1) } - - // Determine source and target versions based on revert flag - var sourceVer, targetVer string - if revert { - // When reverting, we go backwards: fs-repo-15-to-16 with -revert goes 16โ†’15 - sourceVer = "%s" - targetVer = "%s" - } else { - // Normal forward migration: fs-repo-15-to-16 goes 15โ†’16 - sourceVer = "%s" - targetVer = "%s" + if lockFile != nil { + lockFile.Close() + defer os.Remove(lockPath) } - - // Print migration message (same format as real migrations) - fmt.Printf("fake applying %%s-to-%%s repo migration\n", sourceVer, targetVer) - - // Update version file - versionFile := filepath.Join(repoPath, "version") - err := os.WriteFile(versionFile, []byte(targetVer), 0644) - if err != nil { - fmt.Fprintf(os.Stderr, "Error updating version: %%v\n", err) + + // Small delay to simulate migration work + time.Sleep(10 * time.Millisecond) + + if err := os.WriteFile(filepath.Join(path, "version"), []byte(to), 0644); err != nil { + fmt.Fprintf(os.Stderr, "Error: %%v\n", err) os.Exit(1) } -} -`, toVer, fromVer, fromVer, toVer) +}`, fromVer, toVer) require.NoError(t, os.WriteFile(sourceFile, []byte(goSource), 0644)) // Compile the Go binary - require.NoError(t, os.Setenv("CGO_ENABLED", "0")) // Ensure static binary - require.NoError(t, exec.Command("go", "build", "-o", binaryPath, sourceFile).Run()) - - // Add bin directory to PATH for this test - currentPath := os.Getenv("PATH") - newPath := binDir + string(filepath.ListSeparator) + currentPath - require.NoError(t, os.Setenv("PATH", newPath)) - t.Cleanup(func() { os.Setenv("PATH", currentPath) }) + cmd := exec.Command("go", "build", "-o", binaryPath, sourceFile) + cmd.Env = append(os.Environ(), "CGO_ENABLED=0") // Ensure static binary + require.NoError(t, cmd.Run()) // Verify the binary exists and is executable _, err := os.Stat(binaryPath) require.NoError(t, err, "Mock binary should exist") + + // Return the bin directory to be added to PATH + return binDir } // expectedMigrationSteps generates the expected migration step strings for a version range. @@ -416,26 +439,19 @@ func testRepoReverseHybridMigrationLatestTo15(t *testing.T) { // Start with v15 fixture and migrate forward to latest to create proper backup files node := setupStaticV15Repo(t) - // Create mock migration binary for 15โ†’16 (needed for forward migration) - createMockMigrationBinary(t, "15", "16") - // Create mock migration binary for 16โ†’15 (needed for downgrade) - createMockMigrationBinary(t, "16", "15") + // Create mock migration binaries for both forward and reverse migrations + mockBinDirs := []string{ + createMockMigrationBinary(t, "15", "16"), // for forward migration + createMockMigrationBinary(t, "16", "15"), // for downgrade + } + customPath := buildCustomPath(mockBinDirs...) configPath := filepath.Join(node.Dir, "config") versionPath := filepath.Join(node.Dir, "version") // Step 1: Forward migration from v15 to latest to create backup files t.Logf("Step 1: Forward migration v15 โ†’ v%d", ipfs.RepoVersion) - result := node.Runner.Run(harness.RunRequest{ - Path: node.IPFSBin, - Args: []string{"repo", "migrate"}, - CmdOpts: []harness.CmdOpt{ - func(cmd *exec.Cmd) { - // Ensure the command inherits our modified PATH with mock binaries - cmd.Env = append(cmd.Env, "PATH="+os.Getenv("PATH")) - }, - }, - }) + result := runMigrationWithCustomPath(node, customPath, "repo", "migrate") // Debug: print the output to see what happened t.Logf("Forward migration stdout:\n%s", result.Stdout.String()) @@ -459,16 +475,7 @@ func testRepoReverseHybridMigrationLatestTo15(t *testing.T) { // Step 2: Reverse hybrid migration from latest to v15 t.Logf("Step 2: Reverse hybrid migration v%d โ†’ v15", ipfs.RepoVersion) - result = node.Runner.Run(harness.RunRequest{ - Path: node.IPFSBin, - Args: []string{"repo", "migrate", "--to=15", "--allow-downgrade"}, - CmdOpts: []harness.CmdOpt{ - func(cmd *exec.Cmd) { - // Ensure the command inherits our modified PATH with mock binaries - cmd.Env = append(cmd.Env, "PATH="+os.Getenv("PATH")) - }, - }, - }) + result = runMigrationWithCustomPath(node, customPath, "repo", "migrate", "--to=15", "--allow-downgrade") require.Empty(t, result.Stderr.String(), "Reverse hybrid migration should succeed without errors") // Debug output