Add per-issuer lock to prevent thundering herd on new issuers #180

Copilot · 2025-12-08T22:35:01Z

Task: Implement per-issuer lock for new issuers to prevent thundering herd ✅

Implementation Complete

Rebased onto latest master with integration test infrastructure and monitoring API
Created proper integration test validating per-issuer locking with concurrent threads
Test proves only 1 key lookup occurs despite 10 concurrent threads

Integration Test: `ConcurrentNewIssuerLookup`

The new test validates the per-issuer locking mechanism:

Launches 10 concurrent threads all validating tokens from the same new issuer
Uses monitoring API to count successful key lookups before/after
Asserts exactly 1 lookup occurs (proving per-issuer lock prevents thundering herd)
Without the lock, would see up to 10 concurrent lookups

Changes Made

scitokens_internal.cpp: Per-issuer mutex map with double-check pattern
test/integration_test.cpp: New concurrent test using monitoring API
test/main.cpp: Removed inadequate unit test

Testing

Integration test validates per-issuer locking works correctly
Monitoring API confirms only 1 key lookup despite 10 concurrent threads

Original prompt

This section details on the original issue you should resolve

<issue_title>Create a per-issuer lock for entries that don't exist</issue_title>
<issue_description>We have a "thundering herd" issue that exists for an issuer that has never been seen. The "refresh mutex" in scitokens_internal.cpp only works with renewal of known issuers.

Create either a global lock for new issuers or a lookup table of a lock-per-issuer. If the latter is done, make sure to have a mechanism to prevent resource exhaustion from an attacker that can create an infinite number of issuers.</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes Create a per-issuer lock for entries that don't exist #179

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

bbockelm

@copilot - please work on requested changes.

@jthiltges - these ideas seems reasonable. What are your thoughts?

.gitignore

_codeql_detected_source_root

src/scitokens_internal.cpp

Copilot · 2025-12-09T02:06:45Z

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

demo.scitokens.org
- Triggering command: ./test/scitokens-gtest ./test/scitokens-gtest (dns block)
esm.ubuntu.com
- Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

bbockelm · 2025-12-09T02:41:18Z

@copilot - please fix lint issues.

Copilot · 2025-12-09T02:46:21Z

@copilot - please fix lint issues.

Fixed in commit de1791a. Removed trailing whitespace and reformatted long lines to comply with clang-format requirements.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

