Merge pull request 'feat(sidecar): protocol 6 — carry the key, answer bridge.busy (Phase 11a)' (#36) from feature/protocol-v6-idempotency into edge

Reviewed-on: #36

.gitea/workflows/release.yml

@@ -149,6 +149,39 @@ jobs:
             fi
           fi
 
+          # ── Orphan sweep ────────────────────────────────────────────────
+          #
+          # The check above is VERSION-SCOPED: it only ever asks about the one
+          # version this run computed. That is enough to recover an orphan on
+          # the very next run, and useless afterwards — once any releasable
+          # commit lands, the next run computes a NEW version, never looks at
+          # the old tag again, and the orphan becomes permanent and silent.
+          #
+          # servuo-plugins v0.1.0 is the proof, and the proof is pointed: the
+          # commit that ADDED the recovery above was itself typed
+          # `fix(release): ... recover the orphaned v0.1.0 tag`, so it bumped to
+          # v0.1.1 — and the run that introduced the recovery stepped straight
+          # past the tag it was written to rescue. That tag is still orphaned.
+          #
+          # So every v* tag is checked, and anything missing a release is
+          # WARNED about. Deliberately not recovered: publishing an old version
+          # would mean building today's tree and shipping it under a tag whose
+          # tree it is not, which is worse than the inconsistency it fixes.
+          # A human decides whether to recover or drop it.
+          #
+          # Never fails the run. A sweep that can break a good release is a
+          # sweep someone will delete.
+          ORPHANS=""
+          for T in $(git tag -l 'v*' --sort=-v:refname); do
+            T_HTTP="$(curl -s -o /dev/null -w '%{http_code}' \
+              -H "Authorization: token $(printf '%s' "${REGISTRY_TOKEN:-}" | tr -d '\r\n')" \
+              "https://${GITEA_HOST}/api/v1/repos/${REPO}/releases/tags/${T}" || echo 000)"
+            [ "$T_HTTP" = "404" ] && ORPHANS="${ORPHANS} ${T}"
+          done
+          if [ -n "${ORPHANS}" ]; then
+            echo "::warning::Tags with no release:${ORPHANS} — a run failed after tagging. Publish or delete them; this job will not do either."
+          fi
+
           # Changelog range. A recovery run has nothing after the tag, so
           # summarize what the tag itself contains rather than emitting an empty
           # list: the range that produced it, i.e. previous-tag..this-tag.
@@ -342,18 +375,75 @@ jobs:
           # corrupt the Authorization header.
           CI_TOKEN="$(printf '%s' "${REGISTRY_TOKEN}" | tr -d '\r\n')"
 
-          REL_ID="$(curl -sSf -X POST "${API}/releases" \
+          PAYLOAD="$(jq -n --arg tag "$TAG" --arg body "$BODY" \
-            -H "Authorization: token ${CI_TOKEN}" \
+                       '{tag_name:$tag, name:$tag, body:$body, draft:false, prerelease:false}')"
-            -H "Content-Type: application/json" \
+
-            -d "$(jq -n --arg tag "$TAG" --arg body "$BODY" \
+          # This POST is the step that orphaned tag v0.1.1 (run 75): it landed one
-                   '{tag_name:$tag, name:$tag, body:$body, draft:false, prerelease:false}')" \
+          # second after the tag push and Gitea answered 500, having not finished
-            | jq -r '.id')"
+          # processing the pushed tag. Re-running the workflow published the same
+          # four assets untouched, so the failure was a race, not a bad request.
+          #
+          # Two things went wrong there, and both are fixed here.
+          #
+          # 1. `curl -sSf` prints NO response body on an error status, so all the
+          #    log carried was "curl: (22) ... error: 500" and the cause had to be
+          #    inferred from timestamps. Capture the body and print it.
+          # 2. Nothing retried, so a transient 5xx became a permanent orphan tag.
+          #    The plan step CAN recover one, but only on a run that reaches it --
+          #    and a later push with no releasable commits stands down before it
+          #    gets there, so in practice the tag sits until a human notices.
+          #
+          # 4xx is deliberately NOT retried: a bad token or a malformed body does
+          # not improve by being sent again, and retrying only turns a clear
+          # failure into a slow one.
+          REL_ID=""
+          for attempt in 1 2 3 4 5; do
+            HTTP="$(curl -s -o /tmp/rel.json -w '%{http_code}' -X POST "${API}/releases" \
+              -H "Authorization: token ${CI_TOKEN}" \
+              -H "Content-Type: application/json" \
+              -d "${PAYLOAD}" || echo 000)"
+
+            if [ "$HTTP" = "201" ] || [ "$HTTP" = "200" ]; then
+              REL_ID="$(jq -r '.id' /tmp/rel.json)"
+              break
+            fi
+
+            echo "::warning::POST /releases attempt ${attempt} returned HTTP ${HTTP}"
+            echo "--- response body ---"
+            cat /tmp/rel.json || true
+            echo
+            echo "---------------------"
+
+            case "$HTTP" in
+              4*) echo "::error::HTTP ${HTTP} is a client error - not retrying."; exit 1 ;;
+            esac
+
+            if [ "$attempt" = 5 ]; then
+              echo "::error::POST /releases still failing after 5 attempts. Tag ${TAG} is pushed but has no release."
+              echo "::error::Re-run this workflow - the plan step detects the orphan tag and republishes it."
+              exit 1
+            fi
+            sleep $(( attempt * 5 ))
+          done
+
+          if [ -z "$REL_ID" ] || [ "$REL_ID" = "null" ]; then
+            echo "::error::Release created but no id came back; refusing to upload assets blind."
+            exit 1
+          fi
           echo "Created release ${TAG} (id=${REL_ID})"
 
           for f in "${BIN}-linux-x86_64" "${BIN}-linux-aarch64" "${BIN}-windows-x86_64.exe" SHA256SUMS; do
