Repository navigation
Reliability fixes: score 500, DB pooling, async CVE enrichment, thread-safe rate limit - #162
Merged
Merged
Conversation
…t, thread-safe rate limit Closes #150. - REL-001: remove duplicate GROUP BY in get_score() that made /api/score always return 500 - REL-002: replace fragile INTERVAL '%s minutes' string substitution with make_interval(mins => %s) - REL-003: DatabaseManager now borrows connections from a shared psycopg2 connection pool per DSN (DB_POOL_MAX_CONN, default 10) instead of opening a new connection per request - REL-004: POST /api/scans/<id>/enrich now runs CVE enrichment in a background thread and returns 202 immediately, instead of blocking on synchronous NVD lookups until the request times out - REL-005: lock-guard the NVD client's rate-limit check so concurrent callers can't fire requests closer together than the allowed gap - Fix a bug found during review: a failed enrichment write could leave the connection in an aborted-transaction state, silently swallowing the follow-up FAILED status update and leaving the scan stuck at ENRICHING forever; now rolls back first
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…-quality bot Both test_database_manager_reliability.py and test_nvd_client.py imported scanner/api modules with both 'import X as x' and 'from X import y' forms. Consolidated to one form each; test_nvd_client.py switches to string-path mock.patch() targets instead of importing the module object, avoiding the need to touch ~15 pre-existing call sites.
…obal false positive Replaces the bare module-level _last_request_time global (mutated via the global keyword) with a small _RateLimiter class holding the timestamp and lock as instance state. Same behavior, but removes the pattern the code-quality bot's static analysis was misreading as an unused global (it couldn't see the value is read by the next invocation of the same function via module state).
Collaborator
|
@TFT444 , kindlly fix the conflicts please before merging. |
Collaborator
|
@TFT444 , CI is failing, touchbase with this PR pls. |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
The "mark failed" query lost its COALESCE(attempt_count, 1) >= %s clause during an earlier merge, while still passing 2 params to a query with only 1 placeholder — this would raise at runtime against a real database and would mark stale scans failed on their first timeout instead of after max_attempts retries. Restore the clause to match the tested, correct version. Also drop the now-superseded REL-002 test, whose single-query assumptions predate the retry-attempt feature; equivalent coverage now lives in tests/test_async_scan_persistence.py.
A previous merge auto-resolved cleanly across files but left this call site stranded: api/models/finding.py dropped run_migrations() (schema is now Alembic-managed, applied via startup.sh) while api/app.py still called it whenever DATABASE_URL was set. Git's merge saw no conflict because the removal and the call site were in different files, but the result crashed create_app() with AttributeError in any environment with a real DATABASE_URL configured (e.g. CI's Postgres service), which is exactly why this passed locally (no DATABASE_URL set) but failed in CI.
Collaborator
Author
|
@Vishnu2707 The other two that show as failing Backend Tests and CI Summary both actually passed individually. They appear red on the PR page because when any job in a workflow run fails (Trivy in this case), GitHub marks the entire workflow run as failed and surfaces all jobs under it with a red indicator, even the ones that passed. So the code is clean, all tests pass, lint is green. The only thing left is for reviewers to re-review and update their review status. |
Vishnu2707
approved these changes
Jul 11, 2026
Vishnu2707
left a comment
Vishnu2707
left a comment
Collaborator
There was a problem hiding this comment.
Approved. All the changes are addressed.
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
Closes #150 (FIX 2: Reliability).
/api/scorealways 500): removed a duplicateGROUP BY severityinget_score()that caused a SQL syntax error on every call.INTERVAL '%s minutes'withmake_interval(mins => %s)inrecover_stale_scans().DatabaseManagernow borrows connections from a sharedpsycopg2.pool.ThreadedConnectionPoolper DSN (size configurable viaDB_POOL_MAX_CONN, default 10) instead of opening a new PostgreSQL connection on every request.POST /api/scans/<id>/enrichnow runs enrichment in a background thread and returns 202 immediately; clients pollGET /api/scans/<id>(cve_enrichment_status) for completion, instead of the request blocking on NVD's ~7s-per-lookup rate limit until it times out._wait_for_rate_limit()is now lock-guarded so concurrent callers (e.g. background enrichment threads) can't both read a stale timestamp and fire requests too close together.Bug found and fixed during self-review
A failed enrichment write (e.g. in
update_cve_fields) could leave the DB connection in an aborted-transaction state. The follow-up call to mark the scanFAILEDreused that same connection before returning it to the pool, so it silently raisedInFailedSqlTransactionand got swallowed — leaving the scan stuck showingENRICHINGforever with no way to tell it had actually failed. Fixed by rolling back before that follow-up write, with regression tests (test_background_enrichment_rolls_back_before_marking_failed).Known trade-offs (not fixed, out of scope for this issue)
recover_stale_scans-style recovery job for a scan stuck atENRICHING(only scan execution has that today). Worth a follow-up issue if needed.Test plan
tests/test_database_manager_reliability.py— singleGROUP BY, correct score deduction,make_intervalSQL, pool acquire/return/reuse-per-DSN, broken-connection discardtests/test_scans_enrich.py— 202 + background thread scheduling, already-completed/in-progress/missing-scan/no-findings branches, success/failure background outcomes, rollback-before-FAILED regressiontests/test_nvd_client.py— newTestRateLimitThreadSafetyproving concurrent callers are serialized, not interleavedtests/smoke_test.py— TC-33/34 updated for the async contract, plus a new polling assertion (TC-34b) that verifies enrichment actually reachesCOMPLETED, not just that it was accepteddevcheckout)