fix: Fix deadlock in ConvertChapter when context cancelled during page processing (#152)

* Initial plan * Fix deadlock in ConvertChapter when context is cancelled after wgConvertedPages.Add Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com> * Fix test comments to remove placeholder issue references Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
2026-01-09 07:14:42 +01:00 · 2025-12-20 14:26:33 -05:00
parent af5bfe8000
commit e535809e74
4 changed files with 373 additions and 2 deletions
--- a/cmd/cbzoptimizer/commands/optimize_command_test.go
+++ b/cmd/cbzoptimizer/commands/optimize_command_test.go
@@ -2,6 +2,7 @@ package commands

 import (
 	"context"
+	"fmt"
 	"log"
 	"os"
 	"path/filepath"
@@ -345,3 +346,201 @@ func TestFormatFlagCaseInsensitive(t *testing.T) {
 		})
 	}
 }
+
+// TestConvertCbzCommand_ManyFiles_NoDeadlock tests that processing many files in parallel
+// does not cause a deadlock. This reproduces the scenario where processing
+// recursive folders of CBZ files with parallelism > 1 could cause a "all goroutines are asleep - deadlock!" error.
+func TestConvertCbzCommand_ManyFiles_NoDeadlock(t *testing.T) {
+	// Create a temporary directory for testing
+	tempDir, err := os.MkdirTemp("", "test_cbz_many_files")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer errs.CaptureGeneric(&err, os.RemoveAll, tempDir, "failed to remove temporary directory")
+
+	// Locate the testdata directory
+	testdataDir := filepath.Join("../../../testdata")
+	if _, err := os.Stat(testdataDir); os.IsNotExist(err) {
+		t.Fatalf("testdata directory not found")
+	}
+
+	// Create subdirectories to simulate the recursive folder structure from the bug report
+	subdirs := []string{"author1/book1", "author2/book2", "author3/book3", "author4/book4"}
+	for _, subdir := range subdirs {
+		err := os.MkdirAll(filepath.Join(tempDir, subdir), 0755)
+		if err != nil {
+			t.Fatalf("Failed to create subdirectory: %v", err)
+		}
+	}
+
+	// Find a sample CBZ file to copy
+	var sampleCBZ string
+	err = filepath.Walk(testdataDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if !info.IsDir() && strings.HasSuffix(strings.ToLower(info.Name()), ".cbz") && !strings.Contains(info.Name(), "converted") {
+			sampleCBZ = path
+			return filepath.SkipDir
+		}
+		return nil
+	})
+	if err != nil || sampleCBZ == "" {
+		t.Fatalf("Failed to find sample CBZ file: %v", err)
+	}
+
+	// Copy the sample file to multiple locations (simulating many files to process)
+	numFilesPerDir := 5
+	totalFiles := 0
+	for _, subdir := range subdirs {
+		for i := 0; i < numFilesPerDir; i++ {
+			destPath := filepath.Join(tempDir, subdir, fmt.Sprintf("Chapter_%d.cbz", i+1))
+			data, err := os.ReadFile(sampleCBZ)
+			if err != nil {
+				t.Fatalf("Failed to read sample file: %v", err)
+			}
+			err = os.WriteFile(destPath, data, 0644)
+			if err != nil {
+				t.Fatalf("Failed to write test file: %v", err)
+			}
+			totalFiles++
+		}
+	}
+	t.Logf("Created %d test files across %d directories", totalFiles, len(subdirs))
+
+	// Mock the converter.Get function
+	originalGet := converter.Get
+	converter.Get = func(format constant.ConversionFormat) (converter.Converter, error) {
+		return &MockConverter{}, nil
+	}
+	defer func() { converter.Get = originalGet }()
+
+	// Set up the command with parallelism = 2 (same as the bug report)
+	cmd := &cobra.Command{
+		Use: "optimize",
+	}
+	cmd.Flags().Uint8P("quality", "q", 85, "Quality for conversion (0-100)")
+	cmd.Flags().IntP("parallelism", "n", 2, "Number of chapters to convert in parallel")
+	cmd.Flags().BoolP("override", "o", false, "Override the original CBZ/CBR files")
+	cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks")
+	cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter")
+
+	converterType = constant.DefaultConversion
+	setupFormatFlag(cmd, &converterType, false)
+
+	// Run the command with a timeout to detect deadlocks
+	done := make(chan error, 1)
+	go func() {
+		done <- ConvertCbzCommand(cmd, []string{tempDir})
+	}()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("Command execution failed: %v", err)
+		}
+		t.Logf("Command completed successfully without deadlock")
+	case <-time.After(60 * time.Second):
+		t.Fatal("Deadlock detected: Command did not complete within 60 seconds")
+	}
+
+	// Verify that converted files were created
+	var convertedCount int
+	err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if !info.IsDir() && strings.HasSuffix(info.Name(), "_converted.cbz") {
+			convertedCount++
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("Error counting converted files: %v", err)
+	}
+
+	if convertedCount != totalFiles {
+		t.Errorf("Expected %d converted files, found %d", totalFiles, convertedCount)
+	}
+	t.Logf("Found %d converted files as expected", convertedCount)
+}
+
+// TestConvertCbzCommand_HighParallelism_NoDeadlock tests processing with high parallelism setting.
+func TestConvertCbzCommand_HighParallelism_NoDeadlock(t *testing.T) {
+	// Create a temporary directory
+	tempDir, err := os.MkdirTemp("", "test_cbz_high_parallel")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer errs.CaptureGeneric(&err, os.RemoveAll, tempDir, "failed to remove temporary directory")
+
+	// Locate the testdata directory
+	testdataDir := filepath.Join("../../../testdata")
+	if _, err := os.Stat(testdataDir); os.IsNotExist(err) {
+		t.Fatalf("testdata directory not found")
+	}
+
+	// Find and copy sample CBZ files
+	var sampleCBZ string
+	err = filepath.Walk(testdataDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if !info.IsDir() && strings.HasSuffix(strings.ToLower(info.Name()), ".cbz") && !strings.Contains(info.Name(), "converted") {
+			sampleCBZ = path
+			return filepath.SkipDir
+		}
+		return nil
+	})
+	if err != nil || sampleCBZ == "" {
+		t.Fatalf("Failed to find sample CBZ file: %v", err)
+	}
+
+	// Create many test files
+	numFiles := 15
+	for i := 0; i < numFiles; i++ {
+		destPath := filepath.Join(tempDir, fmt.Sprintf("test_file_%d.cbz", i+1))
+		data, err := os.ReadFile(sampleCBZ)
+		if err != nil {
+			t.Fatalf("Failed to read sample file: %v", err)
+		}
+		err = os.WriteFile(destPath, data, 0644)
+		if err != nil {
+			t.Fatalf("Failed to write test file: %v", err)
+		}
+	}
+
+	// Mock the converter
+	originalGet := converter.Get
+	converter.Get = func(format constant.ConversionFormat) (converter.Converter, error) {
+		return &MockConverter{}, nil
+	}
+	defer func() { converter.Get = originalGet }()
+
+	// Test with high parallelism (8)
+	cmd := &cobra.Command{
+		Use: "optimize",
+	}
+	cmd.Flags().Uint8P("quality", "q", 85, "Quality for conversion (0-100)")
+	cmd.Flags().IntP("parallelism", "n", 8, "Number of chapters to convert in parallel")
+	cmd.Flags().BoolP("override", "o", false, "Override the original CBZ/CBR files")
+	cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks")
+	cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter")
+
+	converterType = constant.DefaultConversion
+	setupFormatFlag(cmd, &converterType, false)
+
+	done := make(chan error, 1)
+	go func() {
+		done <- ConvertCbzCommand(cmd, []string{tempDir})
+	}()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("Command execution failed: %v", err)
+		}
+	case <-time.After(60 * time.Second):
+		t.Fatal("Deadlock detected with high parallelism")
+	}
+}
--- a/go.sum
+++ b/go.sum
@@ -225,8 +225,6 @@ golang.org/x/exp v0.0.0-20191030013958-a1ab85dbe136/go.mod h1:JXzH8nQsPlswgeRAPE
 golang.org/x/exp v0.0.0-20191129062945-2f5052295587/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
 golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
