diff --git a/src/index.ts b/src/index.ts index 901feb3..0b55fa4 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,8 +4,11 @@ import { deleteRecords, fetchAllRecords, markRecordsSynced, + MARK_ABORTED, MARK_SYNCED, + MARK_TIMED_OUT, MarkSyncedItem, + MarkSyncedStop, PENDING_STATUS, } from '@/libs/records.js'; import { describeApiError, isSystemicApiFailure } from '@/libs/api.js'; @@ -423,7 +426,7 @@ function reportDeferredServerChanges(deferredRecords: WrittenRecord[]): void { // Projects each written record down to the `{ uuid, filePath }` shape the bulk // mark-synced call needs, preserving order so the returned outcomes stay aligned -// to `writtenRecords` by index. Chunking and the stop-on-timeout abort live in +// to `writtenRecords` by index. Chunking and the stop-on-abort logic live in // `markRecordsSynced` (the records lib), keeping the API surface isolated there. function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { return writtenRecords.map(({ record, filePath }) => ({ @@ -432,16 +435,44 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { })); } -// Headline for the mark-synced failure report. A timeout abort reads -// differently from a scatter of per-record failures: it stopped the run early, -// so the count includes records never attempted. Both leave the listed records -// pending on the server. -function markFailureHeadline(pendingCount: number, timedOut: boolean): string { - if (timedOut) { - return `Timed out marking records synced — stopped after the batch that first timed out; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`; +// How a mark-synced run ended, for the failure report: the stop reason plus how +// many pending records the run never reached. Bundled because they're only ever +// meaningful together (the abort description), so the headline can't be handed a +// count that belongs to a different reason. +type MarkStopReport = { + reason: MarkSyncedStop; + unattemptedCount: number; +}; + +// Headline for the mark-synced failure report. An abort reads differently from a +// scatter of per-record failures: it stopped the run early, so the pending count +// can fold in records never attempted after the abort. `unattemptedCount` is how +// many of those pending records were never sent (the chunks after the stop), so +// the abort wording only claims "the rest were not attempted" when that's true — +// an abort on the final chunk leaves nothing unattempted. All cases leave the +// listed records pending on the server. +function markFailureHeadline( + pendingCount: number, + stop: MarkStopReport, +): string { + if (stop.reason === MARK_TIMED_OUT) { + return `Timed out marking records synced — stopped after the first timeout; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`; + } + + if (stop.reason === MARK_ABORTED) { + const notAttemptedClause = + stop.unattemptedCount > 0 ? ' and the rest were not attempted' : ''; + return `Aborted marking records synced — the server rejected the request wholesale (a 400/422), so every record would fail the same way${notAttemptedClause}; ${pendingCount} record(s) still pending on the server, they may be re-written next run.`; } - return `Failed to mark ${pendingCount} record(s) synced — written locally but still pending on the server; they may be re-written next run.`; + // The generic branch also covers a systemic abort (auth/rate-limit/5xx), which + // stops the run early with no distinct stop reason — so surface how many of the + // pending records were never sent rather than implying all N were attempted. + const neverAttemptedClause = + stop.unattemptedCount > 0 + ? ` (${stop.unattemptedCount} never attempted — the run stopped early)` + : ''; + return `Failed to mark ${pendingCount} record(s) synced — written locally but still pending on the server${neverAttemptedClause}; they may be re-written next run.`; } // Surfaces mark-synced failures loudly (never as success): an unmarked record @@ -451,10 +482,10 @@ function markFailureHeadline(pendingCount: number, timedOut: boolean): string { function reportMarkFailures( failures: WrittenRecord[], markedCount: number, - timedOut: boolean, + stop: MarkStopReport, spinner: Spinner, ): void { - spinner.error(markFailureHeadline(failures.length, timedOut)); + spinner.error(markFailureHeadline(failures.length, stop)); failures.forEach(({ record, filePath }) => { // Sanitize the composed line: record.uuid comes from the same untrusted API // response as a title, and filePath embeds the user-configured output path — @@ -502,14 +533,15 @@ async function markWrittenRecordsSynced( spinner.start('Marking records synced...'); - const { outcomes, timedOut } = await markRecordsSynced( + const { outcomes, stoppedBy } = await markRecordsSynced( toMarkSyncedItems(writtenRecords), ); // A record is settled only when its mark-synced outcome is MARK_SYNCED; it is // pending if its mark failed or was never attempted (its outcome is undefined - // because a timeout aborted the run before its batch). Evict settled records - // from the written-path map so a long-running autoSync daemon doesn't leak - // memory — the "settled" half of the written-vs-settled split. + // because an abort — a timeout, a systemic failure, or a request-shape 4xx — + // stopped the run before its chunk). Evict settled records from the written- + // path map so a long-running autoSync daemon doesn't leak memory — the + // "settled" half of the written-vs-settled split. const settled = writtenRecords.filter( (_written, index) => outcomes[index] === MARK_SYNCED, ); @@ -523,10 +555,13 @@ async function markWrittenRecordsSynced( ); if (pending.length > 0) { + // Records the run never reached: an abort/timeout stops before later chunks, + // so their outcome index is undefined and they have no per-record outcome. + const unattemptedCount = writtenRecords.length - outcomes.length; reportMarkFailures( pending, writtenRecords.length - pending.length, - timedOut, + { reason: stoppedBy, unattemptedCount }, spinner, ); return; diff --git a/src/libs/api.ts b/src/libs/api.ts index 446bec7..1eea4e5 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,15 @@ export const isSystemicApiFailure = ( ): error is ApiRequestError => error instanceof ApiRequestError && error.isSystemic; +// Narrowing guard: true only for a request-shape `ApiRequestError` (a 400/422 +// rejection — NOT a per-record 404, an auth 401/403, or a transient 429). Lets a +// bulk caller TAG the outcome so it can decide, after seeing a SECOND chunk agree +// with nothing synced, whether the request shape itself is wrong (see +// `markRecordsSynced`). It does not itself mean "abort now" — a lone 400/422 can +// still be an isolated rejection. +export const isFatalRequestError = (error: unknown): error is ApiRequestError => + error instanceof ApiRequestError && error.isFatalRequest; + // Labels the failure by kind so the classification stays inside the API layer // instead of leaking status-code logic into command code. Falls back to a // generic label if handed a non-systemic error, so a mislabel can't happen. diff --git a/src/libs/records.ts b/src/libs/records.ts index 1bf8ec0..d4cc120 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -1,6 +1,7 @@ import { ApiTimeoutError, authedRequest, + isFatalRequestError, isSystemicApiFailure, logApiFailure, unwrapResourceAttributes, @@ -28,12 +29,18 @@ const SYNCED_STATUS = 'synced'; // Per-record result of a bulk mark-synced run (see `markRecordsSynced`, which // PATCHes records in chunks of up to MAX_MARK_SYNCED_BATCH_SIZE). `MARK_SYNCED` // — the server returned this record among the ones it updated. `MARK_FAILED` — -// the record's chunk failed (a non-timeout error), or the server didn't return -// this uuid (partial success); it stays pending to re-sync next run. A plain -// chunk failure doesn't stop the run, but a systemic one (auth/rate-limit/5xx) -// aborts the remaining chunks — see `markSyncedChunk`. `MARK_TIMED_OUT` — the -// record's chunk hit the request timeout, a signal the server is hung; the run -// stops there rather than paying the full timeout on every remaining chunk. +// the record's chunk failed (a non-timeout, non-request-shape error), or the +// server didn't return this uuid (partial success); it stays pending to re-sync +// next run. A plain chunk failure doesn't stop the run, but a systemic one +// (auth/rate-limit/5xx) aborts the remaining chunks — see `markSyncedChunk`. +// `MARK_TIMED_OUT` — the record's chunk hit the request timeout, a signal the +// server is hung; the run stops there rather than paying the full timeout on +// 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 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 @@ -42,9 +49,17 @@ 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; // markpost paginates with a cursor: each response's `links.next` embeds the // `page[after]` cursor to request the following page, and is `null` once @@ -381,15 +396,17 @@ export type MarkSyncedItem = { // Outcome of a whole bulk mark-synced run. `outcomes` holds one entry per // record in the ORIGINAL input order, so the caller can align each result back -// to its record by index. On an abort (a timeout OR a systemic auth/rate-limit/ -// 5xx failure) it's shorter than the input: the chunk that aborted and every -// chunk after it were never confirmed, so those trailing records have no outcome -// and the caller treats them as still pending. `timedOut` is true only when a -// timeout (not a systemic failure) caused the abort, so the caller can word its -// report accordingly. +// to its record by index. On an abort (a timeout, a systemic auth/rate-limit/ +// 5xx failure, or a request-shape 4xx) it's shorter than the input: the chunk +// that aborted and every chunk after it were never confirmed, so those trailing +// records have no outcome and the caller treats them as still pending. +// `stoppedBy` records which distinctly-reported reason ended the run early — a +// timeout (`MARK_TIMED_OUT`) or a wholesale request-shape rejection +// (`MARK_ABORTED`) — or null when the run finished or stopped on a systemic +// failure (reported as a plain mark failure), so the caller can word its report. export type MarkSyncedResult = { outcomes: MarkSyncedOutcome[]; - timedOut: boolean; + stoppedBy: MarkSyncedStop; }; // The `records[]` item markpost's bulk PATCH expects: the uuid to match plus @@ -449,16 +466,32 @@ 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 or a systemic auth/rate-limit/5xx failure), so the caller -// stops rather than firing a burst it already knows will fail. `timedOut` -// distinguishes the hung-server case from a systemic one so the caller can word -// its report accordingly. +// How a chunk ended, from the chunk's OWN perspective — the run-level decision to +// 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 +// 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; - timedOut: boolean; + stop: ChunkStop; + message: string | null; }; // PATCHes one chunk (<= MAX_MARK_SYNCED_BATCH_SIZE records) synced in a single @@ -470,13 +503,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 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`). 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_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 ( items: MarkSyncedItem[], syncedAt: string, @@ -501,8 +542,8 @@ const markSyncedChunk = async ( return { outcomes: outcomesFromResponse(items, body), - abort: false, - timedOut: false, + stop: null, + message: null, }; } catch (error) { // Identify the chunk by its uuid range so a stderr reader can tell which @@ -518,19 +559,46 @@ const markSyncedChunk = async ( if (error instanceof ApiTimeoutError) { return { outcomes: items.map(() => MARK_TIMED_OUT), - abort: true, - timedOut: true, + stop: STOP_TIMEOUT, + message: null, + }; + } + + // 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: failedOutcomes, + stop: STOP_REQUEST_SHAPE, + message: error.message, }; } return { - outcomes: items.map(() => MARK_FAILED), - abort: isSystemicApiFailure(error), - timedOut: false, + outcomes: failedOutcomes, + 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, @@ -546,18 +614,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 or a systemic -// failure (auth/rate-limit/5xx) 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. `timedOut` is true only when a timeout (not a systemic failure) -// caused the stop, 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; @@ -565,16 +643,50 @@ 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(...chunkResult.outcomes); + outcomes.push(...chunkOutcomes); + anySynced = anySynced || chunkOutcomes.includes(MARK_SYNCED); - if (chunkResult.abort) { - return { outcomes, timedOut: chunkResult.timedOut }; + if (stop === STOP_TIMEOUT) { + return { outcomes, stoppedBy: MARK_TIMED_OUT }; } + + if (stop === STOP_SYSTEMIC) { + return { outcomes, stoppedBy: null }; + } + + 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; + } + + // 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. 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: withAbortedTail(outcomes, chunkOutcomes.length), + stoppedBy: MARK_ABORTED, + }; + } + + lastRequestShapeMessage = message; } - return { outcomes, timedOut: false }; + return { outcomes, stoppedBy: null }; }; export const fetchRecord = async (uuid: string): Promise => { diff --git a/tests/index.test.ts b/tests/index.test.ts index 6e5fe9a..291e9d1 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -5,6 +5,7 @@ import { Record } from '@/types/records.types.js'; import { UserSettings, ConflictStrategy } from '@/types/settings.types.js'; import { SettingsReadResult } from '@/libs/settings.js'; import { + MARK_ABORTED, MARK_FAILED, MARK_SYNCED, MARK_TIMED_OUT, @@ -81,16 +82,16 @@ const mockRecord: Record = { // (chunk boundaries and the timeout abort are covered in tests/libs/records.test.ts). // `markResultBy` maps each item's uuid to an outcome; `markResultAll` is the // common "every record shares one outcome" shorthand. Both always report every -// record attempted (`timedOut: false`, full-length outcomes) — an abort produces +// record attempted (`stoppedBy: null`, full-length outcomes) — an abort produces // a SHORTER outcomes array, so timeout/abort cases use an explicit -// `mockResolvedValue({ outcomes: [...], timedOut: true })` instead of these. +// `mockResolvedValue({ outcomes: [...], stoppedBy: MARK_TIMED_OUT })` instead. const markResultBy = (outcomeFor: (uuid: string) => MarkSyncedOutcome) => async ( items: { uuid: string; filePath: string }[], ): Promise => ({ outcomes: items.map((item) => outcomeFor(item.uuid)), - timedOut: false, + stoppedBy: null, }); const markResultAll = (outcome: MarkSyncedOutcome) => @@ -1310,7 +1311,7 @@ describe('index', () => { // outcome). The short outcomes array models the real timeout abort. vi.mocked(markRecordsSynced).mockResolvedValue({ outcomes: [MARK_SYNCED, MARK_TIMED_OUT], - timedOut: true, + stoppedBy: MARK_TIMED_OUT, }); await import('@/index.js'); @@ -1334,6 +1335,159 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); + it('uses the abort wording when a request-shape 4xx stops the run', 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`, + ); + // uuid-0 synced, uuid-1's chunk was rejected as a request-shape 4xx (abort), + // uuid-2 never attempted (no outcome) — the short outcomes array models the + // real abort, and stoppedBy drives the abort-specific headline. + vi.mocked(markRecordsSynced).mockResolvedValue({ + outcomes: [MARK_SYNCED, MARK_ABORTED], + stoppedBy: MARK_ABORTED, + }); + + await import('@/index.js'); + + // Rejected uuid-1 plus the never-attempted uuid-2 = two pending. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('2 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-1 -> /mock/output/uuid-1.md'), + ); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('! uuid-2 -> /mock/output/uuid-2.md'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 1 record(s) synced despite'), + ); + 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('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}`, @@ -1414,7 +1568,7 @@ describe('index', () => { // only the timed-out one is pending — no never-attempted tail. vi.mocked(markRecordsSynced).mockResolvedValue({ outcomes: [MARK_SYNCED, MARK_TIMED_OUT], - timedOut: true, + stoppedBy: MARK_TIMED_OUT, }); await import('@/index.js'); @@ -1461,7 +1615,7 @@ describe('index', () => { // All three non-synced records are pending and the run uses timeout wording. vi.mocked(markRecordsSynced).mockResolvedValue({ outcomes: [MARK_SYNCED, MARK_FAILED, MARK_TIMED_OUT], - timedOut: true, + stoppedBy: MARK_TIMED_OUT, }); await import('@/index.js'); 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 3f1606d..84e4989 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -7,6 +7,7 @@ import { fetchPaginatedRecords, fetchRecord, markRecordsSynced, + MARK_ABORTED, MARK_FAILED, MARK_SYNCED, MARK_TIMED_OUT, @@ -1304,7 +1305,7 @@ describe('markRecordsSynced', () => { global.fetch = vi.fn(); const result = await markRecordsSynced([]); expect(global.fetch).not.toHaveBeenCalled(); - expect(result).toEqual({ outcomes: [], timedOut: false }); + expect(result).toEqual({ outcomes: [], stoppedBy: null }); }); it('marks every record synced in a single request at exactly the batch size', async () => { @@ -1317,7 +1318,7 @@ describe('markRecordsSynced', () => { expect(result.outcomes.every((outcome) => outcome === MARK_SYNCED)).toBe( true, ); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); it('splits one-over-the-batch-size into two requests (ceil(N/100))', async () => { @@ -1344,7 +1345,7 @@ describe('markRecordsSynced', () => { true, ); expect(result.outcomes).toHaveLength(250); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); it('pairs each record its own uuid, filePath, and syncedAt across chunks', async () => { @@ -1387,7 +1388,7 @@ describe('markRecordsSynced', () => { MARK_SYNCED, MARK_SYNCED, ]); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); it('aligns a per-record failure to its index when it lands in a later chunk', async () => { @@ -1408,7 +1409,7 @@ describe('markRecordsSynced', () => { index === 120 ? outcome === MARK_FAILED : outcome === MARK_SYNCED, ), ).toBe(true); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); it('aborts remaining chunks on a timeout and marks the timed-out chunk pending', async () => { @@ -1427,7 +1428,7 @@ 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.timedOut).toBe(true); + expect(result.stoppedBy).toBe(MARK_TIMED_OUT); // Chunk 1 synced (100), chunk 2 all timed out (100); chunk 3 has no outcome. expect(result.outcomes).toHaveLength(200); expect( @@ -1456,7 +1457,7 @@ describe('markRecordsSynced', () => { const result = await markRecordsSynced(items(250)); // All three chunks are attempted — no abort. expect(global.fetch).toHaveBeenCalledTimes(3); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); expect(result.outcomes).toHaveLength(250); expect( result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), @@ -1475,7 +1476,7 @@ describe('markRecordsSynced', () => { mockFetch({ data: [], meta: { updated: 0 } }); const result = await markRecordsSynced(items(3)); expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED, MARK_FAILED]); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); // Off-contract safety net: the declared contract always sends `data` as an @@ -1485,7 +1486,7 @@ describe('markRecordsSynced', () => { mockFetch({ meta: { updated: 3 } }); const result = await markRecordsSynced(items(3)); expect(result.outcomes).toEqual([MARK_SYNCED, MARK_SYNCED, MARK_SYNCED]); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); // `data: null` is off-contract (markpost always sends the updated array), so @@ -1526,7 +1527,7 @@ describe('markRecordsSynced', () => { // Only the first chunk is attempted — the other two are never sent. expect(global.fetch).toHaveBeenCalledTimes(1); // A systemic abort is not a timeout, so the caller uses the failure wording. - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); // The attempted chunk is all pending; the unsent chunks have no outcome. expect(result.outcomes).toHaveLength(100); expect(result.outcomes.every((outcome) => outcome === MARK_FAILED)).toBe( @@ -1541,7 +1542,7 @@ describe('markRecordsSynced', () => { ); const result = await markRecordsSynced(items(3)); expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED, MARK_FAILED]); - expect(result.timedOut).toBe(false); + expect(result.stoppedBy).toBeNull(); }); it('marks a whole chunk MARK_FAILED on a network failure', async () => { @@ -1558,4 +1559,274 @@ describe('markRecordsSynced', () => { const result = await markRecordsSynced(items(1)); 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 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); + // Chunk 1 was run past (MARK_FAILED); only the stopping chunk 2 is MARK_ABORTED. + expect(result.outcomes).toHaveLength(200); + 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 () => { + 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.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 + // 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 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_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, + ); + }); + + // 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 === 1) { + return Promise.resolve({ + ok: false, + status: 422, + json: () => + Promise.resolve({ + data: { errors: [{ title: 'Rejected', detail: 'nope' }] }, + }), + }); + } + + return echoBulkPatch(init); + }); + + const result = await markRecordsSynced(items(250)); + // 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_FAILED), + ).toBe(true); + expect( + result.outcomes.slice(100).every((outcome) => outcome === MARK_SYNCED), + ).toBe(true); + }); + + // 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(250)); + // 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_FAILED), + ).toBe(true); + }); + + // A 404 is neither a request-shape 4xx nor systemic, so it stays a plain chunk + // failure that does NOT abort — a later chunk may still succeed. Guards the + // fatal-request abort against widening beyond 400/422. + it('does not abort on a 404 — the chunk is MARK_FAILED and the run continues', async () => { + let callCount = 0; + global.fetch = vi.fn().mockImplementation((_url, init) => { + callCount += 1; + if (callCount === 1) { + return Promise.resolve({ + ok: false, + status: 404, + json: () => + Promise.resolve({ + data: { + errors: [{ title: 'Not Found', detail: 'No such record' }], + }, + }), + }); + } + + return echoBulkPatch(init); + }); + + const result = await markRecordsSynced(items(250)); + // All three chunks are attempted — a 404 is not a categorical abort. + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(result.stoppedBy).toBeNull(); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), + ).toBe(true); + expect( + result.outcomes.slice(100).every((outcome) => outcome === MARK_SYNCED), + ).toBe(true); + }); + + // A 429 is systemic (rate-limit): it aborts the run to back off, but is NOT a + // request-shape rejection — it maps to MARK_FAILED with a null stop reason (the + // plain-failure wording), never MARK_ABORTED. Guards the fatal-request abort + // against catching a transient rate-limit. + it('treats a 429 as a systemic abort (MARK_FAILED, not MARK_ABORTED)', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 429, + json: () => + Promise.resolve({ + data: { + errors: [{ title: 'Too Many Requests', detail: 'Slow down' }], + }, + }), + }); + + const result = await markRecordsSynced(items(250)); + // Systemic abort after the first chunk — the other two never fire. + expect(global.fetch).toHaveBeenCalledTimes(1); + expect(result.stoppedBy).toBeNull(); + expect(result.outcomes).toHaveLength(100); + expect(result.outcomes.every((outcome) => outcome === MARK_FAILED)).toBe( + true, + ); + }); + + // 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('does not abort on a 422 whose body is not JSON — the chunk is MARK_FAILED', async () => { + let callCount = 0; + global.fetch = vi.fn().mockImplementation((_url, init) => { + callCount += 1; + if (callCount === 1) { + return Promise.resolve({ + ok: false, + status: 422, + json: () => Promise.reject(new SyntaxError('Unexpected token <')), + }); + } + + return echoBulkPatch(init); + }); + + const result = await markRecordsSynced(items(250)); + // The unparseable body degrades to a plain failure, so the run continues. + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(result.stoppedBy).toBeNull(); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), + ).toBe(true); + }); });