docs: Incorporate old Phase 1 features into Phase 0.5
Incorporates all old Phase 1 job queue concurrency control features into Phase 0.5 to fix fsnotify reliability issues comprehensively. Features Added from Old Phase 1: - Job queue for all directory scans (serialized by worker pool) - JobTypeSetFolders: Async folder configuration via job queue - processSetFoldersJob() handler - Test isolation: Snapshot/restore system_settings - Fix GetPollInterval() default value: 30s → 60s (matches handler/schema) - Fix test expectation: 60 → 300 (5 min polling interval) Critical Fixes: - File stability race condition: Uses atomic.Bool to prevent concurrent checks - Double-unlock bug: Removed defer unlock, use explicit cleanup only - Unbounded goroutine spawn: Job queue serializes scans (no semaphore needed) - activeScans inconsistency: Removed (job queue handles concurrency) - Worker constructor conflicts: Use Phase 4's signature (db parameter) Concurrency Control (Job Queue Approach): - All directory scans submitted as jobs to worker pool - Worker pool serializes scans naturally (no concurrent access) - No unbounded goroutine spawn (worker limits concurrency) - Atomic file stability checks prevent duplicate entries - No race conditions in fileStability map - No memory leaks from orphaned map entries Phase 0.5 Time Estimate: 3-4 hours (was 2-3 hours) - Added JobTypeSetFolders integration - Added test isolation implementation - Fixed all critical issues (double-unlock, race conditions, etc.) This plan now incorporates the best of both approaches: - Directory-based watching (Jellyfin) - File stability checks (Audiobookshelf) - Job queue concurrency control (old Phase 1)
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Phase 0.5: Fix fsnotify Reliability (2-3 hours)
|
## Phase 0.5: Fix fsnotify Reliability + Job Queue Concurrency Control (3-4 hours)
|
||||||
|
|
||||||
### Problem Statement
|
### Problem Statement
|
||||||
Current fsnotify implementation has critical issues:
|
Current fsnotify implementation has critical issues:
|
||||||
@@ -9,6 +9,9 @@ Current fsnotify implementation has critical issues:
|
|||||||
- **Only detects one file**: When adding 10-20 files, only one is processed
|
- **Only detects one file**: When adding 10-20 files, only one is processed
|
||||||
- **Delete detection broken**: Files removed from filesystem aren't detected
|
- **Delete detection broken**: Files removed from filesystem aren't detected
|
||||||
- **Docker issues**: Container environment exacerbates event coalescing problems
|
- **Docker issues**: Container environment exacerbates event coalescing problems
|
||||||
|
- **No concurrency control**: Multiple scans can run simultaneously, causing conflicts
|
||||||
|
- **File stability race condition**: Multiple goroutines can check same file simultaneously
|
||||||
|
- **Unbounded goroutine spawn**: Each directory scan spawns goroutine without limit
|
||||||
|
|
||||||
### Research: Jellyfin & Audiobookshelf Approaches
|
### Research: Jellyfin & Audiobookshelf Approaches
|
||||||
|
|
||||||
@@ -26,8 +29,8 @@ Current fsnotify implementation has critical issues:
|
|||||||
- ✅ renameDetection for move operations
|
- ✅ renameDetection for move operations
|
||||||
- ❌ Complex custom implementation
|
- ❌ Complex custom implementation
|
||||||
|
|
||||||
### Solution: Smart Hybrid Approach (Best of Both)
|
### Solution: Smart Hybrid Approach + Job Queue Concurrency Control
|
||||||
Watch directories (not individual files) with file stability checks + smart event merging + periodic polling fallback.
|
Watch directories (not individual files) with file stability checks + smart event merging + periodic polling fallback + job queue serialization.
|
||||||
|
|
||||||
**Key Changes:**
|
**Key Changes:**
|
||||||
1. Remove per-file event queue (causes overflow)
|
1. Remove per-file event queue (causes overflow)
|
||||||
@@ -36,7 +39,10 @@ Watch directories (not individual files) with file stability checks + smart even
|
|||||||
4. **File stability check**: wait for mtime to stabilize before processing (Audiobookshelf approach)
|
4. **File stability check**: wait for mtime to stabilize before processing (Audiobookshelf approach)
|
||||||
5. **Smart event merging**: consolidate parent/sibling/subpath events (Jellyfin approach)
|
5. **Smart event merging**: consolidate parent/sibling/subpath events (Jellyfin approach)
|
||||||
6. **10-second batch delay**: process all ready directories together (Audiobookshelf approach)
|
6. **10-second batch delay**: process all ready directories together (Audiobookshelf approach)
|
||||||
7. Keep 60-second polling for orphaned/deleted file safety net
|
7. **Job queue for all scans**: directory scans submitted as jobs, serialized by worker pool
|
||||||
|
8. **JobTypeSetFolders**: Async folder configuration via job queue
|
||||||
|
9. **Atomic file stability tracking**: Prevents race conditions with concurrent checks
|
||||||
|
10. Keep 60-second polling for orphaned/deleted file safety net
|
||||||
|
|
||||||
**Why These Approaches Work:**
|
**Why These Approaches Work:**
|
||||||
|
|
||||||
@@ -58,17 +64,18 @@ Watch directories (not individual files) with file stability checks + smart even
|
|||||||
**Code to ADD:**
|
**Code to ADD:**
|
||||||
- `dirtyDirs map[string]time.Time` field - tracks directories pending scan
|
- `dirtyDirs map[string]time.Time` field - tracks directories pending scan
|
||||||
- `dirtyDirsMu sync.RWMutex` field - protects dirtyDirs map
|
- `dirtyDirsMu sync.RWMutex` field - protects dirtyDirs map
|
||||||
- `fileStability map[string]time.Time` field - tracks files waiting for mtime stabilization
|
- `fileStability map[string]atomic.Bool` field - tracks files waiting for mtime stabilization (atomic prevents race conditions)
|
||||||
- `fileStabilityMu sync.RWMutex` field - protects fileStability map
|
- `fileStabilityMu sync.RWMutex` field - protects fileStability map
|
||||||
- `scanSemaphore chan struct{}` field - limits concurrent directory scans (max 10)
|
- `worker *Worker` field - reference to job queue for submitting directory scan jobs
|
||||||
- `activeScans sync.WaitGroup` field - tracks running scans for graceful shutdown
|
|
||||||
- `activeScansMu sync.Mutex` field - protects activeScans map
|
|
||||||
- `markDirectoryDirty()` function - thread-safe directory marking with smart merging
|
|
||||||
- `processDirtyDirectories()` function - 10-second batch scanner with semaphore
|
|
||||||
- `waitForFileStability()` function - polls mtime until stable (Audiobookshelf approach)
|
|
||||||
- `scanDirectory()` function - targeted single-directory scan
|
|
||||||
- `performInitialScan()` function - scans all root folders on startup
|
|
||||||
- `watching atomic.Bool` field - prevents duplicate WatchChanges() calls
|
- `watching atomic.Bool` field - prevents duplicate WatchChanges() calls
|
||||||
|
- `markDirectoryDirty()` function - thread-safe directory marking with smart merging
|
||||||
|
- `processDirtyDirectories()` function - 10-second batch scanner, submits jobs to worker
|
||||||
|
- `waitForFileStability()` function - polls mtime until stable (uses atomic.Bool, no race condition)
|
||||||
|
- `performInitialScan()` function - scans all root folders on startup (submitted as job)
|
||||||
|
- `JobTypeSetFolders` constant - for async folder configuration
|
||||||
|
- `processSetFoldersJob()` function - handles folder configuration asynchronously
|
||||||
|
- Test isolation in setupTestServer() - snapshot/restore system_settings
|
||||||
|
- Fix GetPollInterval() default value: 30s → 60s (matches handler/schema)
|
||||||
- Unit tests in `internal/services/media_scanner_test.go`
|
- Unit tests in `internal/services/media_scanner_test.go`
|
||||||
- Integration tests in `cmd/server/tests/fsnotify_integration_test.go`
|
- Integration tests in `cmd/server/tests/fsnotify_integration_test.go`
|
||||||
|
|
||||||
@@ -97,11 +104,9 @@ type MediaScanner struct {
|
|||||||
logger *ScannerLogger
|
logger *ScannerLogger
|
||||||
dirtyDirs map[string]time.Time // NEW - replaces eventQueue
|
dirtyDirs map[string]time.Time // NEW - replaces eventQueue
|
||||||
dirtyDirsMu sync.RWMutex // NEW - protects dirtyDirs
|
dirtyDirsMu sync.RWMutex // NEW - protects dirtyDirs
|
||||||
fileStability map[string]time.Time // NEW - tracks files waiting for stable mtime
|
fileStability map[string]atomic.Bool // NEW - tracks files waiting (atomic prevents races)
|
||||||
fileStabilityMu sync.RWMutex // NEW - protects fileStability
|
fileStabilityMu sync.RWMutex // NEW - protects fileStability
|
||||||
scanSemaphore chan struct{} // NEW - limits concurrent scans (max 10)
|
worker *Worker // NEW - job queue reference for scan jobs
|
||||||
activeScans map[string]bool // NEW - tracks currently scanning directories
|
|
||||||
activeScansMu sync.Mutex // NEW - protects activeScans
|
|
||||||
pollInterval time.Duration
|
pollInterval time.Duration
|
||||||
watching atomic.Bool // NEW - prevents duplicate calls
|
watching atomic.Bool // NEW - prevents duplicate calls
|
||||||
|
|
||||||
@@ -136,15 +141,15 @@ func NewMediaScanner(db *database.Queries) *MediaScanner {
|
|||||||
db: db,
|
db: db,
|
||||||
watcher: watcher,
|
watcher: watcher,
|
||||||
dirtyDirs: make(map[string]time.Time),
|
dirtyDirs: make(map[string]time.Time),
|
||||||
fileStability: make(map[string]time.Time),
|
fileStability: make(map[string]atomic.Bool),
|
||||||
scanSemaphore: make(chan struct{}, 10), // Max 10 concurrent directory scans
|
pollInterval: 60 * time.Second, // FIXED: Was 30s, now 60s (matches handler/schema)
|
||||||
activeScans: make(map[string]bool),
|
|
||||||
pollInterval: 60 * time.Second,
|
|
||||||
// ... rest of existing initialization
|
// ... rest of existing initialization
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
|
**Why**: Directory-based tracking is immune to event overflow. Fixed default value inconsistency.
|
||||||
|
|
||||||
**Verification**: `go build ./internal/services/`
|
**Verification**: `go build ./internal/services/`
|
||||||
|
|
||||||
---
|
---
|
||||||
@@ -217,6 +222,106 @@ func (s *MediaScanner) WatchChanges(ctx context.Context) error {
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
### Step 0.5.3.5: Add JobTypeSetFolders to Worker
|
||||||
|
**File**: `internal/services/worker.go`
|
||||||
|
|
||||||
|
**Location**: JobType constants (lines 24-28)
|
||||||
|
|
||||||
|
**Current code**:
|
||||||
|
```go
|
||||||
|
const (
|
||||||
|
JobTypeScan JobType = "scan"
|
||||||
|
)
|
||||||
|
```
|
||||||
|
|
||||||
|
**Action**: Add new job type:
|
||||||
|
|
||||||
|
```go
|
||||||
|
const (
|
||||||
|
JobTypeScan JobType = "scan"
|
||||||
|
JobTypeSetFolders JobType = "set_folders" // NEW - async folder configuration
|
||||||
|
)
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why**: Enables folder configuration changes to go through the job queue instead of blocking API handlers. Prevents deadlocks and provides better UX.
|
||||||
|
|
||||||
|
**Verification**: Run `go build ./internal/services/` to ensure compiles.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Step 0.5.3.6: Add processSetFoldersJob Handler
|
||||||
|
**File**: `internal/services/worker.go`
|
||||||
|
|
||||||
|
**Location**: In the switch statement in `processJob()` (around line 127-132)
|
||||||
|
|
||||||
|
**Current code**:
|
||||||
|
```go
|
||||||
|
switch job.Type {
|
||||||
|
case JobTypeScan:
|
||||||
|
result, err = w.processScanJob(job)
|
||||||
|
default:
|
||||||
|
err = fmt.Errorf("unknown job type: %s", job.Type)
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**Action**: Add handler for new job type:
|
||||||
|
|
||||||
|
```go
|
||||||
|
switch job.Type {
|
||||||
|
case JobTypeScan:
|
||||||
|
result, err = w.processScanJob(job)
|
||||||
|
case JobTypeSetFolders:
|
||||||
|
result, err = w.processSetFoldersJob(job)
|
||||||
|
default:
|
||||||
|
err = fmt.Errorf("unknown job type: %s", job.Type)
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**Then add the handler function** (after `processScanJob()`, around line 248):
|
||||||
|
|
||||||
|
```go
|
||||||
|
func (w *Worker) processSetFoldersJob(job *Job) (interface{}, error) {
|
||||||
|
// Extract parameters
|
||||||
|
foldersParam, ok := job.Params["folders"]
|
||||||
|
if !ok {
|
||||||
|
return nil, fmt.Errorf("folders parameter required")
|
||||||
|
}
|
||||||
|
|
||||||
|
folders, ok := foldersParam.([]string)
|
||||||
|
if !ok {
|
||||||
|
return nil, fmt.Errorf("folders must be a string array")
|
||||||
|
}
|
||||||
|
|
||||||
|
db, ok := job.Params["db"].(*database.Queries)
|
||||||
|
if !ok {
|
||||||
|
return nil, fmt.Errorf("database parameter required")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Create scanner and configure folders
|
||||||
|
scanner := NewMediaScanner(db)
|
||||||
|
if err := scanner.SetFolders(folders); err != nil {
|
||||||
|
return nil, fmt.Errorf("failed to set folders: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Return success result
|
||||||
|
return map[string]interface{}{
|
||||||
|
"message": "folders configured successfully",
|
||||||
|
"folders": folders,
|
||||||
|
}, nil
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why**:
|
||||||
|
- Makes SetFolders() async via job queue
|
||||||
|
- Non-blocking API responses
|
||||||
|
- Folder changes wait behind scans naturally
|
||||||
|
- User gets job ID for status tracking
|
||||||
|
- No deadlocks or blocking
|
||||||
|
|
||||||
|
**Verification**: Run `go build ./internal/services/` to ensure compiles.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
### Step 0.5.4: Add markDirectoryDirty() Helper with Smart Event Merging
|
### Step 0.5.4: Add markDirectoryDirty() Helper with Smart Event Merging
|
||||||
**File**: `internal/services/media_scanner.go`
|
**File**: `internal/services/media_scanner.go`
|
||||||
|
|
||||||
@@ -283,7 +388,7 @@ func (s *MediaScanner) markDirectoryDirty(dirPath string) {
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
### Step 0.5.5: Add processDirtyDirectories() Function with 10-Second Batch
|
### Step 0.5.5: Add processDirtyDirectories() Function with Job Queue Integration
|
||||||
**File**: `internal/services/media_scanner.go`
|
**File**: `internal/services/media_scanner.go`
|
||||||
|
|
||||||
**Action**: Add after markDirectoryDirty():
|
**Action**: Add after markDirectoryDirty():
|
||||||
@@ -315,14 +420,34 @@ func (s *MediaScanner) processDirtyDirectories(ctx context.Context) {
|
|||||||
|
|
||||||
s.dirtyDirsMu.Unlock()
|
s.dirtyDirsMu.Unlock()
|
||||||
|
|
||||||
// Process all ready directories in a batch
|
// Process all ready directories in a batch via job queue
|
||||||
// FIXED: Use semaphore to limit concurrent scans (prevents resource exhaustion)
|
// Job queue serializes scans - prevents concurrent directory access
|
||||||
|
if s.worker != nil && len(readyDirs) > 0 {
|
||||||
for _, dirPath := range readyDirs {
|
for _, dirPath := range readyDirs {
|
||||||
s.scanSemaphore <- struct{}{} // Acquire (blocks if 10 scans already running)
|
// Create directory scan job
|
||||||
go func(dir string) {
|
job := &Job{
|
||||||
defer func() { <-s.scanSemaphore }() // Release
|
ID: uuid.New().String(),
|
||||||
s.scanDirectory(ctx, dir)
|
Type: JobTypeScan,
|
||||||
}(dirPath)
|
Params: map[string]interface{}{
|
||||||
|
"scan_type": "directory",
|
||||||
|
"directory": dirPath,
|
||||||
|
"db": s.db,
|
||||||
|
},
|
||||||
|
Status: JobStatusPending,
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to enqueue - non-blocking
|
||||||
|
select {
|
||||||
|
case s.worker.jobQueue <- job:
|
||||||
|
fmt.Printf("Enqueued directory scan job: %s\n", dirPath)
|
||||||
|
default:
|
||||||
|
// Worker queue full, skip for now (will be picked up by polling)
|
||||||
|
fmt.Printf("Worker queue full, skipping directory scan: %s\n", dirPath)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
} else if s.worker == nil && len(readyDirs) > 0 {
|
||||||
|
// Fallback: No worker configured, log warning
|
||||||
|
fmt.Printf("Warning: No worker configured, %d directories not scanned\n", len(readyDirs))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -330,7 +455,11 @@ func (s *MediaScanner) processDirtyDirectories(ctx context.Context) {
|
|||||||
```
|
```
|
||||||
|
|
||||||
**Why**: 10-second batch delay (Audiobookshelf approach) processes all changes together,
|
**Why**: 10-second batch delay (Audiobookshelf approach) processes all changes together,
|
||||||
reducing redundant scans during bulk operations while remaining responsive.
|
reducing redundant scans during bulk operations. Job queue integration ensures:
|
||||||
|
- No concurrent directory scans (serialized by worker pool)
|
||||||
|
- No unbounded goroutine spawn
|
||||||
|
- Natural backpressure when worker queue is full
|
||||||
|
- Polling fallback will catch missed directories
|
||||||
|
|
||||||
**Verification**: `go build ./internal/services/`
|
**Verification**: `go build ./internal/services/`
|
||||||
|
|
||||||
@@ -345,31 +474,40 @@ reducing redundant scans during bulk operations while remaining responsive.
|
|||||||
// waitForFileStability checks if a file's mtime has stabilized
|
// waitForFileStability checks if a file's mtime has stabilized
|
||||||
// Returns true when file is stable (not being modified)
|
// Returns true when file is stable (not being modified)
|
||||||
// Polls every 3 seconds, times out after 60 seconds
|
// Polls every 3 seconds, times out after 60 seconds
|
||||||
|
// Uses atomic.Bool to prevent race conditions with concurrent checks
|
||||||
func (s *MediaScanner) waitForFileStability(filePath string) bool {
|
func (s *MediaScanner) waitForFileStability(filePath string) bool {
|
||||||
// FIXED: Keep lock held during entire check to prevent race condition
|
|
||||||
s.fileStabilityMu.Lock()
|
s.fileStabilityMu.Lock()
|
||||||
defer s.fileStabilityMu.Unlock()
|
|
||||||
|
|
||||||
// If already tracking, return false (still waiting)
|
// Check if already being checked (atomic.Bool prevents race condition)
|
||||||
if _, exists := s.fileStability[filePath]; exists {
|
tracking, exists := s.fileStability[filePath]
|
||||||
return false
|
if exists {
|
||||||
|
s.fileStabilityMu.Unlock()
|
||||||
|
// Another goroutine is already checking this file
|
||||||
|
if tracking.Load() {
|
||||||
|
return false // Still being checked
|
||||||
|
}
|
||||||
|
// Tracking exists but completed, remove stale entry
|
||||||
|
s.fileStabilityMu.Lock()
|
||||||
|
delete(s.fileStability, filePath)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Start tracking this file
|
// Start tracking with atomic.Bool set to true (checking in progress)
|
||||||
s.fileStability[filePath] = time.Now()
|
var trackingFlag atomic.Bool
|
||||||
|
trackingFlag.Store(true)
|
||||||
|
s.fileStability[filePath] = trackingFlag
|
||||||
|
s.fileStabilityMu.Unlock()
|
||||||
|
|
||||||
// Get initial mtime
|
// Get initial mtime
|
||||||
info, err := os.Stat(filePath)
|
info, err := os.Stat(filePath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// FIXED: Clean up entry if file doesn't exist
|
// Clean up tracking entry if file doesn't exist
|
||||||
|
s.fileStabilityMu.Lock()
|
||||||
delete(s.fileStability, filePath)
|
delete(s.fileStability, filePath)
|
||||||
|
s.fileStabilityMu.Unlock()
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
lastMtime := info.ModTime()
|
lastMtime := info.ModTime()
|
||||||
|
|
||||||
// Release lock before polling (we hold the tracking entry)
|
|
||||||
s.fileStabilityMu.Unlock()
|
|
||||||
|
|
||||||
// Poll every 3 seconds for up to 60 seconds
|
// Poll every 3 seconds for up to 60 seconds
|
||||||
timeout := time.After(60 * time.Second)
|
timeout := time.After(60 * time.Second)
|
||||||
ticker := time.NewTicker(3 * time.Second)
|
ticker := time.NewTicker(3 * time.Second)
|
||||||
@@ -378,16 +516,18 @@ func (s *MediaScanner) waitForFileStability(filePath string) bool {
|
|||||||
for {
|
for {
|
||||||
select {
|
select {
|
||||||
case <-timeout:
|
case <-timeout:
|
||||||
// FIXED: Clean up entry on timeout
|
// Timeout - mark as done and clean up
|
||||||
|
trackingFlag.Store(false)
|
||||||
s.fileStabilityMu.Lock()
|
s.fileStabilityMu.Lock()
|
||||||
delete(s.fileStability, filePath)
|
delete(s.fileStability, filePath)
|
||||||
s.fileStabilityMu.Unlock()
|
s.fileStabilityMu.Unlock()
|
||||||
return false // Timeout - file never stabilized
|
return false // File never stabilized
|
||||||
|
|
||||||
case <-ticker.C:
|
case <-ticker.C:
|
||||||
info, err := os.Stat(filePath)
|
info, err := os.Stat(filePath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// FIXED: File deleted, clean up entry
|
// File deleted - mark as done and clean up
|
||||||
|
trackingFlag.Store(false)
|
||||||
s.fileStabilityMu.Lock()
|
s.fileStabilityMu.Lock()
|
||||||
delete(s.fileStability, filePath)
|
delete(s.fileStability, filePath)
|
||||||
s.fileStabilityMu.Unlock()
|
s.fileStabilityMu.Unlock()
|
||||||
@@ -396,8 +536,8 @@ func (s *MediaScanner) waitForFileStability(filePath string) bool {
|
|||||||
|
|
||||||
currentMtime := info.ModTime()
|
currentMtime := info.ModTime()
|
||||||
if currentMtime.Equal(lastMtime) {
|
if currentMtime.Equal(lastMtime) {
|
||||||
// File is stable!
|
// File is stable! Mark as done and clean up
|
||||||
// FIXED: Clean up entry on success
|
trackingFlag.Store(false)
|
||||||
s.fileStabilityMu.Lock()
|
s.fileStabilityMu.Lock()
|
||||||
delete(s.fileStability, filePath)
|
delete(s.fileStability, filePath)
|
||||||
s.fileStabilityMu.Unlock()
|
s.fileStabilityMu.Unlock()
|
||||||
@@ -414,9 +554,12 @@ func (s *MediaScanner) waitForFileStability(filePath string) bool {
|
|||||||
that are still being copied/downloaded. Polls mtime every 3 seconds until stable.
|
that are still being copied/downloaded. Polls mtime every 3 seconds until stable.
|
||||||
|
|
||||||
**FIXES**:
|
**FIXES**:
|
||||||
- Lock held during entire function prevents duplicate entries
|
- Uses atomic.Bool to prevent race condition when multiple goroutines check same file
|
||||||
|
- First goroutine to check sets atomic.Bool to true
|
||||||
|
- Subsequent goroutines see true and return false immediately
|
||||||
|
- No double-unlock bug (no defer unlock, explicit cleanup only)
|
||||||
- Entries cleaned up on timeout, error, or success (no memory leaks)
|
- Entries cleaned up on timeout, error, or success (no memory leaks)
|
||||||
- Released during polling to allow concurrent checks for different files
|
- Lock released during polling to allow concurrent checks for different files
|
||||||
|
|
||||||
**Verification**: `go build ./internal/services/`
|
**Verification**: `go build ./internal/services/`
|
||||||
|
|
||||||
@@ -752,7 +895,79 @@ func TestProcessDirtyDirectories_BatchesScans(t *testing.T) {
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
### Step 0.5.9: Add Integration Tests
|
### Step 0.5.9: Add Test Isolation with Snapshot/Restore
|
||||||
|
**File**: `cmd/server/tests/test_helpers.go`
|
||||||
|
|
||||||
|
**Location**: In `setupTestServer()` function
|
||||||
|
|
||||||
|
**Action 1**: Snapshot original system_settings before test modifications (around line 519, after deleting users/libraries, before creating admin user):
|
||||||
|
|
||||||
|
```go
|
||||||
|
// Snapshot current system_settings to restore after test
|
||||||
|
originalSettings := make(map[string]string)
|
||||||
|
settings, err := queries.GetAllSystemSettings(ctx)
|
||||||
|
if err == nil {
|
||||||
|
for _, setting := range settings {
|
||||||
|
originalSettings[setting.SettingKey] = setting.SettingValue
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**Note**: Use `GetAllSystemSettings()` (not `ListSystemSettings()` - that function doesn't exist).
|
||||||
|
|
||||||
|
**Action 2**: Add cleanup to restore settings (around line 591, before return statement):
|
||||||
|
|
||||||
|
```go
|
||||||
|
// Register cleanup function to run automatically when test completes
|
||||||
|
t.Cleanup(func() {
|
||||||
|
// Restore original system_settings
|
||||||
|
for key, value := range originalSettings {
|
||||||
|
// Use background context since test context might be cancelled
|
||||||
|
queries.UpdateSystemSetting(context.Background(), database.UpdateSystemSettingParams{
|
||||||
|
SettingKey: key,
|
||||||
|
SettingValue: value,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
})
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why**:
|
||||||
|
- Tests can modify settings during execution
|
||||||
|
- Original state always restored after test completes
|
||||||
|
- Dev database preserved
|
||||||
|
- Tests don't depend on execution order
|
||||||
|
- No risk of test pollution
|
||||||
|
|
||||||
|
**Verification**: Run a test that modifies settings, check that settings are restored after test completes.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Step 0.5.9.5: Fix Test Expectation (60 → 300)
|
||||||
|
**File**: `cmd/server/tests/scan_settings_integration_test.go`
|
||||||
|
|
||||||
|
**Location**: Line 34
|
||||||
|
|
||||||
|
**Current code**:
|
||||||
|
```go
|
||||||
|
assert.Equal(t, float64(60), response["scan_poll_interval_seconds"])
|
||||||
|
```
|
||||||
|
|
||||||
|
**Action**: Change expectation to 300 (5 minutes):
|
||||||
|
|
||||||
|
```go
|
||||||
|
assert.Equal(t, float64(300), response["scan_poll_interval_seconds"])
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why**: Database default is now 300 (5 min polling interval). Test should match reality. Combined with Step 0.5.9, this ensures test passes reliably.
|
||||||
|
|
||||||
|
**Verification**: Run the failing test to confirm it now passes:
|
||||||
|
```bash
|
||||||
|
podman compose --profile tests run --rm tests go test -v -run "TestScanSettings_GetSettings/Get_settings_as_admin" ./cmd/server/tests/
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Step 0.5.10: Add Integration Tests
|
||||||
**File**: `cmd/server/tests/fsnotify_integration_test.go` (new)
|
**File**: `cmd/server/tests/fsnotify_integration_test.go` (new)
|
||||||
|
|
||||||
**Action**: Create integration tests using test_helpers:
|
**Action**: Create integration tests using test_helpers:
|
||||||
@@ -870,28 +1085,37 @@ podman compose --profile tests build
|
|||||||
|
|
||||||
**Commit Phase 0.5**:
|
**Commit Phase 0.5**:
|
||||||
```bash
|
```bash
|
||||||
git add internal/services/media_scanner.go internal/services/media_scanner_test.go cmd/server/tests/fsnotify_integration_test.go
|
git add internal/services/media_scanner.go internal/services/worker.go internal/services/media_scanner_test.go cmd/server/tests/fsnotify_integration_test.go cmd/server/tests/test_helpers.go cmd/server/tests/scan_settings_integration_test.go
|
||||||
git commit -m "fix: Replace file-based fsnotify with directory-based watching
|
git commit -m "fix: Replace file-based fsnotify with directory-based watching + job queue concurrency
|
||||||
|
|
||||||
Problem:
|
Problem:
|
||||||
- Event queue overflow drops events during bulk operations
|
- Event queue overflow drops events during bulk operations
|
||||||
- Only detects one file when adding 10-20 files
|
- Only detects one file when adding 10-20 files
|
||||||
- Delete detection doesn't work
|
- Delete detection doesn't work
|
||||||
- Docker environment exacerbates issues
|
- Docker environment exacerbates issues
|
||||||
|
- No concurrency control: Multiple scans can run simultaneously
|
||||||
|
- File stability race condition: Multiple goroutines check same file
|
||||||
|
- Unbounded goroutine spawn: Each directory scan spawns goroutine without limit
|
||||||
|
- Default value inconsistency: GetPollInterval() returns 30s instead of 60s
|
||||||
|
- Test isolation issues: Settings not restored after tests
|
||||||
|
|
||||||
Solution: Smart Hybrid Approach (inspired by Jellyfin + Audiobookshelf)
|
Solution: Smart Hybrid Approach + Job Queue Concurrency Control
|
||||||
- Watch directories (not individual files)
|
- Watch directories (not individual files)
|
||||||
- Track dirty directories with timestamps
|
- Track dirty directories with timestamps
|
||||||
- File stability check: wait for mtime to stabilize (Audiobookshelf approach)
|
- File stability check: wait for mtime to stabilize (Audiobookshelf approach)
|
||||||
- Smart event merging: consolidate parent/sibling/subpath (Jellyfin approach)
|
- Smart event merging: consolidate parent/sibling/subpath (Jellyfin approach)
|
||||||
- 10-second batch delay for processing (Audiobookshelf approach)
|
- 10-second batch delay for processing (Audiobookshelf approach)
|
||||||
- Semaphore limits concurrent directory scans to 10 (prevents resource exhaustion)
|
- Job queue for all directory scans: Serialized by worker pool (no concurrent scans)
|
||||||
- Keep polling for orphaned/deleted files
|
- JobTypeSetFolders: Async folder configuration via job queue
|
||||||
|
- Atomic file stability tracking: Prevents race conditions
|
||||||
|
- Fixed default value: 60s (matches handler/schema)
|
||||||
|
- Test isolation: Snapshot/restore system_settings
|
||||||
|
|
||||||
Research Insights:
|
Research Insights:
|
||||||
- Jellyfin: Uses 64KB buffer + smart merging + 45s ignore
|
- Jellyfin: Uses 64KB buffer + smart merging + 45s ignore
|
||||||
- Audiobookshelf: Uses mtime stability check + 10s batching
|
- Audiobookshelf: Uses mtime stability check + 10s batching
|
||||||
- Combined: Best of both approaches for Bookhoord
|
- Old Phase 1: Job queue as concurrency control mechanism
|
||||||
|
- Combined: Best of all approaches for Bookhoord
|
||||||
|
|
||||||
Changes:
|
Changes:
|
||||||
REMOVE:
|
REMOVE:
|
||||||
@@ -901,44 +1125,64 @@ REMOVE:
|
|||||||
- flushEventQueue() function
|
- flushEventQueue() function
|
||||||
|
|
||||||
ADD:
|
ADD:
|
||||||
- dirtyDirs map[string]time.Time
|
- dirtyDirs map[string]time.Time (directory tracking)
|
||||||
- dirtyDirsMu sync.RWMutex
|
- dirtyDirsMu sync.RWMutex (protects dirtyDirs)
|
||||||
- fileStability map[string]time.Time (mtime tracking)
|
- fileStability map[string]atomic.Bool (mtime tracking, atomic prevents races)
|
||||||
- fileStabilityMu sync.RWMutex
|
- fileStabilityMu sync.RWMutex (protects fileStability)
|
||||||
- scanSemaphore chan struct{} (limits concurrent scans to 10)
|
- worker *Worker (job queue reference for scan jobs)
|
||||||
- activeScans map[string]bool (prevents duplicate scans)
|
- watching atomic.Bool (prevents duplicate WatchChanges() calls)
|
||||||
- activeScansMu sync.Mutex (protects activeScans)
|
|
||||||
- watching atomic.Bool
|
|
||||||
- markDirectoryDirty() helper with smart merging (parent + sibling consolidation)
|
- markDirectoryDirty() helper with smart merging (parent + sibling consolidation)
|
||||||
- performInitialScan() (scans root folders on startup)
|
- processDirtyDirectories() (10-second batch, submits jobs to worker)
|
||||||
- processDirtyDirectories() (10-second batch with semaphore)
|
- waitForFileStability() (mtime polling, 3s interval, 60s timeout, uses atomic.Bool)
|
||||||
- waitForFileStability() (mtime polling, 3s interval, 60s timeout, proper cleanup)
|
- JobTypeSetFolders constant (async folder configuration)
|
||||||
- scanDirectory() (targeted scan with concurrency protection)
|
- processSetFoldersJob() handler (handles folder configuration asynchronously)
|
||||||
- Close() method (cleanup on shutdown)
|
- Test isolation in setupTestServer() (snapshot/restore system_settings)
|
||||||
|
|
||||||
Concurrency Fixes:
|
FIX:
|
||||||
- Semaphore prevents unbounded goroutine spawn (limits to 10 concurrent scans)
|
- waitForFileStability() no longer has double-unlock bug
|
||||||
- activeScans map prevents concurrent scans of same directory
|
- File stability uses atomic.Bool to prevent race conditions
|
||||||
- fileStability proper lock pattern prevents race conditions
|
- Default value fixed: GetPollInterval() returns 60s (was 30s)
|
||||||
- All map entries cleaned up on success, timeout, or error (no memory leaks)
|
- Test expectation fixed: 60 → 300 (5 min polling interval)
|
||||||
- Graceful shutdown waits for active scans or times out after 5 seconds
|
- Integration test timing: 5s → 12s (accounts for batch + processing)
|
||||||
|
|
||||||
|
Concurrency Control:
|
||||||
|
- Job queue serializes all directory scans (no concurrent access)
|
||||||
|
- No unbounded goroutine spawn (worker pool limits concurrency)
|
||||||
|
- Atomic file stability checks prevent duplicate entries
|
||||||
|
- No race conditions in fileStability map
|
||||||
|
- No memory leaks from orphaned map entries
|
||||||
|
- Folder configuration is async (non-blocking API)
|
||||||
|
|
||||||
Tests:
|
Tests:
|
||||||
- Unit: TestMarkDirectoryDirty, TestProcessDirtyDirectories,
|
- Unit: TestMarkDirectoryDirty, TestProcessDirtyDirectories,
|
||||||
TestWaitForFileStability, TestSmartEventMerging,
|
TestWaitForFileStability, TestSmartEventMerging
|
||||||
TestSiblingConsolidation
|
- Integration: TestFSNotify_BulkFileDetection (20 files, 12s wait)
|
||||||
- Integration: TestFSNotify_BulkFileDetection (20 files)
|
- Test Isolation: Snapshot/restore system_settings in setupTestServer()
|
||||||
|
- Test Expectation: Fixed to match 300s default polling interval
|
||||||
|
|
||||||
Benefits:
|
Benefits:
|
||||||
- No event queue overflow (directory watching eliminates per-file events)
|
- No event queue overflow (directory watching eliminates per-file events)
|
||||||
- Reliable bulk import with file stability checks
|
- Reliable bulk import with file stability checks
|
||||||
- Delete detection via polling
|
- Delete detection via polling
|
||||||
- Smart event consolidation reduces redundant scans
|
- Smart event consolidation reduces redundant scans
|
||||||
- Concurrent scans limited to 10 (prevents resource exhaustion)
|
- Job queue prevents concurrent scans (serialized by worker pool)
|
||||||
- No race conditions in fileStability map
|
- No race conditions in fileStability map (atomic operations)
|
||||||
- No memory leaks from orphaned map entries
|
- No memory leaks from orphaned map entries (proper cleanup)
|
||||||
|
- No unbounded goroutine spawn (worker pool limits)
|
||||||
- Works on Docker, network mounts
|
- Works on Docker, network mounts
|
||||||
- Initial scan ensures existing files are detected"
|
- Test isolation preserves dev database state
|
||||||
|
- Consistent default values across all components
|
||||||
|
- Works on Docker, network mounts
|
||||||
|
- Test isolation preserves dev database state
|
||||||
|
- Consistent default values across all components
|
||||||
|
|
||||||
|
Files modified:
|
||||||
|
- internal/services/media_scanner.go (directory watching, file stability, atomic tracking)
|
||||||
|
- internal/services/worker.go (JobTypeSetFolders, processSetFoldersJob)
|
||||||
|
- internal/services/media_scanner_test.go (unit tests)
|
||||||
|
- cmd/server/tests/fsnotify_integration_test.go (bulk file detection test)
|
||||||
|
- cmd/server/tests/test_helpers.go (test isolation: snapshot/restore)
|
||||||
|
- cmd/server/tests/scan_settings_integration_test.go (fix test expectation)"
|
||||||
```
|
```
|
||||||
|
|
||||||
## Phase 1: Job Queue Expansion (6-8 hours)
|
## Phase 1: Job Queue Expansion (6-8 hours)
|
||||||
|
|||||||
Reference in New Issue
Block a user