-golang.org/x/exp v0.0.0-20251209150349-8475f28825e9 h1:MDfG8Cvcqlt9XXrmEiD4epKn7VJHZO84hejP9Jmp0MM=
-golang.org/x/exp v0.0.0-20251209150349-8475f28825e9/go.mod h1:EPRbTFwzwjXj9NpYyyrvenVh9Y+GFeEvMNh7Xuz7xgU=
 golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93 h1:fQsdNF2N+/YewlRZiricy4P1iimyPKZ/xwniHj8Q2a0=
 golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93/go.mod h1:EPRbTFwzwjXj9NpYyyrvenVh9Y+GFeEvMNh7Xuz7xgU=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
--- a/pkg/converter/webp/webp_converter.go
+++ b/pkg/converter/webp/webp_converter.go
@@ -184,6 +184,7 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C
 				select {
 				case pagesChan <- manga.NewContainer(page, img, format, false):
 				case <-ctx.Done():
+					wgConvertedPages.Done()
 					return
 				}

@@ -195,6 +196,7 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C
 				select {
 				case pagesChan <- manga.NewContainer(page, img, format, true):
 				case <-ctx.Done():
+					wgConvertedPages.Done()
 					return
 				}
 				return
@@ -227,6 +229,7 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C
 				select {
 				case pagesChan <- manga.NewContainer(newPage, img, "N/A", true):
 				case <-ctx.Done():
+					wgConvertedPages.Done()
 					return
 				}
 			}
