Skip to content

fix(gitlab): stop re-walking every group and project per npm request - #1

Open
buffcode wants to merge 3 commits into
masterfrom
meeva/single-flight-and-page-size
Open

fix(gitlab): stop re-walking every group and project per npm request#1
buffcode wants to merge 3 commits into
masterfrom
meeva/single-flight-and-page-size

Conversation

@buffcode

Copy link
Copy Markdown

Why

Every npm metadata request for a private scope triggers a fresh
authenticate(), and each one walked the whole GitLab instance:
Users.current, then Groups.all and Projects.all page by page at the
API default of 20 per page — roughly 37 API calls per npm lookup.

The auth cache cannot help, because it is only written once a walk
resolves. A burst of npm requests all miss the cache, all start their own
walk, and the walks pile up on the GitLab server.

On 2026-08-29 this took our GitLab down for six minutes: 50 logins in that
window produced ~1900 API requests and occupied ~19 of the instance's 32
Puma threads. Rails p95 queue wait went from an 0.08 s week-long baseline
to over 10 s.

This is upstream #125,
open since 2021. Upstream PRs bufferoverflow#126 and bufferoverflow#144 address it and were never
merged; the last release was 3.0.1 in January 2020.

What changed

  • Single-flight. Concurrent logins with the same credentials share one
    in-flight walk, keyed with the same hash the auth cache uses
    (AuthCache.generateKeyHash, made public for this). The slot is released
    on rejection as well as success, so a failed walk is never replayed to
    later logins.
  • Page size. Group and project queries ask for 100 items per page, the
    API maximum, cutting a walk from ~32 pages to ~7. Verified per_page is
    the correct key by reading the pinned gitlab@3.5.1 RequestHelper.js:
    it spreads every option except showPagination/maxPages straight into
    the query string.
  • Base image moved to verdaccio/verdaccio:6 with a Node 22 builder.
    verdaccio 4 is deprecated and gets no security fixes. verdaccio 6 still
    supports the callback-style auth plugin API, so the plugin itself needed
    no changes.

A pre-existing test bug, fixed first

The mock's Users.current resolved unconditionally, so authenticating
with a wrong password succeeded and returned the full group list
(probed: err=null, data=["myUser","myGroup","anotherGroup/myProject"]).

The two "should fail authentication" tests passed anyway — but only
because the failing assertion threw inside the plugin's promise chain,
the plugin's own .catch swallowed it and re-invoked the callback with an
unauthorized error, and that second call satisfied both expectations.
They were false positives. The mock now rejects on a bad token, so they
fail for the reason they claim to test.

Verification

  • Baseline before any change: 19/19 green on Node 22.
  • New tests red as intended: 5 walks instead of 1, and no per_page.
  • After the fix: 22/22 green, yarn type-check clean, yarn lint:ts
    clean.
  • Image built from this branch runs with plugin verdaccio-gitlab successfully loaded (authentication) on verdaccio/6.10.1.

Not yet proven: the call-count reduction is verified against the jest mock
only. The real numbers and npm token continuity get verified against a
live instance before this is deployed.

Note for future edits: this Babel setup rejects optional chaining, so no
?. or ?? in src/.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Cyn4NGPGBVjY3epfwv9fXB

buffcode and others added 3 commits August 29, 2026 02:33
Users.current resolved unconditionally, so authenticating with a wrong
password succeeded and returned the full group list. The two "should fail
authentication" tests still passed, but only by accident: the failing
assertion threw inside the plugin's promise chain, the plugin's own catch
swallowed it and re-invoked the callback with an unauthorized error, and
the second invocation satisfied both expectations.

Reject in the mock when the token does not match, so those tests fail for
the reason they claim to test. The API calls also become shared jest.fn()
instances exposed as Gitlab.__calls, so tests can assert how many requests
a single authenticate() makes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyn4NGPGBVjY3epfwv9fXB
Each npm metadata request for a private scope triggers a fresh
authenticate(), and each one walked the whole instance: Users.current,
then Groups.all and Projects.all page by page at the API default of 20
per page. On an instance with a few hundred projects that is roughly 37
API calls per npm lookup.

The auth cache could not help, because it is only written once a walk
resolves. A burst of requests all miss the cache, all start their own
walk, and the walks pile up on the gitlab server.

Two changes:

- Concurrent logins with the same credentials now share one in-flight
  walk, keyed by the same hash the auth cache uses. The slot is released
  on rejection as well as success, so a failed walk is never replayed to
  later logins.
- Group and project queries ask for 100 items per page, the API maximum,
  cutting a walk from roughly 32 pages to 7.

Fixes the upstream report in bufferoverflow#125.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyn4NGPGBVjY3epfwv9fXB
verdaccio 4 is deprecated and no longer receives security fixes, and the
node 12 builder cannot build against it. verdaccio 6 still supports the
callback-style authentication plugin API, so the plugin needs no changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyn4NGPGBVjY3epfwv9fXB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant