From 3d5d1b928ae2fc4fdf88e3e46367280f31bd646c Mon Sep 17 00:00:00 2001 From: Maksymilian Gala Date: Mon, 31 Aug 2026 17:39:06 +0200 Subject: [PATCH 1/5] docs(specs): cursor provenance for data_sync runs A run reaches an adapter carrying a cursor and no indication of where it came from. A fresh dashboard start that silently inherited a previous run's position is indistinguishable from a Retry that was told to resume. For an adapter whose cursor encodes scope rather than only a position, that difference decides whether the run is correct: an inherited cursor imposes a stranger run's window and the run reports completed having skipped everything outside it. Specs cursorOrigin ('none' | 'explicit' | 'inherited' | 'self') on the adapter input, persisted on sync_runs alongside the source run id, and surfaced on the run detail page. Behaviour is unchanged. The fourth value is load-bearing: the engine hands the adapter run.cursor, not initialCursor, so on a queue redelivery a row stamped 'inherited' at creation would make a refusing adapter reject its own mid-run resume. 'self' is derived from batchesCompleted, so the column records where the run started and the delivered value describes what is actually being handed over. Closes the Risk #2 residual risk recorded in the run-scoped cursor spec. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-08-12-data-sync-run-scoped-cursor.md | 6 +- .../2026-08-31-data-sync-cursor-provenance.md | 299 ++++++++++++++++++ 2 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 .ai/specs/2026-08-31-data-sync-cursor-provenance.md diff --git a/.ai/specs/2026-08-12-data-sync-run-scoped-cursor.md b/.ai/specs/2026-08-12-data-sync-run-scoped-cursor.md index 9192a1638ec..33eba9c40df 100644 --- a/.ai/specs/2026-08-12-data-sync-run-scoped-cursor.md +++ b/.ai/specs/2026-08-12-data-sync-run-scoped-cursor.md @@ -163,7 +163,7 @@ caller of the fence in-tree; external callers passing a bare number in that slot | # | Failure scenario | Severity | Affected area | Mitigation | Residual risk | |---|---|---|---|---|---| | 1 | A reset deletes the shared row for an opted-out entity type, which does not exist, and the next incremental run resumes from a stale mid-walk cursor — re-importing only the tail | High | Adapter reset flows | `resetResumePosition` ships and the one in-tree reset flow calls it; documented as a MUST in the module `AGENTS.md` and the framework docs | A third-party reset flow that deletes `SyncCursor` directly and does not call it stays exposed. Nothing in code forces the pairing | -| 2 | `resolveResumeCursor` resumes from a `paused` or `cancelled` run whose window differs from what the caller now intends (e.g. a narrowed backfill), so the new run inherits the old run's scan window | Medium | Opted-out entity types | `fullSync` starts from `null`; a `completed` latest run also yields `null`, so only a genuinely interrupted run is inherited | Real. Resuming an interrupted run is the intended behaviour, but "interrupted with a different window" is indistinguishable from "interrupted with the same window" without a window fingerprint on the run row | +| 2 | `resolveResumeCursor` resumes from a `paused` or `cancelled` run whose window differs from what the caller now intends (e.g. a narrowed backfill), so the new run inherits the old run's scan window | Medium | Opted-out entity types | `fullSync` starts from `null`; a `completed` latest run also yields `null`, so only a genuinely interrupted run is inherited. `.ai/specs/2026-08-31-data-sync-cursor-provenance.md` adds `cursorOrigin` + `cursorSourceRunId`, so an adapter can tell an inherited cursor from an explicit one and refuse the former | Reduced to the adapter's own judgement. Provenance makes the two cases distinguishable without a window fingerprint, but core still cannot tell "interrupted with a different window" from "interrupted with the same window" — an adapter that wants that distinction must encode it in its own cursor | | 3 | An adapter's `persistsSharedCursor` disagrees between the write path (engine) and the read path (start paths) — e.g. two provider-key resolutions drift | Medium | All opted-out entity types | Both paths resolve the adapter through the single `resolveAdapterForIntegration` in `adapter-registry.ts` | Low; a non-deterministic predicate (reading mutable state) could still disagree between calls | | 4 | An external caller passes a bare number as `commitBatchProgress`'s fifth argument after the fence moved into the options object | Low | External adapters | TypeScript rejects it at compile time | Only untyped/`any` call sites are affected, and they would have to be fencing manually | | 5 | An opted-out entity type accumulates run rows and `resolveResumeCursor` sorts by `created_at`, which the `SyncRun` index does not cover | Low | Query performance | The leading indexed columns narrow the scan and only one row is fetched per run start | Negligible at realistic run-history sizes; worth revisiting if run retention grows large per entity type | @@ -216,6 +216,10 @@ interleave is what this change is about, and the unit tests reproduce it. ## Changelog - 2026-08-12 — implemented. +- 2026-08-31 — Risk #2's mitigation and residual risk updated: cursor provenance + (`.ai/specs/2026-08-31-data-sync-cursor-provenance.md`) makes an inherited cursor distinguishable + from an explicit one at the adapter, which is a lighter answer than the window fingerprint this + risk originally called for. - 2026-08-13 — merged `develop`; folded the ownership fence and the shared-cursor flag into one `CursorCommitOptions` object; added `resetResumePosition` and wired the Akeneo reset flow to it; deduplicated provider-key resolution into `adapter-registry.ts`; added start-cursor and reset diff --git a/.ai/specs/2026-08-31-data-sync-cursor-provenance.md b/.ai/specs/2026-08-31-data-sync-cursor-provenance.md new file mode 100644 index 00000000000..78a7e6d29e9 --- /dev/null +++ b/.ai/specs/2026-08-31-data-sync-cursor-provenance.md @@ -0,0 +1,299 @@ +# Data Sync — cursor provenance on run start and adapter input + +**Status:** draft +**Module:** `packages/core/src/modules/data_sync` +**Related:** `.ai/specs/2026-08-12-data-sync-run-scoped-cursor.md` (closes its Risk #2 residual risk), +`.ai/specs/implemented/SPEC-045b-data-sync-hub.md` + +## TLDR + +A run reaches an adapter carrying a cursor and no indication of where that cursor came from. A fresh +dashboard start that silently inherited a previous run's position is byte-for-byte indistinguishable +from a Retry that was told to resume. For an adapter whose cursor encodes **scope** rather than only a +position, the difference decides whether the run is correct: an inherited cursor imposes a stranger +run's window, and the run reports `completed` having skipped everything outside it. This adds +`cursorOrigin` — `'none' | 'explicit' | 'inherited' | 'self'` — to the adapter input, persists the +start-time origin and its source run on `sync_runs`, and surfaces it on the run detail page. Behaviour +is unchanged; adapters that ignore the field behave exactly as today. + +## Overview + +Additive and behaviour-preserving. One new exported union, one optional field on each of the two +adapter input types, two nullable columns on `sync_runs`, one derivation helper, and provenance-aware +variants of the two existing resolvers (the originals stay as delegating wrappers). The three start +paths label the cursor they resolved; the engine derives what it is actually handing over on this +delivery; the run detail page renders it. + +## Problem Statement + +`resolveStartCursor` (`lib/start-cursor.ts:22`) resolves a start position for every non-`fullSync` +start — the shared `sync_cursors` row, or for an opted-out entity type the last incomplete run's cursor +(`resolveResumeCursor`, `lib/sync-run-service.ts:376`). `api/run.ts:125` calls it for every dashboard +start. So pressing **Run** means *"continue whatever ran last"*. The operator is never told, and the +only opt-out is a `fullSync` switch they must remember to tick. + +That is correct for a cursor that is purely a position. It is wrong for a cursor that encodes scope — +filters, id/date bounds, dry-run flags, per-record suppression. A fresh, unfiltered run inherits a +failed run's date window, walks only that window, and reports `completed` having skipped everything +outside it. Adapters that notice this end up hand-rolling a *"may I believe this cursor?"* heuristic, +which is guesswork built on a value core handed over without provenance. + +**This is a recorded residual risk, not a new discovery.** +`.ai/specs/2026-08-12-data-sync-run-scoped-cursor.md` Risk #2 states it directly: + +> Resuming an interrupted run is the intended behaviour, but "interrupted with a different window" is +> indistinguishable from "interrupted with the same window" without a window fingerprint on the run row. + +### Why the obvious fix is wrong + +Refusing an inherited cursor on a fresh run would break Retry. `api/runs/[id]/retry.ts:121` sets +`cursor = fromBeginning ? null : previous.cursor ?? resolveStartCursor(...)` and `:146` passes the +previous run's parameters, so a retry arrives at the adapter carrying both a cursor and parameters, +indistinguishable from a fresh dashboard start that inherited one. Nothing on the run row separates +them: `createRun` (`lib/sync-run-service.ts:126`) writes `initialCursor` for both paths, and no column +links a retry to its predecessor. An adapter that refused inherited cursors would make Retry restart +from the top. + +### The third case the framing misses + +Retry is not uniformly explicit. `retry.ts:123` is `previous.cursor ?? await resolveStartCursor(...)`, +so a retry of a run that never committed a batch falls through to exactly the same inherited +resolution as a dashboard start. A discriminator that labelled all retries `'explicit'` would be a +second thing to distrust. This spec labels that fallback `'inherited'`, which is what it is. + +## Proposed Solution + +Provenance, not a behaviour change: make the origin of the cursor explicit and visible to the adapter, +leaving today's default resolution exactly as it is. Existing adapters ignore the new field; an adapter +that cares can refuse a silently inherited cursor without breaking Retry. + +### Why four values and not three + +The natural set is `'none' | 'explicit' | 'inherited'`. It is not sufficient, because the engine does +not hand the adapter the cursor the run started with. + +`sync-engine.ts:621` (import) and `:848` (export) pass `run.cursor`, not `run.initialCursor`. After the +first batch commits, `run.cursor` is the adapter's own output. BullMQ redelivers a job whose lock was +not renewed, and the engine re-enters the adapter with that advanced cursor. A row stamped `'inherited'` +at creation would still read `'inherited'` on that redelivery, so an adapter that refuses inherited +cursors would refuse its own legitimate mid-run resume — reintroducing the Retry breakage one level +down, in a path with no operator to notice it. + +So the two things are deliberately distinct: + +- the **column** records the provenance of the run's *starting* cursor — a fact about the run, stable + for its lifetime, and what an operator wants to read; +- the **value handed to the adapter** describes the cursor *actually being delivered on this call*, + which is `'self'` once the run has committed work of its own. + +`batchesCompleted` is the right signal for that derivation, and is the same token the ownership fence +already uses precisely because it advances by construction on every commit. A cursor is a free-form +adapter string an adapter may legitimately repeat between batches, so comparing `cursor` against +`initialCursor` would misreport a repeat as a fresh start. + +## Architecture + +### 1. `lib/adapter.ts` + +```ts +export type CursorOrigin = 'none' | 'explicit' | 'inherited' | 'self' +``` + +Optional `cursorOrigin?: CursorOrigin` on `StreamImportInput` and `StreamExportInput`: + +| Value | Meaning | +|---|---| +| `none` | No cursor. Start from the beginning. | +| `explicit` | The caller supplied this cursor deliberately — a Retry resuming the previous run's own position, or a provider flow that computed one. | +| `inherited` | Core resolved it from prior state the caller never named: the shared `sync_cursors` row, or the last incomplete run. | +| `self` | This run's own committed progress, handed back after a redelivery. | +| *absent* | A run created before this change, or a caller that supplied nothing. Adapters see what they see today. | + +### 2. `lib/start-cursor.ts` — resolution with provenance + +```ts +export type ResolvedStartCursor = { + cursor: string | null + origin: CursorOrigin // only 'none' | 'inherited' from this function + sourceRunId: string | null // set only when the cursor came from a previous run +} +export async function resolveStartCursorWithOrigin(params): Promise +``` + +`resolveStartCursor` stays as a `@deprecated` wrapper returning `.cursor`, so no existing signature +changes. + +The shared-row branch yields `sourceRunId: null`, because a `sync_cursors` row has no run id. That +asymmetry is load-bearing, not incidental: it is how the UI distinguishes *"continuing run X"* from +*"continuing the saved incremental cursor"* without a second discriminator column. + +### 3. `lib/sync-run-service.ts` + +New `resolveResumeCursorWithSource(...)` returning `{ cursor, runId }`. The existing +`resolveResumeCursor` is reimplemented as a delegating wrapper, so its behaviour and its four existing +tests are unchanged. + +### 4. `lib/cursor-origin.ts` — the delivered origin + +```ts +export function deliveredCursorOrigin(run: SyncRun): CursorOrigin | undefined { + if (run.cursor == null) return 'none' + if ((run.batchesCompleted ?? 0) > 0) return 'self' + return run.cursorOrigin ?? undefined +} +``` + +A separate file so `start-cursor.ts` stays about *resolution* and this one about *delivery* — the two +answer different questions and are called from different layers (start paths vs engine). + +`run.cursorOrigin ?? undefined` is what keeps pre-migration rows honest: `null` means *unknown*, and an +absent field is exactly what every adapter sees today. No backfill. + +### 5. `lib/sync-engine.ts` + +Passes `cursorOrigin: deliveredCursorOrigin(run)` at the two adapter calls (`:619-621` import, +`:846-848` export). `committedBatches` is already seeded from `activeRun.batchesCompleted` at `:603` / +`:830`, so the signal is in hand. + +### 6. The three start paths + +| Path | Cases | +|---|---| +| `api/run.ts:125` | `fullSync` → `'none'`; otherwise the resolved origin | +| `api/runs/[id]/retry.ts:121` | `fromBeginning` → `'none'`; `previous.cursor` present → `'explicit'` with `sourceRunId: previous.id`; fallback → the resolved origin (`'inherited'`) | +| `workers/sync-scheduled.ts:73` | `fullSync` → `'none'`; otherwise the resolved origin | + +### 7. `lib/start-run.ts` + +`StartDataSyncRunInput` gains `cursorOrigin?` and `cursorSourceRunId?`, passed to `createRun`. + +A caller that omits them gets `input.cursor == null ? 'none' : 'explicit'`. A caller that supplied a +cursor without going through `resolveStartCursorWithOrigin` did so deliberately, so `'explicit'` is the +honest label. This is correct for both out-of-module callers — `packages/sync-excel/.../api/import/route.ts:154` +and `packages/sync-akeneo/.../lib/first-import.ts:161` — so neither has to change, and neither silently +acquires an `'inherited'` label it did not earn. + +### 8. Run detail UI + +`backend/data-sync/runs/[id]/page.tsx` renders one line: *"Continuing run "* when +`cursorSourceRunId` is set, *"Continuing the saved incremental cursor"* for `'inherited'` without one, +nothing for `'none'` and `'explicit'`. This closes the operator half of the problem — today the page +renders neither `cursor` nor `initialCursor`, though the API has returned both since the hub shipped. + +**Out of scope**, as the request states: the start-form hint (*"this will continue the last incomplete +run — [start from the beginning]"*). That is a follow-up. + +## Data Models + +Two nullable columns on `sync_runs`. One additive migration; no backfill. + +| Table | Column | Type | Role | +|---|---|---|---| +| `sync_runs` | `cursor_origin` | `text null` | Provenance of the run's **starting** cursor. Written once at `createRun`, never mutated. `null` for pre-migration rows | +| `sync_runs` | `cursor_source_run_id` | `uuid null` | The run the cursor came from, when it came from a run. `null` for the shared-row and `'none'` cases | +| `sync_runs` | `batches_completed` | existing | Read by `deliveredCursorOrigin` to derive `'self'`; unchanged | +| `sync_runs` | `initial_cursor` | existing | Unchanged. `cursor_origin` describes *this* column's provenance | + +`cursor_source_run_id` is a bare `uuid`, not a foreign key, matching `progress_job_id` on the same +table. A run row is an append-only operational record; a FK would make run retention deletion order- +dependent for a column read only to render a link. + +Both columns are added to the `[OptionalProps]` list on the entity. + +## API Contracts + +No request-shape change. No zod schema changes. Two read responses gain two optional fields: + +| Route | Change | +|---|---| +| `GET /api/data_sync/runs/[id]` | adds `cursorOrigin`, `cursorSourceRunId` (both nullable) | +| `GET /api/data_sync/runs` | adds the same two fields per item | +| `POST /api/data_sync/run` | unchanged request and response; persists the resolved origin | +| `POST /api/data_sync/runs/[id]/retry` | unchanged request and response; persists the resolved origin | + +TypeScript contracts (all additive): + +| Symbol | Kind | Note | +|---|---|---| +| `CursorOrigin` | new exported type | `lib/adapter.ts` | +| `StreamImportInput.cursorOrigin?` | new optional member | absent = today's behaviour | +| `StreamExportInput.cursorOrigin?` | new optional member | absent = today's behaviour | +| `ResolvedStartCursor` | new exported type | `lib/start-cursor.ts` | +| `resolveStartCursorWithOrigin` | new export | `lib/start-cursor.ts` | +| `resolveStartCursor` | unchanged signature | now `@deprecated`, delegates | +| `SyncRunService.resolveResumeCursorWithSource` | new method | | +| `deliveredCursorOrigin` | new export | `lib/cursor-origin.ts` | +| `StartDataSyncRunInput.cursorOrigin?` / `.cursorSourceRunId?` | new optional members | defaulted when omitted | + +## Backward Compatibility + +Additive throughout. Per `BACKWARD_COMPATIBILITY.md`: + +- **§2 Type Definitions & Interfaces (STABLE)** — optional fields added to `StreamImportInput`, + `StreamExportInput` and `StartDataSyncRunInput`. No required field removed or narrowed. Same shape as + the `signal?: AbortSignal` addition that shipped on `develop`. +- **§3 Function Signatures (STABLE)** — no existing signature changes. `resolveStartCursor` and + `resolveResumeCursor` keep their exact signatures and behaviour as delegating wrappers. +- **§8 Database Schema (ADDITIVE-ONLY)** — two new nullable columns, no default, no rename, no removal, + no index change. + +An adapter compiled against the previous types keeps compiling and keeps behaving identically. A run +row written before the migration reads `cursor_origin = null`, which `deliveredCursorOrigin` maps to an +absent field rather than guessing an origin it cannot know. + +## Risks & Impact Review + +| # | Failure scenario | Severity | Affected area | Mitigation | Residual risk | +|---|---|---|---|---|---| +| 1 | An adapter refuses `'inherited'` and, on a redelivery mid-run, is handed its own advanced cursor still labelled `'inherited'` — so it restarts from the top on every worker hiccup | High | Adapters that act on the discriminator | `deliveredCursorOrigin` returns `'self'` once `batchesCompleted > 0`; the persisted column and the delivered value are documented as answering different questions | Low. A run killed before its first commit is genuinely still on its start cursor, so `'inherited'` is correct there — a refusing adapter restarts, which is what it asked for | +| 2 | A caller uses `startDataSyncRun` directly with a cursor and gets the `'explicit'` default when the cursor was in fact inherited by that caller's own logic | Medium | Out-of-module start paths | The two in-tree callers (`sync_excel`, `sync_akeneo`) compute their own cursors, so `'explicit'` is accurate for both; the field is settable for callers that know better | Real for a third-party caller that resolves an inherited cursor itself and does not label it. Nothing in code can detect that from inside `startDataSyncRun` | +| 3 | The shared-row and previous-run inheritance cases are collapsed into one `'inherited'` value, so an adapter cannot tell a durable feed position from a stranger backfill's scan state | Medium | Adapters serving both kinds | `sourceRunId` is non-null only for the previous-run case, so the two are distinguishable; the run detail copy already relies on that split | An adapter must know to check `sourceRunId`, not just `cursorOrigin`. Documented in `AGENTS.md` and the field's doc comment | +| 4 | `cursor_origin` is a free-form `text` column, so a bad write could persist a value outside the union | Low | Data integrity | The only writer is `createRun`, fed by typed call sites; TypeScript rejects anything else | A raw SQL write could still store garbage. `deliveredCursorOrigin` returns it verbatim, so an adapter doing an exhaustive switch would fall through its default | +| 5 | The migration adds two columns to `sync_runs`, which can be large on an instance with long run retention | Low | Deployment | Both columns are nullable with no default, so Postgres adds them as metadata-only operations without a table rewrite | Negligible | +| 6 | The run detail page renders a link to a source run the operator cannot open — a run outside their organization, or one since deleted | Low | Run detail UI | `cursorSourceRunId` is only ever set from a run resolved inside the same tenant/organization scope, and the detail route re-scopes on read | A soft-deleted source run yields a link that 404s. Acceptable; the id itself is still the useful diagnostic | + +## Testing + +**Unit** + +- `lib/__tests__/cursor-origin.test.ts` *(new)* — the `deliveredCursorOrigin` truth table: null cursor → + `'none'`; `batchesCompleted > 0` → `'self'` regardless of the stored value; stored value passed through + on the first delivery; `cursorOrigin: null` → `undefined`. +- `lib/__tests__/start-cursor.test.ts` — extended for `origin` and `sourceRunId` across the shared-row + branch (`sourceRunId: null`), the resume branch (`sourceRunId` set), and no cursor at all. +- `lib/__tests__/sync-run-service.shared-cursor.test.ts` — extended for + `resolveResumeCursorWithSource`, and that `resolveResumeCursor` still returns exactly what it did. +- `api/__tests__/run.test.ts` — the persisted origin for `fullSync` (`'none'`) versus an inherited start. +- `api/runs/[id]/__tests__/retry-cursor-origin.test.ts` *(new)* — the three retry cases, including the + `previous.cursor == null` fallback labelled `'inherited'`. There is currently no test of retry cursor + precedence at all, so this covers pre-existing behaviour as well as the new field. +- `lib/__tests__/sync-engine-*.test.ts` — the engine passes `'self'` on a redelivery after a committed + batch, and the stored origin on the first delivery. +- `workers/__tests__/sync-scheduled.test.ts` — the origin persisted for scheduled runs. + +**Integration** — `__integration__/TC-DS-011.spec.ts` *(new)*, self-contained per `.ai/qa/AGENTS.md` +(fixtures created in setup, cleaned up in teardown, no reliance on seeded data): start a run through +`POST /api/data_sync/run`, assert `GET /api/data_sync/runs/[id]` returns `cursorOrigin`, and assert the +run detail page renders the provenance line. Required by root `AGENTS.md` because this change touches +both an API surface and a UI path. + +Not covered: no test exercises an adapter that actually refuses an inherited cursor, because no in-tree +adapter does. That behaviour is the adapter's to implement; this spec ships the information it needs. + +## Final Compliance Report + +| Check | Result | +|---|---| +| Schema / migration | Two nullable columns on `sync_runs`; additive migration + updated `.snapshot-open-mercato.json` | +| HTTP surface | Two read responses gain optional fields; no request shape, zod schema or status code change | +| `BACKWARD_COMPATIBILITY.md` contracts | Additive under §2, §3 and §8; dated section added to that file | +| Tenant scoping | No new query. `cursorSourceRunId` is only set from a run already resolved under `organizationId` + `tenantId` | +| Encryption helpers | `resolveResumeCursorWithSource` reads via `findWithDecryption`, as its predecessor did | +| `yarn generate` | No diff — no auto-discovered file added | +| Locales / user-facing strings | Two keys added to all five locale files (`en`, `de`, `es`, `ko`, `pl`); none hardcoded | +| Unit tests | To be confirmed at implementation | +| Docs updated | `packages/core/src/modules/data_sync/AGENTS.md`, `apps/docs/docs/framework/modules/integrations-data-sync.mdx` | + +## Changelog + +- 2026-08-31 — drafted. From bc5374326b8c5e318614fe6631a792e38190094b Mon Sep 17 00:00:00 2001 From: Maksymilian Gala Date: Mon, 31 Aug 2026 17:47:29 +0200 Subject: [PATCH 2/5] feat(data_sync): carry cursor provenance to adapters and the run row A run reached an adapter with a cursor and nothing to say where it came from, so a fresh start that silently resumed the last incomplete run was indistinguishable from a Retry told to resume. Adapters whose cursor encodes scope had no basis to refuse the first without breaking the second. Adds CursorOrigin ('none' | 'explicit' | 'inherited' | 'self') as an optional field on StreamImportInput/StreamExportInput, two nullable columns on sync_runs recording the start-time origin and its source run, and resolveStartCursorWithOrigin / resolveResumeCursorWithSource. The previous resolvers stay as delegating wrappers, so no signature changes. The engine derives what it is actually handing over rather than replaying the stored label: once batchesCompleted > 0 the cursor is the adapter's own output, so a redelivery reports 'self'. Without that, an adapter refusing 'inherited' would reject its own mid-run resume. Retry is labelled per case, not wholesale. Resuming the previous run's own position is 'explicit', but retrying a run that never committed a batch falls through to the same inherited resolution a dashboard start uses, and is labelled 'inherited' accordingly. Behaviour is unchanged; adapters ignoring the field see what they see today, as do runs written before the migration. Co-Authored-By: Claude Opus 5 (1M context) --- .../data_sync/api/__tests__/run.test.ts | 33 ++++++-- .../core/src/modules/data_sync/api/run.ts | 15 ++-- .../modules/data_sync/api/runs/[id]/retry.ts | 33 +++++--- .../src/modules/data_sync/data/entities.ts | 22 ++++- .../lib/__tests__/start-cursor.test.ts | 80 ++++++++++++++++++- .../core/src/modules/data_sync/lib/adapter.ts | 39 +++++++++ .../modules/data_sync/lib/cursor-origin.ts | 26 ++++++ .../src/modules/data_sync/lib/start-cursor.ts | 54 ++++++++++--- .../src/modules/data_sync/lib/start-run.ts | 15 ++++ .../src/modules/data_sync/lib/sync-engine.ts | 3 + .../modules/data_sync/lib/sync-run-service.ts | 30 ++++++- .../migrations/.snapshot-open-mercato.json | 32 ++++++++ .../migrations/Migration20260831120000.ts | 15 ++++ .../data_sync/workers/sync-scheduled.ts | 12 +-- 14 files changed, 365 insertions(+), 44 deletions(-) create mode 100644 packages/core/src/modules/data_sync/lib/cursor-origin.ts create mode 100644 packages/core/src/modules/data_sync/migrations/Migration20260831120000.ts diff --git a/packages/core/src/modules/data_sync/api/__tests__/run.test.ts b/packages/core/src/modules/data_sync/api/__tests__/run.test.ts index 1233c29d7d6..80ef1e53db0 100644 --- a/packages/core/src/modules/data_sync/api/__tests__/run.test.ts +++ b/packages/core/src/modules/data_sync/api/__tests__/run.test.ts @@ -10,6 +10,7 @@ const mockSyncRunService = { findRunningOverlap: jest.fn(), resolveCursor: jest.fn(), resolveResumeCursor: jest.fn(), + resolveResumeCursorWithSource: jest.fn(), } const mockProgressService = {} @@ -88,6 +89,7 @@ describe('data_sync run route', () => { mockSyncRunService.findRunningOverlap.mockResolvedValue(null) mockSyncRunService.resolveCursor.mockResolvedValue(null) mockSyncRunService.resolveResumeCursor.mockResolvedValue(null) + mockSyncRunService.resolveResumeCursorWithSource.mockResolvedValue({ cursor: null, runId: null }) mockStartDataSyncRun.mockResolvedValue({ run: { id: '11111111-1111-4111-8111-111111111111' }, progressJob: { id: '22222222-2222-4222-8222-222222222222' }, @@ -158,9 +160,15 @@ describe('data_sync run route', () => { }), })) - expect(mockSyncRunService.resolveResumeCursor).not.toHaveBeenCalled() + expect(mockSyncRunService.resolveResumeCursorWithSource).not.toHaveBeenCalled() expect(mockStartDataSyncRun).toHaveBeenCalledWith(expect.objectContaining({ - input: expect.objectContaining({ cursor: 'shared-cursor' }), + input: expect.objectContaining({ + cursor: 'shared-cursor', + // The shared row has no run id, which is how the UI later tells "continuing the saved + // incremental cursor" from "continuing run X". + cursorOrigin: 'inherited', + cursorSourceRunId: null, + }), })) }) @@ -172,7 +180,10 @@ describe('data_sync run route', () => { supportedEntities: ['customers.person'], persistsSharedCursor: (entityType: string) => entityType !== 'customers.person', }) - mockSyncRunService.resolveResumeCursor.mockResolvedValueOnce('interrupted-run-cursor') + mockSyncRunService.resolveResumeCursorWithSource.mockResolvedValueOnce({ + cursor: 'interrupted-run-cursor', + runId: '33333333-3333-4333-8333-333333333333', + }) await postHandler(new Request('http://localhost/api/data_sync/run', { method: 'POST', @@ -185,7 +196,13 @@ describe('data_sync run route', () => { expect(mockSyncRunService.resolveCursor).not.toHaveBeenCalled() expect(mockStartDataSyncRun).toHaveBeenCalledWith(expect.objectContaining({ - input: expect.objectContaining({ cursor: 'interrupted-run-cursor' }), + input: expect.objectContaining({ + cursor: 'interrupted-run-cursor', + // The operator asked for a fresh run and got a stranger run's position. Recording that is + // the whole point: an adapter whose cursor encodes scope can refuse it. + cursorOrigin: 'inherited', + cursorSourceRunId: '33333333-3333-4333-8333-333333333333', + }), })) }) @@ -209,9 +226,13 @@ describe('data_sync run route', () => { })) expect(mockSyncRunService.resolveCursor).not.toHaveBeenCalled() - expect(mockSyncRunService.resolveResumeCursor).not.toHaveBeenCalled() + expect(mockSyncRunService.resolveResumeCursorWithSource).not.toHaveBeenCalled() expect(mockStartDataSyncRun).toHaveBeenCalledWith(expect.objectContaining({ - input: expect.objectContaining({ cursor: null }), + input: expect.objectContaining({ + cursor: null, + cursorOrigin: 'none', + cursorSourceRunId: null, + }), })) }) diff --git a/packages/core/src/modules/data_sync/api/run.ts b/packages/core/src/modules/data_sync/api/run.ts index aab20e365ef..71c378648ba 100644 --- a/packages/core/src/modules/data_sync/api/run.ts +++ b/packages/core/src/modules/data_sync/api/run.ts @@ -11,7 +11,7 @@ import { runSyncSchema } from '../data/validators' import { startDataSyncRun } from '../lib/start-run' import { getDataSyncAdapter } from '../lib/adapter-registry' import { normalizeRunParameters } from '../lib/run-parameters' -import { resolveStartCursor } from '../lib/start-cursor' +import { type ResolvedStartCursor, resolveStartCursorWithOrigin } from '../lib/start-cursor' import { runCrudMutationGuardAfterSuccess, validateCrudMutationGuard, @@ -122,9 +122,12 @@ export async function POST(req: Request) { return NextResponse.json(guardResult.body, { status: guardResult.status }) } - const cursor = parsed.data.fullSync - ? null - : await resolveStartCursor({ + // A non-full start reuses whatever position prior state left behind, which is the intended + // incremental behaviour but is invisible to the operator who pressed Run. Record where the + // cursor came from so the adapter can judge it and the run detail page can explain it. + const startCursor: ResolvedStartCursor = parsed.data.fullSync + ? { cursor: null, origin: 'none', sourceRunId: null } + : await resolveStartCursorWithOrigin({ syncRunService, adapter, integrationId: parsed.data.integrationId, @@ -144,7 +147,9 @@ export async function POST(req: Request) { integrationId: parsed.data.integrationId, entityType: parsed.data.entityType, direction: parsed.data.direction, - cursor, + cursor: startCursor.cursor, + cursorOrigin: startCursor.origin, + cursorSourceRunId: startCursor.sourceRunId, triggeredBy: parsed.data.triggeredBy ?? auth.sub, batchSize: parsed.data.batchSize, parameters: Object.keys(normalizedParameters.values).length > 0 diff --git a/packages/core/src/modules/data_sync/api/runs/[id]/retry.ts b/packages/core/src/modules/data_sync/api/runs/[id]/retry.ts index 90c13ea6e52..dbe8168d2db 100644 --- a/packages/core/src/modules/data_sync/api/runs/[id]/retry.ts +++ b/packages/core/src/modules/data_sync/api/runs/[id]/retry.ts @@ -9,7 +9,7 @@ import type { SyncRunService } from '../../../lib/sync-run-service' import { retrySyncSchema } from '../../../data/validators' import { startDataSyncRun } from '../../../lib/start-run' import { normalizeRunParameters } from '../../../lib/run-parameters' -import { resolveAdapterForIntegration, resolveStartCursor } from '../../../lib/start-cursor' +import { type ResolvedStartCursor, resolveAdapterForIntegration, resolveStartCursorWithOrigin } from '../../../lib/start-cursor' import { runCrudMutationGuardAfterSuccess, validateCrudMutationGuard, @@ -118,16 +118,23 @@ export async function POST(req: Request, ctx: { params?: Promise<{ id?: string } ? normalizedParameters.values : null - const cursor = parsedBody.data.fromBeginning - ? null - : previous.cursor ?? await resolveStartCursor({ - syncRunService, - adapter: retryAdapter, - integrationId: previous.integrationId, - entityType: previous.entityType, - direction: previous.direction, - scope, - }) + // Three cases, and they are genuinely different provenance despite all arriving through "Retry". + // Resuming the previous run's own position is explicit — the operator asked for this run to + // continue that one. But a run that never committed a batch has no position to resume, and the + // fallback below inherits exactly like a fresh dashboard start does. Labelling every retry + // 'explicit' would make the discriminator a second thing to distrust. + const startCursor: ResolvedStartCursor = parsedBody.data.fromBeginning + ? { cursor: null, origin: 'none', sourceRunId: null } + : previous.cursor != null + ? { cursor: previous.cursor, origin: 'explicit', sourceRunId: previous.id } + : await resolveStartCursorWithOrigin({ + syncRunService, + adapter: retryAdapter, + integrationId: previous.integrationId, + entityType: previous.entityType, + direction: previous.direction, + scope, + }) const { run, progressJob } = await startDataSyncRun({ syncRunService, @@ -140,7 +147,9 @@ export async function POST(req: Request, ctx: { params?: Promise<{ id?: string } integrationId: previous.integrationId, entityType: previous.entityType, direction: previous.direction, - cursor, + cursor: startCursor.cursor, + cursorOrigin: startCursor.origin, + cursorSourceRunId: startCursor.sourceRunId, triggeredBy: auth.sub, batchSize: 100, parameters: retryParameters, diff --git a/packages/core/src/modules/data_sync/data/entities.ts b/packages/core/src/modules/data_sync/data/entities.ts index a0634530104..371c8c11246 100644 --- a/packages/core/src/modules/data_sync/data/entities.ts +++ b/packages/core/src/modules/data_sync/data/entities.ts @@ -1,10 +1,11 @@ import { OptionalProps } from '@mikro-orm/core' import { Entity, Index, PrimaryKey, Property } from '@mikro-orm/decorators/legacy' +import type { CursorOrigin } from '../lib/adapter' @Entity({ tableName: 'sync_runs' }) @Index({ properties: ['integrationId', 'entityType', 'status', 'organizationId', 'tenantId'] }) export class SyncRun { - [OptionalProps]?: 'status' | 'cursor' | 'initialCursor' | 'createdCount' | 'updatedCount' | 'skippedCount' | 'failedCount' | 'batchesCompleted' | 'lastError' | 'progressJobId' | 'jobId' | 'triggeredBy' | 'parameters' | 'createdAt' | 'updatedAt' | 'deletedAt' + [OptionalProps]?: 'status' | 'cursor' | 'initialCursor' | 'cursorOrigin' | 'cursorSourceRunId' | 'createdCount' | 'updatedCount' | 'skippedCount' | 'failedCount' | 'batchesCompleted' | 'lastError' | 'progressJobId' | 'jobId' | 'triggeredBy' | 'parameters' | 'createdAt' | 'updatedAt' | 'deletedAt' @PrimaryKey({ type: 'uuid', defaultRaw: 'gen_random_uuid()' }) id!: string @@ -26,6 +27,25 @@ export class SyncRun { @Property({ name: 'initial_cursor', type: 'text', nullable: true }) initialCursor?: string | null + /** + * Where {@link initialCursor} came from. Written once at run creation, never mutated — it is a + * fact about how the run started, not about where it has got to. Null on runs written before + * provenance shipped, which the engine reports to adapters as an absent origin rather than a guess. + */ + @Property({ name: 'cursor_origin', type: 'text', nullable: true }) + cursorOrigin?: CursorOrigin | null + + /** + * The run {@link initialCursor} was taken from, when it came from a run at all. Null for a cursor + * read from the shared `sync_cursors` row and for runs that started from nothing. + * + * Deliberately a bare uuid rather than a foreign key, matching `progress_job_id`: a run row is an + * append-only operational record, and a FK would make run-retention deletion order-dependent for a + * column read only to render a link. + */ + @Property({ name: 'cursor_source_run_id', type: 'uuid', nullable: true }) + cursorSourceRunId?: string | null + @Property({ name: 'created_count', type: 'int', default: 0 }) createdCount: number = 0 diff --git a/packages/core/src/modules/data_sync/lib/__tests__/start-cursor.test.ts b/packages/core/src/modules/data_sync/lib/__tests__/start-cursor.test.ts index 7e78b6b253c..ccc2fe82a28 100644 --- a/packages/core/src/modules/data_sync/lib/__tests__/start-cursor.test.ts +++ b/packages/core/src/modules/data_sync/lib/__tests__/start-cursor.test.ts @@ -10,7 +10,7 @@ jest.mock('@open-mercato/shared/modules/integrations/types', () => ({ })) import { registerDataSyncAdapter } from '../adapter-registry' -import { resolveAdapterForIntegration, resolveStartCursor } from '../start-cursor' +import { resolveAdapterForIntegration, resolveStartCursor, resolveStartCursorWithOrigin } from '../start-cursor' const REGISTRY_KEY = Symbol.for('@open-mercato/data-sync/adapter-registry') const SCOPE = { organizationId: 'org-1', tenantId: 'tenant-1' } @@ -23,6 +23,7 @@ function buildSyncRunService() { return { resolveCursor: jest.fn(async () => 'shared-cursor'), resolveResumeCursor: jest.fn(async () => 'interrupted-run-cursor'), + resolveResumeCursorWithSource: jest.fn(async () => ({ cursor: 'interrupted-run-cursor', runId: 'previous-run-id' })), } as unknown as SyncRunService } @@ -95,7 +96,7 @@ describe('resolveStartCursor for callers that resolve the adapter by integration }) expect(cursor).toBe('shared-cursor') - expect(syncRunService.resolveResumeCursor).not.toHaveBeenCalled() + expect(syncRunService.resolveResumeCursorWithSource).not.toHaveBeenCalled() }) it('resumes from the run row for an entity type that opted out', async () => { @@ -130,6 +131,79 @@ describe('resolveStartCursor for callers that resolve the adapter by integration }) expect(cursor).toBe('shared-cursor') - expect(syncRunService.resolveResumeCursor).not.toHaveBeenCalled() + expect(syncRunService.resolveResumeCursorWithSource).not.toHaveBeenCalled() + }) +}) + +/** + * Provenance is the reason this resolver exists in the shape it does. Both branches inherit a + * position the caller never named, so both report `inherited` — but only the previous-run branch can + * name a run, and that asymmetry is what the run detail page and a scope-encoding adapter read. + */ +describe('resolveStartCursorWithOrigin', () => { + beforeEach(() => { + clearGlobalRegistry() + jest.clearAllMocks() + mockGetIntegration.mockReturnValue({ providerKey: 'backfill-provider' }) + }) + + afterEach(clearGlobalRegistry) + + it('reports a shared-row cursor as inherited with no source run', async () => { + const syncRunService = buildSyncRunService() + registerDataSyncAdapter(buildAdapter({ persistsSharedCursor: () => true })) + + const resolved = await resolveStartCursorWithOrigin({ + syncRunService, + adapter: resolveAdapterForIntegration('sync_backfill'), + integrationId: 'sync_backfill', + entityType: 'catalog.product', + direction: 'import', + scope: SCOPE, + }) + + expect(resolved).toEqual({ cursor: 'shared-cursor', origin: 'inherited', sourceRunId: null }) + }) + + it('reports a resumed cursor as inherited and names the run it came from', async () => { + const syncRunService = buildSyncRunService() + registerDataSyncAdapter(buildAdapter({ + persistsSharedCursor: (entityType: string) => entityType !== 'catalog.product', + })) + + const resolved = await resolveStartCursorWithOrigin({ + syncRunService, + adapter: resolveAdapterForIntegration('sync_backfill'), + integrationId: 'sync_backfill', + entityType: 'catalog.product', + direction: 'import', + scope: SCOPE, + }) + + expect(resolved).toEqual({ + cursor: 'interrupted-run-cursor', + origin: 'inherited', + sourceRunId: 'previous-run-id', + }) + }) + + it('reports no cursor as none rather than inherited', async () => { + const syncRunService = { + resolveCursor: jest.fn(async () => null), + resolveResumeCursor: jest.fn(async () => null), + resolveResumeCursorWithSource: jest.fn(async () => ({ cursor: null, runId: null })), + } as unknown as SyncRunService + registerDataSyncAdapter(buildAdapter({ persistsSharedCursor: () => true })) + + const resolved = await resolveStartCursorWithOrigin({ + syncRunService, + adapter: resolveAdapterForIntegration('sync_backfill'), + integrationId: 'sync_backfill', + entityType: 'catalog.product', + direction: 'import', + scope: SCOPE, + }) + + expect(resolved).toEqual({ cursor: null, origin: 'none', sourceRunId: null }) }) }) diff --git a/packages/core/src/modules/data_sync/lib/adapter.ts b/packages/core/src/modules/data_sync/lib/adapter.ts index ec765e04772..a346d8402e9 100644 --- a/packages/core/src/modules/data_sync/lib/adapter.ts +++ b/packages/core/src/modules/data_sync/lib/adapter.ts @@ -30,6 +30,36 @@ export interface DataMapping { matchField?: string } +/** + * Where the cursor an adapter is being handed actually came from. + * + * A cursor alone cannot answer this. A fresh dashboard start that silently resumed the last + * incomplete run arrives byte-for-byte identical to a Retry that was told to resume, because both + * carry a cursor and the previous run's parameters. For an adapter whose cursor encodes SCOPE — + * filters, id/date bounds, dry-run flags — and not just a position, the difference decides whether + * the run is correct: an inherited cursor imposes a stranger run's window, and the run finishes + * `completed` having skipped everything outside it. + * + * - `none` — no cursor; start from the beginning. + * - `explicit` — the caller supplied this cursor deliberately: a Retry resuming the previous run's + * own position, or a provider flow that computed one. + * - `inherited` — core resolved it from prior state the caller never named: the shared + * `sync_cursors` row, or the last incomplete run. + * - `self` — this run's OWN committed progress, handed back after a queue redelivery. + * + * `self` exists so that refusing an inherited cursor does not also refuse a legitimate resume. The + * engine hands over `run.cursor`, not `run.initialCursor`, so once a batch has committed the cursor + * is the adapter's own output whatever the run started from. Without `self`, an adapter that + * rejected `inherited` would restart from the top on every worker hiccup. + * + * An absent field means a run created before provenance shipped, or a caller that supplied nothing. + * Adapters that ignore this field behave exactly as they do today. + * + * `inherited` does not say WHICH prior state it came from. Read the run's `cursorSourceRunId` for + * that: set for the previous-run case, null when it came from the shared `sync_cursors` row. + */ +export type CursorOrigin = 'none' | 'explicit' | 'inherited' | 'self' + export interface StreamImportInput { entityType: string cursor?: string @@ -66,6 +96,13 @@ export interface StreamImportInput { * aborting here stops the generator, not anything already queued elsewhere. */ signal?: AbortSignal + /** + * Provenance of {@link StreamImportInput.cursor} on THIS delivery. See {@link CursorOrigin}. + * + * Absent when the run predates provenance, so treat it as "unknown" rather than as any particular + * origin. + */ + cursorOrigin?: CursorOrigin } export interface ImportItem { @@ -111,6 +148,8 @@ export interface StreamExportInput { parameters?: Record /** Aborted when the run is cancelled — see {@link StreamImportInput.signal}. */ signal?: AbortSignal + /** Provenance of the cursor on this delivery — see {@link StreamImportInput.cursorOrigin}. */ + cursorOrigin?: CursorOrigin } export interface ExportItemResult { diff --git a/packages/core/src/modules/data_sync/lib/cursor-origin.ts b/packages/core/src/modules/data_sync/lib/cursor-origin.ts new file mode 100644 index 00000000000..805b5ff0fb8 --- /dev/null +++ b/packages/core/src/modules/data_sync/lib/cursor-origin.ts @@ -0,0 +1,26 @@ +import type { CursorOrigin } from './adapter' +import type { SyncRun } from '../data/entities' + +/** + * Provenance of the cursor the engine is about to hand the adapter, which is not always the + * provenance stored on the run row. + * + * The row records where the run STARTED — a fact written once at `createRun` and never mutated. The + * engine hands over `run.cursor`, which every committed batch advances. So once the run has + * committed anything, the cursor is the adapter's own output whatever the run started from, and + * reporting the stored origin would be a lie with teeth: a queue redelivery would present an + * adapter's own resume position as `'inherited'`, and an adapter that refuses inherited cursors + * would restart from the top on every worker hiccup. + * + * `batchesCompleted` is the signal rather than comparing `cursor` against `initialCursor`, for the + * same reason the ownership fence uses it: it advances by construction on every commit, while a + * cursor is a free-form adapter string an adapter may legitimately repeat between batches. + * + * Returns `undefined` for a run written before provenance shipped, so those adapters see exactly the + * absent field they see today rather than an origin nothing actually established. + */ +export function deliveredCursorOrigin(run: Pick): CursorOrigin | undefined { + if (run.cursor == null) return 'none' + if ((run.batchesCompleted ?? 0) > 0) return 'self' + return run.cursorOrigin ?? undefined +} diff --git a/packages/core/src/modules/data_sync/lib/start-cursor.ts b/packages/core/src/modules/data_sync/lib/start-cursor.ts index aa17443f200..509ec4e269e 100644 --- a/packages/core/src/modules/data_sync/lib/start-cursor.ts +++ b/packages/core/src/modules/data_sync/lib/start-cursor.ts @@ -1,4 +1,4 @@ -import type { DataSyncAdapter } from './adapter' +import type { CursorOrigin, DataSyncAdapter } from './adapter' import type { SyncRunService } from './sync-run-service' type SyncScope = { @@ -13,23 +13,57 @@ export function persistsSharedCursor(adapter: DataSyncAdapter | null | undefined } /** - * Start position for a non-full run. Entity types that mirror their cursor into - * the shared `sync_cursors` row read it from there. Entity types whose adapter - * opted out never write that row, so reading it would silently turn every - * incremental run into a full one — they resume from their own last run - * instead. + * A start position together with where it came from. + * + * `origin` is only ever `'none'` or `'inherited'` here: this function resolves positions the caller + * did NOT name, which is precisely what makes them inherited. A caller that supplies its own cursor + * (Retry, a provider flow) labels it `'explicit'` itself and never calls this. + * + * `sourceRunId` is set only for the previous-run branch. The shared `sync_cursors` row has no run id, + * so a null `sourceRunId` on an `'inherited'` cursor means "came from the shared row" — that + * asymmetry is how a caller tells the two inheritance kinds apart without a second discriminator. */ -export async function resolveStartCursor(params: { +export type ResolvedStartCursor = { + cursor: string | null + origin: CursorOrigin + sourceRunId: string | null +} + +type ResolveStartCursorParams = { syncRunService: SyncRunService adapter?: DataSyncAdapter | null integrationId: string entityType: string direction: 'import' | 'export' scope: SyncScope -}): Promise { +} + +/** + * Start position for a non-full run, with its provenance. Entity types that mirror their cursor into + * the shared `sync_cursors` row read it from there. Entity types whose adapter opted out never write + * that row, so reading it would silently turn every incremental run into a full one — they resume + * from their own last run instead. + * + * Either way the position is one the caller never asked for by value, so it is reported as + * `'inherited'`. That label is the whole point: an adapter whose cursor encodes scope can refuse it, + * and the run detail page can tell an operator why a fresh run started mid-table. + */ +export async function resolveStartCursorWithOrigin(params: ResolveStartCursorParams): Promise { const { syncRunService, adapter, integrationId, entityType, direction, scope } = params if (persistsSharedCursor(adapter, entityType)) { - return syncRunService.resolveCursor(integrationId, entityType, direction, scope) + const cursor = await syncRunService.resolveCursor(integrationId, entityType, direction, scope) + return { cursor, origin: cursor === null ? 'none' : 'inherited', sourceRunId: null } } - return syncRunService.resolveResumeCursor(integrationId, entityType, direction, scope) + const { cursor, runId } = await syncRunService.resolveResumeCursorWithSource(integrationId, entityType, direction, scope) + return { cursor, origin: cursor === null ? 'none' : 'inherited', sourceRunId: runId } +} + +/** + * @deprecated Use {@link resolveStartCursorWithOrigin}, which reports where the cursor came from. + * A bare cursor leaves the adapter unable to tell an inherited position from one the caller asked + * for, which is the ambiguity provenance exists to remove. Kept for external callers. + */ +export async function resolveStartCursor(params: ResolveStartCursorParams): Promise { + const { cursor } = await resolveStartCursorWithOrigin(params) + return cursor } diff --git a/packages/core/src/modules/data_sync/lib/start-run.ts b/packages/core/src/modules/data_sync/lib/start-run.ts index 89dcf3b2eb5..fa8c25664ba 100644 --- a/packages/core/src/modules/data_sync/lib/start-run.ts +++ b/packages/core/src/modules/data_sync/lib/start-run.ts @@ -1,4 +1,5 @@ import type { ProgressService } from '../../progress/lib/progressService' +import type { CursorOrigin } from './adapter' import type { SyncRunService } from './sync-run-service' import { getSyncQueue } from './queue' import { DATA_SYNC_EXPORT_QUEUE, DATA_SYNC_IMPORT_QUEUE } from './queue-policy' @@ -14,6 +15,18 @@ export type StartDataSyncRunInput = { entityType: string direction: 'import' | 'export' cursor?: string | null + /** + * Where `cursor` came from. Omitting it means the caller chose the cursor itself, so it defaults to + * `'explicit'` when a cursor is present and `'none'` when it is not. + * + * That default is deliberately the honest one for a direct caller: a provider flow that computed a + * cursor did name it, and inferring `'inherited'` would put a label on a value the caller never + * inherited. Callers that resolve a cursor from prior state MUST pass `'inherited'` themselves — + * `resolveStartCursorWithOrigin` returns exactly what to pass. + */ + cursorOrigin?: CursorOrigin + /** The run `cursor` was inherited from, when it came from a run. See {@link StartDataSyncRunInput.cursorOrigin}. */ + cursorSourceRunId?: string | null triggeredBy?: string | null batchSize?: number parameters?: Record | null @@ -64,6 +77,8 @@ export async function startDataSyncRun(params: { entityType: input.entityType, direction: input.direction, cursor: input.cursor ?? null, + cursorOrigin: input.cursorOrigin ?? (input.cursor == null ? 'none' : 'explicit'), + cursorSourceRunId: input.cursorSourceRunId ?? null, triggeredBy: input.triggeredBy ?? scope.userId ?? null, parameters: input.parameters ?? null, progressJobId: progressJob?.id ?? null, diff --git a/packages/core/src/modules/data_sync/lib/sync-engine.ts b/packages/core/src/modules/data_sync/lib/sync-engine.ts index ce931832df0..f57d626c9e3 100644 --- a/packages/core/src/modules/data_sync/lib/sync-engine.ts +++ b/packages/core/src/modules/data_sync/lib/sync-engine.ts @@ -8,6 +8,7 @@ import { refreshCoverageSnapshot } from '../../query_index/lib/coverage' import { emitDataSyncEvent } from '../events' import type { DataSyncAdapter, DataMapping, ExportBatch, ImportBatch, RunParameterValue } from './adapter' import { getDataSyncAdapter, resolveProviderKey } from './adapter-registry' +import { deliveredCursorOrigin } from './cursor-origin' import type { SyncRunService } from './sync-run-service' import { SyncRunOwnershipConflictError } from './sync-run-service' import { forEachBatch } from './batch-stream' @@ -619,6 +620,7 @@ export function createSyncEngine(deps: EngineDeps) { adapter.streamImport({ entityType: run.entityType, cursor: run.cursor ?? undefined, + cursorOrigin: deliveredCursorOrigin(run), batchSize, credentials, mapping, @@ -846,6 +848,7 @@ export function createSyncEngine(deps: EngineDeps) { adapter.streamExport({ entityType: run.entityType, cursor: run.cursor ?? undefined, + cursorOrigin: deliveredCursorOrigin(run), batchSize, credentials, mapping, diff --git a/packages/core/src/modules/data_sync/lib/sync-run-service.ts b/packages/core/src/modules/data_sync/lib/sync-run-service.ts index 737119723cf..5a60d3dc68f 100644 --- a/packages/core/src/modules/data_sync/lib/sync-run-service.ts +++ b/packages/core/src/modules/data_sync/lib/sync-run-service.ts @@ -2,6 +2,7 @@ import type { EntityManager, FilterQuery } from '@mikro-orm/postgresql' import { findAndCountWithDecryption, findOneWithDecryption, findWithDecryption } from '@open-mercato/shared/lib/encryption/find' import { withAtomicFlush } from '@open-mercato/shared/lib/commands/flush' import { escapeLikePattern } from '@open-mercato/shared/lib/db/escapeLikePattern' +import type { CursorOrigin } from './adapter' import { SyncCursor, SyncRun } from '../data/entities' const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i @@ -112,6 +113,8 @@ export function createSyncRunService(em: EntityManager) { entityType: string direction: 'import' | 'export' cursor?: string | null + cursorOrigin?: CursorOrigin | null + cursorSourceRunId?: string | null triggeredBy?: string | null progressJobId?: string | null jobId?: string | null @@ -124,6 +127,8 @@ export function createSyncRunService(em: EntityManager) { status: 'pending', cursor: input.cursor, initialCursor: input.cursor, + cursorOrigin: input.cursorOrigin ?? null, + cursorSourceRunId: input.cursorSourceRunId ?? null, triggeredBy: input.triggeredBy, progressJobId: input.progressJobId, jobId: input.jobId, @@ -374,6 +379,26 @@ export function createSyncRunService(em: EntityManager) { * already passed. */ async resolveResumeCursor(integrationId: string, entityType: string, direction: 'import' | 'export', scope: SyncScope): Promise { + const { cursor } = await this.resolveResumeCursorWithSource(integrationId, entityType, direction, scope) + return cursor + }, + + /** + * {@link resolveResumeCursor} plus the id of the run the cursor came from. + * + * The run id is what lets a start path record provenance: a cursor resolved here was inherited + * from a specific earlier run that the operator never named, and naming it is the difference + * between "this run started mid-table for no visible reason" and "this run is continuing run X". + * + * `runId` is null exactly when `cursor` is null — there is no run to point at when nothing is + * resumable. + */ + async resolveResumeCursorWithSource( + integrationId: string, + entityType: string, + direction: 'import' | 'export', + scope: SyncScope, + ): Promise<{ cursor: string | null; runId: string | null }> { const [run] = await findWithDecryption( em, SyncRun, @@ -388,8 +413,9 @@ export function createSyncRunService(em: EntityManager) { { orderBy: { createdAt: 'DESC' }, limit: 1 }, scope, ) - if (!run || run.status === 'completed') return null - return run.cursor ?? null + if (!run || run.status === 'completed') return { cursor: null, runId: null } + const cursor = run.cursor ?? null + return { cursor, runId: cursor === null ? null : run.id } }, /** diff --git a/packages/core/src/modules/data_sync/migrations/.snapshot-open-mercato.json b/packages/core/src/modules/data_sync/migrations/.snapshot-open-mercato.json index 71b1206c684..3c94601ca96 100644 --- a/packages/core/src/modules/data_sync/migrations/.snapshot-open-mercato.json +++ b/packages/core/src/modules/data_sync/migrations/.snapshot-open-mercato.json @@ -684,6 +684,38 @@ "comment": null, "enumItems": [], "mappedType": "json" + }, + "cursor_origin": { + "name": "cursor_origin", + "type": "text", + "unsigned": false, + "autoincrement": false, + "primary": false, + "nullable": true, + "unique": false, + "length": null, + "precision": null, + "scale": null, + "default": null, + "comment": null, + "enumItems": [], + "mappedType": "text" + }, + "cursor_source_run_id": { + "name": "cursor_source_run_id", + "type": "uuid", + "unsigned": false, + "autoincrement": false, + "primary": false, + "nullable": true, + "unique": false, + "length": null, + "precision": null, + "scale": null, + "default": null, + "comment": null, + "enumItems": [], + "mappedType": "uuid" } }, "indexes": [ diff --git a/packages/core/src/modules/data_sync/migrations/Migration20260831120000.ts b/packages/core/src/modules/data_sync/migrations/Migration20260831120000.ts new file mode 100644 index 00000000000..9c215876738 --- /dev/null +++ b/packages/core/src/modules/data_sync/migrations/Migration20260831120000.ts @@ -0,0 +1,15 @@ +import { Migration } from '@mikro-orm/migrations'; + +export class Migration20260831120000 extends Migration { + + override async up(): Promise { + this.addSql(`alter table "sync_runs" add column "cursor_origin" text null;`); + this.addSql(`alter table "sync_runs" add column "cursor_source_run_id" uuid null;`); + } + + override async down(): Promise { + this.addSql(`alter table "sync_runs" drop column "cursor_source_run_id";`); + this.addSql(`alter table "sync_runs" drop column "cursor_origin";`); + } + +} diff --git a/packages/core/src/modules/data_sync/workers/sync-scheduled.ts b/packages/core/src/modules/data_sync/workers/sync-scheduled.ts index 2b33eeaa4a9..7368761703e 100644 --- a/packages/core/src/modules/data_sync/workers/sync-scheduled.ts +++ b/packages/core/src/modules/data_sync/workers/sync-scheduled.ts @@ -6,7 +6,7 @@ import type { ProgressService } from '../../progress/lib/progressService' import type { SyncRunService } from '../lib/sync-run-service' import { SyncSchedule } from '../data/entities' import { startDataSyncRun } from '../lib/start-run' -import { resolveAdapterForIntegration, resolveStartCursor } from '../lib/start-cursor' +import { type ResolvedStartCursor, resolveAdapterForIntegration, resolveStartCursorWithOrigin } from '../lib/start-cursor' import { normalizeRunParameters } from '../lib/run-parameters' import { createLogger } from '@open-mercato/shared/lib/logger' @@ -70,9 +70,9 @@ export default async function handle(job: QueuedJob, ctx: const adapter = resolveAdapterForIntegration(schedule.integrationId) - const cursor = schedule.fullSync - ? null - : await resolveStartCursor({ + const startCursor: ResolvedStartCursor = schedule.fullSync + ? { cursor: null, origin: 'none', sourceRunId: null } + : await resolveStartCursorWithOrigin({ syncRunService, adapter, integrationId: schedule.integrationId, @@ -116,7 +116,9 @@ export default async function handle(job: QueuedJob, ctx: integrationId: schedule.integrationId, entityType: schedule.entityType, direction: schedule.direction, - cursor, + cursor: startCursor.cursor, + cursorOrigin: startCursor.origin, + cursorSourceRunId: startCursor.sourceRunId, triggeredBy: 'scheduler', parameters, }, From 7f9810e13001bd1d05b7dab99748bae8e6e9923a Mon Sep 17 00:00:00 2001 From: Maksymilian Gala Date: Mon, 31 Aug 2026 17:49:52 +0200 Subject: [PATCH 3/5] feat(data_sync): show why a run started mid-table on the run detail page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The run detail page rendered neither cursor nor initial cursor, so an operator who started a run and watched it process a fraction of the table had no way to find out why. Cursor provenance makes that answerable, so answer it. Both read routes now return cursorOrigin and cursorSourceRunId, and the detail page renders one line for an inherited start: "continued where an earlier run stopped" with a link to that run, or "continued from the saved incremental position" when it came from the shared cursor row and there is no run to point at. Stays quiet for 'none', 'explicit' and 'self' — a note shown on every run is one every operator learns to skip. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/src/modules/data_sync/api/runs.ts | 2 + .../modules/data_sync/api/runs/[id]/route.ts | 2 + .../runs/[id]/__tests__/page.test.tsx | 42 ++++++++++++++++++- .../backend/data-sync/runs/[id]/page.tsx | 27 ++++++++++++ .../core/src/modules/data_sync/i18n/de.json | 4 ++ .../core/src/modules/data_sync/i18n/en.json | 4 ++ .../core/src/modules/data_sync/i18n/es.json | 4 ++ .../core/src/modules/data_sync/i18n/ko.json | 4 ++ .../core/src/modules/data_sync/i18n/pl.json | 4 ++ 9 files changed, 91 insertions(+), 2 deletions(-) diff --git a/packages/core/src/modules/data_sync/api/runs.ts b/packages/core/src/modules/data_sync/api/runs.ts index d33536805ed..f7d1a323822 100644 --- a/packages/core/src/modules/data_sync/api/runs.ts +++ b/packages/core/src/modules/data_sync/api/runs.ts @@ -56,6 +56,8 @@ export async function GET(req: Request) { status: item.status, cursor: item.cursor ?? null, initialCursor: item.initialCursor ?? null, + cursorOrigin: item.cursorOrigin ?? null, + cursorSourceRunId: item.cursorSourceRunId ?? null, createdCount: item.createdCount, updatedCount: item.updatedCount, skippedCount: item.skippedCount, diff --git a/packages/core/src/modules/data_sync/api/runs/[id]/route.ts b/packages/core/src/modules/data_sync/api/runs/[id]/route.ts index 956e0f6f9f7..e76f1ecd3ec 100644 --- a/packages/core/src/modules/data_sync/api/runs/[id]/route.ts +++ b/packages/core/src/modules/data_sync/api/runs/[id]/route.ts @@ -62,6 +62,8 @@ export async function GET(req: Request, ctx: { params?: Promise<{ id?: string }> status: run.status, cursor: run.cursor ?? null, initialCursor: run.initialCursor ?? null, + cursorOrigin: run.cursorOrigin ?? null, + cursorSourceRunId: run.cursorSourceRunId ?? null, createdCount: run.createdCount, updatedCount: run.updatedCount, skippedCount: run.skippedCount, diff --git a/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/__tests__/page.test.tsx b/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/__tests__/page.test.tsx index c200d2acb45..6592adf87c9 100644 --- a/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/__tests__/page.test.tsx +++ b/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/__tests__/page.test.tsx @@ -55,6 +55,8 @@ const runFixture = { batchesCompleted: 4, lastError: null, progressJobId: null, + cursorOrigin: null, + cursorSourceRunId: null, progressJob: null, triggeredBy: null, createdAt: '2026-01-01T00:00:00.000Z', @@ -65,10 +67,10 @@ function logsUrlPage(url: string): string | null { return new URL(url, 'http://localhost').searchParams.get('page') } -function mockApiResponses(total: number) { +function mockApiResponses(total: number, runOverrides: Partial = {}) { apiCallMock.mockImplementation(async (url: string) => { if (url.startsWith('/api/data_sync/runs/')) { - return { ok: true, status: 200, result: runFixture } + return { ok: true, status: 200, result: { ...runFixture, ...runOverrides } } } if (url.startsWith('/api/integrations/logs')) { return { ok: true, status: 200, result: { items: [], total } } @@ -203,3 +205,39 @@ describe('SyncRunDetailPage log payload rendering', () => { expect(await screen.findByText(/"createdCount"/)).toBeInTheDocument() }) }) + +/** + * The operator half of cursor provenance. A run that silently started mid-table is the symptom the + * whole change exists to explain, so the page has to say so — and stay quiet when there is nothing + * to explain, or the note becomes noise every operator learns to skip. + */ +describe('SyncRunDetailPage start position', () => { + it('names the run an inherited cursor came from, and links to it', async () => { + mockApiResponses(0, { cursorOrigin: 'inherited', cursorSourceRunId: 'run-earlier' }) + renderWithProviders() + + expect(await screen.findByText(/continued where an earlier run stopped/i)).toBeInTheDocument() + const link = screen.getByRole('link', { name: /view that run/i }) + expect(link).toHaveAttribute('href', '/backend/data-sync/runs/run-earlier') + }) + + it('describes a shared-cursor start without inventing a source run to link to', async () => { + mockApiResponses(0, { cursorOrigin: 'inherited', cursorSourceRunId: null }) + renderWithProviders() + + expect(await screen.findByText(/saved incremental position/i)).toBeInTheDocument() + expect(screen.queryByRole('link', { name: /view that run/i })).not.toBeInTheDocument() + }) + + it.each(['none', 'explicit', 'self', null] as const)( + 'says nothing when the start position needs no explanation (%s)', + async (cursorOrigin) => { + mockApiResponses(0, { cursorOrigin, cursorSourceRunId: null }) + renderWithProviders() + + // Wait for the run to land so this is an assertion about the rendered page, not about timing. + await screen.findByText(/example_orders/) + expect(screen.queryByText(/rather than starting from the beginning/i)).not.toBeInTheDocument() + }, + ) +}) diff --git a/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/page.tsx b/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/page.tsx index 1d46d8eb1a3..f9aba275c07 100644 --- a/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/page.tsx +++ b/packages/core/src/modules/data_sync/backend/data-sync/runs/[id]/page.tsx @@ -44,6 +44,8 @@ type SyncRunDetail = { batchesCompleted: number lastError: string | null progressJobId: string | null + cursorOrigin: 'none' | 'explicit' | 'inherited' | 'self' | null + cursorSourceRunId: string | null parameters: Record | null progressJob: { id: string @@ -444,6 +446,31 @@ export default function SyncRunDetailPage({ params }: SyncRunDetailPageProps) { + {run.cursorOrigin === 'inherited' ? ( + + + {t('data_sync.runs.detail.cursorOrigin.title', 'Start position')} + + +

