feat(sync): keep the engine up when the server refuses, and let the reason be read

scripts/internal/remote-sync.mjs

@@ -2,7 +2,7 @@
 import { createHash } from "node:crypto";
 import { constants } from "node:fs";
 import { spawn } from "node:child_process";
-import { appendFile, lstat, mkdir, open, readFile, rename, rmdir, stat, writeFile } from "node:fs/promises";
+import { appendFile, lstat, mkdir, open, readFile, rename, rm, rmdir, stat, writeFile } from "node:fs/promises";
 import { basename, dirname, join, resolve, sep } from "node:path";
 import process from "node:process";
 import { realpathSync } from "node:fs";
@@ -142,6 +142,111 @@ async function recordCycleSuccess(team, at, writeFileCall = writeFile) {
   } catch { /* best-effort: never fail a working cycle over its own bookkeeping */ }
 }
 
+// A refusal the caller can act on, recorded where something can read it (#773).
+//
+// A remote may answer a write with a status meaning "the caller must do
+// something" rather than "try again later". The engine used to treat that as a
+// transport failure: not retryable, so it left the loop, and the process
+// exited. `status` then said "engine stopped -- run: remote.sh sync start",
+// which invites the one action that cannot work; starting it again produces
+// the same refusal and the same exit, for as long as the server's answer
+// stands.
+//
+// THE REASON WAS NEVER MISSING. `event()` writes `fatal` to stdout and
+// `sync start` captures it into `run/remote-sync.<team>.log`, with a
+// timestamp, at a known path. What was missing is a place to READ it from:
+// `status` opens a pidfile and the cycle stamp, and nothing an agent consults
+// mentions the log. So this is one more fact beside the cycle stamp, not a new
+// mechanism -- the same move #760 made.
+//
+// WHAT IS STORED, AND WHAT IS DELIBERATELY NOT.
+//
+// Stored: the status, the code, the time, and the host from the endpoint the
+// config already holds. Verbatim, as the server said them.
+//
+// Not stored: any sentence about what the refusal MEANS or what to do about
+// it. This engine talks to *a* remote -- self-hosted, someone else's, or a
+// service -- and it cannot know why a particular one refused. A sentence it
+// invents is wrong for some server. Interpretation belongs to whoever operates
+// that server, and the host is there so a reader knows who that is.
+function refusalPath(team) {
+  const connectionRoot = process.env.AGMSG_SYNC_CONNECTION_DIR ?? process.env.SKILL_DIR;
+  if (!connectionRoot) throw new Error("sync connection root is unavailable");
+  return join(connectionRoot, "run", `remote-sync.${team}.refusal.json`);
+}
+
+/**
+ * A refusal is a 4xx the retry policy does not cover.
+ *
+ * BY CLASS, NOT BY NUMBER. There is one such status in use today, and it is
+ * deliberately not named anywhere in this file — not in the code and not in
+ * this comment, which a check enforces. A server may refuse for reasons this
+ * protocol never enumerates, and every one of them is "the server decided, and
+ * said so" rather than "ask again later"; naming one would invite the next
+ * reader to special-case it, and the one after that to add a sentence about
+ * what it means.
+ *
+ * Everything in the 4xx range that the retry policy does not cover lands here,
+ * for that same reason. 5xx stays a transport failure and the retryable
+ * statuses stay retryable, because `isRetryable` is asked first.
+ *
+ * 5xx stays a transport failure, and the retryable 4xx (408, 429) stay
+ * retryable, because `isRetryable` is asked first.
+ */
+/** The host of an endpoint, or null when it cannot be read as a URL. */
+function hostOf(endpoint) {
+  try {
+    return new URL(endpoint).host;
+  } catch {
+    return null;
+  }
+}
+
+export function isRefusal(error) {
+  const status = error?.status;
+  if (typeof status !== "number") return false;
+  if (isRetryable(error)) return false;
+  return status >= 400 && status < 500;
+}
+
+/**
+ * Write the refusal down. Best-effort, like the cycle stamp and for the same
+ * reason: bookkeeping must never be the thing that takes syncing down.
+ */
+async function recordRefusal(team, fact, writeFileCall = writeFile) {
+  try {
+    await writeFileCall(refusalPath(team), `${JSON.stringify({
+      type: "sync_refusal", ...fact,
+    })}\n`);
+  } catch { /* best-effort */ }
+}
+
+/**
+ * Forget it, because a cycle has since succeeded.
+ *
+ * A refusal that outlives its truth is worse than no record: `status` would
+ * keep reporting a server decision that has been reversed, and the operator
+ * would keep acting on it.
+ *
+ * BEST-EFFORT, AND NOT WHAT MAKES THAT TRUE. This removal can fail — an
+ * unwritable run directory, a permission change, a crash between the two
+ * writes — and it is deliberately not retried or escalated, for the same
+ * reason `recordCycleSuccess` is not: bookkeeping must never take down a cycle
+ * that worked.
+ *
+ * What makes the guarantee hold is on the READING side: `remote.sh` compares
+ * the record to the last successful cycle and reports nothing older. Deleting
+ * can fail; comparing cannot. An earlier version of this comment said the two
+ * facts were written in the same place and so could never disagree — they are,
+ * and they still could, because one of the two writes is allowed to fail
+ * (raised in review).
+ */
+async function clearRefusal(team, rmCall = rm) {
+  try {
+    await rmCall(refusalPath(team), { force: true });
+  } catch { /* best-effort */ }
+}
+
 // What actually went wrong, when the thing that threw is a wrapper.
 //
 // `fetch` rejects with a bare `TypeError: fetch failed` whose own `code` is
