GitHub plugin 1.0.2: fix the connection leak and the swallowed status

.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
 

backend/plugin/plugin.properties

@@ -1,3 +1,3 @@
 pluginGroupId=com.ritense.valtimoplugins
 pluginArtifactId=github-plugin
-pluginVersion=1.0.1
+pluginVersion=1.0.2

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

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 }

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

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"

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