-            curl -sSf -X POST "${API}/releases/${REL_ID}/assets?name=${f}" \
+            # Same treatment. An upload that fails quietly leaves a release whose
+            # SHA256SUMS does not cover every binary it advertises, which is worse
+            # than no release at all -- that file IS the trust anchor.
+            HTTP="$(curl -s -o /tmp/asset.json -w '%{http_code}' -X POST "${API}/releases/${REL_ID}/assets?name=${f}" \
               -H "Authorization: token ${CI_TOKEN}" \
-              -F "attachment=@dist/${f}" >/dev/null
+              -F "attachment=@dist/${f}" || echo 000)"
+            if [ "$HTTP" != "201" ] && [ "$HTTP" != "200" ]; then
+              echo "::error::uploading ${f} returned HTTP ${HTTP}"
+              cat /tmp/asset.json || true
+              exit 1
+            fi
             echo "  uploaded ${f}"
           done
 

sidecar/src/main.rs

@@ -52,7 +52,46 @@ use tracing_subscriber::EnvFilter;
 /// kinds are new, `GET /guilds` grows a `roster` key, and nothing existing changed shape. This is the
 /// first bump that also needed a **store migration** (`guilds.members`), because it is the first to
 /// add a column to a table that already exists rather than a whole new table; see `store::migrate`.
