From 0270df420868f976f1d2ee871db64f11d435aa09 Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 15 Sep 2026 12:14:31 +0200 Subject: [PATCH] fix(translator): run auto-translate inside the save that triggered it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An auto-translation now joins the transaction of the save that triggered it, so the two commit or roll back together. Before this, auto-translate did nothing at all on PostgreSQL. The hook fires while the save's transaction is still open, and the translation's first act is to read the document back. That read opened its own transaction, where the row did not exist yet, so Payload answered "Not Found" and the translation stopped. SQLite and MongoDB do not isolate the uncommitted row the same way, which is why the suite stayed green. The transaction travels as `TransactionScope`, a slice carrying the one field an operation needs to join it, from the hook to every operation call on the path. Passing the whole request instead would have dragged a god type through every handler and risked `createLocalReq` mutating a request Payload shares across every document of a bulk update. Reads and writes both join it: the write targets the very document the caller has not committed, so from outside there is nothing to update. A failure inside that transaction is now surfaced instead of swallowed. Payload's `killTransaction` fires from the catch of every operation and rolls back whenever a transaction id is present, without checking whose it is — so a translation Payload rejects takes the editor's save with it. Rethrowing cannot save the edit, which is already gone; it stops the translator reporting a save that did not happen. Narrowed to `APIError`, because a provider outage throws before any operation runs and leaves the save intact. The provenance receipt joins the same transaction, or it would outlive the translation it certifies and report a document translated at a fingerprint that never committed. Two writes deliberately stay outside: the jobs runner, whose job runs after the commit and never had this defect, and the provenance cleanup on document delete, which must never roll back the delete that triggered it. `LifecycleNotifier` now logs a failed translation when the host configured no `onFailed` callback. Without it this defect left no trace anywhere, which is most of why it survived so long. Closes #124 --- .../transaction-failure-isolation.int.test.ts | 57 ++++ .../transaction-rollback.int.test.ts | 96 ++++++ ...transaction-validation-failure.int.test.ts | 62 ++++ ...late-in-the-triggering-transaction.task.md | 278 ++++++++++++++++++ .../translate-document/handler.test.ts | 4 +- .../handler.transaction.test.ts | 110 +++++++ .../features/translate-document/handler.ts | 29 +- .../translate-document/wireTranslateRunner.ts | 22 +- .../features/translate-field/handler.test.ts | 1 + .../AutoTranslateEnqueue.hook.ts | 13 +- .../AutoTranslateEnqueue.transaction.test.ts | 93 ++++++ .../lifecycle/LifecycleNotifier.test.ts | 31 ++ .../modules/lifecycle/LifecycleNotifier.ts | 14 +- .../lifecycle/withQueuedNotification.test.ts | 2 +- .../lifecycle/withQueuedNotification.ts | 4 +- .../provenance/Provenance.service.test.ts | 27 ++ .../modules/provenance/Provenance.service.ts | 17 +- .../modules/provenance/Provenance.store.ts | 39 ++- .../modules/provenance/Provenance.wiring.ts | 8 +- .../task-runner/TaskRunner.interface.ts | 6 +- .../TaskRunnerProvider.interface.ts | 7 +- .../sync-runner/SyncRunnerProvider.test.ts | 3 +- .../sync-runner/SyncTaskRunner.test.ts | 20 +- .../SyncTaskRunner.transaction.test.ts | 45 +++ .../task-runner/sync-runner/SyncTaskRunner.ts | 33 ++- .../payload/TransactionScope.shapes.test.ts | 30 ++ .../shared/payload/TransactionScope.shapes.ts | 26 ++ .../server/shared/payload/sourceDocument.ts | 7 +- 28 files changed, 1025 insertions(+), 59 deletions(-) create mode 100644 apps/dev/src/integration/translator/transaction-failure-isolation.int.test.ts create mode 100644 apps/dev/src/integration/translator/transaction-rollback.int.test.ts create mode 100644 apps/dev/src/integration/translator/transaction-validation-failure.int.test.ts create mode 100644 packages/payload-plugin-translator/docs/plans/2026-09-14-auto-translate-in-the-triggering-transaction.task.md create mode 100644 packages/payload-plugin-translator/src/server/features/translate-document/handler.transaction.test.ts create mode 100644 packages/payload-plugin-translator/src/server/modules/auto-translate/AutoTranslateEnqueue.transaction.test.ts create mode 100644 packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.transaction.test.ts create mode 100644 packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.test.ts create mode 100644 packages/payload-plugin-translator/src/server/shared/payload/TransactionScope.shapes.ts 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,