From 3ecc2c893dc4ea6907937ecd958f3154a231af06 Mon Sep 17 00:00:00 2001 From: Siarhei Date: Fri, 4 Sep 2026 18:54:21 +0200 Subject: [PATCH 1/7] fix(translator): translate a document's locales in one workflow, not parallel jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Translating one document into several locales lost translations. Measured on the real path — the enqueue endpoint, the jobs runner, and the same `jobs.run` call the autorun cron makes — nine trials of nine across SQLite, Postgres and MongoDB landed one translation of two. Which one varied. No error was raised and every job reported success. Every write Payload makes is a whole-document version snapshot, drafts included: publishing or saving a locale reads the current version as its base and merges that locale onto it. The plugin queued one job per target locale and Payload runs a batch through `Promise.all`, so two locales built their snapshots from the same base and the second silently dropped the first's work. The parallelism was ours, so the fix removes it rather than coordinating it. A document's locales are now one workflow whose handler awaits one task per locale. Ordering comes from `await` rather than from any lock, all the locales finish inside one cron tick, and nothing is asked of the host. Payload's own concurrency control was measured and rejected: a job blocked on a key is not held for it but deferred to the next cron tick — about a minute per locale at the default autorun — and enabling it adds an indexed column to the jobs collection. Recorded in #128. Per-locale detail survives. Payload writes a log entry per task inside a workflow, so the status endpoints rebuild their per-locale rows from it; a locale with no entry yet reports the job's own state. Jobs queued in the old per-locale shape still read correctly, the same expand-and-contract `readCollectionRef` already does for the collection reference. Supersession is now per document and narrowed to work that has not begun. A running workflow holds locales it has already translated, and cancelling it would throw them away — which is the loss this change exists to stop. The suite never caught any of this because every integration spec booted the sync runner, which translates inline and in order. These specs boot the jobs runner, the production default, which had no coverage until the harness gained a runner option. Closes #114 --- .../integration/translator/bootTestPayload.ts | 8 + .../translator/job-supersede.int.test.ts | 54 ++++--- .../locale-workflow-failure.int.test.ts | 83 +++++++++++ .../translator/locale-workflow.int.test.ts | 131 ++++++++++++++++ .../plans/2026-09-04-locale-workflow.task.md | 132 +++++++++++++++++ .../PayloadJobsRunnerProvider.ts | 33 ++++- .../PayloadJobsTaskRunner.test.ts | 140 +++++++++--------- .../PayloadJobsTaskRunner.ts | 112 +++++++------- .../payload-jobs-runner/normalizeJob.ts | 42 +++++- .../task-runner/payload-jobs-runner/types.ts | 15 ++ 10 files changed, 597 insertions(+), 153 deletions(-) create mode 100644 apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts create mode 100644 apps/dev/src/integration/translator/locale-workflow.int.test.ts create mode 100644 packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md diff --git a/apps/dev/src/integration/translator/bootTestPayload.ts b/apps/dev/src/integration/translator/bootTestPayload.ts index 041dcae69..dc54ca14b 100644 --- a/apps/dev/src/integration/translator/bootTestPayload.ts +++ b/apps/dev/src/integration/translator/bootTestPayload.ts @@ -60,6 +60,8 @@ export type TestPayload = { * publish; the enqueue route still works. * @param opts.collections - replaces the shared fixture set entirely (not merged). The set must * still contain a `docs` collection when `autoTranslate` is passed. + * @param opts.failFor - target locales the fake provider should throw for, so a spec can exercise a + * partial failure. Every other locale translates normally. * @param opts.runner - defaults to the sync runner. `createPayloadJobsRunner({ autoRun: false })` * leaves queued jobs unprocessed in `payload-jobs`, so a spec can read the rows. * @param opts.fallback - localization fallback, off by default: an unwritten locale reads as @@ -69,6 +71,7 @@ export async function bootTestPayload(opts?: { autoTranslate?: { targets: string[]; strategy?: "overwrite" | "skip_existing" }; collections?: CollectionConfig[]; fallback?: boolean; + failFor?: string[]; runner?: TaskRunnerProvider; }): Promise { const dir = mkdtempSync(join(tmpdir(), "translator-int-")); @@ -84,10 +87,12 @@ export async function bootTestPayload(opts?: { : collections; const baseProvider = createTranslationProvider({ complete: reverseComplete }); + const failFor = new Set(opts?.failFor); let translateCalls = 0; const countingProvider: TranslationProvider = { translate: (input, sourceLng, targetLng) => { translateCalls += 1; + if (failFor.has(targetLng)) throw new Error(`provider unavailable for ${targetLng}`); return baseProvider.translate(input, sourceLng, targetLng); }, }; @@ -110,6 +115,9 @@ export async function bootTestPayload(opts?: { ], }, collections, + // Matches the dev app: a host that wants translation history keeps completed jobs. Payload + // deletes them by default, which would leave the status panels with nothing to read. + jobs: { deleteJobOnComplete: false }, plugins: [ translatorPlugin({ collections: managed, diff --git a/apps/dev/src/integration/translator/job-supersede.int.test.ts b/apps/dev/src/integration/translator/job-supersede.int.test.ts index dd733310b..a83c653e6 100644 --- a/apps/dev/src/integration/translator/job-supersede.int.test.ts +++ b/apps/dev/src/integration/translator/job-supersede.int.test.ts @@ -5,6 +5,9 @@ import { bootTestPayload } from "./bootTestPayload"; import type { TestPayload } from "./bootTestPayload"; import { callEndpoint } from "./callEndpoint"; +// What a re-enqueue supersedes. Booted with the real job runner and autorun off, so jobs are written +// to `payload-jobs` and left there — the rows are the subject, not the translations. +// // Rows are counted, never identified by id: SQLite reuses the rowid of a deleted row (integer primary // key, no AUTOINCREMENT), so a replacement job can arrive carrying the deleted job's id and an // id-based assertion would pass for the wrong reason. @@ -12,16 +15,17 @@ import { callEndpoint } from "./callEndpoint"; type Job = { id: string | number; completedAt?: string | null; - input?: { target_lng?: string; collection_id?: string }; + processing?: boolean; + input?: { collection_id?: string; target_lngs?: string[] }; }; let ctx: TestPayload; -const enqueue = async (id: string, target = "de") => { +const enqueue = async (id: string, targets: string[] = ["de"]) => { const res = await callEndpoint(ctx.payload, "post", "/translate/enqueue", { body: { source_lng: "en", - target_lng: target, + target_lng: targets, collection_slug: "docs", collection_id: [id], strategy: "overwrite", @@ -29,15 +33,17 @@ const enqueue = async (id: string, target = "de") => { }, }); expect(res.status, "the enqueue endpoint rejected the request").toBe(200); - return (res.data as { data: { queued: number } }).data.queued; + const body = res.data as { data: { queued: number } }; + return body.data.queued; }; -// The cases share one boot, so the table also holds every earlier case's jobs. +// One workflow per document, so these are workflow rows, not task rows. The cases share one boot, so +// the table also holds every earlier case's jobs. const jobs = async (documentId: string): Promise => { const { docs } = await ctx.payload.find({ collection: "payload-jobs" as "pages", pagination: false, - where: { taskSlug: { equals: "translate_document" } } as never, + where: { workflowSlug: { equals: "translate_document_locales" } } as never, }); return (docs as Job[]).filter((j) => j.input?.collection_id === documentId); }; @@ -49,6 +55,13 @@ const markFinished = (jobId: string | number) => data: { completedAt: new Date().toISOString(), processing: false } as never, }); +const markRunning = (jobId: string | number) => + ctx.payload.update({ + collection: "payload-jobs" as "pages", + id: jobId, + data: { processing: true } as never, + }); + const createDoc = async () => { const doc = await ctx.payload.create({ collection: "docs" as "pages", @@ -65,10 +78,10 @@ afterAll(async () => { await ctx?.cleanup(); }); -describe("re-enqueue supersedes unfinished work, not finished work", () => { - it("keeps a finished job when the same locale is translated again", async () => { +describe("re-enqueue supersedes work that has not begun, and nothing else", () => { + it("keeps a finished job when the document is translated again", async () => { const id = await createDoc(); - expect(await enqueue(id), "fixture: the first enqueue queued a job").toBe(1); + expect(await enqueue(id), "fixture: the first enqueue queued a workflow").toBe(1); const [first] = await jobs(id); await markFinished(first.id); @@ -80,29 +93,26 @@ describe("re-enqueue supersedes unfinished work, not finished work", () => { expect(after.length, "finished job plus the new one").toBe(2); }); - it("still supersedes an unfinished job for the same locale", async () => { + it("supersedes a job that has not started", async () => { const id = await createDoc(); await enqueue(id); expect((await jobs(id)).length, "fixture").toBe(1); expect(await enqueue(id), "the re-enqueue queued nothing").toBe(1); - expect((await jobs(id)).length, "the unfinished job was not superseded").toBe(1); + expect((await jobs(id)).length, "the pending job was not superseded").toBe(1); }); - it("leaves another locale's unfinished job alone", async () => { + it("leaves a job that is already running alone", async () => { + // The reason supersession is narrowed to not-yet-started work: a running workflow holds locales + // it has already translated, and cancelling it would throw them away. const id = await createDoc(); - await enqueue(id, "de"); - await enqueue(id, "fr"); - expect((await jobs(id)).length, "fixture: one job per locale").toBe(2); + await enqueue(id); + const [running] = await jobs(id); + await markRunning(running.id); - expect(await enqueue(id, "de"), "the re-enqueue queued nothing").toBe(1); + expect(await enqueue(id), "the re-enqueue queued nothing").toBe(1); - const after = await jobs(id); - expect(after.length, "the fr job was collateral damage").toBe(2); - expect(after.map((j) => j.input?.target_lng).sort(), "one job per locale, still").toEqual([ - "de", - "fr", - ]); + expect((await jobs(id)).length, "the running job was cancelled").toBe(2); }); }); diff --git a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts new file mode 100644 index 000000000..6ffdee7f4 --- /dev/null +++ b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts @@ -0,0 +1,83 @@ +import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { callEndpoint } from "./callEndpoint"; + +// A locale whose provider fails mid-run. Its own file because the failing provider is a property of +// the boot, and `getPayload` caches per process — a second boot in one file returns the first. + +const rev = (s: string) => [...s].reverse().join(""); + +let failing: TestPayload; + +beforeAll(async () => { + failing = await bootTestPayload({ + runner: createPayloadJobsRunner({ autoRun: false }), + failFor: ["fr"], + }); +}); +afterAll(async () => { + await failing?.cleanup(); +}); + +describe("when one locale's provider fails", () => { + it("stops there, leaves the locales after it untouched, and resumes on retry", async () => { + const source = "Partial source"; + const doc = await failing.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: source, _status: "published" } as never, + }); + const id = String(doc.id); + + const res = await callEndpoint(failing.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: ["de", "fr"], + collection_slug: "docs", + collection_id: [id], + strategy: "overwrite", + publish_on_translation: false, + }, + }); + expect(res.status).toBe(200); + + await failing.payload.jobs.run({ queue: "translations", limit: 50 }); + + const read = async (locale: string) => + ( + (await failing.payload.findByID({ + collection: "docs" as "pages", + id, + locale: locale as "en", + fallbackLocale: false, + draft: true, + })) as Record + ).title; + + expect(await read("de"), "the locale before the failure should have landed").toBe(rev(source)); + expect(await read("fr"), "the failing locale should not have landed").toBeUndefined(); + + // Payload backs a failed job off exponentially, so the picker would skip it for the next few + // seconds. Clearing the delay lets the retry happen now rather than making the spec wait. + const { docs } = await failing.payload.find({ + collection: "payload-jobs" as "pages", + pagination: false, + where: { workflowSlug: { equals: "translate_document_locales" } } as never, + }); + await failing.payload.update({ + collection: "payload-jobs" as "pages", + id: (docs[0] as { id: string | number }).id, + data: { waitUntil: null, processing: false } as never, + }); + + // The retry must not translate `de` again — Payload's own task restoration skips a locale the + // log already records as succeeded. + const before = failing.translateCount(); + await failing.payload.jobs.run({ queue: "translations", limit: 50 }); + + expect(failing.translateCount() - before, "de was translated a second time").toBe(1); + }); +}); diff --git a/apps/dev/src/integration/translator/locale-workflow.int.test.ts b/apps/dev/src/integration/translator/locale-workflow.int.test.ts new file mode 100644 index 000000000..649ca91f2 --- /dev/null +++ b/apps/dev/src/integration/translator/locale-workflow.int.test.ts @@ -0,0 +1,131 @@ +import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { callEndpoint } from "./callEndpoint"; + +// Every requested locale must be translated. Booted with the REAL jobs runner — the production +// default — because the parallel fan-out this guards against exists only there: `createSyncRunner` +// translates inline and in order, which is why the rest of the suite never saw the defect. + +const rev = (s: string) => [...s].reverse().join(""); + +let ctx: TestPayload; + +beforeAll(async () => { + ctx = await bootTestPayload({ runner: createPayloadJobsRunner({ autoRun: false }) }); +}); +afterAll(async () => { + await ctx?.cleanup(); +}); + +const createDoc = async (title: string) => { + const doc = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title, _status: "published" } as never, + }); + return String(doc.id); +}; + +const enqueue = async (id: string, targets: string[]) => { + const res = await callEndpoint(ctx.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: targets, + collection_slug: "docs", + collection_id: [id], + strategy: "overwrite", + publish_on_translation: false, + }, + }); + expect(res.status, "the enqueue endpoint rejected the request").toBe(200); +}; + +// The same call the autorun cron makes, so the batching behaviour under test is the real one. +const runQueue = () => ctx.payload.jobs.run({ queue: "translations", limit: 50 }); + +const titleIn = async (id: string, locale: string) => + ( + (await ctx.payload.findByID({ + collection: "docs" as "pages", + id, + locale: locale as "en", + fallbackLocale: false, + draft: true, + })) as Record + ).title; + +const workflowJob = async (id: string) => { + const { docs } = await ctx.payload.find({ + collection: "payload-jobs" as "pages", + pagination: false, + where: { workflowSlug: { equals: "translate_document_locales" } } as never, + }); + return ( + docs as Array<{ input?: { collection_id?: string }; log?: Array> }> + ).find((j) => j.input?.collection_id === id); +}; + +describe("translating one document into several locales", () => { + it("translates every requested locale, not just one", async () => { + const source = "Multi source"; + const id = await createDoc(source); + + await enqueue(id, ["de", "fr"]); + await runQueue(); + + expect(await titleIn(id, "de"), "de was not translated").toBe(rev(source)); + expect(await titleIn(id, "fr"), "fr was not translated").toBe(rev(source)); + }); + + it("records one log entry per locale, in the order they were requested", async () => { + const id = await createDoc("Ordered source"); + + await enqueue(id, ["de", "fr"]); + await runQueue(); + + const job = await workflowJob(id); + expect(job, "no workflow job was written").toBeDefined(); + expect( + job?.log?.map((e) => (e.input as { target_lng?: string })?.target_lng), + "the log should carry a row per locale, in request order" + ).toEqual(["de", "fr"]); + expect(job?.log?.map((e) => e.state)).toEqual(["succeeded", "succeeded"]); + }); + + it("the status endpoint still reports a row per target locale", async () => { + const id = await createDoc("Status source"); + + await enqueue(id, ["de", "fr"]); + await runQueue(); + + const res = await callEndpoint( + ctx.payload, + "get", + "/translate/document/:collection_slug/:collection_id", + { + routeParams: { collection_slug: "docs", collection_id: id }, + } + ); + expect(res.status).toBe(200); + const rows = (res.data as { data?: Array<{ input?: { target_lng?: string } }> }).data ?? []; + expect( + rows.map((r) => r.input?.target_lng).sort(), + "the panel lost its per-locale detail" + ).toEqual(["de", "fr"]); + }); + + it("does not re-run a workflow that already completed", async () => { + const id = await createDoc("Idempotent source"); + + await enqueue(id, ["de", "fr"]); + await runQueue(); + + const before = ctx.translateCount(); + await runQueue(); + + expect(ctx.translateCount() - before, "a completed workflow was run again").toBe(0); + }); +}); diff --git a/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md b/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md new file mode 100644 index 000000000..f62d82134 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md @@ -0,0 +1,132 @@ +# Task contract — one workflow per document instead of one job per locale (#114) + +**Risk: HIGH.** Changes what the jobs runner queues, what supersession cancels, and where the admin +panel reads per-locale state. Four callers, and it affects data an editor sees. + +## The defect, measured + +Parallel per-locale jobs on one document lose **the translations themselves**, not only their +publication as #114's title says. Measured on the plugin's real path — the `/translate/enqueue` +endpoint, the Payload jobs runner, and `payload.jobs.run` (the call the autorun cron makes): + +| adapter | trial 1 | trial 2 | trial 3 | +|---|---|---|---| +| SQLite | 1 of 2 | 1 of 2 | 1 of 2 | +| Postgres | 1 of 2 | 1 of 2 | 1 of 2 | +| MongoDB | 1 of 2 | 1 of 2 | 1 of 2 | + +Nine of nine: one translation of two lands, which one varies, no error raised, both jobs report +success. + +**Cause.** Every write is a whole-document version snapshot, drafts included +(`payload/dist/collections/operations/utilities/update.js:188` reads the last published version as +its base and merges one locale onto it). Two jobs run that read-modify-write from the same base; +whoever writes second produces a snapshot that never contained the other's work. + +**The parallelism is ours.** `AutoTranslate.policy.ts:138` emits one task per target locale, and +`runJobs` batches them through `Promise.all` unless `sequential` is passed — which the autorun cron +does not pass and `AutorunCronConfig` cannot express. + +**Why the suite never caught it.** Every integration spec boots `createSyncRunner`, which runs +translations inline and in order. The jobs runner — the production default — had no coverage until +#126 added `bootTestPayload({ runner })`. + +## Design decisions + +**D1 — a Payload workflow with one task per locale.** +Verified by running it before choosing it: tasks execute strictly in sequence; the job's `log` array +records one entry per task with `taskSlug`, `input`, `output` and `state`; a failure stops the run and +leaves later locales unattempted; a retry resumes at the failed locale rather than redoing the +successful ones; and no migration is needed, because a job's `input` is a single JSON column and +`payload_jobs_log` already exists. + +Rejected, with measurements, in #128: Payload's `enableConcurrencyControl` defers a blocked job to the +**next cron tick** — roughly a minute per locale at the plugin's `* * * * *` autorun — and adds an +indexed column to the jobs collection. + +**D2 — the grouping happens inside `PayloadJobsTaskRunner`, not at its callers.** +`TaskRunner.enqueue(tasks: TaskInput[])` already receives one entry per locale, and the runner already +groups them by collection. Grouping by document and queueing one workflow is a change contained to +that class. Rejected: changing `TaskInput` to carry a locale array — it would touch both callers, the +sync runner, the lifecycle wrapper and the endpoint, for no gain, since the array already arrives. + +**D3 — supersession cancels only jobs that have not started.** +The user's decision. A workflow already running keeps its already-translated locales and finishes; +the new request queues behind it. Rejected: cancelling the whole workflow, which is closer to today's +semantics but discards locales that were already translated in the current run — the very work this +change exists to stop losing. + +**D4 — the panel reads per-locale state from the job log.** +The user's decision. Payload writes an entry per task, so the detail survives the collapse from N jobs +to one. Rejected: one row per document — less work now, but the editor loses sight of which locale is +done and which failed, which is the information the panel exists to show. + +**Placement:** the workflow registration goes beside the task registration in +`PayloadJobsRunnerProvider.configure()`; the grouping in `PayloadJobsTaskRunner.enqueue`; the log +reduction where `latestTaskPerTargetLocale` lives today. + +**New surface:** none outside the plugin. The `/translate/enqueue` contract is unchanged — it already +accepts `target_lng` as a string or an array. + +**Written contract owed:** yes — what the workflow guarantees about ordering and about partial +completion after a failure. Neither is expressible in a signature. + +**Escalate:** no. One module, no new dependency, no schema change. + +## Acceptance criteria + +1. **Every requested locale is translated.** Enqueue two locales for one document through the endpoint + with the jobs runner, run the queue, and both translations are present. *Check: integration test on + SQLite.* Currently fails — one of two lands. +2. **The same holds on Postgres and MongoDB.** *Check: `test:integration:postgres` and `:mongo`.* +3. **Locales run in order, not concurrently.** *Check: integration test asserting the job log's entries + are ordered and non-overlapping.* +4. **A failure stops the run and leaves later locales unattempted**, and the job records which locale + failed. *Check: integration test with a provider that throws for one locale.* +5. **A retry resumes rather than restarting** — a locale already translated is not sent to the provider + again. *Check: integration test using the boot's `translateCount()`.* +6. **Supersession cancels only jobs that have not started.** *Check: unit test on the runner.* +7. **The status endpoints still report per-locale state**, now from the job log. *Check: integration + test through the real endpoints.* +8. **The sync runner is unaffected.** *Check: the existing 73 integration tests stay green on SQLite.* +9. **Checks clean:** check-types both packages, unit tests, lint at the repo baseline. + +## Human choices + +- **Supersede only not-yet-started jobs** (D3). Rejected alternative recorded above. +- **Panel reads the job log** (D4). Rejected alternative recorded above. +- **Fix this under #114** rather than opening a separate issue, with #114's description extended by a + comment to cover the wider defect. + +## Risks + +- The panel's data source changes; a mistake there is visible to editors even though no translation is + lost. +- Old jobs queued before the upgrade carry the per-locale shape. `readCollectionRef` is the precedent + for reading two stored shapes and should be followed rather than reinvented. +- Workflows are a Payload concept the plugin has not used before. The behaviour above was verified on + 3.84.1; the plugin's peer floor is `^3.76.0` and that gap is unverified. + +## Corrected while building + +**`bootTestPayload` cannot boot twice in one file.** `getPayload` caches per process, so a second +`bootTestPayload` in the same spec returns the first — the failing-provider case silently ran against +the healthy boot and reported the wrong thing. Split into its own file, which is what the harness's +own docblock already warns about. + +**The per-locale rows must keep the real job id.** The first version of `normalizeJobLocales` +synthesised `${jobId}:${locale}` for uniqueness. That id is handed straight to `cancel()`, which +addresses jobs — so supersession stopped cancelling anything. The rows share the job id, because they +are rows of one job. + +**Completed jobs are deleted by default.** Payload's `deleteJobOnComplete` defaults to `true`, and +`bootTestPayload` inherited that — so the panel had nothing to read after a run. The harness now sets +it to `false`, matching the dev app and the README's own recommendation. Worth stating plainly: the +panel reading per-locale state from the job log only works for hosts that keep completed jobs. + +**A retry is delayed by backoff.** Payload backs a failed job off exponentially, so a retry cannot be +observed by simply running the queue again; the spec clears `waitUntil` rather than waiting. + +## Review log + +_(appended by review runs)_ diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts index c5ca8a34a..31f2db81c 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts @@ -16,6 +16,7 @@ const DEFAULT_STALE_JOB_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes const defaultValues = { taskName: "translate_document", + workflowName: "translate_document_locales", queueName: "translations", jobsCollection: "payload-jobs", autoRun: defaultAutoRun, @@ -54,6 +55,7 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { this.config = { taskName: options?.taskName ?? defaultValues.taskName, + workflowName: options?.workflowName ?? defaultValues.workflowName, queueName: options?.queueName ?? defaultValues.queueName, jobsCollection: options?.jobsCollection ?? defaultValues.jobsCollection, autoRun, @@ -67,7 +69,7 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { } configure(context: TaskRunnerContext): (config: Config) => Config { - const { taskName, queueName, retries, autoRun } = this.config; + const { taskName, workflowName, queueName, retries, autoRun } = this.config; const { handler, collections } = context; return (config) => { @@ -128,6 +130,12 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { }, ]; + // Same fields as the task, except the single target locale becomes the list the workflow walks. + const workflowInputSchema: Field[] = [ + ...inputSchema.filter((f) => "name" in f && f.name !== "target_lng"), + { type: "json", name: "target_lngs", required: true }, + ]; + const task = { slug: taskName, inputSchema, @@ -158,9 +166,32 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { }, }; + // One workflow per document, awaiting one task per locale. The tasks run in sequence, which is + // the whole point: every write Payload makes is a whole-document version snapshot, so two + // locales translated in parallel build their snapshots from the same base and the second one + // silently drops the first's work. See issue #114. + const workflow = { + slug: workflowName, + inputSchema: workflowInputSchema, + retries, + handler: async (args: { + job: { input: { target_lngs?: string[] } & Record }; + tasks: Record Promise>; + }) => { + const { target_lngs: targets = [], ...shared } = args.job.input; + for (const target of targets) { + // The locale is the task id, so Payload's own restoration skips a locale already logged + // as succeeded when a failed workflow is retried. + await args.tasks[taskName](target, { input: { ...shared, target_lng: target } }); + } + }, + }; + if (!config.jobs) config.jobs = {}; if (!config.jobs.tasks) config.jobs.tasks = []; config.jobs.tasks.push(task); + if (!config.jobs.workflows) config.jobs.workflows = []; + config.jobs.workflows.push(workflow as never); // Skip autoRun configuration when disabled (e.g., for Vercel/serverless deployments) if (autoRun) { diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts index 1a2ee342b..513a1a4b3 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts @@ -35,6 +35,7 @@ describe("PayloadJobsTaskRunner", () => { }; config = { taskName: "translate_document", + workflowName: "translate_document_locales", queueName: "translations", jobsCollection: "payload-jobs", autoRun: { @@ -75,22 +76,31 @@ describe("PayloadJobsTaskRunner", () => { const whereArg = mockPayload.find.mock.calls[0][0].where; expect(whereArg).toEqual({ - and: [{ taskSlug: { equals: "translate_document" } }, { completedAt: { exists: false } }], + and: [ + { + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], + }, + { completedAt: { exists: false } }, + ], }); }); - it("queues tasks with correct input", async () => { - const input = createInput(); - await runner.enqueue([input]); + it("queues one workflow per document, carrying its locales", async () => { + await runner.enqueue([createInput({ targetLng: "de" }), createInput({ targetLng: "fr" })]); + expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(1); expect(mockPayload.jobs.queue).toHaveBeenCalledWith({ - task: "translate_document", + workflow: "translate_document_locales", queue: "translations", + waitUntil: undefined, input: { collection_slug: "posts", collection_id: "doc-123", source_lng: "en", - target_lng: "de", + target_lngs: ["de", "fr"], strategy: "overwrite", publish_on_translation: false, }, @@ -113,12 +123,12 @@ describe("PayloadJobsTaskRunner", () => { expect(arg.waitUntil).toBeUndefined(); }); - it("queues multiple tasks", async () => { - const inputs = [ - createInput({ collectionId: "doc-1" }), - createInput({ collectionId: "doc-2" }), - ]; - await runner.enqueue(inputs); + it("queues one workflow per document, not one per task", async () => { + await runner.enqueue([ + createInput({ collectionId: "doc-1", targetLng: "de" }), + createInput({ collectionId: "doc-1", targetLng: "fr" }), + createInput({ collectionId: "doc-2", targetLng: "de" }), + ]); expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(2); }); @@ -162,69 +172,36 @@ describe("PayloadJobsTaskRunner", () => { expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(1); }); - it("supersedes only the same-locale job when several locales have jobs", async () => { - const deJob = createJob({ - id: "de-job", - input: { - collection: { relationTo: "posts" as CollectionSlug, value: "doc-123" }, - source_lng: "en", - target_lng: "de", - strategy: "overwrite", - }, + it("supersedes a job that has not started, and leaves a running one alone", async () => { + // Narrowed deliberately: a running workflow holds the locales it has already translated, and + // cancelling it would discard them — the loss this whole change exists to stop. See issue #114. + const pending = createJob({ + id: "pending-job", + input: { collection_slug: "posts", collection_id: "doc-123", source_lng: "en" }, }); - const frJob = createJob({ - id: "fr-job", - input: { - collection: { relationTo: "posts" as CollectionSlug, value: "doc-123" }, - source_lng: "en", - target_lng: "fr", - strategy: "overwrite", - }, + const running = createJob({ + id: "running-job", + processing: true, + input: { collection_slug: "posts", collection_id: "doc-123", source_lng: "en" }, }); - mockPayload.find.mockResolvedValueOnce({ docs: [deJob, frJob] }); + mockPayload.find.mockResolvedValueOnce({ docs: [pending, running] }); - await runner.enqueue([createInput({ targetLng: "fr" })]); + await runner.enqueue([createInput()]); expect(mockPayload.jobs.cancel).toHaveBeenCalledWith({ - where: { id: { in: ["fr-job"] } }, + where: { id: { in: ["pending-job"] } }, queue: "translations", }); - expect(mockPayload.delete).toHaveBeenCalledWith({ - collection: "payload-jobs", - where: { id: { in: ["fr-job"] } }, - }); - }); - - it("does not cancel when no existing jobs", async () => { - mockPayload.find.mockResolvedValue({ docs: [] }); - - const input = createInput(); - await runner.enqueue([input]); - - expect(mockPayload.jobs.cancel).not.toHaveBeenCalled(); - expect(mockPayload.delete).not.toHaveBeenCalled(); }); - it("groups tasks by collection", async () => { - const inputs = [ - createInput({ - collectionSlug: "posts" as CollectionSlug, - collectionId: "post-1", - }), - createInput({ - collectionSlug: "posts" as CollectionSlug, - collectionId: "post-2", - }), - createInput({ - collectionSlug: "pages" as CollectionSlug, - collectionId: "page-1", - }), - ]; - - await runner.enqueue(inputs); + it("looks for existing jobs once per document", async () => { + await runner.enqueue([ + createInput({ collectionSlug: "posts" as CollectionSlug, collectionId: "post-1" }), + createInput({ collectionSlug: "posts" as CollectionSlug, collectionId: "post-2" }), + createInput({ collectionSlug: "pages" as CollectionSlug, collectionId: "page-1" }), + ]); - // Should check for existing jobs per collection - expect(mockPayload.find).toHaveBeenCalledTimes(2); + expect(mockPayload.find).toHaveBeenCalledTimes(3); }); it("stores the reference as flat text, coercing the id to a string", async () => { @@ -387,11 +364,19 @@ describe("PayloadJobsTaskRunner", () => { }); // a pending job (processing:false) needs no lock reset expect(mockPayload.update).not.toHaveBeenCalled(); - // findJobsInternal must narrow by taskSlug AND the given id + // findJobsInternal must narrow by the job's own slugs AND the given id expect(mockPayload.find).toHaveBeenCalledWith( expect.objectContaining({ where: { - and: [{ taskSlug: { equals: "translate_document" } }, { id: { equals: "job-123" } }], + and: [ + { + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], + }, + { id: { equals: "job-123" } }, + ], }, }) ); @@ -524,7 +509,7 @@ describe("PayloadJobsTaskRunner", () => { }); }); - it("narrows the SQL where clause by taskSlug only", async () => { + it("narrows the SQL where clause by the job's own slugs only", async () => { // Slug and id are matched in memory (see PayloadJobsTaskRunner.findByCollection // for the full reasoning), spanning both the new flat-text shape and the // legacy relationship shape. The WHERE sent to Payload must narrow only by @@ -534,7 +519,14 @@ describe("PayloadJobsTaskRunner", () => { const whereArg = mockPayload.find.mock.calls[0][0].where; expect(whereArg).toEqual({ - and: [{ taskSlug: { equals: "translate_document" } }], + and: [ + { + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], + }, + ], }); expect(JSON.stringify(whereArg)).not.toContain("collection_id"); expect(JSON.stringify(whereArg)).not.toContain("collection.value"); @@ -545,7 +537,15 @@ describe("PayloadJobsTaskRunner", () => { const whereArg = mockPayload.find.mock.calls[0][0].where; expect(whereArg).toEqual({ - and: [{ taskSlug: { equals: "translate_document" } }, { completedAt: { exists: false } }], + and: [ + { + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], + }, + { completedAt: { exists: false } }, + ], }); expect(JSON.stringify(whereArg)).not.toContain("collection_id"); }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts index 1d4d5e5dc..30ef6cd17 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts @@ -2,21 +2,11 @@ import type { Payload, Where, CollectionSlug } from "payload"; import type { TaskFilter, TaskRunner } from "../TaskRunner.interface"; import { toTaskFilter } from "../toTaskFilter"; -import type { Task, TaskInput, RunResult, ID } from "../types"; +import type { Task, TaskInput, RunResult } from "../types"; import type { PayloadJobsRunnerConfig, PayloadJob } from "./types"; -import { normalizeJob } from "./normalizeJob"; - -// A translation job's supersession identity: same document AND same target locale. IDs are -// String()-normalized to match the stored (string) form, so a number id compares equal to its -// persisted job. -const documentLocaleKey = (collectionId: ID, targetLng: string): string => - `${String(collectionId)}:${targetLng}`; - -/** - * TaskRunner implementation using Payload Jobs. - * - * Handles queuing, cancellation, status tracking, and execution of translation tasks. - */ +import { normalizeJobLocales } from "./normalizeJob"; + +/** {@link TaskRunner} backed by Payload's job queue (`payload-jobs`). */ export class PayloadJobsTaskRunner implements TaskRunner { constructor( private readonly payload: Payload, @@ -24,56 +14,50 @@ export class PayloadJobsTaskRunner implements TaskRunner { ) {} async enqueue(tasks: TaskInput[]): Promise { - const byCollection = this.groupByCollection(tasks); - - for (const [collectionSlug, items] of byCollection) { - const documentIds = items.map((t) => t.collectionId); - // Finished jobs must stay out of this set: superseding deletes (`cancelAndDeleteJobs` reaches - // `payload.delete`), so a completed job for the same (document, locale) would be erased along - // with the pending one. - const existing = await this.findByCollection(collectionSlug, { - documentIds, + // One workflow per document, carrying its locales, rather than one job per locale. Payload runs a + // batch of jobs through `Promise.all`, and every write it makes is a whole-document version + // snapshot — so two locales translated in parallel build their snapshots from the same base and + // the second silently drops the first's work. Measured at one translation of two landing, on all + // three adapters. See issue #114. + const byDocument = new Map(); + for (const task of tasks) { + const key = `${task.collectionSlug}:${task.collectionId}`; + byDocument.set(key, [...(byDocument.get(key) ?? []), task]); + } + + for (const group of byDocument.values()) { + const [first] = group; + const existing = await this.findByCollection(first.collectionSlug, { + documentIds: [first.collectionId], excludeCompleted: true, }); - // Supersede only jobs for the SAME (document, target locale) being re-enqueued — never a - // concurrent job for a *different* locale of the same document. Cancelling per-document would - // kill an in-flight translation of another locale (the concurrent re-translate bug). - const supersededKeys = new Set( - items.map((t) => documentLocaleKey(t.collectionId, t.targetLng)) - ); - const toCancel = existing.filter((t) => - supersededKeys.has(documentLocaleKey(t.input.collectionId, t.input.targetLng)) + // Only work that has not begun. A workflow already running keeps the locales it has finished + // and completes; the new request queues behind it. Cancelling it would discard translations + // that already landed — the very loss this change exists to stop. + const toCancel = existing.filter( + (t) => t.input.collectionId === String(first.collectionId) && t.status === "pending" ); if (toCancel.length > 0) { await this.cancelAndDeleteJobs(toCancel.map((t) => t.id)); } - } - await Promise.all( - tasks.map((task) => - this.payload.jobs.queue({ - task: this.config.taskName, - queue: this.config.queueName, - // Debounce: when set, Payload holds the job until this instant. A superseding enqueue for - // the same (document, targetLng) cancels the pending delayed job first (see enqueue above), - // so rapid source edits coalesce to the final one. Undefined for the manual path. - waitUntil: task.waitUntil, - input: { - // Flat text reference (ID-agnostic). Stored as a string — no - // relationship type validation against the collection's ID type, - // which is what previously left number-id jobs stuck in processing. - // This is the single write boundary, so `String(...)` here is the - // one place IDs are normalized for storage. - collection_slug: task.collectionSlug, - collection_id: String(task.collectionId), - source_lng: task.sourceLng, - target_lng: task.targetLng, - strategy: task.strategy, - publish_on_translation: task.publishOnTranslation, - }, - }) - ) - ); + await this.payload.jobs.queue({ + workflow: this.config.workflowName as never, + queue: this.config.queueName, + // Debounce: Payload holds the job until this instant, so rapid source edits coalesce. + waitUntil: first.waitUntil, + input: { + collection_slug: first.collectionSlug, + // The one place an id is normalized for storage; the stored shape is text so a job stays + // ID-agnostic. See docs/DEPRECATIONS.md#jobs-input-collection-field + collection_id: String(first.collectionId), + source_lng: first.sourceLng, + target_lngs: group.map((t) => t.targetLng), + strategy: first.strategy, + publish_on_translation: first.publishOnTranslation, + } as never, + }); + } } async cancel(taskIds: string[]): Promise { @@ -202,7 +186,17 @@ export class PayloadJobsTaskRunner implements TaskRunner { where?: Where, params?: { limit?: number; pagination?: boolean } ): Promise { - const and: Where[] = [{ taskSlug: { equals: this.config.taskName } }]; + // Both shapes: a document's work is a workflow now, but jobs queued before that change — and + // still sitting in the table — carry the per-locale task slug. Same expand/contract as + // `readCollectionRef` does for the collection reference. + const and: Where[] = [ + { + or: [ + { workflowSlug: { equals: this.config.workflowName } }, + { taskSlug: { equals: this.config.taskName } }, + ], + }, + ]; if (where) and.push(where); const response = await this.payload.find({ @@ -212,6 +206,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { where: { and }, }); - return (response.docs as PayloadJob[]).map(normalizeJob); + return (response.docs as PayloadJob[]).flatMap(normalizeJobLocales); } } diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts index 40e2dcc2f..92fda6995 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts @@ -1,5 +1,5 @@ import type { Task, TaskStatus } from "../types"; -import type { PayloadJob } from "./types"; +import type { JobLogEntry, PayloadJob } from "./types"; import { readCollectionRef } from "./readCollectionRef"; /** @@ -55,3 +55,43 @@ function isCancelled(error: unknown): boolean { error.cancelled ); } + +/** + * Expand a stored job into one {@link Task} per target locale. + * + * A document's locales are one workflow job now, but the status panels are per-locale — so the + * per-locale rows are rebuilt from the job's `log`, which Payload writes an entry into as each + * locale's task finishes. A locale with no entry yet has not run: it reports the job's own state. + * + * A job in the pre-workflow shape carries a single `target_lng` and expands to itself, so jobs + * queued before the change stay readable. See docs/DEPRECATIONS.md#jobs-input-collection-field for + * the same expand/contract elsewhere in this file's neighbourhood. + */ +export function normalizeJobLocales(job: PayloadJob): Task[] { + const targets = job.input?.target_lngs; + if (!Array.isArray(targets) || targets.length === 0) return [normalizeJob(job)]; + + const base = normalizeJob(job); + const byLocale = new Map(); + for (const entry of job.log ?? []) { + const lng = entry?.input?.target_lng; + if (typeof lng === "string") byLocale.set(lng, entry); + } + + return targets.map((targetLng) => { + const entry = byLocale.get(targetLng); + return { + // The real job id, repeated across the locales: they are rows of one job, and cancelling any of + // them cancels that job. A synthetic per-locale id would be handed straight to `cancel()`, + // which addresses jobs. + ...base, + status: entry ? logStateToStatus(entry.state) : base.status, + completedAt: entry?.completedAt ?? undefined, + input: { ...base.input, targetLng }, + }; + }); +} + +function logStateToStatus(state: JobLogEntry["state"]): TaskStatus { + return state === "succeeded" ? "completed" : "failed"; +} diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts index 08cfeb8dc..d43a2a0cc 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts @@ -25,6 +25,11 @@ export type PayloadJobsRunnerOptions = { * @default 'translate_document' */ taskName?: string; + /** + * Name of the Payload workflow that walks a document's target locales. + * @default 'translate_document_locales' + */ + workflowName?: string; /** * Name of the job queue. * @default 'translations' @@ -72,6 +77,7 @@ export type PayloadJobsRunnerOptions = { */ export type PayloadJobsRunnerConfig = { taskName: string; + workflowName: string; queueName: string; jobsCollection: CollectionSlug; autoRun: false | Required; @@ -83,6 +89,7 @@ export type PayloadJobsRunnerConfig = { * Raw Payload job structure */ export type PayloadJob = { + log?: JobLogEntry[]; id: string; completedAt?: string | null; createdAt: string; @@ -104,7 +111,15 @@ export type PayloadJob = { }; source_lng?: string; target_lng?: string; + target_lngs?: string[]; strategy?: string; publish_on_translation?: boolean; }; }; + +/** One entry Payload writes to a job's `log` as each task inside a workflow settles. */ +export type JobLogEntry = { + state: "succeeded" | "failed"; + completedAt?: string | null; + input?: { target_lng?: string }; +}; From 69274e0191e5d05aa967a520016df546a953c25c Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 8 Sep 2026 12:00:02 +0200 Subject: [PATCH 2/7] fix(translator): extend a document's live job instead of replacing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second request for a document used to cancel its live job and queue a replacement carrying only the locales of that request, so every locale the old job still owed disappeared without a trace. The panel's per-row "re-translate" button sends exactly one locale, which made this the ordinary path, not an edge case. A request now adds its locales to the live job's stored list. The write touches only the input column: the full document operation re-reads and rewrites the whole row, which was measured to revert log entries written in between. After writing it verifies, retries once if a competing append replaced the list, and gives anything still undelivered a job of its own — which races nothing, because that only happens once the old job is done. A job carries one source locale, one strategy and one publish flag for all its locales, so it only takes work from a request that chose the same three. Also fixed, found by review: the panel crashed rendering a failed locale whose job carried no final error yet; /translate/cancel deleted any row in payload-jobs, not only ours; cancel-by-collection skipped jobs waiting to retry; and manual run reported success without running anything, while being unable to retry a failed job at all. When the host enables Payload's enableConcurrencyControl, the workflow declares a per-document concurrency key so the queue holds a second job until the running one finishes. The plugin never sets that flag — it adds an indexed column and needs a migration on SQL, which is the host's call. Closes #114 --- apps/dev/docs/multi-db-verification.md | 7 + apps/dev/package.json | 8 +- .../integration/translator/bootTestPayload.ts | 24 +- .../translator/exclusive-queue.int.test.ts | 140 ++++++++ ...ede.int.test.ts => job-extend.int.test.ts} | 88 ++--- .../translator/locale-append.int.test.ts | 128 ++++++++ .../locale-workflow-failure.int.test.ts | 48 ++- .../translator/locale-workflow.int.test.ts | 39 ++- packages/payload-plugin-translator/README.md | 37 +++ .../plans/2026-09-04-locale-workflow.task.md | 72 ++++- ...26-09-08-one-live-job-per-document.task.md | 187 +++++++++++ .../translation/model/statusRows.test.ts | 15 + .../entities/translation/model/statusRows.ts | 2 +- .../entities/translation/model/types.ts | 7 +- .../cancel-by-collection/handler.test.ts | 38 ++- .../features/cancel-by-collection/handler.ts | 14 +- .../features/enqueue-translation/handler.ts | 2 - .../PayloadJobsRunnerProvider.test.ts | 29 ++ .../PayloadJobsRunnerProvider.ts | 31 +- .../PayloadJobsTaskRunner.test.ts | 300 ++++++++++++++---- .../PayloadJobsTaskRunner.ts | 281 +++++++++++----- .../payload-jobs-runner/normalizeJob.test.ts | 92 +++++- .../payload-jobs-runner/normalizeJob.ts | 95 +++--- .../payload-jobs-runner/planEnqueue.test.ts | 145 +++++++++ .../payload-jobs-runner/planEnqueue.ts | 71 +++++ .../task-runner/payload-jobs-runner/types.ts | 8 +- 26 files changed, 1616 insertions(+), 292 deletions(-) create mode 100644 apps/dev/src/integration/translator/exclusive-queue.int.test.ts rename apps/dev/src/integration/translator/{job-supersede.int.test.ts => job-extend.int.test.ts} (53%) create mode 100644 apps/dev/src/integration/translator/locale-append.int.test.ts create mode 100644 packages/payload-plugin-translator/docs/plans/2026-09-08-one-live-job-per-document.task.md create mode 100644 packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts create mode 100644 packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts diff --git a/apps/dev/docs/multi-db-verification.md b/apps/dev/docs/multi-db-verification.md index 44116c91c..8c0182dee 100644 --- a/apps/dev/docs/multi-db-verification.md +++ b/apps/dev/docs/multi-db-verification.md @@ -54,6 +54,13 @@ is knowingly red (see below) and `&&` would stop the chain before Mongo ever ran `bootTestPayload` picks its adapter from `DB_ADAPTER` through `resolveTestDbAdapter()`, the test-side sibling of `resolveDbAdapter()` in `src/lib/database/resolveAdapter.ts`. +**Two queue modes, not one.** `EXCLUSIVE_QUEUE=1` turns on Payload's `enableConcurrencyControl` for +every boot, which is the setting a host enables to have the queue itself hold a second job for a +document. `test:integration:all` runs the three adapters in both modes — six runs — because the +plugin behaves differently under it: without the flag a second request extends the live job, with it +the request gets a job of its own and waits. Two specs pin their own mode rather than following the +env var (`locale-append` needs it off, `exclusive-queue` needs it on); everything else runs in both. + **Isolation.** Each boot needs a database of its own: the twelve spec files run serially but share one server, so without it they would read each other's rows. SQLite gets this for free — a throwaway file per boot. Postgres and MongoDB do not, so each boot is given a namespace named by a random run diff --git a/apps/dev/package.json b/apps/dev/package.json index a49e65655..fb459c715 100644 --- a/apps/dev/package.json +++ b/apps/dev/package.json @@ -28,8 +28,12 @@ "test:integration:sqlite": "cross-env DB_ADAPTER=sqlite bun run test:integration", "test:integration:postgres": "cross-env DB_ADAPTER=postgres bun run test:integration", "test:integration:mongo": "cross-env DB_ADAPTER=mongo bun run test:integration", - "test:integration:all": "bun run test:integration:sqlite; bun run test:integration:postgres; bun run test:integration:mongo", - "check-types": "tsgo --noEmit -p tsconfig.check.json" + "test:integration:all": "bun run test:integration:sqlite; bun run test:integration:postgres; bun run test:integration:mongo; bun run test:integration:exclusive:sqlite; bun run test:integration:exclusive:postgres; bun run test:integration:exclusive:mongo", + "check-types": "tsgo --noEmit -p tsconfig.check.json", + "test:integration:exclusive": "cross-env EXCLUSIVE_QUEUE=1 bun run test:integration", + "test:integration:exclusive:sqlite": "cross-env DB_ADAPTER=sqlite bun run test:integration:exclusive", + "test:integration:exclusive:postgres": "cross-env DB_ADAPTER=postgres bun run test:integration:exclusive", + "test:integration:exclusive:mongo": "cross-env DB_ADAPTER=mongo bun run test:integration:exclusive" }, "dependencies": { "@focus-reactive/payload-plugin-ab": "workspace:*", diff --git a/apps/dev/src/integration/translator/bootTestPayload.ts b/apps/dev/src/integration/translator/bootTestPayload.ts index dc54ca14b..66a9346c7 100644 --- a/apps/dev/src/integration/translator/bootTestPayload.ts +++ b/apps/dev/src/integration/translator/bootTestPayload.ts @@ -25,6 +25,9 @@ import { buildTestCollections } from "./testCollections"; /** * A booted test Payload plus the throwaway resources to tear down after the suite. */ +/** Payload's `autoRun.limit` default — these specs reproduce the cron's batching, not a run of one. */ +export const CRON_BATCH_LIMIT = 50; + export type TestPayload = { payload: Payload; cleanup: () => Promise; @@ -64,6 +67,9 @@ export type TestPayload = { * partial failure. Every other locale translates normally. * @param opts.runner - defaults to the sync runner. `createPayloadJobsRunner({ autoRun: false })` * leaves queued jobs unprocessed in `payload-jobs`, so a spec can read the rows. + * @param opts.onTranslate - awaited before each provider call, so a spec can hold a locale mid-run. + * @param opts.exclusiveQueue - Payload's `enableConcurrencyControl` for this boot; `EXCLUSIVE_QUEUE=1` + * sets it for every boot, which is how the suite is run in that mode without touching a spec. * @param opts.fallback - localization fallback, off by default: an unwritten locale reads as * empty, not as the default locale's text. Localization-level, so it applies to the whole boot. */ @@ -71,7 +77,9 @@ export async function bootTestPayload(opts?: { autoTranslate?: { targets: string[]; strategy?: "overwrite" | "skip_existing" }; collections?: CollectionConfig[]; fallback?: boolean; + exclusiveQueue?: boolean; failFor?: string[]; + onTranslate?: (targetLng: string) => Promise | void; runner?: TaskRunnerProvider; }): Promise { const dir = mkdtempSync(join(tmpdir(), "translator-int-")); @@ -90,10 +98,11 @@ export async function bootTestPayload(opts?: { const failFor = new Set(opts?.failFor); let translateCalls = 0; const countingProvider: TranslationProvider = { - translate: (input, sourceLng, targetLng) => { + translate: async (input, sourceLng, targetLng) => { translateCalls += 1; + await opts?.onTranslate?.(targetLng); if (failFor.has(targetLng)) throw new Error(`provider unavailable for ${targetLng}`); - return baseProvider.translate(input, sourceLng, targetLng); + return await baseProvider.translate(input, sourceLng, targetLng); }, }; @@ -112,12 +121,17 @@ export async function bootTestPayload(opts?: { { code: "en", label: "English" }, { code: "de", label: "Deutsch" }, { code: "fr", label: "Français" }, + // A third target so a spec can tell "the run stopped at the failure" from "the run carried + // on and one locale threw" — with two locales the failing one is always the last. + { code: "es", label: "Español" }, ], }, collections, - // Matches the dev app: a host that wants translation history keeps completed jobs. Payload - // deletes them by default, which would leave the status panels with nothing to read. - jobs: { deleteJobOnComplete: false }, + // Payload deletes completed jobs by default, which would leave the status panels nothing to read. + jobs: { + deleteJobOnComplete: false, + enableConcurrencyControl: opts?.exclusiveQueue ?? process.env.EXCLUSIVE_QUEUE === "1", + }, plugins: [ translatorPlugin({ collections: managed, diff --git a/apps/dev/src/integration/translator/exclusive-queue.int.test.ts b/apps/dev/src/integration/translator/exclusive-queue.int.test.ts new file mode 100644 index 000000000..f3744039d --- /dev/null +++ b/apps/dev/src/integration/translator/exclusive-queue.int.test.ts @@ -0,0 +1,140 @@ +import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { callEndpoint } from "./callEndpoint"; + +// Booted with `enableConcurrencyControl`. Its own file because the setting is fixed at boot and +// `getPayload` caches per process. + +type RunResult = { jobStatus?: Record }; + +type Job = { + processing?: boolean; + input?: { collection_id?: string; target_lngs?: string[] }; +}; + +let ctx: TestPayload; +let release: (() => void) | undefined; +let reached: (() => void) | undefined; +let held: Promise; + +const armBarrier = () => { + held = new Promise((resolve) => { + reached = resolve; + }); +}; + +beforeAll(async () => { + armBarrier(); + ctx = await bootTestPayload({ + exclusiveQueue: true, + runner: createPayloadJobsRunner({ autoRun: false }), + onTranslate: async (targetLng) => { + if (targetLng !== "de") return; + reached?.(); + await new Promise((resolve) => { + release = resolve; + }); + }, + }); +}); +afterAll(async () => { + release?.(); + await ctx?.cleanup(); +}); + +const createDoc = async (title: string) => { + const doc = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title, _status: "published" } as never, + }); + return String(doc.id); +}; + +const enqueue = (id: string, targets: string[]) => + callEndpoint(ctx.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: targets, + collection_slug: "docs", + collection_id: [id], + strategy: "overwrite", + publish_on_translation: false, + }, + }); + +const jobsFor = async (documentId: string) => { + const { docs } = await ctx.payload.find({ + collection: "payload-jobs" as "pages", + pagination: false, + where: { workflowSlug: { equals: "translate_document_locales" } } as never, + }); + return (docs as Job[]).filter((job) => job.input?.collection_id === documentId); +}; + +const runQueue = () => + ctx.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }) as Promise; + +const titleIn = async (id: string, locale: string) => + ( + (await ctx.payload.findByID({ + collection: "docs" as "pages", + id, + locale: locale as "en", + fallbackLocale: false, + draft: true, + })) as Record + ).title; + +describe("with the host's concurrency control on", () => { + it("holds a second job for the same document, then runs it and loses nothing", async () => { + armBarrier(); + const id = await createDoc("Exclusive source"); + + await enqueue(id, ["de"]); + const first = runQueue(); + await held; + + await enqueue(id, ["fr"]); + + // Without this the check below is satisfied by "there was nothing to pick": if the request had + // extended the running job instead of getting one of its own, the picker would also take + // nothing and `fr` would still translate. + const queued = await jobsFor(id); + expect(queued.length, "the request did not get a job of its own").toBe(2); + expect( + queued.find((job) => !job.processing)?.input?.target_lngs, + "the second job should carry only the locale that was asked for" + ).toEqual(["fr"]); + + const whileRunning = await runQueue(); + expect( + Object.keys(whileRunning.jobStatus ?? {}), + "the picker took a second job for a document already being written" + ).toEqual([]); + + release?.(); + await first; + await runQueue(); + + expect(await titleIn(id, "de"), "de was lost").toBe("ecruos evisulcxE"); + expect(await titleIn(id, "fr"), "fr was lost").toBe("ecruos evisulcxE"); + }); + + it("still runs jobs for different documents together", async () => { + const first = await createDoc("Doc one"); + const second = await createDoc("Doc two"); + await enqueue(first, ["fr"]); + await enqueue(second, ["fr"]); + + const batch = await runQueue(); + + expect( + Object.keys(batch.jobStatus ?? {}).length, + "two documents were serialized against each other" + ).toBe(2); + }); +}); diff --git a/apps/dev/src/integration/translator/job-supersede.int.test.ts b/apps/dev/src/integration/translator/job-extend.int.test.ts similarity index 53% rename from apps/dev/src/integration/translator/job-supersede.int.test.ts rename to apps/dev/src/integration/translator/job-extend.int.test.ts index a83c653e6..7d194e618 100644 --- a/apps/dev/src/integration/translator/job-supersede.int.test.ts +++ b/apps/dev/src/integration/translator/job-extend.int.test.ts @@ -1,17 +1,10 @@ import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { bootTestPayload } from "./bootTestPayload"; +import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; import type { TestPayload } from "./bootTestPayload"; import { callEndpoint } from "./callEndpoint"; -// What a re-enqueue supersedes. Booted with the real job runner and autorun off, so jobs are written -// to `payload-jobs` and left there — the rows are the subject, not the translations. -// -// Rows are counted, never identified by id: SQLite reuses the rowid of a deleted row (integer primary -// key, no AUTOINCREMENT), so a replacement job can arrive carrying the deleted job's id and an -// id-based assertion would pass for the wrong reason. - type Job = { id: string | number; completedAt?: string | null; @@ -37,8 +30,7 @@ const enqueue = async (id: string, targets: string[] = ["de"]) => { return body.data.queued; }; -// One workflow per document, so these are workflow rows, not task rows. The cases share one boot, so -// the table also holds every earlier case's jobs. +// The cases share one boot, so the table also holds every earlier case's jobs. const jobs = async (documentId: string): Promise => { const { docs } = await ctx.payload.find({ collection: "payload-jobs" as "pages", @@ -55,13 +47,6 @@ const markFinished = (jobId: string | number) => data: { completedAt: new Date().toISOString(), processing: false } as never, }); -const markRunning = (jobId: string | number) => - ctx.payload.update({ - collection: "payload-jobs" as "pages", - id: jobId, - data: { processing: true } as never, - }); - const createDoc = async () => { const doc = await ctx.payload.create({ collection: "docs" as "pages", @@ -78,41 +63,60 @@ afterAll(async () => { await ctx?.cleanup(); }); -describe("re-enqueue supersedes work that has not begun, and nothing else", () => { - it("keeps a finished job when the document is translated again", async () => { +describe("a second request extends the live job rather than replacing it", () => { + it("adds its locales to a job that has not started", async () => { const id = await createDoc(); - expect(await enqueue(id), "fixture: the first enqueue queued a workflow").toBe(1); - - const [first] = await jobs(id); - await markFinished(first.id); - - expect(await enqueue(id), "the re-enqueue queued nothing").toBe(1); - - const after = await jobs(id); - expect(after.filter((j) => j.completedAt).length, "the finished job was deleted").toBe(1); - expect(after.length, "finished job plus the new one").toBe(2); + await enqueue(id, ["de", "fr"]); + await enqueue(id, ["es"]); + + const live = await jobs(id); + expect(live.length, "the second request queued a job of its own").toBe(1); + expect(live[0].input?.target_lngs, "the locales already owed were dropped").toEqual([ + "de", + "fr", + "es", + ]); }); - it("supersedes a job that has not started", async () => { + it("translates every locale the extended job accumulated", async () => { const id = await createDoc(); - await enqueue(id); - expect((await jobs(id)).length, "fixture").toBe(1); + await enqueue(id, ["de", "fr"]); + await enqueue(id, ["es"]); + + await ctx.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }); + + for (const locale of ["de", "fr", "es"]) { + const doc = (await ctx.payload.findByID({ + collection: "docs" as "pages", + id, + locale: locale as "en", + fallbackLocale: false, + draft: true, + })) as Record; + expect(doc.title, `${locale} was not translated`).toBe("crS"); + } + }); - expect(await enqueue(id), "the re-enqueue queued nothing").toBe(1); + it("keeps a locale the live job already owes from being listed twice", async () => { + const id = await createDoc(); + await enqueue(id, ["de"]); + await enqueue(id, ["de"]); - expect((await jobs(id)).length, "the pending job was not superseded").toBe(1); + const live = await jobs(id); + expect(live.length).toBe(1); + expect(live[0].input?.target_lngs, "the locale was queued a second time").toEqual(["de"]); }); - it("leaves a job that is already running alone", async () => { - // The reason supersession is narrowed to not-yet-started work: a running workflow holds locales - // it has already translated, and cancelling it would throw them away. + it("leaves a finished job alone and gives the new request its own", async () => { const id = await createDoc(); - await enqueue(id); - const [running] = await jobs(id); - await markRunning(running.id); + await enqueue(id, ["de"]); + const [first] = await jobs(id); + await markFinished(first.id); - expect(await enqueue(id), "the re-enqueue queued nothing").toBe(1); + await enqueue(id, ["fr"]); - expect((await jobs(id)).length, "the running job was cancelled").toBe(2); + const after = await jobs(id); + expect(after.filter((j) => j.completedAt).length, "the finished job was touched").toBe(1); + expect(after.length, "finished job plus the new one").toBe(2); }); }); diff --git a/apps/dev/src/integration/translator/locale-append.int.test.ts b/apps/dev/src/integration/translator/locale-append.int.test.ts new file mode 100644 index 000000000..eaf12b37a --- /dev/null +++ b/apps/dev/src/integration/translator/locale-append.int.test.ts @@ -0,0 +1,128 @@ +import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { callEndpoint } from "./callEndpoint"; + +// Guards an undocumented Payload behaviour: after each task settles it re-reads the job row onto the +// live `job` object, which is how a locale appended mid-run reaches the handler. Measured on 3.84.1; +// the plugin's peer floor is ^3.76.0. If a future version stops doing it, this file goes red instead +// of translations going missing. + +type Job = { + id: string | number; + completedAt?: string | null; + input?: { target_lngs?: string[] }; + log?: Array<{ state: string; input?: { target_lng?: string } }>; +}; + +const rev = (value: string) => [...value].reverse().join(""); + +let ctx: TestPayload; +let held: (() => void) | undefined; +let heldStarted: (() => void) | undefined; +const heldReached = new Promise((resolve) => { + heldStarted = resolve; +}); + +beforeAll(async () => { + ctx = await bootTestPayload({ + // Pinned off: a running job is only extended without the host's concurrency control — the other + // mode is `exclusive-queue.int.test.ts`. + exclusiveQueue: false, + runner: createPayloadJobsRunner({ autoRun: false }), + onTranslate: async (targetLng) => { + if (targetLng !== "fr") return; + heldStarted?.(); + await new Promise((resolve) => { + held = resolve; + }); + }, + }); +}); +afterAll(async () => { + held?.(); + await ctx?.cleanup(); +}); + +const readJob = async (): Promise => { + const { docs } = await ctx.payload.find({ + collection: "payload-jobs" as "pages", + pagination: false, + where: { workflowSlug: { equals: "translate_document_locales" } } as never, + }); + return docs[0] as Job; +}; + +describe("adding a locale to a running job", () => { + it("reaches the handler, and the log written before it survives", async () => { + const doc = await ctx.payload.create({ + collection: "docs" as "pages", + locale: "en", + data: { title: "Append source", _status: "published" } as never, + }); + const id = String(doc.id); + + await callEndpoint(ctx.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: ["de", "fr"], + collection_slug: "docs", + collection_id: [id], + strategy: "overwrite", + publish_on_translation: false, + }, + }); + + const run = ctx.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }); + await heldReached; + + // Mid-flight on `fr`. Without this the surviving-log claim is untested: if the log were still + // empty when the second request lands, nothing could be lost from it. + const before = await readJob(); + expect( + (before.log ?? []).map((e) => e.input?.target_lng), + "fixture: de should already be logged when the second request lands" + ).toEqual(["de"]); + + await callEndpoint(ctx.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: ["es"], + collection_slug: "docs", + collection_id: [id], + strategy: "overwrite", + publish_on_translation: false, + }, + }); + + held?.(); + await run; + + const after = await readJob(); + const logged = (after.log ?? []).map((e) => [e.input?.target_lng, e.state]); + + const esTitle = ( + (await ctx.payload.findByID({ + collection: "docs" as "pages", + id, + locale: "es" as "en", + fallbackLocale: false, + draft: true, + })) as Record + ).title; + + expect( + logged.map((l) => l[0]), + "the appended locale never ran" + ).toEqual(["de", "fr", "es"]); + expect(esTitle, "the appended locale was not translated").toBe(rev("Append source")); + + const { totalDocs } = await ctx.payload.count({ + collection: "payload-jobs" as "pages", + where: { workflowSlug: { equals: "translate_document_locales" } } as never, + }); + expect(totalDocs, "a second job was queued alongside the running one").toBe(1); + }); +}); diff --git a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts index 6ffdee7f4..97e2da65c 100644 --- a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts +++ b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts @@ -1,7 +1,7 @@ import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { bootTestPayload } from "./bootTestPayload"; +import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; import type { TestPayload } from "./bootTestPayload"; import { callEndpoint } from "./callEndpoint"; @@ -35,7 +35,7 @@ describe("when one locale's provider fails", () => { const res = await callEndpoint(failing.payload, "post", "/translate/enqueue", { body: { source_lng: "en", - target_lng: ["de", "fr"], + target_lng: ["de", "fr", "es"], collection_slug: "docs", collection_id: [id], strategy: "overwrite", @@ -44,7 +44,7 @@ describe("when one locale's provider fails", () => { }); expect(res.status).toBe(200); - await failing.payload.jobs.run({ queue: "translations", limit: 50 }); + await failing.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }); const read = async (locale: string) => ( @@ -59,25 +59,55 @@ describe("when one locale's provider fails", () => { expect(await read("de"), "the locale before the failure should have landed").toBe(rev(source)); expect(await read("fr"), "the failing locale should not have landed").toBeUndefined(); + expect(await read("es"), "the locale after the failure should be untouched").toBeUndefined(); - // Payload backs a failed job off exponentially, so the picker would skip it for the next few - // seconds. Clearing the delay lets the retry happen now rather than making the spec wait. const { docs } = await failing.payload.find({ collection: "payload-jobs" as "pages", pagination: false, where: { workflowSlug: { equals: "translate_document_locales" } } as never, }); + const log = (docs[0] as { log?: Array<{ state: string; input?: { target_lng?: string } }> }) + .log; + expect( + log?.map((entry) => [entry.input?.target_lng, entry.state]), + "the job should record which locale failed, and attempt no locale after it" + ).toEqual([ + ["de", "succeeded"], + ["fr", "failed"], + ]); + + const status = await callEndpoint( + failing.payload, + "get", + "/translate/document/:collection_slug/:collection_id", + { routeParams: { collection_slug: "docs", collection_id: id } } + ); + const rows = + (status.data as { data?: Array<{ status: string; input: { target_lng: string } }> }).data ?? + []; + expect( + rows.map((row) => [row.input.target_lng, row.status]).sort(), + "each locale should report its own outcome" + ).toEqual([ + ["de", "completed"], + ["es", "pending"], + ["fr", "failed"], + ]); + + // Payload backs a failed job off exponentially, so the picker would skip it for the next few + // seconds. Clearing the delay lets the retry happen now rather than making the spec wait. await failing.payload.update({ collection: "payload-jobs" as "pages", id: (docs[0] as { id: string | number }).id, data: { waitUntil: null, processing: false } as never, }); - // The retry must not translate `de` again — Payload's own task restoration skips a locale the - // log already records as succeeded. const before = failing.translateCount(); - await failing.payload.jobs.run({ queue: "translations", limit: 50 }); + await failing.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }); - expect(failing.translateCount() - before, "de was translated a second time").toBe(1); + expect( + failing.translateCount() - before, + "the retry should attempt only the locale that failed" + ).toBe(1); }); }); diff --git a/apps/dev/src/integration/translator/locale-workflow.int.test.ts b/apps/dev/src/integration/translator/locale-workflow.int.test.ts index 649ca91f2..451ea0825 100644 --- a/apps/dev/src/integration/translator/locale-workflow.int.test.ts +++ b/apps/dev/src/integration/translator/locale-workflow.int.test.ts @@ -1,13 +1,12 @@ import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { bootTestPayload } from "./bootTestPayload"; +import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; import type { TestPayload } from "./bootTestPayload"; import { callEndpoint } from "./callEndpoint"; -// Every requested locale must be translated. Booted with the REAL jobs runner — the production -// default — because the parallel fan-out this guards against exists only there: `createSyncRunner` -// translates inline and in order, which is why the rest of the suite never saw the defect. +// Must boot the real jobs runner: `createSyncRunner` translates inline and in order, so the fan-out +// this file guards against cannot occur under it. const rev = (s: string) => [...s].reverse().join(""); @@ -43,8 +42,7 @@ const enqueue = async (id: string, targets: string[]) => { expect(res.status, "the enqueue endpoint rejected the request").toBe(200); }; -// The same call the autorun cron makes, so the batching behaviour under test is the real one. -const runQueue = () => ctx.payload.jobs.run({ queue: "translations", limit: 50 }); +const runQueue = () => ctx.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }); const titleIn = async (id: string, locale: string) => ( @@ -64,7 +62,11 @@ const workflowJob = async (id: string) => { where: { workflowSlug: { equals: "translate_document_locales" } } as never, }); return ( - docs as Array<{ input?: { collection_id?: string }; log?: Array> }> + docs as Array<{ + completedAt?: string | null; + input?: { collection_id?: string }; + log?: Array>; + }> ).find((j) => j.input?.collection_id === id); }; @@ -80,19 +82,28 @@ describe("translating one document into several locales", () => { expect(await titleIn(id, "fr"), "fr was not translated").toBe(rev(source)); }); - it("records one log entry per locale, in the order they were requested", async () => { + it("runs the locales one after another, never overlapping", async () => { const id = await createDoc("Ordered source"); - await enqueue(id, ["de", "fr"]); + await enqueue(id, ["de", "fr", "es"]); await runQueue(); const job = await workflowJob(id); expect(job, "no workflow job was written").toBeDefined(); + const log = job?.log ?? []; expect( - job?.log?.map((e) => (e.input as { target_lng?: string })?.target_lng), + log.map((e) => (e.input as { target_lng?: string })?.target_lng), "the log should carry a row per locale, in request order" - ).toEqual(["de", "fr"]); - expect(job?.log?.map((e) => e.state)).toEqual(["succeeded", "succeeded"]); + ).toEqual(["de", "fr", "es"]); + expect(log.map((e) => e.state)).toEqual(["succeeded", "succeeded", "succeeded"]); + + // The order assertion above would also pass under `Promise.all`; only non-overlap rules it out. + for (let i = 1; i < log.length; i++) { + expect( + Date.parse(log[i].executedAt as string), + `locale ${i} started before locale ${i - 1} finished` + ).toBeGreaterThanOrEqual(Date.parse(log[i - 1].completedAt as string)); + } }); it("the status endpoint still reports a row per target locale", async () => { @@ -123,6 +134,10 @@ describe("translating one document into several locales", () => { await enqueue(id, ["de", "fr"]); await runQueue(); + // Without this the check passes for the wrong reason: a job left *failed* is also skipped on the + // second run, because the retry backoff pushes its `waitUntil` into the future. + expect((await workflowJob(id))?.completedAt, "the workflow did not complete").toBeTruthy(); + const before = ctx.translateCount(); await runQueue(); diff --git a/packages/payload-plugin-translator/README.md b/packages/payload-plugin-translator/README.md index dce52dd13..b43d87549 100644 --- a/packages/payload-plugin-translator/README.md +++ b/packages/payload-plugin-translator/README.md @@ -562,6 +562,43 @@ createPayloadJobsRunner({ taskName: "translate_document", queueName: "translatio > By default Payload deletes a job as soon as it completes, so the "Completed" status never shows in the UI. Set `jobs: { deleteJobOnComplete: false }` in your Payload config to keep it. +##### One job per document + +A document's target locales are queued as a **single job** that translates them one after another. +Every write Payload makes is a whole-document version snapshot, so two locales translated in parallel +build from the same base and the second silently drops the first's work. + +A later request for the same document adds its locales to that job rather than replacing it — the +locales the job still owes are never lost. Two cases get a job of their own instead: re-translating a +locale the live job has already finished (its log records it as done, so it would be skipped), and a +request that picked a different source locale, strategy or publish flag — a job carries one of each +for all its locales, so it cannot take work that chose differently. + +##### Optional: strict one-at-a-time per document + +Two requests landing at the same instant, or a re-translation of an already-finished locale, can still +put two jobs on one document. If your content is edited often enough for that to matter, enable +Payload's own concurrency control: + +```typescript +// payload.config.ts +export default buildConfig({ + jobs: { enableConcurrencyControl: true }, + // ... +}); +``` + +The plugin picks this up on its own — there is no option to set here. With it on, the queue holds a +second job for a document until the running one finishes, so two jobs can never write the same +document at once. Jobs for *different* documents still run in parallel. + +The cost is yours to weigh: the setting adds an indexed `concurrencyKey` column to the jobs +collection, so a SQL database needs a migration (`payload migrate:create` then `payload migrate`); +MongoDB needs none. A second job also waits for the next queue run rather than starting immediately. + +> With the setting on, a job stuck at `processing: true` blocks every other job for that document +> until its lock is reclaimed. The plugin clears stale locks on boot — see `staleJobTimeoutMs`. + #### `createSyncRunner()` Runs translations inline (no queue) — handy for development or small datasets. diff --git a/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md b/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md index f62d82134..a2d6340a0 100644 --- a/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md +++ b/packages/payload-plugin-translator/docs/plans/2026-09-04-locale-workflow.task.md @@ -50,11 +50,17 @@ groups them by collection. Grouping by document and queueing one workflow is a c that class. Rejected: changing `TaskInput` to carry a locale array — it would touch both callers, the sync runner, the lifecycle wrapper and the endpoint, for no gain, since the array already arrives. -**D3 — supersession cancels only jobs that have not started.** -The user's decision. A workflow already running keeps its already-translated locales and finishes; -the new request queues behind it. Rejected: cancelling the whole workflow, which is closer to today's -semantics but discards locales that were already translated in the current run — the very work this -change exists to stop losing. +**D3 — a re-enqueue supersedes every live job for the document, the one in flight included.** +The user's decision, taken twice. The first version spared a running workflow so it could keep its +already-translated locales; review showed that premise does not hold — Payload has no "queue behind", +its picker takes any job that is not currently processing, so the spared job and the new one write the +document at once on the next cron tick. That is the lost update at workflow granularity. Rejected on +the second pass: leaving the running job and dropping the new request, which loses no work but also +never translates the edit the user just asked for. + +What a cancelled run actually costs: nothing that was translated, because those locales are already +written to the documents. Only the job's log and its unfinished locale go, and the new workflow +redoes that locale against the newer source — which is what the user asked for by re-enqueueing. **D4 — the panel reads per-locale state from the job log.** The user's decision. Payload writes an entry per task, so the detail survives the collapse from N jobs @@ -85,7 +91,9 @@ completion after a failure. Neither is expressible in a signature. failed. *Check: integration test with a provider that throws for one locale.* 5. **A retry resumes rather than restarting** — a locale already translated is not sent to the provider again. *Check: integration test using the boot's `translateCount()`.* -6. **Supersession cancels only jobs that have not started.** *Check: unit test on the runner.* +6. **A re-enqueue leaves exactly one live job on the document**, whatever state the old one was in — + not started, queued for retry with every locale logged, or in flight. *Check: unit tests on the + runner plus `job-extend.int.test.ts`.* 7. **The status endpoints still report per-locale state**, now from the job log. *Check: integration test through the real endpoints.* 8. **The sync runner is unaffected.** *Check: the existing 73 integration tests stay green on SQLite.* @@ -93,7 +101,8 @@ completion after a failure. Neither is expressible in a signature. ## Human choices -- **Supersede only not-yet-started jobs** (D3). Rejected alternative recorded above. +- **Supersede every live job for the document** (D3), after the first version's premise was + disproved. Rejected alternative recorded above. - **Panel reads the job log** (D4). Rejected alternative recorded above. - **Fix this under #114** rather than opening a separate issue, with #114's description extended by a comment to cover the wider defect. @@ -127,6 +136,53 @@ panel reading per-locale state from the job log only works for hosts that keep c **A retry is delayed by backoff.** Payload backs a failed job off exponentially, so a retry cannot be observed by simply running the queue again; the spec clears `waitUntil` rather than waiting. + ## Review log -_(appended by review runs)_ +**2026-09-04 — three review passes over the first implementation.** All findings were checked against +Payload 3.84.1's source before being acted on; four were real defects and are fixed here. + +- **`reclaimStaleJobs` had gone dead.** It narrowed by `taskSlug`, and `jobs.queue({ workflow })` + writes `workflowSlug` and leaves `taskSlug` null (`queues/localAPI.js:51-54`), so after this change + no job matched it. Boot-time stale-lock recovery would have reported zero reclaimed, forever, + without erroring. Both slug predicates now come from one `ownJobs()`. +- **`run()` read a locale row instead of the job.** `handleTaskError.js:43` stamps `completedAt` on a + *failed* log entry too, so a partially-failed workflow presented rows that all carried one, and the + Retry button answered `already_completed` → 404 for exactly the jobs a user presses it on. The + runner's internal reads now go through `findRawJobs` + `normalizeJob`; only `findByCollection`, + which feeds the panels, expands to locale rows. +- **The supersession predicate reopened #114.** It filtered on an expanded row's `status`, and a job + whose locales are all logged has no `pending` row — yet a non-final task failure leaves it queued + (`hasError: false`, `processing: false`). Two live jobs on one document, picked into one + `Promise.all` batch. See D3 above for how this was settled. +- **N reads of the job table.** The lookup sat inside the per-document loop while `findRawJobs` is + unpaginated and filters in memory, so `select_all` meant one full scan per document. One read now + serves the whole batch, and the orphaned `groupByCollection` is gone. + +**Test findings, and what closed them.** The audit's central result was that D4 was not tested at all: +`normalizeJobLocales` could have its entire `job.log` branch deleted and the suite stayed green — in +production that is the panel reporting "completed" for a locale that failed. Seven unit cases now +cover it, and the audit's own empty implementation was pasted in to confirm four of them go red. +Three more gaps closed: the sequencing check asserted order, which survives `Promise.all`, and now +asserts non-overlap (`executedAt` of each locale against the previous locale's `completedAt`) — +verified by mutating the handler to `Promise.all`, which reddens it; the failure spec failed the +*last* locale, so "stops there" was unobservable, and now runs three locales failing the middle one; +and "does not re-run a completed workflow" passed equally for a job left failed, so it now asserts the +workflow reached `completedAt` first. A third locale (`es`) was added to the shared harness for this. + +**Comment audit.** 110 comment lines over 569 lines of code, judged too dense. The #114 lost-update +story had been written out in five places; `PayloadJobsTaskRunner.enqueue` now owns it and the others +point at it or are gone. Twenty-two comments deleted or shortened, mostly narration of the line below. +What was kept is measured Payload behaviour a reader cannot recover without opening `node_modules`: +`completedAt` on failed log entries, the locale-as-task-id restoration, the retry backoff, the +`getPayload` per-process cache. + +**Verification.** Unit 1332 in the plugin; integration 15/15 on SQLite and MongoDB, 13/15 on Postgres +— the two failing files are the known #124 auto-translate cases, red on `main` as well. check-types +clean in both packages; lint at the repo baseline. + +**Left open, deliberately.** `enqueue` cancels the old job and queues the new one without a +transaction, so a batch spanning several documents can leave one document's old job deleted and its +new workflow unqueued if a later document throws. _Closed by +[2026-09-08-one-live-job-per-document](./2026-09-08-one-live-job-per-document.task.md): `enqueue` no +longer cancels or deletes anything._ diff --git a/packages/payload-plugin-translator/docs/plans/2026-09-08-one-live-job-per-document.task.md b/packages/payload-plugin-translator/docs/plans/2026-09-08-one-live-job-per-document.task.md new file mode 100644 index 000000000..1681073b7 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-09-08-one-live-job-per-document.task.md @@ -0,0 +1,187 @@ +# Task contract — one live job per document, extended rather than replaced + +Continues [2026-09-04-locale-workflow.task.md](./2026-09-04-locale-workflow.task.md), which made a +document's locales one workflow job. That change left the *second request* case wrong: a new request +replaced the live job outright and dropped whatever locales it still owed. + +**Risk: HIGH.** Changes what `enqueue` writes, removes supersession entirely, touches a security +defect in the cancel path, and adds a second execution mode. Data an editor sees is affected. + +## Stage 1 — extend the live job instead of replacing it + +**The defect.** `enqueue` cancelled every live job for the document and queued a replacement carrying +only the locales of the current request. The panel's per-row "re-translate" button sends exactly one +locale, so a live job holding `['de','fr','es']` became `['fr']` and two locales vanished with no +trace. Introduced by the supersession rule added on 2026-09-04. + +**D1 — a locale is added to the live job's stored list; no cancel, no delete.** +Measured before choosing (see *Evidence*): a locale written into a running job's row IS picked up by +its handler, on all three adapters. So "queue behind" needs no new store — `target_lngs` already is +the queue. Rejected: keep replacing and carry over the unfinished locales. It works, but it cancels a +running job, and cancellation was measured not to stop one (see D3 of the earlier contract, now +superseded) — the row is deleted before the handler can read the flag. + +**D2 — the write is narrow: the `input` column only, through the database adapter.** +Measured: `payload.update` is the full document operation — it re-reads, merges and writes the row +whole, reverting log entries pushed in between. Observed at 3/120 on Postgres and 1/120 on MongoDB. +A narrow write showed 0 clobbering across ~1000 rounds on all three adapters. The jobs collection's +only `beforeChange` hook guards a *cancelled* job from being revived; we never append to a cancelled +job, so skipping hooks costs nothing. + +**D3 — the choice lives in one pure function, `planEnqueue`.** +Three situations (no live job · add to it · give it a job of its own) and, from stage 3, one more +input (is the queue exclusive). Keeping it inline would put the mode check in the middle of an +I/O-heavy method. Rejected: a strategy object per mode — the two modes differ in exactly one branch, +so an interface plus two implementations costs more than the branch. + +**D4 — after writing, verify.** +The residual loss is the read-modify-write gap: the job can complete between our read and our write, +leaving the locale in the list with nobody to run it. Measured at 0–5% of appends landing mid-run. +Detectable: re-read after writing; if the job has completed, or the locale is not in the stored list, +give those locales a job of their own. That fallback fires only when the old job is already finished, +so it never creates two live jobs. + +## Stage 2 — defects found by the review passes + +1. **The panel crashes** when a locale failed and the job carries no final error yet: the client reads + `run.error.message` unconditionally for `status: "failed"`. Introduced on 2026-09-04, when a row's + status started coming from the job log while its error still came from the job. +2. **`/translate/cancel` deletes any row in `payload-jobs`.** Ids come from the request body and the + delete is not narrowed to this plugin's jobs; the default access guard allows everyone. `ownJobs()` + already exists and is simply not applied there. +3. **`cancel-by-collection` skips jobs waiting to retry** — the same expanded-row `status === "pending"` + predicate that was already fixed in `enqueue`. +4. **`run()` claims success without running anything.** Payload returns `noJobsRemaining` / + `remainingJobsFromQueried: 0` when the picker takes nothing; we ignore it. + +## Stage 3 — honour the host's `enableConcurrencyControl` + +**D5 — we never set the flag; we adapt to it.** +The user's decision. Enabling it adds an indexed column to `payload-jobs` and needs a migration on SQL +— that is the host's call, not a plugin's. Rejected: a plugin option that sets it, and setting it +unconditionally; both make the plugin responsible for someone else's schema. + +**D6 — no plugin option, no config field.** The fact is readable at the point of use +(`payload.config.jobs.enableConcurrencyControl`). Adding an option would be a second source of truth +for something Payload already stores, and Payload refuses to boot if `concurrency` is declared while +the flag is off — so the two can never legitimately disagree. + +With the flag on, the workflow declares `concurrency: { key: :, exclusive: true }` and +`planEnqueue` stops appending to a *running* job: the new job is queued alongside and Payload holds it +until the running one finishes. + +## Evidence gathered before designing + +Probes run on SQLite, Postgres and MongoDB, ~1000 rounds total. + +| Question | Answer | +|---|---| +| Does a running handler see a locale appended to its row? | Yes, on all three adapters (28/28 with a barrier) | +| Is the probe able to fail? | Yes — reverting the handler's loop to a one-time read reddens it | +| Does `payload.update` clobber the job log? | Yes: 3/120 Postgres, 1/120 MongoDB | +| Does a narrow write clobber it? | No: 0 across all adapters | +| Residual loss with a narrow write | 0–5% of appends landing mid-run; always "write landed, handler never ran it", never "write vanished" | +| Does the handler reading the row itself help? | No measurable difference — dropped | + +## Acceptance criteria + +1. **A second request adds its locales instead of replacing the job.** Enqueue `['de','fr']`, then + enqueue `['es']` for the same document before the queue runs; one job exists carrying all three. + *Check: integration test on SQLite.* Fails now — today the second request replaces the first. +2. **Nothing a live job still owes is dropped.** After the sequence in (1), running the queue + translates all three locales. *Check: same test.* Fails now. +3. **A locale appended to a job already running is picked up.** *Check: integration test with a + barrier holding one locale.* Fails now — no append path exists. +4. **The job log survives an append.** *Check: the same test asserts the entry written before the + append is still present.* +5. **A locale that cannot be delivered gets a job of its own.** *Check: unit test on the runner — + the stored row reports completed after the write.* +6. **`planEnqueue` returns the right plan for each situation**, in both modes. *Check: unit tests.* +7. **The panel no longer crashes on a failed locale with no job error.** *Check: unit test on + `buildTranslationStatusRows`.* Fails now. +8. **Cancel only ever deletes this plugin's jobs.** *Check: unit test asserting the delete's `where` + carries the own-jobs predicate.* Fails now. +9. **`cancel-by-collection` cancels a job waiting to retry.** *Check: unit test.* Fails now. +10. **`run()` reports failure when the picker took nothing.** *Check: unit test.* Fails now. +11. **With the host's flag on, two jobs for one document never run at once, and none is lost.** + *Check: integration test booted with `enableConcurrencyControl`.* +12. **With the flag on, jobs for different documents still run in parallel.** *Check: same file.* +13. **The workflow declares `concurrency` only when the flag is on.** *Check: unit test on the + provider — Payload refuses to boot otherwise.* +14. **The existing integration suite passes in both modes**, on all three adapters. +15. **Checks clean:** unit tests, check-types in both packages, lint at the repo baseline. + +## Human choices + +- **Extend the live job rather than replace it** (D1), after being shown that a replacement drops the + locales the old job still owed. +- **Never set `enableConcurrencyControl` ourselves** (D5). Stated as: requiring the host to flip a flag + to fix our bug would be offloading the bug onto them, and flipping it silently is worse. +- **One gate, not two implementations** (D3) — the user asked explicitly not to smear two modes across + the code. +- **Test the second mode too**, including the whole suite under the flag, not just a targeted test. +- **The row/job type split is out of scope** and stays a follow-up. + +## Risks + +- The append path rests on Payload refreshing `job.input` from the row after each task — an + implementation detail, not a documented contract, measured on 3.84.1 against a peer floor of + `^3.76.0`. Criterion 3's test is what turns a future change into a red run instead of silent loss. +- Removing supersession means a document can briefly hold a finished job and a new one. Nothing reads + "the" job for a document, so no caller breaks, but it changes what the panel lists. +- Enabling the host's flag adds one query per queue run for every job in the host's app, not just ours. + +## Review log + +**2026-09-08 — three review passes, all findings checked against the code before acting.** + +**Design review.** Three real defects, all fixed here. +- *The auto-translate debounce had stopped coalescing.* The common case is the same locales on every + save, which leaves nothing to append — so `enqueue` did nothing at all and the pending job kept the + first edit's `waitUntil`. The plan now names the host job even when it has nothing to add, and the + debounce moves with the request. +- *A request's settings were silently discarded.* A job carries one source locale, one strategy and + one publish flag for all of its locales; a request that chose differently would have run under the + job's. `pickHost` now requires all three to match, and `enqueue` groups by them, which makes + `queueWorkflow` taking them from the first task true by construction rather than by luck. +- *Two concurrent appends lost one, undetectably.* The verify step only asked about its own locales, + so a competing write that replaced the whole list looked like success. The write now retries once + from the stored row — a set union, so retrying is harmless — and anything still missing gets a job + of its own. + +Also: documents are served in parallel again (the loop had made a `select_all` enqueue N sequential +round trips), `findRawJobs` reads at `depth: 0` so the legacy relationship field is not populated, +and `workflowName` was removed from the public options and derived from `taskName` — nobody asked to +set it, and keeping it out means no new public surface and no `@since` to date. + +Corrected while doing this: `ultracite fix` had rewritten two constructors away from the parameter +properties every sibling handler uses, and dropped a `private` in the process. Reverted. + +**Test audit (mutation-based).** 13 mutations, **7 survived** the first suite. The worst: deleting +the per-document filter on the live jobs handed to `planEnqueue` kept all 127 unit tests green — under +it, enqueuing for one document appends to another document's job. Also surviving: ignoring +`exclusiveQueue`; taking the last live job instead of the newest; dropping the debounce carry; +blanking the other stored fields on append; and the plugin switching the host's +`enableConcurrencyControl` on for them. Each now has a check, and each mutation was re-run to confirm +it dies. + +One integration check was self-satisfying: `exclusive-queue` asserted the picker took nothing, which +is equally true when there was nothing to take. It now first requires the second job to exist and to +carry exactly the requested locale. + +**Comment audit.** 215 comment lines over 1313 of code, judged too dense; now 164 over ~1570. One +comment was outright false — "groups are disjoint by document" stopped being true once the grouping +key gained the settings — and several pointed at code the diff had moved or renamed. The recurring +fault was one rationale written out in up to eight places; each now has a single owner and the rest +point at it or are gone. What was kept is measured Payload behaviour a reader cannot recover without +opening `node_modules`. + +**Verification.** Unit 1359 in the plugin; check-types clean in both packages; lint 8 warnings on the +changed files against 10 on `main` for the same files, 0 errors. Integration on three adapters in both +queue modes — six runs: SQLite 82/82 and MongoDB 82/82 in each, Postgres 79/82 in each, the three +failures being the known #124 auto-translate cases that are red on `main` as well. + +**Left open.** The row/job type split (several panel rows share one job id, so per-row Cancel acts on +the whole document) stays a follow-up — it changes the `TaskRunner` contract, both runners, five +handlers and the client. + diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.test.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.test.ts index fe767e694..fb02387bc 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.test.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.test.ts @@ -123,6 +123,21 @@ describe("buildTranslationStatusRows", () => { expect(buildTranslationStatusRows({})).toEqual([]); expect(buildTranslationStatusRows({ staleness: { locales: [] }, runs: [] })).toEqual([]); }); + it("renders a failed locale whose job carries no error yet", () => { + const failedWithoutError = { + id: "job-de", + status: DocumentTranslationStatus.FAILED, + created_at: "2026-07-06T00:00:00.000Z", + updated_at: "2026-07-07T00:00:00.000Z", + input: { source_lng: "en", target_lng: "de" }, + } as DocumentTranslation; + + const rows = buildTranslationStatusRows({ runs: [failedWithoutError] }); + + expect(rows).toHaveLength(1); + expect(rows[0].state).toBe("failed"); + expect(rows[0].error).toBeUndefined(); + }); }); describe("STATE_DOT", () => { diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.ts index 9d2634e6d..e421fa13c 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/statusRows.ts @@ -91,7 +91,7 @@ export function buildTranslationStatusRows(input: { state: existing && !TRANSIENT.has(state) ? existing.state : state, at: run.updated_at, jobId: TRANSIENT.has(state) ? run.id : existing?.jobId, - error: run.status === "failed" ? run.error.message : undefined, + error: run.status === "failed" ? run.error?.message : undefined, }); } } diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts index fd2f02d89..c28fece78 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts @@ -19,7 +19,12 @@ export type DocumentTranslationFailed = { created_at: string; updated_at: string; input: InputData; - error: { + /** + * Absent while the job itself carries no final error. A locale's failure is recorded in the job + * log as soon as it happens, but the job keeps `error` unset until it stops retrying — so a row + * can read `failed` with nothing to show yet. + */ + error?: { message: string; }; }; diff --git a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts index f4104fe85..fd35b0fc4 100644 --- a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts +++ b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts @@ -95,10 +95,10 @@ describe("CancelByCollectionHandler", () => { expect(response.body).toBeNull(); }); - it("returns 204 when all tasks are already completed", async () => { + it("returns 204 when every job is in flight", async () => { const tasks = [ - createMockTask({ id: "task-1", status: "completed" }), - createMockTask({ id: "task-2", status: "failed" }), + createMockTask({ id: "task-1", status: "running" }), + createMockTask({ id: "task-2", status: "running" }), ]; (mockTaskRunner.findByCollection as ReturnType).mockResolvedValue(tasks); @@ -125,12 +125,11 @@ describe("CancelByCollectionHandler", () => { }); describe("cancelling pending tasks", () => { - it("cancels only pending tasks", async () => { + it("cancels every queued job and leaves the one in flight alone", async () => { const tasks = [ createMockTask({ id: "task-1", status: "pending" }), createMockTask({ id: "task-2", status: "running" }), createMockTask({ id: "task-3", status: "pending" }), - createMockTask({ id: "task-4", status: "completed" }), ]; (mockTaskRunner.findByCollection as ReturnType).mockResolvedValue(tasks); @@ -141,12 +140,39 @@ describe("CancelByCollectionHandler", () => { expect(mockTaskRunner.cancel).toHaveBeenCalledWith(["task-1", "task-3"]); }); + it("cancels a job waiting to retry after a failure", async () => { + const tasks = [ + createMockTask({ id: "task-1", status: "completed" }), + createMockTask({ id: "task-1", status: "failed" }), + ]; + (mockTaskRunner.findByCollection as ReturnType).mockResolvedValue(tasks); + + const req = createMockRequest({ collection_slug: "posts" }); + await handler.handle(req); + + expect(mockTaskRunner.cancel).toHaveBeenCalledWith(["task-1"]); + }); + + it("names each job once, however many locale rows it has", async () => { + const tasks = [ + createMockTask({ id: "task-1", status: "pending" }), + createMockTask({ id: "task-1", status: "pending" }), + ]; + (mockTaskRunner.findByCollection as ReturnType).mockResolvedValue(tasks); + + await handler.handle(createMockRequest({ collection_slug: "posts" })); + + expect(mockTaskRunner.cancel).toHaveBeenCalledWith(["task-1"]); + }); + it("calls findByCollection with correct collection slug", async () => { const req = createMockRequest({ collection_slug: "pages" }); await handler.handle(req); - expect(mockTaskRunner.findByCollection).toHaveBeenCalledWith("pages"); + expect(mockTaskRunner.findByCollection).toHaveBeenCalledWith("pages", { + excludeCompleted: true, + }); }); it("creates task runner with request payload", async () => { diff --git a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts index fa90f2309..d1a601341 100644 --- a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts +++ b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts @@ -29,13 +29,17 @@ export class CancelByCollectionHandler { return ServerResponse.badRequest("Collection not available for translation"); const runner = this.taskRunnerFactory.create(req.payload); - const tasks = await runner.findByCollection(collectionSlug); - if (tasks.length === 0) return ServerResponse.noContent(); + const rows = await runner.findByCollection(collectionSlug, { excludeCompleted: true }); + if (rows.length === 0) return ServerResponse.noContent(); - const pendingTaskIds = tasks.filter((task) => task.status === "pending").map((task) => task.id); - if (pendingTaskIds.length === 0) return ServerResponse.noContent(); + // Rows are per locale, cancel addresses jobs. Filtering rows by `pending` misses a job waiting + // to retry: all of its locales are logged, so it has no pending row, yet the picker takes it + // again. + const running = new Set(rows.filter((row) => row.status === "running").map((row) => row.id)); + const queuedJobIds = [...new Set(rows.map((row) => row.id))].filter((id) => !running.has(id)); + if (queuedJobIds.length === 0) return ServerResponse.noContent(); - await runner.cancel(pendingTaskIds); + await runner.cancel(queuedJobIds); return ServerResponse.noContent(); } diff --git a/packages/payload-plugin-translator/src/server/features/enqueue-translation/handler.ts b/packages/payload-plugin-translator/src/server/features/enqueue-translation/handler.ts index 619cd6e10..622135bcc 100644 --- a/packages/payload-plugin-translator/src/server/features/enqueue-translation/handler.ts +++ b/packages/payload-plugin-translator/src/server/features/enqueue-translation/handler.ts @@ -77,8 +77,6 @@ export class EnqueueTranslationHandler { : collection_id; const runner = this.taskRunnerFactory.create(req.payload); - // One task per (document x target locale). The runner keys/supersedes per (document, targetLng), - // so N concurrent targets of one document coexist (PR #75) — no runner change needed. const tasks = collectionIds.flatMap((id) => targets.map((targetLng) => ({ collectionSlug, diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts index 96cba607f..71d4541ed 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts @@ -18,6 +18,13 @@ const makePayload = (overrides?: { update?: ReturnType }) => ({ logger: { error: vi.fn() }, }); +const workflowOf = (config: Config) => + (config.jobs?.workflows ?? []).find( + (w) => w.slug === "translate_document_locales" + ) as unknown as { + concurrency?: { key: (a: never) => string; exclusive?: boolean }; + }; + describe("PayloadJobsRunnerProvider", () => { describe("configure().onInit", () => { it("returns a config whose onInit is a function that triggers reclaim", async () => { @@ -85,4 +92,26 @@ describe("PayloadJobsRunnerProvider", () => { expect(() => createPayloadJobsRunner({ staleJobTimeoutMs: 60_000 })).not.toThrow(); }); }); + + describe("the workflow's concurrency declaration", () => { + it("is absent when the host has not enabled concurrency control", () => { + const config = createPayloadJobsRunner().configure(minimalContext)(makeConfig()); + + expect(workflowOf(config).concurrency).toBeUndefined(); + // The plugin must not turn the flag on for the host — it is a schema decision, see the README. + expect(config.jobs?.enableConcurrencyControl).toBeUndefined(); + }); + + it("keys on the document when the host has enabled it", () => { + const config = createPayloadJobsRunner().configure(minimalContext)( + makeConfig({ jobs: { enableConcurrencyControl: true } }) + ); + + const concurrency = workflowOf(config).concurrency; + expect(concurrency?.exclusive).toBe(true); + expect( + concurrency?.key({ input: { collection_slug: "posts", collection_id: "7" } } as never) + ).toBe("posts:7"); + }); + }); }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts index 31f2db81c..11c988087 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts @@ -16,7 +16,6 @@ const DEFAULT_STALE_JOB_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes const defaultValues = { taskName: "translate_document", - workflowName: "translate_document_locales", queueName: "translations", jobsCollection: "payload-jobs", autoRun: defaultAutoRun, @@ -55,7 +54,7 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { this.config = { taskName: options?.taskName ?? defaultValues.taskName, - workflowName: options?.workflowName ?? defaultValues.workflowName, + workflowName: `${options?.taskName ?? defaultValues.taskName}_locales`, queueName: options?.queueName ?? defaultValues.queueName, jobsCollection: options?.jobsCollection ?? defaultValues.jobsCollection, autoRun, @@ -130,9 +129,9 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { }, ]; - // Same fields as the task, except the single target locale becomes the list the workflow walks. const workflowInputSchema: Field[] = [ ...inputSchema.filter((f) => "name" in f && f.name !== "target_lng"), + // json, not an array field: the input stays one column, so no migration is needed { type: "json", name: "target_lngs", required: true }, ]; @@ -166,20 +165,34 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { }, }; - // One workflow per document, awaiting one task per locale. The tasks run in sequence, which is - // the whole point: every write Payload makes is a whole-document version snapshot, so two - // locales translated in parallel build their snapshots from the same base and the second one - // silently drops the first's work. See issue #114. + // Locales run in sequence: every Payload write is a whole-document version snapshot, so two + // translated in parallel build from the same base and the second drops the first (issue #114). const workflow = { slug: workflowName, inputSchema: workflowInputSchema, retries, + // Conditional because Payload refuses to boot when a workflow declares `concurrency` while + // `enableConcurrencyControl` is off. The flag is the host's to set — see the README. + ...(config.jobs?.enableConcurrencyControl + ? { + concurrency: { + key: ({ input }: { input: { collection_slug?: string; collection_id?: string } }) => + `${input.collection_slug}:${input.collection_id}`, + exclusive: true, + }, + } + : {}), handler: async (args: { job: { input: { target_lngs?: string[] } & Record }; tasks: Record Promise>; }) => { - const { target_lngs: targets = [], ...shared } = args.job.input; - for (const target of targets) { + // Re-read the list every turn instead of destructuring it once. Payload replaces + // `job.input` with a freshly-read row after each task settles, so a locale appended to the + // stored job while this one runs is picked up here rather than being lost. + for (let i = 0; ; i++) { + const { target_lngs: targets, ...shared } = args.job.input; + const target = targets?.[i]; + if (target === undefined) return; // The locale is the task id, so Payload's own restoration skips a locale already logged // as succeeded when a failed workflow is retried. await args.tasks[taskName](target, { input: { ...shared, target_lng: target } }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts index 513a1a4b3..203190991 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts @@ -9,6 +9,8 @@ describe("PayloadJobsTaskRunner", () => { find: ReturnType; delete: ReturnType; update: ReturnType; + db: { updateOne: ReturnType }; + config: { jobs?: { enableConcurrencyControl?: boolean } }; jobs: { queue: ReturnType; cancel: ReturnType; @@ -24,10 +26,15 @@ describe("PayloadJobsTaskRunner", () => { find: vi.fn().mockResolvedValue({ docs: [] }), delete: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue({ docs: [] }), + db: { updateOne: vi.fn().mockResolvedValue(undefined) }, + config: { jobs: {} }, jobs: { queue: vi.fn().mockResolvedValue(undefined), cancel: vi.fn().mockResolvedValue(undefined), - run: vi.fn().mockResolvedValue({ jobStatus: {}, remainingJobsFromQueried: 0 }), + // A non-empty `jobStatus` is how Payload reports that the picker actually took a job. + run: vi + .fn() + .mockResolvedValue({ jobStatus: { "job-123": {} }, remainingJobsFromQueried: 0 }), // kept only so tests can assert run() never falls back to the broken // runByID id-path — production code does not call it. runByID: vi.fn().mockResolvedValue(undefined), @@ -70,6 +77,20 @@ describe("PayloadJobsTaskRunner", () => { ...overrides, }); + /** A live workflow job whose settings match `createInput()`'s, so `pickHost` accepts it. */ + const createLiveJob = (overrides: Partial = {}): PayloadJob => + createJob({ + input: { + collection_slug: "posts", + collection_id: "doc-123", + source_lng: "en", + strategy: "overwrite", + publish_on_translation: false, + target_lngs: ["de"], + }, + ...overrides, + }); + describe("enqueue", () => { it("looks for superseded jobs among unfinished ones only", async () => { await runner.enqueue([createInput()]); @@ -133,75 +154,171 @@ describe("PayloadJobsTaskRunner", () => { expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(2); }); - it("cancels existing jobs before queuing new ones", async () => { - const existingJob = createJob({ id: "existing-job" }); - mockPayload.find.mockResolvedValueOnce({ docs: [existingJob] }); + it("adds the locale to a live job instead of queuing a second one", async () => { + const live = createLiveJob({ id: "live-job" }); + // First read builds the plan; the second is the check that the write landed, so it answers + // with the row as the write leaves it. + mockPayload.find.mockResolvedValueOnce({ docs: [live] }).mockResolvedValueOnce({ + docs: [{ ...live, input: { ...live.input, target_lngs: ["de", "fr"] } }], + }); - const input = createInput(); - await runner.enqueue([input]); + await runner.enqueue([createInput({ targetLng: "fr" })]); - expect(mockPayload.jobs.cancel).toHaveBeenCalledWith({ - where: { id: { in: ["existing-job"] } }, - queue: "translations", + const write = mockPayload.db.updateOne.mock.calls[0][0]; + expect(write.collection).toBe("payload-jobs"); + expect(write.id).toBe("live-job"); + expect(write.data.input).toEqual({ + collection_slug: "posts", + collection_id: "doc-123", + source_lng: "en", + strategy: "overwrite", + publish_on_translation: false, + target_lngs: ["de", "fr"], }); - expect(mockPayload.delete).toHaveBeenCalledWith({ - collection: "payload-jobs", - where: { id: { in: ["existing-job"] } }, + expect(mockPayload.jobs.queue).not.toHaveBeenCalled(); + }); + + it("does not extend a live job that belongs to a different document", async () => { + mockPayload.find.mockResolvedValue({ + docs: [ + createJob({ + id: "other-doc-job", + input: { + collection_slug: "posts", + collection_id: "doc-999", + source_lng: "en", + strategy: "overwrite", + target_lngs: ["de"], + }, + }), + ], }); + + await runner.enqueue([createInput({ targetLng: "fr" })]); + + expect(mockPayload.db.updateOne).not.toHaveBeenCalled(); + expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(1); }); - it("does not cancel a running job for a different target locale of the same document", async () => { - // A `de` job is in flight; the user re-translates `fr` on the same document. The `de` job must - // survive — cancelling it is the concurrent re-translate bug. - const runningDe = createJob({ - id: "de-job", - processing: true, - input: { - collection: { relationTo: "posts" as CollectionSlug, value: "doc-123" }, - source_lng: "en", - target_lng: "de", - strategy: "overwrite", - }, + it("never cancels or deletes a job when enqueuing", async () => { + const live = createLiveJob({ + id: "live-job", + }); + mockPayload.find.mockResolvedValueOnce({ docs: [live] }).mockResolvedValueOnce({ + docs: [{ ...live, input: { ...live.input, target_lngs: ["de", "fr"] } }], }); - mockPayload.find.mockResolvedValueOnce({ docs: [runningDe] }); await runner.enqueue([createInput({ targetLng: "fr" })]); expect(mockPayload.jobs.cancel).not.toHaveBeenCalled(); expect(mockPayload.delete).not.toHaveBeenCalled(); - expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(1); }); - it("supersedes a job that has not started, and leaves a running one alone", async () => { - // Narrowed deliberately: a running workflow holds the locales it has already translated, and - // cancelling it would discard them — the loss this whole change exists to stop. See issue #114. - const pending = createJob({ - id: "pending-job", - input: { collection_slug: "posts", collection_id: "doc-123", source_lng: "en" }, + it("gives the locale its own job when the live one finished after the write landed", async () => { + const live = createLiveJob({ id: "live-job" }); + mockPayload.find.mockResolvedValueOnce({ docs: [live] }).mockResolvedValueOnce({ + docs: [ + { + ...live, + completedAt: "2026-01-01T00:00:01Z", + input: { ...live.input, target_lngs: ["de", "fr"] }, + }, + ], }); - const running = createJob({ - id: "running-job", - processing: true, - input: { collection_slug: "posts", collection_id: "doc-123", source_lng: "en" }, + + await runner.enqueue([createInput({ targetLng: "fr" })]); + + expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(1); + expect(mockPayload.jobs.queue).toHaveBeenCalledWith( + expect.objectContaining({ + input: expect.objectContaining({ target_lngs: ["fr"] }), + }) + ); + }); + + it("retries the write once when a concurrent append replaced the list", async () => { + // Not finished, unlike the case above — the locale is simply missing from the stored row. + const live = createLiveJob({ id: "live-job" }); + const clobbered = { ...live, input: { ...live.input, target_lngs: ["de", "es"] } }; + mockPayload.find + .mockResolvedValueOnce({ docs: [live] }) + .mockResolvedValueOnce({ docs: [clobbered] }) + .mockResolvedValueOnce({ + docs: [{ ...live, input: { ...live.input, target_lngs: ["de", "es", "fr"] } }], + }); + + await runner.enqueue([createInput({ targetLng: "fr" })]); + + expect(mockPayload.db.updateOne).toHaveBeenCalledTimes(2); + expect(mockPayload.jobs.queue).not.toHaveBeenCalled(); + }); + + it("gives the locale its own job when even the retry does not land it", async () => { + const live = createLiveJob({ id: "live-job" }); + const withoutIt = { ...live, input: { ...live.input, target_lngs: ["de", "es"] } }; + mockPayload.find + .mockResolvedValueOnce({ docs: [live] }) + .mockResolvedValueOnce({ docs: [withoutIt] }) + .mockResolvedValueOnce({ docs: [withoutIt] }); + + await runner.enqueue([createInput({ targetLng: "fr" })]); + + expect(mockPayload.jobs.queue).toHaveBeenCalledWith( + expect.objectContaining({ + input: expect.objectContaining({ target_lngs: ["fr"] }), + }) + ); + }); + + it("pushes a not-yet-started job's debounce out to this request's", async () => { + const live = createLiveJob({ id: "live-job" }); + mockPayload.find.mockResolvedValue({ docs: [live] }); + const waitUntil = new Date("2026-02-02T00:00:00.000Z"); + + await runner.enqueue([createInput({ targetLng: "de", waitUntil })]); + + expect(mockPayload.db.updateOne.mock.calls[0][0].data).toMatchObject({ + waitUntil: "2026-02-02T00:00:00.000Z", }); - mockPayload.find.mockResolvedValueOnce({ docs: [pending, running] }); + }); - await runner.enqueue([createInput()]); + it("leaves a running job's schedule alone", async () => { + const running = createLiveJob({ id: "live-job", processing: true }); + mockPayload.find.mockResolvedValue({ docs: [running] }); - expect(mockPayload.jobs.cancel).toHaveBeenCalledWith({ - where: { id: { in: ["pending-job"] } }, - queue: "translations", + await runner.enqueue([ + createInput({ targetLng: "de", waitUntil: new Date("2026-02-02T00:00:00.000Z") }), + ]); + + expect(mockPayload.db.updateOne).not.toHaveBeenCalled(); + }); + + it("queues alongside a running job when the host enabled concurrency control", async () => { + mockPayload.config.jobs = { enableConcurrencyControl: true }; + mockPayload.find.mockResolvedValue({ + docs: [ + createLiveJob({ + id: "running-job", + processing: true, + }), + ], }); + + await runner.enqueue([createInput({ targetLng: "fr" })]); + + expect(mockPayload.db.updateOne).not.toHaveBeenCalled(); + expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(1); }); - it("looks for existing jobs once per document", async () => { + it("reads the job table once for the whole batch, however many documents it spans", async () => { await runner.enqueue([ createInput({ collectionSlug: "posts" as CollectionSlug, collectionId: "post-1" }), createInput({ collectionSlug: "posts" as CollectionSlug, collectionId: "post-2" }), createInput({ collectionSlug: "pages" as CollectionSlug, collectionId: "page-1" }), ]); - expect(mockPayload.find).toHaveBeenCalledTimes(3); + expect(mockPayload.find).toHaveBeenCalledTimes(1); + expect(mockPayload.jobs.queue).toHaveBeenCalledTimes(3); }); it("stores the reference as flat text, coercing the id to a string", async () => { @@ -251,7 +368,29 @@ describe("PayloadJobsTaskRunner", () => { }); expect(mockPayload.delete).toHaveBeenCalledWith({ collection: "payload-jobs", - where: { id: { in: ["job-1", "job-2"] } }, + where: { + and: [ + { + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], + }, + { id: { in: ["job-1", "job-2"] } }, + ], + }, + }); + }); + + it("deletes only this plugin's jobs, whatever ids it is handed", async () => { + await runner.cancel(["someone-elses-job"]); + + const where = mockPayload.delete.mock.calls[0][0].where as { and?: unknown[] }; + expect(where.and?.[0]).toEqual({ + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], }); }); @@ -285,6 +424,58 @@ describe("PayloadJobsTaskRunner", () => { expect(result).toEqual({ success: false, error: "already_completed" }); }); + it("retries a workflow whose locales are logged but whose run failed", async () => { + const partiallyFailed = createJob({ + input: { + collection_slug: "posts", + collection_id: "doc-123", + source_lng: "en", + target_lngs: ["de", "fr"], + strategy: "overwrite", + }, + log: [ + { state: "succeeded", completedAt: "2024-01-01T00:01:00Z", input: { target_lng: "de" } }, + { state: "failed", completedAt: "2024-01-01T00:02:00Z", input: { target_lng: "fr" } }, + ], + }); + mockPayload.find.mockResolvedValue({ docs: [partiallyFailed] }); + + const result = await runner.run("job-123"); + + expect(result).toEqual({ success: true }); + expect(mockPayload.jobs.run).toHaveBeenCalledWith({ + queue: "translations", + where: { id: { equals: "job-123" } }, + limit: 1, + }); + }); + + it("reports failure when the picker took nothing", async () => { + mockPayload.find.mockResolvedValue({ docs: [createJob()] }); + mockPayload.jobs.run.mockResolvedValue({ jobStatus: {}, remainingJobsFromQueried: 0 }); + + expect(await runner.run("job-123")).toEqual({ + success: false, + error: "already_running", + }); + }); + + it("clears what blocks the picker before retrying a failed job", async () => { + mockPayload.find.mockResolvedValue({ + docs: [createJob({ error: { message: "provider down" } })], + }); + + await runner.run("job-123"); + + expect(mockPayload.update).toHaveBeenCalledWith( + expect.objectContaining({ + collection: "payload-jobs", + where: { id: { equals: "job-123" } }, + data: { processing: false, hasError: false, error: null, waitUntil: null }, + }) + ); + }); + it("returns already_running when a job is genuinely in flight (fresh lock)", async () => { const runningJob = createJob({ processing: true, @@ -316,7 +507,7 @@ describe("PayloadJobsTaskRunner", () => { collection: "payload-jobs", depth: 0, where: { id: { equals: "job-123" } }, - data: { processing: false }, + data: { processing: false, hasError: false, error: null, waitUntil: null }, }); // run via the where-based picker, NOT runByID expect(mockPayload.jobs.run).toHaveBeenCalledWith({ @@ -346,7 +537,7 @@ describe("PayloadJobsTaskRunner", () => { collection: "payload-jobs", depth: 0, where: { id: { equals: "job-123" } }, - data: { processing: false }, + data: { processing: false, hasError: false, error: null, waitUntil: null }, }); }); @@ -364,7 +555,6 @@ describe("PayloadJobsTaskRunner", () => { }); // a pending job (processing:false) needs no lock reset expect(mockPayload.update).not.toHaveBeenCalled(); - // findJobsInternal must narrow by the job's own slugs AND the given id expect(mockPayload.find).toHaveBeenCalledWith( expect.objectContaining({ where: { @@ -411,7 +601,7 @@ describe("PayloadJobsTaskRunner", () => { vi.useRealTimers(); }); - it("resets stale processing locks via a real-column where clause", async () => { + it("narrows the stale-lock reset to both stored slugs", async () => { vi.useFakeTimers(); vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); @@ -433,7 +623,12 @@ describe("PayloadJobsTaskRunner", () => { ).toISOString(); expect(arg.where).toEqual({ and: [ - { taskSlug: { equals: "translate_document" } }, + { + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, + ], + }, { processing: { equals: true } }, { completedAt: { exists: false } }, { updatedAt: { less_than: expectedCutoff } }, @@ -510,11 +705,8 @@ describe("PayloadJobsTaskRunner", () => { }); it("narrows the SQL where clause by the job's own slugs only", async () => { - // Slug and id are matched in memory (see PayloadJobsTaskRunner.findByCollection - // for the full reasoning), spanning both the new flat-text shape and the - // legacy relationship shape. The WHERE sent to Payload must narrow only by - // taskSlug — never by the collection slug or id — otherwise we both - // re-introduce the SQLite type-coercion bug and drop one of the two shapes. + // Narrowing by the collection slug or id would re-introduce the SQLite coercion bug and drop + // the legacy shape — see `findByCollection`'s docblock. await runner.findByCollection("posts" as CollectionSlug, [5, 6]); const whereArg = mockPayload.find.mock.calls[0][0].where; diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts index 30ef6cd17..b90381092 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts @@ -4,9 +4,49 @@ import type { TaskFilter, TaskRunner } from "../TaskRunner.interface"; import { toTaskFilter } from "../toTaskFilter"; import type { Task, TaskInput, RunResult } from "../types"; import type { PayloadJobsRunnerConfig, PayloadJob } from "./types"; -import { normalizeJobLocales } from "./normalizeJob"; +import { normalizeJob, normalizeJobLocales } from "./normalizeJob"; +import { planEnqueue } from "./planEnqueue"; +import type { RequestShape } from "./planEnqueue"; +import { readCollectionRef } from "./readCollectionRef"; + +type StoredWorkflowInput = { + collection_slug: CollectionSlug; + collection_id: string; + source_lng: string; + target_lngs: string[]; + strategy: string; + publish_on_translation: boolean; +}; + +/** Ids are stringified here: the stored `collection_id` is text, so a number id must coerce to match. */ +function requestShape(task: TaskInput): RequestShape { + return { + collectionSlug: task.collectionSlug, + collectionId: String(task.collectionId), + sourceLng: task.sourceLng, + strategy: task.strategy, + publishOnTranslation: task.publishOnTranslation, + }; +} + +function requestKey(task: TaskInput): string { + const r = requestShape(task); + // NUL separates: no slug, id, locale or strategy can contain it, so no two different requests can + // collide on one key. + return [ + r.collectionSlug, + r.collectionId, + r.sourceLng, + r.strategy, + String(r.publishOnTranslation), + ].join("\u0000"); +} + +function sameDocument(job: PayloadJob, request: RequestShape): boolean { + const { collectionSlug, collectionId } = readCollectionRef(job.input); + return collectionSlug === request.collectionSlug && collectionId === request.collectionId; +} -/** {@link TaskRunner} backed by Payload's job queue (`payload-jobs`). */ export class PayloadJobsTaskRunner implements TaskRunner { constructor( private readonly payload: Payload, @@ -14,50 +54,110 @@ export class PayloadJobsTaskRunner implements TaskRunner { ) {} async enqueue(tasks: TaskInput[]): Promise { - // One workflow per document, carrying its locales, rather than one job per locale. Payload runs a - // batch of jobs through `Promise.all`, and every write it makes is a whole-document version - // snapshot — so two locales translated in parallel build their snapshots from the same base and - // the second silently drops the first's work. Measured at one translation of two landing, on all - // three adapters. See issue #114. - const byDocument = new Map(); + // Keyed by everything a job carries one of, not by document alone — see `pickHost`. + const byRequest = new Map(); for (const task of tasks) { - const key = `${task.collectionSlug}:${task.collectionId}`; - byDocument.set(key, [...(byDocument.get(key) ?? []), task]); + const key = requestKey(task); + const group = byRequest.get(key) ?? []; + group.push(task); + byRequest.set(key, group); } - for (const group of byDocument.values()) { - const [first] = group; - const existing = await this.findByCollection(first.collectionSlug, { - documentIds: [first.collectionId], - excludeCompleted: true, - }); - // Only work that has not begun. A workflow already running keeps the locales it has finished - // and completes; the new request queues behind it. Cancelling it would discard translations - // that already landed — the very loss this change exists to stop. - const toCancel = existing.filter( - (t) => t.input.collectionId === String(first.collectionId) && t.status === "pending" - ); - if (toCancel.length > 0) { - await this.cancelAndDeleteJobs(toCancel.map((t) => t.id)); - } - - await this.payload.jobs.queue({ - workflow: this.config.workflowName as never, - queue: this.config.queueName, - // Debounce: Payload holds the job until this instant, so rapid source edits coalesce. - waitUntil: first.waitUntil, - input: { - collection_slug: first.collectionSlug, - // The one place an id is normalized for storage; the stored shape is text so a job stays - // ID-agnostic. See docs/DEPRECATIONS.md#jobs-input-collection-field - collection_id: String(first.collectionId), - source_lng: first.sourceLng, - target_lngs: group.map((t) => t.targetLng), - strategy: first.strategy, - publish_on_translation: first.publishOnTranslation, - } as never, + // `findRawJobs` is unpaginated and filters in memory, so narrowing per document would re-read the + // whole table once per document. + const live = await this.findRawJobs({ completedAt: { exists: false } }, { pagination: false }); + const exclusiveQueue = Boolean(this.payload.config.jobs?.enableConcurrencyControl); + + // At most one group can match any live job — `pickHost` requires the same source locale, strategy + // and publish flag — so no two of these writes touch the same row. + await Promise.all( + [...byRequest.values()].map((group) => this.serve(group, live, exclusiveQueue)) + ); + } + + private async serve( + group: TaskInput[], + live: PayloadJob[], + exclusiveQueue: boolean + ): Promise { + const [first] = group; + const request = requestShape(first); + const plan = planEnqueue({ + live: live.filter((job) => sameDocument(job, request)), + request, + requested: group.map((t) => t.targetLng), + exclusiveQueue, + }); + + const undelivered = plan.host + ? await this.extendJob(plan.host, plan.append, first.waitUntil) + : []; + const queue = [...plan.queue, ...undelivered]; + if (queue.length > 0) await this.queueWorkflow(request, queue, first.waitUntil); + } + + /** @returns the locales that did not reach the job and need one of their own. */ + private async extendJob(job: PayloadJob, locales: string[], waitUntil?: Date): Promise { + let current: PayloadJob | undefined = job; + // Two attempts: `input` is one JSON column, so a concurrent append replaces the whole list and + // ours can be lost. Adding a locale is a set union, which makes retrying from the stored row + // harmless. Anything still missing after that gets a job of its own rather than being dropped. + for (let attempt = 0; attempt < 2; attempt++) { + if (!current || current.completedAt) return locales; + + const listed = current.input?.target_lngs ?? []; + const missing = locales.filter((locale) => !listed.includes(locale)); + // Keep the debounce: a job that has not started is still coalescing rapid source edits, and + // this request is the latest of them. A running job's schedule is not ours to move. + const debounce = waitUntil && !current.processing ? waitUntil.toISOString() : undefined; + if (missing.length === 0 && !debounce) return []; + + const data: Record = { + input: { ...current.input, target_lngs: [...listed, ...missing] }, + ...(debounce ? { waitUntil: debounce } : {}), + }; + + // Adapter write, not `payload.update`: the document operation re-reads and rewrites the whole + // row, reverting log entries written in between. Measured — see D2 of + // docs/plans/2026-09-08-one-live-job-per-document.task.md. + await this.payload.db.updateOne({ + collection: this.config.jobsCollection, + id: job.id, + data, + returning: false, }); + + [current] = await this.findRawJobs({ id: { equals: job.id } }, { limit: 1 }); + // Finished between the plan's read and this write: the locales are stored but nobody will run + // them, so they need a job of their own. + if (!current || current.completedAt) return locales; + const stored = new Set(current.input?.target_lngs); + if (locales.every((locale) => stored.has(locale))) return []; } + const stored = new Set(current?.input?.target_lngs); + return locales.filter((locale) => !stored.has(locale)); + } + + private async queueWorkflow( + request: RequestShape, + targetLngs: string[], + waitUntil?: Date + ): Promise { + const input: StoredWorkflowInput = { + collection_slug: request.collectionSlug as CollectionSlug, + collection_id: request.collectionId, + source_lng: request.sourceLng, + target_lngs: targetLngs, + strategy: request.strategy, + publish_on_translation: request.publishOnTranslation, + }; + + await this.payload.jobs.queue({ + workflow: this.config.workflowName as never, + queue: this.config.queueName, + waitUntil, + input: input as never, + }); } async cancel(taskIds: string[]): Promise { @@ -66,32 +166,38 @@ export class PayloadJobsTaskRunner implements TaskRunner { } async run(taskId: string): Promise { - const tasks = await this.findJobsInternal({ id: { equals: taskId } }, { limit: 1 }); - const task = tasks[0]; - - if (!task) { + const [job] = await this.findRawJobs({ id: { equals: taskId } }, { limit: 1 }); + if (!job) { return { success: false, error: "not_found" }; } + // The job's own state, not a locale row's: a locale row carries the log entry's `completedAt`, + // which Payload stamps on failures too. + const task = normalizeJob(job); if (task.completedAt) { return { success: false, error: "already_completed" }; } - if (task.status === "running") { - // The picker below selects only `processing: false`, so a stale lock must be cleared first. - if (!this.isStale(task.updatedAt)) { - return { success: false, error: "already_running" }; - } - await this.resetProcessing({ id: { equals: taskId } }); + if (task.status === "running" && !this.isStale(task.updatedAt)) { + return { success: false, error: "already_running" }; + } + if (task.status === "running" || task.status === "failed") { + await this.clearPickerBlockers(taskId); } // `where` picker, not `payload.jobs.runByID({ id })`: in `runJobs` the guard block // (processing:false, hasError not true, waitUntil due) is built only for the non-id branch, so // the id path would re-run a job that already exhausted its retries. Checked against payload // 3.84.1. - await this.payload.jobs.run({ + const result = (await this.payload.jobs.run({ queue: this.config.queueName, where: { id: { equals: taskId } }, limit: 1, - }); + })) as { jobStatus?: Record }; + + // An empty `jobStatus` is Payload reporting that the picker took nothing — usually a document + // already running under the host's concurrency control. + if (Object.keys(result?.jobStatus ?? {}).length === 0) { + return { success: false, error: "already_running" }; + } return { success: true }; } @@ -106,7 +212,7 @@ export class PayloadJobsTaskRunner implements TaskRunner { const cutoff = new Date(Date.now() - this.config.staleJobTimeoutMs).toISOString(); return this.resetProcessing({ and: [ - { taskSlug: { equals: this.config.taskName } }, + this.ownJobs(), { processing: { equals: true } }, { completedAt: { exists: false } }, { updatedAt: { less_than: cutoff } }, @@ -114,6 +220,22 @@ export class PayloadJobsTaskRunner implements TaskRunner { }); } + /** + * Clears the processing lock, a spent retry budget and a pending backoff — the three things that + * make the picker skip a job, so a manual run really is the retry. + * + * Goes through `payload.update` rather than the adapter, unlike `extendJob`, so the jobs + * collection's own `beforeChange` hook still runs — it is what keeps a cancelled job cancelled. + */ + private async clearPickerBlockers(taskId: string): Promise { + await this.payload.update({ + collection: this.config.jobsCollection, + depth: 0, + where: { id: { equals: taskId } }, + data: { processing: false, hasError: false, error: null, waitUntil: null }, + }); + } + /** Clears the `processing` lock on every job matching `where`. `depth: 0` — only the count is read. */ private async resetProcessing(where: Where): Promise { const result = await this.payload.update({ @@ -136,7 +258,8 @@ export class PayloadJobsTaskRunner implements TaskRunner { /** * Find translation jobs for a collection. * - * Only `taskSlug` and `completedAt` reach the database; slug and document ids are matched in memory + * Only `workflowSlug`/`taskSlug` and `completedAt` reach the database; the collection slug and + * document ids are matched in memory * because a job's collection reference may sit in either the flat-text fields or the legacy * relationship shape (`readCollectionRef`), so a `where` on `input.collection_slug` would silently * drop every pre-migration job. `excludeCompleted` is what bounds the read — see issue #108. @@ -147,23 +270,14 @@ export class PayloadJobsTaskRunner implements TaskRunner { ): Promise { const { documentIds, excludeCompleted } = toTaskFilter(filter); const where = excludeCompleted ? { completedAt: { exists: false } } : undefined; - const all = await this.findJobsInternal(where, { pagination: false }); + const jobs = await this.findRawJobs(where, { pagination: false }); + const all = jobs.flatMap(normalizeJobLocales); const bySlug = all.filter((t) => t.input.collectionSlug === collectionSlug); if (!documentIds?.length) return bySlug; const wanted = new Set(documentIds.map(String)); return bySlug.filter((t) => wanted.has(t.input.collectionId)); } - private groupByCollection(tasks: TaskInput[]): Map { - const map = new Map(); - for (const task of tasks) { - const existing = map.get(task.collectionSlug) ?? []; - existing.push(task); - map.set(task.collectionSlug, existing); - } - return map; - } - private async cancelAndDeleteJobs(taskIds: string[]): Promise { if (taskIds.length === 0) return; @@ -176,36 +290,41 @@ export class PayloadJobsTaskRunner implements TaskRunner { queue: this.config.queueName, }); + // Narrowed to our own jobs: the ids arrive straight from the request body, and `payload-jobs` is + // shared with whatever else the host queues there. await this.payload.delete({ collection: this.config.jobsCollection, - where: { id: { in: taskIds } }, + where: { and: [this.ownJobs(), { id: { in: taskIds } }] }, }); } - private async findJobsInternal( + private ownJobs(): Where { + // Jobs queued before the workflow change are still in the table and carry the per-locale task slug. + return { + or: [ + { workflowSlug: { equals: this.config.workflowName } }, + { taskSlug: { equals: this.config.taskName } }, + ], + }; + } + + private async findRawJobs( where?: Where, params?: { limit?: number; pagination?: boolean } - ): Promise { - // Both shapes: a document's work is a workflow now, but jobs queued before that change — and - // still sitting in the table — carry the per-locale task slug. Same expand/contract as - // `readCollectionRef` does for the collection reference. - const and: Where[] = [ - { - or: [ - { workflowSlug: { equals: this.config.workflowName } }, - { taskSlug: { equals: this.config.taskName } }, - ], - }, - ]; + ): Promise { + const and: Where[] = [this.ownJobs()]; if (where) and.push(where); const response = await this.payload.find({ collection: this.config.jobsCollection, + // The legacy `input.collection` is a declared relationship; at the default depth Payload + // populates it, and `readCollectionRef` would then read a document where it wants an id. + depth: 0, limit: params?.limit, pagination: params?.pagination, where: { and }, }); - return (response.docs as PayloadJob[]).flatMap(normalizeJobLocales); + return response.docs as PayloadJob[]; } } diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.test.ts index 76773258a..64a711204 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from "vitest"; import type { CollectionSlug } from "payload"; -import { normalizeJob } from "./normalizeJob"; +import { normalizeJob, normalizeJobLocales } from "./normalizeJob"; import type { PayloadJob } from "./types"; describe("normalizeJob", () => { @@ -265,3 +265,93 @@ describe("normalizeJob", () => { }); }); }); + +describe("normalizeJobLocales", () => { + const workflowJob: PayloadJob = { + id: "job-999", + createdAt: "2024-01-01T00:00:00Z", + updatedAt: "2024-01-01T00:05:00Z", + input: { + collection_slug: "posts", + collection_id: "doc-456", + source_lng: "en", + target_lngs: ["de", "fr", "es"], + strategy: "overwrite", + }, + }; + + it("expands a pre-workflow job to itself", () => { + const legacy: PayloadJob = { + id: "job-1", + createdAt: "2024-01-01T00:00:00Z", + updatedAt: "2024-01-01T00:00:00Z", + input: { collection_slug: "posts", collection_id: "doc-1", target_lng: "de" }, + }; + const rows = normalizeJobLocales(legacy); + expect(rows).toEqual([normalizeJob(legacy)]); + }); + + it("gives every requested locale a row, in the requested order", () => { + const rows = normalizeJobLocales(workflowJob); + expect(rows.map((r) => r.input.targetLng)).toEqual(["de", "fr", "es"]); + }); + + it("reports each locale's own outcome from the job log, not the job's status", () => { + const rows = normalizeJobLocales({ + ...workflowJob, + processing: true, + log: [ + { state: "succeeded", completedAt: "2024-01-01T00:01:00Z", input: { target_lng: "de" } }, + { state: "failed", completedAt: "2024-01-01T00:02:00Z", input: { target_lng: "fr" } }, + ], + }); + expect(rows.map((r) => [r.input.targetLng, r.status])).toEqual([ + ["de", "completed"], + ["fr", "failed"], + ["es", "running"], + ]); + }); + + it("stamps completedAt only on a locale that succeeded", () => { + const rows = normalizeJobLocales({ + ...workflowJob, + log: [ + { state: "succeeded", completedAt: "2024-01-01T00:01:00Z", input: { target_lng: "de" } }, + { state: "failed", completedAt: "2024-01-01T00:02:00Z", input: { target_lng: "fr" } }, + ], + }); + expect(rows[0].completedAt).toBe("2024-01-01T00:01:00Z"); + expect(rows[1].completedAt).toBeUndefined(); + }); + + it("keeps the failure on the locale that failed, off the ones that landed", () => { + const rows = normalizeJobLocales({ + ...workflowJob, + error: { message: "provider refused fr" }, + log: [ + { state: "succeeded", completedAt: "2024-01-01T00:01:00Z", input: { target_lng: "de" } }, + { state: "failed", completedAt: "2024-01-01T00:02:00Z", input: { target_lng: "fr" } }, + ], + }); + expect(rows[0].error).toBeUndefined(); + expect(rows[1].error).toEqual({ message: "provider refused fr" }); + }); + + it("takes a retried locale's most recent log entry", () => { + const rows = normalizeJobLocales({ + ...workflowJob, + input: { ...workflowJob.input, target_lngs: ["de"] }, + log: [ + { state: "failed", completedAt: "2024-01-01T00:01:00Z", input: { target_lng: "de" } }, + { state: "succeeded", completedAt: "2024-01-01T00:03:00Z", input: { target_lng: "de" } }, + ], + }); + expect(rows[0].status).toBe("completed"); + expect(rows[0].completedAt).toBe("2024-01-01T00:03:00Z"); + }); + + it("keeps the real job id on every row, because cancelling one cancels the job", () => { + const rows = normalizeJobLocales(workflowJob); + expect(rows.map((r) => r.id)).toEqual(["job-999", "job-999", "job-999"]); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts index 92fda6995..58c9fff1e 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts @@ -2,31 +2,6 @@ import type { Task, TaskStatus } from "../types"; import type { JobLogEntry, PayloadJob } from "./types"; import { readCollectionRef } from "./readCollectionRef"; -/** - * Transform Payload job to normalized Task - */ -export function normalizeJob(job: PayloadJob): Task { - const { collectionSlug, collectionId } = readCollectionRef(job.input); - - return { - id: job.id, - status: getJobStatus(job), - input: { - collectionSlug, - collectionId, - sourceLng: job.input?.source_lng ?? "", - targetLng: job.input?.target_lng ?? "", - strategy: (job.input?.strategy as "overwrite" | "skip_existing") ?? "overwrite", - publishOnTranslation: job.input?.publish_on_translation ?? false, - }, - createdAt: job.createdAt, - updatedAt: job.updatedAt, - completedAt: job.completedAt ?? undefined, - error: job.error ? { message: extractErrorMessage(job.error) } : undefined, - cancelled: isCancelled(job.error), - }; -} - function getJobStatus(job: PayloadJob): TaskStatus { if (job.completedAt) return "completed"; if (job.processing) return "running"; @@ -46,7 +21,7 @@ function extractErrorMessage(error: unknown): string { return "Unknown error"; } -function isCancelled(error: unknown): boolean { +export function isCancelled(error: unknown): boolean { return ( error !== null && typeof error === "object" && @@ -57,41 +32,65 @@ function isCancelled(error: unknown): boolean { } /** - * Expand a stored job into one {@link Task} per target locale. - * - * A document's locales are one workflow job now, but the status panels are per-locale — so the - * per-locale rows are rebuilt from the job's `log`, which Payload writes an entry into as each - * locale's task finishes. A locale with no entry yet has not run: it reports the job's own state. - * - * A job in the pre-workflow shape carries a single `target_lng` and expands to itself, so jobs - * queued before the change stay readable. See docs/DEPRECATIONS.md#jobs-input-collection-field for - * the same expand/contract elsewhere in this file's neighbourhood. + * Transform Payload job to normalized Task + */ +export function normalizeJob(job: PayloadJob): Task { + const { collectionSlug, collectionId } = readCollectionRef(job.input); + + return { + id: job.id, + status: getJobStatus(job), + input: { + collectionSlug, + collectionId, + sourceLng: job.input?.source_lng ?? "", + targetLng: job.input?.target_lng ?? "", + strategy: (job.input?.strategy as "overwrite" | "skip_existing") ?? "overwrite", + publishOnTranslation: job.input?.publish_on_translation ?? false, + }, + createdAt: job.createdAt, + updatedAt: job.updatedAt, + completedAt: job.completedAt ?? undefined, + error: job.error ? { message: extractErrorMessage(job.error) } : undefined, + cancelled: isCancelled(job.error), + }; +} + +/** + * One {@link Task} per target locale, its state read from {@link latestLogByLocale}. A pre-workflow + * job carries a single `target_lng` and expands to itself. */ export function normalizeJobLocales(job: PayloadJob): Task[] { const targets = job.input?.target_lngs; if (!Array.isArray(targets) || targets.length === 0) return [normalizeJob(job)]; const base = normalizeJob(job); - const byLocale = new Map(); - for (const entry of job.log ?? []) { - const lng = entry?.input?.target_lng; - if (typeof lng === "string") byLocale.set(lng, entry); - } + const latestByLocale = latestLogByLocale(job); return targets.map((targetLng) => { - const entry = byLocale.get(targetLng); + const entry = latestByLocale.get(targetLng); + if (!entry) return { ...base, input: { ...base.input, targetLng } }; + const succeeded = entry.state === "succeeded"; return { - // The real job id, repeated across the locales: they are rows of one job, and cancelling any of - // them cancels that job. A synthetic per-locale id would be handed straight to `cancel()`, - // which addresses jobs. ...base, - status: entry ? logStateToStatus(entry.state) : base.status, - completedAt: entry?.completedAt ?? undefined, + status: succeeded ? "completed" : "failed", + completedAt: succeeded ? (entry.completedAt ?? undefined) : undefined, + error: succeeded ? undefined : base.error, + cancelled: succeeded ? false : base.cancelled, input: { ...base.input, targetLng }, }; }); } -function logStateToStatus(state: JobLogEntry["state"]): TaskStatus { - return state === "succeeded" ? "completed" : "failed"; +/** + * Each target locale's most recent log entry: Payload appends to `log` chronologically, so + * last-write-wins leaves the latest attempt. An absent entry means that locale has not run. + */ +export function latestLogByLocale(job: PayloadJob): Map { + const byLocale = new Map(); + for (const entry of job.log ?? []) { + const lng = entry?.input?.target_lng; + if (typeof lng === "string") byLocale.set(lng, entry); + } + return byLocale; } diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts new file mode 100644 index 000000000..5f6353fc1 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, it } from "vitest"; + +import { planEnqueue } from "./planEnqueue"; +import type { RequestShape } from "./planEnqueue"; +import type { PayloadJob } from "./types"; + +const request: RequestShape = { + collectionSlug: "posts", + collectionId: "doc-1", + sourceLng: "en", + strategy: "overwrite", + publishOnTranslation: false, +}; + +const storedInput = { + collection_slug: "posts", + collection_id: "doc-1", + source_lng: "en", + strategy: "overwrite", + publish_on_translation: false, + target_lngs: ["de"], +}; + +const job = (overrides: Partial = {}): PayloadJob => ({ + id: "job-1", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + input: storedInput, + ...overrides, +}); + +const plan = (live: PayloadJob[], requested: string[], exclusiveQueue = false) => + planEnqueue({ live, request, requested, exclusiveQueue }); + +describe("planEnqueue", () => { + it("queues everything when the document has no live job", () => { + expect(plan([], ["de", "fr"])).toEqual({ host: null, append: [], queue: ["de", "fr"] }); + }); + + it("adds locales the live job does not carry yet", () => { + const live = job(); + expect(plan([live], ["fr", "es"])).toEqual({ host: live, append: ["fr", "es"], queue: [] }); + }); + + it("does nothing for a locale the live job already owes", () => { + const live = job(); + expect(plan([live], ["de"])).toEqual({ host: live, append: [], queue: [] }); + }); + + it("gives a locale the live job has already translated a job of its own", () => { + const live = job({ + log: [{ state: "succeeded", input: { target_lng: "de" } }], + }); + expect(plan([live], ["de"])).toEqual({ host: live, append: [], queue: ["de"] }); + }); + + it("splits a request across both when it mixes the two", () => { + const live = job({ log: [{ state: "succeeded", input: { target_lng: "de" } }] }); + expect(plan([live], ["de", "fr"])).toEqual({ host: live, append: ["fr"], queue: ["de"] }); + }); + + it("ignores duplicates in the request", () => { + const live = job(); + expect(plan([live], ["fr", "fr"])).toEqual({ host: live, append: ["fr"], queue: [] }); + }); + + it.each([ + ["oldest first", ["old", "new"]], + ["newest first", ["new", "old"]], + ])("extends the newest live job, %s", (_label, order) => { + // Both orders, because "take the last element" and "sort the other way" each pass on one of them. + const byId = { + old: job({ id: "old", createdAt: "2026-01-01T00:00:00Z" }), + new: job({ id: "new", createdAt: "2026-01-02T00:00:00Z" }), + }; + expect( + plan( + order.map((id) => byId[id as "old" | "new"]), + ["fr"] + ).host?.id + ).toBe("new"); + }); + + it("picks the newest job that is usable, not the newest job", () => { + const newerButCancelled = job({ + id: "cancelled", + createdAt: "2026-01-03T00:00:00Z", + error: { cancelled: true }, + }); + const usable = job({ id: "usable", createdAt: "2026-01-02T00:00:00Z" }); + expect(plan([newerButCancelled, usable], ["fr"]).host?.id).toBe("usable"); + }); + + it("does not extend a job in the pre-workflow shape", () => { + const legacy = job({ + input: { ...storedInput, target_lngs: undefined, target_lng: "de" }, + }); + expect(plan([legacy], ["fr"])).toEqual({ host: null, append: [], queue: ["fr"] }); + }); + + it("does not extend a cancelled job", () => { + const cancelled = job({ error: { cancelled: true } }); + expect(plan([cancelled], ["fr"])).toEqual({ host: null, append: [], queue: ["fr"] }); + }); + + describe("when the host enabled Payload's concurrency control", () => { + it("queues alongside a running job instead of extending it", () => { + const running = job({ processing: true }); + expect(plan([running], ["fr"], true)).toEqual({ host: null, append: [], queue: ["fr"] }); + }); + + it("merges an already-translated locale into the alongside job too", () => { + const running = job({ + processing: true, + log: [{ state: "succeeded", input: { target_lng: "de" } }], + }); + expect(plan([running], ["de", "fr"], true)).toEqual({ + host: null, + append: [], + queue: ["fr", "de"], + }); + }); + + it("still extends a job that has not started", () => { + const pending = job({ processing: false }); + expect(plan([pending], ["fr"], true)).toEqual({ host: pending, append: ["fr"], queue: [] }); + }); + }); + + it("extends a running job when the host has NOT enabled concurrency control", () => { + const running = job({ processing: true }); + expect(plan([running], ["fr"])).toEqual({ host: running, append: ["fr"], queue: [] }); + }); + + describe("a job can only take locales from a request it matches", () => { + it.each([ + ["a different strategy", { strategy: "skip_existing" }], + ["a different source locale", { source_lng: "fr" }], + ["a different publish flag", { publish_on_translation: true }], + ])("starts its own job for %s", (_label, storedOverride) => { + const live = job({ input: { ...storedInput, ...storedOverride } }); + expect(plan([live], ["fr"])).toEqual({ host: null, append: [], queue: ["fr"] }); + }); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts new file mode 100644 index 000000000..5788098e2 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts @@ -0,0 +1,71 @@ +import { isCancelled, latestLogByLocale } from "./normalizeJob"; +import type { PayloadJob } from "./types"; + +/** The part of a request that every locale in it shares. */ +export type RequestShape = { + collectionSlug: string; + collectionId: string; + sourceLng: string; + strategy: string; + publishOnTranslation: boolean; +}; + +/** + * `append` and `queue` are independent: a request can both extend a live job and need a job of its + * own, when a locale it asks for has already been translated by that job. + */ +export type EnqueuePlan = { + host: PayloadJob | null; + append: string[]; + queue: string[]; +}; + +/** + * One live job per document: a later request extends that job's locale list rather than replacing + * it, because replacing drops whatever locales it still owed. + * + * @param live - non-completed jobs for **this document only**; the caller filters by document. + * @param requested - target locales in request order; duplicates ignored. + * @param exclusiveQueue - the host's `enableConcurrencyControl`; with it on a running job is queued + * behind rather than extended. + */ +export function planEnqueue(args: { + live: PayloadJob[]; + request: RequestShape; + requested: string[]; + exclusiveQueue: boolean; +}): EnqueuePlan { + const requested = [...new Set(args.requested)]; + const host = pickHost(args.live, args.request); + if (!host) return { host: null, append: [], queue: requested }; + + // Already-succeeded locales need a fresh job: the workflow passes the locale as the task id, so + // Payload's restoration would skip them (see the workflow handler in PayloadJobsRunnerProvider). + const settled = latestLogByLocale(host); + const done = requested.filter((locale) => settled.get(locale)?.state === "succeeded"); + const listed = new Set(host.input?.target_lngs); + const missing = requested.filter((locale) => !listed.has(locale)); + + if (args.exclusiveQueue && host.processing) { + return { host: null, append: [], queue: [...missing, ...done] }; + } + return { host, append: missing, queue: done }; +} + +/** + * A job carries one source locale, one strategy and one publish flag for all of its locales, so it + * can only take locales from a request that chose the same three — otherwise the request would run + * under settings the user did not pick. Pre-workflow jobs (a single `target_lng`) and cancelled jobs + * are skipped outright. + */ +function pickHost(live: PayloadJob[], request: RequestShape): PayloadJob | null { + const usable = live.filter( + (job) => + Array.isArray(job.input?.target_lngs) && + !isCancelled(job.error) && + job.input?.source_lng === request.sourceLng && + job.input?.strategy === request.strategy && + (job.input?.publish_on_translation ?? false) === request.publishOnTranslation + ); + return usable.sort((a, b) => Date.parse(b.createdAt) - Date.parse(a.createdAt))[0] ?? null; +} diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts index d43a2a0cc..f3eee85c6 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts @@ -25,11 +25,6 @@ export type PayloadJobsRunnerOptions = { * @default 'translate_document' */ taskName?: string; - /** - * Name of the Payload workflow that walks a document's target locales. - * @default 'translate_document_locales' - */ - workflowName?: string; /** * Name of the job queue. * @default 'translations' @@ -77,6 +72,7 @@ export type PayloadJobsRunnerOptions = { */ export type PayloadJobsRunnerConfig = { taskName: string; + /** Derived from `taskName`, not configurable — see `createPayloadJobsRunner`. */ workflowName: string; queueName: string; jobsCollection: CollectionSlug; @@ -117,7 +113,7 @@ export type PayloadJob = { }; }; -/** One entry Payload writes to a job's `log` as each task inside a workflow settles. */ +/** Written by Payload only once a task settles — an absent entry means that locale has not run. */ export type JobLogEntry = { state: "succeeded" | "failed"; completedAt?: string | null; From 7d7e9879a32e03c0ed6e03fbcebec5316abf181e Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 8 Sep 2026 13:23:30 +0200 Subject: [PATCH 3/7] refactor(translator): type the workflow registration instead of casting it away `as never` on the workflow object and on the `jobs.queue` call silenced the whole shape, so a typo in a field name would have compiled. Payload's own `WorkflowConfig` and a signature naming what `jobs.queue` actually accepts cover both, and the concurrency key and the handler's `job.input` are now checked rather than opaque. Comments removed from PayloadJobsRunnerProvider. --- .../PayloadJobsRunnerProvider.ts | 83 ++++--------------- .../PayloadJobsTaskRunner.ts | 16 +++- 2 files changed, 30 insertions(+), 69 deletions(-) diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts index 11c988087..e6bb91038 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts @@ -1,4 +1,4 @@ -import type { Config, Field, Payload } from "payload"; +import type { Config, Field, Payload, WorkflowConfig } from "payload"; import type { TaskRunner } from "../TaskRunner.interface"; import type { PayloadJobsRunnerOptions, PayloadJobsRunnerConfig, AutoRunConfig } from "./types"; @@ -12,7 +12,14 @@ const defaultAutoRun: Required = { limit: 50, }; -const DEFAULT_STALE_JOB_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes +type StoredWorkflowInput = { + collection_slug?: string; + collection_id?: string; + target_lngs?: string[]; +} & Record; +type RunLocaleTask = (taskID: string, args: { input: Record }) => Promise; + +const DEFAULT_STALE_JOB_TIMEOUT_MS = 5 * 60 * 1000; const defaultValues = { taskName: "translate_document", @@ -24,16 +31,11 @@ const defaultValues = { attempts: 3, backoff: { type: "exponential" as const, - delay: 5000, // 5s, 10s, 20s + delay: 5000, }, }, }; -/** - * TaskRunnerProvider implementation using Payload Jobs. - * - * Configures Payload jobs, tasks, and autorun for translation processing. - */ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { private readonly config: PayloadJobsRunnerConfig; @@ -73,9 +75,6 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { return (config) => { const inputSchema: Field[] = [ - // Flat text reference (ID-agnostic). Current shape that jobs are - // written with — no relationship type validation against the target - // collection's ID type, so string IDs work for number-id collections. { type: "text", name: "collection_slug", @@ -86,14 +85,6 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { name: "collection_id", required: true, }, - /** - * Legacy relationship reference, kept as a read-only fallback so jobs - * queued before the ID-agnostic migration stay readable. No longer - * written; demoted to `required: false` so new jobs (which omit it) - * pass validation. Removed in the next major. - * See docs/DEPRECATIONS.md#jobs-input-collection-field - * @deprecated - */ { type: "relationship", name: "collection", @@ -131,7 +122,6 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { const workflowInputSchema: Field[] = [ ...inputSchema.filter((f) => "name" in f && f.name !== "target_lng"), - // json, not an array field: the input stays one column, so no migration is needed { type: "json", name: "target_lngs", required: true }, ]; @@ -144,7 +134,6 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { input: { collection_slug?: string; collection_id?: string; - // Legacy fallback shape, see docs/DEPRECATIONS.md#jobs-input-collection-field collection?: { relationTo: string; value: string | number }; source_lng: string; target_lng: string; @@ -165,37 +154,25 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { }, }; - // Locales run in sequence: every Payload write is a whole-document version snapshot, so two - // translated in parallel build from the same base and the second drops the first (issue #114). - const workflow = { + const workflow: WorkflowConfig = { slug: workflowName, inputSchema: workflowInputSchema, retries, - // Conditional because Payload refuses to boot when a workflow declares `concurrency` while - // `enableConcurrencyControl` is off. The flag is the host's to set — see the README. ...(config.jobs?.enableConcurrencyControl ? { concurrency: { - key: ({ input }: { input: { collection_slug?: string; collection_id?: string } }) => - `${input.collection_slug}:${input.collection_id}`, + key: ({ input }) => `${input.collection_slug}:${input.collection_id}`, exclusive: true, }, } : {}), - handler: async (args: { - job: { input: { target_lngs?: string[] } & Record }; - tasks: Record Promise>; - }) => { - // Re-read the list every turn instead of destructuring it once. Payload replaces - // `job.input` with a freshly-read row after each task settles, so a locale appended to the - // stored job while this one runs is picked up here rather than being lost. + handler: async ({ job, tasks }) => { + const runLocale = (tasks as Record)[taskName]; for (let i = 0; ; i++) { - const { target_lngs: targets, ...shared } = args.job.input; + const { target_lngs: targets, ...shared } = job.input; const target = targets?.[i]; if (target === undefined) return; - // The locale is the task id, so Payload's own restoration skips a locale already logged - // as succeeded when a failed workflow is retried. - await args.tasks[taskName](target, { input: { ...shared, target_lng: target } }); + await runLocale(target, { input: { ...shared, target_lng: target } }); } }, }; @@ -204,9 +181,8 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { if (!config.jobs.tasks) config.jobs.tasks = []; config.jobs.tasks.push(task); if (!config.jobs.workflows) config.jobs.workflows = []; - config.jobs.workflows.push(workflow as never); + config.jobs.workflows.push(workflow); - // Skip autoRun configuration when disabled (e.g., for Vercel/serverless deployments) if (autoRun) { const autoRunConfig = { queue: queueName, @@ -227,13 +203,6 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { } } - // Reset stale locks on boot so jobs abandoned by a killed process - // (deploy/crash/timeout) become eligible for the autorun picker again. - // The picker requires processing:false, no error, and no pending waitUntil; - // a mid-run casualty (no error, no waitUntil) satisfies the rest, so - // clearing processing is sufficient for that case. Threshold-based, so a - // job genuinely in flight on another live instance (fresh updatedAt) is - // left alone. Wrapped so a reclaim failure never blocks startup. const existingOnInit = config.onInit; config.onInit = async (payload) => { if (existingOnInit) await existingOnInit(payload); @@ -252,24 +221,6 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { } } -/** - * Creates the **recommended** task runner: translations run as Payload Jobs - * (queued, executed by autoRun cron or a manual run, with stale-lock recovery). - * Durable across restarts and suited to production/serverless. Pass the result - * as `translatorPlugin({ runner })`. - * - * @param options - Queue/task names, `autoRun` cron (or `false` to disable), - * `staleJobTimeoutMs`, and retry policy. See {@link PayloadJobsRunnerOptions}. - * @returns A {@link TaskRunnerProvider} for the plugin's `runner` option. - * @example - * ```ts - * translatorPlugin({ - * collections: [Posts], - * translationProvider: createOpenAIProvider({ apiKey: process.env.OPENAI_API_KEY! }), - * runner: createPayloadJobsRunner({ autoRun: { cron: '* * * * *' } }), - * }) - * ``` - */ export function createPayloadJobsRunner(options?: PayloadJobsRunnerOptions): TaskRunnerProvider { return new PayloadJobsRunnerProvider(options); } diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts index b90381092..030e11156 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts @@ -9,6 +9,13 @@ import { planEnqueue } from "./planEnqueue"; import type { RequestShape } from "./planEnqueue"; import { readCollectionRef } from "./readCollectionRef"; +type QueueWorkflow = (args: { + workflow: string; + queue: string; + waitUntil?: Date; + input: StoredWorkflowInput; +}) => Promise; + type StoredWorkflowInput = { collection_slug: CollectionSlug; collection_id: string; @@ -152,11 +159,14 @@ export class PayloadJobsTaskRunner implements TaskRunner { publish_on_translation: request.publishOnTranslation, }; - await this.payload.jobs.queue({ - workflow: this.config.workflowName as never, + // `jobs.queue` is generic over the host's generated job slugs; this workflow is registered at + // config time, so the call goes through a signature naming what it actually accepts. + const queueJob = this.payload.jobs.queue as unknown as QueueWorkflow; + await queueJob({ + workflow: this.config.workflowName, queue: this.config.queueName, waitUntil, - input: input as never, + input, }); } From 4f5145214b5eca9df2553907309a05b2ef9c6683 Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 8 Sep 2026 13:28:26 +0200 Subject: [PATCH 4/7] docs(translator): point the deprecation register at the renamed read method --- packages/payload-plugin-translator/docs/DEPRECATIONS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/payload-plugin-translator/docs/DEPRECATIONS.md b/packages/payload-plugin-translator/docs/DEPRECATIONS.md index c677c6524..c63c46dc1 100644 --- a/packages/payload-plugin-translator/docs/DEPRECATIONS.md +++ b/packages/payload-plugin-translator/docs/DEPRECATIONS.md @@ -46,7 +46,7 @@ the single source of truth — code annotations link here by anchor instead of d remain readable. Removed (along with the fallback read path) in the next major. - **Code refs:** - `src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts` (inputSchema, handler input type/unpacking) - - `src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts` (enqueue write, `findByCollection` / `findJobsInternal` query + in-memory filter) + - `src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts` (enqueue write, `findByCollection` / `findRawJobs` query + in-memory filter) - `src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts` (read fallback) - `src/server/modules/task-runner/payload-jobs-runner/types.ts` (`PayloadJob.input` shape) From a9a68a9412b6232e064cdf00d1f9fa5657cfc7e9 Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 8 Sep 2026 14:14:06 +0200 Subject: [PATCH 5/7] docs(translator): cut the comment noise from the jobs runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runner file carried 64 comment lines over 340 — most of them restating the line below, or a third copy of a fact the task contract and a named test already hold. It is now 39 over 318, and the diff as a whole 138 over 1650. Two of them were stranded rather than merely noisy: cancel-by-collection's docblock still described the "pending" predicate the change removed, and the client's DocumentTranslation type still said one job carries one locale. The pre-workflow task shape now has a deprecation-register entry, so the fallback read paths point at a record of when they may be dropped instead of explaining themselves in three places. A duplicate test case left by an earlier rename is gone, and the "does not enable the host's concurrency control" assertion is its own case rather than a comment inside another. --- .../integration/translator/bootTestPayload.ts | 18 +++-- .../translator/exclusive-queue.int.test.ts | 8 +- .../translator/job-extend.int.test.ts | 1 - .../translator/locale-append.int.test.ts | 6 +- .../locale-workflow-failure.int.test.ts | 4 +- .../translator/locale-workflow.int.test.ts | 3 +- .../docs/DEPRECATIONS.md | 21 ++++++ .../entities/translation/model/types.ts | 11 +-- .../cancel-by-collection/handler.test.ts | 16 +--- .../features/cancel-by-collection/handler.ts | 9 +-- .../PayloadJobsRunnerProvider.test.ts | 6 +- .../PayloadJobsTaskRunner.test.ts | 12 +-- .../PayloadJobsTaskRunner.ts | 74 +++++++------------ .../payload-jobs-runner/normalizeJob.ts | 7 +- .../payload-jobs-runner/planEnqueue.test.ts | 1 - .../payload-jobs-runner/planEnqueue.ts | 13 ++-- .../task-runner/payload-jobs-runner/types.ts | 5 +- 17 files changed, 89 insertions(+), 126 deletions(-) diff --git a/apps/dev/src/integration/translator/bootTestPayload.ts b/apps/dev/src/integration/translator/bootTestPayload.ts index 66a9346c7..48ad5be32 100644 --- a/apps/dev/src/integration/translator/bootTestPayload.ts +++ b/apps/dev/src/integration/translator/bootTestPayload.ts @@ -22,12 +22,12 @@ import { createTestDatabase } from "../../lib/database/resolveAdapter"; import { reverseComplete } from "../../lib/translator/fakeComplete"; import { buildTestCollections } from "./testCollections"; -/** - * A booted test Payload plus the throwaway resources to tear down after the suite. - */ /** Payload's `autoRun.limit` default — these specs reproduce the cron's batching, not a run of one. */ export const CRON_BATCH_LIMIT = 50; +/** + * A booted test Payload plus the throwaway resources to tear down after the suite. + */ export type TestPayload = { payload: Payload; cleanup: () => Promise; @@ -52,6 +52,8 @@ export type TestPayload = { * `runId` (see `resolveTestDbAdapter`), so schema `push` is a clean CREATE with no data-loss branch * — Payload never drops to the interactive "accept data loss?" prompt that would hang an * unattended/headless run. `cleanup()` drops the namespace and removes the temp dir even on failure. + * - **One boot per process:** `getPayload` caches, so a second `bootTestPayload` in the same spec + * file returns the first — a case that needs its own boot needs its own file. * - **Sync runner:** a translation runs INLINE inside the triggering `afterChange`, so it is complete * when the awaited `payload.update`/`create` resolves — no job autorun, no polling, no async race * in the specs. @@ -68,8 +70,8 @@ export type TestPayload = { * @param opts.runner - defaults to the sync runner. `createPayloadJobsRunner({ autoRun: false })` * leaves queued jobs unprocessed in `payload-jobs`, so a spec can read the rows. * @param opts.onTranslate - awaited before each provider call, so a spec can hold a locale mid-run. - * @param opts.exclusiveQueue - Payload's `enableConcurrencyControl` for this boot; `EXCLUSIVE_QUEUE=1` - * sets it for every boot, which is how the suite is run in that mode without touching a spec. + * @param opts.exclusiveQueue - Payload's `enableConcurrencyControl` for this boot; defaults to + * `EXCLUSIVE_QUEUE=1`. * @param opts.fallback - localization fallback, off by default: an unwritten locale reads as * empty, not as the default locale's text. Localization-level, so it applies to the whole boot. */ @@ -121,14 +123,14 @@ export async function bootTestPayload(opts?: { { code: "en", label: "English" }, { code: "de", label: "Deutsch" }, { code: "fr", label: "Français" }, - // A third target so a spec can tell "the run stopped at the failure" from "the run carried - // on and one locale threw" — with two locales the failing one is always the last. + // Three targets, not two: with two, the failing locale is always the last and "stopped at + // the failure" is unobservable. { code: "es", label: "Español" }, ], }, collections, - // Payload deletes completed jobs by default, which would leave the status panels nothing to read. jobs: { + // Payload deletes completed jobs by default, leaving the status panels nothing to read. deleteJobOnComplete: false, enableConcurrencyControl: opts?.exclusiveQueue ?? process.env.EXCLUSIVE_QUEUE === "1", }, diff --git a/apps/dev/src/integration/translator/exclusive-queue.int.test.ts b/apps/dev/src/integration/translator/exclusive-queue.int.test.ts index f3744039d..ac6cfe919 100644 --- a/apps/dev/src/integration/translator/exclusive-queue.int.test.ts +++ b/apps/dev/src/integration/translator/exclusive-queue.int.test.ts @@ -5,8 +5,7 @@ import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; import type { TestPayload } from "./bootTestPayload"; import { callEndpoint } from "./callEndpoint"; -// Booted with `enableConcurrencyControl`. Its own file because the setting is fixed at boot and -// `getPayload` caches per process. +// Its own file: the setting is fixed at boot, and a boot is per process (see `bootTestPayload`). type RunResult = { jobStatus?: Record }; @@ -100,9 +99,8 @@ describe("with the host's concurrency control on", () => { await enqueue(id, ["fr"]); - // Without this the check below is satisfied by "there was nothing to pick": if the request had - // extended the running job instead of getting one of its own, the picker would also take - // nothing and `fr` would still translate. + // The picker check below also passes when there was nothing to take, so first prove the second + // job exists. const queued = await jobsFor(id); expect(queued.length, "the request did not get a job of its own").toBe(2); expect( diff --git a/apps/dev/src/integration/translator/job-extend.int.test.ts b/apps/dev/src/integration/translator/job-extend.int.test.ts index 7d194e618..16481d493 100644 --- a/apps/dev/src/integration/translator/job-extend.int.test.ts +++ b/apps/dev/src/integration/translator/job-extend.int.test.ts @@ -30,7 +30,6 @@ const enqueue = async (id: string, targets: string[] = ["de"]) => { return body.data.queued; }; -// The cases share one boot, so the table also holds every earlier case's jobs. const jobs = async (documentId: string): Promise => { const { docs } = await ctx.payload.find({ collection: "payload-jobs" as "pages", diff --git a/apps/dev/src/integration/translator/locale-append.int.test.ts b/apps/dev/src/integration/translator/locale-append.int.test.ts index eaf12b37a..42f56cf1e 100644 --- a/apps/dev/src/integration/translator/locale-append.int.test.ts +++ b/apps/dev/src/integration/translator/locale-append.int.test.ts @@ -7,8 +7,7 @@ import { callEndpoint } from "./callEndpoint"; // Guards an undocumented Payload behaviour: after each task settles it re-reads the job row onto the // live `job` object, which is how a locale appended mid-run reaches the handler. Measured on 3.84.1; -// the plugin's peer floor is ^3.76.0. If a future version stops doing it, this file goes red instead -// of translations going missing. +// peer floor is ^3.76.0. type Job = { id: string | number; @@ -78,8 +77,7 @@ describe("adding a locale to a running job", () => { const run = ctx.payload.jobs.run({ queue: "translations", limit: CRON_BATCH_LIMIT }); await heldReached; - // Mid-flight on `fr`. Without this the surviving-log claim is untested: if the log were still - // empty when the second request lands, nothing could be lost from it. + // The surviving-log claim is vacuous unless the log is non-empty when the second request lands. const before = await readJob(); expect( (before.log ?? []).map((e) => e.input?.target_lng), diff --git a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts index 97e2da65c..56e5c34fb 100644 --- a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts +++ b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts @@ -5,8 +5,8 @@ import { bootTestPayload, CRON_BATCH_LIMIT } from "./bootTestPayload"; import type { TestPayload } from "./bootTestPayload"; import { callEndpoint } from "./callEndpoint"; -// A locale whose provider fails mid-run. Its own file because the failing provider is a property of -// the boot, and `getPayload` caches per process — a second boot in one file returns the first. +// Its own file: the failing provider is fixed at boot, and a boot is per process (see +// `bootTestPayload`). const rev = (s: string) => [...s].reverse().join(""); diff --git a/apps/dev/src/integration/translator/locale-workflow.int.test.ts b/apps/dev/src/integration/translator/locale-workflow.int.test.ts index 451ea0825..9f0c1accd 100644 --- a/apps/dev/src/integration/translator/locale-workflow.int.test.ts +++ b/apps/dev/src/integration/translator/locale-workflow.int.test.ts @@ -134,8 +134,7 @@ describe("translating one document into several locales", () => { await enqueue(id, ["de", "fr"]); await runQueue(); - // Without this the check passes for the wrong reason: a job left *failed* is also skipped on the - // second run, because the retry backoff pushes its `waitUntil` into the future. + // A *failed* job is skipped on the second run too (backoff), so prove it completed first. expect((await workflowJob(id))?.completedAt, "the workflow did not complete").toBeTruthy(); const before = ctx.translateCount(); diff --git a/packages/payload-plugin-translator/docs/DEPRECATIONS.md b/packages/payload-plugin-translator/docs/DEPRECATIONS.md index c63c46dc1..38d822908 100644 --- a/packages/payload-plugin-translator/docs/DEPRECATIONS.md +++ b/packages/payload-plugin-translator/docs/DEPRECATIONS.md @@ -50,6 +50,27 @@ the single source of truth — code annotations link here by anchor instead of d - `src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts` (read fallback) - `src/server/modules/task-runner/payload-jobs-runner/types.ts` (`PayloadJob.input` shape) +### jobs-per-locale-task-shape + +- **What:** jobs queued as a bare task (`taskSlug: 'translate_document'`, one `target_lng` in the + input) rather than as the locale-walking workflow. +- **Status:** live +- **Deprecated:** 2026-09-04 / PR #133 +- **Replacement:** one workflow job per document (`workflowSlug: 'translate_document_locales'`, + `target_lngs` list). +- **Remove in:** next major +- **Why:** a job per locale meant Payload ran a document's locales through `Promise.all`, and every + write it makes is a whole-document version snapshot — so the second locale's snapshot dropped the + first's work. See [locale workflow](./plans/2026-09-04-locale-workflow.task.md). +- **Migration:** expand/contract. Nothing writes the task shape any more, but rows queued before the + upgrade are still in `payload-jobs`, so every read keeps a fallback for them. Removing the fallback + strands those rows: cancel, stale-lock reclaim and the status panels stop finding them, silently. +- **Code refs:** + - `src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts` (`ownJobs()`) + - `src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts` (`pickHost` skips it) + - `src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts` (`normalizeJobLocales` + expands it to itself) + ### find-by-collection-document-ids-array - **What:** passing a bare `Array` of document ids as the second argument to diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts index c28fece78..ea4426d34 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts @@ -19,11 +19,7 @@ export type DocumentTranslationFailed = { created_at: string; updated_at: string; input: InputData; - /** - * Absent while the job itself carries no final error. A locale's failure is recorded in the job - * log as soon as it happens, but the job keeps `error` unset until it stops retrying — so a row - * can read `failed` with nothing to show yet. - */ + /** Unset until the job stops retrying, so a row can read `failed` with nothing to show. */ error?: { message: string; }; @@ -47,9 +43,8 @@ export type DocumentTranslationCompleted = { }; /** - * One translation job for a single target locale. The document status feed is an array of these — - * the latest job per target locale (see `useDocumentTranslation`), because re-translate queues an - * independent job per locale. + * One row per target locale in the document status feed. Several rows can share one job id — a job + * carries all of a document's locales. */ export type DocumentTranslation = | DocumentTranslationCompleted diff --git a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts index fd35b0fc4..694ded272 100644 --- a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts +++ b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.test.ts @@ -95,20 +95,6 @@ describe("CancelByCollectionHandler", () => { expect(response.body).toBeNull(); }); - it("returns 204 when every job is in flight", async () => { - const tasks = [ - createMockTask({ id: "task-1", status: "running" }), - createMockTask({ id: "task-2", status: "running" }), - ]; - (mockTaskRunner.findByCollection as ReturnType).mockResolvedValue(tasks); - - const req = createMockRequest({ collection_slug: "posts" }); - const response = await handler.handle(req); - - expect(response.status).toBe(204); - expect(mockTaskRunner.cancel).not.toHaveBeenCalled(); - }); - it("returns 204 when all tasks are running (not pending)", async () => { const tasks = [ createMockTask({ id: "task-1", status: "running" }), @@ -124,7 +110,7 @@ describe("CancelByCollectionHandler", () => { }); }); - describe("cancelling pending tasks", () => { + describe("cancelling queued jobs", () => { it("cancels every queued job and leaves the one in flight alone", async () => { const tasks = [ createMockTask({ id: "task-1", status: "pending" }), diff --git a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts index d1a601341..85e68ab1c 100644 --- a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts +++ b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts @@ -7,9 +7,7 @@ import { isCollectionAvailable } from "../_lib/collection-utils"; import { CancelByCollectionInputSchema } from "./model"; import type { CancelConfig } from "./model"; -/** - * Cancels all pending translation tasks for a collection - */ +/** Cancels every queued job for a collection; jobs in flight are left alone. */ export class CancelByCollectionHandler { constructor( private readonly config: CancelConfig, @@ -32,9 +30,8 @@ export class CancelByCollectionHandler { const rows = await runner.findByCollection(collectionSlug, { excludeCompleted: true }); if (rows.length === 0) return ServerResponse.noContent(); - // Rows are per locale, cancel addresses jobs. Filtering rows by `pending` misses a job waiting - // to retry: all of its locales are logged, so it has no pending row, yet the picker takes it - // again. + // A job waiting to retry has every locale logged, so it has no `pending` row — filter by *not + // running* instead. const running = new Set(rows.filter((row) => row.status === "running").map((row) => row.id)); const queuedJobIds = [...new Set(rows.map((row) => row.id))].filter((id) => !running.has(id)); if (queuedJobIds.length === 0) return ServerResponse.noContent(); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts index 71d4541ed..432b114af 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.test.ts @@ -98,7 +98,11 @@ describe("PayloadJobsRunnerProvider", () => { const config = createPayloadJobsRunner().configure(minimalContext)(makeConfig()); expect(workflowOf(config).concurrency).toBeUndefined(); - // The plugin must not turn the flag on for the host — it is a schema decision, see the README. + }); + + it("does not enable the host's concurrency control on its behalf", () => { + const config = createPayloadJobsRunner().configure(minimalContext)(makeConfig()); + expect(config.jobs?.enableConcurrencyControl).toBeUndefined(); }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts index 203190991..82282e40e 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.test.ts @@ -31,7 +31,6 @@ describe("PayloadJobsTaskRunner", () => { jobs: { queue: vi.fn().mockResolvedValue(undefined), cancel: vi.fn().mockResolvedValue(undefined), - // A non-empty `jobStatus` is how Payload reports that the picker actually took a job. run: vi .fn() .mockResolvedValue({ jobStatus: { "job-123": {} }, remainingJobsFromQueried: 0 }), @@ -92,7 +91,7 @@ describe("PayloadJobsTaskRunner", () => { }); describe("enqueue", () => { - it("looks for superseded jobs among unfinished ones only", async () => { + it("reads only unfinished jobs when planning an enqueue", async () => { await runner.enqueue([createInput()]); const whereArg = mockPayload.find.mock.calls[0][0].where; @@ -156,8 +155,7 @@ describe("PayloadJobsTaskRunner", () => { it("adds the locale to a live job instead of queuing a second one", async () => { const live = createLiveJob({ id: "live-job" }); - // First read builds the plan; the second is the check that the write landed, so it answers - // with the row as the write leaves it. + // First `find` builds the plan, the second verifies the write. mockPayload.find.mockResolvedValueOnce({ docs: [live] }).mockResolvedValueOnce({ docs: [{ ...live, input: { ...live.input, target_lngs: ["de", "fr"] } }], }); @@ -237,7 +235,6 @@ describe("PayloadJobsTaskRunner", () => { }); it("retries the write once when a concurrent append replaced the list", async () => { - // Not finished, unlike the case above — the locale is simply missing from the stored row. const live = createLiveJob({ id: "live-job" }); const clobbered = { ...live, input: { ...live.input, target_lngs: ["de", "es"] } }; mockPayload.find @@ -509,7 +506,6 @@ describe("PayloadJobsTaskRunner", () => { where: { id: { equals: "job-123" } }, data: { processing: false, hasError: false, error: null, waitUntil: null }, }); - // run via the where-based picker, NOT runByID expect(mockPayload.jobs.run).toHaveBeenCalledWith({ queue: "translations", where: { id: { equals: "job-123" } }, @@ -553,7 +549,6 @@ describe("PayloadJobsTaskRunner", () => { where: { id: { equals: "job-123" } }, limit: 1, }); - // a pending job (processing:false) needs no lock reset expect(mockPayload.update).not.toHaveBeenCalled(); expect(mockPayload.find).toHaveBeenCalledWith( expect.objectContaining({ @@ -705,8 +700,7 @@ describe("PayloadJobsTaskRunner", () => { }); it("narrows the SQL where clause by the job's own slugs only", async () => { - // Narrowing by the collection slug or id would re-introduce the SQLite coercion bug and drop - // the legacy shape — see `findByCollection`'s docblock. + // See `findByCollection`'s docblock: a `where` on slug or id drops every pre-migration job. await runner.findByCollection("posts" as CollectionSlug, [5, 6]); const whereArg = mockPayload.find.mock.calls[0][0].where; diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts index 030e11156..6f2e4def9 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts @@ -9,6 +9,8 @@ import { planEnqueue } from "./planEnqueue"; import type { RequestShape } from "./planEnqueue"; import { readCollectionRef } from "./readCollectionRef"; +const APPEND_ATTEMPTS = 2; + type QueueWorkflow = (args: { workflow: string; queue: string; @@ -25,7 +27,6 @@ type StoredWorkflowInput = { publish_on_translation: boolean; }; -/** Ids are stringified here: the stored `collection_id` is text, so a number id must coerce to match. */ function requestShape(task: TaskInput): RequestShape { return { collectionSlug: task.collectionSlug, @@ -38,8 +39,7 @@ function requestShape(task: TaskInput): RequestShape { function requestKey(task: TaskInput): string { const r = requestShape(task); - // NUL separates: no slug, id, locale or strategy can contain it, so no two different requests can - // collide on one key. + // NUL: no stored field value can contain it, so two different requests cannot produce one key. return [ r.collectionSlug, r.collectionId, @@ -61,7 +61,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { ) {} async enqueue(tasks: TaskInput[]): Promise { - // Keyed by everything a job carries one of, not by document alone — see `pickHost`. const byRequest = new Map(); for (const task of tasks) { const key = requestKey(task); @@ -70,8 +69,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { byRequest.set(key, group); } - // `findRawJobs` is unpaginated and filters in memory, so narrowing per document would re-read the - // whole table once per document. const live = await this.findRawJobs({ completedAt: { exists: false } }, { pagination: false }); const exclusiveQueue = Boolean(this.payload.config.jobs?.enableConcurrencyControl); @@ -106,16 +103,13 @@ export class PayloadJobsTaskRunner implements TaskRunner { /** @returns the locales that did not reach the job and need one of their own. */ private async extendJob(job: PayloadJob, locales: string[], waitUntil?: Date): Promise { let current: PayloadJob | undefined = job; - // Two attempts: `input` is one JSON column, so a concurrent append replaces the whole list and - // ours can be lost. Adding a locale is a set union, which makes retrying from the stored row - // harmless. Anything still missing after that gets a job of its own rather than being dropped. - for (let attempt = 0; attempt < 2; attempt++) { + // `input` is one JSON column, so a concurrent append replaces the whole list; the union makes a + // retry from the stored row harmless. + for (let attempt = 0; attempt < APPEND_ATTEMPTS; attempt++) { if (!current || current.completedAt) return locales; const listed = current.input?.target_lngs ?? []; const missing = locales.filter((locale) => !listed.includes(locale)); - // Keep the debounce: a job that has not started is still coalescing rapid source edits, and - // this request is the latest of them. A running job's schedule is not ours to move. const debounce = waitUntil && !current.processing ? waitUntil.toISOString() : undefined; if (missing.length === 0 && !debounce) return []; @@ -124,9 +118,8 @@ export class PayloadJobsTaskRunner implements TaskRunner { ...(debounce ? { waitUntil: debounce } : {}), }; - // Adapter write, not `payload.update`: the document operation re-reads and rewrites the whole - // row, reverting log entries written in between. Measured — see D2 of - // docs/plans/2026-09-08-one-live-job-per-document.task.md. + // Not `payload.update`: it rewrites the whole row and reverts log entries written in between. + // See D2 of docs/plans/2026-09-08-one-live-job-per-document.task.md. await this.payload.db.updateOne({ collection: this.config.jobsCollection, id: job.id, @@ -135,8 +128,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { }); [current] = await this.findRawJobs({ id: { equals: job.id } }, { limit: 1 }); - // Finished between the plan's read and this write: the locales are stored but nobody will run - // them, so they need a job of their own. if (!current || current.completedAt) return locales; const stored = new Set(current.input?.target_lngs); if (locales.every((locale) => stored.has(locale))) return []; @@ -159,8 +150,8 @@ export class PayloadJobsTaskRunner implements TaskRunner { publish_on_translation: request.publishOnTranslation, }; - // `jobs.queue` is generic over the host's generated job slugs; this workflow is registered at - // config time, so the call goes through a signature naming what it actually accepts. + // Cast: `jobs.queue` is typed over the host's generated slugs, which cannot include a workflow + // registered at config time. const queueJob = this.payload.jobs.queue as unknown as QueueWorkflow; await queueJob({ workflow: this.config.workflowName, @@ -180,8 +171,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { if (!job) { return { success: false, error: "not_found" }; } - // The job's own state, not a locale row's: a locale row carries the log entry's `completedAt`, - // which Payload stamps on failures too. const task = normalizeJob(job); if (task.completedAt) { return { success: false, error: "already_completed" }; @@ -203,19 +192,17 @@ export class PayloadJobsTaskRunner implements TaskRunner { limit: 1, })) as { jobStatus?: Record }; - // An empty `jobStatus` is Payload reporting that the picker took nothing — usually a document - // already running under the host's concurrency control. - if (Object.keys(result?.jobStatus ?? {}).length === 0) { + const pickerTookNothing = Object.keys(result?.jobStatus ?? {}).length === 0; + if (pickerTookNothing) { return { success: false, error: "already_running" }; } return { success: true }; } /** - * Clear stale `processing` locks — still processing, not completed, `updatedAt` older than - * `staleJobTimeoutMs` — so abandoned jobs are eligible for the autorun picker again. A job that - * exhausted its retries carries `hasError: true` and stays excluded from autorun even after its - * lock is cleared; only a manual `run()` recovers it. + * Clears stale `processing` locks so abandoned jobs are eligible for the autorun picker again. + * A job that exhausted its retries carries `hasError: true` and stays excluded even after its lock + * is cleared; only a manual `run()` recovers it. * @returns how many locks were cleared. */ async reclaimStaleJobs(): Promise { @@ -231,11 +218,8 @@ export class PayloadJobsTaskRunner implements TaskRunner { } /** - * Clears the processing lock, a spent retry budget and a pending backoff — the three things that - * make the picker skip a job, so a manual run really is the retry. - * - * Goes through `payload.update` rather than the adapter, unlike `extendJob`, so the jobs - * collection's own `beforeChange` hook still runs — it is what keeps a cancelled job cancelled. + * `payload.update`, not the adapter write `extendJob` uses, so the jobs collection's `beforeChange` + * hook still runs — it is what keeps a cancelled job cancelled. */ private async clearPickerBlockers(taskId: string): Promise { await this.payload.update({ @@ -246,7 +230,7 @@ export class PayloadJobsTaskRunner implements TaskRunner { }); } - /** Clears the `processing` lock on every job matching `where`. `depth: 0` — only the count is read. */ + /** `depth: 0` — only the row count is read. */ private async resetProcessing(where: Where): Promise { const result = await this.payload.update({ collection: this.config.jobsCollection, @@ -266,13 +250,10 @@ export class PayloadJobsTaskRunner implements TaskRunner { } /** - * Find translation jobs for a collection. - * - * Only `workflowSlug`/`taskSlug` and `completedAt` reach the database; the collection slug and - * document ids are matched in memory - * because a job's collection reference may sit in either the flat-text fields or the legacy - * relationship shape (`readCollectionRef`), so a `where` on `input.collection_slug` would silently - * drop every pre-migration job. `excludeCompleted` is what bounds the read — see issue #108. + * Only the job slugs and `completedAt` reach the database; the collection slug and document ids are + * matched in memory, because a job's collection reference may sit in the flat-text fields or in the + * legacy relationship shape (`readCollectionRef`) — a `where` on `input.collection_slug` would + * silently drop every pre-migration job. `excludeCompleted` bounds the read (#108). */ async findByCollection( collectionSlug: CollectionSlug, @@ -291,17 +272,14 @@ export class PayloadJobsTaskRunner implements TaskRunner { private async cancelAndDeleteJobs(taskIds: string[]): Promise { if (taskIds.length === 0) return; - // Both, in this order: `jobs.cancel` only writes `{ error: { cancelled: true }, hasError: true, - // processing: false }`, which is what signals a running handler to abort. The delete then removes - // the row — under `deleteJobOnComplete: false` a cancelled job would otherwise sit in the status - // feed forever. + // Both, in order: `jobs.cancel` only marks the row (`error.cancelled`), which is the signal a + // running handler aborts on; the delete then keeps it out of the status feed under + // `deleteJobOnComplete: false`. await this.payload.jobs.cancel({ where: { id: { in: taskIds } }, queue: this.config.queueName, }); - // Narrowed to our own jobs: the ids arrive straight from the request body, and `payload-jobs` is - // shared with whatever else the host queues there. await this.payload.delete({ collection: this.config.jobsCollection, where: { and: [this.ownJobs(), { id: { in: taskIds } }] }, @@ -309,7 +287,7 @@ export class PayloadJobsTaskRunner implements TaskRunner { } private ownJobs(): Where { - // Jobs queued before the workflow change are still in the table and carry the per-locale task slug. + // Pre-workflow jobs are still in the table: docs/DEPRECATIONS.md#jobs-per-locale-task-shape return { or: [ { workflowSlug: { equals: this.config.workflowName } }, diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts index 58c9fff1e..7cdbc2671 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/normalizeJob.ts @@ -31,9 +31,6 @@ export function isCancelled(error: unknown): boolean { ); } -/** - * Transform Payload job to normalized Task - */ export function normalizeJob(job: PayloadJob): Task { const { collectionSlug, collectionId } = readCollectionRef(job.input); @@ -83,8 +80,8 @@ export function normalizeJobLocales(job: PayloadJob): Task[] { } /** - * Each target locale's most recent log entry: Payload appends to `log` chronologically, so - * last-write-wins leaves the latest attempt. An absent entry means that locale has not run. + * Each locale's most recent log entry: Payload appends to `log` chronologically, so last-write-wins + * leaves the latest attempt. */ export function latestLogByLocale(job: PayloadJob): Map { const byLocale = new Map(); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts index 5f6353fc1..d20708e34 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.test.ts @@ -68,7 +68,6 @@ describe("planEnqueue", () => { ["oldest first", ["old", "new"]], ["newest first", ["new", "old"]], ])("extends the newest live job, %s", (_label, order) => { - // Both orders, because "take the last element" and "sort the other way" each pass on one of them. const byId = { old: job({ id: "old", createdAt: "2026-01-01T00:00:00Z" }), new: job({ id: "new", createdAt: "2026-01-02T00:00:00Z" }), diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts index 5788098e2..6b42bed56 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts @@ -21,8 +21,7 @@ export type EnqueuePlan = { }; /** - * One live job per document: a later request extends that job's locale list rather than replacing - * it, because replacing drops whatever locales it still owed. + * One live job per document: a later request extends that job's locale list rather than replacing it. * * @param live - non-completed jobs for **this document only**; the caller filters by document. * @param requested - target locales in request order; duplicates ignored. @@ -53,10 +52,9 @@ export function planEnqueue(args: { } /** - * A job carries one source locale, one strategy and one publish flag for all of its locales, so it - * can only take locales from a request that chose the same three — otherwise the request would run - * under settings the user did not pick. Pre-workflow jobs (a single `target_lng`) and cancelled jobs - * are skipped outright. + * A job carries one source locale, one strategy and one publish flag for all of its locales, so it can + * host only a request that chose the same three — otherwise the request runs under settings the user + * did not pick. */ function pickHost(live: PayloadJob[], request: RequestShape): PayloadJob | null { const usable = live.filter( @@ -67,5 +65,6 @@ function pickHost(live: PayloadJob[], request: RequestShape): PayloadJob | null job.input?.strategy === request.strategy && (job.input?.publish_on_translation ?? false) === request.publishOnTranslation ); - return usable.sort((a, b) => Date.parse(b.createdAt) - Date.parse(a.createdAt))[0] ?? null; + const newestFirst = usable.sort((a, b) => Date.parse(b.createdAt) - Date.parse(a.createdAt)); + return newestFirst[0] ?? null; } diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts index f3eee85c6..fd4a9410f 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts @@ -72,7 +72,7 @@ export type PayloadJobsRunnerOptions = { */ export type PayloadJobsRunnerConfig = { taskName: string; - /** Derived from `taskName`, not configurable — see `createPayloadJobsRunner`. */ + /** Derived from `taskName`; deliberately not a plugin option. */ workflowName: string; queueName: string; jobsCollection: CollectionSlug; @@ -81,9 +81,6 @@ export type PayloadJobsRunnerConfig = { retries?: PayloadJobsRunnerOptions["retries"]; }; -/** - * Raw Payload job structure - */ export type PayloadJob = { log?: JobLogEntry[]; id: string; From 93f415d4513af444727a5485a390a2d6eed726fd Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 8 Sep 2026 16:48:46 +0200 Subject: [PATCH 6/7] refactor(translator): shrink the jobs runner to what it must hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file had grown to 340 lines and 64 comments. Four review angles and a complexity pass later it is 302 and 35, with two members fewer and no behaviour change. The substantive removals: extendJob carried an unreachable guard (its job comes from a read that already excludes completed rows) and computed its undelivered set twice; run() normalised a job to read three fields the raw row already has; two one-call private helpers were inlined into their only callers. Two invariants stopped depending on a reader's attention. The grouping key is now the request shape itself, so "the key covers at least what pickHost compares" holds by construction rather than by a comment — that is what keeps parallel appends off the same row. And the stored job input is declared once instead of twice under one name in two files. Enqueue also serves documents in bounded batches rather than all at once, and findByCollection narrows raw jobs before expanding them per locale instead of after. One comment was wrong rather than merely noisy: it said cancelling marks a row so a running handler aborts, while the task contract records the opposite — the delete removes the row first. It now says so and points at the measurement. --- ...8-richtext-container-granularity-design.md | 343 ++++++++++++++++++ .../PayloadJobsRunnerProvider.ts | 15 +- .../PayloadJobsTaskRunner.ts | 210 +++++------ .../payload-jobs-runner/planEnqueue.ts | 4 +- .../task-runner/payload-jobs-runner/types.ts | 10 + 5 files changed, 461 insertions(+), 121 deletions(-) create mode 100644 packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md diff --git a/packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md b/packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md new file mode 100644 index 000000000..2b8348667 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md @@ -0,0 +1,343 @@ +# Design — rich text container granularity (#134) + +- **Issue:** [#134](https://github.com/focusreactive/payload-plugins/issues/134) +- **Date:** 2026-09-08 · **Status:** decided, two questions open (§7) + +--- + +## 1. What is being built + +The unit of rich text translation moves from the **text node** to its **inline container** — +the paragraph, heading, list item or quote that holds it. Fragments inside a container are +wrapped in numbered marks, the model returns them in whatever order the target language +needs, and the container's `children` array is rebuilt in that order. + +Today each text node is translated alone and written back into the node it came from, so the +node order never changes: the translation keeps the source language's word order, and every +inline mark stays pinned to its source position. `a **red** car` becomes *une rouge voiture* +instead of *une voiture **rouge***, and no prompt can fix it — the model is never given the +chance to reorder anything. + +A mark number is **a pointer to the original inline node, not an address to write into**. +That single idea is what keeps this layer from having to understand Lexical formatting: +formatting travels inside the nodes themselves. + +``` +sent: { 7: "<1>a <2>red<3> car" } +returned: { 7: "<1>une <3>voiture <2>rouge" } +applied: children = [ node1("une "), node3("voiture "), node2("rouge") ] +``` + +The boundary between core and provider is the string. The core finds containers, glues +whitespace, emits marks, and afterwards parses, verifies and rebuilds; the provider takes a +string and returns a string, knowing nothing about nodes, gaps or Lexical. + +Reads stay within `type` / `text` / `children`. Writes are `text` on a leaf and `children` +on the container. `format`, `style`, `detail`, a link's `fields` and every other mark +representation are never read and never written — the same surface `kernel/lexical/types.ts` +already declares. + +--- + +## 2. Decisions + +| # | Decision | Why | +| --- | --- | --- | +| D1 | Unit of translation is the nearest node with at least one direct text child | Needs no knowledge of which node types are inline. Exotic trees degrade to today's behaviour instead of breaking | +| D2 | Mark syntax is `text`, self-closing `` for non-text inline nodes, always flat | Models have seen XLIFF and HTML; numeric names cannot collide with meaningful tags; flatness keeps the parser stackless and removes nesting as a failure mode | +| D3 | A container whose source text contains a mark-shaped sequence (`<12>`, ``, `<12/>`) falls back to per-node granularity | Removes the whole escaping problem for the price of losing the optimisation on content that is close to nonexistent. Plain `
` or `5 < 10` are unaffected — only digits between angle brackets collide | +| D4 | Every fragment is wrapped, including unformatted ones | The layer then never constructs a node from scratch — every output node is an existing node with new text | +| D5 | Marks returned in their original order take the current write-into-the-node path | Most content, and all close-language pairs, land here. The new path runs only where it changes the result | +| D6 | A container holding a single unformatted text node emits no marks at all | Typical paragraph pays nothing — not a single extra token | +| D7 | Corrupt reply for a container ⇒ that container is retranslated per-node in one extra batched request | Degrades to today's quality rather than silently dropping inline marks. Pouring the whole translation into the first node would be *worse* than today — every link and emphasis inside that container lost. Bounded: one extra pass, never a loop; a second unusable reply leaves the container untranslated and reported | +| D8 | Granularity is an option, defaulting to `"node"` for this major version | Existing installs, including ones with hand-written providers, keep byte-identical output until they opt in | +| D9 | A provider opts in by declaring `capabilities.inlineMarks` | A machine-translation API behind `CompletionFn` would translate or strip marks. Absent declaration ⇒ per-node, whatever the option says | +| D10 | `translate()` gains an optional 4th parameter carrying which keys hold marks | One request mixes marked containers with plain `text` fields. Optional, so existing implementations are unaffected | +| D11 | The mark instruction is appended to the system prompt by the core, after any override | A `SystemPromptBuilder` that ignores `defaultPrompt` must not be able to drop the one instruction the format depends on | +| D12 | Adjacent fragments sharing one wrapper are not merged in v1 | Two text leaves inside one link keep that link as their `top`, so the rebuilt array holds it twice — the same node, adjacent. Rendering is unchanged; merging is polish, not correctness | +| D13 | Every issued number must come back **exactly once**; order is free, empty content is how a merge is expressed | One set comparison, no occurrence counting. Rejecting a repeated mark is what removes copying from the design entirely (D17) | +| D14 | Plain values and single-node containers are sent unmarked; stray marks in their replies are stripped and warned | No markup inside them to preserve, so marks would be pure token cost | +| D15 | Whitespace-only nodes are glued onto the preceding fragment, never marked on their own | A mark of its own can come back empty, and the gap between two words would be gone. Glued, the space rides inside a fragment that carries text | +| D16 | Edge whitespace is restored by the core after the reply, not by the provider | Models trim edges. The provider's contract is string in, string out — it must not know about nodes or gaps. This is exactly the logic that ossified as a "Fix spaces" patch inside the Storyblok plugin's model call | +| D17 | Nothing is copied: a fragment holds two live references — `node` (the text leaf) and `top` (the container's direct child) | Applying a translation is then `node.text = ...` plus a reordered `children` array. A repeated mark is the only case that would need a clone, and D13 rejects it | +| D18 | One walk, not two: `collectSerializedLexicalTextNodes` returns every text node (whitespace included) plus each one's `node`/`top` pair; the whitespace filter moves into `RichTextExpander` | Collecting is the walk's job, deciding what not to translate is the caller's. Removes a duplicate traversal, at the cost of D19 | +| D19 | Provenance records gain a fingerprint version; a record written under an older version is recomputed, not declared stale | D18 changes the join (`"Buyour product"` → `"Buy our product"`), so every stored fingerprint would stop matching — the admin would show every translation stale and auto-translate would retranslate everything at the customer's expense. `dismissedFingerprint` was added ahead of need for the same reason | + +--- + +## 3. Three facts that shaped this + +**The text-node walk serves two subsystems, on purpose.** +`collectSerializedLexicalTextNodes` has two callers that ask it different questions: + +| Caller | Question it answers | +| --- | --- | +| `RichTextExpander` (translation pipeline) | which pieces of text to send to the model | +| `leafSourceText` → `projectTranslatableContent` → `fingerprint` (provenance) | what counts as a field's content, so a later run can tell whether the source changed | + +The sharing is deliberate — `contentProjector.ts:5-8` says why: projection and translation reuse +one traversal and one leaf predicate *"so projection and translation can never disagree on which +content is translatable"*. The second caller's output is hashed and **stored** in the provenance +store at translation time; `staleness.ts` compares that stored value against a freshly computed +one to decide whether a translation is stale (the admin indicator, and auto-translate's +source-changed check). + +Container mode needs two things the walk does not currently give: whitespace-only nodes (it +filters them out, and they hold the gaps between words) and, per text node, the container's +direct child above it (the flat `{ node }` return cannot say a leaf sat inside a link). + +Rather than a second walk, D18 extends this one and moves the whitespace filter into +`RichTextExpander`, where it is a policy of the per-node path rather than a property of +collecting. That keeps one traversal — and one shared answer to "which content is translatable" — +but it changes what the fingerprint hashes: `["Buy", " ", "our product"]` joins as +`"Buyour product"` today and `"Buy our product"` after. Every stored fingerprint would stop +matching, so D19 versions them and recomputes instead of declaring staleness. That migration is +the price of the single walk, and it is the one piece of work in this design that exists purely +because of an implementation detail rather than the feature. + +**Reference mutation survives.** The pipeline's contract — chunks carry live references into +the tree `DataReconciler` built, and the applicator mutates through them — does not change. +Only the level changes: `containerRef.children = [...]` instead of `nodeRef.text = ...`. No +stage downstream of the applicator learns anything new. + +**Nothing is copied at all.** The rebuilt `children` array holds the *same node objects* in a +new order — the only write into a node is still `node.text`. A clone would be needed for exactly +one model behaviour, a mark returned twice, and D13 rejects that reply instead of supporting it. +So there is one way the tree is built, not two. + +--- + +## 4. Mark contract + +### Emitting + +Walking a container's inline level produces, in document order, one fragment per text leaf and +one per non-text inline node: + +Each fragment carries a mark number and two live references, never a copy (D17): + +| Fragment | Emitted as | `node` (where the translation is written) | `top` (what goes into the rebuilt array) | +| --- | --- | --- | --- | +| text leaf, direct child of the container | `text` | the leaf | the same node | +| text leaf inside a link (or any wrapper chain) | `text` | the leaf | the container's **direct child** — the chain above the leaf rides along inside it | +| non-text inline node (line break, inline block, upload) | `` | — | the node | + +`top` is the container's direct child, not the leaf's immediate parent: with `mark → link → text` +the immediate parent is the link, and pushing that would drop the annotation. Nesting is never +rebuilt by hand — it travels inside `top` by reference. + +### Finding the container + +**A container is a node with at least one direct text child.** Walk from the root down: on a +node that qualifies, stop and take it whole (its nested inline wrappers included); otherwise +descend into its children. + +The rule deliberately names no node types, so it holds for paragraphs, headings, list items, +quotes — and for whatever Payload adds later. + +``` +paragraph ← container: has direct text children +├─ text "Buy " +├─ link → text "our product" the link becomes one fragment inside it +└─ text " today" + +list ← no direct text, descend +├─ listitem → text "first" ← container +└─ listitem → text "second" ← container (each item on its own, as it must be) + +quote → paragraph → text ← the paragraph is the container + +root +├─ paragraph ← container +├─ block (fields, not children) no text children, walked past — unchanged from today +└─ paragraph ← container +``` + +**Known limitation.** A container holding no direct text — a paragraph made of two adjacent +links, say — does not qualify, so each link becomes its own container with a single fragment and +takes the per-node path. The links keep their source order. Fixing that would mean either a list +of known block types (the Lexical knowledge this design avoids) or a depth rule, and depth +cannot work: text sits two levels deep both in a paragraph-with-link and in a list-with-items, +and collapsing list items into one fragment would be flatly wrong. Recorded as a limitation, to +be revisited if real content shows it is common (Q1). + +### Whitespace + +Editors routinely emit a node holding a single space: + +``` +paragraph +├─ text "Buy" +├─ text " " ← this one +└─ text "our product" formatted +``` + +Today's walk drops it (`collectTextNodes.ts:12`) — correct for translation, since a space needs +none — but the serialized string must keep it or the words collide as "Buyour product". + +Per D15 it is **glued onto the preceding fragment** (onto the following one when there is no +preceding), gets no mark, and its node drops out of the result: one node fewer in the tree, +visually identical. A mark of its own would risk coming back empty, and with it the gap. + +Per D16 the core then **restores edge whitespace after the reply**: a fragment whose source +started or ended with a space and whose translation does not gets it back. Non-empty fragments +only — merged fragments legitimately change their edges. + +### Accepting a reply — all or nothing + +**Every number issued must come back exactly once. Order is free, empty content is allowed, +anything else is corrupt.** One set comparison, no sub-cases — and rejecting a repeated mark is +what lets the whole design run without copying a single node (D17). + +| Reply | Verdict | +| --- | --- | +| Same set of numbers, any order | valid | +| A mark carries empty text | valid — the fragment merged into a neighbour; its node drops out of the rebuilt array | +| A mark appears more than once | corrupt — one node cannot sit in two slots without a copy, and the copy is the complexity D17 removes | +| Stray whitespace inside a mark (`< 1 >`) | valid — tolerated rather than losing the container over a space | +| **Any issued number is absent** | corrupt | +| A number we never issued appears | corrupt | +| Unclosed or crossed marks (`<1>text`) | corrupt | +| Nested marks (`<1><2>text`) | corrupt | +| No mark carries any text | corrupt | + +Merging is the case that makes strictness affordable. A translation legitimately turns three +fragments into two, and the model expresses that by returning the third mark **empty** rather +than dropping it. So "every number, exactly once" costs nothing that real translations need — +which is why the earlier bare-vs-wrapped distinction (was a link lost, or did words merge?) is +gone, along with the clone that a repeated mark would have required. + +Splitting a fragment in two is the one thing the model may not do. If it wants to, the container +takes the per-node path and reads as it does today. + +The instruction therefore has to say two things, and the second is easy to forget: **return every +mark, empty if its text moved elsewhere**, and **never introduce a mark into a value that had +none**. + +Corrupt ⇒ D7: the container is queued for a per-node retranslation, batched with every other +corrupt container into one additional request. If that reply is also unusable, the container is +left untranslated and reported — never half-written, because a half-written container with its +markup gone is the failure nobody notices. + +Verification runs in the translation stage, not the applicator: the retranslation needs the +provider, and the applicator has no access to it. The applicator receives fragments that are +already verified. + +### Marks are flat + +A mark denotes a **text leaf together with its whole wrapper chain**, not a markup element — so +nesting cannot arise by construction. A link containing an emphasised word (`read the **docs**`) +holds two leaves and therefore emits two flat, adjacent marks: + +``` +<4>read the <5>docs +``` + +Both name that link as their `top`; the second's leaf also carries the emphasis. The parser reads +left to right and needs no stack, and a nested mark in the reply is simply corrupt. + +The cost is D12: the same link node lands twice, adjacent, in the rebuilt array. Structure-mirroring marks +(`<4><5>docs`) would avoid that and cost a stack in the parser plus a whole class of +model errors — declined in §8. + +### Plain values carry no marks + +`text` and `textarea` fields travel exactly as they do today, unmarked. So does a container +holding a single unformatted text node (D6). The rule: **marks appear only where a value holds +more than one fragment.** + +One request therefore mixes marked and unmarked values, which the model can confuse. If marks +appear in the reply to a value that was sent unmarked, they are stripped and a warning is +emitted — the text itself is usually fine and losing it to the model's overreach would be worse. +Splitting marked and unmarked values into separate requests is declined in §8: an extra request +every time, and the whole-document context that keeps terminology consistent is exactly what it +would break. + +--- + +## 5. Where things go + +| Path | Change | +| --- | --- | +| `src/core/kernel/lexical/collectTextNodes.ts` | extended (D18): keeps whitespace-only nodes, returns each node's `top` alongside it | +| `src/core/kernel/lexical/collectInlineFragments.ts` | **new** — groups the walk's output into containers per D1 | +| `src/core/kernel/lexical/inlineMarks.ts` | **new** — serialize fragments to a marked string; parse a marked string back to `{ markId, text }[]`; pure, no Lexical knowledge | +| `src/core/translation-pipeline/types/TextChunk.ts` | `RichContainerChunk` joins `PlainTextChunk` and `RichTextChunk`, plus its guard | +| `src/core/translation-pipeline/stages/text-expander/RichContainerExpander.ts` | **new** — one chunk per container; falls back to `RichTextExpander` per D3/D6 | +| `src/core/translation-pipeline/stages/text-expander/TextChunkExpander.ts` | picks the expander by the configured granularity | +| `src/core/translation-pipeline/stages/translation/Translation.stage.ts` | parses and verifies marks, and owns the D7 retranslation pass — it is the stage holding the provider | +| `src/core/translation-pipeline/stages/translation-applicator/TranslationMutator.ts` | third branch: rebuild `children` from verified fragments; fast path per D5 | +| `src/core/domain/translation-providers/TranslationProvider.interface.ts` | optional 4th parameter (D10); optional `capabilities` (D9) | +| `src/translation-providers/shared/buildSystemPrompt.ts` | mark instruction appended after any override (D11) | +| `src/translation-providers/openai/openAIComplete.ts` | declares `capabilities.inlineMarks` | +| `src/core/translation-pipeline/stages/text-expander/RichTextExpander.ts` | takes over the whitespace filter (D18) | +| `src/core/domain/provenance/ProvenanceStore.interface.ts` · `src/server/modules/provenance/Provenance.collection.ts` | fingerprint version field (D19) | +| `src/core/domain/provenance/staleness.ts` · `src/core/domain/auto-translate/hasSourceContentChanged.ts` | recompute instead of declaring stale when the version is older (D19) | +| `README.md` | the option, the provider capability, the v1 limitation from D12 | + +`buildResponseSchema`, `parseAndValidateReply` and `runDryRun` are untouched. `leafSourceText` +keeps its `join("")` — only its input widens, which is exactly why D19 exists. + +--- + +## 6. Build sequence + +1. **Contract tests first, on a stub.** Serializer and parser tests written from §4, red before + any implementation: French adjective, German subordinate clause, link with two formatted + leaves, line break mid-paragraph, a mark-shaped sequence in the source text, a merge expressed + as an empty mark, marks appearing in a reply to an unmarked value, a whitespace-only node + between two words, a reply that trimmed an edge space, a paragraph of adjacent links, and every + row of the reply table. +2. **`collectInlineFragments` + `inlineMarks`** — pure, no pipeline wiring. Turn the tests green. +3. **`RichContainerChunk` + applicator branch** with the D5 fast path, behind the option still + defaulting to `"node"`. +4. **`RichContainerExpander`** and expander selection; D3 and D6 fallbacks. +5. **Provider seams** — capability declaration, 4th parameter, prompt instruction; assert marks + survive `runDryRun`. +6. **D7 retranslation pass.** +7. **Fingerprint version (D19)** — field, recompute-on-older-version branch, and a test proving + an upgrade does not flip existing translations to stale. Must land with step 2, not after it, + since step 2 is what changes the join. +8. **Docs**, then flip the default in the next major. + +--- + +## 7. Open questions + +1. **What real content actually contains** — two counts from a live project, not guesses. + (a) Which inline nodes appear inside paragraphs (`linebreak`, `inlineBlock`, mentions, + uploads): the self-closing rule covers them structurally, but corrupt-on-missing is strict, and + a node type models routinely swallow would send containers to the fallback often. + (b) How often a container holds no direct text child (a paragraph of adjacent links), which is + the limitation recorded in §4. Both counts decide whether the simple container rule stands. +2. **How often models drop empty marks** — D13 leans on the model returning a mark whose text + moved elsewhere as `` rather than omitting it. If models routinely omit instead, the + fallback rate makes the whole mode pointless. Measure before flipping any default: a hundred + real containers across the language pairs that matter, counting fallbacks. A high rate means + rewording the instruction, or softening D13 — not shipping and hoping. +3. **Provenance fingerprint direction** — `leafSourceText` + (`src/core/domain/content-projection/translatableLeaf.ts:41-49`) joins source text nodes with + `join("")`. Everything read so far says fingerprints are computed from the source side only, + which would make this change invisible to staleness. To be confirmed by test before step 3, + because if any fingerprint touches the target side, every existing translation flips to stale + on the first container-mode run. + +--- + +## 8. Considered and rejected + +| Option | Why not | +| --- | --- | +| Keep writing into the original nodes, move the formatting instead | Would require reading and writing `format`, and turning a text node into a link node with children. The layer would have to understand Lexical formatting — the one thing this design avoids | +| Structured output: an array of `{ mark, text }` per key | Cleaner than string parsing, but breaks `Record` and with it every hand-written provider | +| Real HTML tags instead of numeric marks | Models "improve" real tags — adding attributes, swapping synonyms. Numeric marks have nothing to improve and are trivial to validate | +| Escape `<` in source text | An escape sequence is itself something the model may "fix" — decode it, duplicate it, drop it. Two directions to implement and test, a new class of failure, and D3 already covers the content it would protect | +| Rare Unicode delimiters instead of angle marks | The model does not recognise them as markup, so it drops them far more often than tags it knows | +| Send neighbouring fields as context, keep per-node granularity | Improves term consistency, not grammar: a fragment still has no correct form outside its sentence, and the context rides along in every request | +| Merge adjacent fragments sharing one wrapper (D12) | Correct output without it; deferred so v1 stays small | +| Structure-mirroring nested marks | Needs a stack in the parser, and nesting is the thing models break most often. Flat marks have no nesting to break | +| Separate requests for marked and unmarked values | An extra request on every document, and it splits the whole-document context that makes terminology consistent | +| A `parents` array on each fragment | Rebuilding nesting by hand, when pushing `top` carries it by reference. Two references (`node`, `top`) say everything the applicator needs | +| Cloning a node so a repeated mark can occupy two slots | Buys one model behaviour nobody needs and pays with a clone path, a leaf-lookup inside the clone, and a second way for the tree to be built. D13 rejects the repeat instead | +| A second walk beside `collectSerializedLexicalTextNodes` | Two traversals drifting apart on "what is a text node", to avoid one fingerprint migration. D18/D19 take the migration | +| Tolerate a missing mark by inspecting what it wrapped | The branch it buys is only needed because merging had no legal encoding; with empty marks it has one | diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts index e6bb91038..485f6fb71 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsRunnerProvider.ts @@ -1,7 +1,12 @@ import type { Config, Field, Payload, WorkflowConfig } from "payload"; import type { TaskRunner } from "../TaskRunner.interface"; -import type { PayloadJobsRunnerOptions, PayloadJobsRunnerConfig, AutoRunConfig } from "./types"; +import type { + PayloadJobsRunnerOptions, + PayloadJobsRunnerConfig, + AutoRunConfig, + StoredWorkflowInput, +} from "./types"; import { PayloadJobsTaskRunner } from "./PayloadJobsTaskRunner"; import { readCollectionRef } from "./readCollectionRef"; import type { TaskRunnerContext, TaskRunnerProvider } from "../TaskRunnerProvider.interface"; @@ -12,11 +17,7 @@ const defaultAutoRun: Required = { limit: 50, }; -type StoredWorkflowInput = { - collection_slug?: string; - collection_id?: string; - target_lngs?: string[]; -} & Record; +type StoredJobInput = Partial & Record; type RunLocaleTask = (taskID: string, args: { input: Record }) => Promise; const DEFAULT_STALE_JOB_TIMEOUT_MS = 5 * 60 * 1000; @@ -154,7 +155,7 @@ export class PayloadJobsRunnerProvider implements TaskRunnerProvider { }, }; - const workflow: WorkflowConfig = { + const workflow: WorkflowConfig = { slug: workflowName, inputSchema: workflowInputSchema, retries, diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts index 6f2e4def9..4d8a6b254 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts @@ -3,14 +3,20 @@ import type { Payload, Where, CollectionSlug } from "payload"; import type { TaskFilter, TaskRunner } from "../TaskRunner.interface"; import { toTaskFilter } from "../toTaskFilter"; import type { Task, TaskInput, RunResult } from "../types"; -import type { PayloadJobsRunnerConfig, PayloadJob } from "./types"; -import { normalizeJob, normalizeJobLocales } from "./normalizeJob"; +import type { PayloadJobsRunnerConfig, PayloadJob, StoredWorkflowInput } from "./types"; +import { normalizeJobLocales } from "./normalizeJob"; import { planEnqueue } from "./planEnqueue"; import type { RequestShape } from "./planEnqueue"; import { readCollectionRef } from "./readCollectionRef"; const APPEND_ATTEMPTS = 2; +/** + * Groups served at once: a `select_all` enqueue can span thousands, each costing two writes and a + * read, and the ceiling is the database pool rather than the CPU. + */ +const ENQUEUE_CONCURRENCY = 10; + type QueueWorkflow = (args: { workflow: string; queue: string; @@ -18,15 +24,6 @@ type QueueWorkflow = (args: { input: StoredWorkflowInput; }) => Promise; -type StoredWorkflowInput = { - collection_slug: CollectionSlug; - collection_id: string; - source_lng: string; - target_lngs: string[]; - strategy: string; - publish_on_translation: boolean; -}; - function requestShape(task: TaskInput): RequestShape { return { collectionSlug: task.collectionSlug, @@ -37,21 +34,15 @@ function requestShape(task: TaskInput): RequestShape { }; } +/** `pickHost` matches on the same shape, so no two groups can pick the same host job — which is what + * makes the parallel `serve` calls safe. */ function requestKey(task: TaskInput): string { - const r = requestShape(task); - // NUL: no stored field value can contain it, so two different requests cannot produce one key. - return [ - r.collectionSlug, - r.collectionId, - r.sourceLng, - r.strategy, - String(r.publishOnTranslation), - ].join("\u0000"); + return JSON.stringify(requestShape(task)); } -function sameDocument(job: PayloadJob, request: RequestShape): boolean { - const { collectionSlug, collectionId } = readCollectionRef(job.input); - return collectionSlug === request.collectionSlug && collectionId === request.collectionId; +function documentKey(collectionSlug: string, collectionId: string): string { + // NUL: no slug or id can contain it, so two different documents cannot produce one key. + return `${collectionSlug}\u0000${collectionId}`; } export class PayloadJobsTaskRunner implements TaskRunner { @@ -69,25 +60,34 @@ export class PayloadJobsTaskRunner implements TaskRunner { byRequest.set(key, group); } - const live = await this.findRawJobs({ completedAt: { exists: false } }, { pagination: false }); + const live = await this.findRawJobs({ completedAt: { exists: false } }); + const liveByDocument = new Map(); + for (const job of live) { + const { collectionSlug, collectionId } = readCollectionRef(job.input); + const key = documentKey(collectionSlug, collectionId); + liveByDocument.set(key, [...(liveByDocument.get(key) ?? []), job]); + } const exclusiveQueue = Boolean(this.payload.config.jobs?.enableConcurrencyControl); - // At most one group can match any live job — `pickHost` requires the same source locale, strategy - // and publish flag — so no two of these writes touch the same row. - await Promise.all( - [...byRequest.values()].map((group) => this.serve(group, live, exclusiveQueue)) - ); + const groups = [...byRequest.values()]; + for (let i = 0; i < groups.length; i += ENQUEUE_CONCURRENCY) { + await Promise.all( + groups + .slice(i, i + ENQUEUE_CONCURRENCY) + .map((group) => this.serve(group, liveByDocument, exclusiveQueue)) + ); + } } private async serve( group: TaskInput[], - live: PayloadJob[], + liveByDocument: Map, exclusiveQueue: boolean ): Promise { const [first] = group; const request = requestShape(first); const plan = planEnqueue({ - live: live.filter((job) => sameDocument(job, request)), + live: liveByDocument.get(documentKey(request.collectionSlug, request.collectionId)) ?? [], request, requested: group.map((t) => t.targetLng), exclusiveQueue, @@ -100,40 +100,38 @@ export class PayloadJobsTaskRunner implements TaskRunner { if (queue.length > 0) await this.queueWorkflow(request, queue, first.waitUntil); } - /** @returns the locales that did not reach the job and need one of their own. */ private async extendJob(job: PayloadJob, locales: string[], waitUntil?: Date): Promise { - let current: PayloadJob | undefined = job; + let current = job; + let undelivered = locales; // `input` is one JSON column, so a concurrent append replaces the whole list; the union makes a // retry from the stored row harmless. for (let attempt = 0; attempt < APPEND_ATTEMPTS; attempt++) { - if (!current || current.completedAt) return locales; - const listed = current.input?.target_lngs ?? []; const missing = locales.filter((locale) => !listed.includes(locale)); const debounce = waitUntil && !current.processing ? waitUntil.toISOString() : undefined; if (missing.length === 0 && !debounce) return []; - const data: Record = { - input: { ...current.input, target_lngs: [...listed, ...missing] }, - ...(debounce ? { waitUntil: debounce } : {}), - }; - // Not `payload.update`: it rewrites the whole row and reverts log entries written in between. // See D2 of docs/plans/2026-09-08-one-live-job-per-document.task.md. await this.payload.db.updateOne({ collection: this.config.jobsCollection, id: job.id, - data, + data: { + input: { ...current.input, target_lngs: [...listed, ...missing] }, + ...(debounce ? { waitUntil: debounce } : {}), + }, returning: false, }); - [current] = await this.findRawJobs({ id: { equals: job.id } }, { limit: 1 }); - if (!current || current.completedAt) return locales; + const reread = await this.findJobById(job.id); + if (!reread || reread.completedAt) return locales; + current = reread; + const stored = new Set(current.input?.target_lngs); - if (locales.every((locale) => stored.has(locale))) return []; + undelivered = locales.filter((locale) => !stored.has(locale)); + if (undelivered.length === 0) return []; } - const stored = new Set(current?.input?.target_lngs); - return locales.filter((locale) => !stored.has(locale)); + return undelivered; } private async queueWorkflow( @@ -142,7 +140,7 @@ export class PayloadJobsTaskRunner implements TaskRunner { waitUntil?: Date ): Promise { const input: StoredWorkflowInput = { - collection_slug: request.collectionSlug as CollectionSlug, + collection_slug: request.collectionSlug, collection_id: request.collectionId, source_lng: request.sourceLng, target_lngs: targetLngs, @@ -163,29 +161,38 @@ export class PayloadJobsTaskRunner implements TaskRunner { async cancel(taskIds: string[]): Promise { if (taskIds.length === 0) return; - await this.cancelAndDeleteJobs(taskIds); + + // Mark then delete: the delete alone would take the row out of the status feed under + // `deleteJobOnComplete: false` without recording why it went. The mark does not reach a running + // handler — see D1 of docs/plans/2026-09-08-one-live-job-per-document.task.md. + await this.payload.jobs.cancel({ + where: { id: { in: taskIds } }, + queue: this.config.queueName, + }); + + await this.payload.delete({ + collection: this.config.jobsCollection, + where: { and: [this.ownJobs(), { id: { in: taskIds } }] }, + }); } async run(taskId: string): Promise { - const [job] = await this.findRawJobs({ id: { equals: taskId } }, { limit: 1 }); + const job = await this.findJobById(taskId); if (!job) { return { success: false, error: "not_found" }; } - const task = normalizeJob(job); - if (task.completedAt) { + if (job.completedAt) { return { success: false, error: "already_completed" }; } - if (task.status === "running" && !this.isStale(task.updatedAt)) { + if (job.processing && !this.isStale(job.updatedAt)) { return { success: false, error: "already_running" }; } - if (task.status === "running" || task.status === "failed") { + if (job.processing || job.error) { await this.clearPickerBlockers(taskId); } - // `where` picker, not `payload.jobs.runByID({ id })`: in `runJobs` the guard block - // (processing:false, hasError not true, waitUntil due) is built only for the non-id branch, so - // the id path would re-run a job that already exhausted its retries. Checked against payload - // 3.84.1. + // Not `jobs.runByID`: payload 3.84.1 builds the picker guard (processing / hasError / waitUntil) + // only on the `where` path, so the id path re-runs a job that exhausted its retries. const result = (await this.payload.jobs.run({ queue: this.config.queueName, where: { id: { equals: taskId } }, @@ -200,21 +207,26 @@ export class PayloadJobsTaskRunner implements TaskRunner { } /** - * Clears stale `processing` locks so abandoned jobs are eligible for the autorun picker again. - * A job that exhausted its retries carries `hasError: true` and stays excluded even after its lock - * is cleared; only a manual `run()` recovers it. + * A job that exhausted its retries carries `hasError: true` and stays excluded from the picker even + * after its lock is cleared; only a manual `run()` recovers it. * @returns how many locks were cleared. */ async reclaimStaleJobs(): Promise { const cutoff = new Date(Date.now() - this.config.staleJobTimeoutMs).toISOString(); - return this.resetProcessing({ - and: [ - this.ownJobs(), - { processing: { equals: true } }, - { completedAt: { exists: false } }, - { updatedAt: { less_than: cutoff } }, - ], + const result = await this.payload.update({ + collection: this.config.jobsCollection, + depth: 0, + where: { + and: [ + this.ownJobs(), + { processing: { equals: true } }, + { completedAt: { exists: false } }, + { updatedAt: { less_than: cutoff } }, + ], + }, + data: { processing: false }, }); + return result.docs.length; } /** @@ -230,30 +242,16 @@ export class PayloadJobsTaskRunner implements TaskRunner { }); } - /** `depth: 0` — only the row count is read. */ - private async resetProcessing(where: Where): Promise { - const result = await this.payload.update({ - collection: this.config.jobsCollection, - depth: 0, - where, - data: { processing: false }, - }); - return result.docs.length; - } - private isStale(updatedAt: string): boolean { const parsed = Date.parse(updatedAt); - // Unknown/corrupt timestamp → treat as stale so the job can be recovered - // rather than permanently refused as already-running. if (Number.isNaN(parsed)) return true; return Date.now() - parsed > this.config.staleJobTimeoutMs; } /** - * Only the job slugs and `completedAt` reach the database; the collection slug and document ids are - * matched in memory, because a job's collection reference may sit in the flat-text fields or in the - * legacy relationship shape (`readCollectionRef`) — a `where` on `input.collection_slug` would - * silently drop every pre-migration job. `excludeCompleted` bounds the read (#108). + * Matched in memory, not in a `where`: a job's collection reference sits either in the flat text + * fields or in the legacy relationship shape (see `readCollectionRef`), so `input.collection_slug` + * as a filter would silently drop every pre-migration job. */ async findByCollection( collectionSlug: CollectionSlug, @@ -261,29 +259,14 @@ export class PayloadJobsTaskRunner implements TaskRunner { ): Promise { const { documentIds, excludeCompleted } = toTaskFilter(filter); const where = excludeCompleted ? { completedAt: { exists: false } } : undefined; - const jobs = await this.findRawJobs(where, { pagination: false }); - const all = jobs.flatMap(normalizeJobLocales); - const bySlug = all.filter((t) => t.input.collectionSlug === collectionSlug); - if (!documentIds?.length) return bySlug; - const wanted = new Set(documentIds.map(String)); - return bySlug.filter((t) => wanted.has(t.input.collectionId)); - } - - private async cancelAndDeleteJobs(taskIds: string[]): Promise { - if (taskIds.length === 0) return; - - // Both, in order: `jobs.cancel` only marks the row (`error.cancelled`), which is the signal a - // running handler aborts on; the delete then keeps it out of the status feed under - // `deleteJobOnComplete: false`. - await this.payload.jobs.cancel({ - where: { id: { in: taskIds } }, - queue: this.config.queueName, - }); - - await this.payload.delete({ - collection: this.config.jobsCollection, - where: { and: [this.ownJobs(), { id: { in: taskIds } }] }, - }); + const wanted = documentIds?.length ? new Set(documentIds.map(String)) : undefined; + const jobs = await this.findRawJobs(where); + return jobs + .filter((job) => { + const ref = readCollectionRef(job.input); + return ref.collectionSlug === collectionSlug && (!wanted || wanted.has(ref.collectionId)); + }) + .flatMap(normalizeJobLocales); } private ownJobs(): Where { @@ -296,10 +279,12 @@ export class PayloadJobsTaskRunner implements TaskRunner { }; } - private async findRawJobs( - where?: Where, - params?: { limit?: number; pagination?: boolean } - ): Promise { + private async findJobById(id: string): Promise { + const [job] = await this.findRawJobs({ id: { equals: id } }); + return job; + } + + private async findRawJobs(where?: Where): Promise { const and: Where[] = [this.ownJobs()]; if (where) and.push(where); @@ -308,8 +293,7 @@ export class PayloadJobsTaskRunner implements TaskRunner { // The legacy `input.collection` is a declared relationship; at the default depth Payload // populates it, and `readCollectionRef` would then read a document where it wants an id. depth: 0, - limit: params?.limit, - pagination: params?.pagination, + pagination: false, where: { and }, }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts index 6b42bed56..cfdb8725a 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/planEnqueue.ts @@ -1,9 +1,11 @@ +import type { CollectionSlug } from "payload"; + import { isCancelled, latestLogByLocale } from "./normalizeJob"; import type { PayloadJob } from "./types"; /** The part of a request that every locale in it shares. */ export type RequestShape = { - collectionSlug: string; + collectionSlug: CollectionSlug; collectionId: string; sourceLng: string; strategy: string; diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts index fd4a9410f..fbf79f851 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/types.ts @@ -110,6 +110,16 @@ export type PayloadJob = { }; }; +/** Snake_case because Payload persists these keys verbatim in the job row. */ +export type StoredWorkflowInput = { + collection_slug: CollectionSlug; + collection_id: string; + source_lng: string; + target_lngs: string[]; + strategy: string; + publish_on_translation: boolean; +}; + /** Written by Payload only once a task settles — an absent entry means that locale has not run. */ export type JobLogEntry = { state: "succeeded" | "failed"; From 23fc243bed856fa010043bcf263c3126162879b2 Mon Sep 17 00:00:00 2001 From: Siarhei Date: Tue, 8 Sep 2026 16:49:14 +0200 Subject: [PATCH 7/7] chore: untrack an unrelated design draft picked up by mistake --- ...8-richtext-container-granularity-design.md | 343 ------------------ 1 file changed, 343 deletions(-) delete mode 100644 packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md diff --git a/packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md b/packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md deleted file mode 100644 index 2b8348667..000000000 --- a/packages/payload-plugin-translator/docs/plans/2026-09-08-richtext-container-granularity-design.md +++ /dev/null @@ -1,343 +0,0 @@ -# Design — rich text container granularity (#134) - -- **Issue:** [#134](https://github.com/focusreactive/payload-plugins/issues/134) -- **Date:** 2026-09-08 · **Status:** decided, two questions open (§7) - ---- - -## 1. What is being built - -The unit of rich text translation moves from the **text node** to its **inline container** — -the paragraph, heading, list item or quote that holds it. Fragments inside a container are -wrapped in numbered marks, the model returns them in whatever order the target language -needs, and the container's `children` array is rebuilt in that order. - -Today each text node is translated alone and written back into the node it came from, so the -node order never changes: the translation keeps the source language's word order, and every -inline mark stays pinned to its source position. `a **red** car` becomes *une rouge voiture* -instead of *une voiture **rouge***, and no prompt can fix it — the model is never given the -chance to reorder anything. - -A mark number is **a pointer to the original inline node, not an address to write into**. -That single idea is what keeps this layer from having to understand Lexical formatting: -formatting travels inside the nodes themselves. - -``` -sent: { 7: "<1>a <2>red<3> car" } -returned: { 7: "<1>une <3>voiture <2>rouge" } -applied: children = [ node1("une "), node3("voiture "), node2("rouge") ] -``` - -The boundary between core and provider is the string. The core finds containers, glues -whitespace, emits marks, and afterwards parses, verifies and rebuilds; the provider takes a -string and returns a string, knowing nothing about nodes, gaps or Lexical. - -Reads stay within `type` / `text` / `children`. Writes are `text` on a leaf and `children` -on the container. `format`, `style`, `detail`, a link's `fields` and every other mark -representation are never read and never written — the same surface `kernel/lexical/types.ts` -already declares. - ---- - -## 2. Decisions - -| # | Decision | Why | -| --- | --- | --- | -| D1 | Unit of translation is the nearest node with at least one direct text child | Needs no knowledge of which node types are inline. Exotic trees degrade to today's behaviour instead of breaking | -| D2 | Mark syntax is `text`, self-closing `` for non-text inline nodes, always flat | Models have seen XLIFF and HTML; numeric names cannot collide with meaningful tags; flatness keeps the parser stackless and removes nesting as a failure mode | -| D3 | A container whose source text contains a mark-shaped sequence (`<12>`, ``, `<12/>`) falls back to per-node granularity | Removes the whole escaping problem for the price of losing the optimisation on content that is close to nonexistent. Plain `
` or `5 < 10` are unaffected — only digits between angle brackets collide | -| D4 | Every fragment is wrapped, including unformatted ones | The layer then never constructs a node from scratch — every output node is an existing node with new text | -| D5 | Marks returned in their original order take the current write-into-the-node path | Most content, and all close-language pairs, land here. The new path runs only where it changes the result | -| D6 | A container holding a single unformatted text node emits no marks at all | Typical paragraph pays nothing — not a single extra token | -| D7 | Corrupt reply for a container ⇒ that container is retranslated per-node in one extra batched request | Degrades to today's quality rather than silently dropping inline marks. Pouring the whole translation into the first node would be *worse* than today — every link and emphasis inside that container lost. Bounded: one extra pass, never a loop; a second unusable reply leaves the container untranslated and reported | -| D8 | Granularity is an option, defaulting to `"node"` for this major version | Existing installs, including ones with hand-written providers, keep byte-identical output until they opt in | -| D9 | A provider opts in by declaring `capabilities.inlineMarks` | A machine-translation API behind `CompletionFn` would translate or strip marks. Absent declaration ⇒ per-node, whatever the option says | -| D10 | `translate()` gains an optional 4th parameter carrying which keys hold marks | One request mixes marked containers with plain `text` fields. Optional, so existing implementations are unaffected | -| D11 | The mark instruction is appended to the system prompt by the core, after any override | A `SystemPromptBuilder` that ignores `defaultPrompt` must not be able to drop the one instruction the format depends on | -| D12 | Adjacent fragments sharing one wrapper are not merged in v1 | Two text leaves inside one link keep that link as their `top`, so the rebuilt array holds it twice — the same node, adjacent. Rendering is unchanged; merging is polish, not correctness | -| D13 | Every issued number must come back **exactly once**; order is free, empty content is how a merge is expressed | One set comparison, no occurrence counting. Rejecting a repeated mark is what removes copying from the design entirely (D17) | -| D14 | Plain values and single-node containers are sent unmarked; stray marks in their replies are stripped and warned | No markup inside them to preserve, so marks would be pure token cost | -| D15 | Whitespace-only nodes are glued onto the preceding fragment, never marked on their own | A mark of its own can come back empty, and the gap between two words would be gone. Glued, the space rides inside a fragment that carries text | -| D16 | Edge whitespace is restored by the core after the reply, not by the provider | Models trim edges. The provider's contract is string in, string out — it must not know about nodes or gaps. This is exactly the logic that ossified as a "Fix spaces" patch inside the Storyblok plugin's model call | -| D17 | Nothing is copied: a fragment holds two live references — `node` (the text leaf) and `top` (the container's direct child) | Applying a translation is then `node.text = ...` plus a reordered `children` array. A repeated mark is the only case that would need a clone, and D13 rejects it | -| D18 | One walk, not two: `collectSerializedLexicalTextNodes` returns every text node (whitespace included) plus each one's `node`/`top` pair; the whitespace filter moves into `RichTextExpander` | Collecting is the walk's job, deciding what not to translate is the caller's. Removes a duplicate traversal, at the cost of D19 | -| D19 | Provenance records gain a fingerprint version; a record written under an older version is recomputed, not declared stale | D18 changes the join (`"Buyour product"` → `"Buy our product"`), so every stored fingerprint would stop matching — the admin would show every translation stale and auto-translate would retranslate everything at the customer's expense. `dismissedFingerprint` was added ahead of need for the same reason | - ---- - -## 3. Three facts that shaped this - -**The text-node walk serves two subsystems, on purpose.** -`collectSerializedLexicalTextNodes` has two callers that ask it different questions: - -| Caller | Question it answers | -| --- | --- | -| `RichTextExpander` (translation pipeline) | which pieces of text to send to the model | -| `leafSourceText` → `projectTranslatableContent` → `fingerprint` (provenance) | what counts as a field's content, so a later run can tell whether the source changed | - -The sharing is deliberate — `contentProjector.ts:5-8` says why: projection and translation reuse -one traversal and one leaf predicate *"so projection and translation can never disagree on which -content is translatable"*. The second caller's output is hashed and **stored** in the provenance -store at translation time; `staleness.ts` compares that stored value against a freshly computed -one to decide whether a translation is stale (the admin indicator, and auto-translate's -source-changed check). - -Container mode needs two things the walk does not currently give: whitespace-only nodes (it -filters them out, and they hold the gaps between words) and, per text node, the container's -direct child above it (the flat `{ node }` return cannot say a leaf sat inside a link). - -Rather than a second walk, D18 extends this one and moves the whitespace filter into -`RichTextExpander`, where it is a policy of the per-node path rather than a property of -collecting. That keeps one traversal — and one shared answer to "which content is translatable" — -but it changes what the fingerprint hashes: `["Buy", " ", "our product"]` joins as -`"Buyour product"` today and `"Buy our product"` after. Every stored fingerprint would stop -matching, so D19 versions them and recomputes instead of declaring staleness. That migration is -the price of the single walk, and it is the one piece of work in this design that exists purely -because of an implementation detail rather than the feature. - -**Reference mutation survives.** The pipeline's contract — chunks carry live references into -the tree `DataReconciler` built, and the applicator mutates through them — does not change. -Only the level changes: `containerRef.children = [...]` instead of `nodeRef.text = ...`. No -stage downstream of the applicator learns anything new. - -**Nothing is copied at all.** The rebuilt `children` array holds the *same node objects* in a -new order — the only write into a node is still `node.text`. A clone would be needed for exactly -one model behaviour, a mark returned twice, and D13 rejects that reply instead of supporting it. -So there is one way the tree is built, not two. - ---- - -## 4. Mark contract - -### Emitting - -Walking a container's inline level produces, in document order, one fragment per text leaf and -one per non-text inline node: - -Each fragment carries a mark number and two live references, never a copy (D17): - -| Fragment | Emitted as | `node` (where the translation is written) | `top` (what goes into the rebuilt array) | -| --- | --- | --- | --- | -| text leaf, direct child of the container | `text` | the leaf | the same node | -| text leaf inside a link (or any wrapper chain) | `text` | the leaf | the container's **direct child** — the chain above the leaf rides along inside it | -| non-text inline node (line break, inline block, upload) | `` | — | the node | - -`top` is the container's direct child, not the leaf's immediate parent: with `mark → link → text` -the immediate parent is the link, and pushing that would drop the annotation. Nesting is never -rebuilt by hand — it travels inside `top` by reference. - -### Finding the container - -**A container is a node with at least one direct text child.** Walk from the root down: on a -node that qualifies, stop and take it whole (its nested inline wrappers included); otherwise -descend into its children. - -The rule deliberately names no node types, so it holds for paragraphs, headings, list items, -quotes — and for whatever Payload adds later. - -``` -paragraph ← container: has direct text children -├─ text "Buy " -├─ link → text "our product" the link becomes one fragment inside it -└─ text " today" - -list ← no direct text, descend -├─ listitem → text "first" ← container -└─ listitem → text "second" ← container (each item on its own, as it must be) - -quote → paragraph → text ← the paragraph is the container - -root -├─ paragraph ← container -├─ block (fields, not children) no text children, walked past — unchanged from today -└─ paragraph ← container -``` - -**Known limitation.** A container holding no direct text — a paragraph made of two adjacent -links, say — does not qualify, so each link becomes its own container with a single fragment and -takes the per-node path. The links keep their source order. Fixing that would mean either a list -of known block types (the Lexical knowledge this design avoids) or a depth rule, and depth -cannot work: text sits two levels deep both in a paragraph-with-link and in a list-with-items, -and collapsing list items into one fragment would be flatly wrong. Recorded as a limitation, to -be revisited if real content shows it is common (Q1). - -### Whitespace - -Editors routinely emit a node holding a single space: - -``` -paragraph -├─ text "Buy" -├─ text " " ← this one -└─ text "our product" formatted -``` - -Today's walk drops it (`collectTextNodes.ts:12`) — correct for translation, since a space needs -none — but the serialized string must keep it or the words collide as "Buyour product". - -Per D15 it is **glued onto the preceding fragment** (onto the following one when there is no -preceding), gets no mark, and its node drops out of the result: one node fewer in the tree, -visually identical. A mark of its own would risk coming back empty, and with it the gap. - -Per D16 the core then **restores edge whitespace after the reply**: a fragment whose source -started or ended with a space and whose translation does not gets it back. Non-empty fragments -only — merged fragments legitimately change their edges. - -### Accepting a reply — all or nothing - -**Every number issued must come back exactly once. Order is free, empty content is allowed, -anything else is corrupt.** One set comparison, no sub-cases — and rejecting a repeated mark is -what lets the whole design run without copying a single node (D17). - -| Reply | Verdict | -| --- | --- | -| Same set of numbers, any order | valid | -| A mark carries empty text | valid — the fragment merged into a neighbour; its node drops out of the rebuilt array | -| A mark appears more than once | corrupt — one node cannot sit in two slots without a copy, and the copy is the complexity D17 removes | -| Stray whitespace inside a mark (`< 1 >`) | valid — tolerated rather than losing the container over a space | -| **Any issued number is absent** | corrupt | -| A number we never issued appears | corrupt | -| Unclosed or crossed marks (`<1>text`) | corrupt | -| Nested marks (`<1><2>text`) | corrupt | -| No mark carries any text | corrupt | - -Merging is the case that makes strictness affordable. A translation legitimately turns three -fragments into two, and the model expresses that by returning the third mark **empty** rather -than dropping it. So "every number, exactly once" costs nothing that real translations need — -which is why the earlier bare-vs-wrapped distinction (was a link lost, or did words merge?) is -gone, along with the clone that a repeated mark would have required. - -Splitting a fragment in two is the one thing the model may not do. If it wants to, the container -takes the per-node path and reads as it does today. - -The instruction therefore has to say two things, and the second is easy to forget: **return every -mark, empty if its text moved elsewhere**, and **never introduce a mark into a value that had -none**. - -Corrupt ⇒ D7: the container is queued for a per-node retranslation, batched with every other -corrupt container into one additional request. If that reply is also unusable, the container is -left untranslated and reported — never half-written, because a half-written container with its -markup gone is the failure nobody notices. - -Verification runs in the translation stage, not the applicator: the retranslation needs the -provider, and the applicator has no access to it. The applicator receives fragments that are -already verified. - -### Marks are flat - -A mark denotes a **text leaf together with its whole wrapper chain**, not a markup element — so -nesting cannot arise by construction. A link containing an emphasised word (`read the **docs**`) -holds two leaves and therefore emits two flat, adjacent marks: - -``` -<4>read the <5>docs -``` - -Both name that link as their `top`; the second's leaf also carries the emphasis. The parser reads -left to right and needs no stack, and a nested mark in the reply is simply corrupt. - -The cost is D12: the same link node lands twice, adjacent, in the rebuilt array. Structure-mirroring marks -(`<4><5>docs`) would avoid that and cost a stack in the parser plus a whole class of -model errors — declined in §8. - -### Plain values carry no marks - -`text` and `textarea` fields travel exactly as they do today, unmarked. So does a container -holding a single unformatted text node (D6). The rule: **marks appear only where a value holds -more than one fragment.** - -One request therefore mixes marked and unmarked values, which the model can confuse. If marks -appear in the reply to a value that was sent unmarked, they are stripped and a warning is -emitted — the text itself is usually fine and losing it to the model's overreach would be worse. -Splitting marked and unmarked values into separate requests is declined in §8: an extra request -every time, and the whole-document context that keeps terminology consistent is exactly what it -would break. - ---- - -## 5. Where things go - -| Path | Change | -| --- | --- | -| `src/core/kernel/lexical/collectTextNodes.ts` | extended (D18): keeps whitespace-only nodes, returns each node's `top` alongside it | -| `src/core/kernel/lexical/collectInlineFragments.ts` | **new** — groups the walk's output into containers per D1 | -| `src/core/kernel/lexical/inlineMarks.ts` | **new** — serialize fragments to a marked string; parse a marked string back to `{ markId, text }[]`; pure, no Lexical knowledge | -| `src/core/translation-pipeline/types/TextChunk.ts` | `RichContainerChunk` joins `PlainTextChunk` and `RichTextChunk`, plus its guard | -| `src/core/translation-pipeline/stages/text-expander/RichContainerExpander.ts` | **new** — one chunk per container; falls back to `RichTextExpander` per D3/D6 | -| `src/core/translation-pipeline/stages/text-expander/TextChunkExpander.ts` | picks the expander by the configured granularity | -| `src/core/translation-pipeline/stages/translation/Translation.stage.ts` | parses and verifies marks, and owns the D7 retranslation pass — it is the stage holding the provider | -| `src/core/translation-pipeline/stages/translation-applicator/TranslationMutator.ts` | third branch: rebuild `children` from verified fragments; fast path per D5 | -| `src/core/domain/translation-providers/TranslationProvider.interface.ts` | optional 4th parameter (D10); optional `capabilities` (D9) | -| `src/translation-providers/shared/buildSystemPrompt.ts` | mark instruction appended after any override (D11) | -| `src/translation-providers/openai/openAIComplete.ts` | declares `capabilities.inlineMarks` | -| `src/core/translation-pipeline/stages/text-expander/RichTextExpander.ts` | takes over the whitespace filter (D18) | -| `src/core/domain/provenance/ProvenanceStore.interface.ts` · `src/server/modules/provenance/Provenance.collection.ts` | fingerprint version field (D19) | -| `src/core/domain/provenance/staleness.ts` · `src/core/domain/auto-translate/hasSourceContentChanged.ts` | recompute instead of declaring stale when the version is older (D19) | -| `README.md` | the option, the provider capability, the v1 limitation from D12 | - -`buildResponseSchema`, `parseAndValidateReply` and `runDryRun` are untouched. `leafSourceText` -keeps its `join("")` — only its input widens, which is exactly why D19 exists. - ---- - -## 6. Build sequence - -1. **Contract tests first, on a stub.** Serializer and parser tests written from §4, red before - any implementation: French adjective, German subordinate clause, link with two formatted - leaves, line break mid-paragraph, a mark-shaped sequence in the source text, a merge expressed - as an empty mark, marks appearing in a reply to an unmarked value, a whitespace-only node - between two words, a reply that trimmed an edge space, a paragraph of adjacent links, and every - row of the reply table. -2. **`collectInlineFragments` + `inlineMarks`** — pure, no pipeline wiring. Turn the tests green. -3. **`RichContainerChunk` + applicator branch** with the D5 fast path, behind the option still - defaulting to `"node"`. -4. **`RichContainerExpander`** and expander selection; D3 and D6 fallbacks. -5. **Provider seams** — capability declaration, 4th parameter, prompt instruction; assert marks - survive `runDryRun`. -6. **D7 retranslation pass.** -7. **Fingerprint version (D19)** — field, recompute-on-older-version branch, and a test proving - an upgrade does not flip existing translations to stale. Must land with step 2, not after it, - since step 2 is what changes the join. -8. **Docs**, then flip the default in the next major. - ---- - -## 7. Open questions - -1. **What real content actually contains** — two counts from a live project, not guesses. - (a) Which inline nodes appear inside paragraphs (`linebreak`, `inlineBlock`, mentions, - uploads): the self-closing rule covers them structurally, but corrupt-on-missing is strict, and - a node type models routinely swallow would send containers to the fallback often. - (b) How often a container holds no direct text child (a paragraph of adjacent links), which is - the limitation recorded in §4. Both counts decide whether the simple container rule stands. -2. **How often models drop empty marks** — D13 leans on the model returning a mark whose text - moved elsewhere as `` rather than omitting it. If models routinely omit instead, the - fallback rate makes the whole mode pointless. Measure before flipping any default: a hundred - real containers across the language pairs that matter, counting fallbacks. A high rate means - rewording the instruction, or softening D13 — not shipping and hoping. -3. **Provenance fingerprint direction** — `leafSourceText` - (`src/core/domain/content-projection/translatableLeaf.ts:41-49`) joins source text nodes with - `join("")`. Everything read so far says fingerprints are computed from the source side only, - which would make this change invisible to staleness. To be confirmed by test before step 3, - because if any fingerprint touches the target side, every existing translation flips to stale - on the first container-mode run. - ---- - -## 8. Considered and rejected - -| Option | Why not | -| --- | --- | -| Keep writing into the original nodes, move the formatting instead | Would require reading and writing `format`, and turning a text node into a link node with children. The layer would have to understand Lexical formatting — the one thing this design avoids | -| Structured output: an array of `{ mark, text }` per key | Cleaner than string parsing, but breaks `Record` and with it every hand-written provider | -| Real HTML tags instead of numeric marks | Models "improve" real tags — adding attributes, swapping synonyms. Numeric marks have nothing to improve and are trivial to validate | -| Escape `<` in source text | An escape sequence is itself something the model may "fix" — decode it, duplicate it, drop it. Two directions to implement and test, a new class of failure, and D3 already covers the content it would protect | -| Rare Unicode delimiters instead of angle marks | The model does not recognise them as markup, so it drops them far more often than tags it knows | -| Send neighbouring fields as context, keep per-node granularity | Improves term consistency, not grammar: a fragment still has no correct form outside its sentence, and the context rides along in every request | -| Merge adjacent fragments sharing one wrapper (D12) | Correct output without it; deferred so v1 stays small | -| Structure-mirroring nested marks | Needs a stack in the parser, and nesting is the thing models break most often. Flat marks have no nesting to break | -| Separate requests for marked and unmarked values | An extra request on every document, and it splits the whole-document context that makes terminology consistent | -| A `parents` array on each fragment | Rebuilding nesting by hand, when pushing `top` carries it by reference. Two references (`node`, `top`) say everything the applicator needs | -| Cloning a node so a repeated mark can occupy two slots | Buys one model behaviour nobody needs and pays with a clone path, a leaf-lookup inside the clone, and a second way for the tree to be built. D13 rejects the repeat instead | -| A second walk beside `collectSerializedLexicalTextNodes` | Two traversals drifting apart on "what is a text node", to avoid one fingerprint migration. D18/D19 take the migration | -| Tolerate a missing mark by inspecting what it wrapped | The branch it buys is only needed because merging had no legal encoding; with empty marks it has one |