-pub const PROTOCOL_VERSION: u32 = 4;
+///
+/// v5 (Protocol 5): three enrichments that are additive in the same way again, bumped together
+/// rather than one at a time because a protocol bump is not cheap here — it costs a sidecar
+/// release, a republished bundle and an operator update on every shard, so a field left out costs
+/// a whole second round of that rather than a follow-up commit. They are:
+///
+///   * `house.decay` gains `ownerName` and a decay SCHEDULE — `nextStage`, `decayPeriodSec`,
+///     `dynamicDecay`, and `estimatedCollapse` only where it is exactly knowable (at IDOC under
+///     dynamic decay; at any stage under static decay, which has no randomness to wait out).
+///   * `vendor.listing` gains `ownerAcct` — without which the frame names an owner nobody can
+///     resolve to a person — and a `fees` object carrying the charge, the funds, the pay interval
+///     and the resolved `dismissalAt`.
+///   * `account.login.result` is a NEW kind: the verdict of a login, which the pre-existing
+///     `account.login.attempt` structurally cannot carry (its EventSink fires before the auth
+///     decision is made).
+///
+/// **No store migration this time**, unlike v4. Every frame is persisted whole and the board tables
+/// index only the columns they already had, so the new fields ride inside the stored JSON and the
+/// new kind lands in `events` like any other. That is the dumb-forwarder property doing its job:
+/// the sidecar defines no schema for a frame's contents and so needs no change when they grow.
+///
+/// v6 (Protocol 6): the first bump that is about a GUARANTEE rather than about data, and the first
+/// the sidecar mostly gets for free. Two things:
+///
+///   * **`idempotencyKey` on inbound commands.** A command that carries one is executed by the shard
+///     at most once; a repeat is answered with the original reply rather than re-run. That is what
+///     makes a world-writing verb retryable at all — until now a lost acknowledgement was
+///     indistinguishable from a command that never applied, so the website had to declare every
+///     write un-retryable and accept losing one rather than risk doubling it. The sidecar's part is
+///     to CARRY the key (it rides in the command body, which every write endpoint already passes
+///     through verbatim) and to understand the one new answer the shard can now give: `bridge.busy`,
+///     meaning a command under that key is still in flight. See `web::respond`.
+///   * **`champ.boss.killed` is a new kind**: a champion's defeat, with the damage table only the
+///     shard ever sees. It was previously inferable from `champ.update` going `bossUp` true then
+///     false alongside a nearby `mob.killed`, which is fragile and says nothing about who did the
+///     work. It lands in `events` and on the feed like any other kind, with no code here at all —
+///     the dumb-forwarder property again.
+///
+/// **No store migration.** Nothing gains a column; the new kind is persisted whole like every other.
+pub const PROTOCOL_VERSION: u32 = 6;
 
 // Not `#[tokio::main]`: on Windows the SCM dispatcher takes over this thread and starts the runtime
 // itself, on its own thread, once the service actually begins. The runtime is built by whichever

sidecar/src/web.rs