@@ -2722,6 +2827,9 @@ export async function runLoop(config, options, dependencies = {}) {
   const eventCall = dependencies.eventCall ?? event;
   const isRetryableCall = dependencies.isRetryableCall ?? isRetryable;
   const recordCycleCall = dependencies.recordCycleCall ?? recordCycleSuccess;
+  const clearRefusalCall = dependencies.clearRefusalCall ?? clearRefusal;
+  const recordRefusalCall = dependencies.recordRefusalCall ?? recordRefusal;
+  const isRefusalCall = dependencies.isRefusalCall ?? isRefusal;
   const nowCall = dependencies.nowCall ?? (() => new Date().toISOString());
 
   // An explicit --limit is a ceiling for BOTH push and pull (request size /
@@ -2751,6 +2859,11 @@ export async function runLoop(config, options, dependencies = {}) {
       // success. Same shape as the `cycle.error` logging below.
       try {
         await recordCycleCall(config.local_team, nowCall());
+        // A success outdates any refusal on record. Cleared HERE, beside the
+        // stamp, so a reader that never looks at the timestamps still sees the
+        // right thing most of the time — but the guarantee is the reader's
+        // comparison, not this line, because this line may fail.
+        await clearRefusalCall(config.local_team);
       } catch { /* bookkeeping is best-effort; the cycle already succeeded */ }
       catchUp = result?.pushSaturated === true;
       // catch-up removes the wait ONLY between successful, progress-making
@@ -2763,6 +2876,40 @@ export async function runLoop(config, options, dependencies = {}) {
       try {
         await eventCall("cycle.error", { message: error.message, ...causeOf(error) });
       } catch { /* logging is best-effort */ }
+
+      // A REFUSAL DOES NOT LEAVE THE LOOP (#773).
+      //
+      // It is not retryable — asking again does not change a decision — and it
+      // is not a transport failure either: the server has answered, and said
+      // what it decided. Exiting on it turns a recoverable condition into a
+      // dead process whose `status` line recommends starting it again, which
+      // reproduces the refusal and the exit.
+      //
+      // And the answer CAN change, out of band: someone pays, a quota resets,
+      // an operator fixes a setting. Staying up means the next cycle recovers
+      // with nobody typing anything. So this backs off to the longest interval
+      // and keeps asking quietly, and the failure count is not advanced —
+      // a refusal is not evidence that the transport is degrading.
+      if (isRefusalCall(error)) {
+        try {
+          await recordRefusalCall(config.local_team, {
+            status: error?.status ?? null,
+            code: error?.code ?? null,
+            at: nowCall(),
+            // From the config the engine already holds. Not an interpretation:
+            // it says WHERE the operator of that server would be reached.
+            endpoint_host: hostOf(config.endpoint),
+          });
+        } catch { /* best-effort */ }
+        try {
+          await eventCall("cycle.refused", {
+            status: error?.status ?? null, code: error?.code ?? null,
+          });
+        } catch { /* logging is best-effort */ }
+        await sleepCall(MAX_BACKOFF_MS);
+        continue;
+      }
+
       if (!isRetryableCall(error)) throw error;
       consecutiveFailures += 1;
       const backoffMs = Math.min(MAX_BACKOFF_MS, BASE_BACKOFF_MS * 2 ** (consecutiveFailures - 1));

scripts/remote.sh

@@ -1358,6 +1358,57 @@ _remote_sync_engine_pidfile() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1
 # Written by the engine after a cycle completes (#756). Derived here the same way
 # the pidfile is, because it has the same lifetime and the same owner.
 _remote_sync_engine_cycle_stamp() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1.cycles.json"; }
+# Written by the engine when the server REFUSED, and removed by the engine on
+# the next successful cycle (#773). Same directory, same derivation, same
+# lifetime as the two above.
+#
+# The reason for its existence is that the reason was already on disk and
+# unread: `event()` writes a `fatal` line into the run log with a timestamp,
+# and nothing `status` opens has ever mentioned it. This is a place to read
+# from, not a new place to write.
+_remote_sync_engine_refusal() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1.refusal.json"; }
+
+# The refusal record, or nothing when a cycle has SINCE succeeded.
+#
+# THE READER DOES NOT TRUST THE DELETE. The engine removes this file after a
+# successful cycle, and that removal is best-effort — an unwritable run
+# directory, a permission change, a crash between the two writes. If the
+# removal is the only thing standing between a reversed decision and the
+# operator, then a failed removal reports a refusal that is no longer true,
+# for ever, on every `status` (raised in review).
+#
+# So the file is not the answer; the file COMPARED TO THE LAST SUCCESSFUL
+# CYCLE is. Deleting can fail. Comparing cannot. That makes this the stronger
+# of the two guarantees — "it does not lie even when the delete failed" rather
+# than "the delete is made certain" — and it is why the clear stays
+# best-effort in the engine rather than growing a retry.
+#
+# The comparison is lexicographic on the two timestamps, which is exact
+# because BOTH are written by the same engine in the same format (an ISO-8601
+# UTC instant). It is not a general date comparison and must not be reused as
+# one.
+#
+# With no cycle stamp there is nothing to compare against, and the refusal is
+# reported: an engine that has refused and never succeeded is exactly the case
+# the record exists for.
+_remote_sync_engine_refusal_current() {
+  local team="$1" file stamp last_cycle refused_at
+  file="$(_remote_sync_engine_refusal "$team")"
+  [ -f "$file" ] || return 0
+  # Quiet on purpose: an unreadable record is ABSENT, and saying so on stderr
+  # would put a parse error into the same stream a caller reads the JSON from
+  # (measured — it turned the "unreadable reads as absent" case red).
+  refused_at="$(_remote_read_config_field "$file" '$.at' 2>/dev/null)"
+  [ -n "$refused_at" ] && [ "$refused_at" != "null" ] || return 0
+  stamp="$(_remote_sync_engine_cycle_stamp "$team")"
+  if [ -f "$stamp" ]; then
+    last_cycle="$(_remote_read_config_field "$stamp" '$.last_success_at' 2>/dev/null)"
+    if [ -n "$last_cycle" ] && [ "$last_cycle" != "null" ] && [[ "$last_cycle" > "$refused_at" ]]; then
+      return 0
+    fi
+  fi
+  cat "$file" 2>/dev/null || true
+}
 
 # _remote_holds_current_key <team> -> 0 when this machine holds the identity
 # for the team's CURRENT epoch, 1 otherwise.
@@ -2133,6 +2184,34 @@ _remote_status_one() {
       echo "		cycles: last successful sync $last_cycle"
     fi
   fi
+  # WHY IT IS NOT SYNCING, when the server has said so (#773).
+  #
+  # Printed for a running engine AND a stopped one: a refusal recorded by an
+  # engine that has since been stopped is still the last thing the server said,
+  # and the operator asking "why" is owed it either way.
+  #
+  # REPEATED, NOT INTERPRETED. The status and the code are the server's words.
+  # This client talks to *a* remote — self-hosted, someone else's, or a service
+  # — and cannot know what a particular one means by a particular code. A
+  # sentence invented here is wrong for some server. What it may add is where
+  # the operator of that server would be reached, which is the host the
+  # binding already names.
+  local refusal_raw refusal_status refusal_code refusal_at refusal_host
+  # Through the currency check, not straight off the file — see the helper.
+  refusal_raw="$(_remote_sync_engine_refusal_current "$team")"
+  if [ -n "$refusal_raw" ]; then
+    local refusal_file="$(_remote_sync_engine_refusal "$team")"
+    refusal_status="$(_remote_read_config_field "$refusal_file" '$.status')"
+    refusal_code="$(_remote_read_config_field "$refusal_file" '$.code')"
+    refusal_at="$(_remote_read_config_field "$refusal_file" '$.at')"
+    refusal_host="$(_remote_read_config_field "$refusal_file" '$.endpoint_host')"
+    if [ -n "$refusal_status" ] && [ "$refusal_status" != "null" ]; then
+      echo "		refused: the server answered $refusal_status${refusal_code:+ $refusal_code} at $refusal_at"
+      if [ -n "$refusal_host" ] && [ "$refusal_host" != "null" ]; then
+        echo "		         that server is $refusal_host — what the answer means is theirs to say"
+      fi
+    fi
+  fi
   if [ "$binding_cipher" = "age-v1" ]; then
     if [ -n "$key_id" ] && [ "$key_id" != "null" ]; then
       echo "		encryption: age-v1, key present"
@@ -2202,8 +2281,22 @@ _remote_status_json_one() {
   _remote_config_shape_ok "$raw" || return 2
 
   IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team")
-  printf '%s' "$raw" | python3 -c '
-import json, sys
+  # THE SURFACE AN AGENT READS (#773).
+  #
+  # Someone asks their agent "why isn't this syncing?" and the agent has to be
+  # able to answer. `/agmsg remote status` runs this with `--json`, and it is
+  # the only thing an agent consults about the engine — so the refusal has to
+  # be here, not only in a line a human reads.
+  #
+  # Passed as a whole file rather than as parsed fields: whatever the engine
+  # recorded is what the reader gets, and adding a field there does not need a
+  # change here.
+  # Same currency check as the human line, from the same helper: the two must
+  # not be able to disagree about whether a refusal still stands.
+  local refusal_raw
+  refusal_raw="$(_remote_sync_engine_refusal_current "$team")"
+  printf '%s' "$raw" | REFUSAL_JSON="$refusal_raw" python3 -c '
+import json, os, sys
 team, engine_state, engine_pid_text = sys.argv[1:4]
 try:
     cfg = json.loads(sys.stdin.read())
@@ -2219,6 +2312,12 @@ engine_pid = int(engine_pid_text) if engine_pid_text else None
 if state == "disconnected":
     engine_state = "stopped"
     engine_pid = None
+try:
+    refusal = json.loads(os.environ.get("REFUSAL_JSON") or "null")
+except Exception:
+    refusal = None
+if not isinstance(refusal, dict):
+    refusal = None
 print(json.dumps({
     "local_team": team,
     "endpoint": binding.get("endpoint"),
@@ -2228,6 +2327,10 @@ print(json.dumps({
     "state": state,
     "engine_state": engine_state,
     "engine_pid": engine_pid,
+    # null when the server has refused nothing, or when what was recorded
+    # cannot be read. An unreadable record is reported as absent rather than
+    # guessed at; the human line above is derived from the same file.
+    "refusal": refusal,
 }, sort_keys=True))
 ' "$team" "$engine_state" "$engine_pid"
 }