diff --git a/.agents/skills/release/SKILL.md b/.agents/skills/release/SKILL.md index 2f000e526..47dea3dd9 100644 --- a/.agents/skills/release/SKILL.md +++ b/.agents/skills/release/SKILL.md @@ -32,11 +32,22 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download//dev/null 2>&1; then + echo "Modrinth already holds version $number (HTTP $code); confirming it by read-back." + code="$(api "$TMP/existing.json" "$API/project/$MODRINTH_PROJECT_ID/version")" + if [ "$code" != "200" ]; then + echo "::error::Modrinth rejected version $number as a duplicate and the existing" + echo "::error::versions could not be listed to confirm it (HTTP $code)." + exit 1 + fi + version_id="$(jq -r --arg n "$number" \ + 'first(.[] | select(.version_number == $n) | .id) // ""' "$TMP/existing.json")" + if [ -z "$version_id" ]; then + echo "::error::Modrinth rejected version $number as a duplicate but does not list" + echo "::error::that version number; the create response and the listing disagree." + exit 1 + fi + else + echo "::error::Modrinth rejected version $number (HTTP $code)." + cat "$TMP/created.json" || true + exit 1 + fi + else + version_id="$(jq -r '.id' "$TMP/created.json")" fi - version_id="$(jq -r '.id' "$TMP/created.json")" + if [ -z "$version_id" ]; then + echo "::error::Modrinth accepted version $number but named no version id; there is" + echo "::error::nothing to read back, so the upload cannot be confirmed." + exit 1 + fi # The create response is the API describing its own request, which # is the same "trust the run, not the artifact" mistake the release @@ -846,17 +889,34 @@ jobs: # and assert on the digests Modrinth computed from the bytes it # actually holds. Size is not enough: two different jars can share # a size and cannot share a digest. - code="$(api "$TMP/stored.json" "$API/version/$version_id")" - if [ "$code" = "401" ] || [ "$code" = "403" ]; then - echo "::error::MODRINTH_TOKEN was refused (HTTP $code) reading version $number back." - echo "::error::Reading a version back requires the VERSION_READ scope." - exit 1 - fi - if [ "$code" != "200" ]; then - echo "::error::Could not read version $number back from Modrinth (HTTP $code);" - echo "::error::the upload cannot be confirmed to have stored our jar." - exit 1 - fi + # + # Only that read-back is retried, and only the one answer that means + # "not visible yet" (HTTP 404) is retryable: a refusal (401/403) and + # a digest that does not match are final on the first read, because + # neither becomes true by waiting. A mismatch is never retried, never + # downgraded, and never reported as published. The step still fails + # closed once the attempts are spent. + while :; do + attempt=$((attempt + 1)) + code="$(api "$TMP/stored.json" "$API/version/$version_id")" + + if [ "$code" = "200" ]; then + break + fi + if [ "$code" = "401" ] || [ "$code" = "403" ]; then + echo "::error::MODRINTH_TOKEN was refused (HTTP $code) reading version $number back." + echo "::error::Reading a version back requires the VERSION_READ scope." + exit 1 + fi + if [ "$code" != "404" ] || [ "$attempt" -ge "$read_attempts" ]; then + echo "::error::Could not read version $number back from Modrinth (HTTP $code);" + echo "::error::the upload cannot be confirmed to have stored our jar." + exit 1 + fi + + echo "Modrinth is not serving $number yet (HTTP $code, attempt $attempt of $read_attempts); retrying in ${read_retry_seconds}s. The upload is only reported as published once the read-back matches." + sleep "$read_retry_seconds" + done got_sha1="$(jq -r --arg f "$filename" \ 'first(.files[] | select(.filename == $f) | .hashes.sha1) // ""' "$TMP/stored.json")" diff --git a/core/src/test/java/com/minekube/connect/release/ReleaseModrinthPublishTest.java b/core/src/test/java/com/minekube/connect/release/ReleaseModrinthPublishTest.java index e74d2f865..eb596aee9 100644 --- a/core/src/test/java/com/minekube/connect/release/ReleaseModrinthPublishTest.java +++ b/core/src/test/java/com/minekube/connect/release/ReleaseModrinthPublishTest.java @@ -25,10 +25,12 @@ package com.minekube.connect.release; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.io.InputStream; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -36,6 +38,8 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.junit.jupiter.api.Test; import org.yaml.snakeyaml.Yaml; @@ -257,4 +261,344 @@ void modrinthTokenIsNotInterpolatedIntoTheScript() throws Exception { assertTrue(!modrinthScript(steps).contains("secrets."), "\"" + MODRINTH_STEP + "\" interpolates a secret into its script body"); } + + /** + * A version created a moment ago is not always readable yet. On 0.15.12 this read-back returned + * HTTP 404 twice in a row for versions Modrinth had in fact stored - the release went red twice + * on a tag that had published correctly, and only manual job reruns got it green. The read-back + * therefore has to tolerate a lagging read. + */ + @Test + void modrinthReadBackRetriesUntilTheVersionIsVisible() throws Exception { + Harness harness = runPublishHarness(readBuildJobSteps(), Map.of("STUB_404_READS", "2")); + + assertEquals(0, harness.exitCode, + "a read-back that is not visible yet must not red the step:\n" + harness.output); + assertTrue(harness.output.contains("OK: 0.15.12+velocity published as vid-1"), + "the retried read-back never concluded the publish:\n" + harness.output); + assertEquals(3, harness.readBackCalls, + "expected the two lagging read-backs to be retried and the third to confirm"); + assertEquals(1, harness.createCalls, + "the version was created once; only the read-back may be retried"); + } + + /** + * The retry is a bound, not a wait. A version that never becomes visible still fails the step - + * silently accepting an unconfirmed upload is the failure this step exists to prevent. + */ + @Test + void modrinthReadBackRetryIsBoundedAndFailsClosed() throws Exception { + String script = modrinthScript(readBuildJobSteps()); + int bound = localIntConstant(script, "read_attempts"); + + Harness harness = runPublishHarness(script, Map.of("STUB_404_READS", "99")); + + assertEquals(1, harness.exitCode, + "a version that never becomes readable must still fail the step:\n" + + harness.output); + assertEquals(bound, harness.readBackCalls, + "the read-back is retried " + harness.readBackCalls + " times; its declared bound is " + + bound); + assertTrue(harness.output.contains( + "Could not read version 0.15.12+velocity back from Modrinth (HTTP 404)"), + "the failure does not name the version it could not confirm:\n" + harness.output); + } + + /** + * A refused read is not a lagging read. Retrying a 401/403 cannot change the answer and delays + * the report of a missing scope, so it ends the step on the first read. + */ + @Test + void modrinthReadBackNeverRetriesARefusedRead() throws Exception { + Harness harness = runPublishHarness( + readBuildJobSteps(), Map.of("STUB_READ_CODE", "403")); + + assertEquals(1, harness.exitCode, + "a refused read-back must fail the step:\n" + harness.output); + assertEquals(1, harness.readBackCalls, + "a refused read-back was retried; a token that cannot read cannot be waited out"); + assertTrue(harness.output.contains("was refused (HTTP 403)"), + "the failure does not report the refusal:\n" + harness.output); + } + + /** + * The property that matters most about the retry: a digest mismatch is final. The bytes are + * wrong, waiting cannot make them right, and the step must never turn a mismatch into a pass - + * so it is not retried, not downgraded, and reported on the first read that sees it. + */ + @Test + void modrinthReadBackNeverRetriesADigestMismatch() throws Exception { + Harness harness = runPublishHarness(readBuildJobSteps(), Map.of( + "STUB_STORED_SHA1", "0000000000000000000000000000000000000000")); + + assertEquals(1, harness.exitCode, + "a digest mismatch must fail the step:\n" + harness.output); + assertEquals(1, harness.readBackCalls, + "the step retried a version whose stored bytes do not match; a mismatch must be " + + "final, never retried and never skipped"); + assertTrue(harness.output.contains("Modrinth is serving different bytes"), + "the mismatch is not reported as different bytes:\n" + harness.output); + } + + /** + * A duplicate version number is the listing saying it already holds this version - inventory, + * not a failure, and not a create to retry (creating again cannot succeed, and re-uploading is + * not a thing). Resolve the existing version and let the same read-back confirm the bytes. + */ + @Test + void modrinthDuplicateCreateIsConfirmedByReadBack() throws Exception { + Harness harness = runPublishHarness(readBuildJobSteps(), + Map.of("STUB_CREATE", "400", "STUB_EMPTY_INVENTORIES", "1")); + + assertEquals(0, harness.exitCode, + "a duplicate create must be resolved by read-back, not red the step:\n" + + harness.output); + assertEquals(1, harness.createCalls, + "the create was retried; a duplicate version number cannot be created again"); + assertEquals(1, harness.readBackCalls, + "a duplicate create was accepted without reading the version back"); + assertTrue(harness.output.contains("OK: 0.15.12+velocity published as vid-1"), + "the duplicate was not confirmed against the stored bytes:\n" + harness.output); + } + + /** + * Pins the shape of the retry itself, so a future edit cannot quietly turn the bounded loop + * into an unbounded one or widen it to answers that waiting cannot fix. The behaviour above is + * executed against a stubbed API; these are the bounds that behaviour is bounded by. + */ + @Test + void modrinthReadBackRetryIsBoundedWithABackoff() throws Exception { + String script = modrinthScript(readBuildJobSteps()); + + int attempts = localIntConstant(script, "read_attempts"); + assertTrue(attempts >= 2 && attempts <= 10, + "read-back attempt bound " + attempts + " is not a sane retry budget"); + + int retrySeconds = localIntConstant(script, "read_retry_seconds"); + assertTrue(retrySeconds >= 1 && retrySeconds <= 30, + "read-back retry delay " + retrySeconds + "s is not a sane backoff"); + + assertTrue(Pattern.compile("(?m)^\\s*while :; do\\s*$").matcher(script).find(), + "\"" + MODRINTH_STEP + "\" has no bounded read-back retry loop"); + assertTrue(script.contains("sleep \"$read_retry_seconds\""), + "\"" + MODRINTH_STEP + "\" retries the read-back without backing off"); + assertTrue(Pattern.compile("\\[\\s*\"\\$code\"\\s*!=\\s*\"404\"\\s*\\]").matcher(script).find(), + "\"" + MODRINTH_STEP + "\" is not restricted to retrying a not-yet-visible " + + "version (HTTP 404); a mismatch or a refusal would be waited out too"); + } + + /** + * The workflow shell is only exercised by a release run, so a syntax error in it would be + * discovered by a tag that has already been cut and announced. Parse it here instead. + */ + @Test + void modrinthPublishStepIsValidShell() throws Exception { + String script = modrinthScript(readBuildJobSteps()); + Path file = Files.createTempFile("modrinth-publish-step", ".sh"); + try { + Files.write(file, script.getBytes(StandardCharsets.UTF_8)); + + Process process = new ProcessBuilder("bash", "-n", file.toString()) + .redirectErrorStream(true).start(); + String output = new String(process.getInputStream().readAllBytes(), + StandardCharsets.UTF_8); + + assertEquals(0, process.waitFor(), + "\"" + MODRINTH_STEP + "\" is not valid shell:\n" + output); + } finally { + Files.deleteIfExists(file); + } + } + + // --------------------------------------------------------------------------------------- + // Harness: executes the step's own shell against a stubbed Modrinth API. + // --------------------------------------------------------------------------------------- + private static final String PUBLISH_FUNCTION = "publish_platform()"; + + /** What a harness run did: exit status, output, and how often the API was actually called. */ + private static final class Harness { + private final int exitCode; + private final String output; + private final int readBackCalls; + private final int createCalls; + + private Harness(int exitCode, String output, int readBackCalls, int createCalls) { + this.exitCode = exitCode; + this.output = output; + this.readBackCalls = readBackCalls; + this.createCalls = createCalls; + } + } + + /** + * Lifts one shell function out of the step so it can be executed instead of described. The + * function ends at the first line that is nothing but a closing brace - the step indents every + * nesting level, so only the function's own terminator is unindented. + */ + private static String extractFunction(String script, String signature) { + Matcher start = Pattern.compile( + "(?m)^\\s*" + Pattern.quote(signature) + "\\s*\\{\\s*$").matcher(script); + assertTrue(start.find(), "\"" + MODRINTH_STEP + "\" has no " + signature + " function"); + + Matcher end = Pattern.compile("(?m)^\\}\\s*$").matcher(script); + end.region(start.end(), script.length()); + assertTrue(end.find(), "the " + signature + " function in \"" + MODRINTH_STEP + + "\" has no closing brace at a nesting boundary"); + + return script.substring(start.start(), end.end()); + } + + private static int localIntConstant(String script, String name) { + Matcher matcher = Pattern.compile( + "(?m)^\\s*local " + Pattern.quote(name) + "=(\\d+)\\s*$").matcher(script); + assertTrue(matcher.find(), "\"" + MODRINTH_STEP + "\" declares no `local " + name + + "=...`; the read-back retry has no bound to hold it"); + return Integer.parseInt(matcher.group(1)); + } + + private static int outputField(String output, String name) { + Matcher matcher = Pattern.compile("(?m)^" + Pattern.quote(name) + "=(\\d+)\\s*$") + .matcher(output); + assertTrue(matcher.find(), "the harness did not report " + name + ":\n" + output); + return Integer.parseInt(matcher.group(1)); + } + + private static Harness runPublishHarness(List> steps, + Map stub) throws Exception { + return runPublishHarness(modrinthScript(steps), stub); + } + + /** + * Runs the step's {@code publish_platform} against a stubbed API whose responses are driven by + * the {@code STUB_*} environment: exactly the answers Modrinth gave in production, including + * the 404-on-a-created-version race that red-ened 0.15.12. + */ + private static Harness runPublishHarness(String script, Map stub) + throws Exception { + Path dir = Files.createTempDirectory("modrinth-publish-harness"); + try { + Path jar = dir.resolve("connect-velocity.jar"); + Files.write(jar, "the bytes this build produced".getBytes(StandardCharsets.UTF_8)); + Path harnessFile = dir.resolve("harness.sh"); + + String harness = String.join("\n", + "set -euo pipefail", + "MODRINTH_PROJECT_ID=PuSyuNRf", + "RELEASE_TAG=0.15.12", + "API=\"https://api.modrinth.com/v2\"", + "UA=\"minekube/connect-java harness\"", + "CHANGELOG=\"changelog\"", + "CHANNEL=release", + "GAME_VERSIONS='[\"1.21\"]'", + "STUB_FILENAME=\"$(basename \"$JAR\")\"", + "TMP=\"$(mktemp -d)\"", + "trap 'rm -rf \"$TMP\"' EXIT", + "WANT_SHA1=\"$(sha1sum \"$JAR\" | awk '{print $1}')\"", + "WANT_SHA512=\"$(sha512sum \"$JAR\" | awk '{print $1}')\"", + "", + "# The step calls api() in a command substitution, so a counter kept in a shell", + "# variable would be incremented in a subshell and lost. Count on disk.", + "count() { local f=\"$TMP/count-$1\";" + + " echo \"$(( $(cat \"$f\" 2>/dev/null || echo 0) + 1 ))\" > \"$f\"; }", + "count_of() { cat \"$TMP/count-$1\" 2>/dev/null || echo 0; }", + "", + "api() {", + " local out=\"$1\"; shift", + " local code=200 n url=\"\" arg", + " for arg in \"$@\"; do", + " case \"$arg\" in \"$API\"/*) url=\"$arg\" ;; esac", + " done", + " case \"$url\" in", + " \"$API/version\")", + " count create", + " code=\"${STUB_CREATE:-200}\"", + " if [ \"$code\" = \"200\" ]; then", + " printf '{\"id\":\"vid-1\"}' > \"$out\"", + " else", + " printf '{\"error\":\"A version with this version number already exists\"}'" + + " > \"$out\"", + " fi", + " ;;", + " \"$API/version/vid-1\")", + " count readback", + " n=\"$(count_of readback)\"", + " if [ \"$n\" -le \"${STUB_404_READS:-0}\" ]; then", + " printf '{\"error\":\"not found\"}' > \"$out\"; code=404", + " elif [ \"${STUB_READ_CODE:-200}\" != \"200\" ]; then", + " printf '{\"error\":\"refused\"}' > \"$out\"; code=\"${STUB_READ_CODE}\"", + " else", + " printf" + + " '{\"id\":\"vid-1\",\"files\":[{\"filename\":\"%s\",\"hashes\":" + + "{\"sha1\":\"%s\",\"sha512\":\"%s\"}}]}' \\", + " \"$STUB_FILENAME\" \"${STUB_STORED_SHA1:-$WANT_SHA1}\"" + + " \"${STUB_STORED_SHA512:-$WANT_SHA512}\" > \"$out\"", + " fi", + " ;;", + " \"$API/project/$MODRINTH_PROJECT_ID/version\")", + " count inventory", + " if [ \"$(count_of inventory)\" -le \"${STUB_EMPTY_INVENTORIES:-99}\" ]; then", + " printf '[]' > \"$out\"", + " else", + " printf '[{\"id\":\"vid-1\",\"version_number\":\"%s\"}]'" + + " \"$RELEASE_TAG+velocity\" > \"$out\"", + " fi", + " ;;", + " *)", + " printf '{}' > \"$out\"; code=500", + " ;;", + " esac", + " printf '%s' \"$code\"", + "}", + "", + "# The step backs off between attempts; the harness should not wait for it.", + "sleep() { echo \"HARNESS_SLEEP $*\" >&2; }", + "", + "# The step's best-effort inventory, taken before it publishes anything.", + "HAVE_INVENTORY=0", + "INVENTORY_CODE=\"$(api \"$TMP/existing.json\"" + + " \"$API/project/$MODRINTH_PROJECT_ID/version\")\"", + "if [ \"$INVENTORY_CODE\" = \"200\" ]; then HAVE_INVENTORY=1; fi", + "", + extractFunction(script, PUBLISH_FUNCTION), + "", + "rc=0", + "# A subshell, because the step reports failure with `exit 1` - in the workflow that", + "# ends the step, here it must only end this publish attempt.", + "( publish_platform velocity \"$JAR\" '[\"velocity\"]' \"Velocity\" ) || rc=$?", + "echo \"PUBLISH_EXIT=$rc\"", + "echo \"READBACK_CALLS=$(count_of readback)\"", + "echo \"CREATE_CALLS=$(count_of create)\""); + + Files.write(harnessFile, harness.getBytes(StandardCharsets.UTF_8)); + + ProcessBuilder builder = new ProcessBuilder("bash", harnessFile.toString()); + builder.redirectErrorStream(true); + builder.environment().put("JAR", jar.toString()); + stub.forEach(builder.environment()::put); + + Process process = builder.start(); + String output = new String(process.getInputStream().readAllBytes(), + StandardCharsets.UTF_8); + + return new Harness(outputField(output, "PUBLISH_EXIT"), output, + outputField(output, "READBACK_CALLS"), outputField(output, "CREATE_CALLS")); + } finally { + deleteRecursively(dir); + } + } + + private static void deleteRecursively(Path root) { + try { + List paths = new ArrayList<>(); + try (java.util.stream.Stream walk = Files.walk(root)) { + walk.forEach(paths::add); + } + paths.sort(java.util.Comparator.reverseOrder()); + for (Path path : paths) { + Files.deleteIfExists(path); + } + } catch (Exception ignored) { + // best effort: the harness directory is throwaway + } + } }