diff --git a/internal/cbz/cbz_loader.go b/internal/cbz/cbz_loader.go index 8375b93..bd50048 100644 --- a/internal/cbz/cbz_loader.go +++ b/internal/cbz/cbz_loader.go @@ -168,16 +168,20 @@ 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{} + // usedOriginalStems tracks the STEM (filename without extension) of every + // OriginalName handed out in this chapter so stems stay unique across + // pages. Tracking stems (not full names) prevents a downstream race in + // pkg/converter/webp: the converter strips the OriginalName extension + // and appends a target-format suffix (e.g. ".webp"), so two pages that + // share a stem but differ in extension — e.g. a/page.png and b/page.jpg + // (legal in zip) — would otherwise race on a single shared intermediate + // output path. The map covers both same-stem-same-ext collisions (e.g. + // a/page.png + b/page.png) and same-stem-different-ext collisions (e.g. + // a/page.png + b/page.jpg). Allocated lazily so the keepFilenames=false + // path stays allocation-free. + var usedOriginalStems map[string]struct{} if keepFilenames { - usedOriginalNames = make(map[string]struct{}) + usedOriginalStems = make(map[string]struct{}) } // For CBZ files, read metadata from zip comment @@ -298,7 +302,7 @@ func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (* FilePath: outputPath, } if keepFilenames { - page.OriginalName = allocateUniqueBaseName(archiveBaseName(path), pageIndex, usedOriginalNames) + page.OriginalName = allocateUniqueBaseName(archiveBaseName(path), pageIndex, usedOriginalStems) } chapter.Pages = append(chapter.Pages, page) @@ -363,29 +367,45 @@ func archiveBaseName(path string) string { return normalized } -// allocateUniqueBaseName returns baseName when it is not already taken by an -// earlier page in this chapter, or a collision-resolved variant otherwise. +// allocateUniqueBaseName returns baseName when its stem 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 - } +// Stem-uniqueness contract: the function guarantees that the STEM +// (filename without extension) of the returned OriginalName is unique +// among all names previously handed out from this chapter. That contract +// matches what downstream code relies on — +// pkg/converter/webp.intermediatePageName strips the OriginalName's +// extension and appends a target-format suffix (e.g. ".webp"), so two +// OriginalNames that share a stem would otherwise race on a single +// intermediate output path. The stem is computed the same way downstream +// does it (strings.TrimSuffix(name, filepath.Ext(name))) to keep both +// definitions in lock-step. +// +// On collision the resolved name matches the existing fallback style used +// by cbz_creator's resolvePageName: stem + "_%04d" + extension. The loop +// checks the CANDIDATE's stem (not the full generated name) so two files +// that share a stem but differ in extension — e.g. "page.png" and +// "page.jpg" — resolve to, e.g., "page.png" and "page_0001.jpg", each +// preserving its original extension. The starting suffix is pageIndex so +// the resolved name stays in the same neighborhood as the page's archive +// position. If the candidate's stem 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 stem is found. The chosen name's stem is recorded in usedStems so +// subsequent calls cannot pick it again. +func allocateUniqueBaseName(baseName string, pageIndex uint16, usedStems map[string]struct{}) string { ext := filepath.Ext(baseName) stem := strings.TrimSuffix(baseName, ext) + if _, taken := usedStems[stem]; !taken { + usedStems[stem] = struct{}{} + return baseName + } for suffix := int(pageIndex); ; suffix++ { - candidate := fmt.Sprintf("%s_%04d%s", stem, suffix, ext) - if _, taken := usedNames[candidate]; !taken { - usedNames[candidate] = struct{}{} - return candidate + candidateStem := fmt.Sprintf("%s_%04d", stem, suffix) + if _, taken := usedStems[candidateStem]; !taken { + usedStems[candidateStem] = struct{}{} + return candidateStem + ext } } } diff --git a/internal/cbz/cbz_loader_test.go b/internal/cbz/cbz_loader_test.go index 052ed00..6ccae6a 100644 --- a/internal/cbz/cbz_loader_test.go +++ b/internal/cbz/cbz_loader_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "testing" "time" @@ -339,6 +340,94 @@ func TestExtractChapter_KeepFilenames_NormalizesWindowsSeparators(t *testing.T) assert.Contains(t, seen, "cover.png", "cover.png should pass through unchanged") } +// writeSameStemDifferentExtCBZ builds a CBZ containing two same-stem pages +// with different extensions plus a distinct unique page. The fixture +// reproduces the CodeRabbit finding for PR #217: a/page.png and b/page.jpg +// share a stem but do NOT share a full filename, so naive full-name +// deduplication lets both OriginalNames through and the WebP converter then +// races on a single shared intermediate output path (outputDir/page.webp). +// The fix flips the dedup tracking to STEM-uniqueness, so one of the +// colliding pair must come out as a bare "page." and the other as +// "page_NNNN." — each preserving its original extension. +func writeSameStemDifferentExtCBZ(t *testing.T, dir string) string { + t.Helper() + cbzPath := filepath.Join(dir, "same_stem.cbz") + f, err := os.Create(cbzPath) + require.NoError(t, err) + w := zip.NewWriter(f) + + for _, entry := range []string{"a/page.png", "b/page.jpg", "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_DeduplicatesSameStemDifferentExtensions(t *testing.T) { + tmpDir := t.TempDir() + cbzPath := writeSameStemDifferentExtCBZ(t, tmpDir) + + 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 same-stem + one unique") + + // Every OriginalName must be a bare base name; full names AND stems + // must each be unique across the chapter. The stem-uniqueness check is + // the contract the WebP converter relies on (it strips the extension + // and appends ".webp" when computing intermediatePageName). + seenNames := make(map[string]struct{}, len(chapter.Pages)) + seenStems := 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 := seenNames[page.OriginalName]; dup { + t.Errorf("duplicate OriginalName full name across pages: %q", page.OriginalName) + } + seenNames[page.OriginalName] = struct{}{} + + stem := strings.TrimSuffix(page.OriginalName, filepath.Ext(page.OriginalName)) + if _, dup := seenStems[stem]; dup { + t.Errorf("duplicate OriginalName stem across pages: %q (from %q)", stem, page.OriginalName) + } + seenStems[stem] = struct{}{} + } + + // The unique page keeps its original name untouched. + assert.Contains(t, seenNames, "cover.png", "the unique page should keep its original name") + + // The colliding pair must split into one bare "page." and one + // "page_NNNN.". The extensions must differ — each must keep the + // extension of the archive entry it came from. Walk order is not + // guaranteed, so the test checks both shapes regardless of which + // entry comes first. + barePattern := regexp.MustCompile(`^page\.(png|jpg)$`) + indexedPattern := regexp.MustCompile(`^page_\d{4}\.(png|jpg)$`) + + var bareExt, indexedExt string + for name := range seenNames { + if barePattern.MatchString(name) { + bareExt = filepath.Ext(name) + } + if indexedPattern.MatchString(name) { + indexedExt = filepath.Ext(name) + } + } + assert.NotEmpty(t, bareExt, + "expected one bare page. OriginalName from the colliding pair, got %v", seenNames) + assert.NotEmpty(t, indexedExt, + "expected one page_NNNN. OriginalName from the colliding pair, got %v", seenNames) + assert.NotEqual(t, bareExt, indexedExt, + "the bare and indexed variants must keep their original different extensions, got bare=%q indexed=%q", + bareExt, indexedExt) +} + 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_test.go b/pkg/converter/webp/webp_converter_test.go index cc28a59..76af3d1 100644 --- a/pkg/converter/webp/webp_converter_test.go +++ b/pkg/converter/webp/webp_converter_test.go @@ -485,3 +485,36 @@ func TestEncodeFileWithCrop(t *testing.T) { require.NoError(t, err) assert.Greater(t, info.Size(), int64(0)) } + +// TestIntermediatePageName_StemsAreUnique pins the contract that +// intermediatePageName must produce a different on-disk filename for two +// pages whose OriginalNames share a stem via the original archive entry +// name. ExtractChapter guarantees stem-unique OriginalNames per chapter +// (so, e.g., a/page.png and b/page.jpg come out as "page.png" and +// "page_0001.jpg"), and the converter must reflect that: stripping the +// extension and appending ".webp" must not collapse two distinct stems +// onto the same intermediate path. +func TestIntermediatePageName_StemsAreUnique(t *testing.T) { + tests := []struct { + name string + originalName string + splitSuffix string + }{ + {name: "bare stem", originalName: "page.png", splitSuffix: ""}, + {name: "indexed stem (post-fix)", originalName: "page_0001.jpg", splitSuffix: ""}, + {name: "bare stem + split suffix", originalName: "page.png", splitSuffix: "-00"}, + {name: "indexed stem + split suffix", originalName: "page_0001.jpg", splitSuffix: "-00"}, + } + + names := make(map[string]struct{}, len(tests)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + page := &manga.PageFile{Index: 0, OriginalName: tt.originalName} + got := intermediatePageName(page, tt.splitSuffix) + if _, dup := names[got]; dup { + t.Errorf("intermediate name %q collides with an earlier page's intermediate name", got) + } + names[got] = struct{}{} + }) + } +}