diff --git a/cmd/cbzoptimizer/commands/optimize_command.go b/cmd/cbzoptimizer/commands/optimize_command.go index b742f0b..7ca0f4b 100644 --- a/cmd/cbzoptimizer/commands/optimize_command.go +++ b/cmd/cbzoptimizer/commands/optimize_command.go @@ -24,10 +24,10 @@ func init() { RunE: ConvertCbzCommand, Args: cobra.ExactArgs(1), } - + // Setup common flags (format, quality, override, split, timeout) setupCommonFlags(command, &converterType, 85, false, false, false) - + // Setup optimize-specific flags command.Flags().IntP("parallelism", "n", 2, "Number of chapters to convert in parallel") @@ -128,15 +128,15 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error { log.Debug().Int("worker_id", workerID).Msg("Worker started") for path := range fileChan { log.Debug().Int("worker_id", workerID).Str("file_path", path).Msg("Worker processing file") - err := utils2.Optimize(&utils2.OptimizeOptions{ - ChapterConverter: chapterConverter, - Path: path, - Quality: quality, - Override: override, - Split: split, - KeepFilenames: keepFilenames, - Timeout: timeout, - }) + err := utils2.Optimize(&utils2.OptimizeOptions{ + ChapterConverter: chapterConverter, + Path: path, + Quality: quality, + Override: override, + Split: split, + KeepFilenames: keepFilenames, + Timeout: timeout, + }) if err != nil { log.Error().Int("worker_id", workerID).Str("file_path", path).Err(err).Msg("Worker encountered error") errMutex.Lock() diff --git a/internal/cbz/cbz_creator.go b/internal/cbz/cbz_creator.go index 1a586c7..9b543a9 100644 --- a/internal/cbz/cbz_creator.go +++ b/internal/cbz/cbz_creator.go @@ -42,9 +42,16 @@ func resolvePageName(page *manga.PageFile, usedNames map[string]struct{}) string } // Collision: fall back to the indexed form so the entry still gets // written, but make it visibly distinct from the preserved one. - fallback := fmt.Sprintf("%s_%04d%s", stem, page.Index, page.Extension) - usedNames[fallback] = struct{}{} - return fallback + // Loop in case the fallback name is itself already taken (e.g. a + // source file literally named "cover_0001.png" colliding with a + // page-index 1 fallback "cover_0001.webp"). + for suffix := page.Index; ; suffix++ { + fallback := fmt.Sprintf("%s_%04d%s", stem, suffix, page.Extension) + if _, taken := usedNames[fallback]; !taken { + usedNames[fallback] = struct{}{} + return fallback + } + } } if page.IsSplitted { diff --git a/internal/cbz/cbz_loader.go b/internal/cbz/cbz_loader.go index 6b1795b..0fa014c 100644 --- a/internal/cbz/cbz_loader.go +++ b/internal/cbz/cbz_loader.go @@ -168,6 +168,18 @@ func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (* TempDir: tempDir, } + // usedOriginalNames tracks every OriginalName handed out in this chapter + // so it stays unique across pages, even when the source archive contains + // the same base filename in different subdirectories (e.g. vol1/page.png + // and vol2/page.png — legal in zip). Uniqueness at the extraction + // boundary prevents a downstream race in pkg/converter/webp, where two + // pages with the same stem would write the same intermediate file. + // Allocated lazily so the keepFilenames=false path stays allocation-free. + var usedOriginalNames map[string]struct{} + if keepFilenames { + usedOriginalNames = make(map[string]struct{}) + } + // For CBZ files, read metadata from zip comment pathLower := strings.ToLower(filepath.Ext(filePath)) if pathLower == ".cbz" { @@ -286,7 +298,7 @@ func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (* FilePath: outputPath, } if keepFilenames { - page.OriginalName = filepath.Base(path) + page.OriginalName = allocateUniqueBaseName(filepath.Base(path), pageIndex, usedOriginalNames) } chapter.Pages = append(chapter.Pages, page) @@ -328,6 +340,33 @@ func isJunkFile(path string) bool { return false } +// allocateUniqueBaseName returns baseName when it is not already taken by an +// earlier page in this chapter, or a collision-resolved variant otherwise. +// +// On collision the resolved name matches the existing fallback style used by +// cbz_creator's resolvePageName: stem + "_%04d" + extension. The starting +// suffix is pageIndex so the resolved name stays in the same neighborhood as +// the page's archive position. If that candidate is itself already taken +// (astronomically rare: the source archive would need both the original name +// and a matching indexed name), the suffix is incremented until a free name +// is found. The chosen name is recorded in usedNames so subsequent calls +// cannot pick it again. +func allocateUniqueBaseName(baseName string, pageIndex uint16, usedNames map[string]struct{}) string { + if _, taken := usedNames[baseName]; !taken { + usedNames[baseName] = struct{}{} + return baseName + } + ext := filepath.Ext(baseName) + stem := strings.TrimSuffix(baseName, ext) + for suffix := int(pageIndex); ; suffix++ { + candidate := fmt.Sprintf("%s_%04d%s", stem, suffix, ext) + if _, taken := usedNames[candidate]; !taken { + usedNames[candidate] = struct{}{} + return candidate + } + } +} + // LoadChapter extracts the chapter from a CBZ/CBR file to disk. // It delegates to ExtractChapter with keepFilenames=false and always // extracts all pages. Use IsAlreadyConverted for a fast conversion status diff --git a/internal/cbz/cbz_loader_test.go b/internal/cbz/cbz_loader_test.go index 8c5f0d2..fe7b037 100644 --- a/internal/cbz/cbz_loader_test.go +++ b/internal/cbz/cbz_loader_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "testing" "time" @@ -202,6 +203,84 @@ func TestExtractChapter_PagesHaveSequentialIndices(t *testing.T) { }) } +// writeCollisionCBZ builds a CBZ containing the same image base name in two +// different subdirectories plus a normal unique page. The test fixture is +// specifically crafted to reproduce the race fixed in ExtractChapter: when +// keep-filenames naively copied filepath.Base(path) into OriginalName, both +// a/page.png and b/page.png would have ended up with the same stem and the +// WebP converter would have raced on a single shared intermediate path. +func writeCollisionCBZ(t *testing.T, dir string) string { + t.Helper() + cbzPath := filepath.Join(dir, "collisions.cbz") + f, err := os.Create(cbzPath) + require.NoError(t, err) + w := zip.NewWriter(f) + + for _, entry := range []string{"a/page.png", "b/page.png", "cover.png"} { + fw, err := w.Create(entry) + require.NoError(t, err) + _, err = fw.Write([]byte("dummy image bytes for " + entry)) + require.NoError(t, err) + } + require.NoError(t, w.Close()) + require.NoError(t, f.Close()) + return cbzPath +} + +func TestExtractChapter_KeepFilenames_DeduplicatesCollidingNames(t *testing.T) { + tmpDir := t.TempDir() + cbzPath := writeCollisionCBZ(t, tmpDir) + + t.Run("keep-filenames resolves colliding base names", func(t *testing.T) { + chapter, err := ExtractChapter(context.Background(), cbzPath, true) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + require.Len(t, chapter.Pages, 3, "expected 3 pages: two colliding + one unique") + + // All OriginalNames must be non-empty bare base names and unique. + seen := make(map[string]struct{}, len(chapter.Pages)) + for _, page := range chapter.Pages { + assert.NotEmpty(t, page.OriginalName, "page %d must have OriginalName set", page.Index) + assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName, + "page %d OriginalName should be a bare base filename", page.Index) + if _, dup := seen[page.OriginalName]; dup { + t.Errorf("duplicate OriginalName across pages: %q", page.OriginalName) + } + seen[page.OriginalName] = struct{}{} + } + + // The unique page keeps its original name; the two colliding pages + // must come out as one bare "page.png" and one indexed variant. + assert.Contains(t, seen, "page.png", "one colliding page should keep the bare page.png name") + assert.Contains(t, seen, "cover.png", "the unique page should keep its original name") + + indexedPattern := regexp.MustCompile(`^page_\d{4}\.png$`) + indexedCount := 0 + for name := range seen { + if indexedPattern.MatchString(name) { + indexedCount++ + } + } + assert.Equal(t, 1, indexedCount, + "exactly one colliding page should be resolved to the page_NNNN.png pattern, got %v", seen) + }) + + t.Run("keep-filenames off leaves OriginalName empty (regression guard)", func(t *testing.T) { + // Same collision-prone archive, but with keep-filenames off: the + // dedup logic must not run, so OriginalName stays empty for every + // page. This is the historical default and must not regress. + chapter, err := ExtractChapter(context.Background(), cbzPath, false) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + require.Len(t, chapter.Pages, 3) + for _, page := range chapter.Pages { + assert.Empty(t, page.OriginalName, "page %d must have empty OriginalName when keep-filenames is off", page.Index) + } + }) +} + func TestExtractChapter_Cleanup(t *testing.T) { chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false) require.NoError(t, err) diff --git a/pkg/converter/webp/webp_converter.go b/pkg/converter/webp/webp_converter.go index 65ff591..97d400e 100644 --- a/pkg/converter/webp/webp_converter.go +++ b/pkg/converter/webp/webp_converter.go @@ -251,11 +251,13 @@ func (converter *Converter) convertPageFile(ctx context.Context, page *manga.Pag err := EncodeFile(page.FilePath, outputPath, uint(quality)) if err == nil { - // Success! No image decoding needed. + // Success! No image decoding needed. Preserve OriginalName so + // --keep-filenames carries through to the final zip entry name. return []*manga.PageFile{{ - Index: page.Index, - Extension: ".webp", - FilePath: outputPath, + Index: page.Index, + Extension: ".webp", + FilePath: outputPath, + OriginalName: page.OriginalName, }}, nil } @@ -356,6 +358,7 @@ func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.Pag FilePath: outputPath, IsSplitted: true, SplitPartIndex: uint16(i), + OriginalName: page.OriginalName, }) }