v2.8.5: oversized-image false positives, worker DB pool, frame check - #81
Merged
Conversation
…corruption Valid images above Pillow's decompression-bomb threshold (~358MP) or ImageMagick's area policy (256MP default) were reported corrupted: PIL deferred to ImageMagick, whose 'cache resources exhausted' exit fell into the generic corruption branch. Both guards are resource limits, not corruption evidence, so they now produce a warning verdict following the existing HEIC libheif pattern. Also re-indents a comment block that visually straddled the else-branch boundary around the transpose call.
…red StaticPool) The parallel file-list scan path fans a single PixelProbe instance across a ThreadPoolExecutor (default num_workers=4), so every worker's cache check and result save went through one StaticPool connection. psycopg2 forbids concurrent use of one connection from multiple threads; this surfaced as sporadic save failures. The worker engine now uses a QueuePool sized to the worker count (pool_size=num_workers, max_overflow=2, pre-ping), and the SQLite test branch allows cross-thread use explicitly. Adds a postgres- marked regression test that fails against the old StaticPool code.
…arning The Stage 1 check paid a full sequential -count_frames decode on every video, then discarded the result: ffprobe orders csv columns by section layout, so the parser's expected column count never matched ffmpeg 8 output and the check has been silently inert. The rewrite probes nb_read_packets first (no decode), confirms an ambiguous count with the full decode only when needed, parses order-independent key=value output, uses avg_frame_rate instead of r_frame_rate, and falls back to the format duration for Matroska streams that report N/A. A confirmed mismatch now produces a warning, not a corruption verdict: container framerate metadata lies on sparse-video files (a 240s QuickTime fixture with 244 real frames declares 25fps and would have scored 95.9% 'frame loss'), and real decode damage is already caught by the err_detect deep-decode stage.
…task path) pixelprobe/services/scan_executor.py had no production callers; its only consumers were its own tests, which validated a code path the application never runs. tasks.py documents the Celery task queue as its replacement.
valid.3gp, valid.flv, valid.mpg, and valid.wmv (plus their format symlinks) were 189-byte HTML error pages committed from failed downloads; valid.mkv failed h264 decode under ffmpeg 8 and valid.webp failed both PIL and ImageMagick. Nothing noticed because CI deselects real_media and the valid-file assertion only covered mp4/jpg/png/wav. All are replaced with small genuine files synthesized by the committed generate_corrupted_fixtures.py script, which also derives detection- verified corrupted counterparts for mkv, opus, wmv, heic, heif, 3gp, flv, and mpg, closing the missing-counterpart gap. corrupted.mpg is documented as warning-level only: MPEG-1 decoders conceal even heavy scattered damage and exit cleanly.
…st assertions - real_scan_results rescanned every fixture for every test and blew each test's 60s timeout, so the real_media suite could never complete (CI deselects the marker, so nothing noticed). Scans now run once per session and each test inserts the cached results into its own database. - New tests/test_synthetic_corruption.py: header/mid/tail truncation, zero-byte files, PNG-renamed-to-JPG format confusion, scattered mdat damage, SVG/PSD/progressive-JPEG/animated-WebP coverage, and a symlink loop discovery test. Fixtures are generated at test time from the committed samples. - Removed the scan_status == 'completed' clause that made three corruption assertions tautological. That exposed six 'corrupted' fixtures (mp3, aiff, jpg, png, gif, bmp) that modern decoders accept without error - the FFmpeg-bug-tracker bugs they exercised were in FFmpeg, not the files. Replaced with detection-verified damage derived from the valid samples (gif stays warning-level: GIF header issues are deliberately demoted). - Added synthesized valid.aiff (no valid AIFF fixture existed).
…suite on 3.12 The real_media marker was always deselected in CI, which is how HTML error pages survived as 'valid' fixtures and how tautological assertions went unnoticed. The suite now runs on one matrix leg. The postgres service backs the new StaticPool-regression concurrency test.
- ffprobe in _probe_stream_counts wraps the path with ensure_cli_safe_path like every other call site (a leading-dash filename silently skipped the check otherwise) - Pillow bomb guard matched by exception type instead of message substring - worker pool max_overflow raised to 10 so an unsized caller degrades to on-demand connections instead of a checkout timeout; engines disposed deterministically when scan threads finish - confirm decode now triggers at the same 5% threshold as the warning (a 1-5% packet diff bought a full decode that could never change the verdict) and the mismatch math lives in one helper - test-time damage recipes import the committed generator's scattered_bytes instead of carrying a copy; tests reuse IMAGEMAGICK_BINARY; twelve identical timeout decorators collapsed to one module mark; SECRET_KEY hoisted to job env in CI
- postgresql+psycopg2:// URLs (accepted by app.py's DATABASE_URL check) fell into the worker engine's SQLite branch, where the check_same_thread connect arg is invalid for psycopg2 and engine init failed - an oversized image that outlasts the size-scaled ImageMagick timeout was still marked corrupted because PIL's bomb guard counted as a PIL failure; the guard now overrides the PIL-failed signal in the timeout and empty-stderr verdicts - caller-supplied num_workers on /api/scan-files-parallel is clamped to MAX_WORKERS; it sizes thread pools and the checker's DB connection pool, so an uncapped value translated directly into PostgreSQL connections
…image path - skip the ImageMagick convert entirely when Pillow's bomb guard fired: the shipped image's IM policy is below the bomb threshold, so the convert could only grind its pixel cache before failing at a resource limit, and any unrecognized resource-error phrasing re-created the false positive - frame check measures against the video stream's own duration (format duration only as the Matroska fallback), so containers carrying audio longer than the video track are not flagged and do not pay a decode on every scan - a failed confirm decode degrades the packet-count evidence to a warning instead of discarding it; a probe timeout surfaces as Stage 1 INCOMPLETE instead of PASSED - worker pool uses pool_recycle instead of per-checkout pre-ping (two checkouts per file made pings real traffic at library scale) - restored @pytest.mark.slow lost from test_sustained_load; bomb-scale test shrunk to 625MP so CI runners do not allocate multiple GB; real_media module timeout covers first-touch corpus scanning; inline imports hoisted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from a full code review and corruption-detection test gap analysis. Three production defects, one dead module, and a repaired test corpus, all in one release.
Fixed
Removed
Tests
Version
2.8.5
Test plan