Skip to content

Commit 4271740

Browse files
authored
Single-file rescan UX, post-fork DB engine disposal, dotenv CVE (v2.6.41) (#56)
Fix the UI flickering to "done" mid-rescan and the silent first-attempt NotImplementedError on Celery worker scans. - scan_service.scan_single_file accepts an optional scan_id and reuses the existing ScanState row when one matches. The Celery scan_media_task passes scan_id through for scan_type='single'. Eliminates the second ScanState that the UI's progress monitor lost track of. - worker_process_init signal now disposes the SQLAlchemy engine in each forked Celery worker, so post-fork libpq sockets aren't shared with the parent process. Removes the recurring PGRES_TUPLES_OK / NotImplementedError pattern in the worker logs. - scan_media_task catch-all preserves the ScanState row across retries (phase='initializing', is_active=True with a 'Retrying after error' progress message) instead of marking it failed/inactive on every attempt. - Extract the corruption-error string check into is_db_connection_corruption in pixelprobe/utils/celery_utils.py and use it at both detection sites. - Replace stringly-typed scan phase constants with SCAN_PHASES['INITIALIZING']. - Bump python-dotenv 1.0.0 -> 1.2.2 for CVE-2026-28684 (MEDIUM, symlink arbitrary file overwrite). New regression test: test_scan_single_file_reuses_existing_scan_state. All 320 tests pass; trivy clean of HIGH/CRITICAL/MEDIUM at the app layer.
1 parent c493af3 commit 4271740

8 files changed

Lines changed: 134 additions & 18 deletions

File tree

CHANGELOG.MD

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,18 @@ All notable changes to this project will be documented in this file.
55
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
66
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0).
77

8+
## [2.6.41] - 2026-04-26
9+
10+
### Security
11+
12+
- **Bump `python-dotenv` from 1.0.0 to 1.2.2** to resolve CVE-2026-28684 (arbitrary file overwrite via symlink follow). MEDIUM severity. The pinned 1.0.0 had no fix available; 1.2.2 is the first release with the patch.
13+
14+
### Fixed
15+
16+
- **Single-file rescan UI flips to "done" then resumes minutes later**. The Flask route created a `ScanState` row keyed on its generated `scan_id`, then the Celery worker's `scan_service.scan_single_file` created a *second* `ScanState` with a different `scan_id` and tracked progress on that one. The UI's progress monitor lost track between the two rows and reported the scan complete before the worker had even started hashing the file. `scan_single_file` now accepts an optional `scan_id` and reuses the existing row when one matches; the Celery `scan_media_task` passes `scan_id` through for `scan_type='single'`.
17+
- **Silent first-attempt failures on Celery worker scans** caused by post-fork PostgreSQL connection sharing. Symptom in logs: `psycopg2.DatabaseError: error with status PGRES_TUPLES_OK and no message from the libpq` on one worker, surfacing as a bare `NotImplementedError` from `sqlalchemy/engine/result.py:_indexes_for_keys` in a sibling worker. Fixed by disposing the SQLAlchemy engine in the `worker_process_init` Celery signal so each forked child builds its own connection pool. The existing log-handler setup in that signal has been merged into a single `_setup_worker_process` handler.
18+
- **Scan progress flickers to "failed" during transient Celery retries**. `scan_media_task`'s catch-all exception handler used to set `phase='failed'` and `is_active=False` on every error, including ones that were about to be retried. The handler now keeps the row active (`phase='initializing'` with a "Retrying after error" progress message) when more retries remain, and only marks the scan failed once the retry budget is exhausted or the error is the known-fatal `PGRES_TUPLES_OK` connection-corruption case.
19+
820
## [2.6.40] - 2026-04-20
921

1022
### Security

pixelprobe/celery_config.py

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -157,16 +157,25 @@ def init_celery(app, celery):
157157

158158

