Skip to content

DELETE /repository leaves a transaction open for the retry.Timeout duration when the client disconnects mid-request #23703

Description

@sebidchi

Expected behavior and actual behavior:

Expected: if a client disconnects during a repository delete, the server-side transaction closes shortly after (or the delete is aborted).

Actual: DELETE /api/v2.0/projects/{project}/repositories/{repository} wraps the whole request in one DB transaction (transaction.Middleware, commit/rollback only after the handler returns). Deleting a repository walks its artifacts and calls the redis cache managers' cleanUp() for each one, which retries a cache-delete via retry.Retry with the library default timeout (1 minute), twice per artifact. retry.Retry never checks whether the request context is already canceled before retrying — so if the client gives up mid-request, the retries keep running anyway, and the transaction sits open the whole time.

We hit this in production: a repository delete that the client had already given up on kept a transaction open for close to 2 hours, until it exhausted the DB connection pool and the registry stopped responding entirely.

Steps to reproduce the problem:

  1. Start a delete on a repository with several artifacts.
  2. Disconnect the client before the request finishes (close the connection / client-side timeout).
  3. Watch pg_stat_activity for the harbor-core connection handling that request: it stays idle in transaction, running the same last query (the DB write from the delete), for as long as the cache-invalidation retries keep going — up to retry.Timeout (default 1 minute) x 2 per artifact.

We reproduced this outside production against real Postgres and the actual lib/orm/lib/retry code: same request, client disconnects after ~1.5s, transaction stays open ~10.5s (scales with artifact count and retry timeout in the real deployment). Patching the retry closure to check ctx.Err() and bail via retry.Abort closes it in ~2.2s instead — same disconnect, same timing, only that one check differs.

Versions:

Additional context:

Root cause, file by file:

  • src/server/middleware/transaction/transaction.goMiddleware wraps the entire handler in one transaction (orm.WithTransaction), commit/rollback only after it fully returns. DELETE /repository isn't in dbTxSkippers (src/core/middlewares/middlewares.go), so it's fully exposed.
  • src/lib/retry/retry.goRetry (line 104) retries until success or its own Timeout elapses. It has an early-exit path (retry.Abort, checked via errors.As at line 150) but nothing calls it unless the closure explicitly does.
  • Nine call sites retry cache deletes with no cancellation check at all, across src/pkg/cached/{artifact,repository,project,project_metadata,manifest}/redis/manager.go.
  • The fix pattern already exists elsewhere in this codebase and works: src/controller/quota/controller.go:260-263 checks ctx.Err() before retrying and calls retry.Abort(ctx.Err()).

We have a PR ready applying that same pattern to the 9 cache-manager call sites (via BaseManager.DeleteCache), plus tests covering the cancellation path, which none of the existing tests do today. Will link it here once opened.

Separately worth a maintainer opinion: retry.Retry has 22 production call sites total; making the check at the library level (auto-abort when the returned error is context.Canceled) would cover this class of bug everywhere instead of per call site, but it's a bigger surface to review, so we're proposing it as a follow-up rather than bundling it into the same PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions