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" 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