From 0bb2f76ff0bef384ded4659fefabc30c68363d72 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 01:20:28 -0500 Subject: [PATCH 1/6] Stop autoSync on a permanent mark-synced failure A permanent mark-synced failure (dead token / forbidden account: 401/403) left autoSync on, so the daemon re-PATCHed the same records forever. Mirror the delete path's permanent-vs-transient handling: markRecordSynced returns a new MARK_PERMANENTLY_FAILED outcome, the batch runner aborts on it, and runDefaultSync returns false to stop the daemon. Transient failures keep autoSync alive to retry. Closes #133 --- src/index.ts | 127 ++++++++++---- src/libs/api.ts | 10 ++ src/libs/records.ts | 58 +++++-- tests/index.test.ts | 344 ++++++++++++++++++++++++++++++++++++- tests/libs/api.test.ts | 27 +++ tests/libs/records.test.ts | 42 +++++ 6 files changed, 561 insertions(+), 47 deletions(-) diff --git a/src/index.ts b/src/index.ts index 8153873..2ac1381 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,10 +6,15 @@ import { markRecordSynced, MARK_SYNCED, MARK_TIMED_OUT, + MARK_PERMANENTLY_FAILED, MarkSyncedOutcome, PENDING_STATUS, } from '@/libs/records.js'; -import { describeApiError, isSystemicApiFailure } from '@/libs/api.js'; +import { + describeApiError, + isPermanentApiFailure, + isSystemicApiFailure, +} from '@/libs/api.js'; import { buildWritePreview, ensureOutputDirectory, @@ -430,21 +435,30 @@ function reportDeferredServerChanges(deferredRecords: WrittenRecord[]): void { }); } +// Why a mark-synced run stopped early, or `null` if it ran every batch. One +// discriminant (not adjacent booleans) so the reason can't be self-contradictory. +// Only `'permanent'` also stops the autoSync daemon; `'timeout'` retries next pass. +type MarkAbortReason = 'timeout' | 'permanent' | null; + // 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 early abort it's shorter than the input +// because the remaining batches were never sent. `abortReason` records why (if) +// the run stopped early. interface MarkSyncedRun { outcomes: MarkSyncedOutcome[]; - timedOut: boolean; + abortReason: MarkAbortReason; } -// 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 the first permanent failure or timeout. A timeout means the server is +// hung, and a permanent failure (dead token / forbidden account) will recur on +// every remaining PATCH: either way firing the remaining batches would only burn +// requests already known to be doomed, so aborting leaves those records pending +// to retry next run, mirroring the push command's batch-abort. Every other +// failure (a per-record 4xx or a transient 429/5xx that may be a blip) does NOT +// abort — the next record may still succeed. A permanent failure outranks a +// timeout in the same batch: it's the non-recoverable signal that also stops the +// daemon, so it wins. async function markRecordsInBatches( writtenRecords: WrittenRecord[], ): Promise { @@ -464,20 +478,39 @@ async function markRecordsInBatches( outcomes.push(...batchOutcomes); + if (batchOutcomes.includes(MARK_PERMANENTLY_FAILED)) { + return { outcomes, abortReason: 'permanent' }; + } + if (batchOutcomes.includes(MARK_TIMED_OUT)) { - return { outcomes, timedOut: true }; + return { outcomes, abortReason: 'timeout' }; } } - return { outcomes, timedOut: false }; + return { outcomes, abortReason: null }; } -// 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 failure and a timeout +// abort both stop the run early (so the count includes records never attempted) +// and each reads differently from a scatter of per-record failures. The +// "auto-sync was stopped" clause is emitted only when `autoSyncStopped` — the +// caller passes the very value it returns to actually stop the daemon, so the +// message can't claim a stop that didn't happen (a one-shot `markpost sync` never +// had a daemon; mirroring the delete path, which says nothing about auto-sync). +function markFailureHeadline( + pendingCount: number, + abortReason: MarkAbortReason, + autoSyncStopped: boolean, +): string { + if (abortReason === 'permanent') { + const daemonClause = autoSyncStopped ? ', so auto-sync was stopped;' : ';'; + // Don't prescribe `markpost config` — a 403 (plan limit / sign-ups disabled) + // isn't a token problem. markRecordSynced already logged the classified, + // case-specific reason per record; point the user at that. + return `Failed to mark ${pendingCount} record(s) synced — 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.`; } @@ -491,10 +524,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 — @@ -530,24 +563,31 @@ 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 markRecordsInBatches(writtenRecords); + const { outcomes, abortReason } = await markRecordsInBatches(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, ); @@ -560,17 +600,31 @@ async function markWrittenRecordsSynced( settled.map(({ record }) => record.uuid), ); + // The single value the headline's "auto-sync was stopped" clause and the + // returned daemon-stop signal both derive from, so the message can never claim + // a stop that didn't happen. + const stoppingAutoSync = permanentlyFailed && autoSyncEnabled; + if (pending.length > 0) { + // Compose the headline here — this function holds the abort reason and the + // stop decision — so reportMarkFailures takes a ready string instead of + // drilling two adjacent, transposable args. + const headline = markFailureHeadline( + pending.length, + abortReason, + stoppingAutoSync, + ); reportMarkFailures( pending, writtenRecords.length - pending.length, - timedOut, + headline, spinner, ); - return; + return stoppingAutoSync; } spinner.success(`Marked ${writtenRecords.length} records synced!`); + return false; } // Ends a truncated sync on the truncation warning, never on a green success @@ -905,13 +959,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 @@ -942,8 +1001,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( @@ -1002,6 +1060,7 @@ 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"). + // `error` is already narrowed to ApiRequestError by the guard above. return error.isPermanent ? false : autoSync; } 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 b01a2df..6918bda 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -1,6 +1,7 @@ import { ApiTimeoutError, authedRequest, + isPermanentApiFailure, isSystemicApiFailure, logApiFailure, unwrapResourceAttributes, @@ -26,10 +27,20 @@ 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 non-timeout, non-permanent error (a lone 404 or bad +// body, but also a transient systemic 429/5xx that may be a one-off blip); the +// record stays pending and the rest of the batch still runs, since the next +// record may succeed. `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_PERMANENTLY_FAILED` — +// a permanent systemic failure (a dead token or a forbidden account: 401/403) +// that will recur on every record and every pass, so the batch runner stops on +// it AND the caller shuts the autoSync daemon down rather than looping into the +// same failure forever — mirroring how the delete path stops the daemon only on +// a permanent delete failure (see runDefaultSync). A transient systemic failure +// deliberately stays `MARK_FAILED`: unlike a permanent one it isn't guaranteed to +// doom every other record (a lone 5xx can be a blip), so aborting the whole run +// would strand records that would have settled. // // Values are prefixed (`mark-*`) so they never collide with the wire // `SYNCED_STATUS = 'synced'` above: these are internal outcome tags, not the @@ -38,9 +49,13 @@ 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_PERMANENTLY_FAILED = 'mark-permanently-failed'; 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_PERMANENTLY_FAILED; // 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 +392,17 @@ 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 +// Returns a discriminated 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 +// succeed) apart from a timeout (`MARK_TIMED_OUT`) and a permanent systemic +// failure (`MARK_PERMANENTLY_FAILED`). A timeout (hung server) stops the +// remaining batches to avoid paying the full timeout on each; a permanent +// failure (dead token / forbidden account) stops them because it will recur on +// every record — and additionally shuts the autoSync daemon down, since it can't +// clear on retry (matching the delete path). Both leave the affected records +// `pending`. +// +// 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 +446,26 @@ 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 permanent systemic failure (dead token / forbidden account: 401/403) + // will recur on every subsequent record and every future pass, so the batch + // runner stops on it and the caller shuts the autoSync daemon down instead + // of re-PATCHing a server it already knows will reject. + if (isPermanentApiFailure(error)) { + return MARK_PERMANENTLY_FAILED; + } + + // Every other error — a per-record 4xx or a transient systemic 5xx that may + // be a one-off — leaves this record pending and lets the rest of the batch + // proceed; aborting on a lone transient failure would strand records that + // would have settled. The daemon stays alive to retry next pass. + // @todo A sustained 429 (rate limit) would be better handled by aborting the + // burst to back off (per the api.ts contract) while keeping the daemon alive; + // out of scope for the permanent-failure fix — tracked as a follow-up. return MARK_FAILED; } }; diff --git a/tests/index.test.ts b/tests/index.test.ts index 9f7b178..a909baa 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -4,7 +4,13 @@ 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_FAILED, + MARK_SYNCED, + MARK_TIMED_OUT, + MARK_PERMANENTLY_FAILED, + MarkSyncedOutcome, +} from '@/libs/records.js'; import type { WrittenRecordState } from '@/libs/markdown.js'; vi.mock('@/libs/config.js', () => ({ @@ -1791,6 +1797,342 @@ describe('index', () => { expect(scheduledAutoSync).toBe(true); }); + // The mark-synced counterpart of the delete-permanent-failure test: with + // autoDelete off, a permanent mark-synced failure (dead token / forbidden + // account) surfaces as MARK_PERMANENTLY_FAILED. 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 { fetchAllRecords, markRecordSynced } = 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'); + + let scheduledAutoSync: boolean | undefined; + vi.mocked(runSyncWithAutoSchedule).mockImplementationOnce( + async (runSync) => { + scheduledAutoSync = await runSync(); + }, + ); + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + // autoSync on, so a naive mark-synced-failure path would return `true` and + // keep the daemon alive looping into the same failure. + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false, autoSync: true }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records: [mockRecord], + partial: false, + }); + vi.mocked(writeMarkdown).mockReturnValue('/mock/output/test-title.md'); + vi.mocked(markRecordSynced).mockResolvedValue(MARK_PERMANENTLY_FAILED); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledTimes(1); + 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(scheduledAutoSync).toBe(false); + }); + + // A TRANSIENT mark-synced failure (a per-record MARK_FAILED from a 5xx/network + // blip) also fails loud, but must keep autoSync alive: the record is still + // pending and the next pass should retry rather than the daemon shutting down. + it('keeps autoSync alive after a transient mark-synced failure', async () => { + const { fetchAllRecords, markRecordSynced } = 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'); + + let scheduledAutoSync: boolean | undefined; + vi.mocked(runSyncWithAutoSchedule).mockImplementationOnce( + async (runSync) => { + scheduledAutoSync = await runSync(); + }, + ); + vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); + vi.mocked(fetchSettings).mockResolvedValue( + mockSettings({ autoDelete: false, autoSync: true }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records: [mockRecord], + partial: false, + }); + vi.mocked(writeMarkdown).mockReturnValue('/mock/output/test-title.md'); + vi.mocked(markRecordSynced).mockResolvedValue(MARK_FAILED); + + 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 + // transient 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); + // Transient failure: the daemon must retry, so autoSync is preserved. + expect(scheduledAutoSync).toBe(true); + }); + + // Drives a batched mark-sync (autoDelete off) of `count` records where each + // record's mark-synced outcome comes from `outcomeFor(uuid)`, capturing what + // the run reports back to the scheduler. Shared by the batch-abort tests below, + // which differ only in count, the outcome map, and the `autoSync` setting. + // Returns a capture whose `scheduledAutoSync` is filled once `@/index.js` runs. + const arrangeBatchedMarkSync = async ({ + count, + outcomeFor, + autoSync, + }: { + count: number; + outcomeFor: (uuid: string) => MarkSyncedOutcome; + 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, markRecordSynced } = 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(markRecordSynced).mockImplementation((uuid: string) => + Promise.resolve(outcomeFor(uuid)), + ); + + return capture; + }; + + // A permanent failure must ABORT the remaining batches, not just flip a flag: + // firing more PATCHes against a dead token is wasted work. With 15 records + // (concurrency 10) and uuid-3 permanently failing in the first batch, the + // second batch of five must never run — so markRecordSynced is called exactly + // 10 times. A single-record test can't catch a regression here (the loop ends + // anyway), so this uses two batches. + it('aborts the remaining batches on a permanent mark-synced failure', async () => { + const capture = await arrangeBatchedMarkSync({ + count: 15, + autoSync: true, + outcomeFor: (uuid) => + uuid === 'uuid-3' ? MARK_PERMANENTLY_FAILED : MARK_SYNCED, + }); + const { markRecordSynced } = await import('@/libs/records.js'); + + await import('@/index.js'); + + // First batch of ten ran; the second batch of five never did. + expect(markRecordSynced).toHaveBeenCalledTimes(10); + expect(markRecordSynced).not.toHaveBeenCalledWith( + 'uuid-10', + expect.anything(), + ); + // uuid-3 failed plus the five never attempted = six pending; nine marked. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 6 record(s) synced'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Marked 9 record(s) synced despite the above.'), + ); + expect(process.exitCode).toBe(1); + // A permanent failure recurs, so the scheduler must not spin another pass. + expect(capture.scheduledAutoSync).toBe(false); + }); + + // The other side of the discriminant: a TIMEOUT aborts the batches too, but + // 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. + it('aborts the remaining batches but keeps autoSync alive on a timeout', async () => { + const capture = await arrangeBatchedMarkSync({ + count: 15, + autoSync: true, + outcomeFor: (uuid) => (uuid === 'uuid-3' ? MARK_TIMED_OUT : MARK_SYNCED), + }); + const { markRecordSynced } = await import('@/libs/records.js'); + + await import('@/index.js'); + + expect(markRecordSynced).toHaveBeenCalledTimes(10); + 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 per-record MARK_FAILED (a lone 404, a transient 5xx blip) must NOT abort: + // the next record can still succeed, so all 15 across both batches are + // attempted and only the one failed record stays pending. Pins that only the + // two systemic/timeout reasons abort, not an ordinary per-record failure. + it('does not abort the remaining batches on a per-record mark-synced failure', async () => { + const capture = await arrangeBatchedMarkSync({ + count: 15, + autoSync: true, + outcomeFor: (uuid) => (uuid === 'uuid-3' ? MARK_FAILED : MARK_SYNCED), + }); + const { markRecordSynced } = await import('@/libs/records.js'); + + await import('@/index.js'); + + // Every record attempted — no abort — so both batches ran. + expect(markRecordSynced).toHaveBeenCalledTimes(15); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to mark 1 record(s) synced'), + ); + expect(process.exitCode).toBe(1); + // A per-record failure isn't permanent, so the daemon retries next pass. + expect(capture.scheduledAutoSync).toBe(true); + }); + + // 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 () => { + 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, autoSync: false }), + ); + vi.mocked(fetchAllRecords).mockResolvedValue({ + ok: true, + records: [mockRecord], + partial: false, + }); + vi.mocked(writeMarkdown).mockReturnValue('/mock/output/test-title.md'); + vi.mocked(markRecordSynced).mockResolvedValue(MARK_PERMANENTLY_FAILED); + + 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); + }); + + // Abort precedence is load-bearing: when a single batch returns BOTH a timeout + // and a permanent failure (a token expiring mid-run against a slow server), the + // permanent reason must win — otherwise the run reports 'timeout', keeps + // autoSync alive, and the daemon reschedules straight back into the dead token. + // uuid-2 times out and uuid-3 permanently fails in the same first batch. + it('lets a permanent failure outrank a timeout in the same batch', async () => { + const outcomeFor = (uuid: string): MarkSyncedOutcome => { + if (uuid === 'uuid-2') { + return MARK_TIMED_OUT; + } + if (uuid === 'uuid-3') { + return MARK_PERMANENTLY_FAILED; + } + return MARK_SYNCED; + }; + const capture = await arrangeBatchedMarkSync({ + count: 15, + autoSync: true, + outcomeFor, + }); + + await import('@/index.js'); + + // The permanent headline wins, not the timeout one. + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('a permanent error'), + ); + expect(mockSpinner.error).not.toHaveBeenCalledWith( + expect.stringContaining('Timed out marking records synced'), + ); + expect(process.exitCode).toBe(1); + // The daemon must stop despite the concurrent timeout. + expect(capture.scheduledAutoSync).toBe(false); + }); + + // A permanent failure in a LATER batch (not the first) guards the + // `outcomes[index]` alignment: `outcomes` already holds 20 entries before the + // aborting third batch would run, so an off-by-one would strand the wrong + // records. 25 records, uuid-13 fails in batch 2 of 3 — batch 3 (five records) + // never runs, so 20 PATCHes fire and 6 records (uuid-13 + the five unattempted) + // are reported pending, 19 marked. + it('aborts a later batch on a permanent failure and counts pending correctly', async () => { + const capture = await arrangeBatchedMarkSync({ + count: 25, + autoSync: true, + outcomeFor: (uuid) => + uuid === 'uuid-13' ? MARK_PERMANENTLY_FAILED : MARK_SYNCED, + }); + const { markRecordSynced } = await import('@/libs/records.js'); + + await import('@/index.js'); + + // Batches 1 and 2 ran (20 records); batch 3 of five never did. + expect(markRecordSynced).toHaveBeenCalledTimes(20); + expect(markRecordSynced).not.toHaveBeenCalledWith( + 'uuid-20', + expect.anything(), + ); + 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 aborting record (batch 2) is listed pending; a settled batch-1 record + // (uuid-0) is not — proving the outcome/index alignment holds across batches. + 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 77fa4ef..f60dcdf 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -10,6 +10,7 @@ import { MARK_FAILED, MARK_SYNCED, MARK_TIMED_OUT, + MARK_PERMANENTLY_FAILED, } from '@/libs/records.js'; import { ApiTimeoutError } from '@/libs/api.js'; import { ApiDeleteMeta } from '@/types/api.types.js'; @@ -1308,4 +1309,45 @@ describe('markRecordSynced', () => { MARK_FAILED, ); }); + + // A permanent systemic failure (a dead token, 401, or a forbidden account, + // 403) will recur on every subsequent record and every future pass, so it gets + // its own outcome the batch runner and sync loop key off — the sync stops the + // autoSync daemon instead of re-PATCHing a server it already knows will reject. + it('returns the permanently-failed outcome on an auth (401) failure', 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_PERMANENTLY_FAILED, + ); + }); + + it('returns the permanently-failed outcome on a forbidden (403) failure', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 403, + json: () => Promise.resolve({ data: { errors: [] } }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_PERMANENTLY_FAILED, + ); + }); + + // A transient systemic failure (rate-limit or 5xx) stays MARK_FAILED, NOT + // permanent: it may be a one-off blip, so the record is left pending, the rest + // of the batch still runs, and the autoSync daemon keeps going to retry next + // pass. Only a permanent 401/403 aborts the run and stops the daemon. + it('returns the failed (not permanent) outcome on a transient server (503) failure', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 503, + json: () => Promise.resolve({ data: { errors: [] } }), + }); + expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + MARK_FAILED, + ); + }); }); From 4865765ce25081107adaf9f22dea08dbcf1dfb3b Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:35:18 -0500 Subject: [PATCH 2/6] Review round 1: distinguish transient systemic abort; fold permanence into abort - Add 'transient' to MarkAbortReason so a 429/5xx abort reports the run stopped early (records never attempted) instead of reading as a scatter of per-record failures; matching markFailureHeadline branch. - Classify permanence once in markSyncedChunk and fold it into abort so abort and abortReason can't disagree even if isPermanent is widened beyond isSystemic. - Drop the stale 429 @todo (the bulk path already backs off on a systemic 429). - Fix the 25-record test comment (short outcomes array is a mocked abort stand-in). --- src/index.ts | 20 ++++++++++------ src/libs/records.ts | 49 ++++++++++++++++++++++++-------------- tests/index.test.ts | 41 +++++++++++++++++++++++++------ tests/libs/records.test.ts | 13 +++++----- 4 files changed, 85 insertions(+), 38 deletions(-) diff --git a/src/index.ts b/src/index.ts index 15fca83..b546a74 100644 --- a/src/index.ts +++ b/src/index.ts @@ -438,13 +438,13 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { })); } -// Headline for the mark-synced failure report. A permanent failure and a timeout -// abort both stop the run early (so the count includes records never attempted) -// and each reads differently from a scatter of per-record failures. The -// "auto-sync was stopped" clause is emitted only when `autoSyncStopped` — the -// caller passes the very value it returns to actually stop the daemon, so the -// message can't claim a stop that didn't happen (a one-shot `markpost sync` never -// had a daemon; mirroring the delete path, which says nothing about auto-sync). +// Headline for the mark-synced failure report. A permanent, timeout, or transient +// abort all stop the run early (so the count includes records never attempted) and +// each reads differently from a scatter of per-record failures. The "auto-sync was +// stopped" clause is emitted only when `autoSyncStopped` — the caller passes the +// very value it returns to actually stop the daemon, so the message can't claim a +// stop that didn't happen (a one-shot `markpost sync` never had a daemon; +// mirroring the delete path, which says nothing about auto-sync). function markFailureHeadline( pendingCount: number, abortReason: MarkAbortReason, @@ -462,6 +462,12 @@ function markFailureHeadline( 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, so some of + // the pending records were never attempted. Say so, and that a retry follows. + return `Failed to mark ${pendingCount} record(s) synced — a systemic error stopped the run early, so some were never attempted; they remain pending on the server and are retried 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.`; } diff --git a/src/libs/records.ts b/src/libs/records.ts index b39f5e2..94d800a 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -392,10 +392,11 @@ export type MarkSyncedItem = { // `'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. A transient systemic abort (429/5xx) and a plain run-completion both map -// to `null`: the run may be short (trailing chunks skipped) but the daemon stays -// alive to retry. -export type MarkAbortReason = 'timeout' | 'permanent' | null; +// 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 @@ -549,20 +550,32 @@ const markSyncedChunk = async ( } // Every failed record in the chunk stays `MARK_FAILED` (pending, retried - // next run). A systemic failure aborts the remaining chunks; a PERMANENT one - // (dead token / forbidden account: 401/403) recurs every pass, so it also - // tells the caller to stop the autoSync daemon (`abortReason: 'permanent'`). - // A transient systemic failure aborts this run but keeps the daemon alive - // (`null`). A plain per-chunk failure doesn't abort — a later chunk may - // still succeed. - // @todo A sustained 429 (rate limit) would be better handled by aborting the - // burst to back off (per the api.ts contract) while keeping the daemon alive; - // out of scope for the permanent-failure fix — tracked as a follow-up. - return { - outcomes: items.map(() => MARK_FAILED), - abort: isSystemicApiFailure(error), - abortReason: isPermanentApiFailure(error) ? 'permanent' : null, - }; + // next run). Permanence is classified ONCE and folded into `abort`, so + // `abort` and `abortReason` can't disagree even if `isPermanent` is ever + // widened beyond `isSystemic`: a permanent failure always aborts AND stops + // the daemon; a transient systemic failure (auth/rate-limit/5xx that may be a + // blip) aborts 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 doesn't abort, since a later chunk may still succeed. + const failedOutcomes: MarkSyncedOutcome[] = items.map(() => MARK_FAILED); + + if (isPermanentApiFailure(error)) { + return { + outcomes: failedOutcomes, + abort: true, + abortReason: 'permanent', + }; + } + + if (isSystemicApiFailure(error)) { + return { + outcomes: failedOutcomes, + abort: true, + abortReason: 'transient', + }; + } + + return { outcomes: failedOutcomes, abort: false, abortReason: null }; } }; diff --git a/tests/index.test.ts b/tests/index.test.ts index c03e8f6..bfeec22 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1895,6 +1895,32 @@ describe('index', () => { 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). + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('a systemic error stopped the run early'), + ); + expect(mockSpinner.error).toHaveBeenCalledWith( + expect.stringContaining('2 record(s)'), + ); + expect(process.exitCode).toBe(1); + // A transient error can clear, so the daemon must retry next pass. + expect(capture.scheduledAutoSync).toBe(true); + }); + // 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 @@ -1917,13 +1943,14 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); - // A permanent abort on a LATER chunk guards index's `outcomes[index]` - // alignment: the bulk call returns a short outcomes array (20 entries for 25 - // records — the aborting chunk fired, the trailing chunk didn't), with uuid-13 - // failed inside it. index must count uuid-13 plus the five never-attempted 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. + // 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, diff --git a/tests/libs/records.test.ts b/tests/libs/records.test.ts index 8f44ae3..f982e77 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -1535,10 +1535,11 @@ describe('markRecordsSynced', () => { ); }); - it('aborts remaining chunks but reports no daemon-stop on a transient (503) failure', async () => { - // A 503 (or 429) is a TRANSIENT systemic failure: it still aborts the - // remaining chunks (it may recur), but must NOT stop the daemon — a lone 5xx - // can be a blip, so the next pass should retry. `abortReason` stays null. + 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, @@ -1547,8 +1548,8 @@ describe('markRecordsSynced', () => { const result = await markRecordsSynced(items(250)); expect(global.fetch).toHaveBeenCalledTimes(1); - // Aborted the run, but no permanent/timeout reason — the daemon stays alive. - expect(result.abortReason).toBe(null); + // 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, From e09f81e3bbc841ea6f9fb1a2a5d5841da1e91188 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:40:22 -0500 Subject: [PATCH 3/6] Review round 2: word transient headline conditionally; clarify null-abort test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - markFailureHeadline now claims 'never attempted' only when a trailing chunk was actually unsent, and 'retried next run' only when a daemon is running — derived from the same (abortReason, autoSyncEnabled) as the returned stop signal, so it can't overclaim on a last-chunk abort or a one-shot sync. - Rename the abortReason:null index test to 'per-chunk (non-systemic)' since a 5xx now maps to 'transient', and add a sibling asserting no overclaim. --- src/index.ts | 56 +++++++++++++++++++++++++--------------- tests/index.test.ts | 63 ++++++++++++++++++++++++++++++++++----------- 2 files changed, 83 insertions(+), 36 deletions(-) diff --git a/src/index.ts b/src/index.ts index b546a74..d5646e3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -439,19 +439,27 @@ function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { } // Headline for the mark-synced failure report. A permanent, timeout, or transient -// abort all stop the run early (so the count includes records never attempted) and -// each reads differently from a scatter of per-record failures. The "auto-sync was -// stopped" clause is emitted only when `autoSyncStopped` — the caller passes the -// very value it returns to actually stop the daemon, so the message can't claim a -// stop that didn't happen (a one-shot `markpost sync` never had a daemon; -// mirroring the delete path, which says nothing about auto-sync). +// 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, - autoSyncStopped: boolean, + { + autoSyncEnabled, + hasUnattempted, + }: { + autoSyncEnabled: boolean; + hasUnattempted: boolean; + }, ): string { if (abortReason === 'permanent') { - const daemonClause = autoSyncStopped ? ', so auto-sync was stopped;' : ';'; + const daemonClause = autoSyncEnabled ? ', so auto-sync was stopped;' : ';'; // 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. @@ -463,9 +471,12 @@ function markFailureHeadline( } if (abortReason === 'transient') { - // A systemic error (rate limit / 5xx) aborted the run to back off, so some of - // the pending records were never attempted. Say so, and that a retry follows. - return `Failed to mark ${pendingCount} record(s) synced — a systemic error stopped the run early, so some were never attempted; they remain pending on the server and are retried next run.`; + // A systemic error (rate limit / 5xx) aborted the run to back off. Only claim + // records were skipped if a trailing chunk was actually unsent, and only + // promise a retry if a daemon is alive to run one. + const skipped = hasUnattempted ? ', so some were never attempted' : ''; + const retry = autoSyncEnabled ? ' and are retried next run' : ''; + return `Failed to mark ${pendingCount} record(s) synced — a systemic error stopped the run early${skipped}; they remain pending on the server${retry}.`; } return `Failed to mark ${pendingCount} record(s) synced — written locally but still pending on the server; they may be re-written next run.`; @@ -556,20 +567,23 @@ async function markWrittenRecordsSynced( settled.map(({ record }) => record.uuid), ); - // The single value the headline's "auto-sync was stopped" clause and the - // returned daemon-stop signal both derive from, so the message can never claim - // a stop that didn't happen. + // 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 - // stop decision — so reportMarkFailures takes a ready string instead of - // drilling two adjacent, transposable args. - const headline = markFailureHeadline( - pending.length, - abortReason, - stoppingAutoSync, - ); + // 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, diff --git a/tests/index.test.ts b/tests/index.test.ts index bfeec22..a9493ea 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1846,11 +1846,12 @@ describe('index', () => { expect(capture.scheduledAutoSync).toBe(false); }); - // A TRANSIENT mark-synced failure (a per-record MARK_FAILED from a 5xx/network - // blip, `abortReason: null`) also fails loud, but must keep autoSync alive: the - // record is still pending and the next pass should retry rather than the daemon - // shutting down. - it('keeps autoSync alive after a transient mark-synced failure', async () => { + // 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, @@ -1861,12 +1862,12 @@ describe('index', () => { // 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 - // transient failure wrongly routed through the timeout branch. + // 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); - // Transient failure: the daemon must retry, so autoSync is preserved. + // A non-aborting failure: the daemon must retry, so autoSync is preserved. expect(capture.scheduledAutoSync).toBe(true); }); @@ -1910,17 +1911,48 @@ describe('index', () => { // uuid-1 failed plus uuid-2 never attempted = two pending, with the // stopped-early wording (not the generic per-record failure line). - expect(mockSpinner.error).toHaveBeenCalledWith( - expect.stringContaining('a systemic error stopped the run early'), - ); - expect(mockSpinner.error).toHaveBeenCalledWith( - expect.stringContaining('2 record(s)'), - ); + 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 and a daemon is alive, so both conditional + // clauses appear. + expect(headline).toContain('so some were never attempted'); + expect(headline).toContain('are retried next run'); 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 clauses are conditional: when the abort lands on the + // LAST chunk (no unattempted tail) and no daemon is running (one-shot sync), it + // must claim neither "never attempted" nor "retried next run" — the same + // false-claim guard the permanent branch has. outcomes length == count, so + // nothing was skipped; autoSync off, so there is no next run. + it('does not overclaim on a transient abort with no unattempted tail and no daemon', async () => { + await arrangeMarkSync({ + count: 2, + autoSync: false, + 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'); + // No daemon, so it must not promise a next run. + expect(headline).not.toContain('retried next run'); + 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 @@ -1952,8 +1984,9 @@ describe('index', () => { // 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 outcomes: MarkSyncedOutcome[] = Array.from( + { length: 20 }, + (_item, index) => (index === 13 ? MARK_FAILED : MARK_SYNCED), ); const capture = await arrangeMarkSync({ count: 25, From 5bd03dd4fb84b1021627379cd35e1a6e0b1f51ee Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:46:28 -0500 Subject: [PATCH 4/6] Review round 3: make abortReason the single abort signal - Drop the redundant `abort` boolean from MarkSyncedChunkResult; a non-null abortReason IS the abort signal, so the type can no longer express a contradictory { abort: false, abortReason: 'permanent' }. Loop gates on abortReason !== null. - Fix the stale doc comment (transient reports 'transient', not null). - Return the daemon-stop signal from the mark-synced success path too, so the decision has one source instead of a hardcoded false. --- src/index.ts | 6 +++++- src/libs/records.ts | 47 +++++++++++++++++++-------------------------- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/index.ts b/src/index.ts index d5646e3..c4089b8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -594,7 +594,11 @@ async function markWrittenRecordsSynced( } spinner.success(`Marked ${writtenRecords.length} records synced!`); - return false; + // 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 diff --git a/src/libs/records.ts b/src/libs/records.ts index 94d800a..7159720 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -470,14 +470,14 @@ 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. `abortReason` -// carries WHY up to the run level — `'timeout'` and `'permanent'` distinguish the -// hung-server and dead-token cases (the latter also stops the daemon); a transient -// systemic abort keeps `null` (aborts this run, daemon lives). `null` with -// `abort: false` is the plain success/per-chunk-failure case. +// stops rather than firing a burst it already knows will fail. A non-null +// `abortReason` IS the abort signal (the run stops on it) 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; abortReason: MarkAbortReason; }; @@ -525,7 +525,6 @@ const markSyncedChunk = async ( return { outcomes: outcomesFromResponse(items, body), - abort: false, abortReason: null, }; } catch (error) { @@ -544,38 +543,30 @@ const markSyncedChunk = async ( if (error instanceof ApiTimeoutError) { return { outcomes: items.map(() => MARK_TIMED_OUT), - abort: true, abortReason: 'timeout', }; } - // Every failed record in the chunk stays `MARK_FAILED` (pending, retried - // next run). Permanence is classified ONCE and folded into `abort`, so - // `abort` and `abortReason` can't disagree even if `isPermanent` is ever - // widened beyond `isSystemic`: a permanent failure always aborts AND stops - // the daemon; a transient systemic failure (auth/rate-limit/5xx that may be a - // blip) aborts to back off — a sustained 429 stops after the first chunk + // 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 (auth/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 doesn't abort, since a later chunk may still succeed. + // 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, - abort: true, - abortReason: 'permanent', - }; + return { outcomes: failedOutcomes, abortReason: 'permanent' }; } if (isSystemicApiFailure(error)) { - return { - outcomes: failedOutcomes, - abort: true, - abortReason: 'transient', - }; + return { outcomes: failedOutcomes, abortReason: 'transient' }; } - return { outcomes: failedOutcomes, abort: false, abortReason: null }; + return { outcomes: failedOutcomes, abortReason: null }; } }; @@ -617,7 +608,9 @@ export const markRecordsSynced = async ( outcomes.push(...chunkResult.outcomes); - if (chunkResult.abort) { + // 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 }; } } From 544d395996f8123165ca09ced4194c0d814ca0a7 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:52:11 -0500 Subject: [PATCH 5/6] Review round 4: doc/consistency cleanup - Fix two stale comments (markSyncedChunk header + MarkSyncedChunkResult doc) that still referenced the removed `abort` field / said transient aborts with null. - Outer sync catch goes through isPermanentApiFailure (the shared seam) instead of reading error.isPermanent directly, matching the mark-synced and delete paths. - Align the transient headline's 're-written next run' hedge with the timeout and generic branches (soft, ungated) instead of a stronger gated 'retried' promise. --- src/index.ts | 13 +++++++------ src/libs/records.ts | 14 +++++++------- tests/index.test.ts | 19 +++++++------------ 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/index.ts b/src/index.ts index c4089b8..463c8b6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -472,11 +472,11 @@ function markFailureHeadline( 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, and only - // promise a retry if a daemon is alive to run one. + // 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' : ''; - const retry = autoSyncEnabled ? ' and are retried next run' : ''; - return `Failed to mark ${pendingCount} record(s) synced — a systemic error stopped the run early${skipped}; they remain pending on the server${retry}.`; + 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.`; @@ -1034,8 +1034,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"). - // `error` is already narrowed to ApiRequestError by the guard above. - 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/records.ts b/src/libs/records.ts index 7159720..3343763 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -467,11 +467,10 @@ 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. A non-null -// `abortReason` IS the abort signal (the run stops on it) and carries WHY: +// 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 @@ -498,8 +497,9 @@ type MarkSyncedChunkResult = { // 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 `null` (daemon lives). Any other (per-chunk) -// failure maps to `MARK_FAILED` without aborting — a later chunk may still +// 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[], diff --git a/tests/index.test.ts b/tests/index.test.ts index a9493ea..e7c8346 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1917,24 +1917,21 @@ describe('index', () => { .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 and a daemon is alive, so both conditional - // clauses appear. + // A trailing chunk was unsent, so the "never attempted" clause appears. expect(headline).toContain('so some were never attempted'); - expect(headline).toContain('are retried next run'); 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 clauses are conditional: when the abort lands on the - // LAST chunk (no unattempted tail) and no daemon is running (one-shot sync), it - // must claim neither "never attempted" nor "retried next run" — the same - // false-claim guard the permanent branch has. outcomes length == count, so - // nothing was skipped; autoSync off, so there is no next run. - it('does not overclaim on a transient abort with no unattempted tail and no daemon', async () => { + // 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: false, + autoSync: true, result: { outcomes: [MARK_SYNCED, MARK_FAILED], abortReason: 'transient' }, }); @@ -1948,8 +1945,6 @@ describe('index', () => { // 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'); - // No daemon, so it must not promise a next run. - expect(headline).not.toContain('retried next run'); expect(process.exitCode).toBe(1); }); From 81716b2e1382fc53263754ec8f0639f990fd344b Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 21:56:55 -0500 Subject: [PATCH 6/6] Review round 5: headline consistency + truthful comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Permanent abort headline notes an unattempted tail (some were never attempted) when the abort truncated the run, matching the transient branch. - Drop 'auth/' from the transient-branch comment (a 401/403 is permanent and is caught by the guard above, so it never reaches the transient branch). - Fix the alignment test's trailing comment (outcomes truncated by an abort, not 'across chunks' — chunking is mocked away in this index-level test). --- src/index.ts | 5 ++++- src/libs/records.ts | 2 +- tests/index.test.ts | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 463c8b6..3a612d3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -460,10 +460,13 @@ function markFailureHeadline( ): 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 — 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.`; + 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') { diff --git a/src/libs/records.ts b/src/libs/records.ts index 3343763..bf20f43 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -551,7 +551,7 @@ const markSyncedChunk = async ( // 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 (auth/rate-limit/5xx that may be a blip) reports + // 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 diff --git a/tests/index.test.ts b/tests/index.test.ts index e7c8346..be5dbe6 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1999,7 +1999,8 @@ describe('index', () => { 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 across chunks. + // 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'), );