From fa195d14bff56127b2a70ab2afad1c42345f01a9 Mon Sep 17 00:00:00 2001 From: Lan Nguyen Si Date: Wed, 5 Aug 2026 20:04:19 +0200 Subject: [PATCH 1/2] feat(memory-sync): escalate when the queue outlives its threshold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A permanently wrong remote (bad remoteUrl, renamed repo path, host without git-upload-pack) queued cleanly with exit 0 on every tick, forever — indistinguishable from a laptop offline. The tick now checks the oldest queued snapshot's already-persisted manifest createdAt after every enqueue and throws RemoteQueueEscalationError (exit 6) once it exceeds queueEscalationThresholdMs (default 24h, i.e. 96 missed ticks at the shipped 15-minute launchd/systemd intervals). Below threshold nothing changes: local commit, queued push, exit 0. The error class deliberately does not extend RemoteUnavailableError so the existing queueing discrimination cannot swallow it. Config key wired end-to-end (file, env, config get/set) after the reachabilityTimeoutMs pattern. Pins from the same review: the deliberate crash-loud fetch boundary in prepareWorkingCopy (stub git failing on fetch, non-zero exit, mutation-verified) and the precheck-bypass path (unsupported-scheme remote failing at lookupRemoteHead still queues cleanly). Docs: README queue-escalation section, machine-setup bullet, both plist template headers incl. the KeepAlive respawn analysis. Task: 11424b5e --- packages/agent-memory-sync/README.md | 41 +++- .../com.agent-memory-sync.sync.plist.template | 13 ++ ...com.agent-memory-sync.watch.plist.template | 19 ++ .../agent-memory-sync/docs/machine-setup.md | 29 +++ .../agent-memory-sync/src/config/loader.ts | 34 ++- packages/agent-memory-sync/src/errors.ts | 29 +++ .../agent-memory-sync/src/memory-sync/push.ts | 73 ++++++- .../src/memory-sync/state-store.ts | 64 +++++- .../remote-unreachable-escalation.test.ts | 203 ++++++++++++++++++ .../integration/watch-mirror-delete.test.ts | 84 ++++++++ .../tests/unit/state-store.test.ts | 86 ++++++++ 11 files changed, 663 insertions(+), 12 deletions(-) create mode 100644 packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts create mode 100644 packages/agent-memory-sync/tests/unit/state-store.test.ts diff --git a/packages/agent-memory-sync/README.md b/packages/agent-memory-sync/README.md index 61972f6..b10ab89 100644 --- a/packages/agent-memory-sync/README.md +++ b/packages/agent-memory-sync/README.md @@ -138,7 +138,7 @@ StartLimitBurst=10 WantedBy=multi-user.target ``` -The `StartLimitIntervalSec` / `StartLimitBurst` pair caps systemd's restart loop for the failures that still exit non-zero — a config/data error raised before the remote working copy is prepared (e.g. a required `syncPaths` entry missing), or any other git-level failure while preparing/committing that working copy (a full disk, a corrupted git config, a broken commit hook, ...) — so a persistently broken cause does not crashloop forever; a remote that is merely unreachable or rejecting the push (see below) no longer exits at all, so it never spends this budget. Inspect `journalctl -u agent-memory-sync-watch.service` for the `snapshot push failed: ...` line `watch` writes to stderr before exiting on one of those failures. +The `StartLimitIntervalSec` / `StartLimitBurst` pair caps systemd's restart loop for the failures that still exit non-zero — a config/data error raised before the remote working copy is prepared (e.g. a required `syncPaths` entry missing), or any other git-level failure while preparing/committing that working copy (a full disk, a corrupted git config, a broken commit hook, ...) — so a persistently broken cause does not crashloop forever; a remote that is merely unreachable or rejecting the push (see below) no longer exits at all, so it never spends this budget. The one exception is [queue escalation](#queue-escalation-a-permanently-broken-remote-does-not-queue-forever): once the queue has been failing to drain past `queueEscalationThresholdMs` (default 24h), a tick DOES exit non-zero again — but only on a real local edit (`watch` is edge-triggered), so it does not spend this budget any faster than this machine's memory actually changes while the remote stays broken. Inspect `journalctl -u agent-memory-sync-watch.service` for the `snapshot push failed: ...` line `watch` writes to stderr before exiting on one of those failures. macOS equivalent (LaunchAgent instead of systemd): see [`docs/launchd/com.agent-memory-sync.watch.plist.template`](docs/launchd/com.agent-memory-sync.watch.plist.template) @@ -173,6 +173,35 @@ fallback-machine setup, not an optional extra — see [docs/machine-setup.md](docs/machine-setup.md) for the launchd/systemd companion jobs. +##### Queue escalation: a permanently broken remote does not queue forever + +The queue-instead-of-crash handling above is deliberately silent for a +remote that is merely *offline* — a laptop closed overnight, on a flight, or +disconnected for a weekend. But a remote that is *correctly* classified +`RemoteUnavailableError` can still be **permanently** wrong (a bad +`remoteUrl`, a renamed repository path, a host that accepts an SSH/TCP +connection but cannot serve the repository) — without a second signal, that +looks identical to a laptop on a plane and would queue cleanly, exit `0`, +forever, never syncing again. + +Every enqueue therefore checks the age of the OLDEST currently-queued +snapshot (`stateDir/queue//manifest.json`'s `createdAt`, already written +on every enqueue — no new state) against `queueEscalationThresholdMs` +(config file / `AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS`, default +24h). Below the threshold, behavior is unchanged: silent, exit `0`, every +tick. Once the oldest queued snapshot is older than the threshold — meaning +the remote has been *continuously* unreachable for that long, not just on +this one tick, since a successful push clears the whole queue at once — the +tick throws instead: a clear message on stderr and a non-zero exit (`6`), +the same supervisor-restart surface a non-network failure already uses. The +snapshot itself is never lost; it stays queued and is replayed automatically +once the remote is reachable again. 24h is sized against this package's own +committed periodic-sync tick interval (900s / 15min — see +[docs/machine-setup.md](docs/machine-setup.md) and the launchd/systemd +templates) — 96 missed ticks, comfortably longer than an overnight or +weekend offline window, still bounding how long a genuinely broken remote +can hide to about a day. + ##### Push authentication `watch` (and `run --mode push`) invoke the system `git` binary; authentication is whatever `git` itself is configured to use, e.g. an SSH key, an OS credential helper, or a `https://x-access-token:$TOKEN@github.com/...` URL. @@ -260,6 +289,7 @@ The `--config` flag overrides the default path. "outputFormat": "text", "verbose": false, "reachabilityTimeoutMs": 4000, + "queueEscalationThresholdMs": 86400000, "syncPaths": [ { "source": "MEMORY.md", "destination": "MEMORY.md", "kind": "file" }, { "source": "logs", "destination": "logs", "kind": "directory" } @@ -309,6 +339,15 @@ Priority order (highest to lowest): CLI flags > environment variables > config f separate on/off switch, this is the supported way to opt out - failed pushes (including ones skipped by the reachability precheck) are queued locally in `stateDir/queue` and replayed on the next successful push +- if the OLDEST queued snapshot is older than `queueEscalationThresholdMs` + (config file / `AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS`, default 24h) — i.e. the + remote has been *continuously* unreachable for that long, not just on this one tick — the + tick throws instead of returning a clean "queued" result: a message on stderr and exit code + `6`, so a permanently misconfigured remote (wrong `remoteUrl`, a renamed repository path, a + host that accepts a connection but cannot serve the repository) does not queue silently + forever. Below the threshold nothing changes: silent, exit `0`, every tick — a merely offline + machine is unaffected. See [watch's "Queue escalation" section](#queue-escalation-a-permanently-broken-remote-does-not-queue-forever) + for the full rationale - append-only concurrent edits are merged automatically; other conflicts default to inline conflict markers - `--dry-run` previews the result without changing local files or the remote repository diff --git a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.sync.plist.template b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.sync.plist.template index 4a084ca..3887110 100644 --- a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.sync.plist.template +++ b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.sync.plist.template @@ -53,6 +53,19 @@ no-op — `run --mode sync`'s own reachability precheck (default 4s bound; see src/memory-sync/reachability.ts) means this never hangs or piles up overlapping ssh attempts. + + Queue escalation: this template's 900s StartInterval is also the tick + cadence the queue-escalation default threshold is sized against — see + src/memory-sync/state-store.ts's DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS + (24h = 96 of these ticks) and docs/machine-setup.md's "Queue escalation" + bullet. If the oldest snapshot in stateDir/queue is older than that + threshold — i.e. the remote has been unreachable for the whole 24h, not + just this one tick — a `run --mode sync` tick exits non-zero (6) instead + of its usual clean 0, with a clear stderr message; that failure is + visible via `launchctl print`'s last-exit-status and this job's + StandardErrorPath log, without launchd respawning anything (this + template has no KeepAlive — it is a plain periodic tick, so a failed run + just tries again at the next StartInterval). --> diff --git a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template index e83d1f7..5bf468f 100644 --- a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template +++ b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template @@ -81,6 +81,25 @@ so pair this job with the periodic-sync companion, com.agent-memory-sync.sync.plist.template — see its header and docs/machine-setup.md for why that companion is required, not optional. + + Queue escalation, and why it DOES respawn (unlike a merely-offline + tick): the queue-instead-of-crash handling above only stays silent + while the queue keeps draining within a bounded time. If the OLDEST + snapshot in stateDir/queue is older than queueEscalationThresholdMs + (default 24h — src/memory-sync/state-store.ts's + DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS, sized against this template's own + companion's 900s StartInterval), the remote has been unreachable for the + WHOLE 24h, not just this one tick — a permanently broken remote (bad + remoteUrl, a renamed repo path, a host that accepts a connection but + can't serve the repository) rather than a laptop offline overnight. That + tick then exits non-zero (6) with a clear stderr message instead of the + usual clean 0, which — per KeepAlive/SuccessfulExit=false above — DOES + trigger a respawn. This is not a tight crash loop: `watch` only ticks on + a real local edit, so it only re-throws (and respawns again) the next + time this machine's memory actually changes while the remote is still + broken, each time writing a fresh, visible line to + watch..err.log — see docs/machine-setup.md's "Queue escalation" + bullet and README.md's "Queue escalation" section under `watch`. --> diff --git a/packages/agent-memory-sync/docs/machine-setup.md b/packages/agent-memory-sync/docs/machine-setup.md index 75beb18..9a97e19 100644 --- a/packages/agent-memory-sync/docs/machine-setup.md +++ b/packages/agent-memory-sync/docs/machine-setup.md @@ -56,6 +56,35 @@ This document wires together the pieces already documented individually git-level failure while that working copy is being prepared or committed (a full disk, a corrupted git config, a broken commit hook, ...) still exit non-zero. +- **Queue escalation: silent-forever is not the same as offline.** The + queue-instead-of-crash behavior above is deliberately quiet for a machine + that is merely offline (closed overnight, on a flight, a weekend without + connectivity) — but a remote that is `RemoteUnavailableError` for a + *permanent* reason (a typo'd `remoteUrl`, a renamed bare-repo path on the + mini, an SSH host that accepts the connection but can no longer serve + `git-upload-pack`) would otherwise look identical: queued, exit `0`, + every tick, forever, never actually syncing again. Every enqueue now + checks the age of the OLDEST currently-queued snapshot + (`stateDir/queue//manifest.json`'s `createdAt`, already written on + every enqueue — no new persisted state) against + `queueEscalationThresholdMs` (config file / + `AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS`, default 24h). Below the + threshold, nothing changes. Once the oldest queued snapshot is older than + the threshold — meaning the remote has been *continuously* unreachable for + that long, not merely on this one tick, since a successful push clears the + whole queue at once — the tick throws instead of returning a clean + "queued" result: a clear message on stderr and a non-zero exit (`6`), the + same supervisor-restart surface a non-network failure already uses (see + the previous bullet). The queued snapshot itself is never lost either way; + it stays queued and replays automatically once the remote is reachable + again. The 24h default is sized against this document's own committed + periodic-sync tick interval — 900s / 15min, see + `docs/launchd/com.agent-memory-sync.sync.plist.template`'s `StartInterval` + (macOS) and the systemd `OnUnitActiveSec=15min` timer in (c) below (Linux) + — 96 missed ticks at that cadence, comfortably past an overnight or + weekend offline window while still bounding a genuinely broken remote's + silence to about a day. See README.md's "Queue escalation" section under + `watch` for the full rationale. - **`watch` is edge-triggered and does not pull — this is why the periodic sync job is required, not optional.** `watch` only commits+pushes when *this* machine's local files change; it never reads from the remote. Its diff --git a/packages/agent-memory-sync/src/config/loader.ts b/packages/agent-memory-sync/src/config/loader.ts index de10427..477ce13 100644 --- a/packages/agent-memory-sync/src/config/loader.ts +++ b/packages/agent-memory-sync/src/config/loader.ts @@ -3,6 +3,7 @@ const { homedir, hostname } = require("node:os"); const path = require("node:path"); const { CliError } = require("../errors"); const { DEFAULT_REACHABILITY_TIMEOUT_MS } = require("../memory-sync/reachability"); +const { DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS } = require("../memory-sync/state-store"); type OutputFormat = "text" | "json" | "yaml"; type RunMode = "sync" | "push" | "pull"; @@ -36,6 +37,7 @@ interface UserConfig { gitBinary?: string; reachabilityTimeoutMs?: number; reachabilityCheckCommand?: string[] | null; + queueEscalationThresholdMs?: number; } interface LoadedConfig { @@ -61,6 +63,7 @@ interface RunConfig extends UserConfig { mode: RunMode; reachabilityTimeoutMs: number; reachabilityCheckCommand: string[] | null; + queueEscalationThresholdMs: number; } interface RunConfigOverrides { @@ -81,6 +84,7 @@ interface RunConfigOverrides { mode?: RunMode; reachabilityTimeoutMs?: number; reachabilityCheckCommand?: string[] | null; + queueEscalationThresholdMs?: number; } const DEFAULT_SYNC_PATHS: SyncPathConfig[] = [ @@ -102,7 +106,8 @@ const DEFAULTS: Omit): UserConfig { reachability_timeout_ms: "reachabilityTimeoutMs", reachabilityTimeoutMs: "reachabilityTimeoutMs", reachability_check_command: "reachabilityCheckCommand", - reachabilityCheckCommand: "reachabilityCheckCommand" + reachabilityCheckCommand: "reachabilityCheckCommand", + queue_escalation_threshold_ms: "queueEscalationThresholdMs", + queueEscalationThresholdMs: "queueEscalationThresholdMs" }; for (const [key, value] of Object.entries(raw)) { @@ -527,6 +547,12 @@ function parseConfigValue(key: string, value: string): unknown { return validateConflictStrategy(value as ConflictStrategy); case "reachabilityTimeoutMs": return validatePositiveInteger(Number(value), "reachabilityTimeoutMs", DEFAULT_REACHABILITY_TIMEOUT_MS); + case "queueEscalationThresholdMs": + return validatePositiveInteger( + Number(value), + "queueEscalationThresholdMs", + DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS + ); case "reachabilityCheckCommand": return value === "null" ? null diff --git a/packages/agent-memory-sync/src/errors.ts b/packages/agent-memory-sync/src/errors.ts index 122da2c..01f71fb 100644 --- a/packages/agent-memory-sync/src/errors.ts +++ b/packages/agent-memory-sync/src/errors.ts @@ -29,6 +29,34 @@ class RemoteUnavailableError extends CliError { } } +// Thrown when the QUEUE — not any single git operation — has been failing +// to drain for longer than the configured escalation threshold (default +// 24h; see StateStore.oldestQueuedSnapshotAgeMs and push.ts's +// checkQueueEscalation). Deliberately NOT a RemoteUnavailableError subclass: +// push.ts's catch discriminates on `instanceof RemoteUnavailableError` to +// decide "queue quietly, exit 0" vs. "something else is wrong, crash loud" +// (see the RemoteUnavailableError comment above) — a RemoteUnavailableError +// raised on any single tick is exactly that: one tick could not reach the +// remote, which by itself is indistinguishable from a laptop that is +// legitimately, temporarily offline (the case the whole queue-instead-of- +// crash contract exists to protect). This error represents a different, +// higher-level fact instead: the queue has now been failing to drain for so +// long that continuing to report a clean "queued" outcome would itself +// become the failure mode — a permanently misconfigured remote (wrong +// remoteUrl, a renamed repository path, a host that accepts a connection but +// cannot serve the repository) is ALSO classified RemoteUnavailableError and +// would otherwise queue forever, exit 0 every tick, indefinitely. Once the +// threshold is crossed this error must propagate and crash loud exactly +// like a non-network failure does, even though its underlying cause is +// still "the remote is unreachable" — that is the whole point of the +// escalation. +class RemoteQueueEscalationError extends CliError { + constructor(message: string, exitCode = 6) { + super(message, exitCode); + this.name = "RemoteQueueEscalationError"; + } +} + function isCliError(error: unknown): error is CliError { return error instanceof CliError; } @@ -44,6 +72,7 @@ function formatErrorMessage(error: unknown): string { module.exports = { CliError, RemoteUnavailableError, + RemoteQueueEscalationError, isCliError, formatErrorMessage }; diff --git a/packages/agent-memory-sync/src/memory-sync/push.ts b/packages/agent-memory-sync/src/memory-sync/push.ts index a854d16..a093bb8 100644 --- a/packages/agent-memory-sync/src/memory-sync/push.ts +++ b/packages/agent-memory-sync/src/memory-sync/push.ts @@ -3,11 +3,11 @@ const { filterOwnerScopedBaseMap, toRepositoryRelativePath } = require("./config"); -const { RemoteUnavailableError } = require("../errors"); +const { RemoteUnavailableError, RemoteQueueEscalationError } = require("../errors"); const { GitClient } = require("./git-client"); const { mergeText } = require("./merge"); const { checkRemoteReachable } = require("./reachability"); -const { StateStore } = require("./state-store"); +const { StateStore, DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS } = require("./state-store"); interface PushOptions { dryRun: boolean; @@ -35,6 +35,14 @@ interface PushConfig { gitBinary: string; reachabilityTimeoutMs?: number; reachabilityCheckCommand?: string[] | null; + // How long the queue may keep failing to drain (oldest queued snapshot's + // age — see StateStore.oldestQueuedSnapshotAgeMs) before a tick that would + // otherwise be a clean, silent "queued" outcome instead throws + // RemoteQueueEscalationError and crashes loud. Defaults to + // DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS (24h) — see that constant's + // comment in state-store.ts for the full rationale, including the real + // launchd/systemd tick interval it is sized against. + queueEscalationThresholdMs?: number; syncPaths: Array<{ source: string; destination?: string; @@ -138,7 +146,8 @@ async function performPush(config: PushConfig, options: PushOptions) { stateStore, currentLocalMap, currentBaseMap, - `remote unreachable (${reachability.reason}); stored the current local snapshot for replay on the next successful run` + `remote unreachable (${reachability.reason}); stored the current local snapshot for replay on the next successful run`, + config.queueEscalationThresholdMs ?? DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS ), ownerScopedWarnings ); @@ -218,7 +227,8 @@ async function performPush(config: PushConfig, options: PushOptions) { stateStore, currentLocalMap, currentBaseMap, - "remote unavailable; stored the current local snapshot for replay on the next successful run" + "remote unavailable; stored the current local snapshot for replay on the next successful run", + config.queueEscalationThresholdMs ?? DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS ), ownerScopedWarnings ); @@ -244,18 +254,27 @@ function appendNotes(result: T, extraNotes: stri // Shared by the reachability-precheck skip path and the catch-all fallback // below: stash the current local state as a new queued snapshot (existing // queued snapshots are left untouched — they are only cleared after a -// successful push) and report a clean "queued" result. +// successful push), then check whether the queue has now been failing to +// drain for longer than queueEscalationThresholdMs (see +// checkQueueEscalation below) before reporting a clean "queued" result — +// escalation takes priority: it throws instead of returning, so a caller +// that has crossed the threshold never sees a benign-looking "queued" +// outcome for that tick, even though the snapshot itself is safely persisted +// either way. function enqueueCurrentSnapshot( stateStore: InstanceType, currentLocalMap: Record, currentBaseMap: Record, - note: string + note: string, + queueEscalationThresholdMs: number ) { const queuedSnapshotId = stateStore.enqueueSnapshot({ localFiles: currentLocalMap, baseFiles: currentBaseMap }); + checkQueueEscalation(stateStore, queueEscalationThresholdMs); + return { kind: "push", status: "queued", @@ -269,6 +288,48 @@ function enqueueCurrentSnapshot( }; } +// Age-based escalation (see DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS in +// state-store.ts for the full "why age, not a counter" rationale and the +// real launchd/systemd tick interval the default is sized against). Runs +// after every enqueue; throws RemoteQueueEscalationError — crashing the +// current tick loud, same supervisor-restart surface as a non-network +// failure — once the OLDEST queued snapshot is older than the threshold, i.e. +// once the remote has been continuously unreachable for that long, not just +// unreachable on this one tick. Below the threshold this is a no-op, so a +// machine that is merely offline (a laptop closed overnight, a flight, a +// weekend) keeps queuing exactly as before this rework: silently, exit 0, +// every tick. +function checkQueueEscalation(stateStore: InstanceType, thresholdMs: number): void { + const oldestAgeMs = stateStore.oldestQueuedSnapshotAgeMs(); + if (oldestAgeMs === null || oldestAgeMs < thresholdMs) { + return; + } + + const queuedCount = stateStore.listQueuedSnapshots().length; + throw new RemoteQueueEscalationError( + `remote has been unreachable for ${formatDurationMs(oldestAgeMs)}, past the ` + + `${formatDurationMs(thresholdMs)} queue escalation threshold (${queuedCount} snapshot(s) queued in ` + + `${stateStore.queueDir()}). This usually means the remote is permanently misconfigured (wrong remoteUrl, ` + + `a renamed repository path, or a host that accepts a connection but cannot serve the repository) rather ` + + `than temporarily offline — check remoteUrl/branch/repositorySubdir. Every queued snapshot is still ` + + `safely stored and will be replayed automatically once the remote is reachable again; tune the ` + + `'queueEscalationThresholdMs' config key if this threshold does not fit this machine's expected offline ` + + `windows.` + ); +} + +function formatDurationMs(ms: number): string { + const totalSeconds = ms / 1000; + if (totalSeconds < 60) { + return `${totalSeconds.toFixed(1)}s`; + } + const totalMinutes = totalSeconds / 60; + if (totalMinutes < 60) { + return `${totalMinutes.toFixed(1)}m`; + } + return `${(totalMinutes / 60).toFixed(1)}h`; +} + function previewPush( config: { stateDir: string; profile: string; conflictStrategy: "inline-markers" | "local-wins" | "remote-wins"; repositorySubdir: string; remoteUrl: string; branch: string; gitBinary: string }, snapshots: Array<{ id: string; localFiles: Record; baseFiles: Record }> diff --git a/packages/agent-memory-sync/src/memory-sync/state-store.ts b/packages/agent-memory-sync/src/memory-sync/state-store.ts index 0011c1e..89dcc96 100644 --- a/packages/agent-memory-sync/src/memory-sync/state-store.ts +++ b/packages/agent-memory-sync/src/memory-sync/state-store.ts @@ -1,6 +1,23 @@ const { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, unlinkSync, writeFileSync } = require("node:fs"); const path = require("node:path"); +// Default escalation threshold for StateStore.oldestQueuedSnapshotAgeMs +// consumers (see push.ts's checkQueueEscalation): how long the queue may +// keep failing to drain before a tick that would otherwise be a clean, +// silent "queued" outcome instead crashes loud. 24h, derived from this +// package's own committed launchd/systemd periodic-sync tick interval — 900s +// (docs/launchd/com.agent-memory-sync.sync.plist.template's StartInterval) +// and the equivalent systemd OnUnitActiveSec=15min +// (docs/machine-setup.md section (c)) — 24h is 96 consecutive missed ticks +// at that cadence, comfortably longer than the "MacBook closed overnight" +// steady state both templates are written to tolerate silently, while still +// bounding how long a genuinely broken remote (wrong remoteUrl, a renamed +// repository path, a host that accepts a connection but cannot serve the +// repository) can hide before an operator is guaranteed to see it within a +// day. See push.ts's checkQueueEscalation and errors.ts's +// RemoteQueueEscalationError for how this value is consumed. +const DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS = 24 * 60 * 60 * 1000; + interface SyncState { version: number; profile: string; @@ -94,6 +111,50 @@ class StateStore { rmSync(path.join(this.queueDir(), id), { recursive: true, force: true }); } + // Age, in milliseconds, of the OLDEST currently-queued snapshot — derived + // entirely from each snapshot's manifest.json `createdAt` (already written + // by enqueueSnapshot above on every enqueue), so this needs no new + // persisted state. Returns null when the queue is empty (nothing to + // escalate) or when every manifest is missing/unparsable (defensive: a + // corrupt manifest must not be treated as "infinitely old" and force a + // spurious escalation). + // + // Why this reflects "how long has the remote been continuously + // unreachable" rather than just "how long has the oldest single snapshot + // sat here": a successful push clears every queued snapshot in one shot + // (see the `removeQueuedSnapshot` loop in performPush, + // src/memory-sync/push.ts, run only after `gitClient.push` succeeds) — so + // the oldest surviving snapshot's age is exactly the time since the FIRST + // tick that failed to reach the remote in the current unbroken failure + // streak; it resets to null the moment a push actually succeeds. See + // push.ts's checkQueueEscalation for how this is used. + oldestQueuedSnapshotAgeMs(referenceTime: number = Date.now()): number | null { + this.ensure(); + const createdTimestamps = readdirSync(this.queueDir(), { withFileTypes: true }) + .filter((entry: { isDirectory: () => boolean }) => entry.isDirectory()) + .map((entry: { name: string }) => { + const manifestPath = path.join(this.queueDir(), entry.name, "manifest.json"); + if (!existsSync(manifestPath)) { + return null; + } + let manifest: { createdAt?: string }; + try { + manifest = JSON.parse(readFileSync(manifestPath, "utf8")); + } catch { + return null; + } + const parsed = manifest.createdAt ? Date.parse(manifest.createdAt) : NaN; + return Number.isFinite(parsed) ? parsed : null; + }) + .filter((value: number | null): value is number => value !== null); + + if (createdTimestamps.length === 0) { + return null; + } + + return Math.max(0, referenceTime - Math.min(...createdTimestamps)); + } + clearTemp(): void { rmSync(this.tempDir(), { recursive: true, force: true }); mkdirSync(this.tempDir(), { recursive: true }); @@ -181,5 +242,6 @@ function walkFiles(rootDir: string): string[] { } module.exports = { - StateStore + StateStore, + DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS }; diff --git a/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts b/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts new file mode 100644 index 0000000..0a62cb2 --- /dev/null +++ b/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts @@ -0,0 +1,203 @@ +// Coverage for the queue escalation rework (agent-tasks 11424b5e), closing +// the mirror case 1b63070d left open: 1b63070d fixed a NON-network error +// being misclassified as "remote unavailable" (see +// tests/integration/watch-mirror-delete.test.ts's stub-git-fails-on-commit +// test). The mirror case is a remote that IS correctly classified +// RemoteUnavailableError (src/errors.ts) — but PERMANENTLY, not +// transiently: a wrong remoteUrl, a renamed repository path, a host that +// accepts an SSH/TCP connection but cannot serve git-upload-pack. Before +// this rework that queued cleanly, exit 0, every tick, forever — completely +// indistinguishable from a laptop that is legitimately, temporarily +// offline. +// +// The fix (src/memory-sync/push.ts's checkQueueEscalation, backed by +// StateStore.oldestQueuedSnapshotAgeMs in src/memory-sync/state-store.ts): +// age-based, not counter-based — the OLDEST queued snapshot's already- +// persisted manifest.json `createdAt` IS "how long has the remote been +// continuously unreachable" (a successful push clears the whole queue in +// one shot), so no new state is needed. Once that age crosses +// queueEscalationThresholdMs (default 24h — see +// DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS's comment in state-store.ts for the +// full rationale, including the real launchd/systemd tick interval it is +// sized against), the tick throws RemoteQueueEscalationError instead of +// returning a benign "queued" result — non-zero exit, a clear stderr +// message — while leaving every snapshot safely queued for replay. +// +// These tests never wait in real time: they backdate an already-queued +// snapshot's manifest.json directly (the same file StateStore.enqueueSnapshot +// writes), so the REAL production default threshold (24h) is exercised +// deterministically and fast, rather than a test-only tiny override. +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { readFileSync, readdirSync, writeFileSync } = require("node:fs"); +const path = require("node:path"); +const { + createSandbox, + fileExists, + runCli, + writeProjectConfig, + writeText +} = require("../helpers/cli.ts"); +const { runWatchTick } = require("../helpers/watch-process.ts"); + +function createConfig(workspaceRoot: string, remoteDir: string) { + return { + rootDir: workspaceRoot, + remoteUrl: remoteDir, + branch: "main", + repositorySubdir: "shared", + stateDir: ".agent-memory-sync/default", + // Local-path remotes resolve reachability via a plain fs existsSync + // check (see src/memory-sync/reachability.ts's classifyRemote), so this + // has no real timing effect here — set small anyway, matching this + // suite's existing convention (see memory-sync.test.ts). + reachabilityTimeoutMs: 500, + syncPaths: [{ source: "MEMORY.md", destination: "MEMORY.md", kind: "file" }] + }; +} + +function queueDirFor(workspaceRoot: string): string { + return path.join(workspaceRoot, ".agent-memory-sync", "default", "queue"); +} + +// Rewrites the manifest.json of every currently-queued snapshot to claim it +// was created `ageMs` before now — the same field +// StateStore.enqueueSnapshot (src/memory-sync/state-store.ts) writes on +// every real enqueue, so this is indistinguishable to +// oldestQueuedSnapshotAgeMs from a snapshot that has genuinely been sitting +// there that long. +function backdateQueuedSnapshots(workspaceRoot: string, ageMs: number): void { + const queueDir = queueDirFor(workspaceRoot); + const createdAt = new Date(Date.now() - ageMs).toISOString(); + for (const id of readdirSync(queueDir)) { + const manifestPath = path.join(queueDir, id, "manifest.json"); + const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); + writeFileSync(manifestPath, `${JSON.stringify({ ...manifest, createdAt }, null, 2)}\n`, "utf8"); + } +} + +// ─── AC2: below the threshold stays silent, exit 0 ────────────────────────── + +test("push stays a silent, exit-0 queue while the oldest queued snapshot is well under the escalation threshold", () => { + const root = createSandbox("escalation-below-threshold"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + // First tick: nothing queued yet, so the oldest age is effectively 0ms — + // nowhere near the 24h default threshold. + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const firstPayload = JSON.parse(firstRun.stdout); + assert.equal(firstRun.status, 0); + assert.equal(firstPayload.runs[0].status, "queued"); + + // Second tick, still offline: the queue now holds a real snapshot from + // moments ago — still nowhere near 24h old. + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const secondPayload = JSON.parse(secondRun.stdout); + assert.equal(secondRun.status, 0, `expected a clean exit 0 below threshold. stderr: ${secondRun.stderr}`); + assert.equal(secondPayload.runs[0].status, "queued"); + assert.equal(readdirSync(queueDirFor(workspaceRoot)).length, 2); +}); + +// ─── AC1: above the threshold becomes visible (non-zero exit) ─────────────── + +test("push crashes loud with a clear message once the oldest queued snapshot is older than the escalation threshold", () => { + const root = createSandbox("escalation-above-threshold"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + // Backdate the just-queued snapshot to 25h old — past the real 24h + // production default (DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS in + // state-store.ts), without waiting a single real second. + backdateQueuedSnapshots(workspaceRoot, 25 * 60 * 60 * 1000); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { expectFailure: true } + ); + + assert.equal(secondRun.status, 6, `expected the escalation's own exit code. stderr: ${secondRun.stderr}`); + assert.match(secondRun.stderr, /remote has been unreachable for/); + assert.match(secondRun.stderr, /permanently misconfigured/); + assert.match(secondRun.stderr, /remoteUrl\/branch\/repositorySubdir/); + + // The escalating tick's own snapshot must still be safely persisted, not + // dropped — enqueueSnapshot runs before the escalation check throws. + const queuedEntries = readdirSync(queueDirFor(workspaceRoot)); + assert.equal(queuedEntries.length, 2, `expected both snapshots to remain queued, found: ${queuedEntries.join(", ")}`); +}); + +// Same scenario as above, but through `watch` (not `run --mode push`) — +// pins that watch's escalation surface actually becomes a non-zero exit +// too. watch.ts's handleSnapshotError writes the thrown error's message to +// stderr and sets the exit code unconditionally (not gated by --quiet or +// --verbose, unlike the ordinary "queued locally"/"watching N path(s)" +// info lines) — --verbose is passed below only so this test can use the +// same robust chokidar-ready detection every other watch-spawning test in +// this suite uses (see tests/helpers/watch-process.ts), not because the +// escalation message itself needs it to be visible. +test("watch tick exits non-zero once the queue has been failing to drain past the escalation threshold", async () => { + const root = createSandbox("escalation-watch"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + const seedRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(seedRun.stdout).runs[0].status, "queued"); + backdateQueuedSnapshots(workspaceRoot, 25 * 60 * 60 * 1000); + + const { exitCode, stderr } = await runWatchTick(configPath, () => { + writeText(path.join(workspaceRoot, "MEMORY.md"), "trigger\n"); + }); + + assert.equal(exitCode, 6, `expected the escalation's own exit code. stderr: ${stderr}`); + assert.match(stderr, /remote has been unreachable for/); + assert.match(stderr, /permanently misconfigured/); +}); + +// ─── AC4: the precheck-bypass path still queues via lookupRemoteHead ──────── +// +// Reviewer-named positive test: a remote whose scheme the reachability +// precheck waves through as "unsupported" (see classifyRemote in +// src/memory-sync/reachability.ts — https/git have no dedicated fast probe, +// so checkRemoteReachable assumes reachable and lets the real git operation +// surface any failure) must still be caught by GitClient.lookupRemoteHead's +// own RemoteUnavailableError (src/memory-sync/git-client.ts) and QUEUE +// cleanly — not crash — exactly like the precheck's own unreachable path +// does. git://127.0.0.1:1/... is hermetic (loopback only, no real network +// access, no DNS) and fails fast with a connection refusal, since nothing +// listens on port 1. +test("push still queues cleanly when an unsupported-scheme remote (precheck assumes reachable) actually fails at lookupRemoteHead", () => { + const root = createSandbox("escalation-precheck-bypass"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + const bypassRemoteUrl = "git://127.0.0.1:1/definitely-not-a-real-remote.git"; + + writeText(path.join(workspaceRoot, "MEMORY.md"), "content\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, bypassRemoteUrl)); + + const result = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const payload = JSON.parse(result.stdout); + + assert.equal(result.status, 0, `expected a clean exit 0, not a crash. stderr: ${result.stderr}`); + assert.equal(payload.runs[0].status, "queued"); + assert.ok(payload.runs[0].queuedSnapshotId); + assert.equal(fileExists(queueDirFor(workspaceRoot)), true); + assert.equal(readdirSync(queueDirFor(workspaceRoot)).length, 1); +}); diff --git a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts index 5f368c4..b9e1986 100644 --- a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts +++ b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts @@ -398,6 +398,90 @@ test("watch tick with a non-network git failure inside the push (e.g. commit fai ); }); +// Sibling to writeStubGitFailingOnCommit above, but for `fetch` — pinning +// the SECOND finding from the 1b63070d review (agent-tasks 11424b5e): +// GitClient.prepareWorkingCopy (src/memory-sync/git-client.ts) runs +// `git fetch origin ` via plain run() WITHOUT allowFailure, so a +// fetch failure throws a generic CliError (exit code 4 — the same numeric +// exit code RemoteUnavailableError also uses; only the throw site/class +// discriminates, see errors.ts) and crashes loud instead of being queued. +// That is a DELIBERATE decision — a fetch failure can be purely local (a +// full disk writing the incoming packfile, a corrupted object database, ...) +// and is not safe to assume is "the remote's fault" the way +// lookupRemoteHead/push's own RemoteUnavailableError throws are — and +// README.md/docs/machine-setup.md describe it, but nothing pinned it with a +// test: a future refactor (e.g. reflexively adding `allowFailure: true` to +// every `this.run(...)` call, or wrapping this specific fetch in a +// try/catch that reclassifies it) could flip this boundary either way +// unnoticed. +function writeStubGitFailingOnFetch(root: string): string { + const stubPath = path.join(root, "stub-git-fails-on-fetch.sh"); + writeText( + stubPath, + [ + "#!/bin/sh", + 'if [ "$1" = "fetch" ]; then', + ' echo "stub-git: forced fetch failure (simulated non-network error, e.g. full disk writing the packfile)" >&2', + " exit 17", + "fi", + 'exec git "$@"', + "" + ].join("\n") + ); + fs.chmodSync(stubPath, 0o755); + return stubPath; +} + +// THE boundary test AC3 (agent-tasks 11424b5e) depends on. Mirrors the +// commit-failure test above almost exactly, with one deliberate difference: +// the remote MUST already be seeded with a real commit before the stub is +// installed, so `lookupRemoteHead`'s `git ls-remote` (never intercepted by +// the stub — it only forces `fetch` to fail) returns a real head and +// prepareWorkingCopy takes its `if (remoteHead)` branch — the only branch +// that calls `git fetch` at all (the `else` branch, an empty remote, calls +// `checkout --orphan` instead and never fetches). Mutation check for this +// test (run manually, not part of the automated suite — see this task's +// final report): temporarily wrap the `this.run(["fetch", ...], repoDir)` +// call in git-client.ts's prepareWorkingCopy with the same +// `allowFailure: true` + RemoteUnavailableError classification +// lookupRemoteHead/push use, confirm this test goes red (exit 0/"queued" +// instead of exit 4/crash), then restore the original code. +test("watch tick with a non-network git failure on fetch (e.g. a full disk) still exits non-zero, not queued", async () => { + const root = createSandbox("watch-non-network-fetch-failure"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + // Seed the remote with the real git binary first, so lookupRemoteHead's + // `git ls-remote` (below, with the stub installed) returns a real head + // and prepareWorkingCopy takes the branch that actually calls `git fetch`. + writeProjectConfig(configPath, createConfig(workspaceRoot, remoteDir)); + runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + + const stubGitBinary = writeStubGitFailingOnFetch(root); + writeProjectConfig(configPath, { + ...createConfig(workspaceRoot, remoteDir), + gitBinary: stubGitBinary + }); + + const { exitCode, stderr } = await runWatchTick(configPath, () => { + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\nlocal edit\n"); + }); + + assert.equal( + exitCode, + 4, + `watch was expected to crash loudly (exit 4) on a non-network git fetch failure; it exited ${exitCode} ` + + `instead. stderr: ${stderr}` + ); + assert.doesNotMatch( + stderr, + /queued locally/, + "a non-network fetch failure must not be reported as a benign queued-instead-of-pushed tick" + ); +}); + // Rework finding (LOW, review of this task): pins that a successful tick // updates stateDir/base to the post-merge remote state — the input the next // tick's 3-way merge relies on to correctly leave an unpulled peer file diff --git a/packages/agent-memory-sync/tests/unit/state-store.test.ts b/packages/agent-memory-sync/tests/unit/state-store.test.ts new file mode 100644 index 0000000..80ca80a --- /dev/null +++ b/packages/agent-memory-sync/tests/unit/state-store.test.ts @@ -0,0 +1,86 @@ +// Unit coverage for StateStore.oldestQueuedSnapshotAgeMs — the mechanism +// push.ts's checkQueueEscalation (src/memory-sync/push.ts) uses to detect a +// permanently unreachable/misconfigured remote (agent-tasks 11424b5e, +// reviewer findings from 1b63070d): the OLDEST queued snapshot's +// manifest.json `createdAt` (already written by enqueueSnapshot on every +// enqueue) is read back and compared against a threshold, with no new +// persisted state. See tests/integration/remote-unreachable-escalation.test.ts +// for the end-to-end behavior through `run`/`watch`. + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { mkdirSync, readdirSync, readFileSync, writeFileSync } = require("node:fs"); +const { tmpdir } = require("node:os"); +const path = require("node:path"); +const { StateStore } = require("../../src/memory-sync/state-store"); + +function sandbox(name: string): string { + const root = path.join( + tmpdir(), + `agent-memory-sync-state-store-${name}-${Date.now()}-${Math.random().toString(16).slice(2, 8)}` + ); + mkdirSync(root, { recursive: true }); + return root; +} + +test("oldestQueuedSnapshotAgeMs: returns null when the queue is empty", () => { + const store = new StateStore(sandbox("empty"), "default"); + assert.equal(store.oldestQueuedSnapshotAgeMs(), null); +}); + +test("oldestQueuedSnapshotAgeMs: computes age from a single queued snapshot's manifest.json createdAt", () => { + const store = new StateStore(sandbox("single"), "default"); + const createdAt = new Date("2026-01-01T00:00:00.000Z"); + store.enqueueSnapshot({ localFiles: { "a.md": "one" }, baseFiles: {} }); + + // Backdate the manifest this test just wrote, exactly like the + // integration test does — no real waiting required. + const [id] = readdirSync(store.queueDir()); + const manifestPath = path.join(store.queueDir(), id, "manifest.json"); + const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); + writeFileSync(manifestPath, JSON.stringify({ ...manifest, createdAt: createdAt.toISOString() }, null, 2)); + + const referenceTime = createdAt.getTime() + 5000; + assert.equal(store.oldestQueuedSnapshotAgeMs(referenceTime), 5000); +}); + +test("oldestQueuedSnapshotAgeMs: reports the OLDEST of several queued snapshots, not the newest", () => { + const store = new StateStore(sandbox("multiple"), "default"); + const older = new Date("2026-01-01T00:00:00.000Z"); + const newer = new Date("2026-01-02T00:00:00.000Z"); + + store.enqueueSnapshot({ localFiles: { "a.md": "one" }, baseFiles: {} }); + store.enqueueSnapshot({ localFiles: { "b.md": "two" }, baseFiles: {} }); + + const ids = readdirSync(store.queueDir()).sort(); + const timestamps = [older, newer]; + ids.forEach((id: string, index: number) => { + const manifestPath = path.join(store.queueDir(), id, "manifest.json"); + const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); + writeFileSync( + manifestPath, + JSON.stringify({ ...manifest, createdAt: timestamps[index].toISOString() }, null, 2) + ); + }); + + const referenceTime = newer.getTime(); + assert.equal(store.oldestQueuedSnapshotAgeMs(referenceTime), newer.getTime() - older.getTime()); +}); + +test("oldestQueuedSnapshotAgeMs: a queued directory with a missing/corrupt manifest.json is ignored, not treated as infinitely old", () => { + const store = new StateStore(sandbox("corrupt"), "default"); + store.ensure(); + mkdirSync(path.join(store.queueDir(), "not-a-real-snapshot"), { recursive: true }); + // No manifest.json written at all inside it. + + assert.equal(store.oldestQueuedSnapshotAgeMs(), null); +}); + +test("oldestQueuedSnapshotAgeMs: clears back to null after removeQueuedSnapshot empties the queue", () => { + const store = new StateStore(sandbox("cleared"), "default"); + const id = store.enqueueSnapshot({ localFiles: { "a.md": "one" }, baseFiles: {} }); + assert.notEqual(store.oldestQueuedSnapshotAgeMs(), null); + + store.removeQueuedSnapshot(id); + assert.equal(store.oldestQueuedSnapshotAgeMs(), null); +}); From 70e49bb93b2b40eb970c78d83326612a6fb814fb Mon Sep 17 00:00:00 2001 From: Lan Nguyen Si Date: Wed, 5 Aug 2026 20:54:04 +0200 Subject: [PATCH 2/2] fix(memory-sync): harden escalation per review round Canonical exit-code table gains code 6. Config wiring is now pinned by tests that only pass if the override actually reaches checkQueueEscalation (2h backdate: under the 24h default, over the 1h override), plus an invalid-env exit-3 case. Clock-skew guard: an implausible age (>30x threshold) skips escalation with a diagnostic note instead of turning a healthy machine crash-loud after an NTP correction; wall-clock dependency documented. Scheduled runs catch the escalation, continue remaining ticks, report full JSON, and exit 6 at the end (single runs unchanged). Null disables escalation end-to-end, mirroring reachabilityCheckCommand's convention. README states the systemd burst arithmetic honestly with reset-failed recovery and a raised StartLimit pairing; machine-setup's no-op claim carves out the post-threshold case. Escalation message trimmed to the actionable core, queue count via direct dir listing, corrupt-manifest one-way bias documented and pinned (surviving older manifest still escalates), and the git-failure catch path has its own escalation test. Task: 11424b5e --- packages/agent-memory-sync/README.md | 22 +- .../agent-memory-sync/docs/architecture.md | 1 + .../agent-memory-sync/docs/machine-setup.md | 15 +- .../agent-memory-sync/src/commands/run.ts | 51 +++- .../agent-memory-sync/src/config/loader.ts | 37 ++- .../agent-memory-sync/src/memory-sync/push.ts | 97 +++++-- .../src/memory-sync/state-store.ts | 30 +++ .../remote-unreachable-escalation.test.ts | 240 ++++++++++++++++++ .../run-schedule-escalation.test.ts | 121 +++++++++ .../tests/unit/state-store.test.ts | 28 ++ 10 files changed, 604 insertions(+), 38 deletions(-) create mode 100644 packages/agent-memory-sync/tests/integration/run-schedule-escalation.test.ts diff --git a/packages/agent-memory-sync/README.md b/packages/agent-memory-sync/README.md index b10ab89..43644d9 100644 --- a/packages/agent-memory-sync/README.md +++ b/packages/agent-memory-sync/README.md @@ -131,8 +131,8 @@ Environment=AGENT_MEMORY_SYNC_BRANCH=main ExecStart=/usr/local/bin/agent-memory-sync watch --verbose Restart=on-failure RestartSec=5s -StartLimitIntervalSec=300 -StartLimitBurst=10 +StartLimitIntervalSec=1800 +StartLimitBurst=30 [Install] WantedBy=multi-user.target @@ -140,6 +140,15 @@ WantedBy=multi-user.target The `StartLimitIntervalSec` / `StartLimitBurst` pair caps systemd's restart loop for the failures that still exit non-zero — a config/data error raised before the remote working copy is prepared (e.g. a required `syncPaths` entry missing), or any other git-level failure while preparing/committing that working copy (a full disk, a corrupted git config, a broken commit hook, ...) — so a persistently broken cause does not crashloop forever; a remote that is merely unreachable or rejecting the push (see below) no longer exits at all, so it never spends this budget. The one exception is [queue escalation](#queue-escalation-a-permanently-broken-remote-does-not-queue-forever): once the queue has been failing to drain past `queueEscalationThresholdMs` (default 24h), a tick DOES exit non-zero again — but only on a real local edit (`watch` is edge-triggered), so it does not spend this budget any faster than this machine's memory actually changes while the remote stays broken. Inspect `journalctl -u agent-memory-sync-watch.service` for the `snapshot push failed: ...` line `watch` writes to stderr before exiting on one of those failures. +Honest arithmetic, measured: one crash-restart cycle (a failed start plus `RestartSec`) is ~11s. Under the original `StartLimitBurst=10` / `StartLimitIntervalSec=300` pairing shown in earlier revisions of this doc, 10 crashes exhausted the budget in ~110s — well inside a single ordinary "edit the config, restart, still broken, edit again" debugging session. Once the burst is exhausted, systemd does not just pause the restart loop, it marks the unit `failed` and **stops trying entirely**, even after the underlying cause is fixed, until the failure counter is explicitly cleared: + +```bash +systemctl reset-failed agent-memory-sync-watch.service +systemctl restart agent-memory-sync-watch.service # reset-failed only clears the counter, it does not start the unit +``` + +The sample unit above raises the pairing to `StartLimitIntervalSec=1800` / `StartLimitBurst=30` (~30 crashes × ~11s ≈ 330s, under 6 minutes of continuous crash-looping) so ordinary iterative config editing has realistic headroom before landing in `failed`, while a genuinely broken cause still gets capped well short of looping forever. macOS's `ThrottleInterval` (see `docs/launchd/com.agent-memory-sync.watch.plist.template`) is not a direct analogue: it only enforces a minimum gap between respawns and has no burst counter or give-up state at all, so a broken `watch` LaunchAgent keeps retrying indefinitely instead of ever reaching a terminal `failed` state that needs a manual reset. + macOS equivalent (LaunchAgent instead of systemd): see [`docs/launchd/com.agent-memory-sync.watch.plist.template`](docs/launchd/com.agent-memory-sync.watch.plist.template) and [docs/machine-setup.md](docs/machine-setup.md). @@ -347,7 +356,14 @@ Priority order (highest to lowest): CLI flags > environment variables > config f host that accepts a connection but cannot serve the repository) does not queue silently forever. Below the threshold nothing changes: silent, exit `0`, every tick — a merely offline machine is unaffected. See [watch's "Queue escalation" section](#queue-escalation-a-permanently-broken-remote-does-not-queue-forever) - for the full rationale + for the full rationale. Set `queueEscalationThresholdMs` to `null` (config file, or + `config set queueEscalationThresholdMs null`) to disable this check entirely — mirrors + `reachabilityCheckCommand`'s null-is-a-real-value convention above. The queue then keeps + queuing silently, exit `0`, forever, regardless of age; a computed age past a 30x-threshold + sanity ceiling is also never escalated even with a finite threshold configured, since an age + that implausible more likely reflects this machine's clock having been wrong when the + snapshot was queued than a genuinely stuck remote — a diagnostic note is emitted on that + otherwise-silent "queued" outcome instead - append-only concurrent edits are merged automatically; other conflicts default to inline conflict markers - `--dry-run` previews the result without changing local files or the remote repository diff --git a/packages/agent-memory-sync/docs/architecture.md b/packages/agent-memory-sync/docs/architecture.md index 66dabca..63acfa8 100644 --- a/packages/agent-memory-sync/docs/architecture.md +++ b/packages/agent-memory-sync/docs/architecture.md @@ -114,6 +114,7 @@ Commands signal failure by raising/returning an error - they never call `os.exit | `3` | Configuration error (bad config file, missing required setting) | | `4` | Runtime error (external service unavailable, permission denied) | | `5` | Not found (resource the command expected does not exist) | +| `6` | Queue escalation (the local queue has not drained within queueEscalationThresholdMs) | Error messages follow the pattern: `error: . .` diff --git a/packages/agent-memory-sync/docs/machine-setup.md b/packages/agent-memory-sync/docs/machine-setup.md index 9a97e19..6488b8c 100644 --- a/packages/agent-memory-sync/docs/machine-setup.md +++ b/packages/agent-memory-sync/docs/machine-setup.md @@ -312,8 +312,19 @@ empty/stale local workspace as if it were authoritative. `Persistent=true` catches up a missed tick (e.g. the machine was off) shortly after boot instead of waiting a full interval. A tick that fires while the mini is unreachable is a fast, clean no-op — same reachability - precheck `run` always uses — so a short 15-minute interval is safe; it - will not pile up overlapping ssh attempts or spam logs. Install with: + precheck `run` always uses — so a short 15-minute interval is safe below + the queue escalation threshold (see the "Queue escalation" bullet above): + it will not pile up overlapping ssh attempts or spam logs. That stops + being true once the OLDEST queued snapshot crosses + `queueEscalationThresholdMs` (default 24h, ~96 missed ticks at this + interval) — from then on, each 15-minute tick exits non-zero (`6`) with a + clear stderr message instead of a silent no-op. Because this is a + `Type=oneshot` service fired by a `.timer`, not a `Restart=on-failure` + daemon, the timer keeps firing it again every 15 minutes regardless of + that failure, so expect one failure line in `journalctl -u + agent-memory-sync-sync.service` per tick until the remote is fixed — that + repeated visibility is the intended outcome of escalation, not log spam + to suppress. Install with: `systemctl daemon-reload && systemctl enable --now agent-memory-sync-sync.timer`. ## d) Restore / rollback diff --git a/packages/agent-memory-sync/src/commands/run.ts b/packages/agent-memory-sync/src/commands/run.ts index 61ee957..31c5dc2 100644 --- a/packages/agent-memory-sync/src/commands/run.ts +++ b/packages/agent-memory-sync/src/commands/run.ts @@ -3,7 +3,7 @@ const { requireRemoteUrl, resolveRunConfig } = require("../config/loader"); -const { CliError, isCliError } = require("../errors"); +const { CliError, RemoteQueueEscalationError, formatErrorMessage, isCliError } = require("../errors"); const { performPull } = require("../memory-sync/pull"); const { performPush } = require("../memory-sync/push"); const { summarizeOperation } = require("../memory-sync/preview"); @@ -96,13 +96,56 @@ function registerRunCommand(program: import("commander").Command): void { const runs: Array> = []; let remainingRuns = maxRuns || (runConfig.schedule ? Number.POSITIVE_INFINITY : 1); + // Set once a scheduled tick escalates (RemoteQueueEscalationError) so + // the whole invocation still exits 6 after the loop below finishes — + // see the try/catch inside the loop for why a scheduled run does not + // stop ticking the moment that happens. + let queueEscalationError: unknown = null; while (remainingRuns > 0) { if (options.dryRun) { writeDryRun(`executing ${runConfig.mode} for profile '${runConfig.profile}'`, outputOptions); } - const execution = await executeMode(runConfig, { dryRun: options.dryRun }, outputOptions); + let execution: Record; + try { + execution = await executeMode(runConfig, { dryRun: options.dryRun }, outputOptions); + } catch (error) { + // Single run (no --schedule): preserve the pre-fix behavior + // exactly — RemoteQueueEscalationError (and everything else) + // propagates immediately, uncaught, straight out of this action + // handler to main.ts's top-level catch, with writeResult() below + // never reached. Immediate exit 6, same as before this fix. + // + // Scheduled run: an escalating tick must not kill the scheduler + // outright. run --schedule IS its own supervisor/replay loop — a + // tick that escalated failed to drain the queue on THIS tick, but + // the queue is still safely persisted and a later tick, once the + // remote recovers, is exactly what replays it. Dying on the first + // escalation stranded every remaining tick from ever getting that + // chance (measured: exit 6 after tick 1 of 3, zero stdout, no + // further ticks ran at all). So: record it, keep ticking, and + // still exit 6 once the loop ends — just after every remaining + // tick had its shot, and after writeResult() below has run so a + // --output json consumer still sees every tick that did complete. + if (!runConfig.schedule || !(error instanceof RemoteQueueEscalationError)) { + throw error; + } + + queueEscalationError = error; + execution = { + kind: runConfig.mode, + status: "escalated", + remoteHeadBefore: null, + remoteHeadAfter: null, + appliedFiles: [], + mergedFiles: [], + conflictFiles: [], + queuedSnapshotId: null, + notes: [formatErrorMessage(error)] + }; + } + runs.push(execution); remainingRuns -= 1; @@ -128,6 +171,10 @@ function registerRunCommand(program: import("commander").Command): void { }; writeResult(payload, runConfig.outputFormat, () => runs.map(summarizeOperation).join("\n")); + + if (queueEscalationError) { + throw queueEscalationError; + } }); } diff --git a/packages/agent-memory-sync/src/config/loader.ts b/packages/agent-memory-sync/src/config/loader.ts index 477ce13..fdecf50 100644 --- a/packages/agent-memory-sync/src/config/loader.ts +++ b/packages/agent-memory-sync/src/config/loader.ts @@ -37,7 +37,12 @@ interface UserConfig { gitBinary?: string; reachabilityTimeoutMs?: number; reachabilityCheckCommand?: string[] | null; - queueEscalationThresholdMs?: number; + // `undefined` (key absent) falls through the DEFAULTS/env/config-file merge + // chain to the default threshold; explicit `null` is a distinct, real + // value meaning "escalation disabled" — mirrors reachabilityCheckCommand's + // null-is-a-real-value convention. See validateQueueEscalationThresholdMs + // and push.ts's resolveQueueEscalationThresholdMs. + queueEscalationThresholdMs?: number | null; } interface LoadedConfig { @@ -63,7 +68,7 @@ interface RunConfig extends UserConfig { mode: RunMode; reachabilityTimeoutMs: number; reachabilityCheckCommand: string[] | null; - queueEscalationThresholdMs: number; + queueEscalationThresholdMs: number | null; } interface RunConfigOverrides { @@ -84,7 +89,7 @@ interface RunConfigOverrides { mode?: RunMode; reachabilityTimeoutMs?: number; reachabilityCheckCommand?: string[] | null; - queueEscalationThresholdMs?: number; + queueEscalationThresholdMs?: number | null; } const DEFAULT_SYNC_PATHS: SyncPathConfig[] = [ @@ -172,11 +177,7 @@ function resolveRunConfig(loaded: LoadedConfig, overrides: RunConfigOverrides = DEFAULT_REACHABILITY_TIMEOUT_MS ), reachabilityCheckCommand: normalizeReachabilityCheckCommand(merged.reachabilityCheckCommand), - queueEscalationThresholdMs: validatePositiveInteger( - merged.queueEscalationThresholdMs, - "queueEscalationThresholdMs", - DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS - ) + queueEscalationThresholdMs: validateQueueEscalationThresholdMs(merged.queueEscalationThresholdMs) }; } @@ -507,6 +508,18 @@ function validatePositiveInteger(value: number | undefined, key: string, fallbac return value; } +// Explicit `null` is a real, distinct value here (see UserConfig.queueEscalation +// ThresholdMs's comment) meaning "escalation disabled" — passed straight +// through, not defaulted. Anything else still goes through the same +// positive-integer validation `reachabilityTimeoutMs` uses. +function validateQueueEscalationThresholdMs(value: number | null | undefined): number | null { + if (value === null) { + return null; + } + + return validatePositiveInteger(value, "queueEscalationThresholdMs", DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS); +} + function normalizeReachabilityCheckCommand(value?: string[] | null): string[] | null { if (!value) { return null; @@ -548,11 +561,9 @@ function parseConfigValue(key: string, value: string): unknown { case "reachabilityTimeoutMs": return validatePositiveInteger(Number(value), "reachabilityTimeoutMs", DEFAULT_REACHABILITY_TIMEOUT_MS); case "queueEscalationThresholdMs": - return validatePositiveInteger( - Number(value), - "queueEscalationThresholdMs", - DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS - ); + return value === "null" + ? null + : validatePositiveInteger(Number(value), "queueEscalationThresholdMs", DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS); case "reachabilityCheckCommand": return value === "null" ? null diff --git a/packages/agent-memory-sync/src/memory-sync/push.ts b/packages/agent-memory-sync/src/memory-sync/push.ts index a093bb8..2ed6138 100644 --- a/packages/agent-memory-sync/src/memory-sync/push.ts +++ b/packages/agent-memory-sync/src/memory-sync/push.ts @@ -1,3 +1,4 @@ +const { readdirSync } = require("node:fs"); const { collectLocalSyncFiles, filterOwnerScopedBaseMap, @@ -41,8 +42,12 @@ interface PushConfig { // RemoteQueueEscalationError and crashes loud. Defaults to // DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS (24h) — see that constant's // comment in state-store.ts for the full rationale, including the real - // launchd/systemd tick interval it is sized against. - queueEscalationThresholdMs?: number; + // launchd/systemd tick interval it is sized against. Explicit `null` + // (as opposed to `undefined`, which falls through to the default above) + // disables the escalation check entirely — mirrors + // reachabilityCheckCommand's null-is-a-real-value convention: the queue + // then keeps queuing silently, exit 0, forever, regardless of age. + queueEscalationThresholdMs?: number | null; syncPaths: Array<{ source: string; destination?: string; @@ -147,7 +152,7 @@ async function performPush(config: PushConfig, options: PushOptions) { currentLocalMap, currentBaseMap, `remote unreachable (${reachability.reason}); stored the current local snapshot for replay on the next successful run`, - config.queueEscalationThresholdMs ?? DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS + resolveQueueEscalationThresholdMs(config.queueEscalationThresholdMs) ), ownerScopedWarnings ); @@ -228,13 +233,26 @@ async function performPush(config: PushConfig, options: PushOptions) { currentLocalMap, currentBaseMap, "remote unavailable; stored the current local snapshot for replay on the next successful run", - config.queueEscalationThresholdMs ?? DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS + resolveQueueEscalationThresholdMs(config.queueEscalationThresholdMs) ), ownerScopedWarnings ); } } +// `undefined` (the field was never set at all — e.g. a PushConfig built +// outside the config loader) falls back to the default threshold, same as +// before this fix. Explicit `null` is a distinct, real value meaning +// "escalation disabled" — see the PushConfig.queueEscalationThresholdMs +// comment above and loader.ts's queueEscalationThresholdMs validation for +// where that convention is enforced end-to-end. +function resolveQueueEscalationThresholdMs(value: number | null | undefined): number | null { + if (value === null) { + return null; + } + return value ?? DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS; +} + // Fix-Runde HIGH finding (05-review-findings.md, agent-tasks 06d09cde): // merges collectLocalSyncFiles' ownerScoped "own file missing among peer // files" warnings (see config.ts's CollectLocalSyncFilesResult.warnings) @@ -260,20 +278,23 @@ function appendNotes(result: T, extraNotes: stri // escalation takes priority: it throws instead of returning, so a caller // that has crossed the threshold never sees a benign-looking "queued" // outcome for that tick, even though the snapshot itself is safely persisted -// either way. +// either way. A non-null diagnostic note from checkQueueEscalation (the +// clock-skew sanity-ceiling guard fired instead of escalating) is folded +// into the returned "queued" result's own notes, so it is still visible on +// this otherwise-silent, exit-0 path. function enqueueCurrentSnapshot( stateStore: InstanceType, currentLocalMap: Record, currentBaseMap: Record, note: string, - queueEscalationThresholdMs: number + queueEscalationThresholdMs: number | null ) { const queuedSnapshotId = stateStore.enqueueSnapshot({ localFiles: currentLocalMap, baseFiles: currentBaseMap }); - checkQueueEscalation(stateStore, queueEscalationThresholdMs); + const skewNote = checkQueueEscalation(stateStore, queueEscalationThresholdMs); return { kind: "push", @@ -284,10 +305,25 @@ function enqueueCurrentSnapshot( mergedFiles: [], conflictFiles: [], queuedSnapshotId, - notes: [note] + notes: skewNote ? [note, skewNote] : [note] }; } +// 30x the effective threshold: a sanity ceiling guarding against clock skew. +// A queued manifest's `createdAt` (StateStore.enqueueSnapshot) is a +// wall-clock timestamp, so oldestQueuedSnapshotAgeMs is only ever as +// trustworthy as this machine's clock was AT ENQUEUE TIME (see +// DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS's comment in state-store.ts). A +// machine that enqueued under a wrong-in-the-past system clock (dead RTC +// battery, a container that started before NTP synced, ...) would otherwise +// compute an implausibly large age — and escalate — the moment NTP corrects +// the clock forward, which is exactly backwards: that machine's queue may +// not have been stuck at all. An age past this ceiling is far more likely a +// clock artifact than 30x the configured "how long is too long" threshold of +// genuine remote unavailability, so checkQueueEscalation below skips +// escalating and emits a diagnostic note instead of crashing loud. +const QUEUE_ESCALATION_SANITY_CEILING_MULTIPLE = 30; + // Age-based escalation (see DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS in // state-store.ts for the full "why age, not a counter" rationale and the // real launchd/systemd tick interval the default is sized against). Runs @@ -298,23 +334,48 @@ function enqueueCurrentSnapshot( // unreachable on this one tick. Below the threshold this is a no-op, so a // machine that is merely offline (a laptop closed overnight, a flight, a // weekend) keeps queuing exactly as before this rework: silently, exit 0, -// every tick. -function checkQueueEscalation(stateStore: InstanceType, thresholdMs: number): void { +// every tick. `thresholdMs === null` means escalation is disabled outright +// (see PushConfig.queueEscalationThresholdMs) — also a no-op. Returns a +// diagnostic note (string) instead of throwing when the computed age is past +// the clock-skew sanity ceiling above; returns null when there is nothing to +// report. +function checkQueueEscalation( + stateStore: InstanceType, + thresholdMs: number | null +): string | null { + if (thresholdMs === null) { + return null; + } + const oldestAgeMs = stateStore.oldestQueuedSnapshotAgeMs(); if (oldestAgeMs === null || oldestAgeMs < thresholdMs) { - return; + return null; + } + + // Direct directory count instead of stateStore.listQueuedSnapshots() — + // that helper reads every queued snapshot's full local/base file trees + // off disk just to report a count here, on every single enqueue. + const queuedCount = readdirSync(stateStore.queueDir(), { withFileTypes: true }).filter( + (entry: { isDirectory: () => boolean }) => entry.isDirectory() + ).length; + + const sanityCeilingMs = thresholdMs * QUEUE_ESCALATION_SANITY_CEILING_MULTIPLE; + if (oldestAgeMs > sanityCeilingMs) { + return ( + `note: the oldest queued snapshot in ${stateStore.queueDir()} claims to be ` + + `${formatDurationMs(oldestAgeMs)} old, past the ${formatDurationMs(sanityCeilingMs)} sanity ceiling ` + + `(${QUEUE_ESCALATION_SANITY_CEILING_MULTIPLE}x the ${formatDurationMs(thresholdMs)} queue escalation ` + + `threshold) — skipping escalation instead of crashing loud, since an age this implausible more likely ` + + `means this machine's clock was wrong when the snapshot was queued than that the remote has genuinely ` + + `been unreachable this long.` + ); } - const queuedCount = stateStore.listQueuedSnapshots().length; throw new RemoteQueueEscalationError( `remote has been unreachable for ${formatDurationMs(oldestAgeMs)}, past the ` + `${formatDurationMs(thresholdMs)} queue escalation threshold (${queuedCount} snapshot(s) queued in ` + - `${stateStore.queueDir()}). This usually means the remote is permanently misconfigured (wrong remoteUrl, ` + - `a renamed repository path, or a host that accepts a connection but cannot serve the repository) rather ` + - `than temporarily offline — check remoteUrl/branch/repositorySubdir. Every queued snapshot is still ` + - `safely stored and will be replayed automatically once the remote is reachable again; tune the ` + - `'queueEscalationThresholdMs' config key if this threshold does not fit this machine's expected offline ` + - `windows.` + `${stateStore.queueDir()}); this usually means the remote is permanently misconfigured rather than ` + + `temporarily offline. Check remoteUrl/branch/repositorySubdir.` ); } diff --git a/packages/agent-memory-sync/src/memory-sync/state-store.ts b/packages/agent-memory-sync/src/memory-sync/state-store.ts index 89dcc96..264226a 100644 --- a/packages/agent-memory-sync/src/memory-sync/state-store.ts +++ b/packages/agent-memory-sync/src/memory-sync/state-store.ts @@ -16,6 +16,19 @@ const path = require("node:path"); // repository) can hide before an operator is guaranteed to see it within a // day. See push.ts's checkQueueEscalation and errors.ts's // RemoteQueueEscalationError for how this value is consumed. +// +// Wall-clock dependency: oldestQueuedSnapshotAgeMs derives this entirely +// from each queued manifest.json's `createdAt` (an ISO wall-clock timestamp +// written at enqueue time) compared against `Date.now()` at read time — so +// the age this threshold is measured against is only ever as trustworthy as +// this machine's system clock was AT ENQUEUE TIME, not some monotonic +// duration. A machine that enqueued snapshots under a wrong-in-the-past +// clock (a dead RTC battery, a container that started before NTP synced, +// ...) computes an implausibly large age the moment NTP corrects the clock +// forward, even though the remote may not have been unreachable that long at +// all. checkQueueEscalation in push.ts guards against exactly this with a +// sanity ceiling (30x the effective threshold) above which it skips +// escalating and emits a diagnostic note instead of crashing loud. const DEFAULT_QUEUE_ESCALATION_THRESHOLD_MS = 24 * 60 * 60 * 1000; interface SyncState { @@ -128,6 +141,23 @@ class StateStore { // tick that failed to reach the remote in the current unbroken failure // streak; it resets to null the moment a push actually succeeds. See // push.ts's checkQueueEscalation for how this is used. + // + // One-way bias, worth naming: because a missing/corrupt manifest is + // EXCLUDED from the age computation above rather than treated as + // "infinitely old", the result is always biased toward UNDER-estimating + // the true queue age, never over-estimating it. If the OLDEST snapshots + // happen to be the ones whose manifests got corrupted (e.g. a partial disk + // failure hit only the earliest entries), the computed age silently + // collapses to that of the oldest SURVIVING valid manifest — which may be + // much newer — and, in the limit, if every manifest is corrupt, this + // returns null exactly as if the queue were empty, suppressing escalation + // entirely even though a genuinely stuck queue is still sitting there. + // This is a deliberate defensive choice (a corrupt manifest must not + // itself manufacture a false escalation), but it can never cause a FALSE + // escalation, only a missed one. See the "a mix of some corrupt manifests + // and one older valid manifest" test in tests/unit/state-store.test.ts, + // which pins the other half of this trade-off: as long as at least one old + // manifest survives intact, escalation still fires from it. oldestQueuedSnapshotAgeMs(referenceTime: number = Date.now()): number | null { this.ensure(); const createdTimestamps = readdirSync(this.queueDir(), { withFileTypes: true }) diff --git a/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts b/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts index 0a62cb2..979fbe4 100644 --- a/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts +++ b/packages/agent-memory-sync/tests/integration/remote-unreachable-escalation.test.ts @@ -201,3 +201,243 @@ test("push still queues cleanly when an unsupported-scheme remote (precheck assu assert.equal(fileExists(queueDirFor(workspaceRoot)), true); assert.equal(readdirSync(queueDirFor(workspaceRoot)).length, 1); }); + +// Reviewer-named positive test (agent-tasks 11424b5e fix round, finding +// #11): escalation must fire from the catch-all git-failure path +// (push.ts's catch block around GitClient.lookupRemoteHead/push, ~line +// 225-234), not only from the reachability-precheck skip path every test +// above exercises. Reuses AC4's precheck-bypass mechanism above (an +// unsupported-scheme remote the precheck assumes reachable, so the failure +// only ever surfaces once the real git operation runs and throws inside the +// try block) — but this time with the queue already backdated past the +// threshold, so the SAME checkQueueEscalation call now runs from inside the +// catch block instead of the reachability-precheck branch. +test("escalation also fires from the catch-all git-failure path, not only the reachability-precheck path", () => { + const root = createSandbox("escalation-catch-path"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + const bypassRemoteUrl = "git://127.0.0.1:1/definitely-not-a-real-remote.git"; + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, bypassRemoteUrl)); + + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + backdateQueuedSnapshots(workspaceRoot, 25 * 60 * 60 * 1000); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { expectFailure: true } + ); + + assert.equal( + secondRun.status, + 6, + `expected escalation via the catch-all git-failure path. stderr: ${secondRun.stderr}` + ); + assert.match(secondRun.stderr, /remote has been unreachable for/); + assert.match(secondRun.stderr, /permanently misconfigured/); + + // The escalating tick's own snapshot must still be safely persisted. + const queuedEntries = readdirSync(queueDirFor(workspaceRoot)); + assert.equal(queuedEntries.length, 2, `expected both snapshots to remain queued, found: ${queuedEntries.join(", ")}`); +}); + +// ─── Clock-skew guard: an implausibly large age skips escalation ──────────── +// +// Reviewer finding (MEDIUM #3): a queued manifest's `createdAt` is a +// wall-clock ISO timestamp, so oldestQueuedSnapshotAgeMs is only ever as +// trustworthy as this machine's system clock was AT ENQUEUE TIME. A machine +// that enqueued under a wrong-in-the-past clock must not turn crash-loud the +// moment NTP corrects it forward. checkQueueEscalation (push.ts) guards this +// with a sanity ceiling (30x the effective threshold) — past it, the tick +// stays a clean, silent-except-for-one-note "queued" outcome instead of +// escalating. +test("push does not escalate when the oldest queued snapshot's age is implausibly large (clock-skew guard) — stays exit 0 with a diagnostic note", () => { + const root = createSandbox("escalation-clock-skew"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + // 100x the real 24h production default — well past the 30x sanity + // ceiling, so this must be treated as an implausible clock artifact, not + // 100 days of genuine remote unavailability. + backdateQueuedSnapshots(workspaceRoot, 100 * 24 * 60 * 60 * 1000); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const secondPayload = JSON.parse(secondRun.stdout); + + assert.equal(secondRun.status, 0, `expected a clean exit 0 despite the implausible age. stderr: ${secondRun.stderr}`); + assert.equal(secondPayload.runs[0].status, "queued"); + assert.ok( + (secondPayload.runs[0].notes || []).some((note: string) => /implausible/.test(note)), + `expected a clock-skew diagnostic note, got: ${JSON.stringify(secondPayload.runs[0].notes)}` + ); +}); + +// A plain 25h age (well under the 30x sanity ceiling) must still escalate — +// this is the SAME assertion the "push crashes loud..." AC1 test above +// already makes; restated here in the same section as the guard's negative +// case per the fix-round brief, without duplicating the full test body. +test("push still escalates a plain 25h age — the clock-skew guard does not swallow genuine escalations", () => { + const root = createSandbox("escalation-clock-skew-normal"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + backdateQueuedSnapshots(workspaceRoot, 25 * 60 * 60 * 1000); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { expectFailure: true } + ); + + assert.equal(secondRun.status, 6, `expected the escalation's own exit code. stderr: ${secondRun.stderr}`); + assert.match(secondRun.stderr, /remote has been unreachable for/); +}); + +// ─── Config wiring: queueEscalationThresholdMs must actually reach +// checkQueueEscalation (MEDIUM finding #2 — mutation-invisible before +// this fix: every prior test above uses the 24h default, so unwiring +// the config/env override entirely at loader.ts still left every +// existing test green). Each case below backdates the queued manifest +// to an age LESS than the real 24h default but MORE than the override +// configured — so a run that silently fell back to the default instead +// of honoring the override would wrongly stay exit 0 here, catching +// exactly the mutation this finding names. See the mutation-verify note +// in the fix-round summary for the actual red/green run against this +// pair. ────────────────────────────────────────────────────────────── + +const CONFIG_WIRING_OVERRIDE_MS = 60 * 60 * 1000; // 1h — well under 24h +const CONFIG_WIRING_BACKDATE_MS = 2 * 60 * 60 * 1000; // 2h — under 24h, over the 1h override + +test("queueEscalationThresholdMs from the project config file is actually wired through", () => { + const root = createSandbox("escalation-config-override"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, { + ...createConfig(workspaceRoot, offlineRemoteDir), + queueEscalationThresholdMs: CONFIG_WIRING_OVERRIDE_MS + }); + + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + backdateQueuedSnapshots(workspaceRoot, CONFIG_WIRING_BACKDATE_MS); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { expectFailure: true } + ); + + assert.equal( + secondRun.status, + 6, + `expected the configured 1h override to escalate at 2h old (would stay exit 0 under the unwired 24h ` + + `default). stderr: ${secondRun.stderr}` + ); + assert.match(secondRun.stderr, /remote has been unreachable for/); +}); + +test("queueEscalationThresholdMs from AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS is actually wired through", () => { + const root = createSandbox("escalation-env-override"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + const env = { + ...process.env, + AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS: String(CONFIG_WIRING_OVERRIDE_MS) + }; + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + const firstRun = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { env } + ); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + backdateQueuedSnapshots(workspaceRoot, CONFIG_WIRING_BACKDATE_MS); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { env, expectFailure: true } + ); + + assert.equal( + secondRun.status, + 6, + `expected the env override to escalate at 2h old. stderr: ${secondRun.stderr}` + ); + assert.match(secondRun.stderr, /remote has been unreachable for/); +}); + +test("an invalid AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS value fails loud with a config error, not a silent fallback", () => { + const root = createSandbox("escalation-env-invalid"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + const env = { ...process.env, AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS: "not-a-number" }; + + writeText(path.join(workspaceRoot, "MEMORY.md"), "content\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, path.join(root, "unused-remote.git"))); + + const result = runCli( + ["run", "default", "--config", configPath, "--mode", "push", "--output", "json"], + { env, expectFailure: true } + ); + + assert.equal(result.status, 3, `expected a config error exit code. stderr: ${result.stderr}`); + assert.match(result.stderr, /AGENT_MEMORY_SYNC_QUEUE_ESCALATION_THRESHOLD_MS/); + assert.match(result.stderr, /positive integer/); +}); + +// ─── Off switch: null disables escalation entirely (MEDIUM finding #8) ────── +test("queueEscalationThresholdMs: null disables escalation entirely — an ancient queue stays exit 0", () => { + const root = createSandbox("escalation-disabled"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "first\n"); + writeProjectConfig(configPath, { + ...createConfig(workspaceRoot, offlineRemoteDir), + queueEscalationThresholdMs: null + }); + + const firstRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(firstRun.stdout).runs[0].status, "queued"); + + // Far older than the real 24h default — would escalate under any finite + // threshold, including the default this machine would otherwise fall + // back to. + backdateQueuedSnapshots(workspaceRoot, 365 * 24 * 60 * 60 * 1000); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "second\n"); + const secondRun = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const secondPayload = JSON.parse(secondRun.stdout); + + assert.equal(secondRun.status, 0, `expected escalation to stay disabled. stderr: ${secondRun.stderr}`); + assert.equal(secondPayload.runs[0].status, "queued"); +}); diff --git a/packages/agent-memory-sync/tests/integration/run-schedule-escalation.test.ts b/packages/agent-memory-sync/tests/integration/run-schedule-escalation.test.ts new file mode 100644 index 0000000..b3ef84b --- /dev/null +++ b/packages/agent-memory-sync/tests/integration/run-schedule-escalation.test.ts @@ -0,0 +1,121 @@ +// Coverage for the `run --schedule` escalation-continuation fix (agent-tasks +// 11424b5e fix round, MEDIUM finding #6): measured before this fix, a +// scheduled `run` died on the FIRST tick that threw +// RemoteQueueEscalationError — exit 6 after tick 1 of 3, zero stdout, and +// (since `run --schedule` is its own scheduler/replay loop, not something an +// external supervisor restarts) no later tick ever got a chance to replay +// the queue once the remote recovered. src/commands/run.ts's scheduling +// loop now catches RemoteQueueEscalationError, records it, and keeps +// ticking — the whole invocation still exits 6 once the loop ends, just +// after every remaining tick had its shot and after the JSON payload +// (including every tick that ran) has been written to stdout. +// +// Real-time cost, deliberate: this package's cron parser (src/memory-sync/ +// scheduler.ts) has no sub-minute granularity, so "short interval" here +// means the shortest expressible one — "* * * * *" (every minute) — and a +// 2-tick run genuinely waits for one real cron boundary (up to ~60s) between +// ticks. There is no seam to inject a fake clock into the scheduling loop +// without expanding this fix's scope, so this test pays that real wait +// rather than skip end-to-end coverage of the loop itself. +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { readdirSync, readFileSync, writeFileSync } = require("node:fs"); +const path = require("node:path"); +const { createSandbox, runCli, writeProjectConfig, writeText } = require("../helpers/cli.ts"); + +function createConfig(workspaceRoot: string, remoteDir: string) { + return { + rootDir: workspaceRoot, + remoteUrl: remoteDir, + branch: "main", + repositorySubdir: "shared", + stateDir: ".agent-memory-sync/default", + reachabilityTimeoutMs: 500, + // A trivially small threshold: this test is about the SCHEDULING LOOP's + // continue-past-escalation behavior, not about reproducing the real 24h + // default, so every tick here is engineered to escalate quickly. 5s + // (not smaller) deliberately keeps the clock-skew sanity ceiling + // (30x threshold, push.ts's checkQueueEscalation — see fix #3 in the + // same round) at 150s, comfortably above the up-to-~60s real cron wait + // this test pays between its two ticks (scheduler.ts has no sub-minute + // granularity) — a smaller threshold here would make the sanity guard + // itself (correctly) suppress the SECOND tick's escalation. + queueEscalationThresholdMs: 5000, + syncPaths: [{ source: "MEMORY.md", destination: "MEMORY.md", kind: "file" }] + }; +} + +function queueDirFor(workspaceRoot: string): string { + return path.join(workspaceRoot, ".agent-memory-sync", "default", "queue"); +} + +function backdateQueuedSnapshots(workspaceRoot: string, ageMs: number): void { + const queueDir = queueDirFor(workspaceRoot); + const createdAt = new Date(Date.now() - ageMs).toISOString(); + for (const id of readdirSync(queueDir)) { + const manifestPath = path.join(queueDir, id, "manifest.json"); + const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); + writeFileSync(manifestPath, `${JSON.stringify({ ...manifest, createdAt }, null, 2)}\n`, "utf8"); + } +} + +test( + "run --schedule keeps ticking past an escalating tick and exits 6 only after every scheduled tick ran", + { timeout: 150_000 }, + () => { + const root = createSandbox("schedule-escalation"); + const offlineRemoteDir = path.join(root, "missing-remote.git"); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, offlineRemoteDir)); + + // Seed one queued snapshot, then backdate it past the 5s threshold + // above, so the FIRST tick of the scheduled run below is already + // escalating — no real waiting needed to reach that state. + const seed = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(seed.stdout).runs[0].status, "queued"); + backdateQueuedSnapshots(workspaceRoot, 6000); + + const result = runCli( + [ + "run", + "default", + "--config", + configPath, + "--mode", + "push", + "--schedule", + "* * * * *", + "--max-runs", + "2", + "--output", + "json" + ], + { expectFailure: true } + ); + + assert.equal( + result.status, + 6, + `expected the invocation to still exit 6 overall once both scheduled ticks ran. stderr: ${result.stderr}` + ); + + const payload = JSON.parse(result.stdout); + assert.equal( + payload.runs.length, + 2, + `expected BOTH scheduled ticks to run (the pre-fix bug died after tick 1). runs: ${JSON.stringify(payload.runs)}` + ); + for (const run of payload.runs) { + assert.equal(run.status, "escalated", `expected every tick here to have escalated. run: ${JSON.stringify(run)}`); + assert.match(run.notes.join(" "), /remote has been unreachable for/); + } + + // Both ticks enqueued their own "current" snapshot on top of the seed — + // none of them ever drained (the remote stayed offline the whole time), + // so all three snapshots are still safely queued for replay. + assert.equal(readdirSync(queueDirFor(workspaceRoot)).length, 3); + } +); diff --git a/packages/agent-memory-sync/tests/unit/state-store.test.ts b/packages/agent-memory-sync/tests/unit/state-store.test.ts index 80ca80a..5d62e86 100644 --- a/packages/agent-memory-sync/tests/unit/state-store.test.ts +++ b/packages/agent-memory-sync/tests/unit/state-store.test.ts @@ -76,6 +76,34 @@ test("oldestQueuedSnapshotAgeMs: a queued directory with a missing/corrupt manif assert.equal(store.oldestQueuedSnapshotAgeMs(), null); }); +// Reviewer-named test (agent-tasks 11424b5e fix round, LOW finding #7): pins +// the other half of the one-way-bias trade-off documented above +// oldestQueuedSnapshotAgeMs — a corrupt manifest must not suppress +// escalation as long as at least one genuinely old manifest survives +// intact. Without this, a naive implementation that defaulted a corrupt +// manifest's timestamp to "now" instead of excluding it would drag the +// MIN() toward the present and hide a real stuck queue. +test("oldestQueuedSnapshotAgeMs: a mix of some corrupt manifests and one older valid manifest still reports the valid one's age, not null", () => { + const store = new StateStore(sandbox("mixed-corrupt"), "default"); + const older = new Date("2026-01-01T00:00:00.000Z"); + + const survivingId = store.enqueueSnapshot({ localFiles: { "a.md": "one" }, baseFiles: {} }); + const corruptId = store.enqueueSnapshot({ localFiles: { "b.md": "two" }, baseFiles: {} }); + + const survivingManifestPath = path.join(store.queueDir(), survivingId, "manifest.json"); + const survivingManifest = JSON.parse(readFileSync(survivingManifestPath, "utf8")); + writeFileSync( + survivingManifestPath, + JSON.stringify({ ...survivingManifest, createdAt: older.toISOString() }, null, 2) + ); + + const corruptManifestPath = path.join(store.queueDir(), corruptId, "manifest.json"); + writeFileSync(corruptManifestPath, "{ not valid json"); + + const referenceTime = older.getTime() + 5000; + assert.equal(store.oldestQueuedSnapshotAgeMs(referenceTime), 5000); +}); + test("oldestQueuedSnapshotAgeMs: clears back to null after removeQueuedSnapshot empties the queue", () => { const store = new StateStore(sandbox("cleared"), "default"); const id = store.enqueueSnapshot({ localFiles: { "a.md": "one" }, baseFiles: {} });