From ee8e5a17a1dfe6cbc6e7f9b70580661e98a2f488 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 13:06:04 -0500 Subject: [PATCH 1/4] Abort mark-synced on a repeated non-transient 4xx Distinguish a request-shape 4xx (400/422) from transient/per-record failures in markRecordSynced and stop the mark-synced run when a whole batch is rejected the same way, instead of firing every remaining PATCH. A confirmation probe of the last record guards against stranding valid records behind a contiguous block of per-record 4xx. 429/404/401/403/5xx stay transient/per-record and never abort. Closes #137 --- src/index.ts | 163 +++++++--- src/libs/api.ts | 26 ++ src/libs/records.ts | 114 ++++++- tests/index.test.ts | 588 ++++++++++++++++++++++++++++++++++++- tests/libs/api.test.ts | 42 +++ tests/libs/records.test.ts | 194 ++++++++++++ 6 files changed, 1070 insertions(+), 57 deletions(-) diff --git a/src/index.ts b/src/index.ts index 8153873..312b311 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,9 +4,13 @@ import { deleteRecords, fetchAllRecords, markRecordSynced, + markSyncedStopReason, + probeStopReason, + MARK_ABORTED, MARK_SYNCED, MARK_TIMED_OUT, MarkSyncedOutcome, + MarkSyncedStop, PENDING_STATUS, } from '@/libs/records.js'; import { describeApiError, isSystemicApiFailure } from '@/libs/api.js'; @@ -431,54 +435,126 @@ function reportDeferredServerChanges(deferredRecords: WrittenRecord[]): void { } // Outcome of a whole mark-synced run. `outcomes` holds one entry per *attempted* -// record in the original order; on a timeout abort it's shorter than the input -// because the remaining batches were never sent. `timedOut` records whether a -// timeout stopped the run early so the caller can report the abort explicitly. +// record in the original order; on an abort it's shorter than the input because +// the remaining batches were never sent. `stoppedBy` records which abort reason +// stopped the run early (or null). `probeSyncedIndex` is the index of the last +// record IF a confirmation probe synced it (else null): the probe runs +// out-of-order and isn't in `outcomes`, so the caller must count that index as +// settled even when a later batch stopped the run before re-marking it. interface MarkSyncedRun { outcomes: MarkSyncedOutcome[]; - timedOut: boolean; + stoppedBy: MarkSyncedStop; + probeSyncedIndex: number | null; } -// PATCHes the written records synced in bounded-concurrency batches, stopping on -// the first timeout. A timeout means the server is hung, so firing the remaining -// batches would burn the full request timeout on each one before reporting; -// aborting leaves those records pending to retry next run, mirroring the push -// command's batch-abort. Non-timeout failures don't abort — the next record may -// still succeed. +// PATCHes the written records synced in bounded-concurrency batches, stopping +// early on a hung server (a timeout) or a request the server rejects wholesale. +// A suspected request-shape abort is CONFIRMED with a probe before stranding the +// records behind it: the LAST written record is attempted, and the run only +// aborts if that fails the same way. Probing the far end (not the record next to +// the rejected block) means a categorically wrong request still fails the probe, +// while a contiguous run of per-record 4xx at the FRONT of the queue does not — +// the valid records behind the bad block keep syncing instead of being stranded +// on every future run (the pending set is re-fetched in the same order each run). +// At most ONE probe runs per invocation: an inconclusive (transient) probe is +// enough evidence to keep going, and re-probing would only hammer one record. An +// abort leaves the unattempted records pending to retry next run, mirroring the +// push command's batch-abort. async function markRecordsInBatches( writtenRecords: WrittenRecord[], ): Promise { const outcomes: MarkSyncedOutcome[] = []; - - for ( - let start = 0; - start < writtenRecords.length; - start += MARK_SYNCED_CONCURRENCY - ) { - const batch = writtenRecords.slice(start, start + MARK_SYNCED_CONCURRENCY); + let anyRecordSynced = false; + let probeUsed = false; + let probeSyncedIndex: number | null = null; + let cursor = 0; + + while (cursor < writtenRecords.length) { + const batch = writtenRecords.slice( + cursor, + cursor + MARK_SYNCED_CONCURRENCY, + ); const batchOutcomes = await Promise.all( batch.map(({ record, filePath }) => markRecordSynced(record.uuid, filePath), ), ); - outcomes.push(...batchOutcomes); + cursor += batch.length; + + const stoppedBy = markSyncedStopReason(batchOutcomes, anyRecordSynced); + anyRecordSynced = anyRecordSynced || batchOutcomes.includes(MARK_SYNCED); + + if (stoppedBy === MARK_TIMED_OUT) { + return { outcomes, stoppedBy, probeSyncedIndex }; + } + + if (stoppedBy !== MARK_ABORTED) { + continue; + } + + // The rejected batch was the last one: every record was already attempted, so + // an abort would save no work and only assert an unverified cause. Report as + // plain per-record failures instead. Also skip if a probe already ran this + // invocation — one inconclusive probe is enough to keep going. + if (cursor >= writtenRecords.length || probeUsed) { + continue; + } + + probeUsed = true; + const probeOutcome = await probeLastRecord(writtenRecords); + // A synced probe proves the request shape is valid, so record it as settled + // (it isn't in `outcomes`) and let no later all-rejected batch abort. + if (probeOutcome === MARK_SYNCED) { + anyRecordSynced = true; + probeSyncedIndex = writtenRecords.length - 1; + } + + const probeStop = probeStopReason(probeOutcome); - if (batchOutcomes.includes(MARK_TIMED_OUT)) { - return { outcomes, timedOut: true }; + if (probeStop === MARK_TIMED_OUT) { + return { outcomes, stoppedBy: MARK_TIMED_OUT, probeSyncedIndex }; + } + + // Confirm the abort only when skipping the rest still saves un-attempted work + // — records other than the probed last one remain. Otherwise everything was + // attempted, so fall through to plain per-record failures. + const savedWorkRemains = cursor < writtenRecords.length - 1; + + if (probeStop === MARK_ABORTED && savedWorkRemains) { + return { outcomes, stoppedBy: MARK_ABORTED, probeSyncedIndex }; } } - return { outcomes, timedOut: false }; + return { outcomes, stoppedBy: null, probeSyncedIndex }; } -// 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.`; +// Attempts the LAST written record and returns its raw outcome (see +// markRecordsInBatches). Isolated so the probe target lives in one place; the +// caller classifies the outcome so it can also learn a synced probe proved the +// shape valid. +async function probeLastRecord( + writtenRecords: WrittenRecord[], +): Promise { + const probe = writtenRecords[writtenRecords.length - 1]; + + return markRecordSynced(probe.record.uuid, probe.filePath); +} + +// 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. All three cases leave the +// listed records pending on the server. +function markFailureHeadline( + pendingCount: number, + stoppedBy: MarkSyncedStop, +): string { + if (stoppedBy === 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 (stoppedBy === MARK_ABORTED) { + return `Aborted marking records synced — a whole batch was rejected the same way and a confirmation request failed too, so the rest were not attempted; ${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.`; @@ -491,10 +567,10 @@ function markFailureHeadline(pendingCount: number, timedOut: boolean): string { function reportMarkFailures( failures: WrittenRecord[], markedCount: number, - timedOut: boolean, + stoppedBy: MarkSyncedStop, spinner: Spinner, ): void { - spinner.error(markFailureHeadline(failures.length, timedOut)); + spinner.error(markFailureHeadline(failures.length, stoppedBy)); 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 — @@ -542,18 +618,19 @@ async function markWrittenRecordsSynced( spinner.start('Marking records synced...'); - const { outcomes, timedOut } = await markRecordsInBatches(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. - const settled = writtenRecords.filter( - (_written, index) => outcomes[index] === MARK_SYNCED, - ); - const pending = writtenRecords.filter( - (_written, index) => outcomes[index] !== MARK_SYNCED, - ); + const { outcomes, stoppedBy, probeSyncedIndex } = + await markRecordsInBatches(writtenRecords); + // A record is settled when its mark-synced outcome is MARK_SYNCED, OR it is the + // record a confirmation probe synced out-of-order (not in `outcomes`, but the + // server already accepted it, so it must not be reported as still pending). It + // is pending if its mark failed or was never attempted (its outcome is + // undefined because an abort stopped 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. + const isSettled = (index: number): boolean => + outcomes[index] === MARK_SYNCED || index === probeSyncedIndex; + const settled = writtenRecords.filter((_written, index) => isSettled(index)); + const pending = writtenRecords.filter((_written, index) => !isSettled(index)); forgetSettledRecords( writtenState, @@ -564,7 +641,7 @@ async function markWrittenRecordsSynced( reportMarkFailures( pending, writtenRecords.length - pending.length, - timedOut, + stoppedBy, spinner, ); return; diff --git a/src/libs/api.ts b/src/libs/api.ts index 446bec7..a861810 100644 --- a/src/libs/api.ts +++ b/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,14 @@ 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 whole batch agree, +// whether the request shape itself is wrong (see markSyncedStopReason). It does +// not itself mean "abort now" — a lone 400/422 can still be per-record. +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. diff --git a/src/libs/records.ts b/src/libs/records.ts index b01a2df..ac3f7aa 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -1,6 +1,7 @@ import { ApiTimeoutError, authedRequest, + isFatalRequestError, isSystemicApiFailure, logApiFailure, unwrapResourceAttributes, @@ -26,10 +27,15 @@ export const PENDING_STATUS = 'pending'; const SYNCED_STATUS = 'synced'; // Result of marking one record synced. `MARK_SYNCED` — the server accepted the -// PATCH. `MARK_FAILED` — a non-timeout error; the record stays pending and the -// rest of the batch still runs. `MARK_TIMED_OUT` — the PATCH hit the request -// timeout, a signal the server is hung; the batch runner stops on the first one -// rather than paying the full timeout on every remaining record. +// PATCH. `MARK_FAILED` — a transient/per-record error (network blip, 429, 5xx, +// an unparseable 2xx); the record stays pending and the rest of the batch still +// runs. `MARK_TIMED_OUT` — the PATCH hit the request timeout, a signal the +// server is hung; the batch runner stops on the first one rather than paying the +// full timeout on every remaining record. `MARK_ABORTED` — a request-shape 4xx +// (a malformed-payload 400 or a contract-validation 422, but NOT a per-record +// 404, an auth 401/403, or a transient 429): the request the CLI built may be +// wrong for every record, so the batch runner aborts once it sees a whole batch +// rejected the same way rather than retrying each doomed record. // // Values are prefixed (`mark-*`) so they never collide with the wire // `SYNCED_STATUS = 'synced'` above: these are internal outcome tags, not the @@ -38,9 +44,76 @@ const SYNCED_STATUS = 'synced'; export const MARK_SYNCED = 'mark-synced'; export const MARK_FAILED = 'mark-failed'; export const MARK_TIMED_OUT = 'mark-timed-out'; +export const MARK_ABORTED = 'mark-aborted'; export type MarkSyncedOutcome = - typeof MARK_SYNCED | typeof MARK_FAILED | typeof MARK_TIMED_OUT; + | typeof MARK_SYNCED + | typeof MARK_FAILED + | typeof MARK_TIMED_OUT + | typeof MARK_ABORTED; + +// Why a mark-synced run stopped early (a hung server or a categorically wrong +// request), or null if every record was attempted. +export type MarkSyncedStop = typeof MARK_TIMED_OUT | typeof MARK_ABORTED | null; + +// Decides whether a mark-synced run should stop early, given ONE concurrency +// batch's outcomes and whether any record has already synced this run. Pure so +// the batch runner stays a thin loop and this policy is unit-testable in +// isolation. +// +// A timeout stops immediately — the server is hung, so firing more batches just +// burns the full request timeout on each. A request-shape 4xx stops the run only +// when the evidence is unambiguous: nothing has synced yet (a single success +// would prove the shape valid) AND a full batch of MORE THAN ONE record was +// UNANIMOUSLY rejected that way. Both guards avoid stranding records: +// - Requiring unanimity (`every`, not `some`) means one transient blip (a 429 +// or 5xx) among the rejections keeps the run going rather than aborting on +// what might be a lone per-record 4xx. Cost is bounded (a few more doomed +// requests); the alternative risks stranding syncable records. +// - Requiring `length > 1` stops a one-record tail batch from trivially +// satisfying `every` — a lone 4xx there is per-record (one bad filePath, a +// record deleted mid-run), not proof the shape is categorically wrong. +// A timeout and an all-aborted batch are mutually exclusive (an aborted batch +// contains no timeout), so their order here is not a tie-break. +export const markSyncedStopReason = ( + batchOutcomes: MarkSyncedOutcome[], + anySynced: boolean, +): MarkSyncedStop => { + if (batchOutcomes.includes(MARK_TIMED_OUT)) { + return MARK_TIMED_OUT; + } + + const wholeBatchRejected = + batchOutcomes.length > 1 && + batchOutcomes.every((outcome) => outcome === MARK_ABORTED); + + if (!anySynced && wholeBatchRejected) { + return MARK_ABORTED; + } + + return null; +}; + +// Maps a single confirmation-probe outcome to a stop reason. After a whole batch +// is rejected with nothing synced, the runner probes ONE record from beyond that +// batch (see markRecordsInBatches). Unlike `markSyncedStopReason`, a lone reject +// here IS decisive: the probe is a different record than the ones that failed, so +// its rejection confirms the request shape itself is wrong. A probe timeout still +// means a hung server. Any other outcome (synced, or a transient failure) leaves +// the shape unproven-bad, so the run keeps going rather than strand the rest. +export const probeStopReason = ( + probeOutcome: MarkSyncedOutcome, +): MarkSyncedStop => { + if (probeOutcome === MARK_TIMED_OUT) { + return MARK_TIMED_OUT; + } + + if (probeOutcome === MARK_ABORTED) { + return MARK_ABORTED; + } + + return null; +}; // markpost paginates with a cursor: each response's `links.next` embeds the // `page[after]` cursor to request the following page, and is `null` once @@ -377,12 +450,14 @@ export const createRecord = async ( // next run, which is far less disruptive than aborting the whole sync after // files have landed. // -// Returns a three-way outcome rather than a bare boolean so the caller can -// tell a per-record failure (`MARK_FAILED`, keep going — the next record may -// succeed) apart from a timeout (`MARK_TIMED_OUT`). A timeout signals a hung -// server, so the caller stops the remaining batches instead of burning the -// full request timeout on every one; the record still stays `pending` either -// way. Reading the body back as a resource would mis-report a legitimate 2xx +// Returns a four-way outcome rather than a bare boolean so the caller can tell +// a per-record failure (`MARK_FAILED`, keep going — the next record may succeed) +// apart from the two abort signals: a timeout (`MARK_TIMED_OUT`, a hung server) +// and a request-shape 4xx (`MARK_ABORTED`, a 400/422 the server rejected). On +// either abort the caller stops the remaining batches — a timeout to avoid +// burning the full request timeout on every one, an aborted 4xx because every +// remaining record would fail identically; the record still stays `pending` +// either way. Reading the body back as a resource would mis-report a legitimate 2xx // that carries no `data` (markpost's PATCH always returns the record, but a // `data: null` shape still counts as success here) as a failure, wrongly // warning the user of duplicates. `filePath` is sent deliberately — markpost @@ -426,12 +501,25 @@ export const markRecordSynced = async ( ); // A timeout gets its own outcome so the caller can abort the remaining - // marks on the first one; every other error just leaves this record - // pending and lets the rest of the batch proceed. + // marks on the first one. if (error instanceof ApiTimeoutError) { return MARK_TIMED_OUT; } + // A request-shape 4xx (a malformed-payload 400 or a contract-validation 422) + // means the request the CLI built may be wrong for every record. Tag it so + // the batch runner can abort — but only once it confirms the WHOLE batch + // failed the same way (see markRecordsInBatches); a per-record 404, an auth + // 401/403, and a transient 429 stay out of this entirely. + if (isFatalRequestError(error)) { + return MARK_ABORTED; + } + + // Everything else just leaves this record pending and lets the rest of the + // batch proceed: a network blip, a 404/401/403/429/5xx, or an unparseable + // body at ANY status — a 4xx delivered as an HTML error page (a WAF/proxy + // interstitial) throws before it can be classified as request-shape, so it + // safely degrades here rather than aborting the run. return MARK_FAILED; } }; diff --git a/tests/index.test.ts b/tests/index.test.ts index 9f7b178..2a9736e 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -4,7 +4,12 @@ import type { Spinner } from 'yocto-spinner'; import { Record } from '@/types/records.types.js'; import { UserSettings, ConflictStrategy } from '@/types/settings.types.js'; import { SettingsReadResult } from '@/libs/settings.js'; -import { MARK_FAILED, MARK_SYNCED, MARK_TIMED_OUT } from '@/libs/records.js'; +import { + MARK_ABORTED, + MARK_FAILED, + MARK_SYNCED, + MARK_TIMED_OUT, +} from '@/libs/records.js'; import type { WrittenRecordState } from '@/libs/markdown.js'; vi.mock('@/libs/config.js', () => ({ @@ -1354,6 +1359,587 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); + it('aborts the run when a whole batch is rejected as a request-shape 4xx — the next batch is never attempted', async () => { + const records: Record[] = Array.from({ length: 15 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Every PATCH is rejected the same way — the request shape itself is wrong (a + // malformed body or a PATCH contract markpost tightened). Batch 1 (10) all + // abort, then the confirmation probe of the LAST record (uuid-14) also aborts, + // confirming the shape is wrong — so uuid-10..13 are never attempted. + vi.mocked(markRecordSynced).mockResolvedValue(MARK_ABORTED); + + await import('@/index.js'); + + // 10 in the first batch plus the one probe of the last record (uuid-14) = 11. + expect(markRecordSynced).toHaveBeenCalledTimes(11); + expect(markRecordSynced).not.toHaveBeenCalledWith( + 'uuid-11', + expect.anything(), + ); + // All 15 stay pending: 10 rejected, the probed last one, and 4 never attempted. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('15 record(s) still pending'), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + // The abort headline must not read as a timeout or a plain scatter of + // failures — guards the reason wiring. + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Timed out marking records synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Failed to mark'), + ); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('! uuid-0 -> /mock/output/uuid-0.md'), + ); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('! uuid-14 -> /mock/output/uuid-14.md'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not abort when only one un-attempted record remains after a rejected batch', async () => { + const records: Record[] = Array.from({ length: 11 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // 11 records all reject: batch 1 (uuid-0..9) triggers a probe of the last + // record (uuid-10) — but uuid-10 IS the only record left, so a confirmed abort + // would save no work. It must NOT abort; the loop marks uuid-10 in its own + // batch (an idempotent repeat of the probe) and reports plain failures. + vi.mocked(markRecordSynced).mockResolvedValue(MARK_ABORTED); + + await import('@/index.js'); + + // 10 (batch 1) + 1 probe (uuid-10) + 1 (uuid-10's own batch) = 12. + expect(markRecordSynced).toHaveBeenCalledTimes(12); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 11 record(s) synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not claim an abort when the whole (only) batch is rejected and nothing is left to probe', async () => { + const records: Record[] = Array.from({ length: 10 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Exactly one batch (10 records), all rejected — the whole pending set was + // already attempted, so there is no later record to probe and no work an + // abort could save. Reporting an abort here would assert a request-shape + // cause we never confirmed, so it must fall back to plain per-record failures. + vi.mocked(markRecordSynced).mockResolvedValue(MARK_ABORTED); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledTimes(10); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 10 record(s) synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(process.exitCode).toBe(1); + }); + + it('aborts on a whole request-shape 4xx batch that lands after an earlier transient-failure batch', async () => { + const records: Record[] = Array.from({ length: 25 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Batch 1 (uuid-0..9) all fail transiently (nothing synced), batch 2 + // (uuid-10..19) is wholly request-shape rejected → probe the last record + // (uuid-24), which also aborts → confirmed, so uuid-20..23 are never + // attempted. Proves the abort works in a later batch, not just the first, and + // uses its headline (not the plain-failure one) despite the earlier failures. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + const index = Number(uuid.replace('uuid-', '')); + return Promise.resolve(index < 10 ? MARK_FAILED : MARK_ABORTED); + }); + + await import('@/index.js'); + + // 10 transient + 10 rejected + the one probe of the last record (uuid-24) = 21. + expect(markRecordSynced).toHaveBeenCalledTimes(21); + expect(markRecordSynced).not.toHaveBeenCalledWith( + 'uuid-20', + expect.anything(), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('25 record(s) still pending'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not strand valid records behind a contiguous block of per-record 4xx — the probe rescues them', async () => { + const records: Record[] = Array.from({ length: 25 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // The first 10 pending records each 4xx for their OWN bad value (a contiguous + // block, not a wrong request shape); uuid-10..24 are valid. Batch 1 all + // aborts, but the probe of the LAST record (uuid-24) SUCCEEDS, proving the + // shape is fine — so the run must NOT abort and must sync every record behind + // the bad block. Without the probe, uuid-10..24 would strand forever. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + const index = Number(uuid.replace('uuid-', '')); + return Promise.resolve(index < 10 ? MARK_ABORTED : MARK_SYNCED); + }); + + await import('@/index.js'); + + // Every record attempted: no stranding. The probed last record (uuid-24) is + // attempted twice — once as the decision-only probe, once in its own batch + // (an idempotent repeat) — so 25 records + 1 probe = 26 calls. + expect(markRecordSynced).toHaveBeenCalledTimes(26); + expect(markRecordSynced).toHaveBeenCalledWith( + 'uuid-24', + expect.anything(), + ); + // The 10 bad records stay pending as plain failures; nothing aborted. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 10 record(s) synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 15 record(s) synced despite'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not strand valid records behind a bad block wider than one batch', async () => { + const records: Record[] = Array.from({ length: 25 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // A per-record bad block of 20 records (uuid-0..19) — TWO full batches, wider + // than one batch — with uuid-20..24 valid. Probing the far end (uuid-24) + // rather than the record next to each rejected batch keeps the block from + // ever confirming an abort, so the valid tail still syncs. This is the case + // an adjacent probe would strand. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + const index = Number(uuid.replace('uuid-', '')); + return Promise.resolve(index < 20 ? MARK_ABORTED : MARK_SYNCED); + }); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledWith('uuid-24', expect.anything()); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 20 record(s) synced'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 5 record(s) synced despite'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not report a probe-synced record as pending when a later batch stops the run', async () => { + const records: Record[] = Array.from({ length: 25 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Batch 1 (uuid-0..9) rejects → probe of the last record (uuid-24) SYNCS, + // proving the shape is fine. Then batch 2 times out (uuid-10) and stops the + // run before uuid-24 is re-marked in order. uuid-24 was accepted server-side, + // so it must NOT be listed as still pending — regression guard for the + // out-of-order probe outcome being discarded. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + const index = Number(uuid.replace('uuid-', '')); + if (index < 10) { + return Promise.resolve(MARK_ABORTED); + } + if (uuid === 'uuid-10') { + return Promise.resolve(MARK_TIMED_OUT); + } + return Promise.resolve(MARK_SYNCED); + }); + + await import('@/index.js'); + + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Timed out marking records synced'), + ); + // uuid-24 synced via the probe — it must not appear in the still-pending list. + expect(console.error).not.toHaveBeenCalledWith( + expect.stringContaining('! uuid-24 -> /mock/output/uuid-24.md'), + ); + expect(process.exitCode).toBe(1); + }); + + it('stops as a timeout when the confirmation probe times out', async () => { + const records: Record[] = Array.from({ length: 15 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Batch 1 (uuid-0..9) all reject; the probe of the LAST record (uuid-14) + // times out. A hung server outranks the shape question, so the run stops as a + // timeout and uuid-10..13 are never attempted. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + if (uuid === 'uuid-14') { + return Promise.resolve(MARK_TIMED_OUT); + } + return Promise.resolve(MARK_ABORTED); + }); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledTimes(11); + expect(markRecordSynced).not.toHaveBeenCalledWith( + 'uuid-11', + expect.anything(), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Timed out marking records synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(process.exitCode).toBe(1); + }); + + it('keeps going when the confirmation probe fails transiently rather than aborting', async () => { + const records: Record[] = Array.from({ length: 25 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Batch 1 (uuid-0..9) all reject; the probe of the LAST record (uuid-24) + // fails transiently (a network blip / 429), which does NOT confirm a wrong + // shape — so the run must keep going and attempt every remaining record, not + // abort. uuid-10..23 are valid. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + const index = Number(uuid.replace('uuid-', '')); + if (index < 10) { + return Promise.resolve(MARK_ABORTED); + } + return Promise.resolve(uuid === 'uuid-24' ? MARK_FAILED : MARK_SYNCED); + }); + + await import('@/index.js'); + + // 25 records + the one decision-only probe of uuid-24 (idempotent repeat) = 26. + expect(markRecordSynced).toHaveBeenCalledTimes(26); + expect(markRecordSynced).toHaveBeenCalledWith('uuid-24', expect.anything()); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + // uuid-0..9 rejected + uuid-24 transient = 11 pending, reported as plain + // failures. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 11 record(s) synced'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not abort on a lone request-shape 4xx — one poison record must not strand the rest', async () => { + const records: Record[] = Array.from({ length: 15 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Only uuid-3 hits a 4xx (its own value the server rejects); the other nine + // in the batch succeed. Since the batch did NOT all fail the same way, the + // run must keep going — otherwise this one record would strand every later + // record on every future run. Guards the batch-agreement abort condition. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => + Promise.resolve(uuid === 'uuid-3' ? MARK_ABORTED : MARK_SYNCED), + ); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledTimes(15); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 1 record(s) synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('! uuid-3 -> /mock/output/uuid-3.md'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 14 record(s) synced despite'), + ); + expect(process.exitCode).toBe(1); + }); + + it('does not abort a whole final batch of 4xx once earlier records have synced — the shape is proven good', async () => { + const records: Record[] = Array.from({ length: 15 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // The first batch of 10 all succeed, proving the request shape the CLI + // builds is valid; the final batch of 5 (uuid-10..14) is then wholly 4xx. + // Because something already synced, those 4xx must be per-record — the run + // must NOT abort, and must report them as plain failures, not an abort. + vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { + const index = Number(uuid.replace('uuid-', '')); + return Promise.resolve(index >= 10 ? MARK_ABORTED : MARK_SYNCED); + }); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledTimes(15); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 5 record(s) synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('! uuid-10 -> /mock/output/uuid-10.md'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 10 record(s) synced despite'), + ); + expect(process.exitCode).toBe(1); + }); + it('reports only the timed-out record when the timeout lands in the final batch', async () => { const records: Record[] = Array.from({ length: 10 }, (_item, index) => ({ uuid: `uuid-${index}`, diff --git a/tests/libs/api.test.ts b/tests/libs/api.test.ts index fa310bb..8a38cb1 100644 --- a/tests/libs/api.test.ts +++ b/tests/libs/api.test.ts @@ -12,6 +12,7 @@ import { formatErrorMessages, getApiToken, getBaseUrl, + isFatalRequestError, isSystemicApiFailure, logApiFailure, rethrowIfTimeout, @@ -430,6 +431,47 @@ describe('ApiRequestError', () => { expect(new ApiRequestError('nope', statusCode).isPermanent).toBe(false); } }); + + // Request-shape 4xx (400/422) is what lets a bulk caller abort on a whole + // batch that failed the same way. The boundaries matter: a per-record 404, an + // auth 401/403, a transient 429, and any 5xx must stay OUT so they don't + // trigger the abort. + it('classifies only 400 and 422 as request-shape (fatal) errors', () => { + for (const statusCode of [400, 422]) { + expect(new ApiRequestError('nope', statusCode).isFatalRequest).toBe( + true, + ); + } + }); + + it('does not classify per-record, auth, rate-limit, or server 4xx/5xx as request-shape', () => { + for (const statusCode of [401, 403, 404, 409, 429, 500, 503]) { + expect(new ApiRequestError('nope', statusCode).isFatalRequest).toBe( + false, + ); + } + }); +}); + +describe('isFatalRequestError', () => { + it('is true only for a 400/422 ApiRequestError', () => { + expect(isFatalRequestError(new ApiRequestError('nope', 400))).toBe(true); + expect(isFatalRequestError(new ApiRequestError('nope', 422))).toBe(true); + }); + + it('is false for a per-record, auth, rate-limit, or 5xx ApiRequestError', () => { + for (const statusCode of [401, 404, 429, 500]) { + expect(isFatalRequestError(new ApiRequestError('nope', statusCode))).toBe( + false, + ); + } + }); + + it('is false for a plain Error or non-error value', () => { + expect(isFatalRequestError(new Error('network down'))).toBe(false); + expect(isFatalRequestError('boom')).toBe(false); + expect(isFatalRequestError(undefined)).toBe(false); + }); }); describe('isSystemicApiFailure', () => { diff --git a/tests/libs/records.test.ts b/tests/libs/records.test.ts index 77fa4ef..c98704d 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -7,6 +7,9 @@ import { fetchPaginatedRecords, fetchRecord, markRecordSynced, + markSyncedStopReason, + probeStopReason, + MARK_ABORTED, MARK_FAILED, MARK_SYNCED, MARK_TIMED_OUT, @@ -1308,4 +1311,195 @@ describe('markRecordSynced', () => { MARK_FAILED, ); }); + + // A request-shape 4xx (a contract-validation 422 or a malformed-payload 400) + // means the request the CLI built may be wrong for every record. Return the + // abort outcome so the batch runner can stop once it sees a whole batch fail + // this way instead of retrying each doomed record. + it('returns the aborted outcome on a 422 the request payload caused', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 422, + json: () => + Promise.resolve({ + data: { + errors: [ + { title: 'Unprocessable', detail: 'Unknown attribute: filePath' }, + ], + }, + }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_ABORTED, + ); + }); + + it('returns the aborted outcome on a malformed-payload 400', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 400, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Bad Request', detail: 'Invalid body' }] }, + }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_ABORTED, + ); + }); + + // A 404 means THIS record is gone server-side (deleted in the UI between the + // fetch and the PATCH), not that the request shape is wrong — the rest of the + // batch can still be marked. Guards the 4xx-abort against catching a per-record + // 404. + it('returns the failed outcome on a 404 instead of aborting', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 404, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Not Found', detail: 'No such record' }] }, + }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_FAILED, + ); + }); + + // An auth 401/403 is systemic but out of this abort's scope — mark-synced + // treats it as a plain per-record failure (the file is already on disk), same + // as before. Guards the 4xx-abort against widening to auth codes. + it('returns the failed outcome on a 401 instead of aborting', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 401, + json: () => Promise.resolve({ data: { errors: [] } }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_FAILED, + ); + }); + + // A 429 is transient (back off and retry), NOT a doomed payload — it must stay + // a plain failure so the run doesn't abort the rest of the batch on a + // rate-limit blip. Guards the 4xx-abort against catching 429. + it('returns the failed outcome on a 429 rate-limit instead of aborting', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 429, + json: () => + Promise.resolve({ + data: { + errors: [{ title: 'Too Many Requests', detail: 'Slow down' }], + }, + }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_FAILED, + ); + }); + + // A 4xx delivered as a non-JSON body (an HTML WAF/proxy interstitial) throws + // while parsing before it can be classified as request-shape, so it must + // degrade to a plain failure — never an abort — failing in the safe direction. + it('returns the failed outcome on a 422 whose body is not JSON', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 422, + json: () => Promise.reject(new SyntaxError('Unexpected token <')), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_FAILED, + ); + }); + + // A 5xx is a server-side fault the payload can't fix and may clear on retry, so + // it stays a plain failure rather than aborting the batch. + it('returns the failed outcome on a 500 instead of aborting', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 500, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Server Error', detail: 'Boom' }] }, + }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_FAILED, + ); + }); +}); + +describe('markSyncedStopReason', () => { + it('stops on a timeout anywhere in the batch, regardless of prior success', () => { + expect(markSyncedStopReason([MARK_SYNCED, MARK_TIMED_OUT], false)).toBe( + MARK_TIMED_OUT, + ); + expect(markSyncedStopReason([MARK_TIMED_OUT, MARK_ABORTED], true)).toBe( + MARK_TIMED_OUT, + ); + }); + + it('aborts when a multi-record batch is unanimously rejected and nothing has synced yet', () => { + expect(markSyncedStopReason([MARK_ABORTED, MARK_ABORTED], false)).toBe( + MARK_ABORTED, + ); + }); + + // A one-record tail batch trivially satisfies `every`, so a lone 4xx there is + // NOT proof the shape is categorically wrong — it's a per-record rejection and + // must not strand whatever follows. + it('does NOT abort a single-record batch — a lone 4xx there is per-record', () => { + expect(markSyncedStopReason([MARK_ABORTED], false)).toBeNull(); + }); + + // An empty batch is unreachable from the batch runner, but the exported policy + // must not treat a vacuous `every` as evidence to abort. + it('does NOT abort an empty batch', () => { + expect(markSyncedStopReason([], false)).toBeNull(); + }); + + // The core guard against stranding: once any record has synced, the request + // shape is proven valid, so a later whole-batch 4xx is per-record and must not + // abort the run. + it('does NOT abort a whole-rejected batch once a record has already synced', () => { + expect(markSyncedStopReason([MARK_ABORTED, MARK_ABORTED], true)).toBeNull(); + }); + + it('does NOT abort a mixed batch — a lone 4xx alongside a success is per-record', () => { + expect(markSyncedStopReason([MARK_SYNCED, MARK_ABORTED], true)).toBeNull(); + }); + + // Deliberately conservative: unanimity is required, so one transient blip (a + // 429/5xx surfaced as MARK_FAILED) among the rejections keeps the run going. + // Erring toward a few extra doomed requests is safer than aborting on what may + // be a lone per-record 4xx and stranding syncable records. + it('does NOT abort a batch of rejections mixed with a transient failure', () => { + expect( + markSyncedStopReason([MARK_ABORTED, MARK_FAILED], false), + ).toBeNull(); + }); + + it('does NOT abort when the batch failed but not as a request-shape 4xx', () => { + expect(markSyncedStopReason([MARK_FAILED, MARK_FAILED], false)).toBeNull(); + }); +}); + +describe('probeStopReason', () => { + // A probe is a DIFFERENT record than the rejected batch, so a lone reject here + // is decisive — it confirms the request shape itself is wrong. + it('confirms an abort when the probe is also rejected', () => { + expect(probeStopReason(MARK_ABORTED)).toBe(MARK_ABORTED); + }); + + it('stops as a timeout when the probe times out', () => { + expect(probeStopReason(MARK_TIMED_OUT)).toBe(MARK_TIMED_OUT); + }); + + // A surviving probe (synced) or an inconclusive one (transient failure) does + // not confirm a bad shape, so the run continues. + it('does not stop when the probe succeeds or fails transiently', () => { + expect(probeStopReason(MARK_SYNCED)).toBeNull(); + expect(probeStopReason(MARK_FAILED)).toBeNull(); + }); }); From 20c96fd051890d7d21770364225f38490ce73e61 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:44:01 -0500 Subject: [PATCH 2/4] Harden mark-synced abort: require two matching request-shape rejections Independent review flagged that aborting on the first (or any two) 400/422 chunks could strand trailing records if a chunk ever rejects for a per-record reason. Abort only when a SECOND chunk carries the SAME error message with nothing synced yet (envelope-level evidence), collapse the redundant abort/stoppedBy fields into one ChunkStop, and drop the 'not attempted' clause when the abort left nothing unattempted. --- src/index.ts | 19 ++++- src/libs/api.ts | 7 +- src/libs/records.ts | 147 ++++++++++++++++++++++-------------- tests/index.test.ts | 45 +++++++++++ tests/libs/records.test.ts | 151 ++++++++++++++++++++++++++++--------- 5 files changed, 269 insertions(+), 100 deletions(-) diff --git a/src/index.ts b/src/index.ts index e71820c..fbc079a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -437,18 +437,24 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { // 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. All three cases leave the +// 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, stoppedBy: MarkSyncedStop, + unattemptedCount: number, ): string { if (stoppedBy === 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 (stoppedBy === MARK_ABORTED) { - return `Aborted marking records synced — the server rejected the request wholesale (a 400/422), so every remaining record would fail the same way and the rest were not attempted; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`; + const notAttemptedClause = + 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.`; @@ -462,9 +468,12 @@ function reportMarkFailures( failures: WrittenRecord[], markedCount: number, stoppedBy: MarkSyncedStop, + unattemptedCount: number, spinner: Spinner, ): void { - spinner.error(markFailureHeadline(failures.length, stoppedBy)); + spinner.error( + markFailureHeadline(failures.length, stoppedBy, unattemptedCount), + ); 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 — @@ -534,10 +543,14 @@ 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, stoppedBy, + unattemptedCount, spinner, ); return; diff --git a/src/libs/api.ts b/src/libs/api.ts index a861810..1eea4e5 100644 --- a/src/libs/api.ts +++ b/src/libs/api.ts @@ -196,9 +196,10 @@ export const isSystemicApiFailure = ( // 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 whole batch agree, -// whether the request shape itself is wrong (see markSyncedStopReason). It does -// not itself mean "abort now" — a lone 400/422 can still be per-record. +// 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; diff --git a/src/libs/records.ts b/src/libs/records.ts index fc4e5ae..7b4ce38 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -38,8 +38,9 @@ const SYNCED_STATUS = 'synced'; // every remaining chunk. `MARK_ABORTED` — the chunk was rejected with a // request-shape 4xx (a malformed-payload 400 or a contract-validation 422, NOT // an auth 401/403 or a transient 429): the CLI builds every chunk's payload -// identically, so a shape the server rejects wholesale dooms every remaining -// chunk too — the run aborts rather than firing the same doomed request again. +// identically, so once a SECOND chunk is rejected the same way with nothing +// synced the run aborts rather than fire the same doomed request again (a lone +// rejection isn't enough — see `markRecordsSynced`). // // Values are prefixed (`mark-*`) so they never collide with the wire // `SYNCED_STATUS = 'synced'` above: these are internal outcome tags, not the @@ -465,18 +466,26 @@ const outcomesFromResponse = ( ); }; -// The result of PATCHing one chunk: a per-item outcome list plus whether the -// run should stop. `abort` is set when the failure dooms every remaining chunk -// too (a hung server, a systemic auth/rate-limit/5xx failure, or a request-shape -// 4xx the CLI builds identically for every chunk), so the caller stops rather -// than firing a burst it already knows will fail. `stoppedBy` names the abort -// reason the caller reports distinctly — a timeout (`MARK_TIMED_OUT`) or a -// request-shape rejection (`MARK_ABORTED`) — and is null for a systemic abort -// (reported as a plain mark failure) or when the chunk didn't abort. +// How a chunk ended, from the chunk's OWN perspective — the run-level decision to +// stop is `markRecordsSynced`'s, which also weighs prior chunks. `timeout` (hung +// server) and `systemic` (auth/rate-limit/5xx) each doom every remaining chunk, +// so the caller aborts immediately. `request-shape` (a 400/422) means the payload +// envelope looks wrong, but the caller only aborts once a SECOND chunk is rejected +// with the SAME error (see `markRecordsSynced`) rather than strand records behind +// a single, possibly isolated rejection. `null` is a clean chunk or a plain +// per-chunk failure the caller runs past. +type ChunkStop = 'timeout' | 'systemic' | 'request-shape' | null; + +// The result of PATCHing one chunk: a per-item outcome list, how the chunk ended, +// and (for a `request-shape` stop only) the server's error message. The caller +// compares that message across chunks so a categorical envelope rejection (the +// same message twice) aborts, while two different per-record rejections that only +// happen to both 4xx do not — it keeps running past those. Null for every other +// stop kind. type MarkSyncedChunkResult = { outcomes: MarkSyncedOutcome[]; - abort: boolean; - stoppedBy: MarkSyncedStop; + stop: ChunkStop; + message: string | null; }; // PATCHes one chunk (<= MAX_MARK_SYNCED_BATCH_SIZE records) synced in a single @@ -488,20 +497,21 @@ type MarkSyncedChunkResult = { // is non-critical post-write bookkeeping (the files are already on disk), so a // failed chunk simply leaves its records `pending` to re-sync next run. // -// A timeout maps every item to `MARK_TIMED_OUT` and aborts (a hung server would -// burn the full request timeout on every remaining chunk). A request-shape 4xx -// (a malformed-payload 400 or a contract-validation 422) maps every item to -// `MARK_ABORTED` and aborts too: the CLI builds every chunk's payload the same -// way, so a shape the server rejects wholesale would be rejected identically on -// every remaining chunk — retrying them just fires the same doomed request (the -// markpost bulk handler validates the whole envelope and 422s before any write; -// foreign uuids are dropped, not 4xx'd, so a 400/422 is never per-record here). -// A systemic failure (auth/rate-limit/5xx) also aborts — it will recur for every -// remaining chunk, so the caller backs off rather than hammering a server that -// just rejected the burst (the same rule the fetch helpers apply via -// `isSystemicApiFailure`) — but is reported as a plain failure, not `MARK_ABORTED`. -// Any other (per-chunk) failure maps to `MARK_FAILED` without aborting — a later -// chunk may still succeed. +// A timeout maps every item to `MARK_TIMED_OUT` and reports `stop: 'timeout'` +// (a hung server would burn the full request timeout on every remaining chunk). +// A request-shape 4xx (a malformed-payload 400 or a contract-validation 422) +// maps every item to `MARK_ABORTED` and reports `stop: 'request-shape'`: the CLI +// builds every chunk's payload the same way, so a shape the server rejects +// wholesale is likely wrong for every chunk — but the run only aborts once a +// SECOND chunk agrees (see `markRecordsSynced`), not on a lone rejection. A +// systemic failure (auth/rate-limit/5xx) reports `stop: 'systemic'` — it will +// recur for every remaining chunk, so the caller backs off rather than hammering +// a server that just rejected the burst (the same rule the fetch helpers apply +// via `isSystemicApiFailure`) — but stays `MARK_FAILED`, reported as a plain +// failure. Any other (per-chunk) failure maps to `MARK_FAILED` with `stop: null` +// — a later chunk may still succeed. A 4xx delivered as an HTML error page (a +// WAF/proxy interstitial) throws unparseable before it can be classified, so it +// degrades to that plain failure rather than aborting on a misread status. const markSyncedChunk = async ( items: MarkSyncedItem[], syncedAt: string, @@ -526,8 +536,8 @@ const markSyncedChunk = async ( return { outcomes: outcomesFromResponse(items, body), - abort: false, - stoppedBy: null, + stop: null, + message: null, }; } catch (error) { // Identify the chunk by its uuid range so a stderr reader can tell which @@ -540,37 +550,26 @@ const markSyncedChunk = async ( error instanceof Error ? error.message : String(error), ); - // A timeout aborts every remaining chunk so the run doesn't burn the full - // request timeout on each. if (error instanceof ApiTimeoutError) { return { outcomes: items.map(() => MARK_TIMED_OUT), - abort: true, - stoppedBy: MARK_TIMED_OUT, + stop: 'timeout', + message: null, }; } - // A request-shape 4xx (a malformed-payload 400 or a contract-validation 422) - // means the payload envelope the CLI built is wrong. Every remaining chunk is - // built identically via `buildBulkRecordPayload`, so they'd all be rejected - // the same way — abort rather than fire the same doomed request again, and - // tag the items `MARK_ABORTED` so the run reports the categorical cause. An - // auth 401/403, a transient 429, and a 5xx stay out of this (they're systemic - // below); a 4xx delivered as an HTML error page (a WAF/proxy interstitial) - // throws unparseable before it can be classified, so it degrades to the - // plain per-chunk failure below rather than aborting on a misread status. if (isFatalRequestError(error)) { return { outcomes: items.map(() => MARK_ABORTED), - abort: true, - stoppedBy: MARK_ABORTED, + stop: 'request-shape', + message: error.message, }; } return { outcomes: items.map(() => MARK_FAILED), - abort: isSystemicApiFailure(error), - stoppedBy: null, + stop: isSystemicApiFailure(error) ? 'systemic' : null, + message: null, }; } }; @@ -590,20 +589,28 @@ const markSyncedChunk = async ( // is already few enough that firing them serially keeps the burst small without // a concurrency limiter. // -// Returns one outcome per record in input order. A timeout, a systemic failure -// (auth/rate-limit/5xx), or a request-shape 4xx (a 400/422 rejected wholesale) -// stops the run at that chunk rather than firing a burst that's already doomed; -// the trailing records get no outcome and stay `pending` (their outcome index is -// `undefined`, which the caller reads as not-synced). A plain per-chunk failure -// doesn't abort — a later chunk may still succeed. `stoppedBy` names the -// distinctly-reported stop reason (`MARK_TIMED_OUT` or `MARK_ABORTED`) or is null -// when the run finished or stopped on a systemic failure, so the caller can word -// its report accordingly. +// Returns one outcome per record in input order. A timeout or a systemic failure +// (auth/rate-limit/5xx) stops the run at that chunk rather than firing a burst +// that's already doomed. A request-shape 4xx (a 400/422) stops the run only once +// a SECOND chunk is rejected with the SAME error message and nothing has synced +// yet: the CLI builds every chunk's payload identically, so two chunks failing +// the same categorical way is strong evidence the envelope shape itself is wrong. +// A lone rejection, two rejections with DIFFERENT messages (which look like two +// isolated per-record problems, not one envelope fault), or any rejection after a +// success (a success proves the shape valid) all keep the run going rather than +// strand syncable records behind an unconfirmed abort. On any stop the trailing +// records get no outcome and stay `pending` (their outcome index is `undefined`, +// which the caller reads as not-synced). A plain per-chunk failure doesn't abort — +// a later chunk may still succeed. `stoppedBy` names the distinctly-reported stop +// reason (`MARK_TIMED_OUT` or `MARK_ABORTED`) or is null when the run finished or +// stopped on a systemic failure, so the caller can word its report accordingly. export const markRecordsSynced = async ( items: MarkSyncedItem[], syncedAt: string = new Date().toISOString(), ): Promise => { const outcomes: MarkSyncedOutcome[] = []; + let anySynced = false; + let lastRequestShapeMessage: string | null = null; for ( let start = 0; @@ -611,13 +618,37 @@ export const markRecordsSynced = async ( start += MAX_MARK_SYNCED_BATCH_SIZE ) { const chunk = items.slice(start, start + MAX_MARK_SYNCED_BATCH_SIZE); - const chunkResult = await markSyncedChunk(chunk, syncedAt); + const { + outcomes: chunkOutcomes, + stop, + message, + } = await markSyncedChunk(chunk, syncedAt); + + outcomes.push(...chunkOutcomes); + anySynced = anySynced || chunkOutcomes.includes(MARK_SYNCED); - outcomes.push(...chunkResult.outcomes); + if (stop === 'timeout') { + return { outcomes, stoppedBy: MARK_TIMED_OUT }; + } - if (chunkResult.abort) { - return { outcomes, stoppedBy: chunkResult.stoppedBy }; + if (stop === 'systemic') { + return { outcomes, stoppedBy: null }; } + + if (stop !== 'request-shape') { + continue; + } + + // Abort only once a SECOND consecutive request-shape rejection carries the + // SAME message, and only while nothing has synced — matching messages across + // two independently-built chunks is what marks the failure as envelope-level + // (categorical) rather than two isolated per-record rejections, and a success + // would have proven the shape valid. + if (!anySynced && message !== null && message === lastRequestShapeMessage) { + return { outcomes, stoppedBy: MARK_ABORTED }; + } + + lastRequestShapeMessage = message; } return { outcomes, stoppedBy: null }; diff --git a/tests/index.test.ts b/tests/index.test.ts index 1a47cd5..8eea11e 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1398,6 +1398,51 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); + it('omits the not-attempted clause when the abort left nothing unattempted', async () => { + const records: Record[] = Array.from({ length: 2 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordsSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // Both records have an outcome (the abort landed on the last chunk), so there + // is no un-attempted tail — the headline must not claim "the rest were not + // attempted." + vi.mocked(markRecordsSynced).mockResolvedValue({ + outcomes: [MARK_ABORTED, MARK_ABORTED], + stoppedBy: MARK_ABORTED, + }); + + await import('@/index.js'); + + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Aborted marking records synced'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('the rest were not attempted'), + ); + expect(process.exitCode).toBe(1); + }); + it('does not use the timeout wording for a plain (non-timeout) failure', async () => { const records: Record[] = Array.from({ length: 4 }, (_item, index) => ({ uuid: `uuid-${index}`, diff --git a/tests/libs/records.test.ts b/tests/libs/records.test.ts index 9671a52..34be9b7 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -1560,32 +1560,96 @@ describe('markRecordsSynced', () => { expect(result.outcomes).toEqual([MARK_FAILED]); }); + // Reject every chunk the given way (a 400/422 error response), so a test can + // drive the two-chunk request-shape confirmation without hand-writing the mock. + const mockAllChunksReject = (status: number, detail: string) => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Rejected', detail }] }, + }), + }); + }; + // A request-shape 4xx (a contract-validation 422 or a malformed-payload 400) // means the payload envelope the CLI built is wrong. Every chunk is built the - // same way, so a rejected chunk dooms every remaining chunk too — abort with - // MARK_ABORTED rather than fire the same doomed request again. markpost's bulk - // handler validates the whole envelope and 422s before any write (foreign - // uuids are dropped, not 4xx'd), so a 400/422 is never per-record here. - it('aborts remaining chunks on a 422, marking the rejected chunk MARK_ABORTED', async () => { - // Chunk 1 succeeds; chunk 2 is rejected with a 422. Chunk 3 must never be - // sent — the same envelope would be rejected identically. + // same way, so once a SECOND chunk is rejected the same way with nothing synced + // the run aborts with MARK_ABORTED rather than fire the same doomed request + // again — but a LONE rejection is not enough (a chunk could be an isolated + // reject), so the first fatal chunk keeps the run going. + it('aborts after two request-shape rejections with nothing synced', async () => { + // Chunks 1 and 2 both 422 with nothing synced — the second confirms the shape + // is wrong, so chunk 3 must never be sent. + mockAllChunksReject(422, 'Unknown attribute: filePath'); + + const result = await markRecordsSynced(items(250)); + // Only two requests fire — the third chunk is never attempted. + expect(global.fetch).toHaveBeenCalledTimes(2); + expect(result.stoppedBy).toBe(MARK_ABORTED); + // Both attempted chunks are MARK_ABORTED (200); chunk 3 has no outcome. + expect(result.outcomes).toHaveLength(200); + expect(result.outcomes.every((outcome) => outcome === MARK_ABORTED)).toBe( + true, + ); + }); + + it('aborts after two malformed-payload 400 rejections with nothing synced', async () => { + mockAllChunksReject(400, 'Invalid body'); + + const result = await markRecordsSynced(items(250)); + expect(global.fetch).toHaveBeenCalledTimes(2); + expect(result.stoppedBy).toBe(MARK_ABORTED); + expect(result.outcomes).toHaveLength(200); + expect(result.outcomes.every((outcome) => outcome === MARK_ABORTED)).toBe( + true, + ); + }); + + // Two 4xx rejections with DIFFERENT messages look like two isolated per-record + // problems, not one envelope fault — so the run must keep going rather than + // abort. Only a repeated, identical categorical error is strong enough evidence. + it('does not abort on two request-shape rejections with different messages', async () => { + let callCount = 0; + global.fetch = vi.fn().mockImplementation(() => { + callCount += 1; + return Promise.resolve({ + ok: false, + status: 422, + json: () => + Promise.resolve({ + data: { + errors: [{ title: 'Rejected', detail: `problem ${callCount}` }], + }, + }), + }); + }); + + const result = await markRecordsSynced(items(250)); + // Messages differ per chunk, so no abort — all three chunks are attempted. + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(result.stoppedBy).toBeNull(); + expect(result.outcomes).toHaveLength(250); + expect(result.outcomes.every((outcome) => outcome === MARK_ABORTED)).toBe( + true, + ); + }); + + // A single request-shape rejection is not enough to abort — the next chunk + // still fires, and if it succeeds the shape is proven valid. Guards against + // stranding syncable records behind one isolated rejection. + it('does not abort on a lone request-shape rejection — the next chunk still fires', async () => { let callCount = 0; global.fetch = vi.fn().mockImplementation((_url, init) => { callCount += 1; - if (callCount === 2) { + if (callCount === 1) { return Promise.resolve({ ok: false, status: 422, json: () => Promise.resolve({ - data: { - errors: [ - { - title: 'Invalid Attribute', - detail: 'Unknown attribute: filePath', - }, - ], - }, + data: { errors: [{ title: 'Rejected', detail: 'nope' }] }, }), }); } @@ -1594,33 +1658,48 @@ describe('markRecordsSynced', () => { }); const result = await markRecordsSynced(items(250)); - // Only two requests fire — the third chunk is never attempted. - expect(global.fetch).toHaveBeenCalledTimes(2); - expect(result.stoppedBy).toBe(MARK_ABORTED); - // Chunk 1 synced (100), chunk 2 all aborted (100); chunk 3 has no outcome. - expect(result.outcomes).toHaveLength(200); + // All three chunks are attempted — one rejection doesn't abort. + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(result.stoppedBy).toBeNull(); expect( - result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_SYNCED), + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_ABORTED), ).toBe(true); expect( - result.outcomes - .slice(100, 200) - .every((outcome) => outcome === MARK_ABORTED), + result.outcomes.slice(100).every((outcome) => outcome === MARK_SYNCED), ).toBe(true); }); - it('aborts with MARK_ABORTED on a malformed-payload 400', async () => { - global.fetch = vi.fn().mockResolvedValue({ - ok: false, - status: 400, - json: () => - Promise.resolve({ - data: { errors: [{ title: 'Bad Request', detail: 'Invalid body' }] }, - }), + // Once a chunk has synced, the envelope is proven valid, so a later 400/422 is + // an isolated per-chunk rejection, not proof the shape is wrong — the run must + // NOT abort even if two later chunks are rejected the same way. + it('does not abort on request-shape rejections once a chunk has synced', async () => { + let callCount = 0; + global.fetch = vi.fn().mockImplementation((_url, init) => { + callCount += 1; + if (callCount >= 2) { + return Promise.resolve({ + ok: false, + status: 422, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Rejected', detail: 'nope' }] }, + }), + }); + } + + return echoBulkPatch(init); }); - const result = await markRecordsSynced(items(2)); - expect(result.outcomes).toEqual([MARK_ABORTED, MARK_ABORTED]); - expect(result.stoppedBy).toBe(MARK_ABORTED); + + const result = await markRecordsSynced(items(250)); + // Chunk 1 synced, so chunks 2 and 3 both fire despite being rejected. + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(result.stoppedBy).toBeNull(); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_SYNCED), + ).toBe(true); + expect( + result.outcomes.slice(100).every((outcome) => outcome === MARK_ABORTED), + ).toBe(true); }); // A 404 is neither a request-shape 4xx nor systemic, so it stays a plain chunk From 6db9ee7dc97339889a21b7829a4225eee2bc2297 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:50:06 -0500 Subject: [PATCH 3/4] Review round 3: consecutive same-message abort, honest outcome tags - Require the two request-shape rejections to be CONSECUTIVE (reset the tracker on any non-request-shape chunk) so an interleaved failure can't confirm a stale match. - Tag request-shape chunks MARK_FAILED and re-tag only the stopping chunk MARK_ABORTED, so a completed run never leaves a stray MARK_ABORTED. - Name the ChunkStop discriminants (STOP_*) and bundle the failure-report reason+unattempted count into one MarkStopReport object. --- src/index.ts | 28 +++++++----- src/libs/records.ts | 90 +++++++++++++++++++++++++------------- tests/libs/records.test.ts | 66 ++++++++++++++++++++++------ 3 files changed, 129 insertions(+), 55 deletions(-) diff --git a/src/index.ts b/src/index.ts index fbc079a..9bf2da3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -435,6 +435,15 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { })); } +// 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 @@ -444,16 +453,15 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { // listed records pending on the server. function markFailureHeadline( pendingCount: number, - stoppedBy: MarkSyncedStop, - unattemptedCount: number, + stop: MarkStopReport, ): string { - if (stoppedBy === MARK_TIMED_OUT) { + 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 (stoppedBy === MARK_ABORTED) { + if (stop.reason === MARK_ABORTED) { const notAttemptedClause = - unattemptedCount > 0 ? ' and the rest were not attempted' : ''; + 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.`; } @@ -467,13 +475,10 @@ function markFailureHeadline( function reportMarkFailures( failures: WrittenRecord[], markedCount: number, - stoppedBy: MarkSyncedStop, - unattemptedCount: number, + stop: MarkStopReport, spinner: Spinner, ): void { - spinner.error( - markFailureHeadline(failures.length, stoppedBy, unattemptedCount), - ); + 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 — @@ -549,8 +554,7 @@ async function markWrittenRecordsSynced( reportMarkFailures( pending, writtenRecords.length - pending.length, - stoppedBy, - unattemptedCount, + { reason: stoppedBy, unattemptedCount }, spinner, ); return; diff --git a/src/libs/records.ts b/src/libs/records.ts index 7b4ce38..0505e4e 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -467,14 +467,20 @@ const outcomesFromResponse = ( }; // How a chunk ended, from the chunk's OWN perspective — the run-level decision to -// stop is `markRecordsSynced`'s, which also weighs prior chunks. `timeout` (hung -// server) and `systemic` (auth/rate-limit/5xx) each doom every remaining chunk, -// so the caller aborts immediately. `request-shape` (a 400/422) means the payload -// envelope looks wrong, but the caller only aborts once a SECOND chunk is rejected -// with the SAME error (see `markRecordsSynced`) rather than strand records behind -// a single, possibly isolated rejection. `null` is a clean chunk or a plain -// per-chunk failure the caller runs past. -type ChunkStop = 'timeout' | 'systemic' | 'request-shape' | null; +// stop is `markRecordsSynced`'s, which also weighs prior chunks. `STOP_TIMEOUT` +// (hung server) and `STOP_SYSTEMIC` (auth/rate-limit/5xx) each doom every +// remaining chunk, so the caller aborts immediately. `STOP_REQUEST_SHAPE` (a +// 400/422) means the payload envelope looks wrong, but the caller only aborts +// once a SECOND consecutive chunk is rejected with the SAME error (see +// `markRecordsSynced`) rather than strand records behind a single, possibly +// isolated rejection. `null` is a clean chunk or a plain per-chunk failure the +// caller runs past. Named constants (not bare literals) so the discriminant a +// third function might compare against can't silently drift on a typo. +const STOP_TIMEOUT = 'timeout'; +const STOP_SYSTEMIC = 'systemic'; +const STOP_REQUEST_SHAPE = 'request-shape'; +type ChunkStop = + typeof STOP_TIMEOUT | typeof STOP_SYSTEMIC | typeof STOP_REQUEST_SHAPE | null; // The result of PATCHing one chunk: a per-item outcome list, how the chunk ended, // and (for a `request-shape` stop only) the server's error message. The caller @@ -497,19 +503,19 @@ type MarkSyncedChunkResult = { // is non-critical post-write bookkeeping (the files are already on disk), so a // failed chunk simply leaves its records `pending` to re-sync next run. // -// A timeout maps every item to `MARK_TIMED_OUT` and reports `stop: 'timeout'` -// (a hung server would burn the full request timeout on every remaining chunk). -// A request-shape 4xx (a malformed-payload 400 or a contract-validation 422) -// maps every item to `MARK_ABORTED` and reports `stop: 'request-shape'`: the CLI -// builds every chunk's payload the same way, so a shape the server rejects -// wholesale is likely wrong for every chunk — but the run only aborts once a -// SECOND chunk agrees (see `markRecordsSynced`), not on a lone rejection. A -// systemic failure (auth/rate-limit/5xx) reports `stop: 'systemic'` — it will -// recur for every remaining chunk, so the caller backs off rather than hammering -// a server that just rejected the burst (the same rule the fetch helpers apply -// via `isSystemicApiFailure`) — but stays `MARK_FAILED`, reported as a plain -// failure. Any other (per-chunk) failure maps to `MARK_FAILED` with `stop: null` -// — a later chunk may still succeed. A 4xx delivered as an HTML error page (a +// A timeout maps every item to `MARK_TIMED_OUT` and reports `STOP_TIMEOUT` (a +// hung server would burn the full request timeout on every remaining chunk). A +// request-shape 4xx (a malformed-payload 400 or a contract-validation 422) maps +// every item to `MARK_FAILED` and reports `STOP_REQUEST_SHAPE` plus the server's +// error message: the chunk was attempted and rejected, so its records are a plain +// failure UNLESS the run actually aborts — `markRecordsSynced` re-tags only the +// chunk it stops on to `MARK_ABORTED`, so a completed run never leaves a stray +// `MARK_ABORTED`. A systemic failure (auth/rate-limit/5xx) reports `STOP_SYSTEMIC` +// — it will recur for every remaining chunk, so the caller backs off rather than +// hammering a server that just rejected the burst (the same rule the fetch helpers +// apply via `isSystemicApiFailure`) — but stays `MARK_FAILED`, reported as a plain +// failure. Any other (per-chunk) failure maps to `MARK_FAILED` with `stop: null` — +// a later chunk may still succeed. A 4xx delivered as an HTML error page (a // WAF/proxy interstitial) throws unparseable before it can be classified, so it // degrades to that plain failure rather than aborting on a misread status. const markSyncedChunk = async ( @@ -553,27 +559,41 @@ const markSyncedChunk = async ( if (error instanceof ApiTimeoutError) { return { outcomes: items.map(() => MARK_TIMED_OUT), - stop: 'timeout', + stop: STOP_TIMEOUT, message: null, }; } if (isFatalRequestError(error)) { return { - outcomes: items.map(() => MARK_ABORTED), - stop: 'request-shape', + outcomes: items.map(() => MARK_FAILED), + stop: STOP_REQUEST_SHAPE, message: error.message, }; } return { outcomes: items.map(() => MARK_FAILED), - stop: isSystemicApiFailure(error) ? 'systemic' : null, + stop: isSystemicApiFailure(error) ? STOP_SYSTEMIC : null, message: null, }; } }; +// Re-tag the final `count` outcomes as `MARK_ABORTED` — the chunk whose repeated +// request-shape rejection actually stopped the run. Returns a new array so the +// caller stays free of in-place mutation; earlier outcomes are untouched. +const withAbortedTail = ( + outcomes: MarkSyncedOutcome[], + count: number, +): MarkSyncedOutcome[] => { + const firstAbortedIndex = outcomes.length - count; + + return outcomes.map((outcome, index) => + index >= firstAbortedIndex ? MARK_ABORTED : outcome, + ); +}; + // Marks written records synced after the CLI has written them to disk, via // markpost's bulk PATCH /api/records (server/api/records/index.patch.ts). This // is the non-destructive counterpart to `deleteRecords`: with autoDelete off, @@ -627,15 +647,19 @@ export const markRecordsSynced = async ( outcomes.push(...chunkOutcomes); anySynced = anySynced || chunkOutcomes.includes(MARK_SYNCED); - if (stop === 'timeout') { + if (stop === STOP_TIMEOUT) { return { outcomes, stoppedBy: MARK_TIMED_OUT }; } - if (stop === 'systemic') { + if (stop === STOP_SYSTEMIC) { return { outcomes, stoppedBy: null }; } - if (stop !== 'request-shape') { + if (stop !== STOP_REQUEST_SHAPE) { + // Reset so the match below stays CONSECUTIVE: a clean or plain-failure + // chunk between two identical rejections breaks the "envelope is wrong" + // evidence, so it must not count toward the two-in-a-row abort. + lastRequestShapeMessage = null; continue; } @@ -643,9 +667,15 @@ export const markRecordsSynced = async ( // SAME message, and only while nothing has synced — matching messages across // two independently-built chunks is what marks the failure as envelope-level // (categorical) rather than two isolated per-record rejections, and a success - // would have proven the shape valid. + // would have proven the shape valid. Re-tag this stopping chunk's records + // `MARK_ABORTED` (they were `MARK_FAILED` until now) so the outcome reflects + // that the run stopped here, while the earlier chunks it ran past stay + // `MARK_FAILED`. if (!anySynced && message !== null && message === lastRequestShapeMessage) { - return { outcomes, stoppedBy: MARK_ABORTED }; + return { + outcomes: withAbortedTail(outcomes, chunkOutcomes.length), + stoppedBy: MARK_ABORTED, + }; } lastRequestShapeMessage = message; diff --git a/tests/libs/records.test.ts b/tests/libs/records.test.ts index 34be9b7..84e4989 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -1588,11 +1588,14 @@ describe('markRecordsSynced', () => { // Only two requests fire — the third chunk is never attempted. expect(global.fetch).toHaveBeenCalledTimes(2); expect(result.stoppedBy).toBe(MARK_ABORTED); - // Both attempted chunks are MARK_ABORTED (200); chunk 3 has no outcome. + // Chunk 1 was run past (MARK_FAILED); only the stopping chunk 2 is MARK_ABORTED. expect(result.outcomes).toHaveLength(200); - expect(result.outcomes.every((outcome) => outcome === MARK_ABORTED)).toBe( - true, - ); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), + ).toBe(true); + expect( + result.outcomes.slice(100, 200).every((outcome) => outcome === MARK_ABORTED), + ).toBe(true); }); it('aborts after two malformed-payload 400 rejections with nothing synced', async () => { @@ -1602,9 +1605,12 @@ describe('markRecordsSynced', () => { expect(global.fetch).toHaveBeenCalledTimes(2); expect(result.stoppedBy).toBe(MARK_ABORTED); expect(result.outcomes).toHaveLength(200); - expect(result.outcomes.every((outcome) => outcome === MARK_ABORTED)).toBe( - true, - ); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), + ).toBe(true); + expect( + result.outcomes.slice(100, 200).every((outcome) => outcome === MARK_ABORTED), + ).toBe(true); }); // Two 4xx rejections with DIFFERENT messages look like two isolated per-record @@ -1627,11 +1633,43 @@ describe('markRecordsSynced', () => { }); const result = await markRecordsSynced(items(250)); - // Messages differ per chunk, so no abort — all three chunks are attempted. + // Messages differ per chunk, so no abort — all three chunks are attempted and + // rejected as plain per-chunk failures. expect(global.fetch).toHaveBeenCalledTimes(3); expect(result.stoppedBy).toBeNull(); expect(result.outcomes).toHaveLength(250); - expect(result.outcomes.every((outcome) => outcome === MARK_ABORTED)).toBe( + expect(result.outcomes.every((outcome) => outcome === MARK_FAILED)).toBe( + true, + ); + }); + + // Two identical rejections with a non-request-shape chunk BETWEEN them are not + // consecutive, so they don't confirm an envelope fault — the run keeps going. + it('does not abort on two matching rejections split by a plain failure', async () => { + let callCount = 0; + global.fetch = vi.fn().mockImplementation(() => { + callCount += 1; + if (callCount === 2) { + return Promise.reject(new Error('Network error')); + } + + return Promise.resolve({ + ok: false, + status: 422, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Rejected', detail: 'nope' }] }, + }), + }); + }); + + const result = await markRecordsSynced(items(250)); + // Chunk 2 (network error) resets the consecutiveness, so chunk 3 doesn't + // confirm chunk 1 — all three fire and nothing aborts. + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(result.stoppedBy).toBeNull(); + expect(result.outcomes).toHaveLength(250); + expect(result.outcomes.every((outcome) => outcome === MARK_FAILED)).toBe( true, ); }); @@ -1658,11 +1696,12 @@ describe('markRecordsSynced', () => { }); const result = await markRecordsSynced(items(250)); - // All three chunks are attempted — one rejection doesn't abort. + // All three chunks are attempted — one rejection doesn't abort, and the run + // completes, so chunk 1 stays a plain MARK_FAILED (never MARK_ABORTED). expect(global.fetch).toHaveBeenCalledTimes(3); expect(result.stoppedBy).toBeNull(); expect( - result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_ABORTED), + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), ).toBe(true); expect( result.outcomes.slice(100).every((outcome) => outcome === MARK_SYNCED), @@ -1691,14 +1730,15 @@ describe('markRecordsSynced', () => { }); const result = await markRecordsSynced(items(250)); - // Chunk 1 synced, so chunks 2 and 3 both fire despite being rejected. + // Chunk 1 synced, so chunks 2 and 3 both fire despite being rejected, and the + // run completes — the rejected chunks stay plain MARK_FAILED. expect(global.fetch).toHaveBeenCalledTimes(3); expect(result.stoppedBy).toBeNull(); expect( result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_SYNCED), ).toBe(true); expect( - result.outcomes.slice(100).every((outcome) => outcome === MARK_ABORTED), + result.outcomes.slice(100).every((outcome) => outcome === MARK_FAILED), ).toBe(true); }); From f7eab45eb15ecb4cf65fb2c77f6f8bfc4d81bb15 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:54:14 -0500 Subject: [PATCH 4/4] Review round 4: surface never-attempted count on a systemic early stop - The generic failure headline now notes how many pending records were never sent when a systemic stop ended the run early, instead of implying all N failed. - Hoist the shared MARK_FAILED outcome list in markSyncedChunk. --- src/index.ts | 9 ++++++++- src/libs/records.ts | 9 +++++++-- tests/index.test.ts | 45 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 9bf2da3..0b55fa4 100644 --- a/src/index.ts +++ b/src/index.ts @@ -465,7 +465,14 @@ function markFailureHeadline( 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 diff --git a/src/libs/records.ts b/src/libs/records.ts index 0505e4e..d4cc120 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -564,16 +564,21 @@ const markSyncedChunk = async ( }; } + // A request-shape 4xx and any other (per-chunk/systemic) failure both leave + // the whole chunk pending, so they share this outcome list; only the stop + // classification differs. + const failedOutcomes: MarkSyncedOutcome[] = items.map(() => MARK_FAILED); + if (isFatalRequestError(error)) { return { - outcomes: items.map(() => MARK_FAILED), + outcomes: failedOutcomes, stop: STOP_REQUEST_SHAPE, message: error.message, }; } return { - outcomes: items.map(() => MARK_FAILED), + outcomes: failedOutcomes, stop: isSystemicApiFailure(error) ? STOP_SYSTEMIC : null, message: null, }; diff --git a/tests/index.test.ts b/tests/index.test.ts index 8eea11e..291e9d1 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1443,6 +1443,51 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); + it('notes never-attempted records when a systemic stop ends the run early', async () => { + const records: Record[] = Array.from({ length: 3 }, (_item, index) => ({ + uuid: `uuid-${index}`, + title: `Title ${index}`, + content: `Content ${index}`, + createdAt: '2024-01-01T00:00:00Z', + })); + const { fetchAllRecords, markRecordsSynced } = await import( + '@/libs/records.js' + ); + const { writeMarkdown } = await import('@/libs/markdown.js'); + const { fetchSettings } = await import('@/libs/settings.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + // A systemic abort (auth/rate-limit/5xx) has no distinct stop reason + // (stoppedBy null) but stops early: only uuid-0 was attempted, so uuid-1 and + // uuid-2 were never sent and the generic headline must say so. + vi.mocked(markRecordsSynced).mockResolvedValue({ + outcomes: [MARK_FAILED], + stoppedBy: null, + }); + + await import('@/index.js'); + + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 3 record(s) synced'), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('2 never attempted'), + ); + expect(process.exitCode).toBe(1); + }); + it('does not use the timeout wording for a plain (non-timeout) failure', async () => { const records: Record[] = Array.from({ length: 4 }, (_item, index) => ({ uuid: `uuid-${index}`,