Skip to content

Lock the credentials store when reading or writing#15610

Merged
zanieb merged 1 commit intofeature/authfrom
zb/lock-auth
Sep 2, 2025
Merged

Lock the credentials store when reading or writing#15610
zanieb merged 1 commit intofeature/authfrom
zb/lock-auth

Conversation

@zanieb
Copy link
Member

@zanieb zanieb commented Aug 31, 2025

Adds locking of the credentials store for concurrency safety. It's important to hold the lock from read -> write so credentials are not dropped during concurrent writes.

I opted not to attach the lock to the store itself. Instead, I return the lock on read and require it on write to encourage safe use. Maybe attaching the source path to the store struct and adding a lock(&self) method would make sense? but then you can forget to take the lock at the right time. The main problem with the interface here is to write a new store you have to take the lock yourself, and you could make a mistake by taking a lock for the wrong path or something. The fix for that would be to introduce a new CredentialStoreHandle type or something, but that seems overzealous rn. We also don't eagerly drop the lock on token read, although we could.

@zanieb zanieb marked this pull request as ready for review August 31, 2025 20:17
@zanieb zanieb temporarily deployed to uv-test-registries August 31, 2025 20:19 — with GitHub Actions Inactive
@zanieb zanieb merged commit cc63c38 into feature/auth Sep 2, 2025
157 checks passed
@zanieb zanieb deleted the zb/lock-auth branch September 2, 2025 12:59
zanieb added a commit that referenced this pull request Sep 2, 2025
Adds locking of the credentials store for concurrency safety. It's
important to hold the lock from read -> write so credentials are not
dropped during concurrent writes.

I opted not to attach the lock to the store itself. Instead, I return
the lock on read and require it on write to encourage safe use. Maybe
attaching the source path to the store struct and adding a `lock(&self)`
method would make sense? but then you can forget to take the lock at the
right time. The main problem with the interface here is to write a _new_
store you have to take the lock yourself, and you could make a mistake
by taking a lock for the wrong path or something. The fix for that would
be to introduce a new `CredentialStoreHandle` type or something, but
that seems overzealous rn. We also don't eagerly drop the lock on token
read, although we could.
@zanieb zanieb mentioned this pull request Sep 3, 2025
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