From 2d6ca1d042fa67a9b06f120be4d7c8bdc53073bd Mon Sep 17 00:00:00 2001 From: Klaas Schuijtemaker Date: Wed, 16 Sep 2026 14:35:13 +0200 Subject: [PATCH 1/2] GitHub plugin 1.0.2: fix the connection leak and the swallowed status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .gitignore | 4 +- backend/plugin/plugin.properties | 2 +- .../github/client/GitHubClient.kt | 100 ++++++++++++------ build.gradle.kts | 5 + documentation/release-notes.md | 29 +++++ frontend/projects/plugin/package.json | 2 +- 6 files changed, 108 insertions(+), 34 deletions(-) diff --git a/.gitignore b/.gitignore index 530f27f..cdfdc6b 100644 --- a/.gitignore +++ b/.gitignore @@ -150,7 +150,9 @@ Desktop.ini ###################### # Deployment directory ###################### -/deployment/** +# Unanchored on purpose: the frontend build writes to frontend/deployment/, which a +# leading-slash pattern does not match. +deployment/ **/AwsCredentials.properties diff --git a/backend/plugin/plugin.properties b/backend/plugin/plugin.properties index 1d0c4ca..8fed186 100644 --- a/backend/plugin/plugin.properties +++ b/backend/plugin/plugin.properties @@ -1,3 +1,3 @@ pluginGroupId=com.ritense.valtimoplugins pluginArtifactId=github-plugin -pluginVersion=1.0.1 +pluginVersion=1.0.2 diff --git a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/github/client/GitHubClient.kt b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/github/client/GitHubClient.kt index 72ea8d8..96eb4a8 100644 --- a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/github/client/GitHubClient.kt +++ b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/github/client/GitHubClient.kt @@ -28,6 +28,7 @@ import org.springframework.http.HttpMethod import org.springframework.http.MediaType import org.springframework.stereotype.Component import org.springframework.web.client.RestClient +import org.springframework.web.client.RestClientResponseException import org.springframework.web.util.UriComponentsBuilder import java.net.URI @@ -248,43 +249,80 @@ class GitHubClient( logger.debug { "GitHub $method $uri" } val entity = - restClientBuilder - .clone() - .build() - .method(method) - .uri(uri) - .headers { headers -> - headers.setBearerAuth(connection.token) - headers.accept = listOf(MediaType.valueOf(ACCEPT)) - headers.set(API_VERSION_HEADER, API_VERSION) - headers.set(HttpHeaders.USER_AGENT, USER_AGENT) - }.apply { - if (body != null) { - contentType(MediaType.APPLICATION_JSON) - body(body) - } - }.exchange({ _, response -> - val raw = response.body.readAllBytes() - val parsed = - if (raw.isEmpty()) { - NullNode.instance - } else { - runCatching { objectMapper.readTree(raw) as JsonNode } - .getOrElse { objectMapper.getNodeFactory().textNode(String(raw)) } + // A refusal has to leave here as a GitHubException whichever half of the stack + // noticed it first. The lambda below raises one from the status it reads, but a + // Valtimo application wraps the response stream in `LoggingRestClientCustomizer`, + // which reads the head of an error response and raises Spring's + // `RestClientResponseException` before the lambda gets that far. Without this + // catch, callers that branch on a status — `createLabel` treating 422 as "the + // label is already there" — silently never match, so a process whose first step + // creates its working label ran exactly once. + // + // What this recovers is the status, and in a Valtimo application only the status: + // that customizer builds its exception from `statusCode` and `statusText` alone + // and drops the body it has just read, so `describe` has nothing to name the + // rejected field with and the message is a bare "GitHub responded 422". The body + // is still used when there is one — a `RestClientResponseException` from anywhere + // that keeps it, the tests included — which is why `parseBody` is here. + // + // The trailing `true` on `exchange` is `close`: it decides whether RestClient + // closes the response once the exchange function returns, and `false` makes that + // the caller's job — which nothing here was doing, 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 with + // "ConnectionRequestTimeoutException: Timeout deadline: 5000 MILLISECONDS" until + // the application was restarted. RestClient closes in a finally, so this covers + // the throwing path too, which is the one that matters: a repository that refuses + // a write refuses it every time. The whole body is in `raw` before the function + // returns, so there is nothing to keep the response open for. + try { + restClientBuilder + .clone() + .build() + .method(method) + .uri(uri) + .headers { headers -> + headers.setBearerAuth(connection.token) + headers.accept = listOf(MediaType.valueOf(ACCEPT)) + headers.set(API_VERSION_HEADER, API_VERSION) + headers.set(HttpHeaders.USER_AGENT, USER_AGENT) + }.apply { + if (body != null) { + contentType(MediaType.APPLICATION_JSON) + body(body) } + }.exchange({ _, response -> + val raw = response.body.readAllBytes() + val parsed = parseBody(raw) - if (response.statusCode.isError) { - throw GitHubException( - message = describe(response.statusCode.value(), parsed), - status = response.statusCode.value(), - ) - } - Response(response.statusCode.value(), parsed, raw, response.headers) - }, false) + if (response.statusCode.isError) { + throw GitHubException( + message = describe(response.statusCode.value(), parsed), + status = response.statusCode.value(), + ) + } + Response(response.statusCode.value(), parsed, raw, response.headers) + }, true) + } catch (e: RestClientResponseException) { + throw GitHubException( + message = describe(e.statusCode.value(), parseBody(e.responseBodyAsByteArray)), + status = e.statusCode.value(), + cause = e, + ) + } return entity ?: Response(0, NullNode.instance, ByteArray(0), HttpHeaders.EMPTY) } + /** A body as JSON where it is JSON, as text where it is not, and null where there is none. */ + private fun parseBody(raw: ByteArray): JsonNode = + if (raw.isEmpty()) { + NullNode.instance + } else { + runCatching { objectMapper.readTree(raw) as JsonNode } + .getOrElse { objectMapper.getNodeFactory().textNode(String(raw)) } + } + /** * GitHub's error bodies carry the reason a write was refused — a failed validation names * the field — and dropping that leaves a process author with a bare 422. Kept short so diff --git a/build.gradle.kts b/build.gradle.kts index 549f92a..aa0c6fa 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -108,6 +108,11 @@ subprojects { tasks.jar { enabled = true + // Spring Boot's Gradle plugin moves the ordinary jar to the `plain` classifier so it + // cannot be mistaken for the fat jar. `bootJar` is disabled here — a plugin is a + // library, not an application — so the classifier only makes the published POM point + // at an artifact that does not exist, which is a 404 for every Maven consumer. + archiveClassifier.set("") manifest { pluginProperties["pluginArtifactId"]?.let { attributes["Implementation-Title"] = it } pluginProperties["pluginVersion"]?.let { attributes["Implementation-Version"] = it } diff --git a/documentation/release-notes.md b/documentation/release-notes.md index 0cea228..359ae86 100644 --- a/documentation/release-notes.md +++ b/documentation/release-notes.md @@ -2,6 +2,35 @@ Overview of the changes per version of the GitHub plugin. +## 1.0.2 + +### The plugin stopped reaching GitHub after five calls + +Every call leaked the connection it borrowed. The pool hands out five per host, so the sixth +call and everything after it waited five seconds for a connection that was never coming back +and then failed with `ConnectionRequestTimeoutException` — and stayed broken until the +application was restarted. + +A process running one or two actions never noticed. A process that walks a queue — read 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. + +### Create label works on the second run + +**Create label** treats "this label already exists" as success: it looks the existing label up +and carries on. That only works if the plugin can see GitHub's 422, and in a Valtimo +application it could not — the response is read by a logging interceptor that raises Spring's +own exception first, so the plugin's own error, the one carrying the status, was never built. + +A process whose first step creates its working label therefore ran exactly once and failed on +every run after that, with `422 Unprocessable Entity` and nothing to say which of its steps +had a problem. + +The status now survives whichever half of the stack noticed the refusal. The *reason* does not, +and cannot yet: that interceptor builds its exception from the status line alone and discards +the body it has just read, so a rejected write still reports `GitHub responded 422` without +naming the field GitHub objected to. Recovering that needs a change in Valtimo, not here. + ## 1.0.1 ### Get job logs now actually shows the log diff --git a/frontend/projects/plugin/package.json b/frontend/projects/plugin/package.json index 300d306..acbb766 100644 --- a/frontend/projects/plugin/package.json +++ b/frontend/projects/plugin/package.json @@ -1,7 +1,7 @@ { "name": "@valtimo-plugins/github-plugin", "license": "EUPL-1.2", - "version": "1.0.1", + "version": "1.0.2", "peerDependencies": { "@angular/common": "19.2.20", "@angular/core": "19.2.20" From b1fbe7fa4ac1ed6984051e75fca5671d63356f44 Mon Sep 17 00:00:00 2001 From: Klaas Schuijtemaker Date: Wed, 16 Sep 2026 15:02:29 +0200 Subject: [PATCH 2/2] Give the Kotlin compile daemon room for the CodeQL scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- gradle.properties | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gradle.properties b/gradle.properties index 7e07cfa..f2cc9f0 100644 --- a/gradle.properties +++ b/gradle.properties @@ -3,6 +3,12 @@ projectGroup="com.ritense" projectVersion=0.0.1 org.gradle.jvmargs=-Xms128m -Xmx1024m -Duser.timezone=UTC +# The Kotlin compile daemon inherits the line above when this is unset, and 1 GB is not +# enough for it under CodeQL's build tracer: the scan's `./gradlew testClasses` died with +# `OOMErrorException: Not enough memory to run compilation` after 23 minutes, while the +# ordinary test job compiled the same code in under three. Only the traced build needs the +# headroom, but the setting is read by every build, so keep it modest. +kotlin.daemon.jvmargs=-Xmx2g org.gradle.welcome=never kotlinVersion=2.1.20