Conversation
A Scan All run drained the core DB connection pool and wedged core completely,
including routes that touch no artifact table. Three things combined to do it,
all of them in the per-artifact scan dispatch:
* startScanAll wrapped the whole scan of an artifact in a transaction, so one
pool connection was held across the read phase, which includes the
artifact_blob join in HasUnscannableLayer, and across the job submission to
jobservice.
* MakePlaceHolder, which runs inside that transaction, read the previous
reports on an ORM of its own. That second acquire is where the goroutines
were parked in the production dump, and it is why every session showed
"idle in transaction" on the artifact_blob join with no lock waits.
* GetReport spawned a goroutine per artifact in an index, each with an ORM of
its own, so one artifact-list request took a pool connection per child
artifact. The same shape appeared in both listScanTasks implementations.
The scan is now split into a read-only planning half and a dispatch half, and
only the dispatch half runs in a transaction. The report lookups run on the
caller's connection: GetReport asks for every artifact's reports in one query,
and the task lookups run in sequence rather than one goroutine and one
connection each.
Removing the extra acquire from the dispatch path also removes the deadlock
itself, not only the time a connection is held: a pool where every connection is
held by a caller waiting for another connection cannot recover on its own, which
is why the incident needed all three core pods restarted.
Closes #856
Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
|
Warning Review limit reachedNext included review available in 34 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
This change may need patch-release backports. Comment with one of these commands to open a cherry-pick PR:
|
Contributor
|
Preview images for this PR are available in
Verify a preview image: Verify the SBOM attestation: |
bupd
added
prio/P1
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.
Closes #856.
The bug
A Scan All run drained the core DB connection pool and wedged core completely, including routes that touch no artifact table. Three things in the per-artifact scan dispatch combined to do it:
startScanAllwrapped the whole scan of an artifact in a transaction, so one pool connection was held across the read phase, which includes theartifact_blobjoin inHasUnscannableLayer, and across the job submission to jobservice.MakePlaceHolder, which runs inside that transaction, read the previous reports on an ORM of its own (h.cloneCtx(ctx)). That second acquire is where the goroutines were parked in the production dump. It also explains thepg_stat_activityevidence in the issue: every sessionidle in transaction, all on theartifact_blobjoin with no lock waits, because the join is the last statement to run on the transaction's connection before the goroutine blocks asking for a second one.GetReportspawned a goroutine per artifact in an index, each with an ORM of its own, so a single artifact-list request took a pool connection per child artifact. The same shape was in bothlistScanTasksimplementations.The fix
The scan is split into a read-only planning half (
planScan: resolve the scanner, walk the artifacts) and a dispatch half (dispatchScan: placeholders, job submission), and only the dispatch half runs in a transaction.Scanstill runs both, so the API path is unchanged.The report lookups run on the caller's connection:
GetReportasks for every artifact's reports in one query grouped by digest, and the task lookups run in sequence instead of one goroutine and one connection each.MakePlaceHolderand the SBOMdeleteread on the caller's context, which removescloneCtxfrom both handlers.Removing the extra acquire from the dispatch path also removes the deadlock itself, not only the time a connection is held. A pool where every connection is held by a caller waiting for another connection cannot recover on its own, which is why the incident needed all three core pods restarted.
Reproduction
Two regression tests, both failing on
main:The last one panics rather than asserting, which is itself the evidence: with
orm.Clonethe handler builds a new ORM, and a new ORM needs a registered database.What this does not do
lib/dbpool/pool.gois already being changed by fix: Reject requests with 429 on DB connection pool exhaustion #773 forPOSTGRESQL_STATEMENT_TIMEOUT, and a bounded acquire belongs next to it.config.AuthMode()on every request still goes through the config cache, so one slow acquire anywhere still parks every caller behind a single builder. fix: Core deadlock — config cache FetchOrSave + DB connection pool circular wait #92 and its PR cover the ordering half of that; the in-memory config snapshot proposed in fix: Core deadlock — config cache FetchOrSave + DB connection pool circular wait #92 is what removes it.GetScanLogstill fans out a goroutine per report UUID. Those share the caller's ORM rather than cloning one each, and it is a single-artifact endpoint, so it is not part of this fix.Test
Release Notes
Fixed a deadlock where a Scan All run could exhaust harbor-core's database connection pool and stop it serving any request, including
/v2/pulls and pushes, until every core pod was restarted. Scanning an artifact no longer holds a database connection while reading the artifact's layers or while submitting the scan job, and listing artifacts with scan overviews now reads every report in one query instead of one connection per artifact.