Bookhoard/bookhoard
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 12 Wiki Activity

docs: Fix critical inconsistencies in Phase 0.5 plan

Browse Source 
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.
This commit is contained in:
john-okeefe
2026-03-05 13:04:05 -05:00
parent 1a552e03f1
commit f2e5114813
1 changed files with 51 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
COMPLETE_INFRASTRUCTURE_ENHANCEMENT_PLAN.md
+51 -26
View File
@@ -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)