From f2e511481301ccf66e61609d63537bf6d9f2fa22 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Thu, 5 Mar 2026 13:04:05 -0500 Subject: [PATCH] docs: Fix critical inconsistencies in Phase 0.5 plan Fixed issues identified during review: 1. Commit message accuracy (lines 1207, 1209): - Changed 'worker *Worker (job queue reference)' to 'WorkerInstance *Worker global (no circular dependency)' - Changed 'fileStability map[string]atomic.Bool' to 'fileStability map[string]*atomic.Bool (pointer)' - Removed claim that worker field was ADDED (it was REMOVED in clean rewrite) 2. Polling interval consistency (60s chosen): - Constructor: 60s (correct, no change) - Test: Changed from expecting 300s to 60s - Commit message: Changed all references from 300s to 60s - Benefits: 'Delete detection via 60s polling (fast safety net)' - Rationale: Real-time fsnotify + 60s polling = best UX 3. Added Step 0.5.3.8: Initialize WorkerInstance in main(): - Previously buried as inline comment in Step 0.5.3.7 - Now dedicated step with file location (cmd/server/main.go) - Critical for system initialization 4. Removed duplicate benefits lines: - Lines 1252-1254 were duplicates of 1249-1251 5. Updated 'Code to ADD' section: - Clarified '*atomic.Bool (pointer to atomic.Bool, not value type)' - Clarified 'WorkerInstance *Worker global (no circular dependency)' - Added 'JobTypeDirectoryScan' to constants list 6. Updated Files modified section: - Added cmd/server/main.go (initialize WorkerInstance) - Clarified worker.go changes (JobTypeDirectoryScan, processDirectoryScanJob, WorkerInstance, Enqueue) - Changed scan_settings_integration_test.go description to 'test expects 60s polling' 7. Enhanced Concurrency Control section: - Added 'No circular dependency (WorkerInstance global)' Result: Plan now accurately reflects clean architecture approach with 60s polling. --- COMPLETE_INFRASTRUCTURE_ENHANCEMENT_PLAN.md | 77 ++++++++++++++------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/COMPLETE_INFRASTRUCTURE_ENHANCEMENT_PLAN.md b/COMPLETE_INFRASTRUCTURE_ENHANCEMENT_PLAN.md index 612279d..b633c93 100644 --- a/COMPLETE_INFRASTRUCTURE_ENHANCEMENT_PLAN.md +++ b/COMPLETE_INFRASTRUCTURE_ENHANCEMENT_PLAN.md @@ -64,22 +64,22 @@ Watch directories (not individual files) with file stability checks + smart even **Code to ADD:** - `dirtyDirs map[string]time.Time` field - tracks directories pending scan - `dirtyDirsMu sync.RWMutex` field - protects dirtyDirs map -- `fileStability map[string]*atomic.Bool` field - tracks files waiting for mtime stabilization (pointer to atomic, prevents races) +- `fileStability map[string]*atomic.Bool` field - tracks files waiting for mtime stabilization (pointer to atomic.Bool, not value type) - `fileStabilityMu sync.RWMutex` field - protects fileStability map - `scan_mutex sync.Mutex` field - prevents concurrent scans (simple, effective, works with job queue) - `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 global WorkerInstance -- `waitForFileStability()` function - polls mtime until stable (uses atomic.Bool, no race condition) +- `waitForFileStability()` function - polls mtime until stable (uses *atomic.Bool, no race condition) - `performInitialScan()` function - scans all root folders on startup (submitted as jobs, not direct calls) - `JobTypeDirectoryScan` constant - for directory scanning via job queue - `processDirectoryScanJob()` function in Worker - handles directory scan jobs -- `WorkerInstance *Worker` global variable - provides access to worker for MediaScanner +- `WorkerInstance *Worker` global variable - provides access to worker for MediaScanner (no circular dependency) - `Enqueue()` method in Worker - non-blocking job submission with safe fallback - `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) +- Fix GetPollInterval() default value: 30s → 60s (matches real-time expectations) - Unit tests in `internal/services/media_scanner_test.go` - Integration tests in `cmd/server/tests/fsnotify_integration_test.go` @@ -405,6 +405,30 @@ go worker.Start(context.Background()) --- +### Step 0.5.3.8: Initialize WorkerInstance in main() +**File**: `cmd/server/main.go` + +**Location**: In main() function, where worker is created (around line 50-100) + +**Action**: Initialize global WorkerInstance after creating worker: + +```go +// Find existing worker creation +worker := NewWorker(numWorkers, connManager, db) + +// Add this line immediately after +WorkerInstance = worker + +// Existing code +go worker.Start(ctx) +``` + +**Why**: MediaScanner needs access to worker for submitting directory scan jobs. Global variable is set during initialization, avoiding circular dependency between scanner and worker. + +**Verification**: Run `podman compose build` to ensure server starts correctly. + +--- + ### Step 0.5.4: Add markDirectoryDirty() Helper with Smart Event Merging **File**: `internal/services/media_scanner.go` @@ -1019,7 +1043,7 @@ t.Cleanup(func() { --- -### Step 0.5.12: Fix Test Expectation (60 → 300) +### Step 0.5.12: Fix Test Expectation (Keep 60s) **File**: `cmd/server/tests/scan_settings_integration_test.go` **Location**: Line 34 @@ -1029,13 +1053,12 @@ t.Cleanup(func() { assert.Equal(t, float64(60), response["scan_poll_interval_seconds"]) ``` -**Action**: Change expectation to 300 (5 minutes): - +**Action**: Keep expectation at 60 seconds (no change needed): ```go -assert.Equal(t, float64(300), response["scan_poll_interval_seconds"]) +assert.Equal(t, float64(60), 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. +**Why**: Real-time fsnotify watching + 60s polling provides fast feedback. Test already expects 60s, which is correct. **Verification**: Run the failing test to confirm it now passes: ```bash @@ -1204,22 +1227,24 @@ REMOVE: ADD: - dirtyDirs map[string]time.Time (directory tracking) - dirtyDirsMu sync.RWMutex (protects dirtyDirs) -- fileStability map[string]atomic.Bool (mtime tracking, atomic prevents races) +- fileStability map[string]*atomic.Bool (mtime tracking, pointer to atomic.Bool prevents races) - fileStabilityMu sync.RWMutex (protects fileStability) -- worker *Worker (job queue reference for scan jobs) +- scan_mutex sync.Mutex (prevents concurrent scans) - watching atomic.Bool (prevents duplicate WatchChanges() calls) - markDirectoryDirty() helper with smart merging (parent + sibling consolidation) -- processDirtyDirectories() (10-second batch, submits jobs to worker) -- waitForFileStability() (mtime polling, 3s interval, 60s timeout, uses atomic.Bool) +- processDirtyDirectories() (10-second batch, submits jobs to WorkerInstance) +- waitForFileStability() (mtime polling, 3s interval, 60s timeout, uses *atomic.Bool) +- JobTypeDirectoryScan constant (directory scanning via job queue) +- processDirectoryScanJob() handler (handles directory scan jobs in worker) +- WorkerInstance *Worker global (no circular dependency between scanner and worker) +- Enqueue() method in Worker (non-blocking job submission) - JobTypeSetFolders constant (async folder configuration) - processSetFoldersJob() handler (handles folder configuration asynchronously) - Test isolation in setupTestServer() (snapshot/restore system_settings) FIX: -- waitForFileStability() no longer has double-unlock bug -- File stability uses atomic.Bool to prevent race conditions -- Default value fixed: GetPollInterval() returns 60s (was 30s) -- Test expectation fixed: 60 → 300 (5 min polling interval) +- File stability uses *atomic.Bool (pointer) to prevent race conditions +- Default value fixed: GetPollInterval() returns 60s (real-time + fast safety net) - Integration test timing: 5s → 12s (accounts for batch + processing) Concurrency Control: @@ -1229,37 +1254,37 @@ Concurrency Control: - No race conditions in fileStability map - No memory leaks from orphaned map entries - Folder configuration is async (non-blocking API) +- No circular dependency (WorkerInstance global instead of struct field) Tests: - Unit: TestMarkDirectoryDirty, TestProcessDirtyDirectories, TestWaitForFileStability, TestSmartEventMerging - Integration: TestFSNotify_BulkFileDetection (20 files, 12s wait) - Test Isolation: Snapshot/restore system_settings in setupTestServer() -- Test Expectation: Fixed to match 300s default polling interval +- Test Expectation: 60s polling interval (real-time + fast safety net) Benefits: - No event queue overflow (directory watching eliminates per-file events) - Reliable bulk import with file stability checks -- Delete detection via polling +- Delete detection via 60s polling (fast safety net) - Smart event consolidation reduces redundant scans - Job queue prevents concurrent scans (serialized by worker pool) - No race conditions in fileStability map (atomic operations) - No memory leaks from orphaned map entries (proper cleanup) - No unbounded goroutine spawn (worker pool limits) +- No circular dependency (WorkerInstance global) - Works on Docker, network mounts - 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 +- Consistent 60s polling interval 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.go (directory watching, file stability, atomic tracking, scan_mutex) +- internal/services/worker.go (JobTypeSetFolders, JobTypeDirectoryScan, processSetFoldersJob, processDirectoryScanJob, WorkerInstance, Enqueue) +- cmd/server/main.go (initialize WorkerInstance) - 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)" +- cmd/server/tests/scan_settings_integration_test.go (test expects 60s polling)" ``` ## Phase 1: Job Queue Expansion (6-8 hours)