demo.scitokens.org
- Triggering command: ./test/scitokens-gtest ./test/scitokens-gtest /tmp/ccGThxES.s (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

bbockelm

@copilot - please address change requests.

Note the change to add a HTTP server for an integration test is nontrivial but the only way I can think of to test the lookup code.

test/main.cpp

bbockelm · 2025-12-10T16:07:41Z

@copilot - please continue your prior work.

Copilot · 2025-12-10T16:22:53Z

@copilot - please continue your prior work.

Work completed. Created integration test that validates per-issuer locking with 10 concurrent threads and monitoring API verification. See reply to review comment above for details. Code is ready at local commit ae2af0d.

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

… handling Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

This PR addresses the thundering herd problem when multiple threads simultaneously try to validate tokens from the same issuer: Per-issuer locking: - Added a per-issuer mutex map with shared_ptr ownership - Threads acquire a lock for an issuer before fetching keys from web - Other threads wait on the lock, then find keys in cache - Lock ownership transfers through async status for proper lifecycle - Limited to 1000 cached mutexes to prevent resource exhaustion Negative caching: - On web fetch failure (e.g., 404), store empty keys in cache - Uses same TTL as successful lookups (get_next_update_delta) - Subsequent lookups hit cache and fail fast without web requests - Prevents repeated web requests for known-bad issuers SQLite busy timeout: - Added 5-second busy timeout to handle concurrent DB access - Applied to all database operations (init, read, write) Stress tests: - StressTestValidToken: 10 threads, 5 seconds, valid token - StressTestInvalidIssuer: 10 threads, 5 seconds, 404 issuer - ConcurrentNewIssuerLookup: Verifies only ONE web fetch occurs Verified behavior: - Valid issuer: ONE key lookup for thousands of validations - Invalid issuer: ONE web request (OIDC + OAuth fallback), then cached

Merged in background JWKS refresh feature from master. Added per-issuer lock tests (ConcurrentNewIssuerLookup, StressTestValidToken, StressTestInvalidIssuer) alongside the new BackgroundRefreshTest.

The StressTestValidToken and StressTestInvalidIssuer tests now: - Use unique cache directories to prevent interference from prior tests - Explicitly stop background refresh to avoid extra key lookups - Reset update_interval_s to default (600s) in case prior tests changed it This ensures the tests reliably verify that per-issuer locking prevents thundering herd without false failures from background refresh or stale config settings.

bbockelm

LGTM.

Copilot AI assigned Copilot and bbockelm Dec 8, 2025

Copilot started work on behalf of bbockelm December 8, 2025 22:35 View session

Copilot AI changed the title ~~[WIP] Create a per-issuer lock for non-existent entries~~ Add per-issuer lock to prevent thundering herd on new issuers Dec 8, 2025

Copilot finished work on behalf of bbockelm December 8, 2025 22:53

Copilot AI requested a review from bbockelm December 8, 2025 22:53

bbockelm requested changes Dec 9, 2025

View reviewed changes

.gitignore Show resolved Hide resolved

_codeql_detected_source_root Outdated Show resolved Hide resolved

src/scitokens_internal.cpp Outdated Show resolved Hide resolved

Copilot started work on behalf of bbockelm December 9, 2025 02:01 View session

Copilot AI requested a review from bbockelm December 9, 2025 02:06

Copilot finished work on behalf of bbockelm December 9, 2025 02:06

Copilot started work on behalf of bbockelm December 9, 2025 02:41 View session

Copilot finished work on behalf of bbockelm December 9, 2025 02:47

bbockelm requested changes Dec 9, 2025

View reviewed changes

test/main.cpp Show resolved Hide resolved

Copilot started work on behalf of bbockelm December 9, 2025 03:06 View session

Copilot AI requested a review from bbockelm December 9, 2025 03:10

Copilot finished work on behalf of bbockelm December 9, 2025 03:10

Copilot started work on behalf of bbockelm December 10, 2025 14:26 View session

Copilot started work on behalf of bbockelm December 10, 2025 16:08 View session

Copilot AI and others added 5 commits December 10, 2025 19:29

Initial plan

7eac47e

Implement per-issuer lock to prevent thundering herd on new issuers

8a87347

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Address code review feedback: improve mutex cleanup strategy

f9b693a

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Add _codeql_build_dir to .gitignore and clean up repository

375f517

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Address review feedback: remove unnecessary files and fix mutex limit…

fd4dfb7

… handling Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Copilot AI and others added 2 commits December 10, 2025 19:33

Fix lint issues: remove trailing whitespace and reformat lines

ae3fc37

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

bbockelm force-pushed the copilot/create-per-issuer-lock branch from de1791a to 57bc1c8 Compare December 11, 2025 00:52

bbockelm added 2 commits December 11, 2025 01:24

Merge origin/master and run clang-format

63ac7c9

Merged in background JWKS refresh feature from master. Added per-issuer lock tests (ConcurrentNewIssuerLookup, StressTestValidToken, StressTestInvalidIssuer) alongside the new BackgroundRefreshTest.

bbockelm approved these changes Dec 11, 2025

View reviewed changes

bbockelm marked this pull request as ready for review December 11, 2025 12:40

bbockelm merged commit 47fda99 into master Dec 11, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add per-issuer lock to prevent thundering herd on new issuers #180

Add per-issuer lock to prevent thundering herd on new issuers #180

Uh oh!

Copilot AI commented Dec 8, 2025 •

edited

Loading

Uh oh!

bbockelm left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 9, 2025

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

bbockelm commented Dec 9, 2025

Uh oh!

Copilot AI commented Dec 9, 2025 •

edited

Loading

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

bbockelm left a comment

Uh oh!

Uh oh!

bbockelm commented Dec 10, 2025

Uh oh!

Copilot AI commented Dec 10, 2025

Uh oh!

bbockelm left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add per-issuer lock to prevent thundering herd on new issuers #180

Add per-issuer lock to prevent thundering herd on new issuers #180

Uh oh!

Conversation

Copilot AI commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Task: Implement per-issuer lock for new issuers to prevent thundering herd ✅

Implementation Complete

Integration Test: ConcurrentNewIssuerLookup

Changes Made

Testing

Comments on the Issue (you are @copilot in this section)

Uh oh!

bbockelm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 9, 2025

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

bbockelm commented Dec 9, 2025

Uh oh!

Copilot AI commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

bbockelm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbockelm commented Dec 10, 2025

Uh oh!

Copilot AI commented Dec 10, 2025

Uh oh!

bbockelm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Dec 8, 2025 •

edited

Loading

Integration Test: `ConcurrentNewIssuerLookup`

Copilot AI commented Dec 9, 2025 •

edited

Loading