Skip to content

Re-use rekor/timestamp connections (thread-locally)#1732

Merged
jku merged 8 commits intomainfrom
thread-local-sessions
Apr 13, 2026
Merged

Re-use rekor/timestamp connections (thread-locally)#1732
jku merged 8 commits intomainfrom
thread-local-sessions

Conversation

@jku
Copy link
Copy Markdown
Member

@jku jku commented Apr 7, 2026 •
edited
Loading

Fixes #1730

  • session reuse was removed from rekor client when rekor2 support was added (as it was not clear if Session is thread safe, but we really needed threads for rekor 2 signing to perform)
  • This becomes an issue if users sign hundreds of artifacts with the same Signer: in that case we use too many connections/ports
  • This PR reintroduces Session reuse, but keeps it thread local -- so even if there is a thread safety issue we should not be affected
  • I'm fine with also not using threading.local() if it's clear that thread safety is not an issue

jku added 4 commits April 7, 2026 11:58
@jku
Add test to verify session reuse
00023cf 
This currently fails (we overuse connections since wanted to be thread
safe for rekor2)

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku
Signing: thread local requests sessions
e958d1e 
Currently we are overusing connections when the same signer is used
to sign multiple artifacts: This was a result of adding threaded
signing and wanting to avoid potential thread safety issues (I'm
still unsure if requests Session is thread safe).

Re-introduce Session re-use in Rekor clients and TSA client (the components
used in parallel threads): Make the Sessions thread-local to avoid
potential issues.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku
Add missing license
20bb739 
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku
linter fixes
3e321a3 
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku jku marked this pull request as draft April 7, 2026 16:58
@jku jku marked this pull request as draft April 7, 2026 16:58
@jku
Remove now unnecessary mypy ignores
4baa97d 
Requests 2.33 is annotated

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku jku marked this pull request as ready for review April 7, 2026 17:04
@jku
Copy link
Copy Markdown
Member Author

jku commented Apr 8, 2026 •
edited
Loading

Documenting the practical effect of this PR:

  • sigstore-python CLI uses ThreadPoolExecutor to run signing in threads with currently no max_workers set: so in practice with this PR the CLI (when called with enough artifacts to sign) uses min(32, os.cpu_count() + 4) connections to each host. 32 is still a bit much but should be acceptable.
  • API users like pypi-attestations can decide their thread use independently: currently pypi-attestations runs in a single thread so will use a single connection per host

@jku jku mentioned this pull request Apr 8, 2026
Open
woodruffw
woodruffw approved these changes Apr 11, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jku! Seems reasonable to me 🙂

@jku jku enabled auto-merge (squash) April 13, 2026 06:46
@jku jku merged commit 297888a into main Apr 13, 2026
47 checks passed
@jku jku deleted the thread-local-sessions branch April 13, 2026 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@woodruffw woodruffw woodruffw approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

better connection re-use

2 participants

@jku @woodruffw