Skip to content

Mark records synced via bulk PATCH /api/records - #128

Merged
grimicorn merged 2 commits into
mainfrom
agent/bulk-mark-synced
Aug 27, 2026
Merged

Mark records synced via bulk PATCH /api/records#128
grimicorn merged 2 commits into
mainfrom
agent/bulk-mark-synced

Conversation

@grimicorn-agent

Copy link
Copy Markdown
Collaborator

What & why

sync's mark-synced step previously fired one PATCH /api/records/[uuid] per written record behind a concurrency-10 batcher whose own comment warned that a first sync of hundreds of records risks rate-limit/connection failures "exactly when the batch is biggest." This switches it to markpost's bulk PATCH /api/records, which takes a records[] array up to MAX_UPDATE_BATCH_SIZE (100) per request.

A large first sync now settles in ceil(N / 100) requests (e.g. 250 records = 3 requests) instead of one request per record.

Changes

  • src/libs/records.ts — replaced the single-record markRecordSynced with markRecordsSynced(items, syncedAt?), which chunks to MAX_MARK_SYNCED_BATCH_SIZE (100, mirroring markpost's cap) and PATCHes each chunk as one bulk request. Per-record outcomes come from the response data collection (a uuid present = synced, absent = still pending), giving real partial-failure detection.
  • src/index.ts — the mark-synced path now calls the bulk function once; removed the now-unused MARK_SYNCED_CONCURRENCY batcher. The written→settled bookkeeping, deferred-record handling, and failure reporting are unchanged.

Key decisions

  • Partial failure: markpost's bulk PATCH always returns the records it actually updated as data (verified against server/api/records/index.patch.ts). A uuid absent from the response stays pending and is reported MARK_FAILED (fail loud) so the user knows which files weren't marked, rather than a bare 2xx being trusted.
  • Abort semantics: a request timeout aborts the remaining chunks (a hung server would otherwise burn the full timeout on each), preserving the old per-record timeout behavior. A systemic failure (auth/rate-limit/5xx) also aborts — it will recur for every remaining chunk, so the CLI backs off (the same rule the fetch helpers apply). A plain per-chunk failure does not abort — a later chunk may still succeed.
  • Off-contract safety net: if data is ever not an array (a proxy/off-contract response the declared contract never produces), the per-uuid diff can't run, so it falls back to the corroborating meta.updated count; anything short of "all accepted" fails the chunk loud so records retry next run.

Viewable

The behavior change is in markpost sync (default sync, autoDelete off) — the "Marking records synced..." phase. No user-facing UI; verify via the CLI against a markpost account with pending records, or via the tests (tests/libs/records.test.ts, tests/index.test.ts).

Closes #123

Follow-up suggestions

  • Abort mark-synced on a repeated non-transient 4xx — a 422 from MAX_UPDATE_BATCH_SIZE drift (or a malformed-payload 400) is not systemic, so every chunk fires and fails identically; abort after the first such 4xx since the payload shape is uniform across chunks (suggested: P3, effort: S, evidence: src/libs/records.ts markSyncedChunk)
  • Distinct headline for a systemic mark-synced abort — a systemic abort returns a short outcomes array with timedOut: false, so the report uses the generic "Failed to mark N" wording and never tells the user the run stopped early with records unattempted (suggested: P3, effort: S, evidence: src/index.ts markFailureHeadline)
  • Stop the autoSync daemon on a permanent mark-synced failure — a permanent 403 on the PATCH leaves autoSync on, so the daemon re-fetches and re-attempts the same records forever; the delete path already guards this via deletePermanentlyFailed (pre-existing, not introduced here) (suggested: P2, effort: M, evidence: src/index.ts runDefaultSync mark-synced branch)

Switch mark-synced off the per-uuid PATCH /api/records/[uuid] onto markpost's
bulk PATCH /api/records, chunking to MAX_MARK_SYNCED_BATCH_SIZE (100) so a large
first sync settles in ceil(N/100) requests instead of one per record.

Closes #123
- Abort remaining chunks on a systemic (auth/rate-limit/5xx) failure, not just
  a timeout, to back off instead of hammering a doomed burst.
- Guard outcomesFromResponse against a non-array data body (fall back to
  meta.updated) so an off-contract/proxied response can't crash or spuriously
  fail a chunk.
- Reuse the exported MarkSyncedItem type; refresh stale outcome docs.
- Tests: cross-chunk index alignment, per-record uuid/filePath/syncedAt pairing,
  empty/null/non-array data, systemic abort; extract a shared echo helper.
@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail

Ran the independent reviewer (Opus, fresh context) as a loop; each round's findings were applied or skipped with a reason, then the diff was re-reviewed. The reviewer is an unbounded improver, so the loop was run to convergence on correctness/robustness/standards and stopped once only speculative enhancements remained.

Rounds 1–2 — applied

  • Inlined the misnamed markRecordsInBatches wrapper (it no longer batched) into a small toMarkSyncedItems projection.
  • Added systemic-failure abort (auth/rate-limit/5xx) alongside the timeout abort, so the CLI backs off instead of firing a doomed burst.
  • Added multi-chunk tests: a plain failure does not abort remaining chunks (ceil(N/100) requests still fire); systemic 401 aborts.
  • Simplified a misleading test helper (markResultBy no longer synthesizes a timedOut: true with a full-length outcomes array — a state production can't produce).
  • Extracted a shared echoBulkPatch test builder (rule of three: the echo body was hand-rolled 4×).

Rounds 3–5 — applied

  • Reused the exported MarkSyncedItem type instead of an inline structural type; refreshed the stale per-record outcome docs to chunk-level semantics.
  • Guarded outcomesFromResponse against a non-array data body (falls back to meta.updated) so an off-contract/proxied response can't throw a TypeError through the catch or spuriously fail a chunk. Tests added for data: [], data: null, meta-only, non-array data, and a short meta.updated.
  • Added a cross-chunk index-alignment test (a per-record failure at index 120 across chunk boundaries) and a per-record payload-pairing test (uuid/filePath/syncedAt paired correctly in the 2nd chunk).
  • Added a uuid range to the lib failure log line; renamed single-letter test vars (callcallCount, keepwasUpdated).

Skipped, with reasons (moved to the follow-up suggestions block on the PR)

  • Treat a bodyless/meta-only 2xx as success (all MARK_SYNCED): not adopted — verified against server/api/records/index.patch.ts that the handler always returns data as the updated array (never null/meta-only), so reading data is the authoritative partial-success signal; trusting meta over an empty data would mask a genuine "nothing matched". A non-array data is handled defensively via the meta.updated fallback.
  • Bisect-retry a chunk on a per-record 422: the payload is fully controlled (server-provided uuids, valid status/syncedAt/filePath), so a per-record validation 422 essentially can't occur; the realistic partial-success case (some uuids not found → 200 with fewer records) is already handled per-record. Captured as a follow-up (abort on repeated non-transient 4xx) rather than adding speculative retry logic.
  • Stop the autoSync daemon on a permanent mark-synced 403: pre-existing behavior (the old per-record path had the same loop); out of scope for this change and captured as a follow-up.

All checks green after each round: lint:fix, typecheck, test:ci (710 tests), build.

@grimicorn-agent grimicorn-agent added the has-suggestions PR carries follow-up suggestions for the improvement digest label Aug 23, 2026
@grimicorn
grimicorn merged commit d85c979 into main Aug 27, 2026
3 checks passed
@grimicorn
grimicorn deleted the agent/bulk-mark-synced branch August 27, 2026 21:48
grimicorn-agent pushed a commit that referenced this pull request Aug 28, 2026
…-synced

Reconciles #138 (stop autoSync on a permanent mark-synced failure) with main's
bulk chunked mark-synced (#128). Permanence is now a run-level MarkAbortReason
('permanent' stops the daemon; 'timeout' and transient-systemic keep it alive)
surfaced by markSyncedChunk/markRecordsSynced, instead of a per-record outcome
on the removed single-record path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-suggestions PR carries follow-up suggestions for the improvement digest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mark records synced with the bulk PATCH /api/records endpoint instead of one PATCH per record

2 participants