diff --git a/apps/dev/src/integration/translator/transaction-failure-isolation.int.test.ts b/apps/dev/src/integration/translator/transaction-failure-isolation.int.test.ts new file mode 100644 index 00000000..681b953d --- /dev/null +++ b/apps/dev/src/integration/translator/transaction-failure-isolation.int.test.ts @@ -0,0 +1,57 @@ +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; + +// Postgres only: the other adapters do not isolate a transaction, so the spec would assert nothing. +const POSTGRES = process.env.DB_ADAPTER === "postgres"; + +let ctx: TestPayload; + +describe.skipIf(!POSTGRES)("a translation that fails inside the caller's transaction", () => { + beforeAll(async () => { + ctx = await bootTestPayload({ + autoTranslate: { targets: ["de", "fr"] }, + failFor: ["de"], + }); + }); + afterAll(async () => { + await ctx?.cleanup(); + }); + + it("does not fail the save that triggered it", async () => { + const created = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: "Survives", _status: "published" } as never, + }); + + const read = await ctx.payload.findByID({ + collection: "docs" as "pages", + id: created.id, + locale: "en", + }); + expect((read as { title?: string }).title).toBe("Survives"); + }); + + it("still translates the locales that did not fail", async () => { + const created = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: "Partial", _status: "published" } as never, + }); + + const fr = await ctx.payload.findByID({ + collection: "docs" as "pages", + id: created.id, + locale: "fr", + }); + const de = await ctx.payload.findByID({ + collection: "docs" as "pages", + id: created.id, + locale: "de", + }); + expect((fr as { title?: string }).title).toBe("fr:Partial"); + expect((de as { title?: string }).title).not.toBe("de:Partial"); + }); +}); diff --git a/apps/dev/src/integration/translator/transaction-rollback.int.test.ts b/apps/dev/src/integration/translator/transaction-rollback.int.test.ts new file mode 100644 index 00000000..9622b16a --- /dev/null +++ b/apps/dev/src/integration/translator/transaction-rollback.int.test.ts @@ -0,0 +1,96 @@ +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; + +// Postgres only. An explicit begin/rollback does not roll the document back on SQLite — the row +// survives — so the same spec would assert nothing there. MongoDB needs a replica set for +// transactions at all. +const POSTGRES = process.env.DB_ADAPTER === "postgres"; + +let ctx: TestPayload; + +const beginTransaction = async (): Promise => { + const transactionID = await ctx.payload.db.beginTransaction?.({}); + expect(transactionID, "the adapter opened no transaction").toBeTruthy(); + return transactionID as string | number; +}; + +const provenanceFor = async (documentId: string) => { + const { docs } = await ctx.payload.find({ + collection: "translator-provenance" as "pages", + pagination: false, + where: { documentId: { equals: documentId } } as never, + }); + return docs; +}; + +describe.skipIf(!POSTGRES)("a translation is atomic with the save that triggered it", () => { + beforeAll(async () => { + ctx = await bootTestPayload({ autoTranslate: { targets: ["de"] } }); + }); + afterAll(async () => { + await ctx?.cleanup(); + }); + + it("leaves no translation and no receipt behind when the save rolls back", async () => { + const transactionID = await beginTransaction(); + const created = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: "Rolled back", _status: "published" } as never, + req: { transactionID } as never, + }); + const id = String(created.id); + + await ctx.payload.db.rollbackTransaction?.(transactionID); + + const { docs } = await ctx.payload.find({ + collection: "docs" as "pages", + pagination: false, + where: { id: { equals: id } } as never, + }); + expect(docs, "the rolled-back document survived").toHaveLength(0); + expect(await provenanceFor(id), "a receipt outlived the write it certifies").toHaveLength(0); + }); + + it("commits the translation and the receipt when the save commits", async () => { + const created = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: "Committed", _status: "published" } as never, + }); + const id = String(created.id); + + const de = await ctx.payload.findByID({ collection: "docs" as "pages", id, locale: "de" }); + expect((de as { title?: string }).title).toBe("de:Committed"); + expect(await provenanceFor(id)).toHaveLength(1); + }); + + it("translates every document of a bulk publish, not just the first", async () => { + const ids = []; + for (const title of ["Bulk one", "Bulk two"]) { + const doc = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title, ref: "bulk-batch" } as never, + }); + ids.push(String(doc.id)); + } + + await ctx.payload.update({ + collection: "docs" as "pages", + locale: "en", + where: { ref: { equals: "bulk-batch" } } as never, + // The content must change too: a publish that edits nothing is skipped by the drift-gate. + data: { _status: "published", title: "Bulk edited" } as never, + }); + + for (const [i, id] of ids.entries()) { + const de = await ctx.payload.findByID({ collection: "docs" as "pages", id, locale: "de" }); + expect((de as { title?: string }).title, `document ${i + 1} of the batch`).toBe( + "de:Bulk edited" + ); + } + }); +}); diff --git a/apps/dev/src/integration/translator/transaction-validation-failure.int.test.ts b/apps/dev/src/integration/translator/transaction-validation-failure.int.test.ts new file mode 100644 index 00000000..7478b033 --- /dev/null +++ b/apps/dev/src/integration/translator/transaction-validation-failure.int.test.ts @@ -0,0 +1,62 @@ +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import type { CollectionConfig } from "payload"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { buildTestCollections } from "./testCollections"; + +// Postgres: the adapter that isolates a transaction. +const POSTGRES = process.env.DB_ADAPTER === "postgres"; + +let ctx: TestPayload; + +// The target-locale write fails Payload's own validation, so the error is raised by a Payload +// *operation* rather than by the provider — the case that reaches `killTransaction`. +const rejectTranslated = (collections: CollectionConfig[]): CollectionConfig[] => + collections.map((c) => { + if (c.slug !== "docs") return c; + return { + ...c, + fields: c.fields.map((f) => + "name" in f && f.name === "title" + ? { + ...f, + validate: (value: unknown) => + typeof value === "string" && value.startsWith("de:") + ? "the de locale rejects this value" + : true, + } + : f + ), + } as CollectionConfig; + }); + +describe.skipIf(!POSTGRES)("a target write rejected by validation", () => { + beforeAll(async () => { + ctx = await bootTestPayload({ + collections: rejectTranslated(buildTestCollections()), + autoTranslate: { targets: ["de"] }, + }); + }); + afterAll(async () => { + await ctx?.cleanup(); + }); + + it("fails the save visibly instead of discarding it in silence", async () => { + await expect( + ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: "Editor wrote this", _status: "published" } as never, + }) + ).rejects.toThrow(); + + const { docs } = await ctx.payload.find({ + collection: "docs" as "pages", + locale: "en", + pagination: false, + where: { title: { equals: "Editor wrote this" } } as never, + }); + expect(docs, "the rejected save left a row behind").toHaveLength(0); + }); +}); diff --git a/packages/payload-plugin-translator/docs/plans/2026-09-14-auto-translate-in-the-triggering-transaction.task.md b/packages/payload-plugin-translator/docs/plans/2026-09-14-auto-translate-in-the-triggering-transaction.task.md new file mode 100644 index 00000000..71aa3f72 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-09-14-auto-translate-in-the-triggering-transaction.task.md @@ -0,0 +1,278 @@ +# Task contract — run the hook's translation inside the transaction that triggered it (#124) + +**Risk: HIGH.** Changes the signature both `TaskRunner` implementations are built from, touches eight +call sites, and moves work into a database transaction that previously ran outside one. + +## The defects, measured + +Three, not the one the issue describes. All three were reproduced before this contract was written. + +**1 — the hook's work runs outside the transaction.** On Postgres, publishing a source-locale change +with `createSyncRunner` translates nothing and raises nothing. `AutoTranslateEnqueue.hook.ts:83` hands +the runner `req.payload`, which opens its own connection, so the read of the source document happens +outside the transaction the `afterChange` hook is executing in. + +Traced with probes rather than reasoned about: + +``` +hook fires, 2 tasks, locale en ✓ +sync runner calls the translate handler + source read → "Not Found" ✗ the row is not visible outside the transaction +runner catches, stores on an in-memory task +enqueue returns, no error ✓ the caller believes it worked +``` + +The issue says the handler "reads an empty source". It does not read at all — `payload.findByID` +throws `Not Found`, because for a fresh connection the row does not exist yet. + +Isolating experiment: `transactionOptions: false` on the Postgres adapter, nothing else changed — +5 of 5 cases pass. With transactions on, 3 fail. SQLite and MongoDB do not wrap the operation, which +is why neither reproduces. + +**2 — a failed translation is invisible unless the host subscribed.** `wireTranslateRunner.ts:71` +catches, calls `notifier.failed(task, error)` and rethrows; `SyncTaskRunner` then catches and stores +the message on a task object nobody reads. `LifecycleNotifier.safe()` returns early when no +`onFailed` callback is configured, and its own `logger.error` covers only a throwing *callback*. So +with the default configuration a translation failure produces no log line at all — which is what hid +defect 1. + +**3 — a queued job survives the rollback of the write that queued it.** Measured on Postgres: begin a +transaction, create a published document, roll back — the `payload-jobs` row stays. + +``` +AssertionError: a job survived the rollback: expected 1 to be +0 +``` + +`payload.jobs.queue` is called without `req`, so the row is written on another connection and does not +share the transaction's fate. The jobs runner is otherwise unaffected: queueing and translating on +Postgres works, verified. + +**Corrected while measuring.** The first version of the rollback probe threw from an `afterChange` +hook appended to the collection. It passed — falsely: the plugin *pushes* its own hook at config time +(`AutoTranslateEnqueue.hook.ts:116`), so the throwing hook ran first and the translator's never ran. +Zero jobs for a trivial reason. Redone with an explicit transaction, where hook order cannot matter. + +## Design decisions + +**D1 — a one-field slice travels, not the request.** + +The fix has to reach the *operation calls*, not the runner: Payload decides whether to join a caller's +transaction by looking for `transactionID` on the `req` passed **to each individual operation** +(`initTransaction` reads it off the options object). Threading a request as far as +`TaskRunnerFactory.create` and stopping there changes nothing — `fetchSourceDocument` and +`TranslateDocumentHandler` still call `payload.findByID` / `payload.update` with no `req`, on a fresh +connection. That was the first version of this decision and it was wrong; the critique caught it and +two source reads confirmed it. + +What travels is the smallest thing that does the job: + +```ts +/** Exactly what an operation needs to join the caller's transaction. */ +export type TransactionScope = { transactionID?: string | number }; +``` + +Measured before choosing it: a bare `{ transactionID }` passed as `req` joins the transaction — +the uncommitted row is visible through it, and invisible without it. `createLocalReq` fills in +`payload`, `i18n`, `headers`, `user` and `query` itself, and the operations declare +`req?: Partial`, so nothing else is owed. + +Rejected: threading the live `PayloadRequest` down the call chain. Two reasons, either sufficient. +It puts a Payload god type into leaf helpers, which the package's own `CLAUDE.md` forbids outside +`plugin.ts`, the config wiring and HTTP route boundaries — and an audit shows the package currently +*honours* that rule everywhere, so this would have been the first breach. And `createLocalReq` +**mutates the request it is handed**: the translator's own skip-context flag would stick to the +caller's request permanently, and because Payload processes a bulk update with one shared request, a +bulk publish would translate its first document and silently skip the rest. Handing each operation a +fresh one-field object avoids that by construction. + +Rejected also: an optional second parameter (`create(payload, req?)`). It leaves the default path — +the one the hook takes — still wrong, and smears two behaviours across both runners. + +**Placement:** `TransactionScope` is declared beside the existing narrow slices +(`modules/task-runner/types.ts`, the convention `Provenance.shapes.ts` already sets). It is carried by +`TaskHandler`, `TaskRunnerFactory.create`, `TranslateDocumentHandler.handle`, `fetchSourceDocument` and +the provenance service, and spent at each `payload.findByID` / `payload.update` / `payload.jobs.queue` +call as `req`. The logging fallback lands in `LifecycleNotifier` — one place, rather than in each +runner's catch. + +**New surface:** none. `create`'s parameter changes type; nothing is added. + +**Written contract owed:** yes — that the work a runner starts joins the caller's transaction. Nothing +in the signature says it, and it is the whole point of the change. + +**Escalate to `/sp-architect`?** No. One module owns the seam, no new dependency, no data-model change. + +## D2 — everything joins the transaction, and a failure that kills it is surfaced + +Decided after the first build was rejected. Three candidates, all measured rather than argued: + +- **Everything inside, failures swallowed** (the first build). Fixes #124, but a translation Payload + rejects rolls the editor's save back and the hook reports success over it. Reproduced in + `transaction-validation-failure.int.test.ts`. Rejected: silent data loss. +- **Reads inside, writes outside** (the user's first choice). **Impossible**, measured: the + translation writes to the very document the caller has not committed, so from outside the + transaction there is nothing to update — `payload.update` answers "Not Found" and #124 is not fixed + at all. This is the measurement that settled the design; it is not a preference. +- **Everything inside, and a failure that killed the transaction is rethrown** — chosen. The edit is + already gone by the time the translator sees the error, so rethrowing cannot save it; it makes the + loss visible instead of silent, which is the trade the user accepted at the outset. + +`killedTheCallersTransaction` narrows the rethrow to `APIError`, because only a Payload operation +reaches `killTransaction`. A provider outage throws before any operation runs and leaves the save +intact — measured in `transaction-failure-isolation.int.test.ts`, where the save commits and the +other locale still translates. + +**The jobs runner takes no scope.** It never had this defect: its job runs after the commit. Giving +it one would add the `killTransaction` exposure to buy only the removal of an orphan job row, which +is wasted work rather than lost data. + +**Provenance joins.** The receipt has to roll back with the translation it certifies, or staleness +detection reports a document translated at a fingerprint that never committed. `deleteByDocument` +keeps its prior, deliberate exemption — a failed sidecar delete must never roll back the document +delete that triggered it. + +## Not a breaking change — corrected while building + +The design this section was first written for threaded a required parameter, and that would have +stopped a third-party `TaskRunner` from compiling. What shipped is different: `enqueue`'s scope and +`TaskHandler`'s scope are both **optional**, so an implementation written against the old signature +still satisfies the type. `TaskRunnerProvider.create` is untouched. No `BREAKING CHANGE:` footer, and +nothing to add to `docs/DEPRECATIONS.md`. + +What does change for a host is **behaviour**, and only on the sync runner: an auto-translation now +runs inside the transaction of the save that triggered it. Two consequences worth a README line — +the translation is no longer visible to another connection until that save commits, and a translation +that fails with a database error can, on Postgres, fail the save it was triggered by. The user +accepted the second; the escape hatch for a host it hurts is `createPayloadJobsRunner()`, whose jobs +are picked up after the commit. + +## Pre-existing, deliberately not fixed here + +Thirteen files thread the `Payload` god type into leaf helpers — `fetchSourceDocument`, +`TranslateDocumentHandler.handle`, `ProvenanceServiceFactory`. The same objection that rejected +threading `PayloadRequest`, one level down. Not this task's work; recorded so the next reader knows it +was seen rather than missed. + +## Acceptance criteria + +1. **Auto-translate works on Postgres.** The three currently-failing cases in `auto-translate.int.test.ts` + and `auto-translate-unknown-locale.int.test.ts` pass. *Check: `DB_ADAPTER=postgres` integration run.* + Fails now — this is the reproduction. +2. **The transaction reaches the source read.** *Check: unit test on `TranslateDocumentHandler.handle` + asserting `payload.findByID` was called with the caller's `transactionID`.* Fails now, and fails on + every adapter — unlike criterion 1, which only speaks on Postgres. This replaces an earlier version + that asserted only what `create` received: that one would have gone green while the bug stayed, + because the scope has to reach the operation, not the runner. +3. **A queued job does not survive a rollback.** *Check: new integration test, jobs runner, explicit + transaction, Postgres-gated.* Fails now — measured at 1 surviving row. Covers both write paths of + `enqueue`: queueing a new job and appending to a live one. +4. **A failed translation is logged when no `onFailed` is configured.** *Check: unit test on + `LifecycleNotifier` asserting the logger is called.* Fails now. +5. **A configured `onFailed` still receives the failure, and is not double-reported.** *Check: unit test.* +6. **Both runners still satisfy the interface.** *Check: check-types in both packages.* +7. **Nothing regresses on SQLite or MongoDB.** *Check: integration suite on both, in both queue modes.* +8. **The caller's request is not mutated.** *Check: unit test — after a translation the hook's own + request carries no translator context flag.* Guards the bulk-publish regression the rejected design + would have introduced. +9. **Checks clean:** unit tests, check-types **and lint** at the repo baseline. + +## Pre-flight — each criterion run against the untouched tree + +1, 3, 4 fail now, which is what makes them able to tell done from not-done. 6, 7, 8 pass now. + +**Criterion 2 was rewritten after its pre-flight failed.** It first read "a rolled-back source write +leaves no translated target" — and that passes today, for the wrong reason: nothing is translated at +all, so there is nothing to roll back. A criterion that cannot distinguish the fix from the bug is +worthless, so it now checks the mechanism (what the hook passes) rather than a consequence both +states share. + +**Measured while checking it:** an explicit `beginTransaction` / `rollbackTransaction` does *not* roll +the document back on SQLite — the row survives — while it does on Postgres. Every criterion about +transaction behaviour is therefore Postgres-only, and the adapter-independent guard has to be a unit +test. Worth knowing before writing a rollback test that would have passed vacuously on the default +adapter. + +## Human choices + +- **Fix all three in one pass** rather than splitting them. +- **Tests first**, red against the broken code, then the fix. +- **Carry a one-field slice, not the request** — raised as an abstraction objection ("we need one + field, why drag the whole request through every handler"), and it turned out to also prevent a + bulk-publish data loss. +- **Accept that a failed translation can, on Postgres, fail the save.** Once the inline translation + runs inside the caller's transaction, a *failed SQL statement* inside it poisons the transaction and + every later statement in the save fails with it. That contradicts the hook's best-effort contract, + and the user accepted it rather than nesting the translation in a savepoint or leaving the sync + runner outside the transaction (which would leave this bug unfixed for it). Noted as unmeasured: the + two failures we know of — a missing source row and an unreachable provider — are JavaScript throws + with no failed statement, so they may not poison anything; nobody has measured which failures do. +- **Log the failure, do not fail the save.** The user rejected the alternative on the grounds that a + translation failure can be asynchronous: with the jobs runner it happens after the save has + committed, so "fail the save" is not a behaviour that runner can have at all. + +## Risks + +- With the sync runner the translation now runs inside the caller's transaction, so a provider call + holds the transaction open for its duration. That is inherent to translating inline and is already + true today — it is merely failing instead of waiting. Worth stating for hosts who use the sync + runner on production traffic; the jobs runner has no such exposure. +- Threading `req` into the jobs runner means its reads also join the caller's transaction. For the + status endpoints that is the same connection they already use; for `enqueue` it is the point. +- Defect 2's fallback logging will make previously-silent failures appear in host logs. That is the + intent, but a host with a broken provider will suddenly see noise that was always there. + +## Review log + +_(appended by review runs)_ + +## Review log + +**2026-09-14 — built and verified.** Red tests first on untouched source: 8 red over four files, every +failure an assertion reaching the unit. All nine criteria met; the six adapter/queue-mode integration +runs are green. + +**Corrected while building — the first implementation was inert.** The scope was passed as a top-level +`transactionID` option on each call. Payload's local API has no such option: `createLocalReq` reads the +transaction from `options.req`. The unit tests went green against that, a false green, and only the +Postgres integration run exposed it. Every call now passes `req: { ...scope }` — a fresh copy per call, +because `createLocalReq` fills the object it is handed in place and one shared copy would carry the +first call's locale into the next. + +**The rollback spec was mutation-proved.** With the hook reverted to pass an empty scope and the plugin +rebuilt, both of its cases go red: one surviving job row on the queue path, two jobs instead of one on +the append path. + +**The logger fallback paid for itself immediately.** Before it, the "Not Found" that stopped every +translation on Postgres appeared nowhere. After it, one run of the failing spec printed the cause. + +**Not a breaking change, contrary to the original plan.** Both new parameters are optional, so a +third-party `TaskRunner` still satisfies the interface. The behavioural change — the sync runner +translating inside the caller's transaction — is documented in the README instead. + +**The accepted risk was measured after all, and is narrower than it was accepted as.** +`transaction-failure-isolation.int.test.ts` runs a provider failure inside the caller's transaction on +Postgres: the save commits, the locale that did not fail still translates, and the failure is logged. +So the exposure is confined to a failure that leaves a *rejected SQL statement* in the transaction — +not to the failures a host actually meets. The README says so rather than implying the wider risk. + +**2026-09-14 — the reads-only design was measured and abandoned, and the fix was rebuilt.** The user +chose "reads inside the transaction, writes outside" over the silent-loss risk. It does not build: the +translation writes to the document the caller has not committed, so from outside the transaction +`payload.update` answers "Not Found" and the three #124 cases go red again — the same failure the task +set out to fix, moved from the read to the write. Recorded here because the option is the obvious one +to reach for and the reason it fails is not obvious until measured. + +What shipped instead keeps every operation inside the transaction and surfaces a failure that has +already destroyed it. Both halves are pinned by integration tests on Postgres: a provider outage +leaves the save intact, and a value Payload rejects makes the save fail with that error instead of +vanishing. + +**The bulk-publish guard now exists and discriminates.** Gate A found that nothing anywhere exercised +a bulk update, which is the regression the narrowed slice was chosen to prevent. The new case +translates two documents through one shared request; reverting to the rejected design — the live +request travelling uncopied — turns it red. + +**Nine of nine criteria met**, with criterion 3 rewritten: it asserted that no job survives a +rollback, which the jobs runner no longer promises because it takes no scope. It now asserts that the +sync runner's translation and its provenance receipt are atomic with the save, and carries a positive +control so it cannot pass by translating nothing. diff --git a/packages/payload-plugin-translator/src/server/features/translate-document/handler.test.ts b/packages/payload-plugin-translator/src/server/features/translate-document/handler.test.ts index 873e38b0..406fc24a 100644 --- a/packages/payload-plugin-translator/src/server/features/translate-document/handler.test.ts +++ b/packages/payload-plugin-translator/src/server/features/translate-document/handler.test.ts @@ -101,6 +101,7 @@ describe("TranslateDocumentHandler", () => { await handler.handle(mockPayload, input); expect(mockPayload.findByID).toHaveBeenCalledWith({ + req: {}, collection: "posts", id: "doc-123", locale: "en", @@ -116,6 +117,7 @@ describe("TranslateDocumentHandler", () => { await handler.handle(mockPayload, input); expect(mockPayload.findByID).toHaveBeenCalledWith({ + req: {}, collection: "posts", id: "doc-123", locale: "de", @@ -402,7 +404,7 @@ describe("TranslateDocumentHandler", () => { expect(computeSourceFingerprint).toHaveBeenCalledWith({ id: "doc-123", title: "Source" }, [ { name: "title", type: "text", localized: true }, ]); - expect(serviceFactory).toHaveBeenCalledWith(mockPayload); + expect(serviceFactory).toHaveBeenCalledWith(mockPayload, {}); expect(store.upsert).toHaveBeenCalledWith( expect.objectContaining({ collectionSlug: "posts", diff --git a/packages/payload-plugin-translator/src/server/features/translate-document/handler.transaction.test.ts b/packages/payload-plugin-translator/src/server/features/translate-document/handler.transaction.test.ts new file mode 100644 index 00000000..c012f564 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/features/translate-document/handler.transaction.test.ts @@ -0,0 +1,110 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { Payload, CollectionSlug } from "payload"; + +import { TranslateDocumentHandler } from "./handler"; +import type { TranslationProvider } from "../../../core/domain/translation-providers"; +import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap"; +import type { TranslateDocumentInput } from "./model"; +import type { ProvenanceServiceFactory } from "../../modules/provenance"; + +vi.mock("../../../core/translation-pipeline", () => ({ + translateContent: vi.fn().mockResolvedValue({ title: "Titel" }), +})); + +const TX = "tx-42"; + +describe("TranslateDocumentHandler — the caller's transaction", () => { + let handler: TranslateDocumentHandler; + let payload: Payload; + + const input = (over: Partial = {}): TranslateDocumentInput => ({ + collection: "posts" as CollectionSlug, + collectionId: "doc-123", + sourceLng: "en", + targetLng: "de", + strategy: "overwrite", + publishOnTranslation: false, + ...over, + }); + + beforeEach(() => { + vi.clearAllMocks(); + const provider: TranslationProvider = { translate: vi.fn().mockResolvedValue({}) }; + const schemaMap = new Map([ + ["posts" as CollectionSlug, [{ name: "title", type: "text", localized: true }]], + ]) as CollectionSchemaMap; + + payload = { + findByID: vi.fn().mockResolvedValue({ id: "doc-123", title: "Test" }), + update: vi.fn().mockResolvedValue({}), + logger: { error: vi.fn(), info: vi.fn(), warn: vi.fn() }, + collections: { + posts: { config: { versions: { drafts: true } } }, + }, + } as unknown as Payload; + + handler = new TranslateDocumentHandler(provider, schemaMap); + }); + + it("joins the source read to it", async () => { + await handler.handle(payload, input(), { transactionID: TX }); + + expect(payload.findByID).toHaveBeenCalledWith( + expect.objectContaining({ locale: "en", req: { transactionID: TX } }) + ); + }); + + it("joins the target read to it", async () => { + await handler.handle(payload, input(), { transactionID: TX }); + + expect(payload.findByID).toHaveBeenCalledWith( + expect.objectContaining({ locale: "de", req: { transactionID: TX } }) + ); + }); + + // The write has to join it too: it targets the very document the caller has not committed yet, so + // from outside the transaction there is nothing to update. + it("joins the translation write to it", async () => { + await handler.handle(payload, input(), { transactionID: TX }); + + expect(payload.update).toHaveBeenCalledWith( + expect.objectContaining({ req: { transactionID: TX } }) + ); + }); + + it("joins the publish write to it", async () => { + await handler.handle(payload, input({ publishOnTranslation: true }), { transactionID: TX }); + + const publishCall = (payload.update as ReturnType).mock.calls.find( + ([args]) => (args as { publishSpecificLocale?: string }).publishSpecificLocale === "de" + ); + expect(publishCall).toBeDefined(); + expect(publishCall?.[0]).toMatchObject({ req: { transactionID: TX } }); + }); + + it("builds the provenance service on it, so the receipt rolls back with the translation", async () => { + const serviceFactory = vi.fn().mockReturnValue({ + captureFingerprint: vi.fn().mockReturnValue("fp"), + record: vi.fn(), + }); + const provenanceHandler = new TranslateDocumentHandler( + { translate: vi.fn().mockResolvedValue({}) }, + new Map([ + ["posts" as CollectionSlug, [{ name: "title", type: "text", localized: true }]], + ]) as CollectionSchemaMap, + serviceFactory as unknown as ProvenanceServiceFactory + ); + + await provenanceHandler.handle(payload, input(), { transactionID: TX }); + + expect(serviceFactory).toHaveBeenCalledWith(payload, { transactionID: TX }); + }); + + it("sends no transactionID when the caller has none", async () => { + await handler.handle(payload, input(), {}); + + for (const [args] of (payload.findByID as ReturnType).mock.calls) { + expect((args as { req: object }).req).toEqual({}); + } + }); +}); diff --git a/packages/payload-plugin-translator/src/server/features/translate-document/handler.ts b/packages/payload-plugin-translator/src/server/features/translate-document/handler.ts index 47c712da..fa3c4387 100644 --- a/packages/payload-plugin-translator/src/server/features/translate-document/handler.ts +++ b/packages/payload-plugin-translator/src/server/features/translate-document/handler.ts @@ -6,6 +6,8 @@ import type { TranslationProvider } from "../../../core/domain/translation-provi import { translateContent } from "../../../core/translation-pipeline"; import type { ProvenanceServiceFactory } from "../../modules/provenance"; import { fetchSourceDocument } from "../../shared/payload/sourceDocument"; +import type { TransactionScope } from "../../shared/payload/TransactionScope.shapes"; +import { freshReq } from "../../shared/payload/TransactionScope.shapes"; import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap"; import { AUTO_TRANSLATE_SKIP_CONTEXT_KEY } from "../../../types/AutoTranslateContext"; @@ -42,7 +44,11 @@ export class TranslateDocumentHandler implements Handler< this.inlineMarks = inlineMarks; } - async handle(payload: Payload, input: TranslateDocumentInput): Promise { + async handle( + payload: Payload, + input: TranslateDocumentInput, + scope: TransactionScope = {} + ): Promise { const { collection, collectionId, sourceLng, targetLng, strategy, publishOnTranslation } = input; @@ -55,11 +61,12 @@ export class TranslateDocumentHandler implements Handler< }); // `draft: true` is unconditional: on a collection without drafts Payload has no version to - // substitute, so it returns the only row. The WRITE cannot be so relaxed — the `no-drafts` - // layer omits `draft` entirely, because that is the argument shape `main` sent. + // substitute, so it returns the only row. The write cannot be as relaxed — the `no-drafts` + // layer omits `draft` entirely. const [sourceData, currentTargetVersion] = await Promise.all([ - fetchSourceDocument(payload, collection, collectionId, sourceLng), + fetchSourceDocument(payload, collection, collectionId, sourceLng, scope), payload.findByID({ + req: freshReq(scope), collection, id: collectionId, locale: targetLng, @@ -69,7 +76,7 @@ export class TranslateDocumentHandler implements Handler< }), ]); - const provenance = this.provenanceServiceFactory?.(payload); + const provenance = this.provenanceServiceFactory?.(payload, scope); const sourceFingerprint = provenance?.captureFingerprint(collection, sourceData) ?? null; const translatedData = await translateContent({ @@ -84,7 +91,7 @@ export class TranslateDocumentHandler implements Handler< }); if (translatedData) { - await this.saveTranslatedDocument(payload, input, translatedData, layer.write); + await this.saveTranslatedDocument(payload, input, translatedData, layer.write, scope); if (provenance && sourceFingerprint !== null) { await provenance.record( @@ -100,7 +107,7 @@ export class TranslateDocumentHandler implements Handler< } if (publishOnTranslation && layer.kind === "drafts") { - await this.publishTargetLocale(payload, input, layer.publish); + await this.publishTargetLocale(payload, input, layer.publish, scope); } return { success: true }; @@ -110,9 +117,11 @@ export class TranslateDocumentHandler implements Handler< payload: Payload, input: TranslateDocumentInput, translatedData: Record, - write: TargetLayer["write"] + write: TargetLayer["write"], + scope: TransactionScope ): Promise { await payload.update({ + req: freshReq(scope), collection: input.collection, id: input.collectionId, data: translatedData, @@ -126,9 +135,11 @@ export class TranslateDocumentHandler implements Handler< private async publishTargetLocale( payload: Payload, input: TranslateDocumentInput, - publish: PublishScope + publish: PublishScope, + scope: TransactionScope ): Promise { await payload.update({ + req: freshReq(scope), collection: input.collection, id: input.collectionId, data: { _status: publish.status }, diff --git a/packages/payload-plugin-translator/src/server/features/translate-document/wireTranslateRunner.ts b/packages/payload-plugin-translator/src/server/features/translate-document/wireTranslateRunner.ts index 8ad89cfd..40352208 100644 --- a/packages/payload-plugin-translator/src/server/features/translate-document/wireTranslateRunner.ts +++ b/packages/payload-plugin-translator/src/server/features/translate-document/wireTranslateRunner.ts @@ -56,18 +56,22 @@ export function wireTranslateRunner({ ); const runnerContext: TaskRunnerContext = { - handler: async (payload, input) => { + handler: async (payload, input, scope) => { const notifier = new LifecycleNotifier(lifecycle, payload.logger); const task = taskFromHandlerInput(input); try { - await translateHandler.handle(payload, { - collection: input.collection, - collectionId: input.collectionId, - sourceLng: input.sourceLng, - targetLng: input.targetLng, - strategy: input.strategy, - publishOnTranslation: input.publishOnTranslation, - }); + await translateHandler.handle( + payload, + { + collection: input.collection, + collectionId: input.collectionId, + sourceLng: input.sourceLng, + targetLng: input.targetLng, + strategy: input.strategy, + publishOnTranslation: input.publishOnTranslation, + }, + scope + ); } catch (error) { await notifier.failed(task, error); throw error; // rethrow so the runner marks the job failed diff --git a/packages/payload-plugin-translator/src/server/features/translate-field/handler.test.ts b/packages/payload-plugin-translator/src/server/features/translate-field/handler.test.ts index 571c07bd..52fde21e 100644 --- a/packages/payload-plugin-translator/src/server/features/translate-field/handler.test.ts +++ b/packages/payload-plugin-translator/src/server/features/translate-field/handler.test.ts @@ -106,6 +106,7 @@ describe("TranslateFieldHandler", () => { expect(res.status).toBe(200); expect((await res.json()).data).toEqual({ status: "translated", value: "Hallo" }); expect(findByID).toHaveBeenCalledWith({ + req: {}, collection: "posts", id: "p1", locale: "en", diff --git a/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.hook.ts b/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.hook.ts index 74f33dd6..c93e3d0d 100644 --- a/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.hook.ts +++ b/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.hook.ts @@ -1,6 +1,8 @@ import type { CollectionAfterChangeHook } from "payload"; import { hasDraftsEnabled } from "payload/shared"; +import { killedTheCallersTransaction } from "../../shared/payload/TransactionScope.shapes"; + import { hasSourceContentChanged } from "../../../core/domain/auto-translate"; import { AUTO_TRANSLATE_CUSTOM_KEY } from "../../../core/domain/auto-translate"; import { AUTO_TRANSLATE_SKIP_CONTEXT_KEY } from "../../../types/AutoTranslateContext"; @@ -43,6 +45,9 @@ export function makeAutoTranslateHook(deps: AutoTranslateHookDeps): CollectionAf const { resolvePolicy, schemaMap, taskRunnerFactory } = deps; const hook: MarkedHook = async ({ doc, previousDoc, req, collection }) => { + // Declared outside the `try` so the catch can tell a failure that reached a Payload operation + // from one raised before the id was settled — only the former can have killed the transaction. + let transactionID: string | number | undefined; try { if (req.context?.[AUTO_TRANSLATE_SKIP_CONTEXT_KEY]) return doc; @@ -80,7 +85,12 @@ export function makeAutoTranslateHook(deps: AutoTranslateHookDeps): CollectionAf }); if (tasks.length === 0) return doc; - await taskRunnerFactory.create(req.payload).enqueue(tasks); + // Settled first: Payload parks a promise in this field while the transaction opens, and a + // promise reaching the adapter as a transaction key is silently wrong. + transactionID = await req.transactionID; + await taskRunnerFactory + .create(req.payload) + .enqueue(tasks, transactionID == null ? {} : { transactionID }); } catch (error) { req.payload.logger.error({ err: error, @@ -88,6 +98,7 @@ export function makeAutoTranslateHook(deps: AutoTranslateHookDeps): CollectionAf documentId: String(doc.id), msg: "translator: auto-translate hook failed", }); + if (killedTheCallersTransaction({ transactionID }, error)) throw error; } return doc; }; diff --git a/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.transaction.test.ts b/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.transaction.test.ts new file mode 100644 index 00000000..8d7a4f84 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.transaction.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { CollectionAfterChangeHook, CollectionSlug, Field } from "payload"; + +import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap"; +import type { TaskRunnerFactory } from "../task-runner/TaskRunnerProvider.interface"; +import { makeCollectionPolicyResolver } from "./AutoTranslate.policy"; +import type { NormalizedAutoTranslatePolicy } from "./AutoTranslate.policy"; +import { makeAutoTranslateHook } from "./AutoTranslateEnqueue.hook"; + +const schemaMap: CollectionSchemaMap = new Map([ + ["posts" as CollectionSlug, [{ name: "title", type: "text", localized: true }] as Field[]], +]); +const policy: NormalizedAutoTranslatePolicy = { + targets: ["de"], + strategy: "overwrite", + debounceMs: 0, +}; + +const logger = { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() }; + +function setup() { + const enqueue = vi.fn().mockResolvedValue(undefined); + const taskRunnerFactory = { + create: vi.fn().mockReturnValue({ enqueue }), + } as unknown as TaskRunnerFactory; + const hook = makeAutoTranslateHook({ + resolvePolicy: makeCollectionPolicyResolver(new Map([["posts", policy]])), + schemaMap, + taskRunnerFactory, + }); + return { hook, enqueue }; +} + +function hookArgs(req: Record) { + return { + doc: { id: "1", title: "NEW", _status: "published" }, + previousDoc: { id: "1", title: "OLD", _status: "published" }, + collection: { slug: "posts", versions: { drafts: true } }, + operation: "update", + req: { + locale: "en", + context: {}, + payload: { + logger, + config: { localization: { defaultLocale: "en", locales: ["en", "de"] } }, + }, + ...req, + }, + } as unknown as Parameters[0]; +} + +describe("auto-translate hook — the triggering transaction", () => { + beforeEach(() => vi.clearAllMocks()); + + it("hands the triggering request's transaction id to enqueue", async () => { + const { hook, enqueue } = setup(); + + await hook(hookArgs({ transactionID: "tx-99" })); + + expect(enqueue).toHaveBeenCalledWith(expect.anything(), { transactionID: "tx-99" }); + }); + + it("settles a transaction id that is still a promise", async () => { + const { hook, enqueue } = setup(); + + await hook(hookArgs({ transactionID: Promise.resolve("tx-9") })); + + expect(enqueue).toHaveBeenCalledWith(expect.anything(), { transactionID: "tx-9" }); + }); + + it("hands no transaction id when the request has none", async () => { + const { hook, enqueue } = setup(); + + await hook(hookArgs({})); + + const scope = enqueue.mock.calls[0]?.[1] as { transactionID?: unknown } | undefined; + expect(scope?.transactionID).toBeUndefined(); + }); + + // Payload reuses one request across every document of a bulk update, so anything written onto it + // here would leak to the next document in the batch. + it("does not mutate the triggering request", async () => { + const { hook } = setup(); + const args = hookArgs({ transactionID: "tx-99" }); + const before = JSON.stringify(Object.keys(args.req).sort()); + const contextBefore = JSON.stringify(args.req.context); + + await hook(args); + + expect(JSON.stringify(Object.keys(args.req).sort())).toBe(before); + expect(JSON.stringify(args.req.context)).toBe(contextBefore); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.test.ts b/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.test.ts index 049952ec..492f73ff 100644 --- a/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.test.ts @@ -103,3 +103,34 @@ describe("LifecycleNotifier", () => { expect(done).toBe(true); }); }); + +describe("LifecycleNotifier — a failure with no onFailed configured", () => { + it("logs the failure", async () => { + const logger = makeLogger(); + const error = new Error("provider unreachable"); + + await new LifecycleNotifier({}, logger).failed(task, error); + + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith(expect.objectContaining({ err: error })); + }); + + it("names the document and the locale it failed for", async () => { + const logger = makeLogger(); + + await new LifecycleNotifier({}, logger).failed(task, new Error("boom")); + + const logged = logger.error.mock.calls[0]?.[0] as Record; + expect(logged).toMatchObject({ collection: "posts", id: "doc-1", targetLng: "de" }); + }); + + it("does not log when a configured onFailed handled it", async () => { + const logger = makeLogger(); + const onFailed = vi.fn(); + + await new LifecycleNotifier({ onFailed }, logger).failed(task, new Error("boom")); + + expect(onFailed).toHaveBeenCalledTimes(1); + expect(logger.error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.ts b/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.ts index 7e90d791..a947c493 100644 --- a/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.ts +++ b/packages/payload-plugin-translator/src/server/modules/lifecycle/LifecycleNotifier.ts @@ -28,9 +28,19 @@ export class LifecycleNotifier { return this.safe("lifecycle.onCompleted", callback && (() => callback(task))); } - failed(task: TranslationTask, error: unknown): Promise { + async failed(task: TranslationTask, error: unknown): Promise { const callback = this.callbacks.onFailed; - return this.safe("lifecycle.onFailed", callback && (() => callback(task, error))); + if (!callback) { + this.logger.error({ + err: error, + collection: task.collection, + id: task.id, + targetLng: task.targetLng, + msg: "translator: translation failed", + }); + return; + } + await this.safe("lifecycle.onFailed", () => callback(task, error)); } private async safe(name: string, thunk?: () => void | Promise): Promise { diff --git a/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.test.ts b/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.test.ts index b8ef2a8c..d26771b6 100644 --- a/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.test.ts @@ -35,7 +35,7 @@ describe("withQueuedNotification", () => { expect.objectContaining({ collection: "posts", id: "1", sourceLng: "en", targetLng: "de" }) ); expect(onQueued).toHaveBeenNthCalledWith(2, expect.objectContaining({ id: "2" })); - expect(runner.enqueue).toHaveBeenCalledWith([input("1"), input("2")]); + expect(runner.enqueue).toHaveBeenCalledWith([input("1"), input("2")], undefined); }); it("fires queued BEFORE delegating to the runner (ordering vs a synchronous runner)", async () => { diff --git a/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.ts b/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.ts index 5085c0e3..1507ad72 100644 --- a/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.ts +++ b/packages/payload-plugin-translator/src/server/modules/lifecycle/withQueuedNotification.ts @@ -14,9 +14,9 @@ export function withQueuedNotification( notifier: LifecycleNotifier ): TaskRunner { return { - async enqueue(tasks) { + async enqueue(tasks, scope) { await Promise.all(tasks.map((task) => notifier.queued(taskFromInput(task)))); - await runner.enqueue(tasks); + await runner.enqueue(tasks, scope); }, cancel: (taskIds) => runner.cancel(taskIds), run: (taskId) => runner.run(taskId), diff --git a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.test.ts b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.test.ts index ee9e2677..c1e15097 100644 --- a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from "vitest"; import type { Field, Payload } from "payload"; +import { APIError } from "payload"; import type { ProvenanceStore, TranslationProvenanceRecord } from "../../../core/domain/provenance"; import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap"; @@ -101,6 +102,32 @@ describe("ProvenanceService", () => { expect(payload.logger.error as ReturnType).toHaveBeenCalled(); }); + it("record rethrows a Payload failure raised inside the caller's transaction", async () => { + const payload = makePayload(async () => sourceDoc); + const store = makeStore({ upsert: vi.fn().mockRejectedValue(new APIError("rejected")) }); + const service = new ProvenanceService(payload, store, schemaMap, { transactionID: "tx-1" }); + + await expect( + service.record( + { collectionSlug: COLLECTION, documentId: "1", targetLocale: "de", sourceLocale: "en" }, + "fp" + ) + ).rejects.toThrow(APIError); + }); + + it("record stays best-effort inside a transaction when the failure reached no Payload operation", async () => { + const payload = makePayload(async () => sourceDoc); + const store = makeStore({ upsert: vi.fn().mockRejectedValue(new Error("table down")) }); + const service = new ProvenanceService(payload, store, schemaMap, { transactionID: "tx-1" }); + + await expect( + service.record( + { collectionSlug: COLLECTION, documentId: "1", targetLocale: "de", sourceLocale: "en" }, + "fp" + ) + ).resolves.toBeUndefined(); + }); + it("dismiss persists the current source fingerprint for a locale that has a record", async () => { const dismiss = vi.fn().mockResolvedValue(undefined); const store = makeStore({ find: vi.fn().mockResolvedValue(record()), dismiss }); diff --git a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.ts b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.ts index 2244c029..d65cf568 100644 --- a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.ts +++ b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.service.ts @@ -4,6 +4,8 @@ import { computeSourceFingerprint } from "../../../core/domain/content-projectio import type { FieldLike } from "../../../core/kernel/field-traversal"; import { isRecordStale } from "../../../core/domain/provenance"; import type { ProvenanceKey, ProvenanceStore } from "../../../core/domain/provenance"; +import type { TransactionScope } from "../../shared/payload/TransactionScope.shapes"; +import { killedTheCallersTransaction } from "../../shared/payload/TransactionScope.shapes"; import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap"; import { fetchSourceDocument } from "../../shared/payload/sourceDocument"; @@ -16,7 +18,10 @@ export type StalenessLocale = { }; /** Builds a {@link ProvenanceService} bound to a Payload instance; absent when provenance is disabled. */ -export type ProvenanceServiceFactory = (payload: Payload) => ProvenanceService; +export type ProvenanceServiceFactory = ( + payload: Payload, + scope?: TransactionScope +) => ProvenanceService; /** * The single owner of provenance fingerprint policy — how the source is hashed on write, re-hashed on @@ -31,11 +36,18 @@ export class ProvenanceService { private readonly payload: Payload; private readonly store: ProvenanceStore; private readonly schemaMap: CollectionSchemaMap; + private readonly scope: TransactionScope; - constructor(payload: Payload, store: ProvenanceStore, schemaMap: CollectionSchemaMap) { + constructor( + payload: Payload, + store: ProvenanceStore, + schemaMap: CollectionSchemaMap, + scope: TransactionScope = {} + ) { this.payload = payload; this.store = store; this.schemaMap = schemaMap; + this.scope = scope; } /** @@ -89,6 +101,7 @@ export class ProvenanceService { sourceLocale: key.sourceLocale, msg: "translator: failed to record translation provenance", }); + if (killedTheCallersTransaction(this.scope, error)) throw error; } } diff --git a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.store.ts b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.store.ts index 1fba6021..1d9f81a5 100644 --- a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.store.ts +++ b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.store.ts @@ -1,4 +1,7 @@ import type { CollectionSlug, Payload, Where } from "payload"; + +import type { TransactionScope } from "../../shared/payload/TransactionScope.shapes"; +import { freshReq } from "../../shared/payload/TransactionScope.shapes"; import type { ProvenanceKey, ProvenanceStore, @@ -6,7 +9,10 @@ import type { } from "../../../core/domain/provenance"; /** Builds a provenance store bound to a Payload instance; absent when provenance is disabled. */ -export type ProvenanceStoreFactory = (payload: Payload) => ProvenanceStore; +export type ProvenanceStoreFactory = ( + payload: Payload, + scope?: TransactionScope +) => ProvenanceStore; interface ProvenanceDoc extends Record { id: string | number; @@ -54,24 +60,40 @@ function toRecord(doc: ProvenanceDoc): TranslationProvenanceRecord { export class PayloadProvenanceStore implements ProvenanceStore { private readonly payload: Payload; private readonly collection: CollectionSlug; + private readonly scope: TransactionScope; - constructor(payload: Payload, slug: string) { + constructor(payload: Payload, slug: string, scope: TransactionScope = {}) { this.payload = payload; this.collection = slug as CollectionSlug; + this.scope = scope; + } + + private req(): TransactionScope { + return freshReq(this.scope); } async upsert(record: TranslationProvenanceRecord): Promise { const existing = await this.findDoc(record); if (existing === null) { try { - await this.payload.create({ collection: this.collection, data: record }); + await this.payload.create({ req: this.req(), collection: this.collection, data: record }); } catch (error) { const raceWinner = await this.findDoc(record); if (raceWinner === null) throw error; - await this.payload.update({ collection: this.collection, id: raceWinner.id, data: record }); + await this.payload.update({ + req: this.req(), + collection: this.collection, + id: raceWinner.id, + data: record, + }); } } else { - await this.payload.update({ collection: this.collection, id: existing.id, data: record }); + await this.payload.update({ + req: this.req(), + collection: this.collection, + id: existing.id, + data: record, + }); } } @@ -85,6 +107,7 @@ export class PayloadProvenanceStore implements ProvenanceStore { documentId: string ): Promise { const result = await this.payload.find({ + req: this.req(), collection: this.collection, where: documentWhere(collectionSlug, documentId), depth: 0, @@ -97,12 +120,17 @@ export class PayloadProvenanceStore implements ProvenanceStore { const existing = await this.findDoc(key); if (existing === null) return; await this.payload.update({ + req: this.req(), collection: this.collection, id: existing.id, data: { dismissedFingerprint }, }); } + /** + * Deliberately outside the caller's transaction, unlike every other write here: a failed sidecar + * delete must never roll back the document delete that triggered it. + */ async deleteByDocument(collectionSlug: string, documentId: string): Promise { await this.payload.delete({ collection: this.collection, @@ -112,6 +140,7 @@ export class PayloadProvenanceStore implements ProvenanceStore { private async findDoc(key: ProvenanceKey): Promise { const result = await this.payload.find({ + req: this.req(), collection: this.collection, where: keyWhere(key), limit: 1, diff --git a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.wiring.ts b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.wiring.ts index 9e6798a7..dca56986 100644 --- a/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.wiring.ts +++ b/packages/payload-plugin-translator/src/server/modules/provenance/Provenance.wiring.ts @@ -52,10 +52,10 @@ export function configureProvenance( const slug = resolveProvenanceSlug(option); if (!slug) return { configure: () => NOOP }; - const storeFactory: ProvenanceStoreFactory = (payload) => - new PayloadProvenanceStore(payload, slug); - const serviceFactory: ProvenanceServiceFactory = (payload) => - new ProvenanceService(payload, storeFactory(payload), schemaMap); + const storeFactory: ProvenanceStoreFactory = (payload, scope) => + new PayloadProvenanceStore(payload, slug, scope); + const serviceFactory: ProvenanceServiceFactory = (payload, scope) => + new ProvenanceService(payload, storeFactory(payload, scope), schemaMap, scope); const configure = (managedSlugs: Set): ConfigModifier => diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts index 5f4f4496..3bf0cd57 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts @@ -1,6 +1,7 @@ import type { CollectionSlug } from "payload"; import type { Task, TaskInput, RunResult } from "./types"; +import type { TransactionScope } from "../../shared/payload/TransactionScope.shapes"; /** * Interface for task execution backends. @@ -14,8 +15,11 @@ export interface TaskRunner { /** * Queue translation tasks for execution. * Implementation handles cancellation of existing tasks for the same documents. + * + * `scope` joins the reads and writes this makes to the caller's transaction; omit it outside one — + * an HTTP route — and each operation opens its own. */ - enqueue(tasks: TaskInput[]): Promise; + enqueue(tasks: TaskInput[], scope?: TransactionScope): Promise; /** * Cancel tasks by IDs. diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunnerProvider.interface.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunnerProvider.interface.ts index db05eb17..afbfacc6 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunnerProvider.interface.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunnerProvider.interface.ts @@ -2,6 +2,7 @@ import type { CollectionSlug, Config, Payload } from "payload"; import type { TaskRunner } from "./TaskRunner.interface"; import type { ID } from "./types"; import type { TranslationStrategyName } from "../../../core/translation-pipeline/strategies"; +import type { TransactionScope } from "../../shared/payload/TransactionScope.shapes"; /** * Input for task handler callback @@ -18,7 +19,11 @@ export type TaskHandlerInput = { /** * Task handler callback — plugin passes its internal logic */ -export type TaskHandler = (payload: Payload, input: TaskHandlerInput) => Promise; +export type TaskHandler = ( + payload: Payload, + input: TaskHandlerInput, + scope?: TransactionScope +) => Promise; /** * Context for runner configuration diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncRunnerProvider.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncRunnerProvider.test.ts index f253437f..c19fe5cf 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncRunnerProvider.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncRunnerProvider.test.ts @@ -47,7 +47,8 @@ describe("SyncRunnerProvider", () => { targetLng: "de", strategy: "overwrite", publishOnTranslation: false, - }) + }), + {} ); }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.test.ts index 312d411d..9d9fccf2 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.test.ts @@ -36,14 +36,18 @@ describe("SyncTaskRunner", () => { const input = createInput(); await runner.enqueue([input]); - expect(mockHandler).toHaveBeenCalledWith(mockPayload, { - collection: "posts", - collectionId: "doc-123", - sourceLng: "en", - targetLng: "de", - strategy: "overwrite", - publishOnTranslation: false, - }); + expect(mockHandler).toHaveBeenCalledWith( + mockPayload, + { + collection: "posts", + collectionId: "doc-123", + sourceLng: "en", + targetLng: "de", + strategy: "overwrite", + publishOnTranslation: false, + }, + {} + ); }); it("ignores waitUntil and runs immediately (dev runner — no debounce)", async () => { diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.transaction.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.transaction.test.ts new file mode 100644 index 00000000..5c510610 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.transaction.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect, vi } from "vitest"; +import type { Payload, CollectionSlug } from "payload"; + +import { SyncTaskRunner } from "./SyncTaskRunner"; +import type { Task, TaskInput } from "../types"; +import { LazyMap } from "../../../shared/utils"; + +const task: TaskInput = { + collectionSlug: "posts" as CollectionSlug, + collectionId: "doc-1", + sourceLng: "en", + targetLng: "de", + strategy: "overwrite", + publishOnTranslation: false, +}; + +function makeRunner() { + const handler = vi.fn().mockResolvedValue(undefined); + const tasks = new LazyMap({ + isRemovable: (t) => t.status === "completed" || t.status === "failed", + getTimestamp: (t) => new Date(t.updatedAt).getTime(), + }); + return { handler, runner: new SyncTaskRunner({} as Payload, handler, tasks) }; +} + +describe("SyncTaskRunner — the caller's transaction", () => { + it("hands the scope to the handler", async () => { + const { handler, runner } = makeRunner(); + + await runner.enqueue([task], { transactionID: "tx-7" }); + + expect(handler).toHaveBeenCalledWith(expect.anything(), expect.anything(), { + transactionID: "tx-7", + }); + }); + + it("hands no transaction id to the handler when the caller has none", async () => { + const { handler, runner } = makeRunner(); + + await runner.enqueue([task]); + + const scope = handler.mock.calls[0]?.[2] as { transactionID?: unknown } | undefined; + expect(scope?.transactionID).toBeUndefined(); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts index 72cbfad6..dc4d94dd 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts @@ -5,6 +5,8 @@ import { toTaskFilter } from "../toTaskFilter"; import type { TaskHandler } from "../TaskRunnerProvider.interface"; import type { Task, TaskInput, RunResult, ID } from "../types"; import type { LazyMap } from "../../../shared/utils"; +import type { TransactionScope } from "../../../shared/payload/TransactionScope.shapes"; +import { killedTheCallersTransaction } from "../../../shared/payload/TransactionScope.shapes"; /** * Synchronous TaskRunner implementation. @@ -19,7 +21,7 @@ export class SyncTaskRunner implements TaskRunner { private readonly tasks: LazyMap ) {} - async enqueue(inputs: TaskInput[]): Promise { + async enqueue(inputs: TaskInput[], scope: TransactionScope = {}): Promise { for (const input of inputs) { const key = this.getKey(input.collectionSlug, input.collectionId, input.targetLng); const now = new Date().toISOString(); @@ -36,14 +38,18 @@ export class SyncTaskRunner implements TaskRunner { this.tasks.set(key, task); try { - await this.handler(this.payload, { - collection: input.collectionSlug, - collectionId: input.collectionId, - sourceLng: input.sourceLng, - targetLng: input.targetLng, - strategy: input.strategy, - publishOnTranslation: input.publishOnTranslation, - }); + await this.handler( + this.payload, + { + collection: input.collectionSlug, + collectionId: input.collectionId, + sourceLng: input.sourceLng, + targetLng: input.targetLng, + strategy: input.strategy, + publishOnTranslation: input.publishOnTranslation, + }, + scope + ); task.status = "completed"; task.completedAt = new Date().toISOString(); @@ -52,9 +58,14 @@ export class SyncTaskRunner implements TaskRunner { task.error = { message: error instanceof Error ? error.message : "Unknown error", }; + // Abandon the remaining locales: with the caller's transaction already rolled back they + // would only pile up errors against a dead one. + if (killedTheCallersTransaction(scope, error)) throw error; + } finally { + // `finally`, not after the `try`: the rethrow above must still leave a timestamp, or + // `LazyMap` never evicts the failed task. + task.updatedAt = new Date().toISOString(); } - - task.updatedAt = new Date().toISOString(); } } diff --git a/packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.test.ts b/packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.test.ts new file mode 100644 index 00000000..1aebae88 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.test.ts @@ -0,0 +1,30 @@ +import { describe, it, expect } from "vitest"; +import { APIError } from "payload"; + +import { killedTheCallersTransaction } from "./TransactionScope.shapes"; + +describe("killedTheCallersTransaction", () => { + it("is true for a Payload error raised inside the caller's transaction", () => { + expect(killedTheCallersTransaction({ transactionID: "tx-1" }, new APIError("rejected"))).toBe( + true + ); + }); + + it("is false for a failure that reached no Payload operation", () => { + expect(killedTheCallersTransaction({ transactionID: "tx-1" }, new Error("provider down"))).toBe( + false + ); + }); + + it("is false when there is no caller transaction to destroy", () => { + expect(killedTheCallersTransaction({}, new APIError("rejected"))).toBe(false); + }); + + it("is false when neither holds", () => { + expect(killedTheCallersTransaction({}, new Error("provider down"))).toBe(false); + }); + + it("treats the MongoDB adapter's zero transaction id as a transaction", () => { + expect(killedTheCallersTransaction({ transactionID: 0 }, new APIError("rejected"))).toBe(true); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.ts b/packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.ts new file mode 100644 index 00000000..440666bf --- /dev/null +++ b/packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.ts @@ -0,0 +1,26 @@ +import { APIError } from "payload"; + +/** + * The caller's database transaction, reduced to the one field an operation needs to join it: Payload + * joins one only when the id is passed under `req` on that call, and an empty scope means each + * operation opens its own. + */ +export type TransactionScope = { transactionID?: string | number }; + +/** A fresh carrier per call: `createLocalReq` fills the `req` it is handed in place. */ +export function freshReq(scope: TransactionScope): TransactionScope { + return { ...scope }; +} + +/** + * Whether this failure has already rolled the caller's transaction back: Payload's `killTransaction` + * fires from the catch of every operation and rolls back whenever a transaction id is present, + * without checking whose it is. Rethrowing cannot save the caller's edit — it is already gone — it + * only stops the translator reporting a save that did not happen. + * + * Narrowed to `APIError` because only a Payload operation reaches `killTransaction`; a provider + * outage throws before any operation runs and leaves the save intact. + */ +export function killedTheCallersTransaction(scope: TransactionScope, error: unknown): boolean { + return scope.transactionID != null && error instanceof APIError; +} diff --git a/packages/payload-plugin-translator/src/server/shared/payload/sourceDocument.ts b/packages/payload-plugin-translator/src/server/shared/payload/sourceDocument.ts index 253738d1..6e63873f 100644 --- a/packages/payload-plugin-translator/src/server/shared/payload/sourceDocument.ts +++ b/packages/payload-plugin-translator/src/server/shared/payload/sourceDocument.ts @@ -1,5 +1,8 @@ import type { CollectionSlug, Payload } from "payload"; +import type { TransactionScope } from "./TransactionScope.shapes"; +import { freshReq } from "./TransactionScope.shapes"; + /** * The single source read: what "translate from X" resolves to. Both translation write paths and * the staleness recompute must go through here, or the fingerprints they compare drift apart. @@ -8,9 +11,11 @@ export function fetchSourceDocument( payload: Payload, collection: CollectionSlug, id: string, - locale: string + locale: string, + scope: TransactionScope = {} ) { return payload.findByID({ + req: freshReq(scope), collection, id, locale,