Audit remediation + reliability fixes (v2.6.49-v2.6.57)#61
Merged
Conversation
Converge directory scanning onto the chunk-distributed engine: /api/scan splits scans into disjoint path-range chunks fanned out across all Celery workers, with last-chunk-finalizes completion under a row lock and the stuck-scan sweeper as backstop. The thread-pool engine internals and the never-registered parallel task path are removed. Fixes from the external audit (21 findings): - Scan claim leaks on validation failure (validate before claim, release on failed launch) - UI heartbeat task occupying a worker slot for entire scans (deleted; chunks update progress every 100 files or 60s) - Sibling-directory over-match in path exclusions and LIKE prefix queries (boundary-safe is_path_under/like_prefix helpers across 14 sites) - Per-file DB round-trips in chunk scanning (bulk UPDATE...RETURNING claim) - Argument-injection hardening for ffprobe/ImageMagick invocations - Unauthenticated /healthz liveness probe (compose healthcheck was always 401) - CORS now opt-in via CORS_ORIGINS; session cookies default Secure - Rate limits moved to Redis (were per-worker memory) - Connection pool sized to fit PostgreSQL max_connections - Compose: datastore ports unpublished, POSTGRES_PASSWORD required, image tag parameterized - Dead code removed: broken DatabaseConnectionManager, stub cleanup task, unscheduled tasks, scheduler commit-in-loop and add-then-modify fixes
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The user-suppliable source field flowed verbatim into scan_id and was echoed in API responses, reports, and healthcheck routing (CodeQL py/reflective-xss). The id is now rebuilt from a strict format match; anything else gets a UUID. The literal scheduled_periodic label is preserved for the default periodic scan.
Cleanup Phase 2 had no task-abandonment timeout: un-ready handles were retried forever, pinning the loop at max concurrency with no log output (observed live: frozen at 20,000/1,187,442 files with idle workers). Progress also only wrote when the counter hit an exact multiple of 100, so the UI froze even while the loop was healthy. Mirrors the file-changes hardening: submitted_at tracking with abandonment after CLEANUP_TASK_TIMEOUT_SECS (default 600s; abandoned files counted unverifiable, never deleted), 10s heartbeat write+log, and delta-based progress writes.
An app-only container update wedged startup: the v2.6.0 migration ran an unconditional ALTER TABLE ... SET DEFAULT on every boot, which blocked on an ACCESS EXCLUSIVE lock held open by the still-running worker container, and every gunicorn worker waited behind the migration step forever. The app service also had no restart policy, so a dead boot stayed dead. - All migration connections set session lock_timeout (10s) and statement_timeout (300s), env-tunable; a blocked migration logs and is retried on the next boot instead of hanging - v2.6.0 SET DEFAULT is conditional on the defaults actually missing - pixelprobe service: restart unless-stopped, healthcheck start_period 120s to cover migration time - Accurate logging for CREATE TABLE IF NOT EXISTS paths
- Migration timeouts use SET LOCAL: a plain SET is session-scoped and survives the connection's return to the pool, imposing migration timeouts on unrelated app queries - Advisory-lock waiter bounded by the statement timeout (last remaining infinite-wait path in startup) - abandon_if_stuck() shared by cleanup and file-changes loops (the two inline copies had already drifted) - env_int() utility replaces three divergent env-parsing styles - Cleanup progress writes are heartbeat-only (per-100-file delta was ~12k commits per run with no UI benefit) - tools/ migration helper imported at top level (no import cycle exists) - Abandonment tests exercise the real helper instead of a local copy
ScanResult.query.all() materialized 1.18M full ORM objects (multiple GB) inside the gunicorn worker hosting the cleanup thread - the most likely driver of the silent-stall symptoms: memory pressure makes every loop iteration glacial, the producer looks blocked, cancel windows are missed, and nothing logs. The loop reads exactly id and file_path, so Phase 1 now loads lean tuples via with_entities (~20-30x less memory). The file-changes flow already did this; cleanup was the only full-ORM holdout.
Maintenance task results were never forgotten and expire only after 24h: a full-library run left 1M+ result keys in the noeviction Redis, slowing every broker/backend operation ~100x in the final stretch (heartbeats minutes apart, misleading '0 active tasks' and 'ETA: 0m'). Both maintenance loops now forget each result after consuming it and on abandonment, bounding Redis to in-flight tasks only. Cleanup and file-changes reports store file lists (objects) in the directories field; the report-details modal rendered them as '[object Object]' rows. It now shows labeled, escaped file lists (Orphaned Files / Changed Files) capped at 100 entries. Sub-minute cleanup ETA reads '<1m' instead of '0m'.
Per-task INFO rows from celery.app.trace generated heavy WAL volume during maintenance runs, driving PostgreSQL checkpoint IO storms that stalled the producer loops. Added to the default log-exclusion list with an additive startup migration backfilling existing installs.
…s (v2.6.54) Celery's Redis result backend subscribes the producer to each task's result channel at dispatch and again in AsyncResult.get(); the cleanup and integrity producer loops never read that socket, so published results accumulated until Redis force-closed the connection, freezing the loops for minutes with idle workers. Cancel the dispatch-time subscription right after apply_async and consume results via the stored meta (task.state/task.result) instead of get().
…URL (v2.6.55) apply_async subscribes the producer to each task's result channel via backend.on_task_call; nothing in the producer reads that socket, so the subscribe/unsubscribe replies accumulated until Redis force-closed the connection every ~3 minutes during maintenance runs. The hook is now a no-op for app instances (producers consume stored result meta), and the per-dispatch unsubscribe from v2.6.54 is removed as redundant. Also mirror CELERY_RESULT_BACKEND as an uppercase config attribute so Flask copies it into app.config - the app-bound Celery instance was silently defaulting its result backend to localhost in containers, breaking AsyncResult state checks in scan routes and the scheduler.
….6.56) v2.6.55 added an uppercase CELERY_RESULT_BACKEND attribute to Config; Celery treats that name as an old-style setting key, so celery.conf.update(app.config) raised ImproperlyConfigured and gunicorn workers failed to boot. Rolled back to 2.6.54 in production. The underlying fix is reimplemented safely: create_celery() falls back to the CELERY_RESULT_BACKEND environment variable for the app path, so the app-bound Celery instance no longer pins its result backend to localhost in containers. A regression test asserts no uppercase Config attribute is in Celery's _OLD_SETTING_KEYS and that conf finalization succeeds against the full app config.
Celery stores the result backend in a threading.local unless result_backend_thread_safe is set, so each thread gets its own backend instance. The 2.6.55/2.6.56 no-op only patched the import thread's instance; the cleanup/integrity producer threads created fresh unpatched backends and kept subscribing to result channels (verified live and reproduced locally with a stack trace through send_task). disable_dispatch_result_subscription now wraps Celery._get_backend so every backend instance any thread creates carries the no-op. Validated end-to-end: the API-triggered cleanup repro went from 2,633 standing result-channel subscriptions to zero.
ttlequals0
changed the title
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
2 participants
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
Remediation of an external code audit (21 findings: 4 High, 9 Medium, 8 Low) plus convergence of the two scan engines onto the chunk-distributed path. The branch then grew into a release train (v2.6.49 through v2.6.57) while chasing one production symptom on a 1.19M-file library: maintenance runs kept slowing down or stalling. Each release peeled back one layer of the same onion.
Scan engine convergence (WORKER-2)
Audit fixes (v2.6.49)
Security (v2.6.49)
The maintenance-run reliability chain (v2.6.50 - v2.6.57)
Five distinct root causes stacked on top of each other. In order of discovery:
Versions
2.6.49 - 2.6.57 (2.6.55 yanked, never referenced by any deployment)
Test plan