Feat/wb 501 decision endpoint - #133
Conversation
|
|
||
| const waits = await countNodeWaits(executionId, nodeId); | ||
| if (waits === 0) return refuse(c, 'node_never_parked', nodeId); | ||
| if (waits !== attempt) return refuse(c, 'attempt_mismatch', undefined, { attempt: waits }); |
There was a problem hiding this comment.
The attempt check lives here in the backend, but the engine never sees the attempt: resolveNode gets only nodeId + resolution, and the waits map in run-workflow.ts is keyed by node id alone. So the check isn't atomic with delivery.
Today that's fine (a node parks once, rerun-source is 501), but once the rerun loop lands, a count read just before a re-park can deliver into the wrong wait, and the engine has no way to notice. The ticket keys the wait instance on execution + node + attempt for exactly this reason. Decision-log 15 acknowledges the gap, but nothing in code marks the seam.
Two options:
Preferred (small, additive, replay-safe since validators write nothing to history):
- add an optional
attempttoresolveNodeon the port and to the update input - give
NodeWaitStateanattemptcounter, incremented at the park site - have the validator reject
verdict.attempt !== state.attemptwith a newverdict_attempt_mismatchcode, mapped todecision_attempt_mismatchinENGINE_REFUSALS
The completeness tests will catch any missed dictionary. Old callers keep working since the field is optional.
Minimum: a (follow-up: decision-attempt-in-engine) marker here and on the waits map, plus the matching Code marker line on the rerun ticket, so the rerun work can't miss it.
b9e4b1d to
7103a30
Compare
|
|
||
| export function createDecisionRoutes( | ||
| assertAuthorized: AssertAuthorized, | ||
| ): Hono<{ Variables: AuthVariables & TenantVariables }> { |
There was a problem hiding this comment.
consider extracting a type as it's immediately repeated
| const routes = new Hono<{ Variables: AuthVariables & TenantVariables }>(); | ||
|
|
||
| routes.post('/:id/decision', async (c) => { | ||
| const executionId = c.req.param('id'); |
There was a problem hiding this comment.
can we change c to ctx or context unless it's a Hono convention?
…a parked run WB-501 step 1. WorkflowEnginePort gains a required resolveNode(executionId, nodeId, resolution) that answers every expected refusal as a result, never a throw: the validator's four codes plus run_not_found and delivery_timeout. The codes live once, as `as const` arrays in the execution-core port module; the validator types its throws with them and the Temporal adapter derives its runtime check from them. The adapter addresses the update by name (RESOLVE_NODE_UPDATE_NAME, a deliberate root export pinned like RUN_WORKFLOW_NAME) and bounds the RPC with client.withDeadline (resolveTimeoutMs, default 10 s). Unknown failure types are rethrown. The harness showed that an update abandoned at the client deadline is not dropped: the server still hands it to the next worker, so a retry may hear verdict_already_delivered. The test pins "exactly one lands". The validator's messages moved into one dictionary keyed by rejection, in the backend's style.
WB-501 step 2. toNodeResolution turns an accepted Decision and its matched action into the completion the engine delivers: output is the decision itself, nextPort the action's port, rerun-source excluded at the type level and the reserved errorRoute port refused. findDecisionRequest reads a node's request out of the parsed snapshot. A non-resume submission carrying edits is now refused with edits_not_allowed, checked before the field rules so the refusal names the edits.
WB-501 step 3. countNodeWaits(executionId, nodeId) counts the node_waiting events of one node in one run. The number is the wait instance a decision must name, and zero says the node never parked. Shared with the coming pending-decision resource, so it lives beside the event query, not in a route.
…ecision to a parked run WB-501 step 4. One door for every future channel. The route loads the row, authorizes executions:decide with the row's attributes (a deny wins over 404), refuses terminal and cancelling runs, parses the body, reads the node's request out of the parsed snapshot, judges the submission, checks the wait instance (attempt = the node's node_waiting count), refuses rerun-source with 501 until the engine can re-run a source, and delivers the completion through engine.resolveNode. Every engine refusal is answered on the first try; no retry. Codes, statuses and messages live once in decision-refusals.ts: the status map spells each code, situations are typed against it, and total maps over the engine's and the lookup's codes make a new code a compile error here.
…lares A code added to the port's validator group without a throw site compiled fine and stayed dead. The dictionary is exported and a type-level pin equates its codes with VerdictRejection in both directions.
…s 503 with Retry-After WB-501 step 5. The adapter's delivery_timeout becomes decision_delivery_timeout. The update is not durable until a worker accepts it, yet the server may still hand it to the next worker, so the message says the decision may or may not have landed and that a retry answering decision_already_made means it did. No retry anywhere on the server side.
WB-501 step 6. The backend README is the one place for the endpoint and its answers; the decision log keeps only the reasons and closes its open points; the Temporal README and decision log say what resolveNode answers with.
…ine error The row is checked before the body, the attempt before the effect. An error the engine throws instead of returning surfaces as 500.
The refusal table reuses the domain's {value} filler instead of carrying its own copy.
The rerun-source marker in the route states the limitation in words.
…rt's code array VERDICT_REJECTIONS meant a private array of codes in execution-core and an exported map of messages in the validator. The validator's is now VERDICT_REJECTION_MESSAGES.
A fake client with a frozen clock checks that withDeadline receives now + 10 s by default and now + resolveTimeoutMs when set. The entry-points table names RUN_WORKFLOW_NAME and RESOLVE_NODE_UPDATE_NAME.
Deny-before-404 hides which ids exist only if the port denies on absent attributes too. Re-parsing a stored snapshot with today's schema can leave a parked run undecidable after a deploy.
…rl spelling Postgres accepts a non-canonical uuid and answers with the canonical row, so the spelling in the url and the row's id can differ. The route passed the url string on to the engine, which builds a case-sensitive workflow name from it: a request that found the right row could address a workflow that does not exist and come back as a 409 for a run that is still parked. Submit and cancel already take the row's id; the decision route was the one call site that did not. Every id downstream of the row read is now the row's.
The 503 told a caller that a decision_already_made answer to a resend proves their own decision landed. Nothing in the endpoint tells two senders apart, so with two deciders racing one wait the answer can be about the other one's verdict while another parked node keeps the run open. The wording now claims only what first-write-wins can prove. The comment above the RPC deadline said a timed-out update is not durable, which conflates the deadline with acceptance: the update may already have been accepted when the deadline hits.
The attempt check reads the node's node_waiting count from Postgres and then calls the engine, which is handed only a node id: the wait map is keyed by node id alone, so the check is not atomic with delivery. It holds today only because a node parks at most once per run, an invariant the rerun loop breaks. Nothing in code said so. Two comments now name the hazard at both ends and the decision log carries the slug, so the rerun work starts from a grep rather than from rediscovering the gap.
2fe4ff4 to
abc8a0f
Compare
|
|
||
| // Every other status goes to the engine: the advisory status write is best-effort, so a | ||
| // parked run can still read 'pending'. | ||
| const NOT_DECIDABLE_STATUSES = new Set<string>([ |
There was a problem hiding this comment.
can we narrow the generic type to make it a Set<ExecutionStatus>?
| delivery_timeout: 'delivery_timeout', | ||
| } as const satisfies Record<ResolveNodeRejection, DecisionRefusal | 'fault'>; | ||
|
|
||
| export function refuse(c: Context, refusal: DecisionRefusal, value?: string, extra: Record<string, unknown> = {}) { |
There was a problem hiding this comment.
I'd consider using an options object parameter for this function.
It's not obvious what undefined is when passed as a third parameter for instance.
No description provided.