159159
@worker_process_init.connect
160-
def _setup_db_log_handler_in_worker(**kwargs):
161-
"""Attach DatabaseLogHandler in each forked Celery worker child process.
162-
163-
Threads don't survive fork(), so the handler set up in the parent process
164-
has a dead _writer_thread in children. This signal fires once per child
165-
and creates a fresh handler with its own background writer thread.
160+
def _setup_worker_process(**kwargs):
161+
"""Initialize each forked Celery worker child process.
162+
163+
1. Dispose the inherited SQLAlchemy engine so each child builds a fresh
164+
connection pool. Without this, child processes share libpq sockets
165+
with the parent, which surfaces as "PGRES_TUPLES_OK and no message
166+
from the libpq" - a NotImplementedError when concurrent SQLAlchemy
167+
queries try to read a row whose cursor was torn out from under them.
168+
2. Attach a fresh DatabaseLogHandler. The handler set up in the parent
169+
process has a dead _writer_thread because threads don't survive
170+
fork(); this signal fires once per child and replaces it.
166171
"""
167172
from app import app
173+
from pixelprobe.models import db
168174
from pixelprobe.utils.log_handler import DatabaseLogHandler
169175

176+
with app.app_context():
177+
db.engine.dispose()
178+
170179
handler = DatabaseLogHandler(app)
171180
handler.setLevel(logging.INFO)
172181
logging.getLogger().addHandler(handler)

pixelprobe/services/scan_service.py

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from typing import List, Dict, Optional, Tuple
1313

1414
from flask import current_app
15+
from pixelprobe.constants import SCAN_PHASES
1516
from pixelprobe.media_checker import PixelProbe, load_exclusions, load_exclusions_with_patterns
1617
from pixelprobe.models import db, ScanResult, ScanState, ScanReport, ScanChunk
1718
from pixelprobe.utils.helpers import ProgressTracker
@@ -92,8 +93,16 @@ def update_progress(self, current: int, total: int, file_path: str, status: str)
9293
except Exception as e:
9394
logger.debug(f"Failed to update Redis progress: {e}")
9495

95-
def scan_single_file(self, file_path: str, force_rescan: bool = False) -> Dict:
96-
"""Scan a single file"""
96+
def scan_single_file(self, file_path: str, force_rescan: bool = False,
97+
scan_id: Optional[str] = None) -> Dict:
98+
"""Scan a single file.
99+
100+
When ``scan_id`` is provided (e.g., the API route created a ScanState
101+
before queueing the Celery task), reuse that row so the UI tracks one
102+
continuous scan from queued through completed. Without this, a second
103+
ScanState is created here and the UI's progress monitor briefly sees
104+
no active scan and flips to "done" before the new row appears.
105+
"""
97106
if not os.path.exists(file_path):
98107
raise FileNotFoundError(f"File not found: {file_path}")
99108

@@ -104,13 +113,29 @@ def scan_single_file(self, file_path: str, force_rescan: bool = False) -> Dict:
104113
self.update_progress(0, 1, file_path, 'scanning')
105114
self.scan_cancelled = False
106115

107-
# Create ScanState record for UI progress tracking
108-
scan_state = ScanState.create_new_scan()
109-
scan_state.start_scan([file_path], force_rescan)
110-
scan_state.phase = 'initializing'
116+
scan_state = None
117+
if scan_id:
118+
scan_state = ScanState.query.filter_by(scan_id=scan_id).first()
119+
120+
if scan_state is None:
121+
scan_state = ScanState.create_new_scan(scan_id=scan_id)
122+
123+
# Apply single-file initialization fields directly. We avoid
124+
# ScanState.start_scan() here because it commits eagerly and sets
125+
# phase='discovering', which we'd immediately overwrite.
126+
now = datetime.now(timezone.utc)
127+
scan_state.is_active = True
128+
scan_state.phase = SCAN_PHASES['INITIALIZING']
111129
scan_state.progress_message = 'Initializing single file scan'
112130
scan_state.estimated_total = 1
113131
scan_state.phase_total = 1
132+
scan_state.files_processed = 0
133+
scan_state.directories = json.dumps([file_path])
134+
scan_state.force_rescan = force_rescan
135+
scan_state.error_message = None
136+
scan_state.start_time = now
137+
scan_state.last_update = now
138+
scan_state.end_time = None
114139
db.session.commit()
115140

116141
# Capture scan ID for UI progress tracking

pixelprobe/tasks.py

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,11 @@
1515
from contextlib import contextmanager
1616

1717
from pixelprobe.celery_config import celery_app
18+
from pixelprobe.constants import SCAN_PHASES
1819
from pixelprobe.services.scan_service import ScanService
1920
from pixelprobe.progress_utils import get_redis_client, get_scan_progress_redis, update_scan_progress_redis
2021
from pixelprobe.models import db, ScanState, ScanResult, ScanReport
22+
from pixelprobe.utils.celery_utils import is_db_connection_corruption
2123
from pixelprobe.utils.log_context import current_scan_id, current_celery_task_id
2224

2325

@@ -273,9 +275,14 @@ def progress_callback(progress_data):
273275
elif scan_type == 'single':
274276
# Single file scan
275277
if paths and len(paths) == 1:
278+
# Pass scan_id so scan_service reuses the ScanState row this task
279+
# is already tracking, instead of creating a second one. Otherwise
280+
# the UI's progress monitor sees a brief gap between rows and
281+
# flips to "done" before the real scan starts.
276282
result = scan_service.scan_single_file(
277283
file_path=paths[0],
278-
force_rescan=force_rescan
284+
force_rescan=force_rescan,
285+
scan_id=scan_id
279286
)
280287

281288
# CRITICAL: Commit Flask-SQLAlchemy session to ensure ScanService changes are visible
@@ -309,6 +316,15 @@ def progress_callback(progress_data):
309316
is_db_error = isinstance(exc, (sqlalchemy.exc.DatabaseError, psycopg2.DatabaseError))
310317
is_connection_error = isinstance(exc, (sqlalchemy.exc.OperationalError, psycopg2.OperationalError))
311318

319+
# Decide whether this exception will be retried so we can preserve the
320+
# ScanState row across attempts. If we marked it failed/inactive on
321+
# every transient error, the UI would flip to "done" during the retry
322+
# window and only re-discover the scan minutes later.
323+
is_corruption_error = is_db_error and is_db_connection_corruption(exc)
324+
will_retry = (
325+
self.request.retries < self.max_retries and not is_corruption_error
326+
)
327+
312328
# Update scan state with error
313329
try:
314330
# Roll back any pending transaction before querying
@@ -318,8 +334,17 @@ def progress_callback(progress_data):
318334
if scan_state:
319335
error_msg = f"Celery task failed: {str(exc)}"
320336
scan_state.error_message = error_msg[:950] # Truncate to fit VARCHAR(1000)
321-
scan_state.is_active = False
322-
scan_state.phase = 'failed'
337+
if will_retry:
338+
# Keep the row active so the UI keeps showing progress
339+
# during the retry backoff instead of jumping to "done".
340+
scan_state.phase = SCAN_PHASES['INITIALIZING']
341+
scan_state.progress_message = (
342+
f'Retrying after error '
343+
f'(attempt {self.request.retries + 1}/{self.max_retries})'
344+
)
345+
else:
346+
scan_state.is_active = False
347+
scan_state.phase = 'failed'
323348
db.session.commit()
324349
except Exception as db_exc:
325350
logger.error(f"Failed to update scan state with error: {str(db_exc)}")
@@ -340,7 +365,7 @@ def progress_callback(progress_data):
340365
elif is_db_error:
341366
logger.error(f"Database error detected: {type(exc).__name__}")
342367
# Don't retry immediately for database corruption errors
343-
if "PGRES_TUPLES_OK" in str(exc) or "no message from the libpq" in str(exc):
368+
if is_db_connection_corruption(exc):
344369
logger.error(f"Database connection corruption detected - task {self.request.id} failed permanently")
345370
raise exc
346371
else:

pixelprobe/utils/celery_utils.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,14 @@ def check_celery_available():
2323
celery_enabled = False
2424

2525
return celery_enabled
26+
27+
28+
def is_db_connection_corruption(exc) -> bool:
29+
"""Detect post-fork PostgreSQL connection corruption.
30+
31+
Surfaces as "PGRES_TUPLES_OK and no message from the libpq" when a forked
32+
worker inherits and uses a parent's libpq socket. The connection is dead;
33+
retrying the same task on the same connection will not help.
34+
"""
35+
msg = str(exc)
36+
return "PGRES_TUPLES_OK" in msg or "no message from the libpq" in msg

pixelprobe/version.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
# Default version - this is the single source of truth
55

66

7-
_DEFAULT_VERSION = '2.6.40'
7+
_DEFAULT_VERSION = '2.6.41'
88

99

1010
# Allow override via environment variable for CI/CD, but default to the hardcoded version

requirements.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ pillow-heif==0.22.0 # HEIC/HEIF support for Pillow (requires libheif system lib
99
python-magic==0.4.27
1010
ffmpeg-python==0.2.0
1111
SQLAlchemy==2.0.41
12-
python-dotenv==1.0.0
12+
python-dotenv==1.2.2
1313
Werkzeug==3.1.6
1414
gunicorn==23.0.0
1515
pytz==2023.3

tests/unit/test_scan_service.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,40 @@ def test_scan_single_file_not_found(self, scan_service):
9292
"""Test scanning non-existent file"""
9393
with pytest.raises(FileNotFoundError):
9494
scan_service.scan_single_file('/nonexistent/file.mp4')
95+
96+
@patch('os.path.exists')
97+
@patch('pixelprobe.services.scan_service.PixelProbe')
98+
def test_scan_single_file_reuses_existing_scan_state(self, mock_probe_class, mock_exists,
99+
scan_service, app, db):
100+
"""Single-file scan reuses an existing ScanState row when scan_id is passed.
101+
102+
Regression test for the v2.6.41 UI flicker bug: the API route created a
103+
ScanState before queueing the Celery task, then scan_single_file created
104+
a *second* row with a different scan_id and the UI lost track in between.
105+
"""
106+
with app.app_context():
107+
mock_exists.return_value = True
108+
mock_probe_class.return_value.scan_file.return_value = Mock()
109+
110+
existing = ScanState.create_new_scan(scan_id='route-scan-id')
111+
existing.start_scan(['/test/file.mp4'], force_rescan=True)
112+
existing.is_active = False # Simulate post-failure state pre-retry
113+
existing.phase = 'failed'
114+
db.session.commit()
115+
existing_id = existing.id
116+
117+
scan_service.scan_single_file('/test/file.mp4', force_rescan=True,
118+
scan_id='route-scan-id')
119+
120+
rows = ScanState.query.filter_by(scan_id='route-scan-id').all()
121+
assert len(rows) == 1
122+
assert rows[0].id == existing_id
123+
assert rows[0].is_active is True
124+
assert rows[0].phase == 'initializing'
125+
assert rows[0].error_message is None
126+
127+
if scan_service.current_scan_thread:
128+
scan_service.current_scan_thread.join(timeout=1)
95129

96130
@patch('os.path.exists')
97131
@patch('pixelprobe.services.scan_service.PixelProbe')

0 commit comments

Comments
 (0)