fix: Simplify new client throttle logic to avoid leaks

[stevehipwell]

Resolves #3603

---

Before the change?

• New client would leak concurrency permits
• New client concurrency constraint based on reading the response body

After the change?

• New client concurrency permits can't leak
• New client concurrency constraint based on remote processing

Pull request checklist

• Schema migrations have been created if needed (example)
• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[github-actions]

👋 Hi, and thank you for this contribution!

This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can.

You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions.

---

_{🤖 This is an automated message.}

[Copilot]

Pull request overview

These provider review instructions are being used.

MEDIUM — Missing targeted regression coverage
internal/ghclient/throttle_test.go:109: The new concurrency test also passes with the leaking implementation. Add the client-level 404 regression from issue #3603.

This PR releases concurrency permits when remote processing completes rather than when response bodies close.

Changes:

• Simplifies semaphore release logic.
• Adds concurrent throttling tests and test helpers.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
internal/ghclient/throttle.go	Releases permits after RoundTrip.
internal/ghclient/throttle_test.go	Updates throttling tests.
internal/ghclient/helpers_test.go	Adds delay and request helpers.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[deiga]

Looks good, but I can't fully vouch for the functionality. This isn't my strongest area 😬

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

internal/ghclient/throttle_test.go:113

• This does not cover the reported regression: the inner transport returns an error and no response, so the previous implementation's explicit error-path release would also pass. Return a successful non-2xx response and deliberately leave its body unclosed; then TryAcquire will fail against the old body-close-based implementation and prove this fix prevents the leak.

		inner := &testRoundTripper{err: errors.New("boom")}
		sema := semaphore.NewWeighted(1)

[robert-crandall]

Nice cleanup — dropping the throttlerReadCloser in favor of a plain defer Release makes the leak impossible by construction, and moving the constraint to remote processing time feels like the right semantics here. Thanks for tracking this down!