diff --git a/src/index.ts b/src/index.ts index b571c2f..f7c0333 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,10 +3,9 @@ import { deleteRecords, fetchAllRecords, - markRecordSynced, + markRecordsSynced, MARK_SYNCED, - MARK_TIMED_OUT, - MarkSyncedOutcome, + MarkSyncedItem, PENDING_STATUS, } from '@/libs/records.js'; import { describeApiError, isSystemicApiFailure } from '@/libs/api.js'; @@ -50,14 +49,6 @@ import { type Spinner = ReturnType; -// Cap how many mark-synced PATCHes are in flight at once. A large first sync -// can write hundreds of records; firing one unbounded `Promise.all` over all -// of them risks rate-limit/connection failures exactly when the batch is -// biggest — and every failed mark stays pending and re-duplicates next run. -// Declared here (above the top-level `dispatch()` call) so the hoisted helpers -// don't hit its temporal dead zone when the default sync runs. -const MARK_SYNCED_CONCURRENCY = 10; - // Slug ownership (resolved `.md` path -> the uuid that wrote it), shared // across every autoSync pass in this process rather than rebuilt per pass. // autoSync deletes each pass's records server-side, so a later pass fetching a @@ -430,46 +421,15 @@ function reportDeferredServerChanges(deferredRecords: WrittenRecord[]): void { }); } -// Outcome of a whole mark-synced run. `outcomes` holds one entry per *attempted* -// record in the original order; on a timeout abort it's shorter than the input -// because the remaining batches were never sent. `timedOut` records whether a -// timeout stopped the run early so the caller can report the abort explicitly. -interface MarkSyncedRun { - outcomes: MarkSyncedOutcome[]; - timedOut: boolean; -} - -// 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. -async function markRecordsInBatches( - writtenRecords: WrittenRecord[], -): Promise { - const outcomes: MarkSyncedOutcome[] = []; - - for ( - let start = 0; - start < writtenRecords.length; - start += MARK_SYNCED_CONCURRENCY - ) { - const batch = writtenRecords.slice(start, start + MARK_SYNCED_CONCURRENCY); - const batchOutcomes = await Promise.all( - batch.map(({ record, filePath }) => - markRecordSynced(record.uuid, filePath), - ), - ); - - outcomes.push(...batchOutcomes); - - if (batchOutcomes.includes(MARK_TIMED_OUT)) { - return { outcomes, timedOut: true }; - } - } - - return { outcomes, timedOut: false }; +// 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 +// `markRecordsSynced` (the records lib), keeping the API surface isolated there. +function toMarkSyncedItems(writtenRecords: WrittenRecord[]): MarkSyncedItem[] { + return writtenRecords.map(({ record, filePath }) => ({ + uuid: record.uuid, + filePath, + })); } // Headline for the mark-synced failure report. A timeout abort reads @@ -542,7 +502,9 @@ async function markWrittenRecordsSynced( spinner.start('Marking records synced...'); - const { outcomes, timedOut } = await markRecordsInBatches(writtenRecords); + const { outcomes, timedOut } = await markRecordsSynced( + toMarkSyncedItems(writtenRecords), + ); // A record is settled only when its mark-synced outcome is MARK_SYNCED; it is // pending if its mark failed or was never attempted (its outcome is undefined // because a timeout aborted the run before its batch). Evict settled records diff --git a/src/libs/records.ts b/src/libs/records.ts index b01a2df..1bf8ec0 100644 --- a/src/libs/records.ts +++ b/src/libs/records.ts @@ -25,11 +25,15 @@ import { 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. +// Per-record result of a bulk mark-synced run (see `markRecordsSynced`, which +// PATCHes records in chunks of up to MAX_MARK_SYNCED_BATCH_SIZE). `MARK_SYNCED` +// — the server returned this record among the ones it updated. `MARK_FAILED` — +// the record's chunk failed (a non-timeout error), or the server didn't return +// this uuid (partial success); it stays pending to re-sync next run. A plain +// chunk failure doesn't stop the run, but a systemic one (auth/rate-limit/5xx) +// aborts the remaining chunks — see `markSyncedChunk`. `MARK_TIMED_OUT` — the +// record's chunk hit the request timeout, a signal the server is hung; the run +// stops there rather than paying the full timeout on every remaining chunk. // // Values are prefixed (`mark-*`) so they never collide with the wire // `SYNCED_STATUS = 'synced'` above: these are internal outcome tags, not the @@ -360,48 +364,125 @@ export const createRecord = async ( } }; -// Marks a single record synced after the CLI has written it to disk, via -// markpost's PATCH /api/records/[uuid] (server/api/records/[uuid].patch.ts), -// which accepts `status`, `syncedAt`, and `filePath`. This is the -// non-destructive counterpart to `deleteRecords`: with autoDelete off, moving -// the record out of `pending` is what stops the next run's pending-only fetch -// from re-writing it. `syncedAt` is injected (defaulting to now) so callers -// and tests can pin the timestamp. Content-Type mirrors createRecord/ -// deleteRecords for consistency; markpost reads the body regardless. +// markpost's bulk update handler (server/api/records/index.patch.ts) caps each +// PATCH /api/records request at this many records (`MAX_UPDATE_BATCH_SIZE`); a +// larger `records[]` array is rejected with a 422. The CLI chunks to this size +// so a first sync of hundreds of records settles in `ceil(N / 100)` requests +// instead of one PATCH per record — the whole point of moving off the per-uuid +// endpoint (issue #123). +export const MAX_MARK_SYNCED_BATCH_SIZE = 100; + +// One record the CLI wants marked synced: the uuid to update and the on-disk +// path markpost stores so its UI can show where the note landed. +export type MarkSyncedItem = { + uuid: string; + filePath: string; +}; + +// 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. +export type MarkSyncedResult = { + outcomes: MarkSyncedOutcome[]; + timedOut: boolean; +}; + +// The `records[]` item markpost's bulk PATCH expects: the uuid to match plus +// the attributes to set. The CLI always sets `status`, `syncedAt`, and +// `filePath` together — moving a written record out of `pending` so the next +// run's pending-only fetch skips it. `filePath` is sent deliberately: markpost +// stores it on the record so its UI can show where a synced note landed — the +// user's own local path going to their own account, not a third-party leak. +const buildBulkRecordPayload = (item: MarkSyncedItem, syncedAt: string) => { + return { + uuid: item.uuid, + status: SYNCED_STATUS, + syncedAt, + filePath: item.filePath, + }; +}; + +// Reads the `meta.updated` count off a bulk-PATCH response, if present. markpost +// sends it alongside `data` (server/api/records/index.patch.ts); it's the +// corroborating signal used only when `data` itself is unreadable. +const updatedCountFromMeta = ( + body: RecordListApiResponse, +): number | undefined => { + const meta = body.meta as { updated?: unknown } | undefined; + + return typeof meta?.updated === 'number' ? meta.updated : undefined; +}; + +// Maps one chunk's request outcome to a per-item result. markpost returns the +// records it actually updated as the `data` collection (always an array; +// foreign/nonexistent uuids are silently dropped, mirroring the bulk delete +// endpoint), so a uuid present there was synced and one absent stays `pending` +// and is reported `MARK_FAILED` — that per-uuid diff is what gives the CLI real +// partial-failure detection across a 100-record chunk, rather than trusting a +// bare 2xx. // -// Goes through `authedRequest` so the PATCH inherits the same request timeout -// as every other API call (a stalled connection can't hang the sync forever). -// Unlike the fetch helpers above, a failure here is logged rather than -// re-thrown: this is non-critical post-write bookkeeping (the file is already -// on disk), so a failed mark simply leaves the record `pending` to re-sync -// next run, which is far less disruptive than aborting the whole sync after -// files have landed. +// If `data` is ever NOT an array (an off-contract or proxied response the +// declared contract never produces), the per-uuid diff can't run, so fall back +// to the corroborating `meta.updated` count: a full count means the whole chunk +// was accepted (all `MARK_SYNCED`); anything else fails the chunk loud so its +// records retry next run rather than being silently reported synced. +const outcomesFromResponse = ( + items: MarkSyncedItem[], + body: RecordListApiResponse, +): MarkSyncedOutcome[] => { + if (!Array.isArray(body.data)) { + const wholeChunkAccepted = updatedCountFromMeta(body) === items.length; + + return items.map(() => (wholeChunkAccepted ? MARK_SYNCED : MARK_FAILED)); + } + + const updated = unwrapResourceCollection('markRecordsSynced', body, 'record'); + const updatedUuids = new Set(updated.map((record) => record.uuid)); + + return items.map((item) => + updatedUuids.has(item.uuid) ? MARK_SYNCED : MARK_FAILED, + ); +}; + +// 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. +type MarkSyncedChunkResult = { + outcomes: MarkSyncedOutcome[]; + abort: boolean; + timedOut: boolean; +}; + +// PATCHes one chunk (<= MAX_MARK_SYNCED_BATCH_SIZE records) synced in a single +// bulk request. Routes through the shared `authedRequest` seam (like every +// other call): it attaches the bearer token, asserts success (throwing on a +// non-2xx, an errors-carrying 2xx, or an unparseable body such as an HTML error +// page behind a 200), and inherits the request timeout so a stalled connection +// can't hang the sync forever. A failure here is logged, not re-thrown — this +// is non-critical post-write bookkeeping (the files are already on disk), so a +// failed chunk simply leaves its records `pending` to re-sync next run. // -// Returns a three-way outcome rather than a bare boolean so the caller can -// tell a per-record failure (`MARK_FAILED`, keep going — the next record may -// succeed) apart from a timeout (`MARK_TIMED_OUT`). A timeout signals a hung -// server, so the caller stops the remaining batches instead of burning the -// full request timeout on every one; the record still stays `pending` either -// way. Reading the body back as a resource would mis-report a legitimate 2xx -// 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 -// stores it on the record so its UI can show where a synced note landed; it's -// the user's own local path going to their own account, not a third-party leak. -export const markRecordSynced = async ( - uuid: string, - filePath: string, - syncedAt: string = new Date().toISOString(), -): Promise => { +// A timeout maps every item to `MARK_TIMED_OUT` and aborts (a hung server would +// burn the full request timeout on every remaining chunk). A systemic failure +// (auth/rate-limit/5xx) also aborts — it will recur for every remaining chunk, +// so the caller backs off rather than hammering a server that just rejected the +// burst (the same rule the fetch helpers apply via `isSystemicApiFailure`). Any +// other (per-chunk) failure maps to `MARK_FAILED` without aborting — a later +// chunk may still succeed. +const markSyncedChunk = async ( + items: MarkSyncedItem[], + syncedAt: string, +): Promise => { try { - // Route through the shared authedRequest seam (like createRecord/ - // fetchRecord): it attaches the bearer token and asserts success (throwing - // on a non-2xx or an errors-carrying 2xx, and on an unparseable body such - // as an HTML error page behind a 200), so a failure lands in the catch - // below rather than being mistaken for a silent success that leaves the - // record pending. We ignore the returned body — the caller only needs to - // know the server accepted the change. - await authedRequest(`/api/records/${encodeURIComponent(uuid)}`, { + const body = (await authedRequest('/api/records', { method: 'PATCH', headers: { 'Content-Type': 'application/vnd.api+json', @@ -410,30 +491,90 @@ export const markRecordSynced = async ( data: { type: 'records', attributes: { - status: SYNCED_STATUS, - syncedAt, - filePath, + records: items.map((item) => + buildBulkRecordPayload(item, syncedAt), + ), }, }, }), - }); + })) as RecordListApiResponse; - return MARK_SYNCED; + return { + outcomes: outcomesFromResponse(items, body), + abort: false, + timedOut: false, + }; } catch (error) { + // Identify the chunk by its uuid range so a stderr reader can tell which + // records this failure left pending without cross-referencing the caller's + // own per-record report. + const firstUuid = items[0]?.uuid; + const lastUuid = items[items.length - 1]?.uuid; logErrorMessage( - `markRecordSynced["${uuid}"]`, + `markRecordsSynced[${firstUuid}..${lastUuid}, ${items.length} record(s)]`, error instanceof Error ? error.message : String(error), ); - // 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. if (error instanceof ApiTimeoutError) { - return MARK_TIMED_OUT; + return { + outcomes: items.map(() => MARK_TIMED_OUT), + abort: true, + timedOut: true, + }; } - return MARK_FAILED; + return { + outcomes: items.map(() => MARK_FAILED), + abort: isSystemicApiFailure(error), + timedOut: false, + }; + } +}; + +// Marks written records synced after the CLI has written them to disk, via +// markpost's bulk PATCH /api/records (server/api/records/index.patch.ts). This +// is the non-destructive counterpart to `deleteRecords`: with autoDelete off, +// moving each record out of `pending` is what stops the next run's pending-only +// fetch from re-writing it. `syncedAt` is injected (defaulting to now) so +// callers and tests can pin the timestamp. +// +// Chunks the input into `ceil(N / MAX_MARK_SYNCED_BATCH_SIZE)` requests so a +// large first sync settles up to 100 records per PATCH instead of one request +// per record — the rate-limit/connection pressure that motivated issue #123. +// Chunks run sequentially (not in parallel): the previous per-record path +// bounded concurrency for exactly this reason, and one request per 100 records +// is already few enough that firing them serially keeps the burst small without +// a concurrency limiter. +// +// Returns one outcome per record in input order. A timeout or a systemic +// failure (auth/rate-limit/5xx) stops the run at that chunk rather than firing a +// burst that's already doomed; the trailing records get no outcome and stay +// `pending` (their outcome index is `undefined`, which the caller reads as +// not-synced). A plain per-chunk failure doesn't abort — a later chunk may still +// succeed. `timedOut` is true only when a timeout (not a systemic failure) +// caused the stop, so the caller can word its report accordingly. +export const markRecordsSynced = async ( + items: MarkSyncedItem[], + syncedAt: string = new Date().toISOString(), +): Promise => { + const outcomes: MarkSyncedOutcome[] = []; + + for ( + let start = 0; + start < items.length; + start += MAX_MARK_SYNCED_BATCH_SIZE + ) { + const chunk = items.slice(start, start + MAX_MARK_SYNCED_BATCH_SIZE); + const chunkResult = await markSyncedChunk(chunk, syncedAt); + + outcomes.push(...chunkResult.outcomes); + + if (chunkResult.abort) { + return { outcomes, timedOut: chunkResult.timedOut }; + } } + + return { outcomes, timedOut: false }; }; export const fetchRecord = async (uuid: string): Promise => { diff --git a/tests/index.test.ts b/tests/index.test.ts index 7e65ab8..9b07794 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, + MarkSyncedOutcome, + MarkSyncedResult, +} from '@/libs/records.js'; import type { WrittenRecordState } from '@/libs/markdown.js'; vi.mock('@/libs/config.js', () => ({ checkConfig: vi.fn() })); @@ -15,7 +21,7 @@ vi.mock('@/libs/records.js', async (importOriginal) => ({ ...(await importOriginal()), fetchAllRecords: vi.fn(), deleteRecords: vi.fn(), - markRecordSynced: vi.fn(), + markRecordsSynced: vi.fn(), })); vi.mock('@/libs/markdown.js', () => ({ writeMarkdown: vi.fn(), @@ -67,6 +73,27 @@ const mockRecord: Record = { createdAt: '2024-01-01T00:00:00Z', }; +// The bulk mark-synced call resolves one outcome per input record in order. +// These build that `MarkSyncedResult` for the mocked `markRecordsSynced` so +// index-level tests drive the settle/report logic without re-deriving chunking +// (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. +const markResultBy = + (outcomeFor: (uuid: string) => MarkSyncedOutcome) => + async ( + items: { uuid: string; filePath: string }[], + ): Promise => ({ + outcomes: items.map((item) => outcomeFor(item.uuid)), + timedOut: false, + }); + +const markResultAll = (outcome: MarkSyncedOutcome) => + markResultBy(() => outcome); + describe('index', () => { // The production code only calls start/success/error on the spinner, so the // mock implements just those three. Typed as the full Spinner it satisfies @@ -502,7 +529,7 @@ describe('index', () => { content: 'x', createdAt: '2024-01-03T00:00:00Z', }; - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -525,17 +552,17 @@ describe('index', () => { return `/mock/output/${record.uuid}.md`; }, ); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_SYNCED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_SYNCED)); await import('@/index.js'); - // Only the clean record is marked synced; the dropped one stays pending so a - // later run can re-surface the unreconciled server revision. - expect(markRecordSynced).toHaveBeenCalledTimes(1); - expect(markRecordSynced).toHaveBeenCalledWith( - 'abc-123', - '/mock/output/abc-123.md', - ); + // Only the clean record is marked synced; the dropped one is held back from + // the bulk call so it stays pending and a later run can re-surface the + // unreconciled server revision. + expect(markRecordsSynced).toHaveBeenCalledTimes(1); + expect(markRecordsSynced).toHaveBeenCalledWith([ + { uuid: 'abc-123', filePath: '/mock/output/abc-123.md' }, + ]); expect(console.log).toHaveBeenCalledWith( expect.stringContaining('Deferred 1 record(s)'), ); @@ -855,7 +882,7 @@ describe('index', () => { }); it('writes but mutates nothing on the server (no delete, no mark) when settings cannot be read', async () => { - const { fetchAllRecords, deleteRecords, markRecordSynced } = await import( + const { fetchAllRecords, deleteRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -883,7 +910,7 @@ describe('index', () => { expect(mockSpinner.start).not.toHaveBeenCalledWith( 'Marking records synced...', ); - expect(markRecordSynced).not.toHaveBeenCalled(); + expect(markRecordsSynced).not.toHaveBeenCalled(); expect(console.log).toHaveBeenCalledWith( expect.stringContaining('Settings unreadable'), ); @@ -973,7 +1000,7 @@ describe('index', () => { }); it('marks records synced (not deleted) when autoDelete is false', async () => { - const { fetchAllRecords, deleteRecords, markRecordSynced } = await import( + const { fetchAllRecords, deleteRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -986,7 +1013,7 @@ describe('index', () => { ); vi.mocked(fetchAllRecords).mockResolvedValue({ ok: true, records: [mockRecord], partial: false }); vi.mocked(writeMarkdown).mockReturnValue('/mock/output/test-title.md'); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_SYNCED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_SYNCED)); await import('@/index.js'); @@ -996,16 +1023,15 @@ describe('index', () => { // The whole fix for #50: written records must be marked synced so the next // pending-only fetch skips them instead of re-writing duplicates. expect(mockSpinner.start).toHaveBeenCalledWith('Marking records synced...'); - expect(markRecordSynced).toHaveBeenCalledWith( - 'abc-123', - '/mock/output/test-title.md', - ); + expect(markRecordsSynced).toHaveBeenCalledWith([ + { uuid: 'abc-123', filePath: '/mock/output/test-title.md' }, + ]); expect(mockSpinner.success).toHaveBeenCalledWith('Marked 1 records synced!'); }); it('marks every written record synced, not just the first', async () => { const mockRecord2: Record = { uuid: 'def-456', title: 'Title 2', content: 'Content 2', createdAt: '2024-01-02T00:00:00Z' }; - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1024,25 +1050,22 @@ describe('index', () => { vi.mocked(writeMarkdown) .mockReturnValueOnce('/mock/output/test-title.md') .mockReturnValueOnce('/mock/output/title-2.md'); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_SYNCED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_SYNCED)); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(2); - expect(markRecordSynced).toHaveBeenCalledWith( - 'abc-123', - '/mock/output/test-title.md', - ); - expect(markRecordSynced).toHaveBeenCalledWith( - 'def-456', - '/mock/output/title-2.md', - ); + // Both written records go up in a single bulk call, in write order. + expect(markRecordsSynced).toHaveBeenCalledTimes(1); + expect(markRecordsSynced).toHaveBeenCalledWith([ + { uuid: 'abc-123', filePath: '/mock/output/test-title.md' }, + { uuid: 'def-456', filePath: '/mock/output/title-2.md' }, + ]); expect(mockSpinner.success).toHaveBeenCalledWith('Marked 2 records synced!'); }); it('excludes skipped records (null write result) from the mark-synced calls', async () => { const mockRecord2: Record = { uuid: 'def-456', title: 'Title 2', content: 'Content 2', createdAt: '2024-01-02T00:00:00Z' }; - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1061,20 +1084,21 @@ describe('index', () => { vi.mocked(writeMarkdown) .mockReturnValueOnce('/mock/output/test-title.md') .mockReturnValueOnce(null); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_SYNCED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_SYNCED)); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(1); - expect(markRecordSynced).toHaveBeenCalledWith( - 'abc-123', - '/mock/output/test-title.md', - ); + // The skipped record never lands on disk, so it must not appear in the bulk + // payload — only the one written record is sent. + expect(markRecordsSynced).toHaveBeenCalledTimes(1); + expect(markRecordsSynced).toHaveBeenCalledWith([ + { uuid: 'abc-123', filePath: '/mock/output/test-title.md' }, + ]); }); it('does not mark synced when every record was skipped', async () => { const mockRecord2: Record = { uuid: 'def-456', title: 'Title 2', content: 'Content 2', createdAt: '2024-01-02T00:00:00Z' }; - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1097,11 +1121,11 @@ describe('index', () => { expect(mockSpinner.start).not.toHaveBeenCalledWith( 'Marking records synced...', ); - expect(markRecordSynced).not.toHaveBeenCalled(); + expect(markRecordsSynced).not.toHaveBeenCalled(); }); it('reports a mark-synced failure loudly instead of claiming success', async () => { - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1118,7 +1142,7 @@ describe('index', () => { partial: false, }); vi.mocked(writeMarkdown).mockReturnValue('/mock/output/test-title.md'); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_FAILED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_FAILED)); await import('@/index.js'); @@ -1133,7 +1157,7 @@ describe('index', () => { it('reports only the records whose mark-synced failed, not the whole batch', async () => { const mockRecord2: Record = { uuid: 'def-456', title: 'Title 2', content: 'Content 2', createdAt: '2024-01-02T00:00:00Z' }; - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1152,11 +1176,12 @@ describe('index', () => { vi.mocked(writeMarkdown) .mockReturnValueOnce('/mock/output/test-title.md') .mockReturnValueOnce('/mock/output/title-2.md'); - // First record succeeds, second fails — the count and the listed path must - // reflect exactly the one failure, guarding against an off-by-one. - vi.mocked(markRecordSynced) - .mockResolvedValueOnce(MARK_SYNCED) - .mockResolvedValueOnce(MARK_FAILED); + // First record succeeds, second fails — the bulk response reports only + // def-456 as unmatched, so the count and listed path must reflect exactly + // the one failure, guarding against an off-by-one in the settle partition. + vi.mocked(markRecordsSynced).mockImplementation( + markResultBy((uuid) => (uuid === 'abc-123' ? MARK_SYNCED : MARK_FAILED)), + ); await import('@/index.js'); @@ -1175,14 +1200,14 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); - it('marks records across multiple concurrency batches and pinpoints a failure in a later batch', async () => { + it('sends every written record in one bulk call and pinpoints a single failure', async () => { const records: Record[] = Array.from({ length: 11 }, (_item, index) => ({ uuid: `uuid-${index}`, title: `Title ${index}`, content: `Content ${index}`, createdAt: '2024-01-01T00:00:00Z', })); - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1201,15 +1226,21 @@ describe('index', () => { vi.mocked(writeMarkdown).mockImplementation( (record: Record) => `/mock/output/${record.uuid}.md`, ); - // Only the 11th record (in the second batch, since concurrency is 10) - // fails, exercising the slice/order arithmetic across batches. - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => - Promise.resolve(uuid === 'uuid-10' ? MARK_FAILED : MARK_SYNCED), + // Every record goes up in a single bulk request; only uuid-10 comes back + // unmatched, exercising the per-record settle/report over one bulk response. + vi.mocked(markRecordsSynced).mockImplementation( + markResultBy((uuid) => (uuid === 'uuid-10' ? MARK_FAILED : MARK_SYNCED)), ); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(11); + expect(markRecordsSynced).toHaveBeenCalledTimes(1); + expect(markRecordsSynced).toHaveBeenCalledWith( + records.map((record) => ({ + uuid: record.uuid, + filePath: `/mock/output/${record.uuid}.md`, + })), + ); expect(mockSpinner.error).toHaveBeenCalledWith( expect.stringContaining('Failed to mark 1 record(s) synced'), ); @@ -1222,14 +1253,18 @@ describe('index', () => { expect(process.exitCode).toBe(1); }); - it('stops marking on the first timeout and reports the unattempted records as still pending', async () => { - const records: Record[] = Array.from({ length: 15 }, (_item, index) => ({ + // A timeout aborts inside markRecordsSynced (chunk boundaries + the abort are + // covered in tests/libs/records.test.ts); here the bulk call resolves a short + // outcomes array — a timed-out record plus a trailing record with no outcome + // because its chunk was never sent. Both must be reported pending. + it('reports timed-out and never-attempted records as pending after an abort', async () => { + const records: Record[] = Array.from({ length: 3 }, (_item, index) => ({ uuid: `uuid-${index}`, title: `Title ${index}`, content: `Content ${index}`, createdAt: '2024-01-01T00:00:00Z', })); - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1248,46 +1283,42 @@ describe('index', () => { vi.mocked(writeMarkdown).mockImplementation( (record: Record) => `/mock/output/${record.uuid}.md`, ); - // One record in the first batch (concurrency 10) times out; the second - // batch of five must never be attempted — those records stay pending. - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => - Promise.resolve(uuid === 'uuid-3' ? MARK_TIMED_OUT : MARK_SYNCED), - ); + // uuid-0 synced, uuid-1 timed out (abort), uuid-2 never attempted (no + // outcome). The short outcomes array models the real timeout abort. + vi.mocked(markRecordsSynced).mockResolvedValue({ + outcomes: [MARK_SYNCED, MARK_TIMED_OUT], + timedOut: true, + }); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(10); - expect(markRecordSynced).not.toHaveBeenCalledWith( - 'uuid-10', - expect.anything(), - ); - // Timed-out record (uuid-3) plus the five never attempted = six pending. + // Timed-out (uuid-1) plus the never-attempted uuid-2 = two pending. expect(mockSpinner.error).toHaveBeenCalledWith( - expect.stringContaining('6 record(s) still pending'), + expect.stringContaining('2 record(s) still pending'), ); expect(mockSpinner.error).toHaveBeenCalledWith( expect.stringContaining('Timed out marking records synced'), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('! uuid-3 -> /mock/output/uuid-3.md'), + expect.stringContaining('! uuid-1 -> /mock/output/uuid-1.md'), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('! uuid-14 -> /mock/output/uuid-14.md'), + expect.stringContaining('! uuid-2 -> /mock/output/uuid-2.md'), ); expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('Marked 9 record(s) synced despite'), + expect.stringContaining('Marked 1 record(s) synced despite'), ); expect(process.exitCode).toBe(1); }); - it('does not abort on a plain failure — a first-batch failure still marks every record', async () => { - const records: Record[] = Array.from({ length: 15 }, (_item, index) => ({ + it('does not use the timeout wording for a plain (non-timeout) failure', async () => { + const records: Record[] = Array.from({ length: 4 }, (_item, index) => ({ uuid: `uuid-${index}`, title: `Title ${index}`, content: `Content ${index}`, createdAt: '2024-01-01T00:00:00Z', })); - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1306,16 +1337,15 @@ describe('index', () => { vi.mocked(writeMarkdown).mockImplementation( (record: Record) => `/mock/output/${record.uuid}.md`, ); - // uuid-3 fails in the first batch. A plain failure must NOT stop the run — - // every record is still attempted, and the report uses the failure wording, - // not the timeout wording. Guards against widening the abort to any failure. - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => - Promise.resolve(uuid === 'uuid-3' ? MARK_FAILED : MARK_SYNCED), + // uuid-3 comes back unmatched (a plain failure, not a timeout). The report + // must use the failure wording, never the timeout wording, and every record + // still had an outcome — nothing was aborted. + vi.mocked(markRecordsSynced).mockImplementation( + markResultBy((uuid) => (uuid === 'uuid-3' ? MARK_FAILED : MARK_SYNCED)), ); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(15); expect(mockSpinner.error).toHaveBeenCalledWith( expect.stringContaining('Failed to mark 1 record(s) synced'), ); @@ -1326,19 +1356,19 @@ describe('index', () => { expect.stringContaining('! uuid-3 -> /mock/output/uuid-3.md'), ); expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('Marked 14 record(s) synced despite'), + expect.stringContaining('Marked 3 record(s) synced despite'), ); expect(process.exitCode).toBe(1); }); - it('reports only the timed-out record when the timeout lands in the final batch', async () => { - const records: Record[] = Array.from({ length: 10 }, (_item, index) => ({ + it('reports only the timed-out record when nothing was left unattempted', async () => { + const records: Record[] = Array.from({ length: 2 }, (_item, index) => ({ uuid: `uuid-${index}`, title: `Title ${index}`, content: `Content ${index}`, createdAt: '2024-01-01T00:00:00Z', })); - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1357,15 +1387,15 @@ describe('index', () => { vi.mocked(writeMarkdown).mockImplementation( (record: Record) => `/mock/output/${record.uuid}.md`, ); - // The ten records fill exactly one batch, so uuid-9's timeout leaves nothing - // unattempted — only the timed-out record itself is pending. - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => - Promise.resolve(uuid === 'uuid-9' ? MARK_TIMED_OUT : MARK_SYNCED), - ); + // The abort lands on the last record, so every record has an outcome and + // only the timed-out one is pending — no never-attempted tail. + vi.mocked(markRecordsSynced).mockResolvedValue({ + outcomes: [MARK_SYNCED, MARK_TIMED_OUT], + timedOut: true, + }); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(10); expect(mockSpinner.error).toHaveBeenCalledWith( expect.stringContaining('1 record(s) still pending'), ); @@ -1373,65 +1403,19 @@ describe('index', () => { expect.stringContaining('Timed out marking records synced'), ); expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('Marked 9 record(s) synced despite'), - ); - expect(process.exitCode).toBe(1); - }); - - it('aborts on a timeout in a later batch, not just the first', async () => { - const records: Record[] = Array.from({ length: 25 }, (_item, index) => ({ - uuid: `uuid-${index}`, - title: `Title ${index}`, - content: `Content ${index}`, - createdAt: '2024-01-01T00:00:00Z', - })); - const { fetchAllRecords, markRecordSynced } = await import( - '@/libs/records.js' - ); - const { writeMarkdown } = await import('@/libs/markdown.js'); - const { fetchSettings } = await import('@/libs/settings.js'); - const { default: yoctoSpinner } = await import('yocto-spinner'); - - vi.mocked(yoctoSpinner).mockReturnValue(mockSpinner); - vi.mocked(fetchSettings).mockResolvedValue( - mockSettings({ autoDelete: false }), - ); - vi.mocked(fetchAllRecords).mockResolvedValue({ - ok: true, - records, - partial: false, - }); - vi.mocked(writeMarkdown).mockImplementation( - (record: Record) => `/mock/output/${record.uuid}.md`, - ); - // uuid-12 sits in the second batch (concurrency 10); the third batch of - // five must never be attempted, proving the abort check runs per batch. - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => - Promise.resolve(uuid === 'uuid-12' ? MARK_TIMED_OUT : MARK_SYNCED), - ); - - await import('@/index.js'); - - expect(markRecordSynced).toHaveBeenCalledTimes(20); - expect(markRecordSynced).not.toHaveBeenCalledWith( - 'uuid-20', - expect.anything(), - ); - // uuid-12 timed out plus the five never attempted (uuid-20..24) = six. - expect(mockSpinner.error).toHaveBeenCalledWith( - expect.stringContaining('6 record(s) still pending'), + expect.stringContaining('Marked 1 record(s) synced despite'), ); expect(process.exitCode).toBe(1); }); - it('counts a plain failure alongside a timeout in the same batch as pending', async () => { - const records: Record[] = Array.from({ length: 12 }, (_item, index) => ({ + it('counts both a failed and a timed-out record as pending in one run', async () => { + const records: Record[] = Array.from({ length: 4 }, (_item, index) => ({ uuid: `uuid-${index}`, title: `Title ${index}`, content: `Content ${index}`, createdAt: '2024-01-01T00:00:00Z', })); - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1450,26 +1434,18 @@ describe('index', () => { vi.mocked(writeMarkdown).mockImplementation( (record: Record) => `/mock/output/${record.uuid}.md`, ); - // uuid-1 fails and uuid-4 times out — both in the first batch. The abort - // still fires, and both records (plus the two never attempted) are pending. - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => { - if (uuid === 'uuid-4') { - return Promise.resolve(MARK_TIMED_OUT); - } - - if (uuid === 'uuid-1') { - return Promise.resolve(MARK_FAILED); - } - - return Promise.resolve(MARK_SYNCED); + // uuid-1 failed and uuid-2 timed out (abort), so uuid-3 was never attempted. + // 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, }); await import('@/index.js'); - expect(markRecordSynced).toHaveBeenCalledTimes(10); - // uuid-1 failed, uuid-4 timed out, uuid-10 and uuid-11 never attempted = four. + // uuid-1 failed, uuid-2 timed out, uuid-3 never attempted = three pending. expect(mockSpinner.error).toHaveBeenCalledWith( - expect.stringContaining('4 record(s) still pending'), + expect.stringContaining('3 record(s) still pending'), ); expect(mockSpinner.error).toHaveBeenCalledWith( expect.stringContaining('Timed out marking records synced'), @@ -1481,7 +1457,7 @@ describe('index', () => { }); it('warns the sync was incomplete on the mark-synced path when a page failed', async () => { - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1499,7 +1475,7 @@ describe('index', () => { partial: true, }); vi.mocked(writeMarkdown).mockReturnValue('/mock/output/test-title.md'); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_SYNCED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_SYNCED)); await import('@/index.js'); @@ -1521,7 +1497,7 @@ describe('index', () => { content: 'c', createdAt: '2024-01-07T00:00:00Z', }; - const { fetchAllRecords, markRecordSynced } = await import( + const { fetchAllRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1538,7 +1514,7 @@ describe('index', () => { partial: false, }); vi.mocked(writeMarkdown).mockReturnValue('/mock/output/evil.md'); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_FAILED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_FAILED)); await import('@/index.js'); @@ -1586,7 +1562,7 @@ describe('index', () => { }); it('deletes records (never marks synced) when autoDelete is true', async () => { - const { fetchAllRecords, deleteRecords, markRecordSynced } = await import( + const { fetchAllRecords, deleteRecords, markRecordsSynced } = await import( '@/libs/records.js' ); const { writeMarkdown } = await import('@/libs/markdown.js'); @@ -1605,7 +1581,7 @@ describe('index', () => { expect(deleteRecords).toHaveBeenCalledWith(['abc-123']); // The delete path must not also PATCH records that are about to be removed. - expect(markRecordSynced).not.toHaveBeenCalled(); + expect(markRecordsSynced).not.toHaveBeenCalled(); expect(mockSpinner.start).not.toHaveBeenCalledWith( 'Marking records synced...', ); @@ -2162,7 +2138,7 @@ describe('index', () => { }); it('keeps an unsettled record in the written-path map so a later pass reuses its file', async () => { - const { fetchAllRecords, markRecordSynced } = await import('@/libs/records.js'); + 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'); @@ -2174,7 +2150,7 @@ describe('index', () => { // The mark-synced step fails both passes, so the record stays pending and is // re-fetched — exactly the case that used to drop a suffixed duplicate. vi.mocked(fetchAllRecords).mockResolvedValue({ ok: true, records: [mockRecord], partial: false }); - vi.mocked(markRecordSynced).mockResolvedValue(MARK_FAILED); + vi.mocked(markRecordsSynced).mockImplementation(markResultAll(MARK_FAILED)); vi.mocked(writeMarkdown).mockImplementation(captureWrittenPaths(snapshots)); vi.mocked(runSyncWithAutoSchedule).mockImplementationOnce(async (runSync) => { await runSync(); @@ -2188,14 +2164,18 @@ describe('index', () => { expect(snapshots[1].get('abc-123')?.path).toBe('/mock/output/abc-123.md'); // The reused record must still flow through to the settle step each pass — // reuse that dropped it from writtenRecords would never converge. - expect(markRecordSynced).toHaveBeenCalledTimes(2); - expect(markRecordSynced).toHaveBeenNthCalledWith(1, 'abc-123', '/mock/output/abc-123.md'); - expect(markRecordSynced).toHaveBeenNthCalledWith(2, 'abc-123', '/mock/output/abc-123.md'); + expect(markRecordsSynced).toHaveBeenCalledTimes(2); + expect(markRecordsSynced).toHaveBeenNthCalledWith(1, [ + { uuid: 'abc-123', filePath: '/mock/output/abc-123.md' }, + ]); + expect(markRecordsSynced).toHaveBeenNthCalledWith(2, [ + { uuid: 'abc-123', filePath: '/mock/output/abc-123.md' }, + ]); }); it('forgets only the mark-synced record that succeeded, keeping the failed one for reuse', async () => { const secondRecord: Record = { uuid: 'def-456', title: 'Title 2', content: 'Two', createdAt: '2024-01-02T00:00:00Z' }; - const { fetchAllRecords, markRecordSynced } = await import('@/libs/records.js'); + 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'); @@ -2206,10 +2186,10 @@ describe('index', () => { vi.mocked(fetchSettings).mockResolvedValue(mockSettings({ autoDelete: false })); // Both records are fetched again next pass; only def-456's mark fails, so it // must stay in the map while abc-123 is forgotten — pins the index alignment - // between the settled and failed filters. + // between the settled and failed filters over the bulk response. vi.mocked(fetchAllRecords).mockResolvedValue({ ok: true, records: [mockRecord, secondRecord], partial: false }); - vi.mocked(markRecordSynced).mockImplementation((uuid: string) => - Promise.resolve(uuid === 'abc-123' ? MARK_SYNCED : MARK_FAILED), + vi.mocked(markRecordsSynced).mockImplementation( + markResultBy((uuid) => (uuid === 'abc-123' ? MARK_SYNCED : MARK_FAILED)), ); vi.mocked(writeMarkdown).mockImplementation(captureWrittenPaths(snapshots)); vi.mocked(runSyncWithAutoSchedule).mockImplementationOnce(async (runSync) => { @@ -2276,7 +2256,7 @@ describe('index', () => { it('writes nothing, deletes nothing, and marks nothing under --dry-run', async () => { process.argv = ['node', 'index.js', 'sync', '--dry-run']; - const { fetchAllRecords, deleteRecords, markRecordSynced } = await import('@/libs/records.js'); + const { fetchAllRecords, deleteRecords, markRecordsSynced } = await import('@/libs/records.js'); const { writeMarkdown, ensureOutputDirectory, buildWritePreview } = await import('@/libs/markdown.js'); await arrangeDryRun(); @@ -2289,7 +2269,7 @@ describe('index', () => { expect(writeMarkdown).not.toHaveBeenCalled(); expect(ensureOutputDirectory).not.toHaveBeenCalled(); expect(deleteRecords).not.toHaveBeenCalled(); - expect(markRecordSynced).not.toHaveBeenCalled(); + expect(markRecordsSynced).not.toHaveBeenCalled(); expect(mockSpinner.start).not.toHaveBeenCalledWith('Writing records...'); expect(mockSpinner.start).not.toHaveBeenCalledWith('Deleting records...'); // A successful preview exits 0. diff --git a/tests/libs/records.test.ts b/tests/libs/records.test.ts index 77fa4ef..3f1606d 100644 --- a/tests/libs/records.test.ts +++ b/tests/libs/records.test.ts @@ -6,10 +6,11 @@ import { fetchAllRecords, fetchPaginatedRecords, fetchRecord, - markRecordSynced, + markRecordsSynced, MARK_FAILED, MARK_SYNCED, MARK_TIMED_OUT, + MAX_MARK_SYNCED_BATCH_SIZE, } from '@/libs/records.js'; import { ApiTimeoutError } from '@/libs/api.js'; import { ApiDeleteMeta } from '@/types/api.types.js'; @@ -1203,35 +1204,67 @@ describe('records API timeout propagation', () => { }); }); -describe('markRecordSynced', () => { +describe('markRecordsSynced', () => { beforeEach(() => { vi.spyOn(console, 'error').mockImplementation(() => {}); }); - // Unlike the other record calls, a mark-synced timeout is non-fatal: the file - // is already written, so a stalled PATCH is logged and reported as its own - // `MARK_TIMED_OUT` outcome (leaving the record to re-sync next run) rather than - // re-thrown to abort the whole sync. The distinct outcome lets the batch runner - // stop on the first timeout instead of paying it on every remaining record. The - // AbortSignal still bounds the wait so it can't hang forever. - it('returns the timed-out outcome on a request timeout instead of re-throwing', async () => { + // Build `count` items with predictable uuids/paths so a test can assert + // per-record outcomes and chunk boundaries without hand-listing records. + const items = (count: number) => + Array.from({ length: count }, (_item, index) => ({ + uuid: `uuid-${index}`, + filePath: `/vault/note-${index}.md`, + })); + + // Builds markpost's real bulk-PATCH success body from a request: it echoes + // back every requested uuid it "updated" as a resource, with a `meta.updated` + // count. `wasUpdated` decides which uuids the server matched — the default + // matches all; a partial-success test narrows it. Reading this `data` + // collection is what gives the CLI per-record outcomes rather than a bare 2xx. + const echoBulkPatch = ( + init: RequestInit, + wasUpdated: (uuid: string) => boolean = () => true, + ) => { + const body = JSON.parse(String(init.body)); + const records = body.data.attributes.records as { uuid: string }[]; + const updated = records.filter((record) => wasUpdated(record.uuid)); + + return Promise.resolve({ + ok: true, + json: () => + Promise.resolve({ + data: updated.map((record) => ({ + type: 'records', + attributes: { uuid: record.uuid }, + })), + meta: { updated: updated.length }, + }), + }); + }; + + // Every chunk resolves MARK_SYNCED for all its items. + const mockBulkPatchEcho = () => { global.fetch = vi .fn() - .mockRejectedValue(new DOMException('timed out', 'TimeoutError')); - expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( - MARK_TIMED_OUT, - ); - }); + .mockImplementation((_url, init) => echoBulkPatch(init)); + }; + + // The number of records in each chunk request, in call order. + const chunkSizes = () => + vi.mocked(global.fetch).mock.calls.map((call) => { + const body = JSON.parse(String(call[1]?.body)); + return body.data.attributes.records.length as number; + }); - it('PATCHes the record uuid with status=synced, syncedAt, and filePath', async () => { - mockFetch({ data: { attributes: mockRecord } }); - await markRecordSynced( - 'abc-123', - '/vault/test-title.md', + it('sends one bulk PATCH per record chunk with the synced attributes', async () => { + mockBulkPatchEcho(); + await markRecordsSynced( + [{ uuid: 'abc-123', filePath: '/vault/test-title.md' }], '2024-01-01T00:00:00.000Z', ); expect(global.fetch).toHaveBeenCalledWith( - 'https://example.com/api/records/abc-123', + 'https://example.com/api/records', expect.objectContaining({ method: 'PATCH', headers: { @@ -1242,9 +1275,14 @@ describe('markRecordSynced', () => { data: { type: 'records', attributes: { - status: 'synced', - syncedAt: '2024-01-01T00:00:00.000Z', - filePath: '/vault/test-title.md', + records: [ + { + uuid: 'abc-123', + status: 'synced', + syncedAt: '2024-01-01T00:00:00.000Z', + filePath: '/vault/test-title.md', + }, + ], }, }, }), @@ -1253,59 +1291,271 @@ describe('markRecordSynced', () => { }); it('defaults syncedAt to the current time when not supplied', async () => { - mockFetch({ data: { attributes: mockRecord } }); - await markRecordSynced('abc-123', '/vault/test-title.md'); + mockBulkPatchEcho(); + await markRecordsSynced([{ uuid: 'abc-123', filePath: '/vault/note.md' }]); const requestInit = vi.mocked(global.fetch).mock.calls[0]?.[1]; const sentBody = JSON.parse(String(requestInit?.body)); - expect(sentBody.data.attributes.syncedAt).toEqual(expect.any(String)); - expect( - Number.isNaN(Date.parse(sentBody.data.attributes.syncedAt)), - ).toBe(false); + const sentRecord = sentBody.data.attributes.records[0]; + expect(sentRecord.syncedAt).toEqual(expect.any(String)); + expect(Number.isNaN(Date.parse(sentRecord.syncedAt))).toBe(false); }); - it('returns the synced outcome on success', async () => { - mockFetch({ data: { attributes: mockRecord } }); - expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( - MARK_SYNCED, + it('sends nothing and reports no outcomes for an empty input', async () => { + global.fetch = vi.fn(); + const result = await markRecordsSynced([]); + expect(global.fetch).not.toHaveBeenCalled(); + expect(result).toEqual({ outcomes: [], timedOut: false }); + }); + + it('marks every record synced in a single request at exactly the batch size', async () => { + mockBulkPatchEcho(); + const result = await markRecordsSynced(items(MAX_MARK_SYNCED_BATCH_SIZE)); + // A full-but-not-over chunk is one request — the off-by-one boundary. + expect(global.fetch).toHaveBeenCalledTimes(1); + expect(chunkSizes()).toEqual([MAX_MARK_SYNCED_BATCH_SIZE]); + expect(result.outcomes).toHaveLength(MAX_MARK_SYNCED_BATCH_SIZE); + expect(result.outcomes.every((outcome) => outcome === MARK_SYNCED)).toBe( + true, ); + expect(result.timedOut).toBe(false); }); - // A 2xx that carries no resource body must count as success, not a spurious - // failure that warns the user of duplicates that never appear. - it('returns the synced outcome for a 2xx response with a null data body', async () => { - mockFetch({ data: null }); - expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( - MARK_SYNCED, + it('splits one-over-the-batch-size into two requests (ceil(N/100))', async () => { + mockBulkPatchEcho(); + const result = await markRecordsSynced( + items(MAX_MARK_SYNCED_BATCH_SIZE + 1), + ); + // 101 records must not go in one over-cap request markpost would 422. + expect(global.fetch).toHaveBeenCalledTimes(2); + expect(chunkSizes()).toEqual([MAX_MARK_SYNCED_BATCH_SIZE, 1]); + expect(result.outcomes).toHaveLength(MAX_MARK_SYNCED_BATCH_SIZE + 1); + expect(result.outcomes.every((outcome) => outcome === MARK_SYNCED)).toBe( + true, ); }); - // A 200 carrying an unparseable body (e.g. an HTML page from a proxy) must - // fail rather than be reported as a silent success that leaves the record - // pending and re-duplicated next run. - it('returns the failed outcome for a 2xx response whose body is not valid JSON', async () => { - global.fetch = vi.fn().mockResolvedValue({ - ok: true, - json: () => Promise.reject(new Error('Unexpected token < in JSON')), + it('chunks 250 records into 100/100/50 requests, each within the cap', async () => { + mockBulkPatchEcho(); + const result = await markRecordsSynced(items(250)); + expect(global.fetch).toHaveBeenCalledTimes(3); + expect(chunkSizes()).toEqual([100, 100, 50]); + // No chunk may ever exceed the server cap. + expect(chunkSizes().every((size) => size <= MAX_MARK_SYNCED_BATCH_SIZE)).toBe( + true, + ); + expect(result.outcomes).toHaveLength(250); + expect(result.timedOut).toBe(false); + }); + + it('pairs each record its own uuid, filePath, and syncedAt across chunks', async () => { + mockBulkPatchEcho(); + await markRecordsSynced(items(250), '2024-05-01T00:00:00.000Z'); + // Inspect the SECOND chunk's body: a mis-pairing (e.g. every record getting + // items[0].filePath) would still pass the size/uuid-echo assertions above. + const secondBody = JSON.parse( + String(vi.mocked(global.fetch).mock.calls[1]?.[1]?.body), + ); + const secondRecords = secondBody.data.attributes.records; + expect(secondRecords[0]).toEqual({ + uuid: 'uuid-100', + status: 'synced', + syncedAt: '2024-05-01T00:00:00.000Z', + filePath: '/vault/note-100.md', }); - expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( + expect(secondRecords[49]).toEqual({ + uuid: 'uuid-149', + status: 'synced', + syncedAt: '2024-05-01T00:00:00.000Z', + filePath: '/vault/note-149.md', + }); + }); + + it('reports MARK_FAILED for a uuid the server did not return (partial success)', async () => { + // The server updates every record except uuid-2 (e.g. it no longer exists); + // an absent uuid in the response means that record stays pending. + global.fetch = vi + .fn() + .mockImplementation((_url, init) => + echoBulkPatch(init, (uuid) => uuid !== 'uuid-2'), + ); + + const result = await markRecordsSynced(items(5)); + expect(result.outcomes).toEqual([ + MARK_SYNCED, + MARK_SYNCED, MARK_FAILED, + MARK_SYNCED, + MARK_SYNCED, + ]); + expect(result.timedOut).toBe(false); + }); + + it('aligns a per-record failure to its index when it lands in a later chunk', async () => { + // uuid-120 sits in the second chunk (records 100-149). The accumulated + // outcomes array must map it to index 120 — pins cross-chunk index + // alignment, which a uniform-outcome multi-chunk test can't catch. + global.fetch = vi + .fn() + .mockImplementation((_url, init) => + echoBulkPatch(init, (uuid) => uuid !== 'uuid-120'), + ); + + const result = await markRecordsSynced(items(150)); + expect(result.outcomes).toHaveLength(150); + expect(result.outcomes[120]).toBe(MARK_FAILED); + expect( + result.outcomes.every((outcome, index) => + index === 120 ? outcome === MARK_FAILED : outcome === MARK_SYNCED, + ), + ).toBe(true); + expect(result.timedOut).toBe(false); + }); + + it('aborts remaining chunks on a timeout and marks the timed-out chunk pending', async () => { + // First chunk succeeds; the second times out. The third chunk must never be + // sent — a hung server would otherwise burn the full timeout on every chunk. + let callCount = 0; + global.fetch = vi.fn().mockImplementation((_url, init) => { + callCount += 1; + if (callCount === 2) { + return Promise.reject(new DOMException('timed out', 'TimeoutError')); + } + + return echoBulkPatch(init); + }); + + 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); + // Chunk 1 synced (100), chunk 2 all timed out (100); chunk 3 has no outcome. + expect(result.outcomes).toHaveLength(200); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_SYNCED), + ).toBe(true); + expect( + result.outcomes + .slice(100, 200) + .every((outcome) => outcome === MARK_TIMED_OUT), + ).toBe(true); + }); + + it('does not abort remaining chunks on a plain (non-timeout) failure', async () => { + // Chunk 1 rejects with a plain network error; unlike a timeout, that must NOT + // stop the run — chunks 2 and 3 still fire and settle their records. + let callCount = 0; + global.fetch = vi.fn().mockImplementation((_url, init) => { + callCount += 1; + if (callCount === 1) { + return Promise.reject(new Error('Network error')); + } + + return echoBulkPatch(init); + }); + + 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.outcomes).toHaveLength(250); + expect( + result.outcomes.slice(0, 100).every((outcome) => outcome === MARK_FAILED), + ).toBe(true); + expect( + result.outcomes.slice(100).every((outcome) => outcome === MARK_SYNCED), + ).toBe(true); + }); + + // markpost's bulk PATCH always returns the records it actually updated as the + // `data` collection (server/api/records/index.patch.ts), so an empty `data` is + // the authoritative "nothing matched" signal, not a shape to treat as success. + // Reporting those records MARK_FAILED (fail loud) is deliberate — it leaves + // them pending to retry rather than falsely claiming a sync that didn't happen. + it('marks every record MARK_FAILED when the server returns an empty data set', async () => { + 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); + }); + + // Off-contract safety net: the declared contract always sends `data` as an + // array, but a proxy or future shape could send a meta-only body. Fall back to + // `meta.updated` rather than crashing or spuriously failing the chunk. + it('accepts the chunk via meta.updated when the response omits the data array', async () => { + 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); + }); + + // `data: null` is off-contract (markpost always sends the updated array), so + // with no `meta.updated` to confirm it, fail the chunk loud rather than + // guessing success — the records retry next run. + it('fails the chunk when data is null and no meta.updated confirms it', async () => { + mockFetch({ data: null }); + const result = await markRecordsSynced(items(2)); + expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED]); + }); + + it('does not crash on a non-array data object, falling back to meta.updated', async () => { + // A single resource object (the old per-uuid shape) must not throw a + // TypeError through the catch; meta.updated confirms the whole chunk. + mockFetch({ data: { attributes: { uuid: 'uuid-0' } }, meta: { updated: 2 } }); + const result = await markRecordsSynced(items(2)); + expect(result.outcomes).toEqual([MARK_SYNCED, MARK_SYNCED]); + }); + + it('fails the chunk when data is unreadable and meta.updated does not confirm all', async () => { + // No data array and a short/absent updated count — fail loud so the records + // retry next run rather than being falsely reported synced. + mockFetch({ meta: { updated: 1 } }); + const result = await markRecordsSynced(items(3)); + 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. + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 401, + json: () => Promise.resolve({}), + }); + + 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); + // 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( + true, ); }); - it('returns the failed outcome when the response contains errors', async () => { + it('marks a whole chunk MARK_FAILED when the request rejects with an error response', async () => { mockFetch( - { data: { errors: [{ title: 'Not Found', detail: 'Record missing' }] } }, + { data: { errors: [{ title: 'Unprocessable', detail: 'bad batch' }] } }, false, ); - expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( - MARK_FAILED, - ); + const result = await markRecordsSynced(items(3)); + expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED, MARK_FAILED]); + expect(result.timedOut).toBe(false); }); - it('returns the failed outcome on network failure', async () => { + it('marks a whole chunk MARK_FAILED on a network failure', async () => { global.fetch = vi.fn().mockRejectedValue(new Error('Network error')); - expect(await markRecordSynced('abc-123', '/vault/test-title.md')).toBe( - MARK_FAILED, - ); + const result = await markRecordsSynced(items(2)); + expect(result.outcomes).toEqual([MARK_FAILED, MARK_FAILED]); + }); + + it('marks the chunk MARK_FAILED for a 2xx response whose body is not valid JSON', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + json: () => Promise.reject(new Error('Unexpected token < in JSON')), + }); + const result = await markRecordsSynced(items(1)); + expect(result.outcomes).toEqual([MARK_FAILED]); }); });