GitHub plugin 1.0.2: fix the connection leak and the swallowed status - #3
Merged
Merged
Conversation
Two faults found by driving a real process against a real repository, both of which look like something other than what they are. The client borrowed a pooled connection per call and never gave it back: `RestClient.exchange` was passed `close = false`, which makes closing the caller's job, and nothing was doing it. Apache lends five per route and Valtimo waits five seconds for one, so the sixth call and everything after it died with `ConnectionRequestTimeoutException` until the application was restarted. One or two actions never noticed; a process that walks a queue hit it every time. A refusal arrived as Spring's exception rather than the plugin's, because Valtimo's logging interceptor reads the head of an error response and raises before the exchange function can. `GitHubException.status` was therefore never set, and `createLabel`, which treats 422 as "the label is already there", could not see the 422 — so a process whose first step creates its working label ran exactly once and failed on every run after that. The status now survives; the body does not, and cannot, because that interceptor drops it. Also: the published jar carried Spring Boot's `plain` classifier while the POM pointed at the jar without one, so `github-plugin-1.0.1.jar` does not exist on Maven Central and no Maven consumer could resolve it. And `/deployment/**` never matched `frontend/deployment/`, so the frontend build output was not ignored.
CodeQL's autobuild runs `./gradlew testClasses` under its tracer, and the Kotlin daemon inherited `org.gradle.jvmargs`' 1 GB ceiling. That is enough untraced — the test job compiles the same code in under three minutes — but the traced build spent 23 minutes and then died with `OOMErrorException: Not enough memory to run compilation`, which is the one thing standing between this branch and a green scan.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two faults found by driving a real process against a real repository, both of which present as something other than what they are.
The plugin stopped reaching GitHub after five calls
RestClient.exchangewas passedclose = false, which makes closing the response the caller's job — and nothing was doing it, so every call leaked the connection it borrowed. Apache's pool lends five per route and Valtimo waits five seconds for one, so the sixth call and everything after it died withConnectionRequestTimeoutException: Timeout deadline: 5000 MILLISECONDSuntil the application was restarted.A process running one or two actions never noticed. A process that walks a queue — the pull requests, then per pull request its reviews, its checks and its runs — hit it every time, somewhere in the middle, with an error that named a timeout and said nothing about GitHub.
RestClientcloses in afinally, so this covers the throwing path too. That is the one that matters: a repository that refuses a write refuses it every time, and each attempt was leaking.Create label works on the second run
Create label treats "this label already exists" as success by looking the existing label up. That only works if it can see GitHub's 422, and it could not: Valtimo's
LoggingRestClientCustomizerreads the head of an error response and raisesHttpClientErrorExceptionbefore the exchange function runs, soGitHubException— and itsstatus— was never built. A process whose first step creates its working label ran exactly once and failed on every run after that.The status now survives whichever half of the stack noticed the refusal. The reason does not: that customizer builds its exception from the status line alone and discards the body it has just read, so a rejected write still reports
GitHub responded 422without naming the field. Recovering that needs a change in Valtimo — it already holdsresponseBodyHeadand could pass it to the exception.Also
plainclassifier while the POM points at the jar without one.github-plugin-1.0.1.jargenuinely does not exist on Maven Central — only-plain.jar— so no Maven consumer could resolve it. Gradle consumers were saved by the module metadata..gitignorehad/deployment/**, which never matchesfrontend/deployment/, so the frontend build output was not ignored.Verification
Both fixes were found and confirmed against a live repository, not by inspection. The leak surfaced mid-sweep while walking a queue of pull requests; the 422 surfaced on the second run of a process that creates its own label.
:backend:plugin:test,ktlintCheckandnpm run lintpass.