@@ -246,13 +246,31 @@ fn constant_time_eq(a: &[u8], b: &[u8]) -> bool {
 
 // ---- shared reply handling ----
 
+/// Protocol 6. `bridge.busy` says a command carrying this `idempotencyKey` is already in flight on
+/// the shard: nothing was run, and the caller should come back.
+///
+/// It maps to **425 Too Early**, which is what that status is for — a server unwilling to risk
+/// processing a request that might be a replay. The obvious alternative, 409, is already the
+/// protocol-version gate's answer, and those two want opposite dispositions from a client: a version
+/// mismatch is a deployment fault nobody should retry, and a busy shard is a retry that should
+/// succeed on its own. Sharing a status would have made the difference readable only by inspecting
+/// the body, which is exactly how a retry loop ends up hiding a mismatched deployment.
+///
+/// It is checked BEFORE the `.error` suffix test in each responder below, and it is deliberately not
+/// spelled `bridge.busy.error`: nothing is wrong. The work is happening.
+const BUSY_KIND: &str = "bridge.busy";
+const BUSY_STATUS: StatusCode = StatusCode::TOO_EARLY;
+
 /// Turns an RPC result into an HTTP response. A `bridge.error` reply from the shard becomes a 4xx;
-/// a real reply is returned as-is; transport failures map to 503/504.
+/// a `bridge.busy` reply becomes a 425; a real reply is returned as-is; transport failures map to
+/// 503/504.
 fn respond(result: Result<Value, RpcError>) -> (StatusCode, Json<Value>) {
     match result {
         Ok(value) => {
             let kind = value.get("kind").and_then(|k| k.as_str()).unwrap_or("");
-            if kind == "bridge.error" || kind.ends_with(".error") {
+            if kind == BUSY_KIND {
+                (BUSY_STATUS, Json(value))
+            } else if kind == "bridge.error" || kind.ends_with(".error") {
                 let reason = value
                     .get("reason")
                     .and_then(|r| r.as_str())
@@ -286,7 +304,9 @@ fn respond_admin(result: Result<Value, RpcError>) -> (StatusCode, Json<Value>) {
     match result {
         Ok(value) => {
             let kind = value.get("kind").and_then(|k| k.as_str()).unwrap_or("");
-            if kind == "admin.error" {
+            if kind == BUSY_KIND {
+                (BUSY_STATUS, Json(value))
+            } else if kind == "admin.error" {
                 let reason = value
                     .get("reason")
                     .and_then(|r| r.as_str())
@@ -324,7 +344,9 @@ fn respond_account(result: Result<Value, RpcError>) -> (StatusCode, Json<Value>)
     match result {
         Ok(value) => {
             let kind = value.get("kind").and_then(|k| k.as_str()).unwrap_or("");
-            if kind == "account.error" {
+            if kind == BUSY_KIND {
+                (BUSY_STATUS, Json(value))
+            } else if kind == "account.error" {
                 let reason = value
                     .get("reason")
                     .and_then(|r| r.as_str())
@@ -454,6 +476,16 @@ async fn link_delete(
 /// Forwards a staff moderation command to the shard, correlated on a fresh reqId. Injects `kind`
 /// and `reqId`, requiring the caller-supplied `actor` up front (the shard enforces it too). The
 /// body's remaining fields (account/serial/durationSec/reason/text/hue) pass straight through.
+///
+/// **Protocol 6: `idempotencyKey` is one of those remaining fields**, and passing it through is the
+/// whole of the sidecar's part in the guarantee. It is worth stating rather than leaving to the
+/// word "remaining", because a later refactor that narrowed this to a known field list would quietly
+/// turn every retried world write back into a possible duplicate, and nothing here would fail.
+///
+/// The key belongs to the CALLER's unit of work — the website's event step — so the sidecar neither
+/// generates one nor validates it. Note also that `reqId` is regenerated on every call: a retry
+/// carries the same idempotency key under a NEW correlation id, which is exactly why the shard
+/// re-stamps a replayed reply rather than echoing the id the first attempt used.
 async fn admin_call(st: &AppState, kind: &str, body: Value) -> (StatusCode, Json<Value>) {
     let mut obj = match body {
         Value::Object(m) => m,
@@ -978,3 +1010,67 @@ async fn ws_client(mut socket: WebSocket, state: AppState) {
 
     info!("ws client disconnected");
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn reply(kind: &str) -> Result<Value, RpcError> {
+        Ok(json!({"t": 1, "kind": kind, "reqId": "r-9"}))
+    }
+
+    /// Protocol 6. Every responder must recognise `bridge.busy`, because every write plane can be
+    /// retried: the staff plane, the account plane and the plain command plane all reach handlers
+    /// that a keyed retry can arrive at. A responder that missed it would return 200 with a body
+    /// saying nothing happened, which is the worst of the three possible answers.
+    #[test]
+    fn busy_maps_to_425_on_every_plane() {
+        assert_eq!(respond(reply("bridge.busy")).0, StatusCode::TOO_EARLY);
+        assert_eq!(respond_admin(reply("bridge.busy")).0, StatusCode::TOO_EARLY);
+        assert_eq!(
+            respond_account(reply("bridge.busy")).0,
+            StatusCode::TOO_EARLY
+        );
+    }
+
+    /// 425 must not collide with the protocol-version gate's 409: a mismatch is a deployment fault
+    /// nobody should retry, a busy shard is a retry that will succeed. Same-status would make the
+    /// two readable only by inspecting the body.
+    #[test]
+    fn busy_is_not_the_version_gates_status() {
+        assert_ne!(BUSY_STATUS, StatusCode::CONFLICT);
+    }
+
+    /// A replayed reply is an ordinary success. The shard marks it `replayed: true` for the log, and
+    /// the caller must be able to treat it exactly as it would have treated the answer it lost.
+    #[test]
+    fn a_replayed_reply_is_still_a_200() {
+        let value = json!({"t": 1, "kind": "admin.ok", "reqId": "r-9", "replayed": true});
+        let (status, body) = respond_admin(Ok(value));
+        assert_eq!(status, StatusCode::OK);
+        assert_eq!(body.0.get("replayed").and_then(|v| v.as_bool()), Some(true));
+    }
+
+    /// The error mapping the busy arm is threaded in front of must be untouched by it.
+    #[test]
+    fn errors_still_map_as_before() {
+        assert_eq!(
+            respond(Ok(
+                json!({"kind": "bridge.error", "reason": "unknown account"})
+            ))
+            .0,
+            StatusCode::NOT_FOUND
+        );
+        assert_eq!(
+            respond_admin(Ok(json!({"kind": "admin.error", "reason": "protected"}))).0,
+            StatusCode::FORBIDDEN
+        );
+        assert_eq!(
+            respond_account(Ok(
+                json!({"kind": "account.error", "reason": "already exists"})
+            ))
+            .0,
+            StatusCode::CONFLICT
+        );
+    }
+}