From e535809e7406310856dbe30a6f87612f11e59d22 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 20 Dec 2025 14:26:33 -0500 Subject: [PATCH] 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> --- .../commands/optimize_command_test.go | 199 ++++++++++++++++++ go.sum | 2 - pkg/converter/webp/webp_converter.go | 3 + pkg/converter/webp/webp_converter_test.go | 171 +++++++++++++++ 4 files changed, 373 insertions(+), 2 deletions(-) diff --git a/cmd/cbzoptimizer/commands/optimize_command_test.go b/cmd/cbzoptimizer/commands/optimize_command_test.go index 5fd853b..1392e27 100644 --- 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") + } +} diff --git a/go.sum b/go.sum index c226b02..6cc0307 100644 --- 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= diff --git a/pkg/converter/webp/webp_converter.go b/pkg/converter/webp/webp_converter.go index 0f24754..ad63b49 100644 --- 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 } } diff --git a/pkg/converter/webp/webp_converter_test.go b/pkg/converter/webp/webp_converter_test.go index 430b9b3..ef8a18e 100644 --- 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") + } +}