fix(scale-down): reselect GitHub Apps and attribute cleanup quota - #5466
guicaulada wants to merge 1 commit into
Conversation
c404f77 to
576ac85
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Quota attribution is missing for pre-installation lookups and failed GitHub requests.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Updates scale-down GitHub App selection, client caching, and quota attribution for multi-App cleanup workflows.
Changes:
- Re-selects Apps before cache lookup and scopes clients by App, runner type, and owner.
- Adds App-specific quota instrumentation.
- Adds tests for App reselection, client reuse, and attribution.
| File | Summary |
|---|---|
lambdas/functions/control-plane/src/scale-runners/scale-down.ts |
Updates client selection, caching, and quota hooks. |
lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts |
Tests multi-App selection and quota attribution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| octokit.hook.after('request', async (response) => { | ||
| await metricGitHubAppRateLimit(response.headers, appIdx); | ||
| }); |
There was a problem hiding this comment.
Fixed in 35ea122. A shared helper now attaches quota attribution hooks to the pre-installation client before installation lookup and to the installation client before runner operations. Added a regression checking the hooks are attached before getOrgInstallation and that both clients attribute responses to the selected App. All 354 control-plane tests pass.
| octokit.hook.after('request', async (response) => { | ||
| await metricGitHubAppRateLimit(response.headers, appIdx); | ||
| }); |
There was a problem hiding this comment.
Fixed in 35ea122. The shared helper also installs a request error hook on both clients. It reports RequestError response headers for the selected App and rethrows the original error, preserving existing handling. Regression coverage checks failed-request attribution, error identity, and network errors without response headers. All 354 control-plane tests pass.
576ac85 to
35ea122
Compare
35ea122 to
8c1013a
Compare
Scale-down caches an installation client by owner, so an invocation can keep using the first selected GitHub App even when additional Apps have available quota. Its runner-state response metrics also omit the selected App index and attribute quota to the primary App.
Re-evaluate App selection before cache lookup and key installation clients by App, runner type, and owner. Attach success and error response hooks to both the pre-installation and installation clients, reporting quota and metrics for the App making each request, including installation lookup, paginated runner lookups, and deletion. Error hooks preserve the original error. Subsequent cleanup operations can select another configured App while retaining client reuse within the correct scope.
Validation: 354 control-plane tests pass, including selection changes for the same owner, client reuse, and correct App attribution. Runtime TypeScript, ESLint, Prettier, Lambda bundle build, and diff checks pass. No live GitHub/AWS changes were performed.
This PR targets main and contains only client-selection and quota-attribution changes.