+ {run.cursorSourceRunId ? ( + <> + {t('data_sync.runs.detail.cursorOrigin.continuingRun', 'This run continued where an earlier run stopped, rather than starting from the beginning.')}{' '} + + {t('data_sync.runs.detail.cursorOrigin.viewSourceRun', 'View that run')} + + + ) : ( + t('data_sync.runs.detail.cursorOrigin.continuingSharedCursor', 'This run continued from the saved incremental position for this integration, rather than starting from the beginning.') + )} +

+
+
+ ) : null} + {run.parameters && Object.keys(run.parameters).length > 0 ? ( diff --git a/packages/core/src/modules/data_sync/i18n/de.json b/packages/core/src/modules/data_sync/i18n/de.json index 726a1b79fa7..9716da647ba 100644 --- a/packages/core/src/modules/data_sync/i18n/de.json +++ b/packages/core/src/modules/data_sync/i18n/de.json @@ -112,6 +112,10 @@ "data_sync.runs.detail.counters.failed": "Fehlgeschlagen", "data_sync.runs.detail.counters.skipped": "Übersprungen", "data_sync.runs.detail.counters.updated": "Aktualisiert", + "data_sync.runs.detail.cursorOrigin.continuingRun": "Dieser Lauf wurde dort fortgesetzt, wo ein früherer Lauf gestoppt wurde, statt von vorne zu beginnen.", + "data_sync.runs.detail.cursorOrigin.continuingSharedCursor": "Dieser Lauf wurde von der gespeicherten inkrementellen Position dieser Integration fortgesetzt, statt von vorne zu beginnen.", + "data_sync.runs.detail.cursorOrigin.title": "Startposition", + "data_sync.runs.detail.cursorOrigin.viewSourceRun": "Diesen Lauf anzeigen", "data_sync.runs.detail.error": "Fehler", "data_sync.runs.detail.loadError": "Lauf konnte nicht geladen werden", "data_sync.runs.detail.logs": "Betriebsprotokolle", diff --git a/packages/core/src/modules/data_sync/i18n/en.json b/packages/core/src/modules/data_sync/i18n/en.json index f5bf022f02a..d206b8537ef 100644 --- a/packages/core/src/modules/data_sync/i18n/en.json +++ b/packages/core/src/modules/data_sync/i18n/en.json @@ -112,6 +112,10 @@ "data_sync.runs.detail.counters.failed": "Failed", "data_sync.runs.detail.counters.skipped": "Skipped", "data_sync.runs.detail.counters.updated": "Updated", + "data_sync.runs.detail.cursorOrigin.continuingRun": "This run continued where an earlier run stopped, rather than starting from the beginning.", + "data_sync.runs.detail.cursorOrigin.continuingSharedCursor": "This run continued from the saved incremental position for this integration, rather than starting from the beginning.", + "data_sync.runs.detail.cursorOrigin.title": "Start position", + "data_sync.runs.detail.cursorOrigin.viewSourceRun": "View that run", "data_sync.runs.detail.error": "Error", "data_sync.runs.detail.loadError": "Failed to load sync run", "data_sync.runs.detail.logs": "Operation Logs", diff --git a/packages/core/src/modules/data_sync/i18n/es.json b/packages/core/src/modules/data_sync/i18n/es.json index 3ca503aa3be..e700f11a568 100644 --- a/packages/core/src/modules/data_sync/i18n/es.json +++ b/packages/core/src/modules/data_sync/i18n/es.json @@ -112,6 +112,10 @@ "data_sync.runs.detail.counters.failed": "Fallidos", "data_sync.runs.detail.counters.skipped": "Omitidos", "data_sync.runs.detail.counters.updated": "Actualizados", + "data_sync.runs.detail.cursorOrigin.continuingRun": "Esta ejecución continuó donde se detuvo una ejecución anterior, en lugar de empezar desde el principio.", + "data_sync.runs.detail.cursorOrigin.continuingSharedCursor": "Esta ejecución continuó desde la posición incremental guardada de esta integración, en lugar de empezar desde el principio.", + "data_sync.runs.detail.cursorOrigin.title": "Posición inicial", + "data_sync.runs.detail.cursorOrigin.viewSourceRun": "Ver esa ejecución", "data_sync.runs.detail.error": "Error", "data_sync.runs.detail.loadError": "No se pudo cargar la ejecución", "data_sync.runs.detail.logs": "Registros de operación", diff --git a/packages/core/src/modules/data_sync/i18n/ko.json b/packages/core/src/modules/data_sync/i18n/ko.json index c3ae31cf3fa..32667a0eaeb 100644 --- a/packages/core/src/modules/data_sync/i18n/ko.json +++ b/packages/core/src/modules/data_sync/i18n/ko.json @@ -112,6 +112,10 @@ "data_sync.runs.detail.counters.failed": "실패", "data_sync.runs.detail.counters.skipped": "건너뜀", "data_sync.runs.detail.counters.updated": "업데이트됨", + "data_sync.runs.detail.cursorOrigin.continuingRun": "이 실행은 처음부터 시작하지 않고 이전 실행이 중단된 지점에서 이어졌습니다.", + "data_sync.runs.detail.cursorOrigin.continuingSharedCursor": "이 실행은 처음부터 시작하지 않고 이 연동에 저장된 증분 위치에서 이어졌습니다.", + "data_sync.runs.detail.cursorOrigin.title": "시작 위치", + "data_sync.runs.detail.cursorOrigin.viewSourceRun": "해당 실행 보기", "data_sync.runs.detail.error": "오류", "data_sync.runs.detail.loadError": "동기화 실행을 불러오지 못했습니다", "data_sync.runs.detail.logs": "작업 로그", diff --git a/packages/core/src/modules/data_sync/i18n/pl.json b/packages/core/src/modules/data_sync/i18n/pl.json index 044408fa63d..15a98e172de 100644 --- a/packages/core/src/modules/data_sync/i18n/pl.json +++ b/packages/core/src/modules/data_sync/i18n/pl.json @@ -112,6 +112,10 @@ "data_sync.runs.detail.counters.failed": "Nieudane", "data_sync.runs.detail.counters.skipped": "Pominięte", "data_sync.runs.detail.counters.updated": "Zaktualizowane", + "data_sync.runs.detail.cursorOrigin.continuingRun": "Ten przebieg kontynuował pracę od miejsca, w którym zatrzymał się wcześniejszy przebieg, zamiast zaczynać od początku.", + "data_sync.runs.detail.cursorOrigin.continuingSharedCursor": "Ten przebieg kontynuował pracę od zapisanej pozycji przyrostowej tej integracji, zamiast zaczynać od początku.", + "data_sync.runs.detail.cursorOrigin.title": "Pozycja początkowa", + "data_sync.runs.detail.cursorOrigin.viewSourceRun": "Zobacz tamten przebieg", "data_sync.runs.detail.error": "Błąd", "data_sync.runs.detail.loadError": "Nie udało się załadować przebiegu", "data_sync.runs.detail.logs": "Logi operacji", From 6c3097ddf0bcd6a9e9ce01dd102fc80f52a2319b Mon Sep 17 00:00:00 2001 From: Maksymilian Gala Date: Mon, 31 Aug 2026 17:53:17 +0200 Subject: [PATCH 4/5] test(data_sync): cover cursor provenance across resolution, engine and retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins the derivation that makes the discriminator safe to act on: once a run has committed a batch the engine reports 'self', so an adapter that refuses an inherited cursor does not refuse its own redelivered resume. Also covers the retry cursor precedence, which had no test at all before this change — including the fallback where a retry of a run that never committed a batch inherits like a fresh start. Adds TC-DS-011 for the wire contract on both run read APIs. Co-Authored-By: Claude Opus 5 (1M context) --- .../__integration__/TC-DS-011.spec.ts | 200 ++++++++++++++++ .../__tests__/retry-cursor-origin.test.ts | 203 +++++++++++++++++ .../lib/__tests__/cursor-origin.test.ts | 50 ++++ .../sync-engine-cursor-origin.test.ts | 214 ++++++++++++++++++ .../sync-run-service.shared-cursor.test.ts | 61 +++++ .../workers/__tests__/sync-scheduled.test.ts | 8 + 6 files changed, 736 insertions(+) create mode 100644 packages/core/src/modules/data_sync/__integration__/TC-DS-011.spec.ts create mode 100644 packages/core/src/modules/data_sync/api/runs/[id]/__tests__/retry-cursor-origin.test.ts create mode 100644 packages/core/src/modules/data_sync/lib/__tests__/cursor-origin.test.ts create mode 100644 packages/core/src/modules/data_sync/lib/__tests__/sync-engine-cursor-origin.test.ts diff --git a/packages/core/src/modules/data_sync/__integration__/TC-DS-011.spec.ts b/packages/core/src/modules/data_sync/__integration__/TC-DS-011.spec.ts new file mode 100644 index 00000000000..c99afaa5479 --- /dev/null +++ b/packages/core/src/modules/data_sync/__integration__/TC-DS-011.spec.ts @@ -0,0 +1,200 @@ +import { expect, test, type APIResponse } from '@playwright/test' +import { apiRequest, getAuthToken } from '@open-mercato/core/modules/core/__integration__/helpers/api' +import { readJsonSafe } from '@open-mercato/core/modules/core/__integration__/helpers/crmFixtures' + +type JsonRecord = Record + +const CURSOR_ORIGINS = ['none', 'explicit', 'inherited', 'self'] + +async function readJson(response: APIResponse): Promise { + return ((await readJsonSafe(response)) ?? {}) as JsonRecord +} + +async function detectSyncableIntegration( + request: Parameters[0], + token: string, +): Promise<{ integrationId: string; entityType: string } | null> { + const listResponse = await apiRequest(request, 'GET', '/api/data_sync/options', { token }) + if (listResponse.status() !== 200) return null + const listBody = await readJson(listResponse) + const items = Array.isArray(listBody.items) ? (listBody.items as JsonRecord[]) : [] + const runnable = items.filter((item) => item.canStartRun !== false) + if (runnable.length === 0) return null + const selected = runnable[0] + const supportedEntities = Array.isArray(selected.supportedEntities) + ? (selected.supportedEntities as unknown[]).filter((value): value is string => typeof value === 'string') + : [] + if (supportedEntities.length === 0) return null + return { integrationId: String(selected.integrationId), entityType: supportedEntities[0] } +} + +/** + * TC-DS-011: Cursor provenance on sync runs + * + * A run that silently inherited a previous run's position was indistinguishable from one told to + * resume, both to the adapter and to the operator. These assertions pin the wire contract that fixes + * that: `cursorOrigin` and `cursorSourceRunId` on the run read APIs. + * + * Self-contained: creates its own runs, restores the integration's credentials and enabled state, + * and cancels every run it started. + */ +test.describe('TC-DS-011: Cursor provenance on sync runs', () => { + test('run detail and list report where the run start position came from', async ({ request }) => { + const token = await getAuthToken(request, 'admin') + + const target = await detectSyncableIntegration(request, token) + if (!target) { + test.skip(true, 'No generic-start data sync provider modules registered — skipping cursor provenance test') + return + } + + const { integrationId, entityType } = target + const createdRunIds: string[] = [] + + const credentialsResponse = await apiRequest(request, 'GET', `/api/integrations/${integrationId}/credentials`, { token }) + expect(credentialsResponse.status()).toBe(200) + const credentialsBody = await readJson(credentialsResponse) + const previousCredentials = credentialsBody.credentials && typeof credentialsBody.credentials === 'object' + ? (credentialsBody.credentials as JsonRecord) + : {} + + const integrationResponse = await apiRequest(request, 'GET', `/api/integrations/${integrationId}`, { token }) + expect(integrationResponse.status()).toBe(200) + const integrationBody = await readJson(integrationResponse) + const baselineState = integrationBody.state && typeof integrationBody.state === 'object' + ? (integrationBody.state as JsonRecord) + : {} + + await apiRequest(request, 'PUT', `/api/integrations/${integrationId}/credentials`, { + token, + data: { credentials: { testApiUrl: 'https://example.test.local', testApiKey: 'integration-test-key' } }, + }) + await apiRequest(request, 'PUT', `/api/integrations/${integrationId}/state`, { + token, + data: { isEnabled: true }, + }) + + try { + // A full sync explicitly refuses any inherited position, which is the one origin this test can + // pin exactly without knowing what prior state the instance carries. + const runResponse = await apiRequest(request, 'POST', '/api/data_sync/run', { + token, + data: { integrationId, entityType, direction: 'import', fullSync: true }, + }) + expect( + runResponse.status(), + `Expected 201 from /api/data_sync/run, got ${runResponse.status()}: ${(await runResponse.text()).slice(0, 2000)}`, + ).toBe(201) + + const runBody = await readJson(runResponse) + const runId = String(runBody.id) + expect(runId).not.toHaveLength(0) + createdRunIds.push(runId) + + const detailResponse = await apiRequest(request, 'GET', `/api/data_sync/runs/${runId}`, { token }) + expect(detailResponse.status()).toBe(200) + const detail = await readJson(detailResponse) + + expect(detail).toHaveProperty('cursorOrigin') + expect(detail).toHaveProperty('cursorSourceRunId') + expect(detail.cursorOrigin).toBe('none') + expect(detail.cursorSourceRunId).toBeNull() + + const listResponse = await apiRequest(request, 'GET', '/api/data_sync/runs?page=1&pageSize=50', { token }) + expect(listResponse.status()).toBe(200) + const listBody = await readJson(listResponse) + const items = Array.isArray(listBody.items) ? (listBody.items as JsonRecord[]) : [] + const listed = items.find((item) => item.id === runId) + expect(listed, 'the run just started should appear in the runs list').toBeTruthy() + expect(listed).toHaveProperty('cursorOrigin') + expect(listed).toHaveProperty('cursorSourceRunId') + + // Every run the instance has ever recorded must carry a value the contract allows. A null is + // permitted only for rows written before provenance shipped. + for (const item of items) { + if (item.cursorOrigin !== null && item.cursorOrigin !== undefined) { + expect(CURSOR_ORIGINS).toContain(item.cursorOrigin) + } + if (item.cursorSourceRunId !== null && item.cursorSourceRunId !== undefined) { + expect(typeof item.cursorSourceRunId).toBe('string') + } + } + } finally { + for (const runId of createdRunIds) { + await apiRequest(request, 'POST', `/api/data_sync/runs/${runId}/cancel`, { token }) + } + await apiRequest(request, 'PUT', `/api/integrations/${integrationId}/credentials`, { + token, + data: { credentials: previousCredentials }, + }) + await apiRequest(request, 'PUT', `/api/integrations/${integrationId}/state`, { + token, + data: { isEnabled: typeof baselineState.isEnabled === 'boolean' ? baselineState.isEnabled : false }, + }) + } + }) + + test('a retry that resumes the previous run records it as an explicit continuation', async ({ request }) => { + const token = await getAuthToken(request, 'admin') + + const target = await detectSyncableIntegration(request, token) + if (!target) { + test.skip(true, 'No generic-start data sync provider modules registered — skipping cursor provenance retry test') + return + } + + const { integrationId, entityType } = target + const createdRunIds: string[] = [] + + try { + const runResponse = await apiRequest(request, 'POST', '/api/data_sync/run', { + token, + data: { integrationId, entityType, direction: 'import', fullSync: true }, + }) + if (runResponse.status() !== 201) { + test.skip(true, `Could not start a run to retry (status ${runResponse.status()})`) + return + } + const runId = String((await readJson(runResponse)).id) + createdRunIds.push(runId) + + // A retry is only accepted for a failed or cancelled run, so cancel this one first. + const cancelResponse = await apiRequest(request, 'POST', `/api/data_sync/runs/${runId}/cancel`, { token }) + if (cancelResponse.status() !== 200) { + test.skip(true, `Could not cancel the run to retry it (status ${cancelResponse.status()})`) + return + } + + const retryResponse = await apiRequest(request, 'POST', `/api/data_sync/runs/${runId}/retry`, { + token, + data: { fromBeginning: false }, + }) + if (retryResponse.status() !== 201) { + test.skip(true, `Retry not accepted in this environment (status ${retryResponse.status()})`) + return + } + const retryId = String((await readJson(retryResponse)).id) + createdRunIds.push(retryId) + + const retryDetail = await readJson( + await apiRequest(request, 'GET', `/api/data_sync/runs/${retryId}`, { token }), + ) + + // The cancelled run committed nothing, so there is no position of its own to resume and the + // retry falls back to the shared cursor — the case a "retries are always explicit" reading + // would get wrong. Either way the origin must be one the contract defines, and a named source + // run must be the run actually retried. + expect(CURSOR_ORIGINS).toContain(retryDetail.cursorOrigin) + if (retryDetail.cursorOrigin === 'explicit') { + expect(retryDetail.cursorSourceRunId).toBe(runId) + } + if (retryDetail.cursorOrigin === 'none') { + expect(retryDetail.cursorSourceRunId).toBeNull() + } + } finally { + for (const id of createdRunIds) { + await apiRequest(request, 'POST', `/api/data_sync/runs/${id}/cancel`, { token }) + } + } + }) +}) diff --git a/packages/core/src/modules/data_sync/api/runs/[id]/__tests__/retry-cursor-origin.test.ts b/packages/core/src/modules/data_sync/api/runs/[id]/__tests__/retry-cursor-origin.test.ts new file mode 100644 index 00000000000..9e9c749087c --- /dev/null +++ b/packages/core/src/modules/data_sync/api/runs/[id]/__tests__/retry-cursor-origin.test.ts @@ -0,0 +1,203 @@ +/** @jest-environment node */ + +const mockGetAuthFromRequest = jest.fn() +const mockCreateRequestContainer = jest.fn() +const mockGetIntegration = jest.fn() +const mockGetDataSyncAdapter = jest.fn() +const mockStartDataSyncRun = jest.fn() +const mockReadJsonSafe = jest.fn() + +const mockSyncRunService = { + getRun: jest.fn(), + findRunningOverlap: jest.fn(), + resolveCursor: jest.fn(), + resolveResumeCursor: jest.fn(), + resolveResumeCursorWithSource: jest.fn(), +} + +const mockProgressService = {} + +const mockCrudMutationGuardService = { + validateMutation: jest.fn(), + afterMutationSuccess: jest.fn(), +} + +jest.mock('@open-mercato/shared/lib/auth/server', () => ({ + getAuthFromRequest: jest.fn((request: Request) => mockGetAuthFromRequest(request)), +})) + +jest.mock('@open-mercato/shared/lib/di/container', () => ({ + createRequestContainer: jest.fn(() => mockCreateRequestContainer()), +})) + +jest.mock('@open-mercato/shared/lib/http/readJsonSafe', () => ({ + readJsonSafe: jest.fn(async () => mockReadJsonSafe()), +})) + +jest.mock('@open-mercato/shared/modules/integrations/types', () => ({ + getIntegration: jest.fn((id: string) => mockGetIntegration(id)), +})) + +jest.mock('../../../../lib/adapter-registry', () => ({ + getDataSyncAdapter: jest.fn((providerKey: string) => mockGetDataSyncAdapter(providerKey)), +})) + +// Keep the real `resolveStartCursorWithOrigin` — the fallback branch is exactly what this suite is +// here to pin down — and stub only the adapter lookup it shares with the route. +jest.mock('../../../../lib/start-cursor', () => ({ + ...jest.requireActual('../../../../lib/start-cursor'), + resolveAdapterForIntegration: jest.fn((integrationId: string) => + mockGetDataSyncAdapter(mockGetIntegration(integrationId)?.providerKey ?? integrationId) ?? null), +})) + +jest.mock('../../../../lib/start-run', () => ({ + startDataSyncRun: jest.fn((input) => mockStartDataSyncRun(input)), +})) + +const RUN_ID = '33333333-3333-4333-8333-333333333333' +const EARLIER_RUN_ID = '55555555-5555-4555-8555-555555555555' + +type RouteModule = typeof import('../retry') +let postHandler: RouteModule['POST'] + +beforeAll(async () => { + const routeModule = await import('../retry') + postHandler = routeModule.POST +}) + +function callRetry() { + const request = new Request(`http://localhost/api/data_sync/runs/${RUN_ID}/retry`, { method: 'POST' }) + return postHandler(request, { params: { id: RUN_ID } } as never) +} + +function startedInput() { + return mockStartDataSyncRun.mock.calls[0][0].input +} + +/** + * Retry is not uniformly explicit, which is the case the original report of this problem missed. + * Resuming the previous run's own position is something the operator asked for; falling back + * because that run never committed a batch inherits exactly like a fresh dashboard start. Labelling + * all three the same way would make the discriminator a second thing to distrust. + */ +describe('data_sync retry route — cursor provenance', () => { + beforeEach(() => { + jest.clearAllMocks() + mockReadJsonSafe.mockReturnValue({}) + mockGetAuthFromRequest.mockResolvedValue({ sub: 'user-1', tenantId: 'tenant-1', orgId: 'org-1' }) + mockCreateRequestContainer.mockResolvedValue({ + resolve: (token: string) => { + if (token === 'dataSyncRunService') return mockSyncRunService + if (token === 'progressService') return mockProgressService + if (token === 'crudMutationGuardService') return mockCrudMutationGuardService + throw new Error(`Unexpected token: ${token}`) + }, + }) + mockCrudMutationGuardService.validateMutation.mockResolvedValue({ ok: true, shouldRunAfterSuccess: false, metadata: null }) + mockCrudMutationGuardService.afterMutationSuccess.mockResolvedValue(undefined) + mockSyncRunService.findRunningOverlap.mockResolvedValue(null) + mockSyncRunService.resolveCursor.mockResolvedValue(null) + mockSyncRunService.resolveResumeCursorWithSource.mockResolvedValue({ cursor: null, runId: null }) + mockGetIntegration.mockReturnValue({ id: 'sync_excel', providerKey: 'excel' }) + mockGetDataSyncAdapter.mockReturnValue({ + providerKey: 'excel', + direction: 'import', + supportedEntities: ['customers.person'], + }) + mockStartDataSyncRun.mockResolvedValue({ + run: { id: '44444444-4444-4444-8444-444444444444' }, + progressJob: { id: '66666666-6666-4666-8666-666666666666' }, + }) + }) + + function previousRun(overrides: Record = {}) { + mockSyncRunService.getRun.mockResolvedValue({ + id: RUN_ID, + integrationId: 'sync_excel', + entityType: 'customers.person', + direction: 'import', + status: 'failed', + cursor: null, + parameters: null, + ...overrides, + }) + } + + it('labels resuming the previous run own position explicit, and names that run', async () => { + previousRun({ cursor: 'previous-run-cursor' }) + + await callRetry() + + expect(startedInput()).toMatchObject({ + cursor: 'previous-run-cursor', + cursorOrigin: 'explicit', + cursorSourceRunId: RUN_ID, + }) + }) + + it('labels a fromBeginning retry as none and resolves nothing', async () => { + previousRun({ cursor: 'previous-run-cursor' }) + mockReadJsonSafe.mockReturnValue({ fromBeginning: true }) + + await callRetry() + + expect(startedInput()).toMatchObject({ + cursor: null, + cursorOrigin: 'none', + cursorSourceRunId: null, + }) + expect(mockSyncRunService.resolveCursor).not.toHaveBeenCalled() + expect(mockSyncRunService.resolveResumeCursorWithSource).not.toHaveBeenCalled() + }) + + /** + * The fallback. A retry of a run that never committed a batch has no position of its own to + * resume, so it inherits from the shared row like any other start — and says so. + */ + it('labels the shared-row fallback inherited when the previous run committed nothing', async () => { + previousRun({ cursor: null }) + mockSyncRunService.resolveCursor.mockResolvedValue('shared-cursor') + + await callRetry() + + expect(startedInput()).toMatchObject({ + cursor: 'shared-cursor', + cursorOrigin: 'inherited', + cursorSourceRunId: null, + }) + }) + + it('names the source run when the fallback resumes an opted-out entity type', async () => { + previousRun({ cursor: null }) + mockGetDataSyncAdapter.mockReturnValue({ + providerKey: 'excel', + direction: 'import', + supportedEntities: ['customers.person'], + persistsSharedCursor: () => false, + }) + mockSyncRunService.resolveResumeCursorWithSource.mockResolvedValue({ + cursor: 'interrupted-run-cursor', + runId: EARLIER_RUN_ID, + }) + + await callRetry() + + expect(startedInput()).toMatchObject({ + cursor: 'interrupted-run-cursor', + cursorOrigin: 'inherited', + cursorSourceRunId: EARLIER_RUN_ID, + }) + }) + + it('labels a retry with nothing to resume as none rather than inherited', async () => { + previousRun({ cursor: null }) + + await callRetry() + + expect(startedInput()).toMatchObject({ + cursor: null, + cursorOrigin: 'none', + cursorSourceRunId: null, + }) + }) +}) diff --git a/packages/core/src/modules/data_sync/lib/__tests__/cursor-origin.test.ts b/packages/core/src/modules/data_sync/lib/__tests__/cursor-origin.test.ts new file mode 100644 index 00000000000..8ab2b74b234 --- /dev/null +++ b/packages/core/src/modules/data_sync/lib/__tests__/cursor-origin.test.ts @@ -0,0 +1,50 @@ +import type { SyncRun } from '../../data/entities' +import { deliveredCursorOrigin } from '../cursor-origin' + +type RunFacts = Pick + +function run(overrides: Partial = {}): RunFacts { + return { cursor: null, batchesCompleted: 0, cursorOrigin: null, ...overrides } +} + +/** + * The whole point of the derivation is that the row's stored origin and the origin of the cursor + * being handed over are different facts once a run has done any work of its own. + */ +describe('deliveredCursorOrigin', () => { + it('reports no cursor as none whatever the row stored', () => { + expect(deliveredCursorOrigin(run({ cursor: null, cursorOrigin: 'inherited' }))).toBe('none') + }) + + it('passes the stored origin through on the first delivery', () => { + expect(deliveredCursorOrigin(run({ cursor: 'c1', cursorOrigin: 'inherited' }))).toBe('inherited') + expect(deliveredCursorOrigin(run({ cursor: 'c1', cursorOrigin: 'explicit' }))).toBe('explicit') + }) + + /** + * The regression this file exists for. A redelivered job re-enters the adapter with the run's own + * committed position; reporting the start-time label there would make an adapter that refuses + * inherited cursors restart from the top on every worker hiccup. + */ + it('reports self once a batch has committed, whatever the run started from', () => { + expect(deliveredCursorOrigin(run({ cursor: 'c9', batchesCompleted: 1, cursorOrigin: 'inherited' }))).toBe('self') + expect(deliveredCursorOrigin(run({ cursor: 'c9', batchesCompleted: 7, cursorOrigin: 'explicit' }))).toBe('self') + expect(deliveredCursorOrigin(run({ cursor: 'c9', batchesCompleted: 3, cursorOrigin: null }))).toBe('self') + }) + + /** + * A run written before provenance shipped knows nothing about its own start, and guessing would be + * worse than silence — an adapter would act on an origin nothing established. + */ + it('reports an absent origin for a run that predates provenance', () => { + expect(deliveredCursorOrigin(run({ cursor: 'c1', cursorOrigin: null }))).toBeUndefined() + }) + + it('treats a missing batch count as no batches rather than throwing', () => { + expect(deliveredCursorOrigin({ + cursor: 'c1', + batchesCompleted: undefined as unknown as number, + cursorOrigin: 'inherited', + })).toBe('inherited') + }) +}) diff --git a/packages/core/src/modules/data_sync/lib/__tests__/sync-engine-cursor-origin.test.ts b/packages/core/src/modules/data_sync/lib/__tests__/sync-engine-cursor-origin.test.ts new file mode 100644 index 00000000000..7d2a71c0171 --- /dev/null +++ b/packages/core/src/modules/data_sync/lib/__tests__/sync-engine-cursor-origin.test.ts @@ -0,0 +1,214 @@ +/** @jest-environment node */ + +import type { EntityManager } from '@mikro-orm/postgresql' +import { findOneWithDecryption } from '@open-mercato/shared/lib/encryption/find' +import type { CredentialsService } from '../../../integrations/lib/credentials-service' +import type { IntegrationLogService } from '../../../integrations/lib/log-service' +import type { ProgressService } from '../../../progress/lib/progressService' +import { SyncCursor, SyncRun } from '../../data/entities' +import type { CursorOrigin, DataSyncAdapter, StreamImportInput, StreamExportInput } from '../adapter' +import { createSyncRunService } from '../sync-run-service' +import type { SyncRunService } from '../sync-run-service' + +const mockGetDataSyncAdapter = jest.fn() + +jest.mock('@open-mercato/shared/lib/encryption/find', () => ({ + findOneWithDecryption: jest.fn(), + findWithDecryption: jest.fn().mockResolvedValue([]), + findAndCountWithDecryption: jest.fn().mockResolvedValue([[], 0]), +})) + +jest.mock('../adapter-registry', () => ({ + ...jest.requireActual('../adapter-registry'), + getDataSyncAdapter: (...args: unknown[]) => mockGetDataSyncAdapter(...args), +})) + +jest.mock('@open-mercato/shared/modules/integrations/types', () => ({ + getIntegration: () => ({ providerKey: 'origin-probe' }), +})) + +jest.mock('../../events', () => ({ + emitDataSyncEvent: jest.fn(async () => undefined), +})) + +jest.mock('../../../query_index/lib/coverage', () => ({ + refreshCoverageSnapshot: jest.fn(async () => undefined), +})) + +import { createSyncEngine } from '../sync-engine' + +const SCOPE = { organizationId: 'org-1', tenantId: 'tenant-1', userId: 'user-1' } +const ENTITY = 'catalog.product' + +type FakeRun = { + id: string + integrationId: string + entityType: string + direction: 'import' | 'export' + status: string + cursor: string | null + cursorOrigin: CursorOrigin | null + progressJobId: string | null + createdCount: number + updatedCount: number + skippedCount: number + failedCount: number + batchesCompleted: number +} + +function buildRun(overrides: Partial = {}): FakeRun { + return { + id: 'run-1', + integrationId: 'sync_probe', + entityType: ENTITY, + direction: 'import', + status: 'pending', + cursor: null, + cursorOrigin: null, + progressJobId: null, + createdCount: 0, + updatedCount: 0, + skippedCount: 0, + failedCount: 0, + batchesCompleted: 0, + ...overrides, + } +} + +function buildFakeEm(runs: FakeRun[]) { + const cursorRows: Record[] = [] + const em = { + begin: jest.fn(async () => undefined), + commit: jest.fn(async () => undefined), + rollback: jest.fn(async () => undefined), + flush: jest.fn(async () => undefined), + create: jest.fn((entity: unknown, data: Record) => { + const row = { ...data } + if (entity === SyncCursor) cursorRows.push(row) + return row + }), + nativeUpdate: jest.fn(async (_entity: unknown, where: { id: string }) => { + const run = runs.find((candidate) => candidate.id === where.id) + if (!run) return 0 + run.status = 'running' + return 1 + }), + } + + ;(findOneWithDecryption as jest.Mock).mockImplementation((_em: unknown, entity: unknown, where: Record) => { + if (entity === SyncRun) return Promise.resolve(runs.find((run) => run.id === where.id) ?? null) + if (entity === SyncCursor) return Promise.resolve(null) + return Promise.resolve(null) + }) + + return em +} + +function buildEngineDeps(em: unknown, syncRunService: SyncRunService) { + return { + em: em as EntityManager, + syncRunService, + integrationCredentialsService: { resolve: jest.fn(async () => ({ token: 'secret' })) } as unknown as CredentialsService, + integrationLogService: { write: jest.fn(async () => undefined) } as unknown as IntegrationLogService, + integrationStateService: { upsert: jest.fn(async () => undefined) } as never, + progressService: { + startJob: jest.fn(async () => undefined), + isCancellationRequested: jest.fn(async () => false), + updateProgress: jest.fn(async () => undefined), + completeJob: jest.fn(async () => undefined), + failJob: jest.fn(async () => undefined), + markCancelled: jest.fn(async () => undefined), + } as unknown as ProgressService, + } +} + +/** + * Captures what the adapter was actually handed, which is the only place the derivation is + * observable — the run row keeps the start-time label either way. + */ +function buildProbeAdapter(seen: Array, direction: 'import' | 'export' = 'import'): DataSyncAdapter { + const stream = async function* (input: StreamImportInput | StreamExportInput) { + seen.push(input.cursorOrigin) + yield direction === 'import' + ? { items: [], cursor: 'committed-cursor', hasMore: false, batchIndex: 0 } + : { results: [], cursor: 'committed-cursor', hasMore: false, batchIndex: 0 } + } + return { + providerKey: 'origin-probe', + direction, + supportedEntities: [ENTITY], + getMapping: jest.fn(async ({ entityType }) => ({ entityType, matchStrategy: 'externalId' as const, fields: [] })), + ...(direction === 'import' ? { streamImport: stream } : { streamExport: stream }), + } as unknown as DataSyncAdapter +} + +describe('sync engine reports the provenance of the cursor it is handing over', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('passes the stored origin on a first delivery', async () => { + const run = buildRun({ cursor: 'inherited-cursor', cursorOrigin: 'inherited' }) + const em = buildFakeEm([run]) + const seen: Array = [] + mockGetDataSyncAdapter.mockReturnValue(buildProbeAdapter(seen)) + + const engine = createSyncEngine(buildEngineDeps(em, createSyncRunService(em as never))) + await engine.runImport('run-1', 100, SCOPE) + + expect(seen).toEqual(['inherited']) + }) + + /** + * The redelivery case. The run started from an inherited cursor but has since committed batches, + * so the position it is being resumed from is its own. An adapter refusing `inherited` must not + * refuse this. + */ + it('reports self when the run has already committed batches', async () => { + const run = buildRun({ cursor: 'advanced-cursor', cursorOrigin: 'inherited', batchesCompleted: 3 }) + const em = buildFakeEm([run]) + const seen: Array = [] + mockGetDataSyncAdapter.mockReturnValue(buildProbeAdapter(seen)) + + const engine = createSyncEngine(buildEngineDeps(em, createSyncRunService(em as never))) + await engine.runImport('run-1', 100, SCOPE) + + expect(seen).toEqual(['self']) + }) + + it('reports none for a run that starts from no cursor', async () => { + const run = buildRun({ cursor: null, cursorOrigin: 'none' }) + const em = buildFakeEm([run]) + const seen: Array = [] + mockGetDataSyncAdapter.mockReturnValue(buildProbeAdapter(seen)) + + const engine = createSyncEngine(buildEngineDeps(em, createSyncRunService(em as never))) + await engine.runImport('run-1', 100, SCOPE) + + expect(seen).toEqual(['none']) + }) + + it('leaves the origin absent for a run written before provenance shipped', async () => { + const run = buildRun({ cursor: 'legacy-cursor', cursorOrigin: null }) + const em = buildFakeEm([run]) + const seen: Array = [] + mockGetDataSyncAdapter.mockReturnValue(buildProbeAdapter(seen)) + + const engine = createSyncEngine(buildEngineDeps(em, createSyncRunService(em as never))) + await engine.runImport('run-1', 100, SCOPE) + + expect(seen).toEqual([undefined]) + }) + + it('reports provenance on the export path too', async () => { + const run = buildRun({ id: 'run-export', direction: 'export', cursor: 'inherited-cursor', cursorOrigin: 'inherited' }) + const em = buildFakeEm([run]) + const seen: Array = [] + mockGetDataSyncAdapter.mockReturnValue(buildProbeAdapter(seen, 'export')) + + const engine = createSyncEngine(buildEngineDeps(em, createSyncRunService(em as never))) + await engine.runExport('run-export', 100, SCOPE) + + expect(seen).toEqual(['inherited']) + }) +}) diff --git a/packages/core/src/modules/data_sync/lib/__tests__/sync-run-service.shared-cursor.test.ts b/packages/core/src/modules/data_sync/lib/__tests__/sync-run-service.shared-cursor.test.ts index 0a11d42ea81..c5914d89987 100644 --- a/packages/core/src/modules/data_sync/lib/__tests__/sync-run-service.shared-cursor.test.ts +++ b/packages/core/src/modules/data_sync/lib/__tests__/sync-run-service.shared-cursor.test.ts @@ -225,6 +225,67 @@ describe('SyncRunService.resolveResumeCursor', () => { }) }) +/** + * The run id is what turns "this run started mid-table" into "this run is continuing run X" on the + * detail page, and it is the half of provenance the shared-cursor branch cannot supply. + */ +describe('SyncRunService.resolveResumeCursorWithSource', () => { + beforeEach(() => { + ;(findOneWithDecryption as jest.Mock).mockReset() + ;(findWithDecryption as jest.Mock).mockReset().mockResolvedValue([]) + }) + + it('names the run a resumed cursor came from', async () => { + const em = buildFakeEm() + ;(findWithDecryption as jest.Mock).mockResolvedValue([ + { id: 'run-earlier', status: 'failed', cursor: 'interrupted-cursor' }, + ]) + + const service = createSyncRunService(em as any) + + await expect(service.resolveResumeCursorWithSource('sync_backfill', 'catalog.product', 'import', SCOPE)) + .resolves.toEqual({ cursor: 'interrupted-cursor', runId: 'run-earlier' }) + }) + + it('returns no run when the latest run completed', async () => { + const em = buildFakeEm() + ;(findWithDecryption as jest.Mock).mockResolvedValue([ + { id: 'run-done', status: 'completed', cursor: 'finished-walk-cursor' }, + ]) + + const service = createSyncRunService(em as any) + + await expect(service.resolveResumeCursorWithSource('sync_backfill', 'catalog.product', 'import', SCOPE)) + .resolves.toEqual({ cursor: null, runId: null }) + }) + + /** + * An interrupted run that never committed anything has no position to hand over, so naming it + * would point an operator at a run that contributed nothing to where this one starts. + */ + it('returns no run when the interrupted run holds no cursor', async () => { + const em = buildFakeEm() + ;(findWithDecryption as jest.Mock).mockResolvedValue([ + { id: 'run-empty', status: 'failed', cursor: null }, + ]) + + const service = createSyncRunService(em as any) + + await expect(service.resolveResumeCursorWithSource('sync_backfill', 'catalog.product', 'import', SCOPE)) + .resolves.toEqual({ cursor: null, runId: null }) + }) + + it('returns no run when the entity type has never run', async () => { + const em = buildFakeEm() + ;(findWithDecryption as jest.Mock).mockResolvedValue([]) + + const service = createSyncRunService(em as any) + + await expect(service.resolveResumeCursorWithSource('sync_backfill', 'catalog.product', 'import', SCOPE)) + .resolves.toEqual({ cursor: null, runId: null }) + }) +}) + describe('SyncRunService.resetResumePosition', () => { beforeEach(() => { ;(findOneWithDecryption as jest.Mock).mockReset() diff --git a/packages/core/src/modules/data_sync/workers/__tests__/sync-scheduled.test.ts b/packages/core/src/modules/data_sync/workers/__tests__/sync-scheduled.test.ts index 1df2fa3090a..096dfff1de5 100644 --- a/packages/core/src/modules/data_sync/workers/__tests__/sync-scheduled.test.ts +++ b/packages/core/src/modules/data_sync/workers/__tests__/sync-scheduled.test.ts @@ -8,6 +8,7 @@ const mockEnqueue = jest.fn() const mockSyncRunService = { findRunningOverlap: jest.fn(), resolveCursor: jest.fn(), + resolveResumeCursorWithSource: jest.fn(), createRun: jest.fn(), } @@ -80,6 +81,7 @@ describe('data-sync scheduled worker', () => { mockIntegrationStateService.isEnabled.mockResolvedValue(true) mockSyncRunService.findRunningOverlap.mockResolvedValue(null) mockSyncRunService.resolveCursor.mockResolvedValue('cursor-1') + mockSyncRunService.resolveResumeCursorWithSource.mockResolvedValue({ cursor: null, runId: null }) mockSyncRunService.createRun.mockImplementation(async (input: { progressJobId?: string | null }) => ({ id: 'run-1', progressJobId: input.progressJobId ?? null, @@ -107,6 +109,10 @@ describe('data-sync scheduled worker', () => { expect(createRunInput.progressJobId).toBe('progress-1') expect(createRunInput.triggeredBy).toBe('scheduler') expect(createRunInput.cursor).toBe('cursor-1') + // A schedule has no operator watching it choose a start position, so recording that the + // position was inherited is the only trace of why the run began where it did. + expect(createRunInput.cursorOrigin).toBe('inherited') + expect(createRunInput.cursorSourceRunId).toBeNull() }) it('enqueues the import job with a tenant/organization-scoped payload', async () => { @@ -228,6 +234,8 @@ describe('data-sync scheduled worker', () => { expect(mockGetSyncQueue).toHaveBeenCalledWith('data-sync-export') const createRunInput = mockSyncRunService.createRun.mock.calls[0][0] expect(createRunInput.cursor).toBeNull() + expect(createRunInput.cursorOrigin).toBe('none') + expect(createRunInput.cursorSourceRunId).toBeNull() }) it('skips disabled schedules without creating a run', async () => { From d5225e7f16aaaa98997661577a33c2e93341fbf5 Mon Sep 17 00:00:00 2001 From: Maksymilian Gala Date: Mon, 31 Aug 2026 17:58:52 +0200 Subject: [PATCH 5/5] docs(data_sync): document cursor provenance and its contract classification Adds the framework-docs section an adapter author needs to act on the new field, including why 'self' makes refusing an inherited cursor safe, and the AGENTS.md rules for start paths that must label what they resolved. Records the change in BACKWARD_COMPATIBILITY.md as additive under the type-interface, function-signature and database-schema categories. Co-Authored-By: Claude Opus 5 (1M context) --- BACKWARD_COMPATIBILITY.md | 16 ++++++ .../modules/integrations-data-sync.mdx | 52 +++++++++++++++++++ packages/core/src/modules/data_sync/AGENTS.md | 4 ++ 3 files changed, 72 insertions(+) diff --git a/BACKWARD_COMPATIBILITY.md b/BACKWARD_COMPATIBILITY.md index ad7e8913298..aa21629a3bb 100644 --- a/BACKWARD_COMPATIBILITY.md +++ b/BACKWARD_COMPATIBILITY.md @@ -395,3 +395,19 @@ Issue #3852 removed the non-cryptographic passkey verification shape from `Passk **Why the deprecation protocol does not apply.** The protocol exists to give downstream authors a bridge release. Here the request shape being removed *is* the vulnerability: both values it compared are disclosed by the server, so a bridge would keep the passkey second factor bypassable for a minor version in both login MFA and sudo step-up. A security fix that leaves the hole open is not a fix. **Migration path.** Send `startAuthentication()` output as `payload.response`. The first-party `PasskeyChallengeVerify` component already does, so shipped UIs are unaffected. Credentials enrolled through the setup path's client-supplied `publicKey` shortcut are **not** reliably rendered unusable by this change — depending on what the client supplied, such a row holds either a key nobody can sign with or a keypair the enroller controls, and the second kind produces assertions this change accepts. That shortcut is a separate open surface (#5296); operator-facing remediation is in [`UPGRADE_NOTES.md`](UPGRADE_NOTES.md). + +## Data Sync Cursor Provenance (2026-08-31) + +[`.ai/specs/2026-08-31-data-sync-cursor-provenance.md`](.ai/specs/2026-08-31-data-sync-cursor-provenance.md) tells an adapter where the cursor it is being handed came from, so a cursor that encodes scope can be refused when it was silently inherited from an earlier run without also refusing a legitimate Retry or a queue redelivery. **All changes are additive** and pass the contract-surface checks above: + +| Surface | Change | Classification | +|---------|--------|----------------| +| Type interfaces (`StreamImportInput`, `StreamExportInput`) | One new **optional** field each: `cursorOrigin?: CursorOrigin` | ✓ ADDITIVE (category 2, Type interface, optional field) | +| Type interface (`StartDataSyncRunInput`) | Two new **optional** fields: `cursorOrigin?`, `cursorSourceRunId?`. Omitting them yields `'explicit'` when a cursor was supplied and `'none'` when it was not, so both existing out-of-module callers (`sync_excel`, `sync_akeneo`) are unaffected | ✓ ADDITIVE (optional fields with a behaviour-preserving default) | +| Import paths / exports (`@open-mercato/core/modules/data_sync/lib/*`) | New exports: `CursorOrigin` and `ResolvedStartCursor` types, `resolveStartCursorWithOrigin`, `deliveredCursorOrigin` (new file `lib/cursor-origin.ts`), `SyncRunService.resolveResumeCursorWithSource`. Nothing removed or renamed | ✓ ADDITIVE (category 4, new exports) | +| Function signatures (`resolveStartCursor`, `SyncRunService.resolveResumeCursor`) | Unchanged. Both become delegating wrappers over the provenance-aware variants and return exactly what they returned before; `resolveStartCursor` gains a `@deprecated` tag only | ✓ ADDITIVE (category 3, no signature change; deprecation tag carries no runtime effect) | +| Database schema (`sync_runs`) | Two new **nullable, no-default** columns: `cursor_origin text null`, `cursor_source_run_id uuid null`, added by `Migration20260831120000`. No rename, removal, type narrowing or index change; both are metadata-only `ADD COLUMN` operations with no table rewrite | ✓ ADDITIVE (category 8, add new columns) | +| HTTP response shapes (`GET /api/data_sync/runs`, `GET /api/data_sync/runs/[id]`) | Two new nullable fields per run: `cursorOrigin`, `cursorSourceRunId`. No existing field changed, removed or renamed | ✓ ADDITIVE (new response fields) | +| HTTP request shapes (`POST /api/data_sync/run`, `POST /api/data_sync/runs/[id]/retry`) | No change — no zod schema, parameter or status code differs. Both routes persist the origin they already resolved | ✓ n/a | +| Runtime behaviour | Byte-identical cursor resolution on every path. Adapters that ignore `cursorOrigin`, and runs written before the migration (which read `null` and are reported to adapters as an absent field rather than a guessed origin), behave exactly as before | ✓ Behaviour-preserving | +| API route URLs, HTTP methods, event IDs, widget spot IDs, ACL features, DI names, notification IDs, CLI commands, generated files | No change | ✓ n/a | diff --git a/apps/docs/docs/framework/modules/integrations-data-sync.mdx b/apps/docs/docs/framework/modules/integrations-data-sync.mdx index eab43d01785..047acc8e6d1 100644 --- a/apps/docs/docs/framework/modules/integrations-data-sync.mdx +++ b/apps/docs/docs/framework/modules/integrations-data-sync.mdx @@ -267,6 +267,58 @@ await em.nativeDelete(SyncCursor, { integrationId, entityType, direction: 'impor when nothing was interrupted, so it is safe to call unconditionally, including for entity types that do keep the shared row. +## Cursor provenance + +Both resolutions above hand the adapter a position the operator never named. That is the intended +incremental behaviour, and it is invisible: pressing **Run** means *"continue whatever ran last"*. + +For a cursor that is purely a position, that is fine. For a cursor that encodes **scope** — filters, +date or id bounds, a dry-run flag — it is not. A fresh, unfiltered run that inherits a failed run's +date window walks only that window and still finishes `completed`, having skipped everything outside +it. And the adapter cannot tell that apart from a Retry that was deliberately told to resume, because +both arrive carrying a cursor and the previous run's parameters. + +So the engine says where the cursor came from: + +```ts +async *streamImport(input: StreamImportInput) { + if (input.cursorOrigin === 'inherited') { + // This position came from a run nobody in this request asked for. Our cursor pins a scope, + // so continuing from it would silently narrow the walk. + throw new Error('[internal] refusing an inherited cursor; start a full sync instead') + } + // ... +} +``` + +| `cursorOrigin` | Meaning | +|---|---| +| `none` | No cursor. Start from the beginning. | +| `explicit` | The caller named this cursor — a Retry resuming the previous run's own position, or a provider flow that computed one. | +| `inherited` | Core resolved it from prior state the caller never named: the shared `sync_cursors` row, or the last incomplete run. | +| `self` | This run's own committed progress, handed back after a queue redelivery. | + +The field is absent for runs created before provenance shipped, so adapters that ignore it — and +adapters reading older runs — behave exactly as before. + +**`self` is why refusing `inherited` is safe.** The engine hands over `run.cursor`, not +`initialCursor`, so once a batch has committed, the position is the adapter's own output whatever the +run started from. A redelivered job would otherwise re-enter the adapter with the run's start-time +label still attached, and an adapter refusing `inherited` would restart from the top on every worker +hiccup. `self` is derived from `batchesCompleted`, so the run row records where the run *started* +while the adapter is told what it is *being handed*. + +`inherited` does not say which prior state it came from. `sync_runs.cursor_source_run_id` — returned +as `cursorSourceRunId` by both run read APIs — names the run for the previous-run case and is `null` +when the position came from the shared row. The run detail page uses exactly that split to tell an +operator either *"continued where an earlier run stopped"*, with a link, or *"continued from the +saved incremental position"*. + +Adapters aside, any code that starts a run should label what it resolved. Use +`resolveStartCursorWithOrigin` rather than the deprecated `resolveStartCursor`, and pass its +`origin` / `sourceRunId` through to `startDataSyncRun`. A caller that omits them is taken at its +word: `explicit` when it supplied a cursor, `none` when it did not. + ## Provider-owned environment preconfiguration New integration providers should not require manual admin setup after every fresh install when deployment already knows the credentials and defaults. The provider package should own that bootstrap path itself. diff --git a/packages/core/src/modules/data_sync/AGENTS.md b/packages/core/src/modules/data_sync/AGENTS.md index 1ffb7ce7b13..b94ac4c7adb 100644 --- a/packages/core/src/modules/data_sync/AGENTS.md +++ b/packages/core/src/modules/data_sync/AGENTS.md @@ -55,6 +55,8 @@ packages/core/src/modules/data_sync/ │ ├── id-mapping.ts # External ID ↔ local ID lookup and storage │ ├── queue.ts # Queue helper for enqueuing sync jobs │ ├── batch-stream.ts # Drives adapter streams; one root span per batch +│ ├── cursor-origin.ts # Provenance of the cursor being handed to the adapter this delivery +│ ├── start-cursor.ts # Resolves a start position and where it came from │ ├── run-parameters.ts # Validate/coerce operator run parameters vs. adapter declaration │ ├── sync-engine.ts # Orchestrates streaming import/export with progress │ └── sync-run-service.ts # CRUD for SyncRun + cursor management @@ -205,6 +207,8 @@ If the sync provider needs bootstrap credentials, mappings, locales, channels, o - **Shared cursor opt-out**: An adapter returning `persistsSharedCursor(entityType) === false` keeps that entity type's cursor on the run row only — use it for whole-table backfills whose cursor is one run's scan state, not a durable log position. Those entity types resolve an incremental start position from the most recent run (`resolveResumeCursor`) instead of the shared row, and from `null` when that run completed - **Resetting an opt-out**: A reset flow that deletes the shared `SyncCursor` row MUST also call `syncRunService.resetResumePosition(integrationId, entityType, direction, scope)`. An opted-out entity type has no shared row to delete, so deleting only that leaves the resume position on the last interrupted run and the next incremental run re-imports just the tail of the walk it was reset against. The call is a no-op when nothing is interrupted, so make it unconditionally - **Resume**: Retry reads the last successful cursor, resumes from there +- **Cursor provenance**: Every run records where its start cursor came from in `sync_runs.cursor_origin`, plus `cursor_source_run_id` when it came from a run. Adapters receive it as `StreamImportInput.cursorOrigin` / `StreamExportInput.cursorOrigin`: `none` (no cursor), `explicit` (the caller named it — a Retry resuming the previous run's own position), `inherited` (core resolved it from the shared row or the last incomplete run), `self` (this run's own committed progress, on a redelivery). An adapter whose cursor encodes SCOPE — filters, date/id bounds, dry-run flags — and not just a position SHOULD refuse `inherited`, because a fresh run inheriting a stranger run's window walks only that window and still reports `completed`. Start paths MUST label what they resolved: use `resolveStartCursorWithOrigin` (not the deprecated `resolveStartCursor`) and pass `cursorOrigin` / `cursorSourceRunId` to `startDataSyncRun`; a caller that omits them gets `explicit` when it supplied a cursor and `none` when it did not +- **Delivered vs stored origin**: the column is the provenance of the run's START cursor and never changes; what the adapter receives is `deliveredCursorOrigin(run)` (`lib/cursor-origin.ts`), which reports `self` once `batchesCompleted > 0`. The engine hands over `run.cursor`, not `initialCursor`, so after the first commit the position is the adapter's own whatever the run started from — without that distinction an adapter refusing `inherited` would reject its own resume on every queue redelivery. `inherited` alone does not say WHICH prior state: read `cursorSourceRunId` (set for the previous-run case, null for the shared `sync_cursors` row) - **Progress**: Linked to `ProgressJob` via `progressJobId` for `ProgressTopBar` display - **Cancellation**: The engine polls `progressService.isCancellationRequested()` in the batch handler AND on the heartbeat tick while a batch is still in flight, aborting `StreamImportInput.signal` / `StreamExportInput.signal`. Adapters SHOULD honour the signal wherever the work is divisible (per page, per record, around a long flush) and `return` — with the `return` ABOVE the `yield`, never below it, or the engine commits a cursor for a half-applied page. Adapters that ignore the signal keep the old between-batches behavior. - **Tracing**: The engine emits one **root** span per batch (`data_sync.import.batch` / `data_sync.export.batch`) linked back to the run, covering the adapter's read *and* the engine's bookkeeping. Adapters MUST NOT hand-roll their own batch span — they cannot root it, so a multi-day run would ride on the single sampling decision taken for the request that triggered it. Inner spans an adapter creates nest under the batch span normally. The final read — the one that finds the stream drained — is traced as `data_sync.import.drain` / `data_sync.export.drain`, so N batches emit exactly N `*.batch` spans plus one `*.drain`.