fix(loader): deduplicate original names by stem to prevent same-stem conversion race

CodeRabbit review on #217: allocateUniqueBaseName deduplicated by full filename, but intermediatePageName strips extensions, so a/page.png and b/page.jpg both converted to outputDir/page.webp concurrently. Track used stems instead so colliding pairs resolve to e.g. page.png + page_0001.jpg, keeping both intermediate and final names unique.
2026-07-22 03:15:41 +02:00 · 2026-07-21 20:13:02 -04:00
parent 4cb5d10d3f
commit b58999c6ec
3 changed files with 171 additions and 29 deletions
@@ -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
 		}
 	}
 }
@@ -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.<ext>" and the other as
+// "page_NNNN.<ext>" — 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.<ext>" and one
+	// "page_NNNN.<ext>". 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.<ext> OriginalName from the colliding pair, got %v", seenNames)
+	assert.NotEmpty(t, indexedExt,
+		"expected one page_NNNN.<ext> 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)
@@ -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{}{}
+		})
+	}
+}