From 218750072362ddbd753c7c2cd8399b4ff529c5c0 Mon Sep 17 00:00:00 2001 From: javi11 Date: Fri, 24 Apr 2026 13:32:46 +0200 Subject: [PATCH 1/4] fix(nzb): handle gzipped NZBs end-to-end; centralize in internal/nzbfile Three related bugs from the .nzb.gz persistence change were producing user-visible failures: - Download handler returned 404 for queue items whose stored NzbPath did not match the on-disk extension variant. - External SABnzbd fallback uploaded raw gzip bytes, which SABnzbd rejected with "XML syntax error on line 2: illegal character code U+001F". - Collision suffixing treated ".nzb.gz" as a single-extension ".gz", producing names like "movie.nzb_1.gz" that broke downstream scanners and the download handler's suffix check. Consolidate gzip NZB I/O in a new internal/nzbfile package (Open, Compress, IsGzipped, PlainFilename, GzExtension, ResolveOnDisk) and route all callers through it: API download handler, importer service (persist + size calc + metadata regen), processor, migration, and the SABnzbd client. Delete the importer-local nzb_file.go helpers and move their tests into the new package. Download now streams via io.Copy(c.Response().BodyWriter(), rc) instead of buffering the whole decompressed payload. Collision logic moves into a testable nextCollisionPath helper using nzbtrim.TrimNzbExtension. --- internal/api/queue_handlers.go | 33 +++----- internal/importer/collision_test.go | 64 +++++++++++++++ internal/importer/migration.go | 4 +- internal/importer/migration_test.go | 9 +- internal/importer/nzb_file.go | 84 ------------------- internal/importer/nzb_file_test.go | 77 ----------------- internal/importer/processor.go | 5 +- internal/importer/service.go | 50 +++++------ internal/nzbfile/nzbfile.go | 123 ++++++++++++++++++++++++++++ internal/nzbfile/nzbfile_test.go | 94 +++++++++++++++++++++ internal/sabnzbd/client.go | 15 ++-- 11 files changed, 335 insertions(+), 223 deletions(-) create mode 100644 internal/importer/collision_test.go delete mode 100644 internal/importer/nzb_file.go delete mode 100644 internal/importer/nzb_file_test.go create mode 100644 internal/nzbfile/nzbfile.go create mode 100644 internal/nzbfile/nzbfile_test.go diff --git a/internal/api/queue_handlers.go b/internal/api/queue_handlers.go index 8c9562fab..464635c33 100644 --- a/internal/api/queue_handlers.go +++ b/internal/api/queue_handlers.go @@ -1,7 +1,6 @@ package api import ( - "compress/gzip" "fmt" "html" "io" @@ -19,6 +18,7 @@ import ( "github.com/javi11/altmount/internal/database" internalerrors "github.com/javi11/altmount/internal/errors" "github.com/javi11/altmount/internal/importer/utils/nzbtrim" + "github.com/javi11/altmount/internal/nzbfile" "github.com/javi11/altmount/internal/nzblnk" ) @@ -1330,39 +1330,26 @@ func (s *Server) handleDownloadNZB(c *fiber.Ctx) error { return RespondNotFound(c, "Queue item", "") } - // Check if NZB file exists - if _, err := os.Stat(item.NzbPath); os.IsNotExist(err) { + resolved, err := nzbfile.ResolveOnDisk(item.NzbPath) + if err != nil { return RespondNotFound(c, "NZB file", "The NZB file no longer exists on disk") } - // Strip .gz suffix from the download filename so clients receive a plain .nzb - filename := filepath.Base(item.NzbPath) - if strings.HasSuffix(strings.ToLower(filename), ".nzb.gz") { - filename = strings.TrimSuffix(filename, ".gz") - } c.Set("Content-Type", "application/x-nzb") - c.Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", filename)) + c.Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", nzbfile.PlainFilename(resolved))) - // For gzip-compressed NZBs, decompress on-the-fly before sending - if strings.HasSuffix(strings.ToLower(item.NzbPath), ".nzb.gz") { - f, err := os.Open(item.NzbPath) + if nzbfile.IsGzipped(resolved) { + rc, err := nzbfile.Open(resolved) if err != nil { return RespondInternalError(c, "Failed to open NZB file", err.Error()) } - defer f.Close() - - gr, err := gzip.NewReader(f) - if err != nil { - return RespondInternalError(c, "Failed to decompress NZB file", err.Error()) - } - defer gr.Close() + defer rc.Close() - data, err := io.ReadAll(gr) - if err != nil { + if _, err := io.Copy(c.Response().BodyWriter(), rc); err != nil { return RespondInternalError(c, "Failed to read NZB content", err.Error()) } - return c.Send(data) + return nil } - return c.SendFile(item.NzbPath) + return c.SendFile(resolved) } diff --git a/internal/importer/collision_test.go b/internal/importer/collision_test.go new file mode 100644 index 000000000..6619bec0d --- /dev/null +++ b/internal/importer/collision_test.go @@ -0,0 +1,64 @@ +package importer + +import ( + "os" + "path/filepath" + "testing" +) + +func TestNextCollisionPath(t *testing.T) { + tests := []struct { + name string + filename string + existing []string + want string + }{ + { + name: "compound .nzb.gz preserved", + filename: "movie.nzb.gz", + existing: []string{"movie.nzb.gz"}, + want: "movie_1.nzb.gz", + }, + { + name: "uppercase compound extension preserved", + filename: "FOO.NZB.GZ", + existing: []string{"FOO.NZB.GZ"}, + want: "FOO_1.NZB.GZ", + }, + { + name: "plain .nzb", + filename: "bar.nzb", + existing: []string{"bar.nzb"}, + want: "bar_1.nzb", + }, + { + name: "skips taken suffixes", + filename: "movie.nzb.gz", + existing: []string{"movie.nzb.gz", "movie_1.nzb.gz", "movie_2.nzb.gz"}, + want: "movie_3.nzb.gz", + }, + { + name: "no recognised extension leaves empty ext", + filename: "weird", + existing: []string{"weird"}, + want: "weird_1", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + sub := t.TempDir() + for _, name := range tc.existing { + if err := os.WriteFile(filepath.Join(sub, name), []byte("x"), 0o644); err != nil { + t.Fatalf("seed %s: %v", name, err) + } + } + + got := nextCollisionPath(sub, tc.filename) + want := filepath.Join(sub, tc.want) + if got != want { + t.Fatalf("nextCollisionPath(%q) = %q, want %q", tc.filename, got, want) + } + }) + } +} diff --git a/internal/importer/migration.go b/internal/importer/migration.go index 4ab188073..34073b262 100644 --- a/internal/importer/migration.go +++ b/internal/importer/migration.go @@ -7,6 +7,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/javi11/altmount/internal/nzbfile" ) const migrationSentinelFile = ".migration_nzb_compressed_v1" @@ -33,7 +35,7 @@ func migrateNzbsToGz(ctx context.Context, nzbDir, sentinelPath string, updater f } gzPath := path + ".gz" - if compErr := compressNzbToGz(path, gzPath); compErr != nil { + if compErr := nzbfile.Compress(path, gzPath); compErr != nil { slog.WarnContext(ctx, "NZB migration: failed to compress file", "path", path, "error", compErr) return nil diff --git a/internal/importer/migration_test.go b/internal/importer/migration_test.go index 2422879b0..675b2527f 100644 --- a/internal/importer/migration_test.go +++ b/internal/importer/migration_test.go @@ -7,10 +7,15 @@ import ( "path/filepath" "testing" + "github.com/javi11/altmount/internal/nzbfile" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +const testNzbContent = ` + +` + func TestNzbCompressionMigration(t *testing.T) { dir := t.TempDir() nzbPath := filepath.Join(dir, "movie.nzb") @@ -27,7 +32,7 @@ func TestNzbCompressionMigration(t *testing.T) { // Compressed version should exist and be readable gzPath := filepath.Join(dir, "movie.nzb.gz") - rc, err := openNzbFile(gzPath) + rc, err := nzbfile.Open(gzPath) require.NoError(t, err) data, err := io.ReadAll(rc) rc.Close() @@ -62,7 +67,7 @@ func TestNzbCompressionMigration_AlreadyGzFiles(t *testing.T) { srcNzb := filepath.Join(dir, "src.nzb") gzPath := filepath.Join(dir, "existing.nzb.gz") require.NoError(t, os.WriteFile(srcNzb, []byte(testNzbContent), 0644)) - require.NoError(t, compressNzbToGz(srcNzb, gzPath)) + require.NoError(t, nzbfile.Compress(srcNzb, gzPath)) require.NoError(t, os.Remove(srcNzb)) err := migrateNzbsToGz(context.Background(), dir, sentinelPath, nil) diff --git a/internal/importer/nzb_file.go b/internal/importer/nzb_file.go deleted file mode 100644 index 588bc3967..000000000 --- a/internal/importer/nzb_file.go +++ /dev/null @@ -1,84 +0,0 @@ -package importer - -import ( - "compress/gzip" - "io" - "os" - "strings" -) - -const nzbGzExtension = ".nzb.gz" - -// openNzbFile opens an NZB file for reading, transparently decompressing .nzb.gz files. -// The caller is responsible for closing the returned ReadCloser. -func openNzbFile(path string) (io.ReadCloser, error) { - f, err := os.Open(path) - if err != nil { - return nil, err - } - - if !strings.HasSuffix(strings.ToLower(path), nzbGzExtension) { - return f, nil - } - - gr, err := gzip.NewReader(f) - if err != nil { - f.Close() - return nil, err - } - - return &gzipReadCloser{file: f, reader: gr}, nil -} - -// gzipReadCloser wraps a gzip.Reader and its underlying file so both are closed together. -type gzipReadCloser struct { - file *os.File - reader *gzip.Reader -} - -func (g *gzipReadCloser) Read(p []byte) (int, error) { - return g.reader.Read(p) -} - -func (g *gzipReadCloser) Close() error { - rerr := g.reader.Close() - ferr := g.file.Close() - if rerr != nil { - return rerr - } - return ferr -} - -// compressNzbToGz reads the NZB at srcPath and writes a gzip-compressed copy to dstPath. -func compressNzbToGz(srcPath, dstPath string) error { - src, err := os.Open(srcPath) - if err != nil { - return err - } - defer src.Close() - - dst, err := os.Create(dstPath) - if err != nil { - return err - } - defer dst.Close() - - gw, err := gzip.NewWriterLevel(dst, gzip.BestSpeed) - if err != nil { - _ = os.Remove(dstPath) - return err - } - - if _, err := io.Copy(gw, src); err != nil { - _ = gw.Close() - _ = os.Remove(dstPath) - return err - } - - if err := gw.Close(); err != nil { - _ = os.Remove(dstPath) - return err - } - - return dst.Close() -} diff --git a/internal/importer/nzb_file_test.go b/internal/importer/nzb_file_test.go deleted file mode 100644 index 19de09a87..000000000 --- a/internal/importer/nzb_file_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package importer - -import ( - "compress/gzip" - "io" - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -const testNzbContent = ` - -` - -func TestOpenNzbFile_PlainNzb(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "test.nzb") - require.NoError(t, os.WriteFile(path, []byte(testNzbContent), 0644)) - - rc, err := openNzbFile(path) - require.NoError(t, err) - defer rc.Close() - - data, err := io.ReadAll(rc) - require.NoError(t, err) - assert.Equal(t, testNzbContent, string(data)) -} - -func TestOpenNzbFile_GzippedNzb(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "test.nzb.gz") - - f, err := os.Create(path) - require.NoError(t, err) - gw := gzip.NewWriter(f) - _, err = gw.Write([]byte(testNzbContent)) - require.NoError(t, err) - require.NoError(t, gw.Close()) - require.NoError(t, f.Close()) - - rc, err := openNzbFile(path) - require.NoError(t, err) - defer rc.Close() - - data, err := io.ReadAll(rc) - require.NoError(t, err) - assert.Equal(t, testNzbContent, string(data)) -} - -func TestOpenNzbFile_NotFound(t *testing.T) { - _, err := openNzbFile("/nonexistent/path/test.nzb") - assert.Error(t, err) -} - -func TestCompressNzbToGz(t *testing.T) { - dir := t.TempDir() - srcPath := filepath.Join(dir, "source.nzb") - require.NoError(t, os.WriteFile(srcPath, []byte(testNzbContent), 0644)) - - dstPath := filepath.Join(dir, "dest.nzb.gz") - require.NoError(t, compressNzbToGz(srcPath, dstPath)) - - rc, err := openNzbFile(dstPath) - require.NoError(t, err) - defer rc.Close() - - data, err := io.ReadAll(rc) - require.NoError(t, err) - assert.Equal(t, testNzbContent, string(data)) - - dstInfo, err := os.Stat(dstPath) - require.NoError(t, err) - assert.NotZero(t, dstInfo.Size()) -} diff --git a/internal/importer/processor.go b/internal/importer/processor.go index bf101da96..86aec5885 100644 --- a/internal/importer/processor.go +++ b/internal/importer/processor.go @@ -22,6 +22,7 @@ import ( "github.com/javi11/altmount/internal/importer/singlefile" "github.com/javi11/altmount/internal/importer/utils/nzbtrim" "github.com/javi11/altmount/internal/metadata" + "github.com/javi11/altmount/internal/nzbfile" "github.com/javi11/altmount/internal/pool" "github.com/javi11/altmount/internal/progress" ) @@ -180,10 +181,8 @@ func (proc *Processor) ProcessNzbFile(ctx context.Context, filePath, relativePat allowedExtensions = *allowedExtensionsOverride } - // Update progress: starting proc.updateProgressWithStage(queueID, 0, "Parsing NZB") - // Step 1: Open and parse the file (handles .nzb and .nzb.gz transparently) - file, err := openNzbFile(filePath) + file, err := nzbfile.Open(filePath) if err != nil { return "", nil, NewNonRetryableError("failed to open file", err) } diff --git a/internal/importer/service.go b/internal/importer/service.go index 63020f417..b546a0647 100644 --- a/internal/importer/service.go +++ b/internal/importer/service.go @@ -26,6 +26,7 @@ import ( "github.com/javi11/altmount/internal/importer/scanner" "github.com/javi11/altmount/internal/importer/utils/nzbtrim" "github.com/javi11/altmount/internal/metadata" + "github.com/javi11/altmount/internal/nzbfile" "github.com/javi11/altmount/internal/pool" "github.com/javi11/altmount/internal/progress" "github.com/javi11/altmount/internal/sabnzbd" @@ -633,6 +634,21 @@ func sanitizeFilename(name string) string { return strings.ReplaceAll(name, "/", "_") } +// nextCollisionPath returns a path in dir whose basename is filename with a +// numeric suffix inserted before its NZB extension, skipping any names that +// already exist on disk. The compound ".nzb.gz" suffix is preserved so the +// resulting filename remains a valid NZB (e.g. "movie.nzb.gz" → "movie_1.nzb.gz"). +func nextCollisionPath(dir, filename string) string { + base := nzbtrim.TrimNzbExtension(filename) + ext := strings.TrimPrefix(filename, base) + for i := 1; ; i++ { + candidate := filepath.Join(dir, fmt.Sprintf("%s_%d%s", base, i, ext)) + if _, err := os.Stat(candidate); os.IsNotExist(err) { + return candidate + } + } +} + // AddToQueue adds a new NZB file to the import queue with optional category and priority func (s *Service) AddToQueue(ctx context.Context, filePath string, relativePath *string, category *string, priority *database.QueuePriority, metadata *string, downloadID *string) (*database.ImportQueueItem, error) { // Check context before proceeding @@ -873,23 +889,15 @@ func (s *Service) ensurePersistentNzb(ctx context.Context, item *database.Import // Generate new filename with sanitized name, always stored compressed filename := filepath.Base(item.NzbPath) newFilename := sanitizeFilename(filename) - if !strings.HasSuffix(strings.ToLower(newFilename), nzbGzExtension) { - newFilename = nzbtrim.TrimNzbExtension(newFilename) + nzbGzExtension + if !strings.HasSuffix(strings.ToLower(newFilename), nzbfile.GzExtension) { + newFilename = nzbtrim.TrimNzbExtension(newFilename) + nzbfile.GzExtension } newPath := filepath.Join(nzbDir, newFilename) // If a file with the same name already exists, append a numeric counter suffix - // (e.g. movie.nzb → movie_1.nzb) to avoid silently overwriting a different item. + // (e.g. movie.nzb.gz → movie_1.nzb.gz) to avoid silently overwriting a different item. if _, err := os.Stat(newPath); err == nil { - ext := filepath.Ext(newFilename) - base := strings.TrimSuffix(newFilename, ext) - for i := 1; ; i++ { - candidate := filepath.Join(nzbDir, fmt.Sprintf("%s_%d%s", base, i, ext)) - if _, statErr := os.Stat(candidate); os.IsNotExist(statErr) { - newPath = candidate - break - } - } + newPath = nextCollisionPath(nzbDir, newFilename) s.log.DebugContext(ctx, "NZB name collision, using alternate path", "path", newPath) } @@ -901,7 +909,7 @@ func (s *Service) ensurePersistentNzb(ctx context.Context, item *database.Import err := os.Rename(item.NzbPath, tmpPath) if err == nil { // Same filesystem: compress the tmp file to the final .nzb.gz path - if compErr := compressNzbToGz(tmpPath, newPath); compErr != nil { + if compErr := nzbfile.Compress(tmpPath, newPath); compErr != nil { _ = os.Rename(tmpPath, item.NzbPath) // attempt to restore on failure return fmt.Errorf("failed to compress NZB: %w", compErr) } @@ -909,7 +917,7 @@ func (s *Service) ensurePersistentNzb(ctx context.Context, item *database.Import } else { // Cross-device: compress directly from source to destination s.log.DebugContext(ctx, "Rename failed, compressing directly from source", "error", err, "src", item.NzbPath, "dst", newPath) - if compErr := compressNzbToGz(item.NzbPath, newPath); compErr != nil { + if compErr := nzbfile.Compress(item.NzbPath, newPath); compErr != nil { return fmt.Errorf("failed to compress NZB: %w", compErr) } if err := os.Remove(item.NzbPath); err != nil { @@ -1288,7 +1296,7 @@ func (s *Service) CalculateFileSizeOnly(filePath string) (int64, error) { return s.calculateStrmFileSize(file) } - file, err := openNzbFile(filePath) + file, err := nzbfile.Open(filePath) if err != nil { return 0, NewNonRetryableError("failed to open file for size calculation", err) } @@ -1393,18 +1401,10 @@ func (s *Service) RegenerateMetadata(ctx context.Context, mountRelativePath stri } filename := d.Name() - lname := strings.ToLower(filename) - if !strings.HasSuffix(lname, ".nzb") && !strings.HasSuffix(lname, nzbGzExtension) { + if !nzbtrim.HasNzbExtension(filename) { return nil } - // Strip .nzb.gz or .nzb to get the bare name for matching - var cleanName string - switch { - case strings.HasSuffix(lname, nzbGzExtension): - cleanName = filename[:len(filename)-len(nzbGzExtension)] - default: - cleanName = strings.TrimSuffix(filename, ".nzb") - } + cleanName := nzbtrim.TrimNzbExtension(filename) if _, after, ok := strings.Cut(cleanName, "_"); ok { // Check both with and without queue ID prefix if after == releaseName || cleanName == releaseName { diff --git a/internal/nzbfile/nzbfile.go b/internal/nzbfile/nzbfile.go new file mode 100644 index 000000000..2890a8ae9 --- /dev/null +++ b/internal/nzbfile/nzbfile.go @@ -0,0 +1,123 @@ +// Package nzbfile centralizes on-disk NZB I/O, hiding gzip compression so +// callers always deal with plain XML regardless of how the file is stored. +package nzbfile + +import ( + "compress/gzip" + "io" + "os" + "path/filepath" + "strings" +) + +// GzExtension is the persistent storage extension for gzip-compressed NZB files. +const GzExtension = ".nzb.gz" + +// IsGzipped reports whether path points to a gzip-compressed NZB by suffix. +func IsGzipped(path string) bool { + return strings.HasSuffix(strings.ToLower(path), GzExtension) +} + +// PlainFilename returns the base filename with any .gz suffix stripped, so +// downstream consumers (download clients, external SABnzbd) see a plain .nzb. +// The case of the original name is preserved. +func PlainFilename(path string) string { + name := filepath.Base(path) + if strings.HasSuffix(strings.ToLower(name), GzExtension) { + return name[:len(name)-len(".gz")] + } + return name +} + +// Open opens an NZB file for reading, transparently decompressing .nzb.gz +// files so callers always receive plain XML. The caller must Close the result. +func Open(path string) (io.ReadCloser, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + + if !IsGzipped(path) { + return f, nil + } + + gr, err := gzip.NewReader(f) + if err != nil { + f.Close() + return nil, err + } + return &gzipReadCloser{file: f, reader: gr}, nil +} + +// Compress reads the NZB at srcPath and writes a gzip-compressed copy to dstPath. +func Compress(srcPath, dstPath string) error { + src, err := os.Open(srcPath) + if err != nil { + return err + } + defer src.Close() + + dst, err := os.Create(dstPath) + if err != nil { + return err + } + defer dst.Close() + + gw, err := gzip.NewWriterLevel(dst, gzip.BestSpeed) + if err != nil { + _ = os.Remove(dstPath) + return err + } + + if _, err := io.Copy(gw, src); err != nil { + _ = gw.Close() + _ = os.Remove(dstPath) + return err + } + + if err := gw.Close(); err != nil { + _ = os.Remove(dstPath) + return err + } + return dst.Close() +} + +// ResolveOnDisk returns an existing path for an NZB, tolerating drift between +// the stored extension and what's actually on disk (e.g. legacy rows pointing +// at .nzb while the file was later gzipped, or vice versa). Returns the input +// path unchanged if it exists, the .gz-toggled variant if that exists, or +// (path, os.ErrNotExist) otherwise. +func ResolveOnDisk(path string) (string, error) { + if _, err := os.Stat(path); err == nil { + return path, nil + } else if !os.IsNotExist(err) { + return path, err + } + + var alt string + if IsGzipped(path) { + alt = strings.TrimSuffix(path, filepath.Ext(path)) + } else { + alt = path + ".gz" + } + if _, err := os.Stat(alt); err == nil { + return alt, nil + } + return path, os.ErrNotExist +} + +type gzipReadCloser struct { + file *os.File + reader *gzip.Reader +} + +func (g *gzipReadCloser) Read(p []byte) (int, error) { return g.reader.Read(p) } + +func (g *gzipReadCloser) Close() error { + rerr := g.reader.Close() + ferr := g.file.Close() + if rerr != nil { + return rerr + } + return ferr +} diff --git a/internal/nzbfile/nzbfile_test.go b/internal/nzbfile/nzbfile_test.go new file mode 100644 index 000000000..ed781f3c4 --- /dev/null +++ b/internal/nzbfile/nzbfile_test.go @@ -0,0 +1,94 @@ +package nzbfile + +import ( + "io" + "os" + "path/filepath" + "testing" +) + +func TestIsGzippedAndPlainFilename(t *testing.T) { + cases := []struct { + in string + gzipped bool + plainFn string + }{ + {"movie.nzb", false, "movie.nzb"}, + {"movie.nzb.gz", true, "movie.nzb"}, + {"MOVIE.NZB.GZ", true, "MOVIE.NZB"}, + {"/a/b/movie.nzb.gz", true, "movie.nzb"}, + } + for _, c := range cases { + if got := IsGzipped(c.in); got != c.gzipped { + t.Errorf("IsGzipped(%q)=%v want %v", c.in, got, c.gzipped) + } + if got := PlainFilename(c.in); got != c.plainFn { + t.Errorf("PlainFilename(%q)=%q want %q", c.in, got, c.plainFn) + } + } +} + +func TestOpenAndCompressRoundtrip(t *testing.T) { + dir := t.TempDir() + plain := filepath.Join(dir, "a.nzb") + gz := filepath.Join(dir, "a.nzb.gz") + content := []byte("hello") + + if err := os.WriteFile(plain, content, 0o644); err != nil { + t.Fatal(err) + } + if err := Compress(plain, gz); err != nil { + t.Fatalf("Compress: %v", err) + } + + rc, err := Open(gz) + if err != nil { + t.Fatalf("Open gz: %v", err) + } + defer rc.Close() + got, err := io.ReadAll(rc) + if err != nil { + t.Fatal(err) + } + if string(got) != string(content) { + t.Fatalf("roundtrip mismatch: %q want %q", got, content) + } + + rc2, err := Open(plain) + if err != nil { + t.Fatalf("Open plain: %v", err) + } + defer rc2.Close() + got2, _ := io.ReadAll(rc2) + if string(got2) != string(content) { + t.Fatalf("plain read mismatch: %q", got2) + } +} + +func TestResolveOnDisk(t *testing.T) { + dir := t.TempDir() + plain := filepath.Join(dir, "only-plain.nzb") + gz := filepath.Join(dir, "only-gz.nzb.gz") + os.WriteFile(plain, []byte("x"), 0o644) + os.WriteFile(gz, []byte("x"), 0o644) + + // Stored path matches disk. + if got, err := ResolveOnDisk(plain); err != nil || got != plain { + t.Errorf("plain exact: got=%q err=%v", got, err) + } + + // Stored as .nzb, on disk as .nzb.gz. + if got, err := ResolveOnDisk(filepath.Join(dir, "only-gz.nzb")); err != nil || got != gz { + t.Errorf("drift .nzb→.nzb.gz: got=%q err=%v", got, err) + } + + // Stored as .nzb.gz, on disk as .nzb. + if got, err := ResolveOnDisk(filepath.Join(dir, "only-plain.nzb.gz")); err != nil || got != plain { + t.Errorf("drift .nzb.gz→.nzb: got=%q err=%v", got, err) + } + + // Neither exists. + if _, err := ResolveOnDisk(filepath.Join(dir, "missing.nzb")); !os.IsNotExist(err) { + t.Errorf("missing: err=%v", err) + } +} diff --git a/internal/sabnzbd/client.go b/internal/sabnzbd/client.go index 00197a423..3c1d46dc4 100644 --- a/internal/sabnzbd/client.go +++ b/internal/sabnzbd/client.go @@ -10,10 +10,10 @@ import ( "net/http" "net/url" "os" - "path/filepath" "time" "github.com/javi11/altmount/internal/httpclient" + "github.com/javi11/altmount/internal/nzbfile" ) // SABnzbdClient handles communication with external SABnzbd instances @@ -72,25 +72,24 @@ func (c *SABnzbdClient) SendNZBFile(ctx context.Context, host, apiKey, nzbPath s return "", fmt.Errorf("NZB path is a directory, not a file") } - // Open the NZB file - file, err := os.Open(nzbPath) + // Open the NZB, transparently decompressing .nzb.gz so external SABnzbd + // receives plain XML regardless of on-disk storage format. + rc, err := nzbfile.Open(nzbPath) if err != nil { return "", fmt.Errorf("failed to open NZB file: %w", err) } - defer file.Close() + defer rc.Close() // Create multipart form data body := &bytes.Buffer{} writer := multipart.NewWriter(body) - // Add the file - filename := filepath.Base(nzbPath) - part, err := writer.CreateFormFile("nzbfile", filename) + part, err := writer.CreateFormFile("nzbfile", nzbfile.PlainFilename(nzbPath)) if err != nil { return "", fmt.Errorf("failed to create form file: %w", err) } - if _, err := io.Copy(part, file); err != nil { + if _, err := io.Copy(part, rc); err != nil { return "", fmt.Errorf("failed to copy file data: %w", err) } From edd15e9a9a8cb4353acf73a58ccf88253b0dcbc6 Mon Sep 17 00:00:00 2001 From: javi11 Date: Fri, 24 Apr 2026 14:03:44 +0200 Subject: [PATCH 2/4] refactor(importer): use queue ID for persistent NZB filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace filesystem collision-suffix scanning with a deterministic "_.nzb.gz" scheme so duplicate names are impossible by construction. AddToQueue now inserts the DB row first (assigning item.ID), then moves/compresses the file; on persistence failure the row is rolled back via RemoveFromQueue so the queue can't leak orphan entries pointing at temp paths. Drops the nextCollisionPath helper and its table-driven test — the problem it solved no longer exists. --- internal/importer/collision_test.go | 64 ----------------------------- internal/importer/service.go | 52 ++++++++--------------- 2 files changed, 18 insertions(+), 98 deletions(-) delete mode 100644 internal/importer/collision_test.go diff --git a/internal/importer/collision_test.go b/internal/importer/collision_test.go deleted file mode 100644 index 6619bec0d..000000000 --- a/internal/importer/collision_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package importer - -import ( - "os" - "path/filepath" - "testing" -) - -func TestNextCollisionPath(t *testing.T) { - tests := []struct { - name string - filename string - existing []string - want string - }{ - { - name: "compound .nzb.gz preserved", - filename: "movie.nzb.gz", - existing: []string{"movie.nzb.gz"}, - want: "movie_1.nzb.gz", - }, - { - name: "uppercase compound extension preserved", - filename: "FOO.NZB.GZ", - existing: []string{"FOO.NZB.GZ"}, - want: "FOO_1.NZB.GZ", - }, - { - name: "plain .nzb", - filename: "bar.nzb", - existing: []string{"bar.nzb"}, - want: "bar_1.nzb", - }, - { - name: "skips taken suffixes", - filename: "movie.nzb.gz", - existing: []string{"movie.nzb.gz", "movie_1.nzb.gz", "movie_2.nzb.gz"}, - want: "movie_3.nzb.gz", - }, - { - name: "no recognised extension leaves empty ext", - filename: "weird", - existing: []string{"weird"}, - want: "weird_1", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - sub := t.TempDir() - for _, name := range tc.existing { - if err := os.WriteFile(filepath.Join(sub, name), []byte("x"), 0o644); err != nil { - t.Fatalf("seed %s: %v", name, err) - } - } - - got := nextCollisionPath(sub, tc.filename) - want := filepath.Join(sub, tc.want) - if got != want { - t.Fatalf("nextCollisionPath(%q) = %q, want %q", tc.filename, got, want) - } - }) - } -} diff --git a/internal/importer/service.go b/internal/importer/service.go index b546a0647..6a984b079 100644 --- a/internal/importer/service.go +++ b/internal/importer/service.go @@ -634,21 +634,6 @@ func sanitizeFilename(name string) string { return strings.ReplaceAll(name, "/", "_") } -// nextCollisionPath returns a path in dir whose basename is filename with a -// numeric suffix inserted before its NZB extension, skipping any names that -// already exist on disk. The compound ".nzb.gz" suffix is preserved so the -// resulting filename remains a valid NZB (e.g. "movie.nzb.gz" → "movie_1.nzb.gz"). -func nextCollisionPath(dir, filename string) string { - base := nzbtrim.TrimNzbExtension(filename) - ext := strings.TrimPrefix(filename, base) - for i := 1; ; i++ { - candidate := filepath.Join(dir, fmt.Sprintf("%s_%d%s", base, i, ext)) - if _, err := os.Stat(candidate); os.IsNotExist(err) { - return candidate - } - } -} - // AddToQueue adds a new NZB file to the import queue with optional category and priority func (s *Service) AddToQueue(ctx context.Context, filePath string, relativePath *string, category *string, priority *database.QueuePriority, metadata *string, downloadID *string) (*database.ImportQueueItem, error) { // Check context before proceeding @@ -688,17 +673,23 @@ func (s *Service) AddToQueue(ctx context.Context, filePath string, relativePath CreatedAt: time.Now(), } - // Ensure NZB is in a persistent location immediately to prevent data loss if /tmp is cleaned on restart - if err := s.ensurePersistentNzb(ctx, item); err != nil { - s.log.ErrorContext(ctx, "Failed to ensure persistent NZB during queue addition", "file", filePath, "error", err) - return nil, fmt.Errorf("failed to make NZB persistent: %w", err) - } - + // Insert the DB row first so item.ID is available for the persistent filename + // (which uses the queue ID as a uniqueness suffix). If persisting the NZB to + // disk fails afterwards, roll back the row so the queue doesn't leak an orphan. if err := s.database.Repository.AddToQueue(ctx, item); err != nil { s.log.ErrorContext(ctx, "Failed to add file to queue", "file", item.NzbPath, "error", err) return nil, err } + if err := s.ensurePersistentNzb(ctx, item); err != nil { + s.log.ErrorContext(ctx, "Failed to ensure persistent NZB during queue addition", "file", filePath, "error", err) + if rmErr := s.database.Repository.RemoveFromQueue(ctx, item.ID); rmErr != nil { + s.log.WarnContext(ctx, "Failed to roll back queue row after persistence failure", + "queue_id", item.ID, "error", rmErr) + } + return nil, fmt.Errorf("failed to make NZB persistent: %w", err) + } + if s.broadcaster != nil { s.broadcaster.BroadcastQueueChanged() } @@ -886,20 +877,13 @@ func (s *Service) ensurePersistentNzb(ctx context.Context, item *database.Import return nil } - // Generate new filename with sanitized name, always stored compressed - filename := filepath.Base(item.NzbPath) - newFilename := sanitizeFilename(filename) - if !strings.HasSuffix(strings.ToLower(newFilename), nzbfile.GzExtension) { - newFilename = nzbtrim.TrimNzbExtension(newFilename) + nzbfile.GzExtension - } - newPath := filepath.Join(nzbDir, newFilename) - - // If a file with the same name already exists, append a numeric counter suffix - // (e.g. movie.nzb.gz → movie_1.nzb.gz) to avoid silently overwriting a different item. - if _, err := os.Stat(newPath); err == nil { - newPath = nextCollisionPath(nzbDir, newFilename) - s.log.DebugContext(ctx, "NZB name collision, using alternate path", "path", newPath) + // Persistent filename uses the queue ID as a suffix to guarantee uniqueness + // across concurrent imports without filesystem collision checks. + if item.ID == 0 { + return fmt.Errorf("cannot persist NZB without queue ID (row must be inserted first)") } + base := nzbtrim.TrimNzbExtension(sanitizeFilename(filepath.Base(item.NzbPath))) + newPath := filepath.Join(nzbDir, fmt.Sprintf("%s_%d%s", base, item.ID, nzbfile.GzExtension)) s.log.DebugContext(ctx, "Moving and compressing NZB to persistent storage", "old_path", item.NzbPath, "new_path", newPath) From aea84508f75189596a2d483702ff56ee81fc046f Mon Sep 17 00:00:00 2001 From: javi11 Date: Fri, 24 Apr 2026 14:31:07 +0200 Subject: [PATCH 3/4] feat(queue): surface download errors in UI; move delete-completed toggle to Import User-facing changes: - Queue download button now surfaces backend errors via a toast instead of swallowing them silently. For completed items a 404 shows a softer "NZB file already removed" info toast, since the file was almost certainly cleaned up post-import. - The "delete NZB after import" option moves from the Metadata section (labelled "Aggressive Cleanup") to the Import section where users actually look for import-lifecycle options. Renamed to a clearer "Delete NZB After Import" with an explicit note that downloads will no longer work after this runs. Config migration: - DeleteCompletedNzb moves from MetadataConfig to ImportConfig in Go. - Config.ShouldDeleteCompletedNzb() prefers the new import.* key but falls back to the legacy metadata.* key so existing config files keep working without manual edits. - Download-cleanup no longer clears nzb_path in the DB, so the queue list keeps displaying the original filename. --- .../config/MetadataConfigSection.tsx | 24 ------------ .../config/WorkersConfigSection.tsx | 27 +++++++++++++ frontend/src/pages/QueuePage.tsx | 39 +++++++++++++++++-- frontend/src/types/config.ts | 1 + internal/config/manager.go | 15 +++++++ internal/importer/service.go | 9 +++-- 6 files changed, 85 insertions(+), 30 deletions(-) diff --git a/frontend/src/components/config/MetadataConfigSection.tsx b/frontend/src/components/config/MetadataConfigSection.tsx index 1b638bbd7..996a324e5 100644 --- a/frontend/src/components/config/MetadataConfigSection.tsx +++ b/frontend/src/components/config/MetadataConfigSection.tsx @@ -315,30 +315,6 @@ export function MetadataConfigSection({ - - diff --git a/frontend/src/components/config/WorkersConfigSection.tsx b/frontend/src/components/config/WorkersConfigSection.tsx index 981639ad1..14c82b7ba 100644 --- a/frontend/src/components/config/WorkersConfigSection.tsx +++ b/frontend/src/components/config/WorkersConfigSection.tsx @@ -282,6 +282,33 @@ export function ImportConfigSection({ + +
+ +
diff --git a/frontend/src/pages/QueuePage.tsx b/frontend/src/pages/QueuePage.tsx index 989b11467..ce23cf45e 100644 --- a/frontend/src/pages/QueuePage.tsx +++ b/frontend/src/pages/QueuePage.tsx @@ -34,6 +34,7 @@ import { Pagination } from "../components/ui/Pagination"; import { PathDisplay } from "../components/ui/PathDisplay"; import { StatusBadge } from "../components/ui/StatusBadge"; import { useConfirm } from "../contexts/ModalContext"; +import { useToast } from "../contexts/ToastContext"; import { useAddTestQueueItem, useBulkCancelQueueItems, @@ -130,6 +131,7 @@ export function QueuePage() { const addTestQueueItem = useAddTestQueueItem(); const regenerateSymlinks = useRegenerateSymlinks(); const { confirmDelete, confirmAction } = useConfirm(); + const { showToast } = useToast(); const handleDelete = useCallback( async (id: number) => { @@ -166,10 +168,36 @@ export function QueuePage() { [confirmAction, cancelItem], ); - const handleDownload = async (id: number) => { + const handleDownload = async (id: number, status?: string) => { try { const response = await fetch(`/api/queue/${id}/download`); - if (!response.ok) throw new Error("Failed to download NZB file"); + if (!response.ok) { + let title = "Download Failed"; + let message = `Server returned ${response.status} ${response.statusText}`; + try { + const body = (await response.json()) as { + error?: { message?: string; details?: string }; + }; + if (body?.error?.message) { + title = body.error.message; + message = body.error.details || ""; + } + } catch { + // Non-JSON error body — fall back to status text. + } + // For completed items, a missing file almost always means the server + // cleaned it up post-import (delete_completed_nzb). Soften the toast. + if (response.status === 404 && status === "completed") { + showToast({ + type: "info", + title: "NZB file already removed", + message: "This NZB was cleaned up after successful import.", + }); + return; + } + showToast({ type: "error", title, message }); + return; + } const contentDisposition = response.headers.get("Content-Disposition"); const filenameMatch = contentDisposition?.match(/filename[^;=\n]*=["']?([^"'\n]*)["']?/); const filename = filenameMatch?.[1] || `queue-${id}.nzb`; @@ -184,6 +212,11 @@ export function QueuePage() { document.body.removeChild(a); } catch (error) { console.error("Failed to download NZB:", error); + showToast({ + type: "error", + title: "Download Failed", + message: error instanceof Error ? error.message : "Network error", + }); } }; @@ -944,7 +977,7 @@ export function QueuePage() {