Repository navigation
feat(translator): carry a run's handle through the translation lifecycle - #175
Closed
SearheiParkhamchuk wants to merge 10 commits into
Closed
SearheiParkhamchuk wants to merge 10 commits into
SearheiParkhamchuk wants to merge 10 commits into
Conversation
`enqueue` returned a count. A host that wanted to cancel a run later, or record on its own row which run is translating a locale, had nothing to store. It now answers with one entry per requested target locale, each naming the handle that will run it. Per locale rather than per handle, because a single job row covers a document's whole locale list: two locales of one document name the same run, and a request spanning several documents still reads unambiguously. `TaskRunner.enqueue` widens its return rather than changing it. The old `void` answer still satisfies the contract and is deprecated for the next major, with a ledger entry. An overload pair is not usable here: an implementation must satisfy every overload, so it would break every third-party runner. `EnqueueAssignment` and `Task` are exported so an implementation outside this package can name what it answers with. A locale an existing run already covers is answered too, with that run's handle — it is being translated, and leaving it out would report nothing for work genuinely under way. `EnqueuePlan` gained `covered`/`coveredBy` for it, and every requested locale now appears in exactly one of `append`, `queue` and `covered`. Two things fell out of making the answer honest. A run whose retries are spent keeps `hasError` and never gets a completion date, so it stayed in the plugin's live set although Payload's own picker had excluded it for good. Appending to it promised a translation that could not happen; `pickHost` now refuses it. A run's handle had three spellings: two helpers stringified the row id and `normalizeJob` passed it through, so one run arrived as `"12"` down one path and `12` down another. One helper forms it now. This changes the wire: `GET /translate/document/:slug/:id` and `GET /translate/collection/:slug` answer `"id": "12"` where they answered `"id": 12`. The declared type always said `string`, so nothing documented changes.
A callback said which document and locale it was about, but nothing identifying the request behind it. A host layering editorial process on top had to write an intent record before enqueue and join on (collection, document, locale) plus "is a run open" — which forced one open run per document and locale, a constraint it would rather not need. Every callback now carries the handle the enqueue answer named, so the join is direct. `onQueued` is the exception and carries none: it fires before the runner is asked, which is what keeps a synchronous runner's `completed` from arriving first, and at that moment no run exists. `onFailed` now means the locale will not be translated. It used to fire on every failed attempt, so "failed" did not mean failed and the consumer ran with retries disabled. Only a runner that retries can tell a transient failure from a final one, so it reports the final one itself and declares `reportsFinalFailure`; a runner that does not retry is reported where the throw happens, as before. There is deliberately no attempt counter: an attempt number still would not say whether another try is coming. When a run gives up it reports every locale it still owed, not only the one that threw. The locales after it never started, so nothing ran for them to throw from and no caller could otherwise learn they are dead. Payload gives up two ways — the locale's attempts against the task limit and the whole run's executions against the workflow limit — and the two diverge as soon as attempts are spent on more than one locale, so both are asked. The error a host receives is the plugin's own. Payload replaces a task's error with a message-only copy, which silently stopped matching the exported error classes, so the handler's error is kept and handed back.
A cancelled run simply stopped existing. The plugin cancels the Payload job and then deletes its row, so a host tracking per-document state had to infer that the work was no longer happening. `onCancelled` now fires once per target locale the run still owed, before the row is deleted, so a host can still read it. A locale the run had already translated is not announced — it was not cancelled. A locale already in flight is announced and still finishes: cancelling does not stop work already running, and the callback contract says so rather than pretending otherwise. The cancel route holds only handles, so naming the locales needs a read back: a new **optional** `findByIds` on `TaskRunner`. A runner that omits it keeps working and never fires the callback — the synchronous runner has nothing to cancel, so it omits it. A failed read is logged and never costs the caller their cancellation, and a runner that cannot resolve handles is not asked, so silence stays distinguishable from a read that failed quietly. The wrapper is now applied for any callback it fires. A host registering only `onCancelled` used to be handed an undecorated runner and heard nothing. Both runners are now held to one contract by one call. `TaskRunner.interface.ts` states the obligations every implementation owes — what an empty request answers, that a handle is a non-empty string, that only requested locales come back, that an unknown handle is an answer rather than a failure — and `TaskRunner.invariants.ts` asserts them against any runner. It was written against the contract alone, run red against a stub first, and it found a real divergence: the synchronous runner answered twice, and translated twice, for a locale a request named twice.
`GET /translate/collection/:slug` answered with a run handle and a status, and a caller could resolve neither back to what was being translated. A run covers a document's whole locale list, so the handle repeats across entries and cannot identify one on its own — the queue view an admin dashboard wanted could not be rendered from it. Each entry now names the document and the target locale beside the handle. The runner's normalized task already carried both; only the response mapping discarded them.
SearheiParkhamchuk
requested review from
ChiefCreator and
dogfrogfog
as code owners
October 8, 2026 10:38
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…tually mean
Two things the owner caught in review, both about names and promises rather than behaviour.
**The enqueue answer no longer renames a handle into a job id.** It answered with
`{ ..., job_id }` under a `jobs` key, so the HTTP handler was translating the runner's
opaque handle into the vocabulary of one particular runner — Payload jobs — in a response
that is meant to be runner-neutral. The synchronous runner has no jobs at all. The answer
now carries `handle` under `assignments`, and the mapping left in the handler is a pure
conversion to the snake_case this surface already answers in.
Putting that mapping inside `TaskRunner.enqueue` instead would push the knowledge the
wrong way: the contract is implemented outside this package, and a third-party runner
would have to produce HTTP field names. The field is new in this change and unreleased,
so renaming it costs nothing.
**The contract now promises only what every store can keep.** It said an unknown handle
resolves rather than rejecting. That is false on PostgreSQL and MongoDB: the job id is an
integer there, so a handle no runner could have issued fails inside the query instead of
finding nothing. The conformance suite found it — those two checks failed on both
databases and passed on SQLite, which is lenient.
The obligation now reaches handles **this runner issued**, and the suite exercises a
handle it obtained for real and then had cancelled: unknown by then, and shaped as that
runner shapes handles. The limit is written down beside the obligations, including its
consequence — `DELETE /translate/cancel` answers with a server error for a value the store
cannot parse. That is left unfixed on purpose: every way to fix it costs something decided
deliberately elsewhere — adapter-specific error codes, an unbounded scan the project
already removed once, or parsing a handle the contract says is never parsed.
The plugin decided whether to wrap a runner in `withQueuedNotification` by asking which callbacks the host had registered: wrap for `onQueued` or `onCancelled`, skip otherwise. The gate bought one avoided read — `cancel` resolves handles back to tasks only so it can name what it is stopping, and a host with no `onCancelled` has no use for that read. It cost more than it bought. Two hosts got structurally different runners depending on a setting unrelated to the difference: a wrapped runner normalizes `findByCollection`'s deprecated array argument, an unwrapped one does not. And the list of callbacks the wrapper raises was written twice — once in the wrapper, once in the gate — with nothing tying them together. That already produced a bug this branch had to fix: the gate asked only about `onQueued`, so a host registering only `onCancelled` was handed an undecorated runner and heard nothing. The runner is wrapped unconditionally now. Every callback the notifier fires is already a no-op when the host registered nothing, so wrapping costs a host that wants no reporting one resolved promise per enqueued task, and on cancel one read whose result is discarded. The check that pinned the old rule pinned the behaviour being removed, so it was rewritten to pin the new one instead, over two cases: a host registering only callbacks the task handler fires, and a host registering nothing at all.
…ot need All three came out of review, none changes behaviour. **`LifecycleNotifier.cancelled` folded into `cancelling`.** It was a private two-line helper with exactly one call site, inside the loop of the method it sat next to. It was kept separate for symmetry with `queued`, `completed` and `failed` — but the symmetry does not hold: those three are public and take a task from the caller, while this one was private and took a task the class had just read itself. **The non-null assertion on `findByIds` is gone.** The wrapper re-exposed the optional method as `runner.findByIds!(ids)`, asserting something the compiler could not check: the guard sat in a ternary and the call inside an arrow, so the narrowing never reached it. The method is now bound into a local once, which both narrows honestly and keeps the receiver a class implementation needs. **The run-to-loop error channel no longer lives at module scope.** Payload replaces a task's error with a message-only copy, so the plugin keeps its own error in a `WeakMap` until the loop that reports it can read it back. That map was a module-level `const`, shared by every plugin instance in the process, although both handlers that use it are created in the same `configure()` call. It is a local there now, and `thrownByTheHandler.ts` — the module and its three exported functions — is deleted. The generated per-file suite loses one check with that file, which is the check that was scanning it.
`EnqueueAssignment` and `Task` were added to the barrel so a third-party runner could annotate what it returns. A compile-only probe then showed a complete outside runner compiles **without naming either**: the shapes are inferred from `TaskRunnerProvider`, the one name that genuinely crosses the boundary. The export enabled nothing, so it goes. It never made them private, and the ADR says so plainly: `TaskRunnerProvider` is exported, its `create` returns a `TaskRunner`, and that interface answers with both types. They are emitted into the published declarations and reachable through `ReturnType` either way. A barrel decides how conveniently a consumer can name a type, not whether the type is part of the contract — so keeping a loose type out of it buys tidiness, never freedom to change it. ADR-0002 records the rule and the three questions it turns on, and the package guide points at it next to the `@since` rule, which governs the same file.
Three places told the host what happened to a translation: a wrapper around the runner raised `queued` and `cancelled`, a catch inside the translation handler raised `completed` and `failed`, and a callback handed to the runner raised `failed` for locales a run gave up on. A boolean on the provider, `reportsFinalFailure`, picked between the last two — a switch that existed only because the knowledge of whether another attempt is coming lives in the runner while the decision to speak was taken in the handler. The runner now owns the whole story. It is handed `report` in its context and calls it for every assignment it accepted: `queued` when the work is taken, then at most one of `delivered`, `failed` or `cancelled`. The plugin turns those four into the host's callbacks and remains the only thing that knows a host exists. Gone with the switch: `withQueuedNotification` and its 24 checks, `TaskRunnerContext.reportFinalFailure`, `TaskRunner.findByIds` — cancellation reports itself now — `TaskHandlerInput.handle`, which nothing read once the handler stopped reporting, and two of the three task mappers. `EnqueueAssignment` gained `sourceLng` and `strategy` so one mapper can turn it into what the host sees; three existed only because the event arrived in three shapes. **Breaking for a third-party runner, and for nobody else.** `create` takes the context rather than the handler, `report` is required so that omitting it fails the build rather than leaving a host silent, and the removals above are compiler errors. A host that merely configures the plugin sees no change: the callbacks, their arguments, the runner factories and every HTTP response are as they were. All of it predates any release — `reportsFinalFailure` was added on this branch and never shipped. The invariants were written against the contract alone by an author that could not read the implementations, which did not exist yet. They ran red on a stub, 29 of 29, and the writing of them found five sentences the contract was missing: when a report becomes observable, that the reported assignment carries the handle the enqueue answer gave, that a locale an existing run already covers still earns its `queued`, that `cancelled` only follows a `cancel`, and that a locale already reported `failed` is not owed. Two defects surfaced on a real database. Payload hands back a completed task's stored output instead of re-running it, so the loop reported `delivered` again on every later pass; it now reads the log before the call. And a handle is unique only while its row exists — SQLite reuses a deleted row's id, so a handle kept across a cancellation can later name another run. That one is a limit now written into the contract rather than a promise the code cannot keep.
Cancelling a run whose retries were spent announced `onCancelled` for the very locales that had just been told `onFailed` — a second ending for one locale, which the event contract says cannot happen. `stillOwed` keeps every locale that is not `completed`, and Payload only writes `completedAt` when a whole workflow succeeds, so a spent run keeps its error, keeps no completion date, and its settled locales still read as owed. The cancel path now asks its own question through `owedOnCancel`: a run that has given up owes nothing, because spending the last attempt already settled everything it had not delivered; a run merely waiting to try again owes its undelivered locales, logged failures among them. Keeping the two apart matters — the obvious narrowing, "not completed and not failed", would have silenced the legitimate case, where a job between attempts has every locale logged as failed and every one of them still owed. Reachable without doing anything unusual: `DELETE /translate/cancel-by-collection/:slug` sweeps up whatever is neither completed nor running, and a spent run is both. Covered on a real database from both the by-handle and the by-collection route, and in a unit check that pins the between-attempts case in the other direction. Also here, from the same verification pass: - **`onQueued` had no check at all.** Its criterion asked for a host registering the callback on a real database, for both runners; nothing did. Two specs now do, and a mutation proves each — reporting `queued` after the work makes the inline runner's spec read "de ended before the host was told it had started", and deleting the queued loop makes the jobs runner's spec red. - **Three places described code that no longer exists.** `TranslationTask.handle` claimed to be absent on `onQueued`; it has not been since the runner became what reports `queued`, and a host branching on its absence would have been misled. `wireTranslateRunner`'s docblock still described the deleted decoration. `DEPRECATIONS.md` pointed a reader at the removed `reportsFinalFailure` instead of at `report`'s `failed` event. - **One criterion was false and is restated.** "A runner that does not implement `report` fails to build" cannot be true: the `silent` conformance fixture never calls `report` and compiles, because no type can require that a function be called. What the required field does enforce — that nothing can build a context without it — breaks nine files when removed, and that is what the criterion, the decision behind it and the fixture's docblock now claim.
Contributor
Author
|
Superseded. The work carried on well past what this pull request described — the stored job input collapsed to one declaration, the config machinery got a name, and the workflow handler was recomposed. A replacement opens from the same branch with the issues linked there. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The plugin had a handle for every run it queued and threw it away. Four issues from one
consumer building an editorial workflow on top of it reduce to that.
What changes
enqueueanswers with what it arranged — one entry per requested target locale, eachnaming the handle that will run it. Per locale rather than per handle, because one job row
covers a document's whole locale list: two locales of one document name the same run, and
a request spanning several documents still reads unambiguously. A locale an existing run
already covers is answered too, with that run's handle.
Every lifecycle callback carries that handle,
onQueuedincluded, so a host can join acallback back to the request behind it without keeping its own index. The runner is what says
queued, and it says it for work it has already taken — so the handle exists by then, and asynchronous runner cannot announce a completion before the start it belongs to.
onFailednow means the locale will not be translated. It fired on every failedattempt, so "failed" did not mean failed and the consumer ran with retries disabled. When
a run gives up it reports every locale it still owed, not only the one that threw — the
locales after it never started, so nothing ran for them to throw from.
Cancelling says what it stopped, once per locale the run still owed, before the row is
deleted.
Collection status names the document and locale beside the handle, which a caller
previously could not resolve back to anything.
One channel, owned by the runner
The first four commits left the telling spread across three places: a wrapper around the
runner raised
queuedandcancelled, a catch inside the translation handler raisedcompletedandfailed, and a callback handed to the runner raisedfailedfor locales arun had given up on. A boolean,
reportsFinalFailure, picked between the last two — aswitch that existed only because the knowledge of whether another attempt is coming lives in
the runner while the decision to speak was taken in the handler.
The runner now owns the whole story. It is handed
reportin its context and calls it forevery assignment it accepted:
queuedwhen the work is taken, then at most one ofdelivered,failedorcancelled. The plugin turns those four into the host's callbacksand stays the only thing that knows a host exists.
Gone with the switch: the wrapper and its 24 checks,
reportFinalFailure,TaskRunner.findByIds— cancellation reports itself now —TaskHandlerInput.handle, andtwo of the three task mappers.
Decisions worth a reviewer's attention
TaskRunner.enqueuewidens its return rather than changing it. The oldvoidanswerstill satisfies the contract, deprecated for the next major with a ledger entry. An
overload pair is not usable: an implementation must satisfy every overload, so it would
break every third-party runner.
reportis required, not optional. A runner that silently omitted it would leave itshost hearing nothing and no compiler would say so. Required means such a runner fails to
build, which is the loudest available signal.
not tell a host whether another try is coming, which is what it actually needs. Recorded
as a need this does not meet.
src/index.tsis byte-identical tomain:a compile-only fixture shows a complete outside runner compiles while naming only
TaskRunnerProviderand inferring the rest. The rule and the case that produced it arewritten down in ADR-0002.
TaskRunner.interface.tsstates theobligations every implementation owes, and
TaskRunner.invariants.tsasserts them againstany runner. Its checks were written against the contract alone, by an author that could not
read the implementations because they did not exist yet; they ran red on a stub, 29 of 29.
Writing them found five sentences the contract was missing — when a report becomes
observable, that the reported assignment carries the handle the enqueue answer gave, that a
locale an existing run already covers still earns its
queued, thatcancelledonlyfollows a
cancel, and that a locale already reportedfailedis not owed.Behaviour changes
hasErrorand nevergot a completion date, so it stayed in the plugin's live set although Payload's own picker
had excluded it for good — asking again for that locale produced no work at all.
GET /translate/document/:slug/:idandGET /translate/collection/:sluganswer"id": "12"where they answered"id": 12. Arun's handle had three spellings in the code; it has one now. The declared type always said
string, so nothing documented changes — but the bytes do.workflow loop reported
deliveredagain on every later pass. It now reads the task logbefore the call.
onCancelledfor locales that had justbeen told
onFailed— a second ending for the same locale. Spending the last attempt settleseverything the run had not delivered, so a spent run now owes nothing to a cancellation. A run
merely waiting to try again still owes its undelivered locales and still announces them.
Breaking, and for whom
For a runner implemented outside this package:
createtakes the context rather than thehandler,
reportis required, and the removals above are compiler errors. All of it predatesany release —
reportsFinalFailurewas added on this branch and never shipped.For a host that merely configures the plugin: nothing. The callbacks, their arguments, the
runner factories and every HTTP response are as they were.
Public API
Added:
TranslationTask.handle,TranslationLifecycleCallbacks.onCancelled,TaskRunnerContext.report, and the widenedTaskRunner.enqueuereturn. All additive for ahost that registers nothing new.
README.mdis deliberately untouched; its lifecycle section still describes the per-attemptbehaviour this change removes, and needs a pass from its owner.
Known gaps, not fixed here
still finishes, so one locale can produce two events.
cancelis byte-identical tomain,so this predates the change; fixing it means changing what cancellation does. The contract
states the exception instead of implying otherwise.
kept across a cancellation can later name another run. Written into the contract as a limit
rather than left as a promise the code cannot keep.
Verification
check-types, every packagetsc)mainreports nothing introducedEach adapter was run twice, with and without Payload's exclusive queue — six combinations,
no failures.
The skips are adapter-specific: transaction behaviour that only one of the three has, and
one check that cancels from inside a running job, which on a transactional adapter blocks on
a row its own transaction holds.
Three reviewers read the diff — correctness, regression and intent. Five findings, all acted on:
the cancellation defect above, the missing
onQueuedcheck, and three places whose prose describedcode this work had deleted. One acceptance criterion turned out to be false as written and was
restated rather than ticked: "a runner that does not implement
reportfails to build" cannot hold,because the
silentconformance fixture never callsreportand compiles — no type can requirethat a function be called. What the required field does enforce, that nothing can build a context
without it, breaks nine files when removed.
Five mutations proved the new checks discriminate: removing
reportfrom the context, narrowingenqueue, reportingqueuedafter the work, deleting the jobs runner's queued loop, and droppingthe give-up guard. Each was restored and the restore verified.
Run against the pinned Payload 3.90.1. The three places in Payload's own source this change
reasons about — the per-task give-up check, the per-workflow one, and where a task's error is
replaced by a message-only copy — were re-read against that version.