--- a/pkg/converter/webp/webp_converter_test.go
+++ b/pkg/converter/webp/webp_converter_test.go
@@ -3,6 +3,7 @@ package webp
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"image"
 	"image/color"
 	"image/gif"
@@ -10,6 +11,7 @@ import (
 	"image/png"
 	"sync"
 	"testing"
+	"time"

 	_ "golang.org/x/image/webp"

@@ -422,3 +424,172 @@ func TestConverter_ConvertChapter_Timeout(t *testing.T) {
 	assert.Nil(t, convertedChapter)
 	assert.Equal(t, context.DeadlineExceeded, err)
 }
+
+// TestConverter_ConvertChapter_ManyPages_NoDeadlock tests that converting chapters with many pages
+// does not cause a deadlock. This test reproduces the scenario where processing
+// many files with context cancellation could cause "all goroutines are asleep - deadlock!" error.
+// The fix ensures that wgConvertedPages.Done() is called when context is cancelled after Add(1).
+func TestConverter_ConvertChapter_ManyPages_NoDeadlock(t *testing.T) {
+	converter := New()
+	err := converter.PrepareConverter()
+	require.NoError(t, err)
+
+	// Create a chapter with many pages to increase the chance of hitting the race condition
+	numPages := 50
+	pages := make([]*manga.Page, numPages)
+	for i := 0; i < numPages; i++ {
+		pages[i] = createTestPage(t, i+1, 100, 100, "jpeg")
+	}
+
+	chapter := &manga.Chapter{
+		FilePath: "/test/chapter_many_pages.cbz",
+		Pages:    pages,
+	}
+
+	progress := func(message string, current uint32, total uint32) {
+		// No-op progress callback
+	}
+
+	// Run multiple iterations to increase the chance of hitting the race condition
+	for iteration := 0; iteration < 10; iteration++ {
+		t.Run(fmt.Sprintf("iteration_%d", iteration), func(t *testing.T) {
+			// Use a very short timeout to trigger context cancellation during processing
+			ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond)
+			defer cancel()
+
+			// This should NOT deadlock - it should return quickly with context error
+			done := make(chan struct{})
+			var convertErr error
+			go func() {
+				defer close(done)
+				_, convertErr = converter.ConvertChapter(ctx, chapter, 80, false, progress)
+			}()
+
+			// Wait with a reasonable timeout - if it takes longer than 5 seconds, we have a deadlock
+			select {
+			case <-done:
+				// Expected - conversion should complete (with error) quickly
+				assert.Error(t, convertErr, "Expected context error")
+			case <-time.After(5 * time.Second):
+				t.Fatal("Deadlock detected: ConvertChapter did not return within 5 seconds")
+			}
+		})
+	}
+}
+
+// TestConverter_ConvertChapter_ManyPages_WithSplit_NoDeadlock tests that converting chapters
+// with many pages and split enabled does not cause a deadlock.
+func TestConverter_ConvertChapter_ManyPages_WithSplit_NoDeadlock(t *testing.T) {
+	converter := New()
+	err := converter.PrepareConverter()
+	require.NoError(t, err)
+
+	// Create pages with varying heights, some requiring splits
+	numPages := 30
+	pages := make([]*manga.Page, numPages)
+	for i := 0; i < numPages; i++ {
+		height := 1000 // Normal height
+		if i%5 == 0 {
+			height = 5000 // Tall image that will be split
+		}
+		pages[i] = createTestPage(t, i+1, 100, height, "png")
+	}
+
+	chapter := &manga.Chapter{
+		FilePath: "/test/chapter_split_test.cbz",
+		Pages:    pages,
+	}
+
+	progress := func(message string, current uint32, total uint32) {
+		// No-op progress callback
+	}
+
+	// Run multiple iterations with short timeouts
+	for iteration := 0; iteration < 10; iteration++ {
+		t.Run(fmt.Sprintf("iteration_%d", iteration), func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond)
+			defer cancel()
+
+			done := make(chan struct{})
+			var convertErr error
+			go func() {
+				defer close(done)
+				_, convertErr = converter.ConvertChapter(ctx, chapter, 80, true, progress) // split=true
+			}()
+
+			select {
+			case <-done:
+				assert.Error(t, convertErr, "Expected context error")
+			case <-time.After(5 * time.Second):
+				t.Fatal("Deadlock detected: ConvertChapter with split did not return within 5 seconds")
+			}
+		})
+	}
+}
+
+// TestConverter_ConvertChapter_ConcurrentChapters_NoDeadlock simulates the scenario from the
+// original bug report where multiple chapters are processed in parallel with parallelism > 1.
+// This test ensures no deadlock occurs when multiple goroutines are converting chapters concurrently.
+func TestConverter_ConvertChapter_ConcurrentChapters_NoDeadlock(t *testing.T) {
+	converter := New()
+	err := converter.PrepareConverter()
+	require.NoError(t, err)
+
+	// Create multiple chapters, each with many pages
+	numChapters := 20
+	pagesPerChapter := 30
+	chapters := make([]*manga.Chapter, numChapters)
+
+	for c := 0; c < numChapters; c++ {
+		pages := make([]*manga.Page, pagesPerChapter)
+		for i := 0; i < pagesPerChapter; i++ {
+			pages[i] = createTestPage(t, i+1, 100, 100, "jpeg")
+		}
+		chapters[c] = &manga.Chapter{
+			FilePath: fmt.Sprintf("/test/chapter_%d.cbz", c+1),
+			Pages:    pages,
+		}
+	}
+
+	progress := func(message string, current uint32, total uint32) {}
+
+	// Process chapters concurrently with short timeouts (simulating parallelism flag)
+	parallelism := 4
+	var wg sync.WaitGroup
+	semaphore := make(chan struct{}, parallelism)
+
+	// Overall test timeout
+	testCtx, testCancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer testCancel()
+
+	for _, chapter := range chapters {
+		wg.Add(1)
+		semaphore <- struct{}{} // Acquire
+
+		go func(ch *manga.Chapter) {
+			defer wg.Done()
+			defer func() { <-semaphore }() // Release
+
+			// Use very short timeout to trigger cancellation
+			ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond)
+			defer cancel()
+
+			// This should not deadlock
+			_, _ = converter.ConvertChapter(ctx, ch, 80, false, progress)
+		}(chapter)
+	}
+
+	// Wait for all conversions with a timeout
+	done := make(chan struct{})
+	go func() {
+		wg.Wait()
+		close(done)
+	}()
+
+	select {
+	case <-done:
+		// All goroutines completed successfully
+	case <-testCtx.Done():
+		t.Fatal("Deadlock detected: Concurrent chapter conversions did not complete within 30 seconds")
+	}
+}