Repository navigation
feat(translator): carry a run's handle through the lifecycle, and say it once - #177
Open
SearheiParkhamchuk wants to merge 11 commits into
Open
SearheiParkhamchuk wants to merge 11 commits into
SearheiParkhamchuk wants to merge 11 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.
…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.
…nner does Four passes over the Payload-jobs runner, each verified against a real database before the next began. **The stored job input is declared once.** The field vocabulary lived in six places — four TypeScript shapes and two Payload `Field[]` literals — and nothing checked any against any other, so adding a field was six edits and forgetting one was silent. A zod schema in `store/jobInput.schema.ts` is now the only declaration: the types are inferred from it and the `Field[]` Payload is registered with is generated from it. The generator reads only zod's public surface, because this repository already carries both zod 3 and zod 4 and their internals differ — `_def.typeName` is a string on one and `undefined` on the other. A characterisation test pins the generated fields against a baseline captured before any edit, and earned its keep within the hour by catching that `z.coerce.string()` turns `undefined` into the string `"undefined"`. **The runner's folder says what each file is for.** `payload-jobs-runner/` had sixteen entries in one listing. It is now `store/` — the shape of what Payload stores, and the readers that turn it into ours — and `model/` — the rules that decide. The dependency runs one way; nothing in `store/` knows `model/` exists. The convention is written down in the package's `CLAUDE.md`. **Adding something to a host's config has one name.** `contribute` takes a field that may be absent, a list, or a function producing one — Payload declares `jobs.autoRun` all three ways — and adds this plugin's entries without repeating what the host already put there. For the function form the duplicate check travels with the wrapper, because what that function will return cannot be known at config time. `chain` does the same for `onInit`. Four hand-rolled blocks became four calls. **That closed a defect on the way.** Applying the plugin twice registered the task and the workflow twice and added a second `autoRun` entry for the same queue, so `translations` was polled twice per tick. Payload does not catch it: it gathers slugs into a `Set` only to validate, and throws only when a task slug collides with a workflow's. **And the workflow handler says what it does.** It interleaved four unnamed mechanics in twenty lines. Two now have names: `localesAsTheyStand`, whose whole point is that it re-reads the locale list on every step so a locale appended to a run in flight is picked up, and `deliveredLocales`, a snapshot taken before anything runs because Payload writes a task's `succeeded` entry during the call. Two readers also stopped walking properties by hand: `readCollectionRef` and the two structural guards over a job's error now parse against the schema, with their own tests unedited. Unit 2057 passing across 148 files, integration 224 passing and 7 skipped on SQLite, `check-types` clean across the workspace, the real `tsc` declaration build green, the analyzer gaining nothing, and the host's generated `payload-types.ts` byte-identical to what it was.
SearheiParkhamchuk
requested review from
ChiefCreator and
dogfrogfog
as code owners
October 9, 2026 17:11
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Closes #107, closes #110, closes #106, closes #103.
Replaces #175, which was closed once the work grew past what its description covered.
What a host gets
enqueueanswers with what it arranged — one entry per requested target locale, each namingthe handle that will run it. Per locale rather than per handle, because one job row covers a
document's whole locale list. 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.
onFailednow means the locale will not be translated. It fired on every failed attempt, 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.
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.
One channel, owned by the runner
Three places used to tell the host what happened, with a boolean picking between two of them. The
runner now owns the whole story: it is handed
reportand calls it for every assignment itaccepted —
queuedwhen the work is taken, then at most one ofdelivered,failedorcancelled. The plugin turns those four into the host's callbacks and stays the only thing thatknows a host exists.
What changed after the first review round
Four further passes, each with its own contract under
docs/plans/and its own verification.The stored job input is declared once. Its field vocabulary lived in six places — four
TypeScript shapes and two Payload
Field[]literals — with nothing checking any against anyother. A zod schema is now the only declaration; the types are inferred and the
Field[]isgenerated. The generator reads only zod's public surface, because this repository carries both
zod 3 and zod 4 and their internals differ. A characterisation test pins the generated fields
against a baseline taken before any edit — and caught, within the hour, that
z.coerce.string()turns
undefinedinto the string"undefined".The runner's folder says what each file is for. Sixteen entries in one listing became
store/— the shape of what Payload stores and the readers that turn it into ours — and
model/, therules that decide. The dependency runs one way, and the convention is written into the package's
CLAUDE.md.Adding something to a host's config has one name.
contributehandles a field that may beabsent, a list, or a function producing one — Payload declares
jobs.autoRunall three ways — andchaindoes the same foronInit. That closed a defect: applying the plugin twice registered thetask and the workflow twice and polled the queue twice per tick, which Payload does not catch.
The workflow handler says what it does. Two of its four unnamed mechanics got names:
localesAsTheyStand, which re-reads the locale list on every step so a locale appended to a run inflight is picked up, and
deliveredLocales, a snapshot taken before anything runs because Payloadwrites a task's
succeededentry during the call.Breaking, and for whom
For a runner implemented outside this package:
createtakes the context rather than the handler,reportis required, and several removals are compiler errors. All of it predates any release.For a host that merely configures the plugin: nothing. The callbacks, their arguments, the runner
factories and every HTTP response are as they were, and the published barrel
src/index.tsisbyte-identical to
main.Known gaps, not fixed here
finishes.
cancelis byte-identical tomain, so this predates the change.across a cancellation can later name another run. Written into the contract as a limit.
thrownis aWeakMapkeyed by the job row, and nothing checks that the task handler and theworkflow loop see the same object.
Verification
check-types, every packagetsc)payload-types.tsEach adapter was run with and without Payload's exclusive queue. Run against the pinned Payload
3.90.1; the places in Payload's own source this reasons about were read against that version.
Eight review passes in total — deep, complexity, abstraction, comment and red-test audits, plus
three rounds of correctness, regression and intent review. Every finding is recorded with its
resolution in the
## Review logof the contract it belongs to, underdocs/plans/.