Abort mark-synced on a repeated non-transient 4xx

src/index.ts

@@ -4,8 +4,11 @@ import {
   deleteRecords,
   fetchAllRecords,
   markRecordsSynced,
+  MARK_ABORTED,
   MARK_SYNCED,
+  MARK_TIMED_OUT,
   MarkSyncedItem,
+  MarkSyncedStop,
   PENDING_STATUS,
 } from '@/libs/records.js';
 import { describeApiError, isSystemicApiFailure } from '@/libs/api.js';
@@ -423,7 +426,7 @@ function reportDeferredServerChanges(deferredRecords: WrittenRecord[]): void {
 
 // Projects each written record down to the `{ uuid, filePath }` shape the bulk
 // mark-synced call needs, preserving order so the returned outcomes stay aligned
-// to `writtenRecords` by index. Chunking and the stop-on-timeout abort live in
+// to `writtenRecords` by index. Chunking and the stop-on-abort logic live in
 // `markRecordsSynced` (the records lib), keeping the API surface isolated there.
 function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] {
   return writtenRecords.map(({ record, filePath }) => ({
@@ -432,16 +435,44 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] {
   }));
 }
 
-// Headline for the mark-synced failure report. A timeout abort reads
-// differently from a scatter of per-record failures: it stopped the run early,
-// so the count includes records never attempted. Both leave the listed records
-// pending on the server.
-function markFailureHeadline(pendingCount: number, timedOut: boolean): string {
-  if (timedOut) {
-    return `Timed out marking records synced — stopped after the batch that first timed out; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`;
+// How a mark-synced run ended, for the failure report: the stop reason plus how
+// many pending records the run never reached. Bundled because they're only ever
+// meaningful together (the abort description), so the headline can't be handed a
+// count that belongs to a different reason.
+type MarkStopReport = {
+  reason: MarkSyncedStop;
+  unattemptedCount: number;
+};
+
+// Headline for the mark-synced failure report. An abort reads differently from a
+// scatter of per-record failures: it stopped the run early, so the pending count
+// can fold in records never attempted after the abort. `unattemptedCount` is how
+// many of those pending records were never sent (the chunks after the stop), so
+// the abort wording only claims "the rest were not attempted" when that's true —
+// an abort on the final chunk leaves nothing unattempted. All cases leave the
+// listed records pending on the server.
+function markFailureHeadline(
+  pendingCount: number,
+  stop: MarkStopReport,
+): string {
+  if (stop.reason === MARK_TIMED_OUT) {
+    return `Timed out marking records synced — stopped after the first timeout; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`;
+  }
+
+  if (stop.reason === MARK_ABORTED) {
+    const notAttemptedClause =
+      stop.unattemptedCount > 0 ? ' and the rest were not attempted' : '';
+    return `Aborted marking records synced — the server rejected the request wholesale (a 400/422), so every record would fail the same way${notAttemptedClause}; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`;
   }
 
-  return `Failed to mark ${pendingCount} record(s) synced — written locally but still pending on the server; they may be re-written next run.`;
+  // The generic branch also covers a systemic abort (auth/rate-limit/5xx), which
+  // stops the run early with no distinct stop reason — so surface how many of the
+  // pending records were never sent rather than implying all N were attempted.
+  const neverAttemptedClause =
+    stop.unattemptedCount > 0
+      ? ` (${stop.unattemptedCount} never attempted — the run stopped early)`
+      : '';
+  return `Failed to mark ${pendingCount} record(s) synced — written locally but still pending on the server${neverAttemptedClause}; they may be re-written next run.`;
 }
 
 // Surfaces mark-synced failures loudly (never as success): an unmarked record
@@ -451,10 +482,10 @@ function markFailureHeadline(pendingCount: number, timedOut: boolean): string {
 function reportMarkFailures(
   failures: WrittenRecord[],
   markedCount: number,
-  timedOut: boolean,
+  stop: MarkStopReport,
   spinner: Spinner,
 ): void {
-  spinner.error(markFailureHeadline(failures.length, timedOut));
+  spinner.error(markFailureHeadline(failures.length, stop));
   failures.forEach(({ record, filePath }) => {
     // Sanitize the composed line: record.uuid comes from the same untrusted API
     // response as a title, and filePath embeds the user-configured output path —
@@ -502,14 +533,15 @@ async function markWrittenRecordsSynced(
 
   spinner.start('Marking records synced...');
 
-  const { outcomes, timedOut } = await markRecordsSynced(
+  const { outcomes, stoppedBy } = await markRecordsSynced(
     toMarkSyncedItems(writtenRecords),
   );
   // A record is settled only when its mark-synced outcome is MARK_SYNCED; it is
   // pending if its mark failed or was never attempted (its outcome is undefined
-  // because a timeout aborted the run before its batch). Evict settled records
-  // from the written-path map so a long-running autoSync daemon doesn't leak
-  // memory — the "settled" half of the written-vs-settled split.
+  // because an abort — a timeout, a systemic failure, or a request-shape 4xx —
+  // stopped the run before its chunk). Evict settled records from the written-
+  // path map so a long-running autoSync daemon doesn't leak memory — the
+  // "settled" half of the written-vs-settled split.
   const settled = writtenRecords.filter(
     (_written, index) => outcomes[index] === MARK_SYNCED,
   );
@@ -523,10 +555,13 @@ async function markWrittenRecordsSynced(
   );
 
   if (pending.length > 0) {
+    // Records the run never reached: an abort/timeout stops before later chunks,
+    // so their outcome index is undefined and they have no per-record outcome.
+    const unattemptedCount = writtenRecords.length - outcomes.length;
     reportMarkFailures(
       pending,
       writtenRecords.length - pending.length,
-      timedOut,
+      { reason: stoppedBy, unattemptedCount },
       spinner,
     );
     return;

src/libs/api.ts

@@ -124,6 +124,15 @@ const AUTH_STATUS_CODES = [401, 403];
 // A rate-limit response will keep rejecting the whole burst, so a bulk caller
 // should back off rather than keep firing requests that make it worse.
 const RATE_LIMIT_STATUS_CODES = [429];
+// Request-shape failures: a malformed payload (400) or an off-contract
+// validation rejection (422, e.g. markpost tightening the PATCH attributes it
+// accepts). When every record in a batch is built the same way, such a failure
+// recurs identically for all of them, so a bulk caller can abort rather than
+// retry each doomed request. A per-record 4xx (a 404 for a record deleted
+// mid-run, a 422 on one record's own value) and a transient 429 are deliberately
+// excluded — the caller confirms the whole batch agreed before treating it as
+// request-shape.
+const FATAL_REQUEST_STATUS_CODES = [400, 422];
 // Any 5xx is a server-side fault, not something the caller's payload can fix.
 const SERVER_ERROR_MIN_STATUS = 500;
 
@@ -152,6 +161,15 @@ export class ApiRequestError extends Error {
     return this.statusCode >= SERVER_ERROR_MIN_STATUS;
   }
 
+  // A request-shape 4xx (400/422) that recurs identically for every record built
+  // the same way — the request the caller constructed is wrong, so firing the
+  // rest of a batch just repeats the same failure. Excludes per-record 4xx (a
+  // 404 for a record deleted mid-run) and the transient 429, which don't doom
+  // the batch.
+  get isFatalRequest(): boolean {
+    return FATAL_REQUEST_STATUS_CODES.includes(this.statusCode);
+  }
+
   // Systemic = will recur for every other request too, so a bulk caller should
   // stop rather than fire N requests it already knows are doomed.
   get isSystemic(): boolean {
@@ -176,6 +194,15 @@ export const isSystemicApiFailure = (
 ): error is ApiRequestError =>
   error instanceof ApiRequestError && error.isSystemic;
 
+// Narrowing guard: true only for a request-shape `ApiRequestError` (a 400/422
+// rejection — NOT a per-record 404, an auth 401/403, or a transient 429). Lets a
+// bulk caller TAG the outcome so it can decide, after seeing a SECOND chunk agree
+// with nothing synced, whether the request shape itself is wrong (see
+// `markRecordsSynced`). It does not itself mean "abort now" — a lone 400/422 can
+// still be an isolated rejection.
+export const isFatalRequestError = (error: unknown): error is ApiRequestError =>
+  error instanceof ApiRequestError && error.isFatalRequest;
+
 // Labels the failure by kind so the classification stays inside the API layer
 // instead of leaking status-code logic into command code. Falls back to a
 // generic label if handed a non-systemic error, so a mislabel can't happen.