diff --git a/src/index.ts b/src/index.ts index 901feb3..3a612d3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,9 +6,14 @@ import { markRecordsSynced, MARK_SYNCED, MarkSyncedItem, + MarkAbortReason, PENDING_STATUS, } from '@/libs/records.js'; -import { describeApiError, isSystemicApiFailure } from '@/libs/api.js'; +import { + describeApiError, + isPermanentApiFailure, + isSystemicApiFailure, +} from '@/libs/api.js'; import { buildWritePreview, ensureOutputDirectory, @@ -423,7 +428,8 @@ 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 (timeout or +// systemic failure, with a permanent one also stopping the daemon) live in // `markRecordsSynced` (the records lib), keeping the API surface isolated there. function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { return writtenRecords.map(({ record, filePath }) => ({ @@ -432,15 +438,50 @@ 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) { +// Headline for the mark-synced failure report. A permanent, timeout, or transient +// abort all stop the run early (so the count can include records never attempted) +// and each reads differently from a scatter of per-record failures. Every +// daemon-aware clause is derived from the SAME (`abortReason`, `autoSyncEnabled`) +// the caller's returned stop signal uses, so the message can't claim a stop — or a +// "next run" — that won't happen (a one-shot `markpost sync` never had a daemon; +// mirroring the delete path, which says nothing about auto-sync). `hasUnattempted` +// is whether the abort left a trailing chunk unsent, so the wording only claims +// records were skipped when some actually were. +function markFailureHeadline( + pendingCount: number, + abortReason: MarkAbortReason, + { + autoSyncEnabled, + hasUnattempted, + }: { + autoSyncEnabled: boolean; + hasUnattempted: boolean; + }, +): string { + if (abortReason === 'permanent') { + const daemonClause = autoSyncEnabled ? ', so auto-sync was stopped;' : ';'; + // Note the unattempted tail like the transient branch does — the abort left + // later chunks unsent, so the count is more than just the records that failed. + const skipped = hasUnattempted ? ' (some were never attempted)' : ''; + // Don't prescribe `markpost config` — a 403 (plan limit / sign-ups disabled) + // isn't a token problem. markRecordsSynced already logged the failing chunk's + // case-specific reason to stderr; point the user at that. + return `Failed to mark ${pendingCount} record(s) synced${skipped} — a permanent error (authentication or a forbidden account) will recur every pass${daemonClause} the record(s) remain pending on the server. Fix the cause reported above and sync again.`; + } + + if (abortReason === 'timeout') { 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.`; } + if (abortReason === 'transient') { + // A systemic error (rate limit / 5xx) aborted the run to back off. Only claim + // records were skipped if a trailing chunk was actually unsent; the "re-written + // next run" hedge matches the timeout/generic branches (soft — true whenever + // the user next syncs, daemon or not), so it isn't gated on autoSyncEnabled. + const skipped = hasUnattempted ? ', so some were never attempted' : ''; + return `Failed to mark ${pendingCount} record(s) synced — a systemic error stopped the run early${skipped}; they remain 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.`; } @@ -451,10 +492,10 @@ function markFailureHeadline(pendingCount: number, timedOut: boolean): string { function reportMarkFailures( failures: WrittenRecord[], markedCount: number, - timedOut: boolean, + headline: string, spinner: Spinner, ): void { - spinner.error(markFailureHeadline(failures.length, timedOut)); + spinner.error(headline); 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 — @@ -490,26 +531,33 @@ function forgetSettledRecords( // Marks every written record synced on the server after a write, so the next // run's pending-only fetch skips them — the autoDelete-off path's -// non-destructive equivalent of the delete step. +// non-destructive equivalent of the delete step. Returns whether the autoSync +// daemon should stop: true only when a permanent failure (dead token / forbidden +// account) struck AND a daemon was running (`autoSyncEnabled`), so the caller can +// break the loop into the same doomed PATCH every pass — the mark-synced +// counterpart of the delete path's `deletePermanentlyFailed`. async function markWrittenRecordsSynced( writtenRecords: WrittenRecord[], spinner: Spinner, writtenState: Map, -): Promise { + { autoSyncEnabled }: { autoSyncEnabled: boolean }, +): Promise { if (writtenRecords.length === 0) { - return; + return false; } spinner.start('Marking records synced...'); - const { outcomes, timedOut } = await markRecordsSynced( + const { outcomes, abortReason } = await markRecordsSynced( toMarkSyncedItems(writtenRecords), ); + const permanentlyFailed = abortReason === 'permanent'; // 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 a timeout or systemic failure 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, ); @@ -522,17 +570,38 @@ async function markWrittenRecordsSynced( settled.map(({ record }) => record.uuid), ); + // The daemon-stop signal the caller returns. The headline derives its + // daemon-aware clauses from the same (abortReason, autoSyncEnabled), so the + // message can never claim a stop — or a "next run" — that won't happen. + const stoppingAutoSync = permanentlyFailed && autoSyncEnabled; + if (pending.length > 0) { + // An abort leaves a trailing chunk unsent, so `outcomes` is shorter than the + // input — the headline uses this to only claim records were skipped when some + // actually were. + const hasUnattempted = outcomes.length < writtenRecords.length; + // Compose the headline here — this function holds the abort reason and the + // daemon state — so reportMarkFailures takes a ready string instead of + // drilling several adjacent, transposable args. + const headline = markFailureHeadline(pending.length, abortReason, { + autoSyncEnabled, + hasUnattempted, + }); reportMarkFailures( pending, writtenRecords.length - pending.length, - timedOut, + headline, spinner, ); - return; + return stoppingAutoSync; } spinner.success(`Marked ${writtenRecords.length} records synced!`); + // Return the same stop signal from both exits so the daemon-stop decision has a + // single source. With zero pending this is always false (a permanent abort maps + // its chunk to MARK_FAILED, so it can't reach here), but deriving it rather than + // hardcoding keeps the two paths from drifting. + return stoppingAutoSync; } // Ends a truncated sync on the truncation warning, never on a green success @@ -867,13 +936,18 @@ async function runDefaultSync(dryRun = false): Promise { // next run's pending-only fetch skips them instead of re-writing duplicate // files. if (!autoDelete) { - await markWrittenRecordsSynced( + const markStopsAutoSync = await markWrittenRecordsSynced( settleableRecords, spinner, processWrittenState, + { autoSyncEnabled: autoSync }, ); reportIncompleteSync(recordsResult.partial); - return autoSync; + // A permanent mark-synced failure (dead token / forbidden account) recurs + // every pass, so — like the delete path — stop the autoSync daemon instead + // of rescheduling into the same doomed PATCH; a transient one keeps + // autoSync alive to retry next pass. + return markStopsAutoSync ? false : autoSync; } // Delete Records — skipped when nothing is settleable (a bare DELETE with an @@ -904,8 +978,7 @@ async function runDefaultSync(dryRun = false): Promise { const deleteMeta = await deleteRecords( settleableRecords.map(({ record }) => record.uuid), ).catch((error: unknown) => { - deletePermanentlyFailed = - isSystemicApiFailure(error) && error.isPermanent; + deletePermanentlyFailed = isPermanentApiFailure(error); // Sanitize before printing, same threat as the outer catch: a // server- or API-derived message can embed an escape. console.error( @@ -964,7 +1037,9 @@ async function runDefaultSync(dryRun = false): Promise { // A permanent failure (dead token, forbidden account) won't clear on // retry — stop the autoSync daemon. A transient one (rate-limit/5xx) is // worth another pass, so keep autoSync alive to retry ("retry shortly"). - return error.isPermanent ? false : autoSync; + // Go through the shared guard (like the mark-synced and delete paths) so + // the permanence rule stays in the API seam, not re-derived here. + return isPermanentApiFailure(error) ? false : autoSync; } spinner.error('Something went wrong!'); diff --git a/src/libs/api.ts b/src/libs/api.ts index 446bec7..0be3aae 100644 --- a/src/libs/api.ts +++ b/src/libs/api.ts @@ -176,6 +176,16 @@ export const isSystemicApiFailure = ( ): error is ApiRequestError => error instanceof ApiRequestError && error.isSystemic; +// Narrowing guard: true only for a PERMANENT failure (a dead token / forbidden +// account) that won't clear on a blind retry. Keeps the permanence rule inside +// the API seam so callers deciding whether to stop an autoSync daemon (the +// sync's mark-synced and delete paths) don't re-derive it. `isPermanent` already +// implies systemic, so no separate systemic check is needed. +export const isPermanentApiFailure = ( + error: unknown, +): error is ApiRequestError => + error instanceof ApiRequestError && error.isPermanent; + // 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..bf20f43 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -1,6 +1,7 @@ import { ApiTimeoutError, authedRequest, + isPermanentApiFailure, isSystemicApiFailure, logApiFailure, unwrapResourceAttributes, @@ -35,6 +36,13 @@ const SYNCED_STATUS = 'synced'; // 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. // +// Whether an abort ALSO stops the autoSync daemon is a run-level decision, not a +// per-record tag: a permanent systemic failure (dead token / forbidden account: +// 401/403) recurs every pass, so the run reports `abortReason: 'permanent'` (see +// `MarkAbortReason`) and the caller shuts the daemon down. A transient systemic +// failure (429/5xx) still aborts the remaining chunks but keeps the daemon alive +// to retry next pass. Either way the affected records stay `MARK_FAILED`. +// // Values are prefixed (`mark-*`) so they never collide with the wire // `SYNCED_STATUS = 'synced'` above: these are internal outcome tags, not the // status string sent to the server, and an accidental cross-comparison should @@ -379,17 +387,27 @@ export type MarkSyncedItem = { filePath: string; }; +// Why a mark-synced run stopped early, or `null` if it ran every chunk. One +// discriminant (not adjacent booleans) so the reason can't be self-contradictory. +// `'timeout'` — a chunk hit the request timeout (hung server), retried next pass. +// `'permanent'` — a chunk hit a permanent systemic failure (dead token / forbidden +// account: 401/403) that recurs every pass, so the caller ALSO stops the autoSync +// daemon. `'transient'` — a chunk hit a transient systemic failure (429/5xx): the +// run stops early to back off (trailing chunks skipped) but the daemon stays alive +// to retry, and the caller can say the run stopped short. `null` — the run +// completed every chunk (any failures were per-chunk, not systemic). +export type MarkAbortReason = 'timeout' | 'permanent' | 'transient' | null; + // 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. +// and the caller treats them as still pending. `abortReason` records why (if) the +// run stopped early, and in particular whether the caller should stop the daemon. export type MarkSyncedResult = { outcomes: MarkSyncedOutcome[]; - timedOut: boolean; + abortReason: MarkAbortReason; }; // The `records[]` item markpost's bulk PATCH expects: the uuid to match plus @@ -449,16 +467,17 @@ 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. +// The result of PATCHing one chunk: a per-item outcome list plus whether (and +// why) the run should stop. A non-null `abortReason` IS the abort signal (the +// run stops on it, backing off rather than firing a burst it knows will fail) +// and carries WHY: +// `'timeout'` and `'permanent'` distinguish the hung-server and dead-token cases +// (the latter also stops the daemon), and `'transient'` a systemic 429/5xx (aborts +// this run, daemon lives). `null` is the plain success/per-chunk-failure case that +// doesn't abort. One field, so "aborted" and "why" can never disagree. type MarkSyncedChunkResult = { outcomes: MarkSyncedOutcome[]; - abort: boolean; - timedOut: boolean; + abortReason: MarkAbortReason; }; // PATCHes one chunk (<= MAX_MARK_SYNCED_BATCH_SIZE records) synced in a single @@ -474,9 +493,14 @@ type MarkSyncedChunkResult = { // 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. +// burst (the same rule the fetch helpers apply via `isSystemicApiFailure`). A +// PERMANENT systemic failure (dead token / forbidden account: 401/403) aborts +// with `abortReason: 'permanent'` so the caller additionally stops the autoSync +// daemon, which can't clear it on retry (matching the delete path); a transient +// systemic failure aborts with `'transient'` (the run stops to back off, but the +// daemon lives). Any other (per-chunk) failure maps to `MARK_FAILED` without +// aborting (`abortReason: null`) — a later chunk may still +// succeed. const markSyncedChunk = async ( items: MarkSyncedItem[], syncedAt: string, @@ -501,8 +525,7 @@ const markSyncedChunk = async ( return { outcomes: outcomesFromResponse(items, body), - abort: false, - timedOut: false, + abortReason: null, }; } catch (error) { // Identify the chunk by its uuid range so a stderr reader can tell which @@ -515,19 +538,35 @@ const markSyncedChunk = async ( error instanceof Error ? error.message : String(error), ); + // A timeout gets its own outcome and aborts so the caller doesn't pay the + // full request timeout on every remaining chunk. if (error instanceof ApiTimeoutError) { return { outcomes: items.map(() => MARK_TIMED_OUT), - abort: true, - timedOut: true, + abortReason: 'timeout', }; } - return { - outcomes: items.map(() => MARK_FAILED), - abort: isSystemicApiFailure(error), - timedOut: false, - }; + // Every failed record in the chunk stays `MARK_FAILED` (pending, retried next + // run). A non-null `abortReason` is what stops the run, so classification is + // the single source of the abort decision: a PERMANENT failure (dead token / + // forbidden account: 401/403) reports `'permanent'` and also stops the daemon; + // a transient systemic failure (rate-limit/5xx that may be a blip) reports + // `'transient'` to back off — a sustained 429 stops after the first chunk + // rather than firing the whole burst — but keeps the daemon alive; a plain + // per-chunk failure is `null` and doesn't abort, since a later chunk may + // still succeed. + const failedOutcomes: MarkSyncedOutcome[] = items.map(() => MARK_FAILED); + + if (isPermanentApiFailure(error)) { + return { outcomes: failedOutcomes, abortReason: 'permanent' }; + } + + if (isSystemicApiFailure(error)) { + return { outcomes: failedOutcomes, abortReason: 'transient' }; + } + + return { outcomes: failedOutcomes, abortReason: null }; } }; @@ -551,8 +590,8 @@ const markSyncedChunk = async ( // 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. +// succeed. `abortReason` carries why (if) the run stopped early, and whether the +// caller should also stop the autoSync daemon (see `MarkAbortReason`). export const markRecordsSynced = async ( items: MarkSyncedItem[], syncedAt: string = new Date().toISOString(), @@ -569,12 +608,14 @@ export const markRecordsSynced = async ( outcomes.push(...chunkResult.outcomes); - if (chunkResult.abort) { - return { outcomes, timedOut: chunkResult.timedOut }; + // A non-null reason is the abort signal: stop here and surface why, leaving + // the trailing chunks unsent (their records get no outcome, read as pending). + if (chunkResult.abortReason !== null) { + return { outcomes, abortReason: chunkResult.abortReason }; } } - return { outcomes, timedOut: false }; + return { outcomes, abortReason: null }; }; export const fetchRecord = async (uuid: string): Promise => { diff --git a/tests/index.test.ts b/tests/index.test.ts index 6e5fe9a..be5dbe6 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -81,16 +81,17 @@ 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 -// a SHORTER outcomes array, so timeout/abort cases use an explicit -// `mockResolvedValue({ outcomes: [...], timedOut: true })` instead of these. +// record attempted (`abortReason: null`, full-length outcomes) — an abort produces +// a SHORTER outcomes array, so timeout/permanent-abort cases use an explicit +// `mockResolvedValue({ outcomes: [...], abortReason: 'timeout' | 'permanent' })` +// instead of these. const markResultBy = (outcomeFor: (uuid: string) => MarkSyncedOutcome) => async ( items: { uuid: string; filePath: string }[], ): Promise => ({ outcomes: items.map((item) => outcomeFor(item.uuid)), - timedOut: false, + abortReason: 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, + abortReason: 'timeout', }); await import('@/index.js'); @@ -1414,7 +1415,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, + abortReason: 'timeout', }); await import('@/index.js'); @@ -1461,7 +1462,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, + abortReason: 'timeout', }); await import('@/index.js'); @@ -1767,6 +1768,249 @@ describe('index', () => { expect(scheduledAutoSync).toBe(true); }); + // Drives a mark-sync (autoDelete off) of `count` records where the bulk + // `markRecordsSynced` resolves the given `result`, capturing what the run + // reports back to the scheduler. The chunking + abort itself lives in the + // records lib (covered in tests/libs/records.test.ts); here we pin index's own + // job — turning a `MarkSyncedResult` into the settle/report and the daemon-stop + // decision. A `result` with a SHORTER `outcomes` array than `count` models the + // real abort, where trailing chunks were never sent. + const arrangeMarkSync = async ({ + count, + result, + autoSync, + }: { + count: number; + result: MarkSyncedResult; + autoSync: boolean; + }): Promise<{ scheduledAutoSync: boolean | undefined }> => { + const records: Record[] = Array.from({ length: count }, (_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 { runSyncWithAutoSchedule } = await import('@/libs/scheduler.js'); + const { default: yoctoSpinner } = await import('yocto-spinner'); + + const capture: { scheduledAutoSync: boolean | undefined } = { + scheduledAutoSync: undefined, + }; + vi.mocked(runSyncWithAutoSchedule).mockImplementationOnce( + async (runSync) => { + capture.scheduledAutoSync = await runSync(); + }, + ); + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false, autoSync }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records, + partial: false, + }); + vi.mocked(writeMarkdown).mockImplementation( + (record: Record) => `/mock/output/${record.uuid}.md`, + ); + vi.mocked(markRecordsSynced).mockResolvedValue(result); + + return capture; + }; + + // The mark-synced counterpart of the delete-permanent-failure test: with + // autoDelete off, a permanent mark-synced failure (dead token / forbidden + // account) surfaces as `abortReason: 'permanent'`. The sync must fail loud AND + // stop rescheduling the autoSync daemon — otherwise it wakes every few minutes + // and re-PATCHes the same records against a server it already knows will + // reject, re-writing them as duplicates every pass (issue #133). + it('stops autoSync from rescheduling on a permanent mark-synced failure', async () => { + const capture = await arrangeMarkSync({ + count: 1, + autoSync: true, + result: { outcomes: [MARK_FAILED], abortReason: 'permanent' }, + }); + + await import('@/index.js'); + + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('auto-sync was stopped'), + ); + expect(process.exitCode).toBe(1); + // A permanent failure recurs, so the scheduler must not spin another pass. + expect(capture.scheduledAutoSync).toBe(false); + }); + + // A per-chunk NON-systemic failure (a per-record MARK_FAILED inside an + // otherwise-successful response, or a non-systemic 4xx) is `abortReason: null`: + // it doesn't abort and must keep autoSync alive — the record is still pending + // and the next pass retries. The systemic-blip (5xx/429) case now maps to + // `'transient'`, covered by the test below. + it('keeps autoSync alive after a per-chunk (non-systemic) mark-synced failure', async () => { + const capture = await arrangeMarkSync({ + count: 1, + autoSync: true, + result: { outcomes: [MARK_FAILED], abortReason: null }, + }); + + await import('@/index.js'); + + // Match text unique to the generic headline — 'still pending on the server' + // alone also appears in the timeout headline, so it wouldn't catch a + // failure wrongly routed through the timeout branch. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('written locally but still pending on the server'), + ); + expect(process.exitCode).toBe(1); + // A non-aborting failure: the daemon must retry, so autoSync is preserved. + expect(capture.scheduledAutoSync).toBe(true); + }); + + // The other side of the discriminant: a TIMEOUT abort must NOT stop the daemon + // — the server may un-hang, so the next pass should retry. Guards against a + // regression that treats any abort reason as a stop. The short outcomes array + // (2 outcomes for 3 records) models the real abort leaving a trailing chunk + // unsent. + it('keeps autoSync alive on a timeout abort', async () => { + const capture = await arrangeMarkSync({ + count: 3, + autoSync: true, + result: { + outcomes: [MARK_SYNCED, MARK_TIMED_OUT], + abortReason: 'timeout', + }, + }); + + await import('@/index.js'); + + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Timed out marking records synced'), + ); + expect(process.exitCode).toBe(1); + // A timeout can clear, so the daemon must retry next pass. + expect(capture.scheduledAutoSync).toBe(true); + }); + + // A TRANSIENT systemic abort (a 429/5xx that stopped the run to back off) must + // read differently from a scatter of per-record failures — some records were + // never attempted — yet keep the daemon alive to retry. The short outcomes + // array models the aborted run leaving a trailing chunk unsent. + it('reports a transient abort as stopped-early and keeps autoSync alive', async () => { + const capture = await arrangeMarkSync({ + count: 3, + autoSync: true, + result: { outcomes: [MARK_SYNCED, MARK_FAILED], abortReason: 'transient' }, + }); + + await import('@/index.js'); + + // uuid-1 failed plus uuid-2 never attempted = two pending, with the + // stopped-early wording (not the generic per-record failure line). + const headline = vi + .mocked(mockSpinner.error) + .mock.calls.map(([message]) => String(message)) + .find((message) => message.includes('a systemic error stopped the run early')); + expect(headline).toBeDefined(); + expect(headline).toContain('2 record(s)'); + // A trailing chunk was unsent, so the "never attempted" clause appears. + expect(headline).toContain('so some were never attempted'); + expect(process.exitCode).toBe(1); + // A transient error can clear, so the daemon must retry next pass. + expect(capture.scheduledAutoSync).toBe(true); + }); + + // The transient headline's "never attempted" clause is conditional: when the + // abort lands on the LAST chunk (outcomes length == count, no unattempted tail), + // it must NOT claim records were skipped — the same false-claim guard the + // permanent branch has for its daemon clause. + it('does not claim records were skipped on a transient abort with no unattempted tail', async () => { + await arrangeMarkSync({ + count: 2, + autoSync: true, + result: { outcomes: [MARK_SYNCED, MARK_FAILED], abortReason: 'transient' }, + }); + + await import('@/index.js'); + + const headline = vi + .mocked(mockSpinner.error) + .mock.calls.map(([message]) => String(message)) + .find((message) => message.includes('a systemic error stopped the run early')); + expect(headline).toBeDefined(); + // Only uuid-1 is pending, and it was attempted — no skipped tail to claim. + expect(headline).toContain('1 record(s)'); + expect(headline).not.toContain('never attempted'); + expect(process.exitCode).toBe(1); + }); + + // A permanent mark-synced failure on a plain one-shot `markpost sync` + // (autoDelete off, autoSync off) must NOT claim "auto-sync was stopped" — there + // was no daemon. The headline still guides the user to fix the cause, but a + // cron log must not read a false statement about what the tool did. + it('does not claim auto-sync was stopped when it was never on', async () => { + await arrangeMarkSync({ + count: 1, + autoSync: false, + result: { outcomes: [MARK_FAILED], abortReason: 'permanent' }, + }); + + await import('@/index.js'); + + const headline = vi + .mocked(mockSpinner.error) + .mock.calls.map(([message]) => String(message)) + .find((message) => message.includes('a permanent error')); + expect(headline).toBeDefined(); + expect(headline).not.toContain('auto-sync was stopped'); + expect(process.exitCode).toBe(1); + }); + + // Guards index's `outcomes[index]` alignment when a permanent abort leaves a + // trailing tail unattended. The bulk call is mocked to return a deliberately + // short outcomes array (20 entries for 25 records) standing in for an abort — + // chunk boundaries themselves live in tests/libs/records.test.ts — with uuid-13 + // failed inside it. index must count uuid-13 plus the five never-attempted + // (uuid-20..24) as six pending, list uuid-13 (not a settled record like + // uuid-0), report 19 marked, and stop the daemon. An off-by-one in the settle + // filter would strand the wrong records. + it('counts pending correctly and stops the daemon on a permanent abort', async () => { + const outcomes: MarkSyncedOutcome[] = Array.from( + { length: 20 }, + (_item, index) => (index === 13 ? MARK_FAILED : MARK_SYNCED), + ); + const capture = await arrangeMarkSync({ + count: 25, + autoSync: true, + result: { outcomes, abortReason: 'permanent' }, + }); + + await import('@/index.js'); + + // uuid-13 failed plus the five never-attempted (uuid-20..24) = six pending. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 6 record(s) synced'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 19 record(s) synced despite the above.'), + ); + // The failed record is listed pending; a settled record (uuid-0) is not — + // proving the outcome/index alignment holds when the outcomes array is + // truncated by an abort. + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('! uuid-13 -> /mock/output/uuid-13.md'), + ); + expect(console.error).not.toHaveBeenCalledWith( + expect.stringContaining('! uuid-0 -> /mock/output/uuid-0.md'), + ); + expect(capture.scheduledAutoSync).toBe(false); + expect(process.exitCode).toBe(1); + }); + // Tests 1 and 2 share the same arrange: two records where mockRecord's write // throws and the second (def-456) succeeds. Extracted per rule of three so // the two assertions read on their own. The write outcome is keyed off the diff --git a/tests/libs/api.test.ts b/tests/libs/api.test.ts index fa310bb..4723d09 100644 --- a/tests/libs/api.test.ts +++ b/tests/libs/api.test.ts @@ -12,6 +12,7 @@ import { formatErrorMessages, getApiToken, getBaseUrl, + isPermanentApiFailure, isSystemicApiFailure, logApiFailure, rethrowIfTimeout, @@ -449,6 +450,32 @@ describe('isSystemicApiFailure', () => { }); }); +describe('isPermanentApiFailure', () => { + // Gates the autoSync daemon shutdown, so its two boundaries matter: a + // permanent 401/403 is true; a systemic-but-transient 429/5xx is false (the + // daemon retries those); and anything that isn't a systemic ApiRequestError is + // false so the caller keeps its per-item handling. + it('is true only for a permanent (auth) ApiRequestError', () => { + expect(isPermanentApiFailure(new ApiRequestError('nope', 401))).toBe(true); + expect(isPermanentApiFailure(new ApiRequestError('nope', 403))).toBe(true); + }); + + it('is false for a transient systemic ApiRequestError (429/5xx)', () => { + expect(isPermanentApiFailure(new ApiRequestError('nope', 429))).toBe(false); + expect(isPermanentApiFailure(new ApiRequestError('nope', 503))).toBe(false); + }); + + it('is false for a non-permanent 4xx ApiRequestError', () => { + expect(isPermanentApiFailure(new ApiRequestError('nope', 422))).toBe(false); + }); + + it('is false for a plain Error or non-error value', () => { + expect(isPermanentApiFailure(new Error('network down'))).toBe(false); + expect(isPermanentApiFailure('boom')).toBe(false); + expect(isPermanentApiFailure(undefined)).toBe(false); + }); +}); + describe('describeSystemicFailure', () => { it('labels an auth failure with its status and message', () => { const error = new ApiRequestError('Invalid or missing token', 401); diff --git a/tests/libs/records.test.ts b/tests/libs/records.test.ts index 3f1606d..f982e77 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -1304,7 +1304,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: [], abortReason: null }); }); it('marks every record synced in a single request at exactly the batch size', async () => { @@ -1317,7 +1317,7 @@ describe('markRecordsSynced', () => { expect(result.outcomes.every((outcome) => outcome === MARK_SYNCED)).toBe( true, ); - expect(result.timedOut).toBe(false); + expect(result.abortReason).toBe(null); }); it('splits one-over-the-batch-size into two requests (ceil(N/100))', async () => { @@ -1344,7 +1344,7 @@ describe('markRecordsSynced', () => { true, ); expect(result.outcomes).toHaveLength(250); - expect(result.timedOut).toBe(false); + expect(result.abortReason).toBe(null); }); it('pairs each record its own uuid, filePath, and syncedAt across chunks', async () => { @@ -1387,7 +1387,7 @@ describe('markRecordsSynced', () => { MARK_SYNCED, MARK_SYNCED, ]); - expect(result.timedOut).toBe(false); + expect(result.abortReason).toBe(null); }); it('aligns a per-record failure to its index when it lands in a later chunk', async () => { @@ -1408,7 +1408,7 @@ describe('markRecordsSynced', () => { index === 120 ? outcome === MARK_FAILED : outcome === MARK_SYNCED, ), ).toBe(true); - expect(result.timedOut).toBe(false); + expect(result.abortReason).toBe(null); }); it('aborts remaining chunks on a timeout and marks the timed-out chunk pending', async () => { @@ -1427,7 +1427,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.abortReason).toBe('timeout'); // Chunk 1 synced (100), chunk 2 all timed out (100); chunk 3 has no outcome. expect(result.outcomes).toHaveLength(200); expect( @@ -1456,7 +1456,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.abortReason).toBe(null); expect(result.outcomes).toHaveLength(250); expect( result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), @@ -1475,7 +1475,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.abortReason).toBe(null); }); // Off-contract safety net: the declared contract always sends `data` as an @@ -1485,7 +1485,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.abortReason).toBe(null); }); // `data: null` is off-contract (markpost always sends the updated array), so @@ -1513,9 +1513,10 @@ describe('markRecordsSynced', () => { expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED, MARK_FAILED]); }); - it('aborts remaining chunks on a systemic failure (e.g. 401) without a timeout flag', async () => { - // A 401 (or any auth/rate-limit/5xx) will recur for every remaining chunk, - // so the run backs off after the first rather than hammering the server. + it('aborts remaining chunks and reports permanent on a 401 failure', async () => { + // A 401 (dead token) is a PERMANENT systemic failure: it recurs for every + // remaining chunk and every future pass, so the run backs off after the first + // chunk AND reports `abortReason: 'permanent'` so the caller stops the daemon. global.fetch = vi.fn().mockResolvedValue({ ok: false, status: 401, @@ -1525,8 +1526,8 @@ describe('markRecordsSynced', () => { const result = await markRecordsSynced(items(250)); // 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); + // A permanent abort — the caller both fails loud and stops the autoSync daemon. + expect(result.abortReason).toBe('permanent'); // 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( @@ -1534,6 +1535,27 @@ describe('markRecordsSynced', () => { ); }); + it('aborts remaining chunks and reports transient on a 503 failure', async () => { + // A 503 (or 429) is a TRANSIENT systemic failure: it aborts the remaining + // chunks to back off (it may recur), reported as `abortReason: 'transient'` + // so the caller can say the run stopped early — but it must NOT stop the + // daemon, since a lone 5xx can be a blip and the next pass should retry. + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 503, + json: () => Promise.resolve({}), + }); + + const result = await markRecordsSynced(items(250)); + expect(global.fetch).toHaveBeenCalledTimes(1); + // Aborted the run with a non-permanent reason — the daemon stays alive. + expect(result.abortReason).toBe('transient'); + expect(result.outcomes).toHaveLength(100); + expect(result.outcomes.every((outcome) => outcome === MARK_FAILED)).toBe( + true, + ); + }); + it('marks a whole chunk MARK_FAILED when the request rejects with an error response', async () => { mockFetch( { data: { errors: [{ title: 'Unprocessable', detail: 'bad batch' }] } }, @@ -1541,7 +1563,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.abortReason).toBe(null); }); it('marks a whole chunk MARK_FAILED on a network failure', async () => { @@ -1558,4 +1580,20 @@ describe('markRecordsSynced', () => { const result = await markRecordsSynced(items(1)); expect(result.outcomes).toEqual([MARK_FAILED]); }); + + // A forbidden account (403) is permanent like a dead token (401, covered + // above): it recurs every pass, so the chunk aborts with `abortReason: + // 'permanent'` and the caller stops the autoSync daemon. Its records stay + // MARK_FAILED (pending). The 401 and transient-503 counterparts sit with the + // other chunk-abort tests above. + it('aborts with a permanent reason on a forbidden (403) failure', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 403, + json: () => Promise.resolve({ data: { errors: [] } }), + }); + const result = await markRecordsSynced(items(2)); + expect(result.abortReason).toBe('permanent'); + expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED]); + }); });