From 28e17a5b080c35e8f4dff4e079b0f10153d578d4 Mon Sep 17 00:00:00 2001 From: Huxley Date: Sun, 5 Jul 2026 17:35:34 +0800 Subject: [PATCH] refactor: add AppError helpers and harden upload/download error handling - refactor: replace inline AppError literals with model.New*Error constructors in service and handler layers - feat: PromoteStaged falls back to copy/delete when os.Rename returns EXDEV (cross-device link) - fix: upload error messages no longer leak internal multipart field names or parser implementation details - fix: upload size violations are converted to domain error ErrUploadTooLarge at the handler boundary - fix: download logs error and size mismatch when io.Copy fails or writes fewer bytes than expected - test: add tests for field name leak prevention, parser error sanitization, read errors after response start, and EXDEV fallback --- docs/decisions.md | 3 +- internal/handler/file.go | 63 ++++++++++++++++-- internal/handler/file_test.go | 118 +++++++++++++++++++++++++++++++-- internal/model/errors.go | 34 ++++++++-- internal/service/file.go | 49 ++++++-------- internal/storage/local.go | 40 ++++++++++- internal/storage/local_test.go | 45 +++++++++++++ 7 files changed, 305 insertions(+), 47 deletions(-) diff --git a/docs/decisions.md b/docs/decisions.md index 5785aa5..b68eeb5 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -81,8 +81,9 @@ | Staging before promotion | Storage backends write upload bytes to a staging path first, then promote the object to the long-term data path only after validation succeeds. | | Promote before DB create | The service promotes the object before creating the active file record, preventing visible DB rows from pointing at missing objects. If DB creation fails after promotion, the service best-effort deletes the promoted object. | | Upload parent location | Multipart upload `parent_id` is passed as a query parameter, keeping the multipart body focused on the file stream. | +| Service-layer upload errors | HTTP transport errors are converted at the handler boundary; the file service only handles domain errors such as oversized uploads. | **Consequences**: - Interrupted, malformed, and oversized uploads leave only staging objects, which are safe to clean by path prefix and age. -- Local storage can promote with `os.Rename`; future S3 storage can implement promotion with copy/delete while keeping business visibility controlled by the DB row. +- Local storage promotes with `os.Rename` and falls back to copy/delete on cross-device `EXDEV`; future S3 storage can implement promotion with copy/delete while keeping business visibility controlled by the DB row. - A DB failure after promotion can still leave a long-term orphan object, but it is not visible through the file API and can be cleaned independently. diff --git a/internal/handler/file.go b/internal/handler/file.go index f1c14d6..e53ac5f 100644 --- a/internal/handler/file.go +++ b/internal/handler/file.go @@ -15,6 +15,7 @@ import ( "github.com/dhao2001/mygo/internal/api" "github.com/dhao2001/mygo/internal/middleware" + "github.com/dhao2001/mygo/internal/model" "github.com/dhao2001/mygo/internal/service" ) @@ -24,6 +25,13 @@ const ( multipartFileField = "file" ) +var ( + errMissingFileField = errors.New("missing file field") + errMissingFileName = errors.New("missing file name") + errUnexpectedMultipartField = errors.New("unexpected multipart field") + errInvalidMultipartForm = errors.New("invalid multipart form") +) + // FileHandler handles file management endpoints. type FileHandler struct { fileService *service.FileService @@ -111,12 +119,12 @@ func (h *FileHandler) Upload(c *gin.Context) { api.Error(c, http.StatusRequestEntityTooLarge, "request body is too large") return } - api.Error(c, http.StatusBadRequest, err.Error()) + api.Error(c, http.StatusBadRequest, uploadPartErrorMessage(err)) return } defer part.Close() - info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, fileName, part) + info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, fileName, uploadBodyReader{reader: part}) if err != nil { api.RespondError(c, err) return @@ -129,30 +137,55 @@ func nextUploadFilePart(reader *multipart.Reader) (*multipart.Part, string, erro part, err := reader.NextPart() if err != nil { if err == io.EOF { - return nil, "", errors.New("missing file field") + return nil, "", errMissingFileField } - return nil, "", err + return nil, "", fmt.Errorf("%w: %v", errInvalidMultipartForm, err) } if part.FormName() != multipartFileField { part.Close() - return nil, "", fmt.Errorf("unexpected multipart field %q", part.FormName()) + return nil, "", fmt.Errorf("%w: %q", errUnexpectedMultipartField, part.FormName()) } fileName := part.FileName() if fileName == "" { part.Close() - return nil, "", errors.New("missing file name") + return nil, "", errMissingFileName } return part, fileName, nil } +func uploadPartErrorMessage(err error) string { + switch { + case errors.Is(err, errMissingFileField): + return "missing file field" + case errors.Is(err, errMissingFileName): + return "missing file name" + case errors.Is(err, errUnexpectedMultipartField): + return "unexpected multipart field" + default: + return "invalid multipart form" + } +} + func isRequestBodyTooLarge(err error) bool { var maxBytesErr *http.MaxBytesError return errors.As(err, &maxBytesErr) } +type uploadBodyReader struct { + reader io.Reader +} + +func (r uploadBodyReader) Read(p []byte) (int, error) { + n, err := r.reader.Read(p) + if isRequestBodyTooLarge(err) { + return n, model.ErrUploadTooLarge + } + return n, err +} + // List handles GET /api/v1/files. func (h *FileHandler) List(c *gin.Context) { userID := middleware.MustGetUserID(c) @@ -223,7 +256,23 @@ func (h *FileHandler) Download(c *gin.Context) { c.Header("Content-Length", strconv.FormatInt(info.Size, 10)) c.Status(http.StatusOK) - io.Copy(c.Writer, reader) + written, err := io.Copy(c.Writer, reader) + if err != nil { + slog.WarnContext(c.Request.Context(), "download copy failed", + "user_id", userID, + "file_id", fileID, + "written", written, + "expected_size", info.Size, + "error", err) + return + } + if written != info.Size { + slog.WarnContext(c.Request.Context(), "download size mismatch", + "user_id", userID, + "file_id", fileID, + "written", written, + "expected_size", info.Size) + } } // Update handles PUT /api/v1/files/:id. diff --git a/internal/handler/file_test.go b/internal/handler/file_test.go index 99dacf2..7df27cc 100644 --- a/internal/handler/file_test.go +++ b/internal/handler/file_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "io" "mime/multipart" "net/http" @@ -23,7 +24,8 @@ import ( // inMemStore implements storage.StorageBackend with an in-memory map. type inMemStore struct { - files map[string][]byte + files map[string][]byte + failReads bool } func newInMemStore() *inMemStore { @@ -54,6 +56,9 @@ func (s *inMemStore) Open(_ context.Context, path string) (io.ReadCloser, error) if !ok { return nil, &fsNotFoundError{path} } + if s.failReads { + return io.NopCloser(&errorAfterReader{data: data, err: errors.New("download read failed")}), nil + } return io.NopCloser(bytes.NewReader(data)), nil } @@ -70,13 +75,27 @@ type fsNotFoundError struct{ path string } func (e *fsNotFoundError) Error() string { return "file not found on disk: " + e.path } +type errorAfterReader struct { + data []byte + err error +} + +func (r *errorAfterReader) Read(p []byte) (int, error) { + if len(r.data) == 0 { + return 0, r.err + } + n := copy(p, r.data) + r.data = r.data[n:] + return n, nil +} + var _ storage.StorageBackend = (*inMemStore)(nil) -func setupFileHandler(t *testing.T) *FileHandler { +func setupFileHandler(t *testing.T) (*FileHandler, *inMemStore) { return setupFileHandlerWithMaxUploadSize(t, 0) } -func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) *FileHandler { +func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) (*FileHandler, *inMemStore) { t.Helper() db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) @@ -91,7 +110,7 @@ func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) *FileH store := newInMemStore() fileService := service.NewFileService(fileRepo, store, maxUploadSize, nil) - return NewFileHandler(fileService) + return NewFileHandler(fileService), store } func setupFileRouter(t *testing.T) *gin.Engine { @@ -99,9 +118,14 @@ func setupFileRouter(t *testing.T) *gin.Engine { } func setupFileRouterWithMaxUploadSize(t *testing.T, maxUploadSize int64) *gin.Engine { + r, _ := setupFileRouterWithMaxUploadSizeAndStore(t, maxUploadSize) + return r +} + +func setupFileRouterWithMaxUploadSizeAndStore(t *testing.T, maxUploadSize int64) (*gin.Engine, *inMemStore) { t.Helper() - handler := setupFileHandlerWithMaxUploadSize(t, maxUploadSize) + handler, store := setupFileHandlerWithMaxUploadSize(t, maxUploadSize) gin.SetMode(gin.TestMode) r := gin.New() @@ -125,7 +149,7 @@ func setupFileRouterWithMaxUploadSize(t *testing.T, maxUploadSize int64) *gin.En files.DELETE("/:id", handler.Delete) } - return r + return r, store } func TestFileHandler_Upload(t *testing.T) { @@ -234,6 +258,52 @@ func TestFileHandler_UploadNoFile(t *testing.T) { } } +func TestFileHandler_UploadUnexpectedFieldDoesNotEchoFieldName(t *testing.T) { + r := setupFileRouter(t) + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, _ := writer.CreateFormField("secret_token") + part.Write([]byte("secret")) + writer.Close() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/files", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req.Header.Set("X-User-ID", "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body = %s", rec.Code, http.StatusBadRequest, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "unexpected multipart field") { + t.Errorf("body = %s, want safe unexpected field message", rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "secret_token") { + t.Errorf("body leaks multipart field name: %s", rec.Body.String()) + } +} + +func TestFileHandler_UploadMalformedMultipartDoesNotLeakParserError(t *testing.T) { + r := setupFileRouter(t) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/files", strings.NewReader("not a valid multipart body")) + req.Header.Set("Content-Type", "multipart/form-data; boundary=mygo-boundary") + req.Header.Set("X-User-ID", "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body = %s", rec.Code, http.StatusBadRequest, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "invalid multipart form") { + t.Errorf("body = %s, want safe invalid multipart message", rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "NextPart") || strings.Contains(rec.Body.String(), "EOF") { + t.Errorf("body leaks parser internals: %s", rec.Body.String()) + } +} + func TestFileHandler_CreateDir(t *testing.T) { r := setupFileRouter(t) @@ -380,6 +450,42 @@ func TestFileHandler_Download(t *testing.T) { } } +func TestFileHandler_DownloadReadErrorAfterResponseStarted(t *testing.T) { + r, store := setupFileRouterWithMaxUploadSizeAndStore(t, 0) + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, _ := writer.CreateFormFile("file", "download-error.txt") + content := []byte("partial response") + part.Write(content) + writer.Close() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/files", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req.Header.Set("X-User-ID", "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + var uploaded service.FileInfo + if err := json.Unmarshal(rec.Body.Bytes(), &uploaded); err != nil { + t.Fatalf("unmarshal upload: %v", err) + } + + store.failReads = true + + req = httptest.NewRequest(http.MethodGet, "/api/v1/files/"+uploaded.ID+"/content", nil) + req.Header.Set("X-User-ID", "user1") + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want %d", rec.Code, http.StatusOK) + } + if !bytes.Equal(rec.Body.Bytes(), content) { + t.Errorf("content = %q, want %q", rec.Body.String(), content) + } +} + func TestFileHandler_Update(t *testing.T) { r := setupFileRouter(t) diff --git a/internal/model/errors.go b/internal/model/errors.go index a4d8719..3389528 100644 --- a/internal/model/errors.go +++ b/internal/model/errors.go @@ -7,10 +7,11 @@ import ( ) var ( - ErrNotFound = errors.New("resource not found") - ErrDuplicate = errors.New("resource already exists") - ErrUnauthorized = errors.New("unauthorized") - ErrForbidden = errors.New("forbidden") + ErrNotFound = errors.New("resource not found") + ErrDuplicate = errors.New("resource already exists") + ErrUnauthorized = errors.New("unauthorized") + ErrForbidden = errors.New("forbidden") + ErrUploadTooLarge = errors.New("upload exceeds maximum size") ) // AppError is a service-layer error that carries an HTTP status, a user-safe @@ -25,6 +26,31 @@ type AppError struct { func (e *AppError) Error() string { return e.Message } func (e *AppError) Unwrap() error { return e.Err } +// NewBadRequestError creates a 400 AppError. +func NewBadRequestError(message string) *AppError { + return &AppError{Status: http.StatusBadRequest, Message: message} +} + +// NewConflictError creates a 409 AppError. +func NewConflictError(message string) *AppError { + return &AppError{Status: http.StatusConflict, Message: message} +} + +// NewNotFoundError creates a 404 AppError. +func NewNotFoundError(message string, cause error) *AppError { + return &AppError{Status: http.StatusNotFound, Message: message, Err: cause} +} + +// NewForbiddenError creates a 403 AppError. +func NewForbiddenError(message string, cause error) *AppError { + return &AppError{Status: http.StatusForbidden, Message: message, Err: cause} +} + +// NewPayloadTooLargeError creates a 413 AppError. +func NewPayloadTooLargeError(message string, cause error) *AppError { + return &AppError{Status: http.StatusRequestEntityTooLarge, Message: message, Err: cause} +} + // NewInternalError creates a 500 AppError with a wrapped internal cause. // msg describes the operation that failed; cause is the underlying error. func NewInternalError(msg string, cause error) *AppError { diff --git a/internal/service/file.go b/internal/service/file.go index 74bca17..a2911a2 100644 --- a/internal/service/file.go +++ b/internal/service/file.go @@ -9,7 +9,6 @@ import ( "fmt" "io" "log/slog" - "net/http" "strings" "time" @@ -41,8 +40,6 @@ type FileList struct { Total int64 `json:"total"` } -var errUploadTooLarge = errors.New("upload exceeds maximum size") - // FileService handles file management business logic. type FileService struct { fileRepo repository.FileRepository @@ -90,7 +87,7 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin // Check for name conflicts in the target directory. if _, err := s.fileRepo.FindByNameAndParent(ctx, userID, parentID, fileName); err == nil { - return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} + return nil, model.NewConflictError("a file with this name already exists in this directory") } else if !errors.Is(err, model.ErrNotFound) { return nil, model.NewInternalError("check name conflict", err) } @@ -156,7 +153,7 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin // Compensate: remove the promoted file. _ = s.storage.Delete(ctx, storagePath) if errors.Is(err, model.ErrDuplicate) { - return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} + return nil, model.NewConflictError("a file with this name already exists in this directory") } return nil, model.NewInternalError("create file record", err) } @@ -179,7 +176,7 @@ func (r *uploadLimitReader) Read(p []byte) (int, error) { n, err := r.reader.Read(one[:]) if n > 0 { p[0] = one[0] - return 1, errUploadTooLarge + return 1, model.ErrUploadTooLarge } return 0, err } @@ -197,18 +194,14 @@ func isUploadTooLargeError(err error) bool { if err == nil { return false } - if errors.Is(err, errUploadTooLarge) { - return true - } - var maxBytesErr *http.MaxBytesError - return errors.As(err, &maxBytesErr) + return errors.Is(err, model.ErrUploadTooLarge) } func uploadTooLargeError(maxUploadSize int64) *model.AppError { if maxUploadSize > 0 { - return &model.AppError{Status: http.StatusRequestEntityTooLarge, Message: fmt.Sprintf("file size exceeds the maximum allowed size of %d bytes", maxUploadSize)} + return model.NewPayloadTooLargeError(fmt.Sprintf("file size exceeds the maximum allowed size of %d bytes", maxUploadSize), model.ErrUploadTooLarge) } - return &model.AppError{Status: http.StatusRequestEntityTooLarge, Message: "request body is too large"} + return model.NewPayloadTooLargeError("request body is too large", model.ErrUploadTooLarge) } // Download returns a reader for the file's content and its metadata. @@ -220,7 +213,7 @@ func (s *FileService) Download(ctx context.Context, userID, fileID string) (io.R } if file.IsDir { - return nil, nil, &model.AppError{Status: http.StatusBadRequest, Message: "cannot download a directory"} + return nil, nil, model.NewBadRequestError("cannot download a directory") } reader, err := s.storage.Open(ctx, file.StoragePath) @@ -279,7 +272,7 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName // If parent is changing, verify the new parent. if newParentID != nil { if *newParentID == fileID { - return nil, &model.AppError{Status: http.StatusBadRequest, Message: "cannot move a directory into itself"} + return nil, model.NewBadRequestError("cannot move a directory into itself") } if err := s.verifyParent(ctx, userID, *newParentID); err != nil { return nil, err @@ -291,7 +284,7 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName if newName != "" || newParentID != nil { conflict, err := s.fileRepo.FindByNameAndParent(ctx, userID, file.ParentID, file.Name) if err == nil && conflict.ID != fileID { - return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in the target directory"} + return nil, model.NewConflictError("a file with this name already exists in the target directory") } else if err != nil && !errors.Is(err, model.ErrNotFound) { return nil, model.NewInternalError("check name conflict", err) } @@ -299,7 +292,7 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName if err := s.fileRepo.Update(ctx, file); err != nil { if errors.Is(err, model.ErrDuplicate) { - return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in the target directory"} + return nil, model.NewConflictError("a file with this name already exists in the target directory") } return nil, model.NewInternalError("update file", err) } @@ -325,7 +318,7 @@ func (s *FileService) Delete(ctx context.Context, userID, fileID string) error { return model.NewInternalError("check directory contents", err) } if total > 0 { - return &model.AppError{Status: http.StatusConflict, Message: "directory is not empty"} + return model.NewConflictError("directory is not empty") } } @@ -350,7 +343,7 @@ func (s *FileService) CreateDir(ctx context.Context, userID string, parentID *st // Check for name conflicts in the target directory. if _, err := s.fileRepo.FindByNameAndParent(ctx, userID, parentID, dirName); err == nil { - return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} + return nil, model.NewConflictError("a file with this name already exists in this directory") } else if !errors.Is(err, model.ErrNotFound) { return nil, model.NewInternalError("check name conflict", err) } @@ -366,7 +359,7 @@ func (s *FileService) CreateDir(ctx context.Context, userID string, parentID *st if err := s.fileRepo.Create(ctx, dir); err != nil { if errors.Is(err, model.ErrDuplicate) { - return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} + return nil, model.NewConflictError("a file with this name already exists in this directory") } return nil, model.NewInternalError("create directory record", err) } @@ -379,13 +372,13 @@ func (s *FileService) getOwnedFile(ctx context.Context, userID, fileID string) ( file, err := s.fileRepo.FindByID(ctx, fileID) if err != nil { if errors.Is(err, model.ErrNotFound) { - return nil, &model.AppError{Status: http.StatusNotFound, Message: "file not found", Err: model.ErrNotFound} + return nil, model.NewNotFoundError("file not found", model.ErrNotFound) } return nil, model.NewInternalError("find file", err) } if file.UserID != userID { - return nil, &model.AppError{Status: http.StatusForbidden, Message: "access denied", Err: model.ErrForbidden} + return nil, model.NewForbiddenError("access denied", model.ErrForbidden) } return file, nil @@ -396,17 +389,17 @@ func (s *FileService) verifyParent(ctx context.Context, userID, parentID string) parent, err := s.fileRepo.FindByID(ctx, parentID) if err != nil { if errors.Is(err, model.ErrNotFound) { - return &model.AppError{Status: http.StatusNotFound, Message: "parent directory not found"} + return model.NewNotFoundError("parent directory not found", model.ErrNotFound) } return model.NewInternalError("find parent", err) } if parent.UserID != userID { - return &model.AppError{Status: http.StatusForbidden, Message: "access denied", Err: model.ErrForbidden} + return model.NewForbiddenError("access denied", model.ErrForbidden) } if !parent.IsDir { - return &model.AppError{Status: http.StatusBadRequest, Message: "parent is not a directory"} + return model.NewBadRequestError("parent is not a directory") } return nil @@ -416,13 +409,13 @@ func (s *FileService) verifyParent(ctx context.Context, userID, parentID string) // null bytes, or are reserved names "." and "..". func validateFileName(name string) error { if name == "" { - return &model.AppError{Status: http.StatusBadRequest, Message: "file name must not be empty"} + return model.NewBadRequestError("file name must not be empty") } if strings.ContainsAny(name, "/\\\x00") { - return &model.AppError{Status: http.StatusBadRequest, Message: "file name contains invalid characters"} + return model.NewBadRequestError("file name contains invalid characters") } if name == "." || name == ".." { - return &model.AppError{Status: http.StatusBadRequest, Message: fmt.Sprintf("file name is reserved: %s", name)} + return model.NewBadRequestError(fmt.Sprintf("file name is reserved: %s", name)) } return nil } diff --git a/internal/storage/local.go b/internal/storage/local.go index d8e781c..ac981e4 100644 --- a/internal/storage/local.go +++ b/internal/storage/local.go @@ -2,11 +2,13 @@ package storage import ( "context" + "errors" "fmt" "io" "os" "path/filepath" "strings" + "syscall" ) const ( @@ -14,6 +16,8 @@ const ( stagingPathPrefix = "staging" ) +var renameLocalFile = os.Rename + // LocalStorage implements StorageBackend using the local filesystem. type LocalStorage struct { basePath string @@ -87,13 +91,47 @@ func (s *LocalStorage) PromoteStaged(_ context.Context, stagedPath, finalPath st return fmt.Errorf("create parent directory: %w", err) } - if err := os.Rename(fullStagedPath, fullFinalPath); err != nil { + if err := renameLocalFile(fullStagedPath, fullFinalPath); err != nil { + if errors.Is(err, syscall.EXDEV) { + return promoteByCopy(fullStagedPath, fullFinalPath) + } return fmt.Errorf("promote staged file: %w", err) } return nil } +func promoteByCopy(stagedPath, finalPath string) error { + source, err := os.Open(stagedPath) + if err != nil { + return fmt.Errorf("open staged file: %w", err) + } + defer source.Close() + + destination, err := os.OpenFile(finalPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0666) + if err != nil { + return fmt.Errorf("create final file: %w", err) + } + + if _, err := io.Copy(destination, source); err != nil { + destination.Close() + os.Remove(finalPath) + return fmt.Errorf("copy staged file: %w", err) + } + + if err := destination.Close(); err != nil { + os.Remove(finalPath) + return fmt.Errorf("close final file: %w", err) + } + + if err := os.Remove(stagedPath); err != nil { + os.Remove(finalPath) + return fmt.Errorf("delete staged file after copy: %w", err) + } + + return nil +} + // Open returns a reader for the file at path under the storage root. func (s *LocalStorage) Open(_ context.Context, path string) (io.ReadCloser, error) { fullPath := filepath.Join(s.basePath, path) diff --git a/internal/storage/local_test.go b/internal/storage/local_test.go index 5ee3d77..37c86a2 100644 --- a/internal/storage/local_test.go +++ b/internal/storage/local_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "strings" + "syscall" "testing" ) @@ -179,6 +180,50 @@ func TestPromoteRequiresDataFinalPrefix(t *testing.T) { } } +func TestPromoteStagedFallsBackToCopyOnEXDEV(t *testing.T) { + dir := t.TempDir() + store, err := NewLocalStorage(dir) + if err != nil { + t.Fatalf("NewLocalStorage: %v", err) + } + ctx := context.Background() + + content := "copy across filesystems" + _, err = store.SaveStaged(ctx, "staging/cross-device.txt", strings.NewReader(content)) + if err != nil { + t.Fatalf("SaveStaged: %v", err) + } + + originalRename := renameLocalFile + renameLocalFile = func(_, _ string) error { + return &os.LinkError{Op: "rename", Err: syscall.EXDEV} + } + t.Cleanup(func() { + renameLocalFile = originalRename + }) + + if err := store.PromoteStaged(ctx, "staging/cross-device.txt", "data/cross-device.txt"); err != nil { + t.Fatalf("PromoteStaged: %v", err) + } + + reader, err := store.Open(ctx, "data/cross-device.txt") + if err != nil { + t.Fatalf("Open promoted file: %v", err) + } + got, err := io.ReadAll(reader) + reader.Close() + if err != nil { + t.Fatalf("ReadAll: %v", err) + } + if string(got) != content { + t.Errorf("content = %q, want %q", string(got), content) + } + + if _, err := os.Stat(dir + "/staging/cross-device.txt"); !os.IsNotExist(err) { + t.Errorf("staged file should be removed after fallback promote, stat err = %v", err) + } +} + func TestOpenMissingFile(t *testing.T) { dir := t.TempDir() store, err := NewLocalStorage(dir)