From 31631f957ee80f945870c91b83c6cf1b6581cde2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 14 Aug 2026 12:11:18 -0700 Subject: [PATCH 1/4] fix(v2): close seven correctness and honesty gaps found sweeping the API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ten-slice sweep of the live v2 surface turned up no regression from the recent cancellation work, but did surface a set of pre-existing defects where an endpoint either lost data, hid a failure, or reported something that was not true. Each is fixed at the layer that owns the behavior. Terminal execution logs. The two force-fail boundaries wrote `status: 'failed'` without `ended_at` or `total_duration_ms`, so a force-failed run dropped out of every duration-filtered log query — the same defect class already closed for cancellation, still open on its sibling. The cancellation payload factory is generalized to take the status; the cancellation call sites are untouched and still emit a byte-identical row. Custom tools. One malformed row failed the whole page, and because the list is keyset-paginated that row made every page containing it permanently unreachable. The projection now validates against the same contract schema the route builder applies, repairing only what can be repaired without inventing information — a stringified schema, and a missing `type` whose contract admits exactly one value — and omitting with a warning what cannot. Both rows observed in production are recovered rather than discarded. Table filters. `eq`/`ne`/`in`/`nin` compiled a wrongly-typed operand into a containment test that silently matched nothing, so a filter written against the value the write path had stored returned an empty page instead of its rows. The operand is now read through the same column-type registry the write used, and rejected only where that registry refuses it. Range operators already behaved this way; `null` and the cleared-cell sentinel still pass through untouched. Error messages. A custom `error` on a string schema also replaced the wrong-type wording, so supplying a number for a name reported that the name was missing. Messages now distinguish an omitted field from a mistyped one, `topK` names its own bounds, the knowledge search refine reports against a field rather than the whole body, and a workspace id is bounded before it reaches a lookup. Archived file metadata. A soft-deleted file was listed but unreadable, leaving no way to check share state before restoring it. The read takes the same `scope` selector the list already exposes; the default is unchanged, and the parameter relaxes only the `deleted_at` predicate, never the authorization. Cancellation reporting. Cancelling an already-terminal run reported a durable write that never happened. The service now distinguishes the no-op and names the state it observed, and both surfaces present one vocabulary instead of the internal route deriving its own. No claim predicate or write changed. Protocol. A 401 carries a challenge naming the header the API actually reads, and a body that failed to parse is reported as an unsupported media type only when the caller positively declared a non-JSON one — after the read has already failed, so nothing that succeeds today can begin to fail. --- apps/docs/openapi-v2-billing.json | 6 +- apps/docs/openapi-v2-files-audit.json | 35 +++- apps/docs/openapi-v2-knowledge.json | 15 ++ apps/docs/openapi-v2-logs.json | 1 + apps/docs/openapi-v2-resources.json | 23 +++ apps/docs/openapi-v2-tables.json | 35 ++++ apps/docs/openapi-v2-workflows.json | 16 +- .../sim/app/api/v2/custom-tools/route.test.ts | 101 ++++++++- apps/sim/app/api/v2/custom-tools/route.ts | 4 +- apps/sim/app/api/v2/custom-tools/utils.ts | 150 +++++++++++++- .../v2/files/[fileId]/metadata/route.test.ts | 83 +++++++- .../api/v2/files/[fileId]/metadata/route.ts | 11 +- apps/sim/app/api/v2/lib/response.test.ts | 64 +++++- apps/sim/app/api/v2/lib/response.ts | 23 +++ .../[id]/runs/[runId]/cancel/route.test.ts | 121 +++++++++++ .../[executionId]/cancel/route.test.ts | 1 + apps/sim/lib/api/contracts/primitives.test.ts | 76 +++++++ apps/sim/lib/api/contracts/primitives.ts | 58 +++++- apps/sim/lib/api/contracts/v2/files.ts | 25 ++- apps/sim/lib/api/contracts/v2/knowledge.ts | 39 +++- .../v2/required-field-messages.test.ts | 175 ++++++++++++++++ apps/sim/lib/api/contracts/v2/skills.ts | 22 +- apps/sim/lib/api/contracts/v2/workflows.ts | 17 +- apps/sim/lib/api/contracts/workflows.test.ts | 18 +- apps/sim/lib/api/contracts/workflows.ts | 24 ++- .../api/server/routes/v2-json-route.test.ts | 107 ++++++++++ .../lib/api/server/routes/v2-json-route.ts | 64 ++++++ .../cancel-workflow-execution.test.ts | 57 +++++ .../execution/cancel-workflow-execution.ts | 90 +++++++- .../lib/logs/execution/cancellation.test.ts | 41 +++- apps/sim/lib/logs/execution/cancellation.ts | 47 +++-- .../logs/execution/logging-session.test.ts | 41 +++- .../sim/lib/logs/execution/logging-session.ts | 7 +- apps/sim/lib/table/__tests__/sql.test.ts | 194 ++++++++++++++++++ apps/sim/lib/table/sql.ts | 149 +++++++++++--- .../human-in-the-loop-manager.test.ts | 53 +++++ .../executor/human-in-the-loop-manager.ts | 7 +- .../read-workspace-file-metadata.test.ts | 56 +++++ .../read-workspace-file-metadata.ts | 6 + 39 files changed, 1952 insertions(+), 110 deletions(-) create mode 100644 apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts create mode 100644 apps/sim/lib/api/contracts/v2/required-field-messages.test.ts diff --git a/apps/docs/openapi-v2-billing.json b/apps/docs/openapi-v2-billing.json index 5b513c1c03d..77de6a7d2ca 100644 --- a/apps/docs/openapi-v2-billing.json +++ b/apps/docs/openapi-v2-billing.json @@ -47,7 +47,8 @@ "schema": { "description": "Workspace whose payer should be resolved. Workspace API keys are pinned to their own workspace.", "type": "string", - "minLength": 1 + "minLength": 1, + "maxLength": 128 } } ], @@ -133,7 +134,8 @@ "schema": { "description": "Restrict results to one workspace whose payer the caller can inspect.", "type": "string", - "minLength": 1 + "minLength": 1, + "maxLength": 128 } }, { diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index e842423e106..045fd369674 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -51,6 +51,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose files should be listed." } }, @@ -343,6 +344,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the upload session." } }, @@ -433,6 +435,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the upload session." } }, @@ -537,6 +540,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the upload session." } }, @@ -629,6 +633,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } } @@ -721,6 +726,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } } @@ -965,8 +971,21 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } + }, + { + "name": "scope", + "in": "query", + "required": false, + "description": "Which lifecycle set to read from: `active` (default) resolves live files only and returns `404` for a file a `DELETE` soft-deleted; `archived` also resolves soft-deleted files, so metadata stays readable before `POST /files/{fileId}/restore`. Authorization is identical for both.", + "schema": { + "default": "active", + "description": "Which lifecycle set to read from: `active` (default) resolves live files only and returns `404` for a file a `DELETE` soft-deleted; `archived` also resolves soft-deleted files, so metadata stays readable before `POST /files/{fileId}/restore`. Authorization is identical for both.", + "type": "string", + "enum": ["active", "archived"] + } } ], "responses": { @@ -1060,7 +1079,8 @@ "schema": { "description": "Filter to actions in one workspace.", "type": "string", - "minLength": 1 + "minLength": 1, + "maxLength": 128 } }, { @@ -1361,6 +1381,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } } @@ -1655,6 +1676,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -1898,6 +1920,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -2437,6 +2460,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the file." }, "name": { @@ -2651,6 +2675,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which the file will be registered." }, "name": { @@ -2822,6 +2847,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." }, "name": { @@ -2877,6 +2903,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the archived file." } }, @@ -3352,6 +3379,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the files." }, "fileIds": { @@ -3446,6 +3474,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." }, "isActive": { @@ -3496,6 +3525,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." }, "content": { @@ -3572,6 +3602,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the files." }, "fileIds": { @@ -3683,6 +3714,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -3701,6 +3733,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 24157c46f4e..6f3da73eccb 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -47,6 +47,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose knowledge bases should be listed." } }, @@ -885,6 +886,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } } @@ -1077,6 +1079,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } }, @@ -1178,6 +1181,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } }, @@ -1293,6 +1297,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } }, @@ -1635,6 +1640,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -1881,6 +1887,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -2469,6 +2476,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the knowledge base." }, "name": { @@ -2509,6 +2517,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "name": { @@ -2798,6 +2807,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge bases." }, "knowledgeBaseIds": { @@ -3116,6 +3126,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "operation": { @@ -3467,6 +3478,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "name": { @@ -3918,6 +3930,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "filename": { @@ -4113,6 +4126,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -4131,6 +4145,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 0a64c9f581f..46f3e4fec47 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -47,6 +47,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose execution logs should be returned." } }, diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 51e85c8125b..30d0b7a5ff5 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -67,6 +67,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace to retrieve." } } @@ -132,6 +133,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace to retrieve." } }, @@ -221,6 +223,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } }, @@ -424,6 +427,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } } @@ -575,6 +579,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } } @@ -651,6 +656,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } }, @@ -729,6 +735,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." } }, @@ -932,6 +939,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." } } @@ -1086,6 +1094,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." } } @@ -1151,6 +1160,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." } }, @@ -1354,6 +1364,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." } } @@ -1508,6 +1519,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." } } @@ -1573,6 +1585,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose credentials should be listed." } }, @@ -1720,6 +1733,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose secret metadata should be listed." } }, @@ -1969,6 +1983,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces." } }, @@ -2291,6 +2306,7 @@ "id": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "name": { @@ -2668,6 +2684,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to register the server." }, "name": { @@ -2854,6 +2871,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." }, "name": { @@ -3252,6 +3270,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the skill." }, "name": { @@ -3345,6 +3364,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." }, "name": { @@ -3618,6 +3638,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the custom tool." }, "title": { @@ -3811,6 +3832,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." }, "title": { @@ -4182,6 +4204,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces." }, "scope": { diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index b1bff5d7222..aa729c5c616 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -47,6 +47,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose tables should be listed." } }, @@ -739,6 +740,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the table." } }, @@ -2648,6 +2650,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -2733,6 +2736,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -2823,6 +2827,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -2927,6 +2932,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -3102,6 +3108,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } } @@ -3176,6 +3183,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } } @@ -3255,6 +3263,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } } @@ -3402,6 +3411,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -3648,6 +3658,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -4304,6 +4315,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "schema": { @@ -4455,6 +4467,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "name": { @@ -4588,6 +4601,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the table." }, "column": { @@ -4680,6 +4694,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the table." }, "columnName": { @@ -4768,6 +4783,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "columnName": { @@ -4929,6 +4945,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "rows": { @@ -4950,6 +4967,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "data": { @@ -5161,6 +5179,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "filter": { @@ -5232,6 +5251,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "filter": { @@ -5284,6 +5304,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "data": { @@ -5372,6 +5393,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "data": { @@ -5432,6 +5454,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "predicate": { @@ -5532,6 +5555,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "predicate": { @@ -6481,6 +6505,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "group": { @@ -6668,6 +6693,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "groupId": { @@ -6938,6 +6964,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "groupId": { @@ -6996,6 +7023,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "groupIds": { @@ -7084,6 +7112,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." } }, @@ -7158,6 +7187,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "q": { @@ -7676,6 +7706,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "source": { @@ -8159,6 +8190,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "format": { @@ -8274,6 +8306,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "scope": { @@ -8398,6 +8431,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -8429,6 +8463,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index 6d9fdd9da01..e0b56db6970 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -51,6 +51,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose workflows should be listed." } }, @@ -1712,6 +1713,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -1964,6 +1966,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -2499,6 +2502,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the workflow." }, "name": { @@ -3828,6 +3832,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to import the workflow." }, "workflow": { @@ -4660,7 +4665,7 @@ }, "durablyRecorded": { "type": "boolean", - "description": "Whether cancellation was recorded durably." + "description": "Whether this request durably recorded a cancellation. Always false for a run that was already terminal, where the request is satisfied but nothing was written." }, "locallyAborted": { "type": "boolean", @@ -4671,10 +4676,13 @@ "description": "Whether a paused execution was cancelled." }, "reason": { - "description": "Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.", + "description": "Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `already_cancelled`, `already_completed`, and `already_failed` mean the run had already reached that terminal state, so nothing was cancelled and `durablyRecorded` is false. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.", "type": "string", "enum": [ "recorded", + "already_cancelled", + "already_completed", + "already_failed", "redis_unavailable", "redis_write_failed", "paused_event_publish_failed", @@ -4692,7 +4700,7 @@ ], "additionalProperties": false, "title": "Cancel workflow run result", - "description": "Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, so poll the run to observe its final state." + "description": "Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, reported as `durablyRecorded: false` with an `already_*` reason naming the state observed." }, "CancelWorkflowRunResponse": { "type": "object", @@ -4838,6 +4846,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -4881,6 +4890,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/sim/app/api/v2/custom-tools/route.test.ts b/apps/sim/app/api/v2/custom-tools/route.test.ts index 3b32d07ccab..1e29a606fc7 100644 --- a/apps/sim/app/api/v2/custom-tools/route.test.ts +++ b/apps/sim/app/api/v2/custom-tools/route.test.ts @@ -4,7 +4,7 @@ import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mocks, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { +const { mocks, log, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { class MockV2ApiKeyUnauthenticatedError extends Error {} return { mocks: { @@ -15,10 +15,23 @@ const { mocks, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { list: vi.fn(), create: vi.fn(), }, + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, MockV2ApiKeyUnauthenticatedError, } }) +/** + * Overrides the global logger mock with one stable instance so the malformed-row + * warnings can be asserted — `createLogger` is called at module load, before any + * `beforeEach` could capture the per-call mock the global stub returns. + */ +vi.mock('@sim/logger', () => ({ + createLogger: () => log, + logger: log, + runWithRequestContext: (_ctx: unknown, fn: () => T): T => fn(), + getRequestContext: () => undefined, +})) + vi.mock('@/lib/api/server/routes/v2-api-key-auth', () => ({ authenticateV2ApiKey: mocks.authenticate, V2ApiKeyUnauthenticatedError: MockV2ApiKeyUnauthenticatedError, @@ -230,6 +243,92 @@ describe('/api/v2/custom-tools', () => { expect(mocks.create).not.toHaveBeenCalled() }) + /** + * Both shapes below are real production rows. A single one of them used to + * throw out of the shared response validator and 500 the entire page, and + * because the list is keyset-paginated the caller could never page past it. + */ + describe('malformed stored rows', () => { + const malformed = (id: string, schema: unknown) => ({ ...tool, id, title: id, schema }) + + async function list() { + const response = await GET(request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}`)) + return { status: response.status, body: await response.json() } + } + + it('recovers a schema stored as a stringified JSON object', async () => { + mocks.list.mockResolvedValue({ + tools: [malformed('stringified', JSON.stringify(TOOL_SCHEMA)), tool], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data.map((t: { id: string }) => t.id)).toEqual(['stringified', 'tool-1']) + expect(body.data[0].schema).toEqual(TOOL_SCHEMA) + expect(log.warn).toHaveBeenCalledWith( + expect.stringContaining('Repaired'), + expect.objectContaining({ toolId: 'stringified', repairs: ['parsed-json-string'] }) + ) + }) + + it('recovers a declaration missing the `type` discriminator', async () => { + mocks.list.mockResolvedValue({ + tools: [malformed('no-type', { function: TOOL_SCHEMA.function }), tool], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data.map((t: { id: string }) => t.id)).toEqual(['no-type', 'tool-1']) + expect(body.data[0].schema).toEqual(TOOL_SCHEMA) + expect(log.warn).toHaveBeenCalledWith( + expect.stringContaining('Repaired'), + expect.objectContaining({ + toolId: 'no-type', + repairs: ['filled-function-discriminator'], + }) + ) + }) + + it('serves the rest of the page when a row is not safely repairable', async () => { + mocks.list.mockResolvedValue({ + tools: [ + malformed('unparseable', 'this is not json'), + malformed('no-parameters-type', { + type: 'function', + function: { name: 'x', parameters: { properties: {} } }, + }), + tool, + ], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data.map((t: { id: string }) => t.id)).toEqual(['tool-1']) + for (const toolId of ['unparseable', 'no-parameters-type']) { + expect(log.warn).toHaveBeenCalledWith( + expect.stringContaining('Omitted'), + expect.objectContaining({ toolId, workspaceId: WORKSPACE_ID }) + ) + } + }) + + it('still mints a next cursor when the page contained a skipped row', async () => { + mocks.list.mockResolvedValue({ + tools: [malformed('unparseable', 'this is not json')], + nextCursorKeys: ['2026-01-01T00:00:00.000Z', 'unparseable'], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data).toEqual([]) + expect(body.nextCursor).toEqual(expect.any(String)) + }) + }) + it('rejects invalid list sort fields before application execution', async () => { const response = await GET( request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}&sortBy=invalid`) diff --git a/apps/sim/app/api/v2/custom-tools/route.ts b/apps/sim/app/api/v2/custom-tools/route.ts index 4cbfafb5492..8f0a9d2c65e 100644 --- a/apps/sim/app/api/v2/custom-tools/route.ts +++ b/apps/sim/app/api/v2/custom-tools/route.ts @@ -14,7 +14,7 @@ import { createWorkspaceCustomToolUseCase, listWorkspaceCustomToolsUseCase, } from '@/lib/custom-tools/application/use-cases' -import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { toV2CustomTool, toV2CustomToolList } from '@/app/api/v2/custom-tools/utils' import { readSortedCursor, writeSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' @@ -46,7 +46,7 @@ export const GET = defineV2JsonRoute({ }), useCase: listWorkspaceCustomToolsUseCase, present: ({ tools, nextCursorKeys }, { query }) => ({ - data: tools.map(toV2CustomTool), + data: toV2CustomToolList(tools), nextCursor: writeSortedCursor( nextCursorKeys, query.sortBy, diff --git a/apps/sim/app/api/v2/custom-tools/utils.ts b/apps/sim/app/api/v2/custom-tools/utils.ts index d3f1181a10e..5af4fdec8b8 100644 --- a/apps/sim/app/api/v2/custom-tools/utils.ts +++ b/apps/sim/app/api/v2/custom-tools/utils.ts @@ -1,21 +1,159 @@ import type { customTools } from '@sim/db/schema' -import type { V2CustomTool } from '@/lib/api/contracts/v2/custom-tools' +import { createLogger } from '@sim/logger' +import { isPlainRecord } from '@sim/utils/object' +import { type V2CustomTool, v2CustomToolSchema } from '@/lib/api/contracts/v2/custom-tools' /** Shared serialization + error mapping for the v2 custom tool surface. */ +const logger = createLogger('V2CustomToolsSerialization') + type CustomToolRow = typeof customTools.$inferSelect /** - * Public custom tool projection. `workspaceId` and `userId` are internal - * scoping columns and are not exposed. + * A stored row whose `schema` column cannot be projected onto the public + * contract even after the safe repairs in {@link repairStoredSchema}. + * + * Single-resource surfaces throw this; the list surface skips the row instead + * so one corrupt row cannot make a whole page unreachable. */ -export function toV2CustomTool(row: CustomToolRow): V2CustomTool { - return { +export class MalformedCustomToolRowError extends Error { + constructor( + readonly toolId: string, + readonly reason: string + ) { + super(`Custom tool ${toolId} has a malformed stored schema: ${reason}`) + this.name = 'MalformedCustomToolRowError' + } +} + +/** Identity fields safe to log for locating a bad row. Never includes `code`. */ +function rowIdentity(row: CustomToolRow) { + return { toolId: row.id, workspaceId: row.workspaceId, title: row.title } +} + +/** + * Safe, information-preserving normalizations for a `schema` column that drifted + * from the contract shape. Both are hypotheses — nothing here is trusted; the + * result is still validated against the response contract before it is emitted, + * so a wrong guess can only downgrade a row to "skipped", never emit bad data. + * + * 1. A `schema` persisted as a JSON *string* is parsed. This is a pure encoding + * fix: the stored bytes already describe the right object. + * 2. A declaration missing the top-level `type` discriminator gets `'function'`. + * The contract types that field as `z.literal('function')`, so there is + * exactly one legal value and filling it invents no information. + * + * Deliberately NOT repaired: `function.parameters.type`, which the contract + * types as an open `z.string()`. Substituting `'object'` there would be a guess + * about JSON-Schema semantics that changes how a model calls the tool. + */ +function repairStoredSchema(stored: unknown): { value: unknown; repairs: string[] } { + const repairs: string[] = [] + let value = stored + + if (typeof value === 'string') { + try { + value = JSON.parse(value) + repairs.push('parsed-json-string') + } catch { + return { value: stored, repairs } + } + } + + if (isPlainRecord(value) && value.type === undefined && isPlainRecord(value.function)) { + value = { ...value, type: 'function' } + repairs.push('filled-function-discriminator') + } + + return { value, repairs } +} + +/** + * Projects a stored row onto the public contract, repairing what is safely + * repairable. Reports a reason instead when the row cannot be made + * contract-valid. + * + * `workspaceId` and `userId` are internal scoping columns and are not exposed. + */ +function projectV2CustomTool(row: CustomToolRow): { tool: V2CustomTool } | { reason: string } { + const { value, repairs } = repairStoredSchema(row.schema) + + const parsed = v2CustomToolSchema.safeParse({ id: row.id, title: row.title, - schema: row.schema as V2CustomTool['schema'], + schema: value, code: row.code, createdAt: row.createdAt.toISOString(), updatedAt: row.updatedAt.toISOString(), + }) + + if (!parsed.success) { + return { + reason: parsed.error.issues + .map((issue) => `${issue.path.join('.') || ''}: ${issue.message}`) + .join('; '), + } + } + + if (repairs.length > 0) { + logger.warn('Repaired a malformed stored custom tool schema', { + ...rowIdentity(row), + repairs, + }) + } + + return { tool: parsed.data } +} + +/** + * Public custom tool projection for single-resource surfaces (read, create, + * update), where there is no other row to serve and failing loudly is the + * honest outcome. + * + * @throws {MalformedCustomToolRowError} when the row is not contract-valid. + */ +export function toV2CustomTool(row: CustomToolRow): V2CustomTool { + const result = projectV2CustomTool(row) + if ('reason' in result) { + logger.error('Custom tool row cannot be projected onto the v2 contract', { + ...rowIdentity(row), + reason: result.reason, + }) + throw new MalformedCustomToolRowError(row.id, result.reason) } + return result.tool +} + +/** + * Public custom tool projection for the keyset-paginated list. + * + * Rows that stay malformed after repair are omitted and logged at `warn` rather + * than thrown. Throwing here fails the whole page, and because the list is + * keyset-paginated the caller cannot page past the bad row — every page + * containing it becomes permanently unreachable. An incomplete page is a real + * cost, but it is strictly smaller than no page at all, and the omission is + * recorded server-side with enough identity to find and fix the row. + * + * Pagination stays coherent: `nextCursor` is minted from the keys the use case + * read out of the database, not from this projection, so a skipped row still + * advances the cursor past itself. The list response carries no total, so no + * count metadata contradicts a short page — a caller must follow `nextCursor` + * rather than infer completeness from a page's length. + */ +export function toV2CustomToolList(rows: CustomToolRow[]): V2CustomTool[] { + const tools: V2CustomTool[] = [] + + for (const row of rows) { + const result = projectV2CustomTool(row) + if ('reason' in result) { + logger.warn('Omitted a malformed custom tool row from the v2 list response', { + ...rowIdentity(row), + reason: result.reason, + }) + continue + } + tools.push(result.tool) + } + + return tools } diff --git a/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts b/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts index c947dff15a9..6401ceb1850 100644 --- a/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts +++ b/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts @@ -35,6 +35,7 @@ vi.mock('@/lib/users/queries', () => ({ })) import { NoWorkspaceAccessError } from '@/lib/core/application' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { GET } from '@/app/api/v2/files/[fileId]/metadata/route' const WORKSPACE_ID = 'workspace-1' @@ -79,6 +80,15 @@ function buildRecord() { const callGet = (query: string) => GET(new NextRequest(`http://localhost:3000/api/v2/files/${FILE_ID}/metadata?${query}`), context) +/** + * Stands in for the application use case's lifecycle predicate: a soft-deleted row only + * resolves when the caller opted into the archived set through `includeDeleted`. + */ +const archivedFileUseCase = async ({ input }: { input: { includeDeleted?: boolean } }) => { + if (!input.includeDeleted) throw new OrchestrationError('not_found', 'File not found') + return { file: { ...buildRecord(), deletedAt: new Date('2024-01-03T00:00:00Z') }, share: SHARE } +} + describe('GET /api/v2/files/[fileId]/metadata', () => { beforeEach(() => { vi.clearAllMocks() @@ -138,11 +148,82 @@ describe('GET /api/v2/files/[fileId]/metadata', () => { }) expect(mocks.readMetadata).toHaveBeenCalledWith({ principal: auth.principal, - input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID }, + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: false }, request: expect.anything(), }) }) + it('leaves an archived file unreachable when scope is omitted', async () => { + mocks.readMetadata.mockImplementation(archivedFileUseCase) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}`) + + expect(response.status).toBe(404) + expect((await response.json()).error.code).toBe('NOT_FOUND') + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: false }, + }) + ) + }) + + it('leaves an archived file unreachable under an explicit scope=active', async () => { + mocks.readMetadata.mockImplementation(archivedFileUseCase) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=active`) + + expect(response.status).toBe(404) + expect((await response.json()).error.code).toBe('NOT_FOUND') + }) + + it('returns archived metadata when scope=archived opts into the archived set', async () => { + mocks.readMetadata.mockImplementation(archivedFileUseCase) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=archived`) + + expect(response.status).toBe(200) + expect(await response.json()).toEqual({ + data: { + id: FILE_ID, + name: 'data.csv', + size: 1024, + type: 'text/csv', + key: 'workspace/ws/1-x-data.csv', + folderPath: '/', + uploadedByEmail: 'ada@example.com', + uploadedAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-02T00:00:00.000Z', + deletedAt: '2024-01-03T00:00:00.000Z', + share: SHARE, + }, + }) + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + principal: auth.principal, + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: true }, + }) + ) + }) + + it('still conceals an unauthorized archived read behind the same 404', async () => { + mocks.readMetadata.mockRejectedValue(new NoWorkspaceAccessError()) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=archived`) + + expect(response.status).toBe(404) + expect((await response.json()).error.code).toBe('NOT_FOUND') + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ principal: auth.principal }) + ) + }) + + it('rejects an unrecognized scope before reaching the use case', async () => { + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=all`) + + expect(response.status).toBe(400) + expect(mocks.readMetadata).not.toHaveBeenCalled() + }) + it('returns a null share when the file has no share configuration', async () => { mocks.readMetadata.mockResolvedValueOnce({ file: buildRecord(), share: null }) diff --git a/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts b/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts index 68d1420c541..722d125e4da 100644 --- a/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts +++ b/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts @@ -8,7 +8,15 @@ import { toV2File } from '@/app/api/v2/files/utils' export const dynamic = 'force-dynamic' export const revalidate = 0 -/** GET /api/v2/files/[fileId]/metadata — Return file metadata without downloading its bytes. */ +/** + * GET /api/v2/files/[fileId]/metadata — Return file metadata without downloading its bytes. + * + * `scope` mirrors the list endpoint's lifecycle selector: it defaults to `active`, and only an + * explicit `scope=archived` relaxes the soft-delete predicate on the row lookup so a caller can + * inspect an archived file before restoring it. Authorization is unaffected — the use case still + * resolves the canonical workspace context for the file and authorizes `files.read_metadata` + * against it either way. + */ export const GET = defineV2JsonRoute({ contract: v2GetFileContract, auth: v2ApiKeyAuth, @@ -18,6 +26,7 @@ export const GET = defineV2JsonRoute({ mapInput: ({ params, query }) => ({ fileId: params.fileId, assertedWorkspaceId: query.workspaceId, + includeDeleted: query.scope === 'archived', }), useCase: readWorkspaceFileMetadata, present: async ({ file, share }) => ({ data: { ...(await toV2File(file)), share } }), diff --git a/apps/sim/app/api/v2/lib/response.test.ts b/apps/sim/app/api/v2/lib/response.test.ts index f84dd84a203..460b188a136 100644 --- a/apps/sim/app/api/v2/lib/response.test.ts +++ b/apps/sim/app/api/v2/lib/response.test.ts @@ -2,7 +2,8 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { v2Error } from '@/app/api/v2/lib/response' +import { HttpError } from '@/lib/core/utils/http-error' +import { v2Error, v2HttpError, v2RateLimitError } from '@/app/api/v2/lib/response' describe('v2Error retry guidance', () => { it('sends Retry-After on 503 so a client does not retry a degraded dependency immediately', () => { @@ -46,3 +47,64 @@ describe('v2Error retry guidance', () => { expect(response.headers.get('Retry-After')).toBeNull() }) }) + +/** + * RFC 9110 §11.6.1 makes `WWW-Authenticate` a MUST on 401, and `v2Error` is the + * one funnel every v2 401 passes through — the missing key, the invalid key, an + * `unauthorized` orchestration failure, and the v1 middleware's auth result all + * render here. + */ +describe('v2 401 authentication challenge', () => { + const challenge = () => + v2Error('UNAUTHORIZED', 'API key required').headers.get('WWW-Authenticate') + + it('sends a challenge on 401', () => { + const response = v2Error('UNAUTHORIZED', 'Invalid API key') + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBeTruthy() + }) + + it('names the x-api-key header, the only channel v2 actually reads', () => { + expect(challenge()).toContain('x-api-key') + }) + + it('does not advertise a scheme v2 does not accept', () => { + const value = challenge() ?? '' + const scheme = value.split(' ')[0].toLowerCase() + + expect(scheme).not.toBe('bearer') + expect(scheme).not.toBe('basic') + expect(scheme).not.toBe('digest') + }) + + it('challenges on a 401 reached through the rate-limit auth result', () => { + const response = v2RateLimitError({ + allowed: false, + remaining: 0, + resetAt: new Date(), + limit: 0, + error: 'Invalid API key', + }) + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBeTruthy() + }) + + it('challenges on a 401 reached through a typed HTTP error', () => { + class UnauthorizedError extends HttpError { + readonly statusCode = 401 + } + + const response = v2HttpError(new UnauthorizedError('Invalid API key')) + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBeTruthy() + }) + + it('does not challenge on statuses that are not 401', () => { + for (const code of ['BAD_REQUEST', 'FORBIDDEN', 'NOT_FOUND', 'RATE_LIMITED'] as const) { + expect(v2Error(code, 'nope').headers.get('WWW-Authenticate')).toBeNull() + } + }) +}) diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index 8f5f61215b5..c96763ac4e9 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -100,6 +100,28 @@ const RETRY_AFTER_SECONDS_BY_STATUS: Partial> = { 503: ADMISSION_RETRY_AFTER_SECONDS, } +/** + * The challenge every v2 `401` carries, so a 401 is a complete one. + * + * RFC 9110 §11.6.1 makes `WWW-Authenticate` a MUST on 401 — a 401 without it is + * a refusal that never says what would have been accepted, and a generic HTTP + * client has nothing to react to. + * + * The scheme name is deliberately Sim-specific rather than a registered one. + * v2 authenticates from the `x-api-key` header and accepts no `Authorization` + * scheme at all — `Authorization: Bearer ` is not a channel here — so + * `Bearer` and `Basic` would both be false advertising. `Basic` is worse than + * false: a browser reacts to it by opening a native credential prompt that + * cannot produce an API key. An unregistered scheme is what remains, and it is + * legal: §11.6.1's grammar requires *an* `auth-scheme` token, not a registered + * one. Every challenge implies "retry via `Authorization: …`" by + * construction, so the token is chosen to be one no client has a built-in + * handler for — the challenge surfaces to a human instead of triggering an + * automatic retry down a channel v2 does not read — and the real channel is + * named outright in the `header` parameter beside it. + */ +const V2_AUTH_CHALLENGE = 'SimApiKey realm="Sim API", header="x-api-key"' + type RateLimitHeaderSource = Pick function rateLimitHeaders(rateLimit?: RateLimitHeaderSource): Record { @@ -183,6 +205,7 @@ export function v2Error( status, headers: { ...PRIVATE_NO_STORE, + ...(status === 401 ? { 'WWW-Authenticate': V2_AUTH_CHALLENGE } : {}), ...(retryAfterSeconds === undefined ? {} : { 'Retry-After': retryAfterSeconds.toString() }), ...options.headers, }, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts new file mode 100644 index 00000000000..12f0b1fbea5 --- /dev/null +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts @@ -0,0 +1,121 @@ +/** + * @vitest-environment node + */ + +import { + V2_OPERATION_RATE_LIMIT_ALLOWED, + V2_PREAUTH_RATE_LIMIT_ALLOWED, + v2ApiKeyAuthModuleMock, + v2GateModuleMock, + v2RateLimiterModuleMock, + v2RouteMocks, +} from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + cancel: vi.fn(), + capture: vi.fn(), +})) + +vi.mock('@/lib/api/server/routes/v2-api-key-auth', () => v2ApiKeyAuthModuleMock) +vi.mock('@/lib/core/rate-limiter', () => v2RateLimiterModuleMock) +vi.mock('@/app/api/v2/lib/gate', () => v2GateModuleMock) +vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mocks.capture })) +vi.mock('@/lib/workflows/application/cancel-run', () => ({ + cancelWorkflowRun: { operation: { id: 'workflows.runs.cancel' }, execute: mocks.cancel }, +})) + +import { POST } from '@/app/api/v2/workflows/[id]/runs/[runId]/cancel/route' + +const WORKSPACE_ID = 'workspace-1' +const WORKFLOW_ID = 'workflow-1' +const RUN_ID = 'run-1' + +const principal = { + kind: 'personal_api_key' as const, + userId: 'user-1', + keyId: 'key-1', +} +const auth = { + principal, + rolloutUserId: 'user-1', + rateLimitSubjectIds: ['api-key:key-1'], + rateLimitSubscription: null, + keyType: 'personal' as const, +} + +const context = { params: Promise.resolve({ id: WORKFLOW_ID, runId: RUN_ID }) } + +function request() { + return new NextRequest( + `http://localhost:3000/api/v2/workflows/${WORKFLOW_ID}/runs/${RUN_ID}/cancel`, + { method: 'POST', headers: { 'x-api-key': 'secret' } } + ) +} + +/** The service result the use case hands back, minus the outcome under test. */ +function serviceResult(overrides: Record) { + return { + executionId: RUN_ID, + redisAvailable: true, + locallyAborted: false, + pausedCancelled: false, + workflowId: WORKFLOW_ID, + workspaceId: WORKSPACE_ID, + ...overrides, + } +} + +describe('POST /api/v2/workflows/[id]/runs/[runId]/cancel', () => { + beforeEach(() => { + vi.clearAllMocks() + v2RouteMocks.authenticate.mockResolvedValue(auth) + v2RouteMocks.gate.mockResolvedValue(null) + v2RouteMocks.preauthRate.mockResolvedValue(V2_PREAUTH_RATE_LIMIT_ALLOWED) + v2RouteMocks.operationRate.mockResolvedValue(V2_OPERATION_RATE_LIMIT_ALLOWED) + }) + + it('reports a durable write when an active run is cancelled', async () => { + mocks.cancel.mockResolvedValue( + serviceResult({ success: true, durablyRecorded: true, reason: 'recorded' }) + ) + + const response = await POST(request(), context) + + expect(response.status).toBe(200) + expect((await response.json()).data).toEqual({ + success: true, + runId: RUN_ID, + redisAvailable: true, + durablyRecorded: true, + locallyAborted: false, + pausedCancelled: false, + reason: 'recorded', + }) + }) + + /** + * The published outcome of a cancel against a run that had already finished. + * `durablyRecorded: true` here is the defect this suite pins: nothing was + * written, so a caller reconciling on that flag would trust a write that never + * happened. + */ + it.each([ + ['cancelled', 'already_cancelled'], + ['completed', 'already_completed'], + ['failed', 'already_failed'], + ])('reports a terminal %s run as a no-op the caller can tell apart', async (_status, reason) => { + mocks.cancel.mockResolvedValue(serviceResult({ success: true, durablyRecorded: false, reason })) + + const response = await POST(request(), context) + + expect(response.status).toBe(200) + expect((await response.json()).data).toMatchObject({ + success: true, + runId: RUN_ID, + durablyRecorded: false, + reason, + }) + }) +}) diff --git a/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts b/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts index 84ba7ec5d3a..21c2fa13e9b 100644 --- a/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts +++ b/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts @@ -1010,6 +1010,7 @@ describe('POST /api/workflows/[id]/executions/[executionId]/cancel', () => { expect(response.status).toBe(200) await expect(response.json()).resolves.toMatchObject({ success: true, + durablyRecorded: false, reason: 'already_cancelled', }) expect(mockCancelByExecution).not.toHaveBeenCalled() diff --git a/apps/sim/lib/api/contracts/primitives.test.ts b/apps/sim/lib/api/contracts/primitives.test.ts index 4e8a605a98f..b9a2d791976 100644 --- a/apps/sim/lib/api/contracts/primitives.test.ts +++ b/apps/sim/lib/api/contracts/primitives.test.ts @@ -2,14 +2,17 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { z } from 'zod' import { customPatternSchema, isCanonicalBase64, + MAX_ID_LENGTH, organizationIdSchema, piiStagePolicySchema, piiStagesSchema, privateSecretProvenanceBundleSchema, resolvedSecretTraceProvenanceSchema, + withMissingFieldMessage, workflowIdSchema, workspaceFileIdSchema, workspaceFileNameSchema, @@ -257,4 +260,77 @@ describe('shared id schemas name the field when it is missing', () => { expect(result.error?.issues[0]?.message).not.toContain('received undefined') }) + + /** + * A schema-level `error` string replaces the message for *every* issue, so the + * required-field wording used to answer a wrong-typed value too: `{"name": 123}` + * came back "Name is required" when a name had in fact been supplied. Missing and + * wrong-typed are different mistakes and must read differently. + */ + for (const [name, schema, message] of cases) { + it(`${name}: a wrong-typed value reports the type, not "${message}"`, () => { + const result = schema.safeParse(123) + + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.message).toBe( + 'Invalid input: expected string, received number' + ) + }) + } + + it('workspaceFileNameSchema separates a missing name from a wrong-typed one', () => { + expect(workspaceFileNameSchema.safeParse(undefined).error?.issues[0]?.message).toBe( + 'Name is required' + ) + expect(workspaceFileNameSchema.safeParse(123).error?.issues[0]?.message).toBe( + 'Invalid input: expected string, received number' + ) + }) +}) + +/** + * An unbounded `workspaceId` reached the workspace lookup at whatever length the + * caller chose. No workspace id this repo mints approaches the bound, so it only + * rejects values that could never have resolved. + */ +describe('workspaceIdSchema length bound', () => { + it('accepts an id at the bound', () => { + expect(workspaceIdSchema.safeParse('a'.repeat(MAX_ID_LENGTH)).success).toBe(true) + }) + + it('rejects an id one character past the bound, naming the field', () => { + const result = workspaceIdSchema.safeParse('a'.repeat(MAX_ID_LENGTH + 1)) + + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.message).toBe('Workspace ID is too long') + }) + + it('still accepts a UUID workspace id', () => { + expect(workspaceIdSchema.safeParse('7a6cce2b-78b8-40bc-b8d3-0a2a6dfd9023').success).toBe(true) + }) +}) + +describe('withMissingFieldMessage', () => { + const base = z.string().min(1, 'Description is required').max(8, 'Description is too long') + const retrofitted = withMissingFieldMessage(base, 'Description is required') + + it('names the field when the value is omitted', () => { + expect(retrofitted.safeParse(undefined).error?.issues[0]?.message).toBe( + 'Description is required' + ) + }) + + it('keeps Zod default wording for a wrong-typed value', () => { + expect(retrofitted.safeParse(5).error?.issues[0]?.message).toBe( + 'Invalid input: expected string, received number' + ) + }) + + it('preserves the checks the source schema carried', () => { + expect(retrofitted.safeParse('').error?.issues[0]?.message).toBe('Description is required') + expect(retrofitted.safeParse('a'.repeat(9)).error?.issues[0]?.message).toBe( + 'Description is too long' + ) + expect(retrofitted.safeParse('ok').success).toBe(true) + }) }) diff --git a/apps/sim/lib/api/contracts/primitives.ts b/apps/sim/lib/api/contracts/primitives.ts index 39b7d6492a8..90306e53060 100644 --- a/apps/sim/lib/api/contracts/primitives.ts +++ b/apps/sim/lib/api/contracts/primitives.ts @@ -192,34 +192,74 @@ export const jobIdParamsSchema = z.object({ */ export const nonEmptyIdSchema = z.string().min(1) +/** + * Schema-level error customizer that applies a message **only when the value is + * absent**, and defers to Zod's default wording for everything else. + * + * A plain `z.string({ error: message })` replaces the message for *every* issue + * the schema raises, including `invalid_type`. A caller who sent `{"name": 123}` + * then reads `Name is required` — a name was supplied, it was the wrong type, and + * the message sends them looking for the wrong bug. Returning `undefined` for a + * present-but-wrong-typed value lets Zod render `Invalid input: expected string, + * received number` instead. + */ +export function missingFieldError(message: string) { + return (issue: z.core.$ZodRawIssue): string | undefined => + issue.input === undefined ? message : undefined +} + +/** + * Re-issues an existing string schema with a missing-value message, keeping every + * check (bounds, regex, trim) it already carries. + * + * Use this when the field's bounds are owned by a shared schema elsewhere and only + * the omitted-field wording needs to be added at this boundary — re-declaring the + * bounds locally would let the two copies drift. + */ +export function withMissingFieldMessage( + schema: TSchema, + message: string +): TSchema { + return schema.clone({ ...schema._zod.def, error: missingFieldError(message) }) +} + +/** + * Bound shared by the id primitives below. Every identifier this repo mints — + * UUID v4, `wf_`, and the legacy free-form `text` keys — is far shorter, + * so the bound rejects only values that were never going to resolve while keeping + * an unbounded string from reaching a lookup. + */ +export const MAX_ID_LENGTH = 128 + /** * Builds a required, non-empty string schema whose message covers **both** * failure modes. * * `.min(1, message)` alone only fires for a present-but-empty string; an omitted * field falls through to Zod's default `Invalid input: expected string, received - * undefined`, which never names the field the caller left out. Passing the same - * message to the `z.string({ error })` constructor closes that gap. + * undefined`, which never names the field the caller left out. + * {@link missingFieldError} closes that gap without also swallowing the + * wrong-type message. * * Prefer this over a bare `z.string().min(1, '...')` for any required request * field. When a named primitive below already carries the right wording, import * that instead of rebuilding it here. */ export function requiredFieldSchema(message: string) { - return z.string({ error: message }).min(1, message) + return z.string({ error: missingFieldError(message) }).min(1, message) } /** Non-empty `workspaceId` field with a stable, human-readable message. */ -export const workspaceIdSchema = requiredFieldSchema('Workspace ID is required').describe( - 'Unique workspace identifier.' -) +export const workspaceIdSchema = requiredFieldSchema('Workspace ID is required') + .max(MAX_ID_LENGTH, 'Workspace ID is too long') + .describe('Unique workspace identifier.') /** * A single workspace-file name, not a path. Folder placement is carried by a * separate folder id or path field, so separators and dot segments are invalid. */ export const workspaceFileNameSchema = z - .string({ error: 'Name is required' }) + .string({ error: missingFieldError('Name is required') }) .trim() .min(1, 'Name is required') .max(255, 'Name is too long') @@ -257,7 +297,7 @@ export const runIdSchema = z * two-state and three-state spellings stay explicit at each call site. */ export const folderIdSchema = requiredFieldSchema('Folder ID is required').max( - 128, + MAX_ID_LENGTH, 'Folder ID is too long' ) @@ -269,7 +309,7 @@ export const folderIdSchema = requiredFieldSchema('Folder ID is required').max( * UUID-only schema — a `.uuid()` constraint here silently 400s every `wf_` file. */ export const workspaceFileIdSchema = requiredFieldSchema('File ID is required') - .max(128, 'File ID is too long') + .max(MAX_ID_LENGTH, 'File ID is too long') .regex(/^[A-Za-z0-9_-]+$/, 'Invalid file id') /** diff --git a/apps/sim/lib/api/contracts/v2/files.ts b/apps/sim/lib/api/contracts/v2/files.ts index 30fdfcc42cc..b422d7792c0 100644 --- a/apps/sim/lib/api/contracts/v2/files.ts +++ b/apps/sim/lib/api/contracts/v2/files.ts @@ -334,6 +334,29 @@ export const v2FileWorkspaceQuerySchema = z export type V2FileWorkspaceQuery = z.output +/** + * Metadata read: the workspace scope plus the same `scope` lifecycle selector the + * list endpoint uses, so a caller that found a file under `GET /files?scope=archived` + * can read it back with the identical spelling. + * + * The default stays `active`, which keeps the read on the live set and continues to + * answer `404` for a soft-deleted file. `scope` only relaxes the `deleted_at` predicate + * on the row lookup — the workspace the file belongs to, the asserted-workspace check, + * and the operation's authorization are unchanged, so it cannot widen who may read. + */ +export const v2GetFileMetadataQuerySchema = z + .object({ + workspaceId: workspaceIdSchema.describe('Workspace that owns the file.'), + scope: v2FileScopeSchema + .default('active') + .describe( + 'Which lifecycle set to read from: `active` (default) resolves live files only and returns `404` for a file a `DELETE` soft-deleted; `archived` also resolves soft-deleted files, so metadata stays readable before `POST /files/{fileId}/restore`. Authorization is identical for both.' + ), + }) + .strict() + +export type V2GetFileMetadataQuery = z.output + export const v2RenameFileBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace that owns the file.'), @@ -610,7 +633,7 @@ export const v2GetFileContract = defineRouteContract({ method: 'GET', path: '/api/v2/files/[fileId]/metadata', params: v2FileParamsSchema, - query: v2FileWorkspaceQuerySchema, + query: v2GetFileMetadataQuerySchema, response: { mode: 'json', schema: v2DataResponse(v2FileMetadataSchema), diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 52cdf284756..b30e5900a1a 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -836,8 +836,16 @@ export const MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS = 10 */ export const MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH = 8192 * 4 -export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema - .safeExtend({ +/** + * Rebuilt from the v1 shape rather than extended from the v1 schema: v1 carries + * the "query or tagFilters" rule as a bare `.refine`, which reports at path `[]`, + * so no client could attach the failure to a field. The rule is restated below as + * a `superRefine` with a `path` — extending v1 would inherit the pathless issue + * alongside it and report the same violation twice. + */ +export const v2KnowledgeSearchBodySchema = z + .object({ + ...v1KnowledgeSearchBodySchema.shape, workspaceId: v1KnowledgeSearchBodySchema.shape.workspaceId.describe( 'Workspace that owns the knowledge bases.' ), @@ -855,9 +863,14 @@ export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema `Natural-language query; required when tag filters are omitted. At most ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters — longer text exceeds the embedding model's per-input token ceiling and would be truncated before the billed search ran.` ) .meta({ examples: ['How do I reset my password?'] }), - topK: v1KnowledgeSearchBodySchema.shape.topK.describe( - 'Maximum number of search results to return. Must be a whole number between 1 and 100; the boundary schema only bounds the range, so a fractional value is admitted here and then rejected with 400 during search.' - ), + topK: z + .number() + .min(1, 'topK must be at least 1') + .max(100, 'topK cannot exceed 100') + .default(10) + .describe( + 'Maximum number of search results to return. Must be a whole number between 1 and 100; the boundary schema only bounds the range, so a fractional value is admitted here and then rejected with 400 during search.' + ), tagFilters: z .array(v2KnowledgeSearchTagFilterSchema) .max( @@ -909,6 +922,22 @@ export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema * parameters never arrived. */ .strict() + /** + * A search with neither a query nor a tag filter has nothing to retrieve on. + * Reported on `query`, the field a caller who sent neither is most likely to be + * missing, so the failure lands on an input instead of on the request as a whole. + */ + .superRefine((body, ctx) => { + const hasQuery = Boolean(body.query && body.query.trim().length > 0) + const hasTagFilters = Boolean(body.tagFilters && body.tagFilters.length > 0) + if (!hasQuery && !hasTagFilters) { + ctx.addIssue({ + code: 'custom', + path: ['query'], + message: 'Either query or tagFilters must be provided', + }) + } + }) export type V2KnowledgeSearchBody = z.input export const v2SearchKnowledgeContract = defineRouteContract({ diff --git a/apps/sim/lib/api/contracts/v2/required-field-messages.test.ts b/apps/sim/lib/api/contracts/v2/required-field-messages.test.ts new file mode 100644 index 00000000000..e72f4e2a57d --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/required-field-messages.test.ts @@ -0,0 +1,175 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { v2KnowledgeSearchBodySchema } from '@/lib/api/contracts/v2/knowledge' +import { v2CreateSkillBodySchema } from '@/lib/api/contracts/v2/skills' +import { v2CreateWorkflowBodySchema } from '@/lib/api/contracts/v2/workflows' + +const workspaceId = '7a6cce2b-78b8-40bc-b8d3-0a2a6dfd9023' +const knowledgeBaseIds = ['ae814592-a730-4ad6-8741-b51e48843300'] + +function messageAt( + result: { success: boolean; error?: { issues: { path: PropertyKey[]; message: string }[] } }, + field: string +) { + return result.error?.issues.find((issue) => issue.path[0] === field)?.message +} + +/** + * A required field that is *omitted* and one that is *wrong-typed* are different + * mistakes. Both used to answer with wording that pointed at the other: the + * create bodies leaked Zod's default "expected string, received undefined" for an + * omitted field, and the shared string primitives answered "… is required" for a + * value that had in fact been supplied. + */ +describe('v2 create bodies name a missing required field', () => { + it('POST /v2/workflows names an omitted name and types a wrong-typed one', () => { + const missing = v2CreateWorkflowBodySchema.safeParse({ workspaceId }) + expect(messageAt(missing, 'name')).toBe('name is required') + + const wrongType = v2CreateWorkflowBodySchema.safeParse({ workspaceId, name: 123 }) + expect(messageAt(wrongType, 'name')).toBe('Invalid input: expected string, received number') + + const empty = v2CreateWorkflowBodySchema.safeParse({ workspaceId, name: ' ' }) + expect(messageAt(empty, 'name')).toBe('name is required') + }) + + it('POST /v2/workflows still names an omitted workspaceId', () => { + const result = v2CreateWorkflowBodySchema.safeParse({ name: 'Triage' }) + expect(messageAt(result, 'workspaceId')).toBe('Workspace ID is required') + }) + + it('POST /v2/skills names each omitted required field', () => { + const result = v2CreateSkillBodySchema.safeParse({ workspaceId }) + + expect(messageAt(result, 'name')).toBe('Skill name is required') + expect(messageAt(result, 'description')).toBe('Description is required') + expect(messageAt(result, 'content')).toBe('Content is required') + }) + + it('POST /v2/skills types a wrong-typed field instead of calling it missing', () => { + const result = v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'my-skill', + description: 123, + content: 'Body', + }) + + expect(messageAt(result, 'description')).toBe('Invalid input: expected string, received number') + }) + + it('POST /v2/skills keeps the bounds the shared field schemas carry', () => { + const badName = v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'Not Kebab', + description: 'A summary', + content: 'Body', + }) + expect(messageAt(badName, 'name')).toBe('Name must be kebab-case (e.g. my-skill)') + + const longContent = v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'my-skill', + description: 'A summary', + content: 'a'.repeat(50_001), + }) + expect(messageAt(longContent, 'content')).toBe('Content is too large') + + expect( + v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'my-skill', + description: 'A summary', + content: 'Body', + }).success + ).toBe(true) + }) +}) + +/** + * `topK` is the one search field whose range violations answered with Zod's + * default phrasing while every sibling — `limit`, `rerankerInputCount`, + * `tagFilters`, `query` — named itself. + */ +describe('v2 knowledge search topK messages name the field', () => { + it('names topK when it exceeds the maximum', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + topK: 101, + }) + + expect(messageAt(result, 'topK')).toBe('topK cannot exceed 100') + }) + + it('names topK when it is below the minimum', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + topK: 0, + }) + + expect(messageAt(result, 'topK')).toBe('topK must be at least 1') + }) + + it('keeps admitting a fractional topK for the use case to reject, and defaults to 10', () => { + const fractional = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + topK: 2.5, + }) + expect(fractional.success).toBe(true) + + const defaulted = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + }) + expect(defaulted.success && defaulted.data.topK).toBe(10) + }) +}) + +/** + * The "query or tagFilters" rule arrived from v1 as a bare `.refine`, which + * reports at path `[]` — a message no client could attach to a field. + */ +describe('v2 knowledge search reports the missing-input rule on a field', () => { + it('attaches the failure to query', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ workspaceId, knowledgeBaseIds }) + + expect(result.success).toBe(false) + expect(result.error?.issues).toHaveLength(1) + expect(result.error?.issues[0]?.path).toEqual(['query']) + expect(result.error?.issues[0]?.message).toBe('Either query or tagFilters must be provided') + }) + + it('still accepts a tag-only search and a query-only search', () => { + expect( + v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + tagFilters: [{ tagName: 'From', operator: 'contains', value: 'brex' }], + }).success + ).toBe(true) + + expect( + v2KnowledgeSearchBodySchema.safeParse({ workspaceId, knowledgeBaseIds, query: 'hello' }) + .success + ).toBe(true) + }) + + it('rejects a whitespace-only query with no tag filters, as v1 did', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: ' ', + }) + + expect(result.success).toBe(false) + expect(messageAt(result, 'query')).toBe('Either query or tagFilters must be provided') + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/skills.ts b/apps/sim/lib/api/contracts/v2/skills.ts index fe2a8cd026d..03dd5a0f230 100644 --- a/apps/sim/lib/api/contracts/v2/skills.ts +++ b/apps/sim/lib/api/contracts/v2/skills.ts @@ -1,5 +1,10 @@ import { z } from 'zod' -import { noInputSchema, nonEmptyIdSchema, workspaceIdSchema } from '@/lib/api/contracts/primitives' +import { + noInputSchema, + nonEmptyIdSchema, + withMissingFieldMessage, + workspaceIdSchema, +} from '@/lib/api/contracts/primitives' import { skillContentSchema, skillDescriptionSchema, @@ -113,14 +118,23 @@ export const v2ListSkillsQuerySchema = v2SkillWorkspaceQuerySchema export type V2ListSkillsQuery = z.output +/** + * Create body. Every field is required, so each one carries the missing-value + * wording the shared field primitives cannot: those are also spelled `.optional()` + * on the update body, where an omitted field is legal, so the message belongs + * here rather than on the shared schema. + */ export const v2CreateSkillBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace in which to create the skill.'), - name: skillNameSchema.describe( + name: withMissingFieldMessage(skillNameSchema, 'Skill name is required').describe( 'Kebab-case name, unique within the workspace and not reserved by a built-in skill.' ), - description: skillDescriptionSchema.describe('One-line summary of when the skill applies.'), - content: skillContentSchema.describe( + description: withMissingFieldMessage( + skillDescriptionSchema, + 'Description is required' + ).describe('One-line summary of when the skill applies.'), + content: withMissingFieldMessage(skillContentSchema, 'Content is required').describe( 'Skill body containing the instructions given to the agent.' ), }) diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index 8d0aa30a245..4432437b508 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -9,6 +9,7 @@ import { } from '@/lib/api/contracts/deployments' import { booleanQueryFlagSchema, + missingFieldError, noInputSchema, runIdSchema, workspaceIdSchema, @@ -398,7 +399,7 @@ export const v2CreateWorkflowBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace in which to create the workflow.'), name: z - .string() + .string({ error: missingFieldError('name is required') }) .trim() .min(1, 'name is required') .max(255, 'name is too long') @@ -1348,25 +1349,29 @@ export const v2CancelWorkflowRunDataSchema = z redisAvailable: z .boolean() .describe('Whether the distributed cancellation channel was available.'), - durablyRecorded: z.boolean().describe('Whether cancellation was recorded durably.'), + durablyRecorded: z + .boolean() + .describe( + 'Whether this request durably recorded a cancellation. Always false for a run that was already terminal, where the request is satisfied but nothing was written.' + ), locallyAborted: z.boolean().describe('Whether an in-process execution was aborted.'), pausedCancelled: z.boolean().describe('Whether a paused execution was cancelled.'), /** * Always emitted by the cancellation service — it is not a partial-failure - * marker. `recorded` is the full-success value; the other four name the step - * that degraded. + * marker. `recorded` is the full-success value; the `already_*` values name + * a terminal no-op; the rest name the step that degraded. */ reason: cancelWorkflowExecutionReasonSchema .optional() .describe( - 'Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.' + 'Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `already_cancelled`, `already_completed`, and `already_failed` mean the run had already reached that terminal state, so nothing was cancelled and `durablyRecorded` is false. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.' ), }) .meta({ id: 'CancelWorkflowRunResult', title: 'Cancel workflow run result', description: - 'Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, so poll the run to observe its final state.', + 'Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, reported as `durablyRecorded: false` with an `already_*` reason naming the state observed.', }) export type V2CancelWorkflowRunData = z.output diff --git a/apps/sim/lib/api/contracts/workflows.test.ts b/apps/sim/lib/api/contracts/workflows.test.ts index ad4a68c59da..5f026cfa9b6 100644 --- a/apps/sim/lib/api/contracts/workflows.test.ts +++ b/apps/sim/lib/api/contracts/workflows.test.ts @@ -135,13 +135,12 @@ describe('workflow contracts', () => { * The v2 cancel endpoint presents `cancelWorkflowRun`'s result unchanged, and * that use case delegates wholly to the cancellation service — so it cannot * emit the outcomes the internal route resolves for itself. Folding those into - * the service enum would publish four reasons v2 never returns, because the - * v2 contract documents this enum value by value. + * the service enum would publish reasons v2 never returns, because the v2 + * contract documents this enum value by value. */ it('keeps internal-only cancellation reasons out of the enum v2 publishes', () => { for (const reason of [ 'queue_cancelled', - 'already_cancelled', 'active_resume_signal_failed', 'cancellation_not_finalized', ]) { @@ -150,6 +149,19 @@ describe('workflow contracts', () => { } }) + /** + * Both surfaces answer a cancel against an already-terminal run, so both name + * it with the same member. The service observes the terminal status itself now + * — the internal route no longer owns `already_cancelled` privately — and + * without these the v2 contract rejects the very body v2 emits. + */ + it('shares the terminal no-op vocabulary between both cancel surfaces', () => { + for (const reason of ['already_cancelled', 'already_completed', 'already_failed']) { + expect(cancelWorkflowExecutionReasonSchema.options).toContain(reason) + expect(internalCancelWorkflowExecutionReasonSchema.options).toContain(reason) + } + }) + /** * `workflowStateSchema` is the PUT `/api/workflows/[id]/state` body and also * the `state` slot of the GET response. A stored value outside these bounds diff --git a/apps/sim/lib/api/contracts/workflows.ts b/apps/sim/lib/api/contracts/workflows.ts index 45a29933d25..25c2d286427 100644 --- a/apps/sim/lib/api/contracts/workflows.ts +++ b/apps/sim/lib/api/contracts/workflows.ts @@ -669,9 +669,16 @@ export const workflowExecutionStatusQuerySchema = z.object({ * `lib/execution/cancel-workflow-execution` (contracts stay import-clean of * server modules). Keeping the internal route's extra outcomes out of here is * what stops the published v2 schema advertising reasons v2 cannot emit. + * + * `already_cancelled`/`already_completed`/`already_failed` report a run that was + * already terminal when the request arrived: the request is satisfied, but no + * durable write happened, so they always pair with `durablyRecorded: false`. */ export const cancelWorkflowExecutionReasonSchema = z.enum([ 'recorded', + 'already_cancelled', + 'already_completed', + 'already_failed', 'redis_unavailable', 'redis_write_failed', 'paused_event_publish_failed', @@ -679,17 +686,20 @@ export const cancelWorkflowExecutionReasonSchema = z.enum([ ]) /** - * The internal route's vocabulary. It resolves four outcomes before the service - * is ever reached: `queue_cancelled` (the run was still queued, so no execution - * log row existed), `already_cancelled` (reconciling a run already cancelled), - * and the two stop-signal failures. Several ride on `success: true` responses, - * so validating them against the service enum makes `requestJson` reject - * cancellations that genuinely applied. + * The internal route's vocabulary. It reimplements cancellation rather than + * calling the service, so it resolves three further outcomes of its own: + * `queue_cancelled` (the run was still queued, so no execution log row existed), + * `active_resume_signal_failed`, and `cancellation_not_finalized`. Several ride + * on `success: true` responses, so validating them against the service enum + * makes `requestJson` reject cancellations that genuinely applied. + * + * The `already_*` outcomes are no longer route-local: the service now observes + * the run's terminal status itself, so both surfaces name a terminal no-op with + * the same member. */ export const internalCancelWorkflowExecutionReasonSchema = z.enum([ ...cancelWorkflowExecutionReasonSchema.options, 'queue_cancelled', - 'already_cancelled', 'active_resume_signal_failed', 'cancellation_not_finalized', ]) diff --git a/apps/sim/lib/api/server/routes/v2-json-route.test.ts b/apps/sim/lib/api/server/routes/v2-json-route.test.ts index 45dfd51000a..937cc8fc97b 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.test.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.test.ts @@ -495,6 +495,113 @@ describe('defineV2JsonRoute', () => { }) }) +/** + * A body that cannot be read as JSON has two very different causes, and the + * single `400 "Request body must be valid JSON"` describes only one of them: a + * caller who sent a form-encoded body is told to go hunting for a syntax error + * in a body that has none. + * + * These pin the split to the *classification* of an already-failing read. The + * final two are the regression guard that keeps it from becoming a media-type + * gate: a body that parses as JSON still succeeds no matter what the caller + * declared, which is what keeps `curl -d '{…}'` (form-urlencoded by default) + * and a headerless browser `fetch` (`text/plain`) working. + */ +describe('defineV2JsonRoute unreadable body classification', () => { + beforeEach(() => { + vi.clearAllMocks() + v2RouteMocks.authenticate.mockResolvedValue(auth) + v2RouteMocks.gate.mockResolvedValue(null) + v2RouteMocks.preauthRate.mockResolvedValue({ allowed: true, remaining: 599, resetAt }) + v2RouteMocks.operationRate.mockResolvedValue(allowedRate) + }) + + function bodyRequest(contentType: string | null, body: string): NextRequest { + return new NextRequest('http://localhost/api/v2/widgets', { + method: 'POST', + headers: { + 'x-api-key': 'secret', + ...(contentType === null ? {} : { 'content-type': contentType }), + }, + body, + }) + } + + it('answers 415 when an unreadable body declared a non-JSON media type', async () => { + const response = await createHandler()( + bodyRequest('application/x-www-form-urlencoded', 'value=ok') + ) + + expect(response.status).toBe(415) + await expect(response.json()).resolves.toEqual({ + error: { + code: 'UNSUPPORTED_MEDIA_TYPE', + message: 'Request body must be sent as application/json', + }, + }) + expect(response.headers.get('Cache-Control')).toBe('private, no-store') + }) + + it('keeps 400 for a truncated JSON body, whose media type was right', async () => { + const response = await createHandler()(bodyRequest('application/json', '{"value":')) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, + }) + }) + + it('keeps 400 when the media type is absent rather than wrong', async () => { + const response = await createHandler()(bodyRequest(null, '{"value":')) + + expect(response.status).toBe(400) + }) + + it('keeps 400 for text/plain, the default of a headerless browser fetch', async () => { + const response = await createHandler()(bodyRequest('text/plain;charset=UTF-8', '{"value":')) + + expect(response.status).toBe(400) + }) + + it('accepts a JSON body sent under a non-JSON media type, as it does today', async () => { + const response = await createHandler()( + bodyRequest('application/x-www-form-urlencoded', JSON.stringify({ value: 'ok' })) + ) + + expect(response.status).toBe(201) + await expect(response.json()).resolves.toEqual({ data: { value: 'ok' } }) + }) + + it('accepts a JSON body sent with no media type at all', async () => { + const response = await createHandler()(bodyRequest(null, JSON.stringify({ value: 'ok' }))) + + expect(response.status).toBe(201) + }) + + it('keeps 400 for a structured JSON suffix media type', async () => { + const response = await createHandler()(bodyRequest('application/merge-patch+json', '{"value":')) + + expect(response.status).toBe(400) + }) + + it('lets a route override the classification entirely', async () => { + const response = await createHandler({ + parseOptions: { + invalidJsonResponse: () => + NextResponse.json( + { error: { code: 'BAD_REQUEST', message: 'Import archive is not JSON' } }, + { status: 400 } + ), + }, + })(bodyRequest('application/x-www-form-urlencoded', 'value=ok')) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: { code: 'BAD_REQUEST', message: 'Import archive is not JSON' }, + }) + }) +}) + /** * A `HEAD` on a route whose `GET` is not safe must answer the question the `GET` * would answer, minus the effect — not merely the question admission can answer. diff --git a/apps/sim/lib/api/server/routes/v2-json-route.ts b/apps/sim/lib/api/server/routes/v2-json-route.ts index 1c284358dd1..f13eb0416b5 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.ts @@ -125,12 +125,75 @@ const v2PayloadTooLargeResponse = () => v2Error('PAYLOAD_TOO_LARGE', 'Request bo */ export const v2InvalidJsonResponse = () => v2Error('BAD_REQUEST', 'Request body must be valid JSON') +/** + * Whether the request declared a media type that is not a JSON body at all. + * + * Only consulted once a body has already **failed** to parse as JSON — see + * {@link v2InvalidBodyResponse} — so this decides how to describe a request + * that is failing either way, never whether one is accepted. + * + * An absent `Content-Type` is not a mismatch. A body sent without one is + * indistinguishable from a client that simply omits the header, and today's + * callers include ones that do; treating absence as a refusal is the likeliest + * way to turn a working client into a 415. + * + * `text/plain` is not a mismatch either, and that carve-out is load-bearing: + * `fetch(url, { method: 'POST', body: JSON.stringify(x) })` with no explicit + * headers sends `text/plain;charset=UTF-8`, so it is the default media type of + * a hand-written JSON body from a browser rather than a declaration that the + * body is not JSON. + * + * Anything else — `application/x-www-form-urlencoded`, `multipart/form-data`, + * `application/xml` — is a positive statement that the body is in some other + * format, which is exactly what 415 names. + */ +function declaresNonJsonBody(request: Request): boolean { + const header = request.headers.get('content-type') + if (!header) return false + const mediaType = header.split(';', 1)[0].trim().toLowerCase() + if (!mediaType || mediaType === 'text/plain') return false + const subtype = mediaType.slice(mediaType.indexOf('/') + 1) + return subtype !== 'json' && !subtype.endsWith('+json') +} + +/** + * The v2 answer to a body that could not be read as JSON: `415` when the caller + * declared a non-JSON media type, `400` otherwise. + * + * `400 "Request body must be valid JSON"` is the same answer for a truncated + * JSON body and for a form-encoded one, which leaves a caller who sent + * `application/x-www-form-urlencoded` hunting a syntax error in a body that has + * none. `UNSUPPORTED_MEDIA_TYPE` was already a declared `V2ErrorCode` with no + * path that reached it; this is that path. + * + * Deliberately a **re-classification of an existing failure**, not a new gate. + * It runs only after the JSON read has already failed, so no request that + * succeeds today can start failing: `curl -d '{"a":1}'` without `-H` sends + * form-urlencoded around a body that parses as JSON perfectly well, and that + * caller keeps working exactly as before. A pre-parse content-type gate would + * have broken them — and would also have to special-case the multipart bodies + * `defineV2BodyLifecycleRoute` legitimately accepts. Only the status and + * `error.code` of an already-4xx request change. + */ +export function v2InvalidBodyResponse(request: Request): NextResponse { + return declaresNonJsonBody(request) + ? v2Error('UNSUPPORTED_MEDIA_TYPE', 'Request body must be sent as application/json') + : v2InvalidJsonResponse() +} + /** * The parse failures every v2 route renders the same way. * * The builders spread this, and so must the handful of raw `withRouteHandler` * v2 routes that call `parseRequest` directly — they are exactly the routes a * builder default cannot reach. + * + * `invalidJsonResponse` is the request-unaware 400. `parseRequest` invokes it + * with no arguments, so the media-type-aware {@link v2InvalidBodyResponse} can + * only be installed by a caller that still holds the request — which + * {@link defineV2JsonRoute} does, overriding this entry. A raw route wanting the + * same 415 passes `invalidJsonResponse: () => v2InvalidBodyResponse(request)` + * after spreading this. */ export const V2_PARSE_DEFAULTS = { payloadTooLargeResponse: v2PayloadTooLargeResponse, @@ -360,6 +423,7 @@ export function defineV2JsonRoute< const parsed = await parseRequest(options.contract, request, context ?? {}, { ...V2_PARSE_DEFAULTS, + invalidJsonResponse: () => v2InvalidBodyResponse(request), ...options.parseOptions, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/lib/execution/cancel-workflow-execution.test.ts b/apps/sim/lib/execution/cancel-workflow-execution.test.ts index 53621352a03..a06f7aed897 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.test.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.test.ts @@ -18,6 +18,7 @@ const { mockReleaseExecutionSlot, mockUpdateSet, mockResolveWorkflowExecutionOwnership, + mockSelectExecutionLogRows, } = vi.hoisted(() => ({ mockAbortManualExecution: vi.fn(), mockBeginPausedCancellation: vi.fn(), @@ -33,10 +34,18 @@ const { mockReleaseExecutionSlot: vi.fn(), mockUpdateSet: vi.fn(), mockResolveWorkflowExecutionOwnership: vi.fn(), + mockSelectExecutionLogRows: vi.fn(), })) vi.mock('@sim/db', () => ({ db: { + select: () => ({ + from: () => ({ + where: () => ({ + limit: () => Promise.resolve(mockSelectExecutionLogRows()), + }), + }), + }), update: () => ({ set: (values: unknown) => { mockUpdateSet(values) @@ -113,6 +122,7 @@ describe('cancelWorkflowExecution', () => { belongsToWorkflow: true, workflowGroupWorkspaceId: null, }) + mockSelectExecutionLogRows.mockReturnValue([{ status: 'running' }]) mockBeginPausedCancellation.mockResolvedValue(false) mockGetPausedCancellationStatus.mockResolvedValue(null) mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: true, reason: 'recorded' }) @@ -128,6 +138,53 @@ describe('cancelWorkflowExecution', () => { }) }) + it('reports a durable write when an active run is cancelled', async () => { + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + }) + + /** + * A cancel against a run that already reached a terminal state changes + * nothing: the log claim's `status = 'running'` predicate matches no row and + * no terminal metadata moves. Reporting `recorded`/`durablyRecorded: true` + * there tells a caller a durable write happened when none did, so the outcome + * names the state that was actually observed instead. + */ + it.each([ + ['cancelled', 'already_cancelled'], + ['completed', 'already_completed'], + ['failed', 'already_failed'], + ])('reports a run already %s as a no-op rather than a durable write', async (status, reason) => { + mockSelectExecutionLogRows.mockReturnValue([{ status }]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: false, reason }) + }) + + it('still reports the failing step when a terminal run has paused work left over', async () => { + mockSelectExecutionLogRows.mockReturnValue([{ status: 'cancelled' }]) + mockBeginPausedCancellation.mockResolvedValue(true) + mockCompletePausedCancellation.mockResolvedValue(false) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: false, + durablyRecorded: true, + reason: 'paused_database_cancel_failed', + }) + }) + + it('reports an undifferentiated outcome when the run has no durable log row', async () => { + mockSelectExecutionLogRows.mockReturnValue([]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + }) + it('releases the plan concurrency reservation after a successful cancellation', async () => { const result = await cancelWorkflowExecution(INPUT) diff --git a/apps/sim/lib/execution/cancel-workflow-execution.ts b/apps/sim/lib/execution/cancel-workflow-execution.ts index 8d542d3bb27..4d2285d1945 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.ts @@ -45,17 +45,69 @@ async function cancelActiveWorkflowJob(executionId: string): Promise { * Cancellation outcome vocabulary produced by this service, and so the whole * vocabulary the public v2 endpoint can return. `recorded`/`redis_unavailable`/ * `redis_write_failed` come from the Redis record step; the two `paused_*` - * values from the paused-HITL path. The internal cancel route resolves further - * outcomes on top of these — see `internalCancelWorkflowExecutionReasonSchema` - * in `lib/api/contracts/workflows`. + * values from the paused-HITL path; the three `already_*` values report a run + * that was already terminal when the request arrived, where the cancel claim + * matched no row and nothing durable was written. The internal cancel route + * resolves further outcomes on top of these — see + * `internalCancelWorkflowExecutionReasonSchema` in `lib/api/contracts/workflows`. */ export type CancelWorkflowExecutionReason = | 'recorded' + | 'already_cancelled' + | 'already_completed' + | 'already_failed' | 'redis_unavailable' | 'redis_write_failed' | 'paused_event_publish_failed' | 'paused_database_cancel_failed' +/** Log statuses a cancel claim can never move, in the order they are reported. */ +const TERMINAL_NO_OP_REASONS = { + cancelled: 'already_cancelled', + completed: 'already_completed', + failed: 'already_failed', +} as const satisfies Record + +type TerminalExecutionStatus = keyof typeof TERMINAL_NO_OP_REASONS + +function toTerminalExecutionStatus(status: string | undefined): TerminalExecutionStatus | null { + return status !== undefined && status in TERMINAL_NO_OP_REASONS + ? (status as TerminalExecutionStatus) + : null +} + +/** + * Reads the durable status the run already carried, so the reported outcome can + * tell a real cancellation apart from a request against a run that had already + * finished. Purely observational — it gates no effect, and a read failure falls + * back to the undifferentiated report rather than blocking the cancel. + * + * Runs alongside `resolveWorkflowExecutionOwnership`, which reads the same row; + * folding the column into that helper would drop the extra round trip, but it + * lives outside this change's file boundary. + */ +async function readTerminalExecutionStatus( + executionId: string, + workflowId: string +): Promise { + try { + const [row] = await db + .select({ status: workflowExecutionLogs.status }) + .from(workflowExecutionLogs) + .where( + and( + eq(workflowExecutionLogs.executionId, executionId), + eq(workflowExecutionLogs.workflowId, workflowId) + ) + ) + .limit(1) + return toTerminalExecutionStatus(row?.status) + } catch (error) { + logger.warn('Failed to read execution status before cancelling', { executionId, error }) + return null + } +} + export interface CancelWorkflowExecutionResult { success: boolean executionId: string @@ -173,10 +225,10 @@ export async function cancelWorkflowExecution( ): Promise { const { executionId, workflowId, userId, workspaceId } = input - const { belongsToWorkflow, workflowGroupWorkspaceId } = await resolveWorkflowExecutionOwnership( - executionId, - workflowId - ) + const [{ belongsToWorkflow, workflowGroupWorkspaceId }, priorTerminalStatus] = await Promise.all([ + resolveWorkflowExecutionOwnership(executionId, workflowId), + readTerminalExecutionStatus(executionId, workflowId), + ]) if (!belongsToWorkflow) throw new WorkflowExecutionNotFoundError() let pausedCancellationStarted = false @@ -426,16 +478,34 @@ export async function cancelWorkflowExecution( ? 'recorded' : cancellation.reason + /** + * A run that was already terminal when the request arrived cannot be + * cancelled again: the claim's `status = 'running'` predicate matched no row + * and no terminal metadata moved, so `recorded`/`durablyRecorded: true` would + * claim a durable write that never happened. Every effect above still ran + * exactly as before — only the report changes. The request is still satisfied, + * because the run is not running, so `success` stays `true`. + * + * An already-`cancelled` run can still carry real paused-HITL reconciliation + * work, and when that step genuinely fails its reason must survive; only an + * otherwise-clean `recorded` is reinterpreted there. + */ + const terminalNoOpReason = + priorTerminalStatus !== null && + (priorTerminalStatus !== 'cancelled' || (reason === 'recorded' && !pausedCancelled)) + ? TERMINAL_NO_OP_REASONS[priorTerminalStatus] + : null + return { - success, + success: terminalNoOpReason ? true : success, executionId, redisAvailable: isPausedCancellationPath || pausedCancelled ? pausedCancellationPublished : cancellation.reason !== 'redis_unavailable', - durablyRecorded, + durablyRecorded: terminalNoOpReason ? false : durablyRecorded, locallyAborted, pausedCancelled, - reason, + reason: terminalNoOpReason ?? reason, } } diff --git a/apps/sim/lib/logs/execution/cancellation.test.ts b/apps/sim/lib/logs/execution/cancellation.test.ts index 4c2485cb08e..d37c5c28cf9 100644 --- a/apps/sim/lib/logs/execution/cancellation.test.ts +++ b/apps/sim/lib/logs/execution/cancellation.test.ts @@ -11,7 +11,9 @@ vi.unmock('@sim/db/schema') process.env.DATABASE_URL ??= 'postgresql://user:pass@localhost:5432/test' const { PgDialect } = await import('drizzle-orm/pg-core') -const { cancelledExecutionLogFields } = await import('@/lib/logs/execution/cancellation') +const { cancelledExecutionLogFields, terminalExecutionLogFields } = await import( + '@/lib/logs/execution/cancellation' +) describe('cancelledExecutionLogFields', () => { /** @@ -47,3 +49,40 @@ describe('cancelledExecutionLogFields', () => { expect(params).toContain(endedAt.toISOString()) }) }) + +describe('terminalExecutionLogFields', () => { + /** + * The force-fail boundaries — `LoggingSession.markExecutionAsFailed` and + * `PauseResumeManager.markResumeFailed` — leave the same row behind as a + * cancellation, only under a different status. Only the status may differ. + */ + it('writes the cancellation field set under the failed status', () => { + const endedAt = new Date('2026-08-13T12:00:05.000Z') + + const failed = terminalExecutionLogFields('failed', endedAt) + + expect(Object.keys(failed).sort()).toEqual( + Object.keys(cancelledExecutionLogFields(endedAt)).sort() + ) + expect(failed.status).toBe('failed') + expect(failed.endedAt).toBe(endedAt) + expect(failed.executionDeadlineAt).toBeNull() + + const { params } = new PgDialect().sqlToQuery(failed.totalDurationMs) + expect(params).toContain(endedAt.toISOString()) + }) + + /** The cancellation call sites must keep emitting exactly what they did. */ + it('is what the cancellation binding emits', () => { + const endedAt = new Date('2026-08-13T12:00:05.000Z') + const dialect = new PgDialect() + + const bound = cancelledExecutionLogFields(endedAt) + const direct = terminalExecutionLogFields('cancelled', endedAt) + + expect({ ...bound, totalDurationMs: dialect.sqlToQuery(bound.totalDurationMs) }).toEqual({ + ...direct, + totalDurationMs: dialect.sqlToQuery(direct.totalDurationMs), + }) + }) +}) diff --git a/apps/sim/lib/logs/execution/cancellation.ts b/apps/sim/lib/logs/execution/cancellation.ts index 09e68df1d12..6191fb18747 100644 --- a/apps/sim/lib/logs/execution/cancellation.ts +++ b/apps/sim/lib/logs/execution/cancellation.ts @@ -1,23 +1,46 @@ import { elapsedDurationMsSql } from '@/lib/logs/execution/duration' /** - * The fields every terminal cancellation sets on a `workflow_execution_logs` - * row, ready to spread into `.set()`. + * The statuses a terminal write outside `completeWorkflowExecution` can land a + * `workflow_execution_logs` row on: cancellation, and the force-fail boundaries + * that bypass the completion path. + */ +type TerminalExecutionLogStatus = 'cancelled' | 'failed' + +/** + * The fields every terminal write outside `completeWorkflowExecution` sets on a + * `workflow_execution_logs` row, ready to spread into `.set()`. + * + * The cancellation paths — direct, workflow-group with and without a sidecar, + * paused, and the async cancel route — and the two force-fail boundaries — + * `LoggingSession.markExecutionAsFailed` and `PauseResumeManager.markResumeFailed` + * — differ in their database handle, their claim predicate, whether they read + * the row back, and what they do when the claim is lost, so they remain separate + * statements. What they must not differ in is the row they leave behind, and + * hand-assembling this payload at each one had already dropped + * `executionDeadlineAt` at a single cancellation site and both the end timestamp + * and the duration at both force-fail sites, leaving a terminal run still + * carrying the deadline of an attempt that had stopped running and invisible to + * every `minDurationMs`/`maxDurationMs` query on `GET /api/v2/logs`. * - * The five cancellation paths — direct, workflow-group with and without a - * sidecar, paused, and the async cancel route — differ in their database - * handle, their claim predicate, whether they read the row back, and what they - * do when the claim is lost, so they remain separate statements. What they must - * not differ in is the row they leave behind, and hand-assembling this payload - * at each one had already dropped `executionDeadlineAt` at a single site, - * leaving a cancelled run still carrying the deadline of an attempt that had - * stopped running. + * A duration a paused run already recorded still wins — that guard lives in + * `elapsedDurationMsSql`, keyed on the row's own status, so a force-fail landing + * on a still-`pending` paused row keeps the checkpoint duration rather than + * redefining it to include the time the run sat waiting. */ -export function cancelledExecutionLogFields(endedAt: Date) { +export function terminalExecutionLogFields( + status: TStatus, + endedAt: Date +) { return { - status: 'cancelled' as const, + status, endedAt, totalDurationMs: elapsedDurationMsSql(endedAt), executionDeadlineAt: null, } } + +/** {@link terminalExecutionLogFields} bound to the cancellation status. */ +export function cancelledExecutionLogFields(endedAt: Date) { + return terminalExecutionLogFields('cancelled', endedAt) +} diff --git a/apps/sim/lib/logs/execution/logging-session.test.ts b/apps/sim/lib/logs/execution/logging-session.test.ts index 172c08c419c..47c0c048ad4 100644 --- a/apps/sim/lib/logs/execution/logging-session.test.ts +++ b/apps/sim/lib/logs/execution/logging-session.test.ts @@ -4,7 +4,13 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' const dbMocks = vi.hoisted(() => ({ eq: vi.fn(), and: vi.fn((...args: unknown[]) => ({ type: 'and', args })), - sql: vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values })), + sql: Object.assign( + vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values })), + { + /** `elapsedDurationMsSql` binds `ended_at` through the column's own mapper. */ + param: vi.fn((value: unknown, encoder?: unknown) => ({ value, encoder })), + } + ), })) const { @@ -1683,6 +1689,39 @@ describe('LoggingSession.markExecutionAsFailed workflowId scoping', () => { expect(statusGuards).toHaveLength(1) }) + it('terminalizes the row it force-fails: end timestamp, derived duration, deadline cleared', async () => { + await LoggingSession.markExecutionAsFailed('exec-terminal', 'boom', undefined, 'wf-1') + + const payload = dbChainMockFns.set.mock.calls[0]?.[0] as { + level: string + status: string + endedAt: Date + totalDurationMs: unknown + executionDeadlineAt: Date | null + executionData: unknown + } + expect(payload.level).toBe('error') + expect(payload.status).toBe('failed') + expect(payload.endedAt).toBeInstanceOf(Date) + expect(payload.executionDeadlineAt).toBeNull() + expect(payload.totalDurationMs).toBeDefined() + expect(dbMocks.sql.param).toHaveBeenCalledWith(payload.endedAt, expect.anything()) + }) + + /** + * A resumed run whose pause state fails to persist can still be `pending`, and + * the duration it banked at the checkpoint must survive the force-fail rather + * than be redefined to include the time it sat waiting. + */ + it('leaves a paused run its checkpoint duration', async () => { + await LoggingSession.markExecutionAsFailed('exec-paused', 'boom', undefined, 'wf-1') + + const durationGuards = dbMocks.sql.mock.calls + .map(([strings]) => String(Array.from(strings))) + .filter((query) => query.includes("= 'pending'")) + expect(durationGuards).toHaveLength(1) + }) + it('clears Redis markers when marking failed (terminal boundary outside completeWorkflowExecution)', async () => { await LoggingSession.markExecutionAsFailed('exec-3', 'boom', undefined, 'wf-3') expect(clearProgressMarkersMock).toHaveBeenCalledWith('exec-3') diff --git a/apps/sim/lib/logs/execution/logging-session.ts b/apps/sim/lib/logs/execution/logging-session.ts index 9102684ee6f..8536910302e 100644 --- a/apps/sim/lib/logs/execution/logging-session.ts +++ b/apps/sim/lib/logs/execution/logging-session.ts @@ -8,6 +8,7 @@ import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attr import { isRetryableInfrastructureError } from '@/lib/core/errors/retryable-infrastructure' import { RESERVATION_TTL_BUFFER_MS } from '@/lib/core/execution-limits' import type { LargeValueStoreContext } from '@/lib/execution/payloads/store' +import { terminalExecutionLogFields } from '@/lib/logs/execution/cancellation' import type { SecretSafeBlockLog } from '@/lib/logs/execution/display-types' import { executionLogger } from '@/lib/logs/execution/logger' import { @@ -1545,7 +1546,11 @@ export class LoggingSession { await execDb .update(workflowExecutionLogs) - .set({ level: 'error', status: 'failed', executionDeadlineAt: null, executionData }) + .set({ + level: 'error', + ...terminalExecutionLogFields('failed', new Date()), + executionData, + }) .where( and( eq(workflowExecutionLogs.executionId, executionId), diff --git a/apps/sim/lib/table/__tests__/sql.test.ts b/apps/sim/lib/table/__tests__/sql.test.ts index 56a2118438c..1737fddc8ea 100644 --- a/apps/sim/lib/table/__tests__/sql.test.ts +++ b/apps/sim/lib/table/__tests__/sql.test.ts @@ -1038,3 +1038,197 @@ describe('legacy compiler rejects a v2 predicate (version-mismatch fail-fast)', ).not.toThrow() }) }) + +/** + * Equality/membership compiles to exact JSONB containment, which is untyped: + * `{"score":8} @> {"score":"8"}` is simply FALSE. Before the operand was read + * through the column type, a wrongly-typed `eq`/`ne`/`in`/`nin` returned an + * empty 200 that a caller could not tell apart from a genuinely empty table — + * while the range operators on the same column answered with a descriptive 400. + */ +describe('containment operators — operand is read through the column type', () => { + const NUM: ColumnDefinition[] = [{ id: 'score', name: 'score', type: 'number' }] + const BOOL: ColumnDefinition[] = [{ id: 'flag', name: 'flag', type: 'boolean' }] + const STR: ColumnDefinition[] = [{ id: 'title', name: 'title', type: 'string' }] + const DATE: ColumnDefinition[] = [{ id: 'due', name: 'due', type: 'date' }] + const MONEY: ColumnDefinition[] = [{ id: 'price', name: 'price', type: 'currency' }] + + describe('coerces an unambiguous operand the way a write would', () => { + it.each([ + ['eq', { all: [{ field: 'score', op: 'eq', value: '8' }] }, '"score":8'], + ['ne', { all: [{ field: 'score', op: 'ne', value: '8' }] }, '"score":8'], + ['in', { all: [{ field: 'score', op: 'in', value: ['8'] }] }, '"score":8'], + ['nin', { all: [{ field: 'score', op: 'nin', value: ['8'] }] }, '"score":8'], + ] as Array<[string, TablePredicate, string]>)('%s on a number column', (_op, p, expected) => { + const out = render(buildPredicateClause(p, TABLE, NUM)) + expect(out).toContain(expected) + expect(out).not.toContain('"score":"8"') + }) + + it('reads "false" as the boolean false', () => { + const p: TablePredicate = { all: [{ field: 'flag', op: 'eq', value: 'false' }] } + const out = render(buildPredicateClause(p, TABLE, BOOL)) + expect(out).toContain('"flag":false') + expect(out).not.toContain('"flag":"false"') + }) + + it('reads a number as text on a string column', () => { + const p: TablePredicate = { all: [{ field: 'title', op: 'eq', value: 8 }] } + const out = render(buildPredicateClause(p, TABLE, STR)) + expect(out).toContain('"title":"8"') + }) + + it('normalizes a date operand the same way the stored cell was normalized', () => { + const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: ' 2024-01-31 ' }] } + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":"2024-01-31"') + }) + + it('reads a formatted amount on a currency column', () => { + const p: TablePredicate = { all: [{ field: 'price', op: 'eq', value: '$1,234.56' }] } + const out = render(buildPredicateClause(p, TABLE, MONEY)) + expect(out).toContain('"price":1234.56') + }) + + it('applies to the legacy $-grammar too', () => { + const out = render(buildFilterClause({ score: { $eq: '8' } }, TABLE, NUM)) + expect(out).toContain('"score":8') + expect(out).toContain('"score":8') + }) + + it('applies to the legacy equality shorthand', () => { + expect(render(buildFilterClause({ score: '8' }, TABLE, NUM))).toContain('"score":8') + }) + }) + + describe('rejects an operand the column could never hold', () => { + it.each(['eq', 'ne'] as const)('%s with an unparseable number', (op) => { + const p = { all: [{ field: 'score', op, value: 'eight' }] } as TablePredicate + expect(() => buildPredicateClause(p, TABLE, NUM)).toThrow( + `Operator "${op}" on column "score" (number) requires a number value, got string "eight"` + ) + }) + + it.each(['in', 'nin'] as const)('%s naming a bad element', (op) => { + const p = { all: [{ field: 'score', op, value: [8, 'eight'] }] } as TablePredicate + expect(() => buildPredicateClause(p, TABLE, NUM)).toThrow(/requires a number value/) + }) + + it('rejects a non-boolean on a boolean column', () => { + const p: TablePredicate = { all: [{ field: 'flag', op: 'eq', value: 'yes' }] } + expect(() => buildPredicateClause(p, TABLE, BOOL)).toThrow( + 'Operator "eq" on column "flag" (boolean) requires a boolean value, got string "yes"' + ) + }) + + it('rejects an unparseable date', () => { + const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: 'not-a-date' }] } + expect(() => buildPredicateClause(p, TABLE, DATE)).toThrow(/requires a date value/) + }) + + it('rejects an object on a string column', () => { + const p = { + all: [{ field: 'title', op: 'eq', value: { a: 1 } }], + } as unknown as TablePredicate + expect(() => buildPredicateClause(p, TABLE, STR)).toThrow(/requires a string value/) + }) + + it('rejects it on the legacy $-grammar too', () => { + expect(() => buildFilterClause({ score: { $eq: 'eight' } }, TABLE, NUM)).toThrow( + /requires a number value/ + ) + }) + }) + + describe('leaves the operands that are not type assertions alone', () => { + it('keeps null — a real containment query for a JSON-null cell', () => { + const p: TablePredicate = { all: [{ field: 'score', op: 'eq', value: null }] } + expect(render(buildPredicateClause(p, TABLE, NUM))).toContain('"score":null') + }) + + it('keeps the empty string — the cleared-cell sentinel', () => { + const p: TablePredicate = { all: [{ field: 'score', op: 'eq', value: '' }] } + expect(render(buildPredicateClause(p, TABLE, NUM))).toContain('"score":""') + }) + + it('leaves a field with no schema entry untouched', () => { + const p: TablePredicate = { all: [{ field: 'adhoc', op: 'eq', value: '8' }] } + expect(render(buildPredicateClause(p, TABLE, NO_COLUMNS))).toContain('"adhoc":"8"') + }) + + it('leaves a select column to its own name→id resolution', () => { + const statusCol: ColumnDefinition = { + id: 'col_status', + name: 'status', + type: 'select', + options: [{ id: 'opt_open', name: 'Open' }], + } + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'opt_open' }] } + expect(render(buildPredicateClause(p, TABLE, [statusCol]))).toContain('"opt_open"') + }) + }) +}) + +/** + * Filters reach the SQL builders storage-keyed — the boundaries translate + * column name → column id first — so a message interpolating the raw field + * reported a `col_…` id the caller never sent and cannot look up. + */ +describe('error messages name the caller-facing column, not the storage id', () => { + const multi: ColumnDefinition = { + id: 'col_934cea93275d46448b0d6c001554e146', + name: 'untitled_2', + type: 'select', + multiple: true, + options: [{ id: 'opt_a', name: 'Alpha' }], + } + const num: ColumnDefinition = { id: 'col_abc123', name: 'overall_score', type: 'number' } + const bool: ColumnDefinition = { id: 'col_def456', name: 'untitled', type: 'boolean' } + const str: ColumnDefinition = { id: 'col_ghi789', name: 'headline', type: 'string' } + + function expectNamed(fn: () => unknown, name: string, id: string) { + expect(fn).toThrow(new RegExp(`"${name}"`)) + expect(fn).not.toThrow(new RegExp(id)) + } + + it('names the column on an unsupported select operator (v2 grammar)', () => { + const p: TablePredicate = { all: [{ field: multi.id as string, op: 'eq', value: 'c' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [multi]), 'untitled_2', 'col_934cea') + }) + + it('names the column on an unsupported select operator (legacy grammar)', () => { + expectNamed( + () => buildFilterClause({ [multi.id as string]: { $eq: 'c' } }, TABLE, [multi]), + 'untitled_2', + 'col_934cea' + ) + }) + + it('names the column on a range-operator type mismatch', () => { + const p: TablePredicate = { all: [{ field: 'col_abc123', op: 'gt', value: '7' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [num]), 'overall_score', 'col_abc123') + }) + + it('names the column on an unorderable range operator', () => { + const p: TablePredicate = { all: [{ field: 'col_def456', op: 'gt', value: 1 }] } + expectNamed(() => buildPredicateClause(p, TABLE, [bool]), 'untitled', 'col_def456') + }) + + it('names the column on an empty pattern operand', () => { + const p: TablePredicate = { all: [{ field: 'col_ghi789', op: 'contains', value: '' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [str]), 'headline', 'col_ghi789') + }) + + it('names the column on a bad $empty flag', () => { + expectNamed( + () => buildFilterClause({ col_ghi789: { $empty: 1 } } as unknown as Filter, TABLE, [str]), + 'headline', + 'col_ghi789' + ) + }) + + it('names the column on a containment type mismatch', () => { + const p: TablePredicate = { all: [{ field: 'col_abc123', op: 'eq', value: 'seven' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [num]), 'overall_score', 'col_abc123') + }) +}) diff --git a/apps/sim/lib/table/sql.ts b/apps/sim/lib/table/sql.ts index 6a90b5280db..11218532130 100644 --- a/apps/sim/lib/table/sql.ts +++ b/apps/sim/lib/table/sql.ts @@ -12,6 +12,7 @@ import { sql } from 'drizzle-orm' import { getColumnId } from '@/lib/table/column-keys' import { columnTypeById, + columnTypeOf, filterOperatorsFor, MULTI_SELECT_OPERATORS, SINGLE_SELECT_OPERATORS, @@ -350,6 +351,19 @@ function validateOperator(operator: string): void { } } +/** + * The caller-facing name for a field in an error message. + * + * Filters reach the SQL builders **storage-keyed** — the boundaries translate + * column name → column id first — so interpolating the raw `field` reports a + * `col_…` id the caller never sent and cannot look up. The definition already in + * hand carries the display name; fall back to `field` for a system column + * (`createdAt`), an unknown key, or a legacy column whose id IS its name. + */ +function columnLabel(field: string, column: ColumnDefinition | undefined): string { + return column?.name ?? field +} + /** * Validates that a range-operator value matches its column's expected JS type * before it reaches Postgres. Surfaces an actionable, column-named error at the @@ -357,31 +371,83 @@ function validateOperator(operator: string): void { * from the database. */ function validateComparisonValue( - field: string, + label: string, columnType: ColumnType | undefined, cast: 'numeric' | 'timestamptz', value: number | string ): void { if (cast === 'numeric' && typeof value !== 'number') { - const label = columnType ?? 'number' + const typeLabel = columnType ?? 'number' throw new TableQueryValidationError( - `Range operator on column "${field}" (${label}) requires a number, got ${typeof value}` + `Range operator on column "${label}" (${typeLabel}) requires a number, got ${typeof value}` ) } if (cast === 'timestamptz') { if (typeof value !== 'string') { throw new TableQueryValidationError( - `Range operator on column "${field}" (date) requires a date string, got ${typeof value}` + `Range operator on column "${label}" (date) requires a date string, got ${typeof value}` ) } if (normalizeDateCellValue(value) === null) { throw new TableQueryValidationError( - `Range operator on column "${field}" (date) requires a parseable date string, got "${truncate(value, 64)}"` + `Range operator on column "${label}" (date) requires a parseable date string, got "${truncate(value, 64)}"` ) } } } +/** + * Equality/membership operators. Their operand is compared by JSONB + * containment, which is exact and untyped: `{"score": 8} @> {"score": "8"}` is + * simply false, so a wrongly-typed operand never matches and the caller cannot + * tell that from a genuinely empty table. + */ +const CONTAINMENT_OPS = new Set(['eq', 'ne', 'in', 'nin']) + +/** Renders an operand for an error message, bounded. */ +function describeOperand(value: JsonValue): string { + if (typeof value === 'string') return `string "${truncate(value, 64)}"` + return `${typeof value} ${truncate(JSON.stringify(value) ?? String(value), 64)}` +} + +/** + * Reads an equality/membership operand the way the **write path** reads a cell, + * so `eq` compares like against like. + * + * The column type's own `coerce` is the single definition of "what this column + * can hold": a write of `"8"` to a number column stores `8`, so a filter for + * `"8"` must look for `8` or it reports zero rows for a row that exists. When + * `coerce` refuses, the operand is one the column could never hold — the range + * operators already answer that with a descriptive 400 rather than an empty + * result set, and this is the same answer for the containment operators. + * + * `null` and `''` are passed through untouched. Neither is a typed operand: + * `null` is a real containment query for a JSON-null cell, and `''` is the + * cleared-cell sentinel the grid writes. Coercing or rejecting either would + * change what an existing caller's filter means rather than fix it. + * + * `select` is excluded: its operands are option **names**, already resolved to + * stored ids upstream by `resolvePredicateSelectValues` / + * `resolveFilterSelectValues`, and its `coerce` returns an array for a + * multi-select — the wrong shape for a membership clause. + */ +function coerceContainmentOperand( + label: string, + column: ColumnDefinition, + op: FilterOp, + value: JsonValue +): JsonValue { + if (value === null || value === '') return value + const result = columnTypeOf(column).coerce(value, column) + if (!result.ok) { + throw new TableQueryValidationError( + `Operator "${op}" on column "${label}" (${column.type}) requires a ${column.type} value, got ${describeOperand(value)}.`, + 'INVALID_FILTER' + ) + } + return result.value +} + /** * Guards a bound that is about to be bound into a `::timestamptz` cast on a * system timestamp column (`createdAt`/`updatedAt`). @@ -430,6 +496,7 @@ function buildFieldCondition( const columnType = column?.type const isSelect = columnType === 'select' const isMultiSelect = isSelect && column?.multiple === true + const label = columnLabel(field, column) // Types whose stored value is opaque (a select's option ids) restrict which // operators mean anything; `null` means the type accepts them all. const allowedOperators = column ? filterOperatorsFor(column) : null @@ -442,13 +509,13 @@ function buildFieldCondition( validateOperator(op) if (allowedOperators && !allowedOperators.has(op)) { throw new TableQueryValidationError( - `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : columnType} column "${field}". Allowed: ${Array.from(allowedOperators).join(', ')}` + `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : columnType} column "${label}". Allowed: ${Array.from(allowedOperators).join(', ')}` ) } if (op === '$empty') { // `$empty: true/false` maps onto the valueless v2 ops. - const filterOp: FilterOp = coerceEmptyFlag(field, value) ? 'isEmpty' : 'isNotEmpty' + const filterOp: FilterOp = coerceEmptyFlag(label, value) ? 'isEmpty' : 'isNotEmpty' const clause = fieldPredicate(tableName, field, filterOp, undefined, column) if (clause) conditions.push(clause) continue @@ -511,6 +578,10 @@ export function fieldPredicate( } const columnType = column?.type + // Messages must name what the CALLER sent. `field` is the storage key by the + // time it reaches here (the boundaries translate name → id before building + // SQL), so a raw `field` reports a `col_…` the caller never supplied. + const label = columnLabel(field, column) const isSelect = columnType === 'select' // A multi-select cell holds an ARRAY of option ids, so equality against a // scalar can never be true; the question is membership. Gating and clause @@ -522,7 +593,7 @@ export function fieldPredicate( const allowed = isMultiSelect ? MULTI_SELECT_OPS : SINGLE_SELECT_OPS if (!allowed.has(op)) { throw new TableQueryValidationError( - `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : 'select'} column "${field}". Allowed: ${Array.from(allowed).join(', ')}` + `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : 'select'} column "${label}". Allowed: ${Array.from(allowed).join(', ')}` ) } } @@ -542,45 +613,60 @@ export function fieldPredicate( } } + // Equality/membership compiles to exact JSONB containment, so a wrongly-typed + // operand is not a narrower match — it is no match at all, reported as an + // empty 200. Read the operand through the column type first, exactly as a + // write would; `coerceContainmentOperand` throws when the column could never + // hold it. Skipped for a field with no schema entry (ad-hoc legacy keys), + // which has no declared type to read it with. + const containmentValue: JsonValue | undefined = + column && !isSelect && CONTAINMENT_OPS.has(op) + ? Array.isArray(value) + ? value.map((v) => coerceContainmentOperand(label, column, op, v as JsonValue)) + : coerceContainmentOperand(label, column, op, value as JsonValue) + : value + switch (op) { case 'eq': - return buildContainmentClause(tableName, field, value as JsonValue) + return buildContainmentClause(tableName, field, containmentValue as JsonValue) case 'ne': - return sql`NOT (${buildContainmentClause(tableName, field, value as JsonValue)})` + return sql`NOT (${buildContainmentClause(tableName, field, containmentValue as JsonValue)})` case 'gt': - return buildComparisonClause(tableName, field, '>', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '>', value as number | string) case 'gte': - return buildComparisonClause(tableName, field, '>=', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '>=', value as number | string) case 'lt': - return buildComparisonClause(tableName, field, '<', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '<', value as number | string) case 'lte': - return buildComparisonClause(tableName, field, '<=', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '<=', value as number | string) case 'in': { - if (!Array.isArray(value) || value.length === 0) return undefined - if (value.length === 1) return buildContainmentClause(tableName, field, value[0]) - const inConditions = value.map((v) => buildContainmentClause(tableName, field, v)) + const values = containmentValue + if (!Array.isArray(values) || values.length === 0) return undefined + if (values.length === 1) return buildContainmentClause(tableName, field, values[0]) + const inConditions = values.map((v) => buildContainmentClause(tableName, field, v)) return sql`(${sql.join(inConditions, sql.raw(' OR '))})` } case 'nin': { - if (!Array.isArray(value) || value.length === 0) return undefined - const ninConditions = value.map( + const values = containmentValue + if (!Array.isArray(values) || values.length === 0) return undefined + const ninConditions = values.map( (v) => sql`NOT (${buildContainmentClause(tableName, field, v)})` ) return sql`(${sql.join(ninConditions, sql.raw(' AND '))})` } case 'contains': - return buildLikeClause(tableName, field, value as string, 'contains') + return buildLikeClause(tableName, field, label, value as string, 'contains') case 'ncontains': - return buildLikeClause(tableName, field, value as string, 'contains', { negate: true }) + return buildLikeClause(tableName, field, label, value as string, 'contains', { negate: true }) case 'startsWith': - return buildLikeClause(tableName, field, value as string, 'startsWith') + return buildLikeClause(tableName, field, label, value as string, 'startsWith') case 'endsWith': - return buildLikeClause(tableName, field, value as string, 'endsWith') + return buildLikeClause(tableName, field, label, value as string, 'endsWith') case 'like': return buildPatternClause(tableName, field, value as string, { caseInsensitive: false }) @@ -813,15 +899,17 @@ function buildArrayMembershipClause(tableName: string, field: string, value: Jso function buildComparisonClause( tableName: string, field: string, + column: ColumnDefinition | undefined, operator: '>' | '>=' | '<' | '<=', - value: number | string, - columnType: ColumnType | undefined + value: number | string ): SQL { const escapedField = field.replace(/'/g, "''") + const label = columnLabel(field, column) + const columnType = column?.type if (columnType === 'boolean' || columnType === 'json') { throw new TableQueryValidationError( - `Range operator on column "${field}" (${columnType}) is not supported — ${columnType} values have no ordering.` + `Range operator on column "${label}" (${columnType}) is not supported — ${columnType} values have no ordering.` ) } @@ -831,7 +919,7 @@ function buildComparisonClause( } const cast = jsonbCastForType(columnType) ?? 'numeric' - validateComparisonValue(field, columnType, cast, value) + validateComparisonValue(label, columnType, cast, value) const cell = sql.raw(`(${tableName}.data->>'${escapedField}')::${cast}`) return cast === 'timestamptz' ? sql`${cell} ${sql.raw(operator)} ${value}::timestamptz` @@ -884,6 +972,7 @@ function buildPatternClause( function buildLikeClause( tableName: string, field: string, + label: string, value: string, position: 'contains' | 'startsWith' | 'endsWith', options?: { negate?: boolean } @@ -898,7 +987,7 @@ function buildLikeClause( if (text.length === 0) { const opName = position === 'contains' && options?.negate ? 'ncontains' : position throw new TableQueryValidationError( - `$${opName} on column "${field}" requires a non-empty value` + `$${opName} on column "${label}" requires a non-empty value` ) } const escaped = escapeLikePattern(text) @@ -920,12 +1009,12 @@ function buildLikeClause( * else throws rather than silently inverting the check — a 400 with a clear * message beats returning the opposite row set. */ -function coerceEmptyFlag(field: string, value: unknown): boolean { +function coerceEmptyFlag(label: string, value: unknown): boolean { if (typeof value === 'boolean') return value if (value === 'true') return true if (value === 'false') return false throw new TableQueryValidationError( - `$empty on column "${field}" requires a boolean, got ${typeof value}` + `$empty on column "${label}" requires a boolean, got ${typeof value}` ) } diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts index 5754349fc63..0d842abe1a7 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts @@ -1533,6 +1533,59 @@ describe('PauseResumeManager blocked resume readmission', () => { }) }) +describe('PauseResumeManager terminal resume failure', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + async function markResumeFailed(): Promise { + const managerInternals = PauseResumeManager as unknown as PauseResumeManagerInternals + await managerInternals.markResumeFailed({ + resumeEntryId: 'resume-entry-1', + pausedExecutionId: 'paused-exec-1', + parentExecutionId: 'execution-1', + contextId: 'context-1', + failureReason: 'Resume execution failed', + }) + } + + it('terminalizes the parent log: end timestamp, derived duration, deadline cleared', async () => { + queueTableRows(workflowExecutionLogs, [{ status: 'running' }]) + queueTableRows(pausedExecutions, [{ status: 'paused' }]) + + await markResumeFailed() + + const logUpdate = dbChainMockFns.set.mock.calls.at(-1)?.[0] as { + status: string + endedAt: Date + totalDurationMs: unknown + executionDeadlineAt: Date | null + } + expect(logUpdate.status).toBe('failed') + expect(logUpdate.endedAt).toBeInstanceOf(Date) + expect(logUpdate.executionDeadlineAt).toBeNull() + expect(JSON.stringify(logUpdate.totalDurationMs)).toContain(logUpdate.endedAt.toISOString()) + }) + + /** + * A resume that fails before the row flips back to `running` leaves it + * `pending` with the duration it banked at the checkpoint. Elapsed wall clock + * would redefine that to include the time the run sat waiting. + */ + it('leaves a still-paused run its checkpoint duration', async () => { + queueTableRows(workflowExecutionLogs, [{ status: 'pending' }]) + queueTableRows(pausedExecutions, [{ status: 'paused' }]) + + await markResumeFailed() + + const logUpdate = dbChainMockFns.set.mock.calls.at(-1)?.[0] as { + totalDurationMs: { toSQL: () => { sql: string } } + } + expect(logUpdate.totalDurationMs.toSQL().sql).toContain("= 'pending'") + }) +}) + describe('PauseResumeManager completed resume transitions', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts index 48029178fdd..278be0cf9f3 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts @@ -33,7 +33,10 @@ import { } from '@/lib/execution/payloads/large-value-metadata' import { compactBlockLogs, compactExecutionPayload } from '@/lib/execution/payloads/serializer' import { preprocessExecution } from '@/lib/execution/preprocessing' -import { cancelledExecutionLogFields } from '@/lib/logs/execution/cancellation' +import { + cancelledExecutionLogFields, + terminalExecutionLogFields, +} from '@/lib/logs/execution/cancellation' import { LoggingSession } from '@/lib/logs/execution/logging-session' import { cleanupExecutionBase64Cache } from '@/lib/uploads/utils/user-file-base64.server' import { executeWorkflowCore } from '@/lib/workflows/executor/execution-core' @@ -2173,7 +2176,7 @@ export class PauseResumeManager { await tx .update(workflowExecutionLogs) - .set({ status: 'failed' }) + .set(terminalExecutionLogFields('failed', now)) .where( and( eq(workflowExecutionLogs.executionId, args.parentExecutionId), diff --git a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts index 4d29c4211c5..3214cdf11f4 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts @@ -24,6 +24,7 @@ vi.mock('@/lib/public-shares/share-manager', () => ({ getShareForResource: mocks.getShareForResource, })) +import { NoWorkspaceAccessError } from '@/lib/core/application' import { readWorkspaceFileMetadata } from '@/lib/workspace-files/application/read-workspace-file-metadata' const canonical = { @@ -91,6 +92,61 @@ describe('readWorkspaceFileMetadata', () => { expect(mocks.getShareForResource).toHaveBeenCalledWith('file', 'file-1') }) + it('resolves a soft-deleted file when the caller opts into the archived lifecycle set', async () => { + const archived = { ...file, deletedAt: new Date('2026-01-03T00:00:00Z') } + mocks.getWorkspaceFile.mockResolvedValueOnce(archived) + + await expect( + readWorkspaceFileMetadata.execute({ + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-1', + includeDeleted: true, + }, + }) + ).resolves.toEqual({ file: archived, share }) + + expect(mocks.loadContext).toHaveBeenCalledWith('file-1', { includeDeleted: true }) + expect(mocks.getWorkspaceFile).toHaveBeenCalledWith('workspace-1', 'file-1', { + includeDeleted: true, + throwOnError: true, + }) + }) + + it('authorizes an archived read exactly like an active one', async () => { + mocks.resolvePermission.mockResolvedValueOnce(null) + + await expect( + readWorkspaceFileMetadata.execute({ + principal: { kind: 'session', userId: 'outsider', sessionId: 'session-2' }, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-1', + includeDeleted: true, + }, + }) + ).rejects.toBeInstanceOf(NoWorkspaceAccessError) + + expect(mocks.getWorkspaceFile).not.toHaveBeenCalled() + expect(mocks.getShareForResource).not.toHaveBeenCalled() + }) + + it('still refuses an archived read that asserts the wrong workspace', async () => { + await expect( + readWorkspaceFileMetadata.execute({ + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-2', + includeDeleted: true, + }, + }) + ).rejects.toMatchObject({ code: 'not_found' }) + + expect(mocks.getWorkspaceFile).not.toHaveBeenCalled() + }) + it('fails fast if the authorized file disappears before projection', async () => { mocks.getWorkspaceFile.mockResolvedValueOnce(null) diff --git a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts index 5be9e1c9c99..621d20f1324 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts @@ -14,6 +14,12 @@ import { resolveActiveWorkspaceFileContext } from '@/lib/workspace-files/applica export interface ReadWorkspaceFileMetadataInput { fileId: string assertedWorkspaceId?: string + /** + * Opt into the archived lifecycle set. It relaxes only the `deleted_at` predicate on the + * canonical row lookup — the workspace the file resolves to, the asserted-workspace check, + * and the `files.read_metadata` authorization that follows are identical either way, so it + * never widens who may read a file. + */ includeDeleted?: boolean } From eb50a58705efe61d525bc2d24f899aee6e84ebf1 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 14 Aug 2026 12:57:29 -0700 Subject: [PATCH 2/4] fix(v2): correct three regressions this branch introduced, and harden its tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the previous commit found that three of its "behavior preserving" claims were wrong. Each is corrected here at the layer that owns it. Table filters no longer coerce a `date` operand, and no longer throw. `date` is the one column type whose registry `coerce` is not idempotent — it drops sub-second precision — and the leaf that compiles a filter also builds the unique-constraint and upsert-conflict probes, so re-reading an already-coerced operand could stop it matching the row it was written from and admit a duplicate inside the write transaction with no error. Throwing was the second mistake: the v2 predicate grammar type-checks structure but not operand values, so a rejected operand no longer failed at submission but inside the delete, update, dispatch and cancel runners, where a filter that cannot compile means the cells it started can no longer be cancelled. Coercion is now total — it rewrites what the registry accepts and passes everything else through unchanged, exactly as before. Reviving a force-failed run no longer inherits its terminal duration. Writing `ended_at` and `total_duration_ms` on the force-fail boundary was correct in isolation, but a partial resume flips that row back to `pending` and those columns survived. The preserved value is meant to be the pause checkpoint — the run's active time — and it had become wall clock measured at the failed resume, which the checkpoint rule then faithfully carried into the next terminal write. The revival clears them only for a row that was terminal, so an ordinary paused row keeps the checkpoint it is supposed to keep. Cancelling reports the terminal state it actually observed. Reclassification now requires that nothing else went wrong, so a genuine paused-reconciliation failure survives instead of being rewritten as an already-terminal no-op, and the claim's own row count — not a snapshot read before it — decides whether this cancel terminalized the run or lost a race to something else. The status the snapshot needed rides along on the ownership query that already reads the row, rather than the second read that query's own contract warns against. A custom tool that cannot be projected now answers the same way everywhere: the list omits it, and reading or patching it by id reports it as absent rather than as a server fault. Analytics stops reporting a cancellation for a request that cancelled nothing. The tests around all of this were audited by mutating each fix and checking the suite noticed. Where it did not, the assertion is stronger now: the absent content-type branch is genuinely exercised rather than relying on a header the client library supplies, the duration encoder is pinned to the column it must measure from, execution ownership is pinned to both ids it must match, and the archived-file concealment test proves it conceals the archived read specifically. Two tests that asserted a paused branch they could not observe are gone; the rendered-SQL test that can decide it already covers them. --- .../api/v2/custom-tools/[id]/route.test.ts | 50 ++++++ .../sim/app/api/v2/custom-tools/[id]/route.ts | 16 +- .../sim/app/api/v2/custom-tools/route.test.ts | 100 +++++++++-- apps/sim/app/api/v2/custom-tools/utils.ts | 102 +++++++----- .../v2/files/[fileId]/metadata/route.test.ts | 16 +- apps/sim/app/api/v2/lib/response.test.ts | 28 +++- .../[id]/runs/[runId]/cancel/route.ts | 10 +- .../routes/v2-body-lifecycle-route.test.ts | 25 +++ .../api/server/routes/v2-json-route.test.ts | 14 +- .../cancel-workflow-execution.test.ts | 136 ++++++++++++--- .../execution/cancel-workflow-execution.ts | 98 +++++++---- .../logs/execution/logging-session.test.ts | 36 ++-- apps/sim/lib/table/__tests__/sql.test.ts | 157 ++++++++++++++---- apps/sim/lib/table/sql.ts | 90 +++++----- .../executor/execution-queries.test.ts | 79 ++++++++- .../workflows/executor/execution-queries.ts | 18 +- .../human-in-the-loop-manager.test.ts | 120 ++++++++++--- .../executor/human-in-the-loop-manager.ts | 28 +++- 18 files changed, 893 insertions(+), 230 deletions(-) diff --git a/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts b/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts index ca4784712e4..11a5b6956c0 100644 --- a/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts +++ b/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts @@ -188,6 +188,56 @@ describe('/api/v2/custom-tools/[id]', () => { expect(mocks.update).not.toHaveBeenCalled() }) + /** + * The list omits a row it cannot project, so this surface must not answer the + * same row with a 500 — a caller who lists and sees nothing, then fetches by + * id and sees a server fault, can act on neither answer. Both surfaces say + * "not addressable here"; the recoveries (DELETE, or a PATCH carrying a valid + * schema) do not go through the projection and still work. + */ + describe('a stored row that cannot be projected onto the contract', () => { + const unrepairable = { ...tool, schema: 'this is not json' } + const repairable = { ...tool, schema: JSON.stringify(tool.schema) } + + it('answers a read with the same 404 the list implies by omitting it', async () => { + mocks.get.mockResolvedValue({ tool: unrepairable }) + + const response = await GET(request('GET'), context) + + expect(response.status).toBe(404) + expect((await response.json()).error).toMatchObject({ + code: 'NOT_FOUND', + message: 'Custom tool not found', + }) + }) + + it('answers a write with the same 404, leaving delete and a full-schema patch as the recoveries', async () => { + mocks.update.mockResolvedValue({ tool: unrepairable }) + expect( + (await PATCH(request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }), context)) + .status + ).toBe(404) + + mocks.remove.mockResolvedValue({ tool: unrepairable }) + expect((await DELETE(request('DELETE'), context)).status).toBe(200) + }) + + it('serves a repairable row on both single-resource verbs', async () => { + mocks.get.mockResolvedValue({ tool: repairable }) + const read = await GET(request('GET'), context) + expect(read.status).toBe(200) + expect((await read.json()).data.schema).toEqual(tool.schema) + + mocks.update.mockResolvedValue({ tool: repairable }) + const written = await PATCH( + request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }), + context + ) + expect(written.status).toBe(200) + expect((await written.json()).data.schema).toEqual(tool.schema) + }) + }) + it('conceals cross-tenant access while preserving same-workspace role denials', async () => { mocks.get.mockRejectedValueOnce(new NoWorkspaceAccessError()) expect((await GET(request('GET'), context)).status).toBe(404) diff --git a/apps/sim/app/api/v2/custom-tools/[id]/route.ts b/apps/sim/app/api/v2/custom-tools/[id]/route.ts index 199c2930a07..bd654423142 100644 --- a/apps/sim/app/api/v2/custom-tools/[id]/route.ts +++ b/apps/sim/app/api/v2/custom-tools/[id]/route.ts @@ -15,13 +15,25 @@ import { getWorkspaceCustomToolUseCase, updateWorkspaceCustomToolUseCase, } from '@/lib/custom-tools/application/use-cases' -import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { MalformedCustomToolRowError, toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { v2CaughtOrchestrationError, v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 +const NOT_FOUND_MESSAGE = 'Custom tool not found' + +/** + * Conceals cross-tenant denials, and answers a row that cannot be projected + * onto the contract with the same `404` — so this surface and the list, which + * omits such a row, tell one caller one story. See {@link toV2CustomTool}. + */ const customToolResourceErrorPolicy = createV2ResourceConcealmentPolicy({ - notFoundMessage: 'Custom tool not found', + notFoundMessage: NOT_FOUND_MESSAGE, + render: (error) => + error instanceof MalformedCustomToolRowError + ? v2Error('NOT_FOUND', NOT_FOUND_MESSAGE) + : v2CaughtOrchestrationError(error), }) /** GET /api/v2/custom-tools/[id] — Fetch a single custom tool. */ diff --git a/apps/sim/app/api/v2/custom-tools/route.test.ts b/apps/sim/app/api/v2/custom-tools/route.test.ts index 1e29a606fc7..bc43d671e36 100644 --- a/apps/sim/app/api/v2/custom-tools/route.test.ts +++ b/apps/sim/app/api/v2/custom-tools/route.test.ts @@ -6,6 +6,24 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const { mocks, log, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { class MockV2ApiKeyUnauthenticatedError extends Error {} + /** + * The same surface `createMockLogger` provides, because this stub *replaces* + * the global `@sim/logger` mock for this file. A narrower one is not merely + * incomplete — the first module in this route's graph to call `logger.trace` + * or `logger.child` would throw `TypeError` here and nowhere else, which reads + * as a route bug rather than a missing mock method. `child`/`withMetadata` + * return the same instance so a chained call still records on `log`. + */ + const log: Record = { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + } + log.child = vi.fn(() => log) + log.withMetadata = vi.fn(() => log) return { mocks: { authenticate: vi.fn(), @@ -15,7 +33,16 @@ const { mocks, log, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { list: vi.fn(), create: vi.fn(), }, - log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + log: log as { + info: ReturnType + warn: ReturnType + error: ReturnType + debug: ReturnType + trace: ReturnType + fatal: ReturnType + child: ReturnType + withMetadata: ReturnType + }, MockV2ApiKeyUnauthenticatedError, } }) @@ -308,24 +335,77 @@ describe('/api/v2/custom-tools', () => { expect(status).toBe(200) expect(body.data.map((t: { id: string }) => t.id)).toEqual(['tool-1']) for (const toolId of ['unparseable', 'no-parameters-type']) { - expect(log.warn).toHaveBeenCalledWith( - expect.stringContaining('Omitted'), + expect(log.error).toHaveBeenCalledWith( + expect.stringContaining('cannot be projected'), expect.objectContaining({ toolId, workspaceId: WORKSPACE_ID }) ) } }) - it('still mints a next cursor when the page contained a skipped row', async () => { - mocks.list.mockResolvedValue({ - tools: [malformed('unparseable', 'this is not json')], - nextCursorKeys: ['2026-01-01T00:00:00.000Z', 'unparseable'], - }) + /** + * The repair guards must be unreachable for a row that already validates — + * otherwise the recovery path could rewrite rows it was never meant to + * touch. Pinned on a stored schema carrying an unrelated extension key, so + * a repair that rebuilt the object rather than leaving it alone would show + * up as a lost field rather than passing on a shallow shape check. + */ + it('emits a valid row exactly as stored, with no repair applied', async () => { + const stored = { + ...TOOL_SCHEMA, + 'x-vendor': { owner: 'billing' }, + function: { ...TOOL_SCHEMA.function, description: 'Look up an order' }, + } + mocks.list.mockResolvedValue({ tools: [{ ...tool, schema: stored }] }) const { status, body } = await list() expect(status).toBe(200) - expect(body.data).toEqual([]) - expect(body.nextCursor).toEqual(expect.any(String)) + expect(body.data[0].schema).toEqual(stored) + expect(log.warn).not.toHaveBeenCalledWith( + expect.stringContaining('Repaired'), + expect.anything() + ) + expect(log.error).not.toHaveBeenCalledWith( + expect.stringContaining('cannot be projected'), + expect.anything() + ) + }) + + /** + * A page whose rows all skip returns `data: []` with a non-null + * `nextCursor`, so `nextCursor` — never page length — is this list's + * completeness signal. Pins that the documented client loop terminates and + * observes every projectable row across an all-skipped page. + */ + it('lets a client following nextCursor terminate and see every projectable row', async () => { + const second = { ...tool, id: 'tool-2', title: 'refund_order' } + const pages = [ + { tools: [tool, malformed('bad-1', 'this is not json')], nextCursorKeys: ['a', 'bad-1'] }, + { tools: [malformed('bad-2', 'this is not json')], nextCursorKeys: ['b', 'bad-2'] }, + { tools: [second] }, + ] + mocks.list.mockImplementation(async ({ input }) => + input.cursorKeys === undefined ? pages[0] : pages[input.cursorKeys[0] === 'a' ? 1 : 2] + ) + + const seen: string[] = [] + const pageSizes: number[] = [] + let cursor: string | null = null + do { + expect(pageSizes.length).toBeLessThan(pages.length) + const query = cursor ? `&cursor=${encodeURIComponent(cursor)}` : '' + const response = await GET( + request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}${query}`) + ) + expect(response.status).toBe(200) + const body = await response.json() + for (const t of body.data) seen.push(t.id) + pageSizes.push(body.data.length) + cursor = body.nextCursor + } while (cursor !== null) + + expect(pageSizes).toEqual([1, 0, 1]) + expect(seen).toEqual(['tool-1', 'tool-2']) }) }) diff --git a/apps/sim/app/api/v2/custom-tools/utils.ts b/apps/sim/app/api/v2/custom-tools/utils.ts index 5af4fdec8b8..e70c0c879dc 100644 --- a/apps/sim/app/api/v2/custom-tools/utils.ts +++ b/apps/sim/app/api/v2/custom-tools/utils.ts @@ -13,14 +13,14 @@ type CustomToolRow = typeof customTools.$inferSelect * A stored row whose `schema` column cannot be projected onto the public * contract even after the safe repairs in {@link repairStoredSchema}. * - * Single-resource surfaces throw this; the list surface skips the row instead - * so one corrupt row cannot make a whole page unreachable. + * Thrown by {@link toV2CustomTool} and consumed by the single-resource route's + * error policy, which renders it as the same `404 Custom tool not found` the + * list surface implies by omitting the row. The identifying detail lives in the + * message and in the structured `error` log at the throw site, so this carries + * no fields of its own. */ export class MalformedCustomToolRowError extends Error { - constructor( - readonly toolId: string, - readonly reason: string - ) { + constructor(toolId: string, reason: string) { super(`Custom tool ${toolId} has a malformed stored schema: ${reason}`) this.name = 'MalformedCustomToolRowError' } @@ -43,6 +43,13 @@ function rowIdentity(row: CustomToolRow) { * The contract types that field as `z.literal('function')`, so there is * exactly one legal value and filling it invents no information. * + * Neither branch can fire on a row that was already contract-valid: a valid + * `schema` is an object, never a string, and the contract types its top-level + * `type` as a required `z.literal('function')`, so it is never `undefined`. + * Both guards therefore only see shapes that had already failed validation, and + * repair can only turn a rejection into an acceptance — never alter a row that + * would have been emitted as stored. + * * Deliberately NOT repaired: `function.parameters.type`, which the contract * types as an open `z.string()`. Substituting `'object'` there would be a guess * about JSON-Schema semantics that changes how a model calls the tool. @@ -71,7 +78,9 @@ function repairStoredSchema(stored: unknown): { value: unknown; repairs: string[ /** * Projects a stored row onto the public contract, repairing what is safely * repairable. Reports a reason instead when the row cannot be made - * contract-valid. + * contract-valid, and logs that failure here — one signal per defective row, so + * both surfaces raise it identically rather than each describing the row in its + * own words. * * `workspaceId` and `userId` are internal scoping columns and are not exposed. */ @@ -88,11 +97,14 @@ function projectV2CustomTool(row: CustomToolRow): { tool: V2CustomTool } | { rea }) if (!parsed.success) { - return { - reason: parsed.error.issues - .map((issue) => `${issue.path.join('.') || ''}: ${issue.message}`) - .join('; '), - } + const reason = parsed.error.issues + .map((issue) => `${issue.path.join('.') || ''}: ${issue.message}`) + .join('; ') + logger.error('Custom tool row cannot be projected onto the v2 contract', { + ...rowIdentity(row), + reason, + }) + return { reason } } if (repairs.length > 0) { @@ -107,51 +119,63 @@ function projectV2CustomTool(row: CustomToolRow): { tool: V2CustomTool } | { rea /** * Public custom tool projection for single-resource surfaces (read, create, - * update), where there is no other row to serve and failing loudly is the - * honest outcome. + * update). * * @throws {MalformedCustomToolRowError} when the row is not contract-valid. + * The single-resource routes render that as `404 Custom tool not found`, which + * is the same answer {@link toV2CustomToolList} gives by omitting the row. A + * `500` there would leave the two surfaces contradicting each other about one + * row — listed as absent, fetched as a server fault — and a caller could act on + * neither. `404` states the one thing that is true of the row on this API: it + * cannot be addressed here. It also stays actionable, because the recoveries + * do not go through this projection — `DELETE` removes the row, and a `PATCH` + * supplying a contract-valid `schema` repairs it and returns `200`. */ export function toV2CustomTool(row: CustomToolRow): V2CustomTool { const result = projectV2CustomTool(row) - if ('reason' in result) { - logger.error('Custom tool row cannot be projected onto the v2 contract', { - ...rowIdentity(row), - reason: result.reason, - }) - throw new MalformedCustomToolRowError(row.id, result.reason) - } + if ('reason' in result) throw new MalformedCustomToolRowError(row.id, result.reason) return result.tool } /** * Public custom tool projection for the keyset-paginated list. * - * Rows that stay malformed after repair are omitted and logged at `warn` rather - * than thrown. Throwing here fails the whole page, and because the list is - * keyset-paginated the caller cannot page past the bad row — every page - * containing it becomes permanently unreachable. An incomplete page is a real - * cost, but it is strictly smaller than no page at all, and the omission is - * recorded server-side with enough identity to find and fix the row. + * Rows that stay malformed after repair are omitted rather than thrown. + * Throwing here fails the whole page, and because the list is keyset-paginated + * the caller cannot page past the bad row — every page containing it becomes + * permanently unreachable. An incomplete page is a real cost, but it is + * strictly smaller than no page at all. * - * Pagination stays coherent: `nextCursor` is minted from the keys the use case - * read out of the database, not from this projection, so a skipped row still - * advances the cursor past itself. The list response carries no total, so no - * count metadata contradicts a short page — a caller must follow `nextCursor` - * rather than infer completeness from a page's length. + * The page is deliberately **not** drained back up to the requested limit. A + * page whose rows all skip therefore returns `data: []` with a non-null + * `nextCursor`, which is safe because `nextCursor` — not page length — is this + * list's completeness signal: + * + * - `listWorkspaceCustomTools` reads `limit + 1` rows and `keysetPage` mints + * `nextCursorKeys` from the extra row, so `nextCursor` is null exactly when + * the keyset is exhausted, independent of anything this projection does. + * - Each page resumes strictly after the last row the previous page *read*, not + * the last row it *emitted*, so a skipped row still advances the cursor past + * itself and is never revisited. + * - A caller looping `while (nextCursor !== null)` therefore terminates in + * `ceil(rows / limit)` requests over any workspace and observes every + * projectable row exactly once — including rows that follow an all-skipped + * page. + * + * Draining instead would mean re-entering the authorized list use case from a + * presenter, which is the surface adapter re-reading protected data, and it + * would still need a bound — so a workspace of mostly-defective rows would + * return a short page anyway, just less predictably. The trap it removes is a + * caller looping on `data.length`, which this list has never been able to + * promise: the response carries no total, and page length has always been an + * artifact of the read rather than a statement about the keyset. */ export function toV2CustomToolList(rows: CustomToolRow[]): V2CustomTool[] { const tools: V2CustomTool[] = [] for (const row of rows) { const result = projectV2CustomTool(row) - if ('reason' in result) { - logger.warn('Omitted a malformed custom tool row from the v2 list response', { - ...rowIdentity(row), - reason: result.reason, - }) - continue - } + if ('reason' in result) continue tools.push(result.tool) } diff --git a/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts b/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts index 6401ceb1850..82b8deb787f 100644 --- a/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts +++ b/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts @@ -174,6 +174,11 @@ describe('GET /api/v2/files/[fileId]/metadata', () => { expect(response.status).toBe(404) expect((await response.json()).error.code).toBe('NOT_FOUND') + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: false }, + }) + ) }) it('returns archived metadata when scope=archived opts into the archived set', async () => { @@ -212,8 +217,17 @@ describe('GET /api/v2/files/[fileId]/metadata', () => { expect(response.status).toBe(404) expect((await response.json()).error.code).toBe('NOT_FOUND') + /** + * `includeDeleted: true` is what makes this the *archived* read being + * concealed rather than the plain cross-workspace 404 the suite already + * pins: without it the request never reaches the archived set and the test + * proves only `NoWorkspaceAccessError → 404`. + */ expect(mocks.readMetadata).toHaveBeenCalledWith( - expect.objectContaining({ principal: auth.principal }) + expect.objectContaining({ + principal: auth.principal, + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: true }, + }) ) }) diff --git a/apps/sim/app/api/v2/lib/response.test.ts b/apps/sim/app/api/v2/lib/response.test.ts index 460b188a136..b45aa898340 100644 --- a/apps/sim/app/api/v2/lib/response.test.ts +++ b/apps/sim/app/api/v2/lib/response.test.ts @@ -55,6 +55,15 @@ describe('v2Error retry guidance', () => { * render here. */ describe('v2 401 authentication challenge', () => { + /** + * Pinned exactly at the primary funnel rather than asserted as merely present. + * `toBeTruthy` accepts any string, so the scheme token, the realm, and the + * `header=` parameter that names the only channel v2 reads could all change + * without a test noticing. The reachability tests below stay loose on purpose + * — they pin that the header arrives down each path, not its value twice. + */ + const EXPECTED_CHALLENGE = 'SimApiKey realm="Sim API", header="x-api-key"' + const challenge = () => v2Error('UNAUTHORIZED', 'API key required').headers.get('WWW-Authenticate') @@ -62,7 +71,7 @@ describe('v2 401 authentication challenge', () => { const response = v2Error('UNAUTHORIZED', 'Invalid API key') expect(response.status).toBe(401) - expect(response.headers.get('WWW-Authenticate')).toBeTruthy() + expect(response.headers.get('WWW-Authenticate')).toBe(EXPECTED_CHALLENGE) }) it('names the x-api-key header, the only channel v2 actually reads', () => { @@ -107,4 +116,21 @@ describe('v2 401 authentication challenge', () => { expect(v2Error(code, 'nope').headers.get('WWW-Authenticate')).toBeNull() } }) + + /** + * `options.headers` is spread *after* the challenge, so a caller-supplied + * `WWW-Authenticate` replaces the default rather than being ignored. That + * precedence is the whole reason the default is safe to install + * unconditionally on 401 — a route with a genuinely different challenge is + * not fighting it — but it also means an accidental override silently wins. + * Documented here so a reordering of the spread is a test failure either way. + */ + it('lets a caller-supplied challenge override the default', () => { + const response = v2Error('UNAUTHORIZED', 'Invalid token', { + headers: { 'WWW-Authenticate': 'Bearer realm="mcp"' }, + }) + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBe('Bearer realm="mcp"') + }) }) diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts index 5418355124d..c492fe46957 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts @@ -27,8 +27,16 @@ export const POST = defineV2JsonRoute({ reason: result.reason, }, }), + /** + * Reports a cancellation, so it needs the run to have actually been + * cancelled. `success` alone no longer implies that: a cancel against an + * already-terminal run satisfies the request without writing anything, and + * reports `success: true` with `durablyRecorded: false`. Requiring both also + * keeps the event off a cancellation that reached the row but failed its + * paused reconciliation, which reports the inverse pair. + */ onSuccess: ({ principal, result }) => { - if (!result.success || principal.kind !== 'personal_api_key') return + if (!result.success || !result.durablyRecorded || principal.kind !== 'personal_api_key') return captureServerEvent( principal.userId, 'workflow_execution_cancelled', diff --git a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts index 6885f3d1a4d..bfb078e8c2f 100644 --- a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts +++ b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts @@ -190,6 +190,31 @@ describe('defineV2BodyLifecycleRoute', () => { expect(response.headers.get('x-request-id')).toBeTruthy() }) + /** + * `v2InvalidBodyResponse` answers `415` when an unreadable body declared a + * non-JSON media type, and `multipart/form-data` is exactly such a type. That + * change is argued safe for this builder in prose — its contract must omit the + * body schema, so `parseRequest` never attempts a JSON read and the + * classification is unreachable — but nothing executed it. A multipart upload + * is the shape this builder exists for, so it gets a test rather than a + * paragraph. + */ + it('accepts a multipart body, which the JSON builder would classify as 415', async () => { + const form = new FormData() + form.append('file', new Blob([new Uint8Array([1, 2, 3])]), 'data.bin') + const request = new NextRequest( + 'http://localhost/api/v2/body-lifecycle/item-1?workspaceId=workspace-1', + { method: 'POST', headers: { 'x-api-key': 'secret' }, body: form } + ) + expect(request.headers.get('content-type')).toContain('multipart/form-data') + + const response = await buildHandler()(request, context()) + + expect(response.status).toBe(201) + expect(await response.json()).toEqual({ data: { id: 'item-1' } }) + expect(mocks.order).toContain('body') + }) + it('rejects at the IP abuse limit before authentication', async () => { v2RouteMocks.preauthRate.mockImplementation(async () => { mocks.order.push('ip-limit') diff --git a/apps/sim/lib/api/server/routes/v2-json-route.test.ts b/apps/sim/lib/api/server/routes/v2-json-route.test.ts index 937cc8fc97b..54755d94317 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.test.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.test.ts @@ -516,15 +516,25 @@ describe('defineV2JsonRoute unreadable body classification', () => { v2RouteMocks.operationRate.mockResolvedValue(allowedRate) }) + /** + * A `string` body makes undici *derive* `content-type: text/plain;charset=UTF-8`, + * so omitting the header from `headers` is not enough to produce the + * absent-media-type request — the `null` case has to send pre-encoded bytes. + * The assertion is the guard that keeps that from silently drifting back: + * without it the two `contentType === null` cases secretly re-test `text/plain` + * and the `if (!header) return false` branch never runs. + */ function bodyRequest(contentType: string | null, body: string): NextRequest { - return new NextRequest('http://localhost/api/v2/widgets', { + const request = new NextRequest('http://localhost/api/v2/widgets', { method: 'POST', headers: { 'x-api-key': 'secret', ...(contentType === null ? {} : { 'content-type': contentType }), }, - body, + body: contentType === null ? new TextEncoder().encode(body) : body, }) + if (contentType === null) expect(request.headers.get('content-type')).toBeNull() + return request } it('answers 415 when an unreadable body declared a non-JSON media type', async () => { diff --git a/apps/sim/lib/execution/cancel-workflow-execution.test.ts b/apps/sim/lib/execution/cancel-workflow-execution.test.ts index a06f7aed897..bc3af8748e5 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.test.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.test.ts @@ -17,8 +17,8 @@ const { mockPublishWorkflowGroupCancellationEvent, mockReleaseExecutionSlot, mockUpdateSet, + mockUpdateReturning, mockResolveWorkflowExecutionOwnership, - mockSelectExecutionLogRows, } = vi.hoisted(() => ({ mockAbortManualExecution: vi.fn(), mockBeginPausedCancellation: vi.fn(), @@ -33,23 +33,16 @@ const { mockPublishWorkflowGroupCancellationEvent: vi.fn(), mockReleaseExecutionSlot: vi.fn(), mockUpdateSet: vi.fn(), + mockUpdateReturning: vi.fn(), mockResolveWorkflowExecutionOwnership: vi.fn(), - mockSelectExecutionLogRows: vi.fn(), })) vi.mock('@sim/db', () => ({ db: { - select: () => ({ - from: () => ({ - where: () => ({ - limit: () => Promise.resolve(mockSelectExecutionLogRows()), - }), - }), - }), update: () => ({ set: (values: unknown) => { mockUpdateSet(values) - return { where: () => Promise.resolve(undefined) } + return { where: () => ({ returning: () => Promise.resolve(mockUpdateReturning()) }) } }, }), }, @@ -121,8 +114,9 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: null, + priorStatus: 'running', }) - mockSelectExecutionLogRows.mockReturnValue([{ status: 'running' }]) + mockUpdateReturning.mockReturnValue([{ id: 'log-1' }]) mockBeginPausedCancellation.mockResolvedValue(false) mockGetPausedCancellationStatus.mockResolvedValue(null) mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: true, reason: 'recorded' }) @@ -138,10 +132,28 @@ describe('cancelWorkflowExecution', () => { }) }) + /** + * The row reads `cancelled` after a successful cancel just as it does after + * someone else's, so a status re-read alone would report this run's own work + * as `already_cancelled`. Nothing is re-read once the claim moved a row. + */ it('reports a durable write when an active run is cancelled', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'running', + }) + .mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'cancelled', + }) + const result = await cancelWorkflowExecution(INPUT) expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(1) }) /** @@ -156,33 +168,109 @@ describe('cancelWorkflowExecution', () => { ['completed', 'already_completed'], ['failed', 'already_failed'], ])('reports a run already %s as a no-op rather than a durable write', async (status, reason) => { - mockSelectExecutionLogRows.mockReturnValue([{ status }]) + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: status, + }) + mockUpdateReturning.mockReturnValue([]) const result = await cancelWorkflowExecution(INPUT) expect(result).toMatchObject({ success: true, durablyRecorded: false, reason }) }) - it('still reports the failing step when a terminal run has paused work left over', async () => { - mockSelectExecutionLogRows.mockReturnValue([{ status: 'cancelled' }]) - mockBeginPausedCancellation.mockResolvedValue(true) - mockCompletePausedCancellation.mockResolvedValue(false) + /** + * Reclassification only ever applies to an otherwise-clean outcome. A run that + * reached any terminal status can still carry paused-HITL state — a + * force-failed run keeps whatever pause rows it had — and when reconciling + * that genuinely fails, the caller is owed the step that failed rather than a + * no-op that also flips `success` to `true`. + */ + it.each([['cancelled'], ['completed'], ['failed']])( + 'still reports the failing step when a %s run has paused work left over', + async (status) => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: status, + }) + mockBeginPausedCancellation.mockResolvedValue(true) + mockCompletePausedCancellation.mockResolvedValue(false) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: false, + durablyRecorded: true, + reason: 'paused_database_cancel_failed', + }) + } + ) + + /** + * The status read at entry can be stale: a run that finishes after it and + * before the claim leaves a `running` snapshot on a cancel whose claim matched + * no row. The claim's own row count is what separates that from a cancel this + * request really performed. + */ + it.each([ + ['completed', 'already_completed'], + ['failed', 'already_failed'], + ])('reports a run that reached %s after the entry read as a no-op', async (status, reason) => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'running', + }) + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: status, + }) + mockUpdateReturning.mockReturnValue([]) const result = await cancelWorkflowExecution(INPUT) - expect(result).toMatchObject({ - success: false, - durablyRecorded: true, - reason: 'paused_database_cancel_failed', + expect(result).toMatchObject({ success: true, durablyRecorded: false, reason }) + }) + + it('reports an undifferentiated outcome when the claim finds no durable log row', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: null, }) + mockUpdateReturning.mockReturnValue([]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) }) - it('reports an undifferentiated outcome when the run has no durable log row', async () => { - mockSelectExecutionLogRows.mockReturnValue([]) + /** + * The re-read is purely observational — it only refines *which* no-op the + * caller is told about. A database that cannot answer it must not take the + * cancel down with it: the run has already been cancelled in Redis and its + * reservation still has to be released, so the failure degrades to the + * undifferentiated outcome rather than propagating. + */ + it('degrades to the undifferentiated outcome when the status re-read fails', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'running', + }) + .mockRejectedValueOnce(new Error('connection terminated')) + mockUpdateReturning.mockReturnValue([]) const result = await cancelWorkflowExecution(INPUT) expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(2) + expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('execution-1') }) it('releases the plan concurrency reservation after a successful cancellation', async () => { @@ -208,6 +296,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) const cancelled = { kind: 'cancelled' as const, @@ -294,6 +383,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) const failure = new Error('Workflow-group cancellation lost its locked workflow-log claim') mockCancelWorkflowGroupExecution.mockRejectedValue(failure) @@ -308,6 +398,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) mockBeginPausedCancellation.mockResolvedValue(true) mockCancelWorkflowGroupExecution.mockRejectedValue(new Error('serialization conflict')) @@ -320,6 +411,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: false, diff --git a/apps/sim/lib/execution/cancel-workflow-execution.ts b/apps/sim/lib/execution/cancel-workflow-execution.ts index 4d2285d1945..527c14a6e75 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.ts @@ -61,7 +61,7 @@ export type CancelWorkflowExecutionReason = | 'paused_event_publish_failed' | 'paused_database_cancel_failed' -/** Log statuses a cancel claim can never move, in the order they are reported. */ +/** Maps each log status a cancel claim can never move to the outcome that reports it. */ const TERMINAL_NO_OP_REASONS = { cancelled: 'already_cancelled', completed: 'already_completed', @@ -70,40 +70,54 @@ const TERMINAL_NO_OP_REASONS = { type TerminalExecutionStatus = keyof typeof TERMINAL_NO_OP_REASONS -function toTerminalExecutionStatus(status: string | undefined): TerminalExecutionStatus | null { - return status !== undefined && status in TERMINAL_NO_OP_REASONS +function toTerminalExecutionStatus( + status: string | null | undefined +): TerminalExecutionStatus | null { + return typeof status === 'string' && status in TERMINAL_NO_OP_REASONS ? (status as TerminalExecutionStatus) : null } /** - * Reads the durable status the run already carried, so the reported outcome can - * tell a real cancellation apart from a request against a run that had already - * finished. Purely observational — it gates no effect, and a read failure falls - * back to the undifferentiated report rather than blocking the cancel. + * What the direct terminal log claim did: moved the run to `cancelled` here and + * now, matched no row, or never ran — the workflow-group and paused paths write + * their terminal row elsewhere, and a failed statement knows nothing either way. + */ +type DirectTerminalWriteOutcome = 'applied' | 'no_row' | 'unknown' + +/** + * Names the terminal state the cancel could not move, or `null` when it did + * real work or when this path cannot tell — in which case the caller keeps the + * undifferentiated report rather than guessing. + * + * The status read at entry is not enough on its own: a run that finishes + * between that read and the claim leaves a stale non-terminal snapshot behind a + * cancel that wrote nothing. The claim's own row count settles that, and a + * plain post-read cannot: after a successful cancel the row reads `cancelled` + * too, so the state has to be attributed to whoever wrote it. A claim that + * moved no row against a non-terminal snapshot re-reads the row it lost the + * race to, through the same ownership query the entry read came from. * - * Runs alongside `resolveWorkflowExecutionOwnership`, which reads the same row; - * folding the column into that helper would drop the extra round trip, but it - * lives outside this change's file boundary. + * Purely observational — it gates no effect, and a read failure falls back to + * the undifferentiated report rather than failing the cancel. */ -async function readTerminalExecutionStatus( +async function resolveTerminalNoOpReason( executionId: string, - workflowId: string -): Promise { + workflowId: string, + priorTerminalStatus: TerminalExecutionStatus | null, + directTerminalWrite: DirectTerminalWriteOutcome +): Promise { + if (priorTerminalStatus !== null) return TERMINAL_NO_OP_REASONS[priorTerminalStatus] + if (directTerminalWrite !== 'no_row') return null try { - const [row] = await db - .select({ status: workflowExecutionLogs.status }) - .from(workflowExecutionLogs) - .where( - and( - eq(workflowExecutionLogs.executionId, executionId), - eq(workflowExecutionLogs.workflowId, workflowId) - ) - ) - .limit(1) - return toTerminalExecutionStatus(row?.status) + const { priorStatus } = await resolveWorkflowExecutionOwnership(executionId, workflowId) + const terminalStatus = toTerminalExecutionStatus(priorStatus) + return terminalStatus !== null ? TERMINAL_NO_OP_REASONS[terminalStatus] : null } catch (error) { - logger.warn('Failed to read execution status before cancelling', { executionId, error }) + logger.warn('Failed to re-read execution status after an unmatched cancel claim', { + executionId, + error, + }) return null } } @@ -225,11 +239,10 @@ export async function cancelWorkflowExecution( ): Promise { const { executionId, workflowId, userId, workspaceId } = input - const [{ belongsToWorkflow, workflowGroupWorkspaceId }, priorTerminalStatus] = await Promise.all([ - resolveWorkflowExecutionOwnership(executionId, workflowId), - readTerminalExecutionStatus(executionId, workflowId), - ]) + const { belongsToWorkflow, workflowGroupWorkspaceId, priorStatus } = + await resolveWorkflowExecutionOwnership(executionId, workflowId) if (!belongsToWorkflow) throw new WorkflowExecutionNotFoundError() + const priorTerminalStatus = toTerminalExecutionStatus(priorStatus) let pausedCancellationStarted = false let pausedCancelled = false @@ -424,6 +437,11 @@ export async function cancelWorkflowExecution( ? groupCancellation : null + /** + * The claim's row count is read back only to report it — `returning` changes + * what the statement returns, never the row it writes or the rows it matches. + */ + let directTerminalWrite: DirectTerminalWriteOutcome = 'unknown' if ( groupCancellation === null && (cancellation.durablyRecorded || queuedJobCancelled || locallyAborted) && @@ -431,7 +449,7 @@ export async function cancelWorkflowExecution( ) { try { const cancelledAt = new Date() - await db + const claimedRows = await db .update(workflowExecutionLogs) .set(cancelledExecutionLogFields(cancelledAt)) .where( @@ -440,6 +458,8 @@ export async function cancelWorkflowExecution( eq(workflowExecutionLogs.status, 'running') ) ) + .returning({ id: workflowExecutionLogs.id }) + directTerminalWrite = claimedRows.length > 0 ? 'applied' : 'no_row' } catch (dbError) { logger.warn('Failed to update execution log status directly', { executionId, @@ -486,14 +506,20 @@ export async function cancelWorkflowExecution( * exactly as before — only the report changes. The request is still satisfied, * because the run is not running, so `success` stays `true`. * - * An already-`cancelled` run can still carry real paused-HITL reconciliation - * work, and when that step genuinely fails its reason must survive; only an - * otherwise-clean `recorded` is reinterpreted there. + * Reinterpreting is only ever right when nothing else went wrong. A terminal + * run — cancelled, or force-failed with paused state left behind — can still + * carry real paused-HITL reconciliation work, and a genuine failure there owes + * the caller the step that failed, not a no-op. So only an otherwise-clean + * `recorded` is a candidate, whatever the prior status was. */ const terminalNoOpReason = - priorTerminalStatus !== null && - (priorTerminalStatus !== 'cancelled' || (reason === 'recorded' && !pausedCancelled)) - ? TERMINAL_NO_OP_REASONS[priorTerminalStatus] + reason === 'recorded' && !pausedCancelled + ? await resolveTerminalNoOpReason( + executionId, + workflowId, + priorTerminalStatus, + directTerminalWrite + ) : null return { diff --git a/apps/sim/lib/logs/execution/logging-session.test.ts b/apps/sim/lib/logs/execution/logging-session.test.ts index 47c0c048ad4..3b11cbce4c7 100644 --- a/apps/sim/lib/logs/execution/logging-session.test.ts +++ b/apps/sim/lib/logs/execution/logging-session.test.ts @@ -1,3 +1,8 @@ +/** + * @vitest-environment node + */ + +import { workflowExecutionLogs } from '@sim/db/schema' import { dbChainMockFns, resetDbChainMock } from '@sim/testing' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' @@ -1696,7 +1701,7 @@ describe('LoggingSession.markExecutionAsFailed workflowId scoping', () => { level: string status: string endedAt: Date - totalDurationMs: unknown + totalDurationMs: { strings: TemplateStringsArray; values: unknown[] } executionDeadlineAt: Date | null executionData: unknown } @@ -1704,22 +1709,21 @@ describe('LoggingSession.markExecutionAsFailed workflowId scoping', () => { expect(payload.status).toBe('failed') expect(payload.endedAt).toBeInstanceOf(Date) expect(payload.executionDeadlineAt).toBeNull() - expect(payload.totalDurationMs).toBeDefined() - expect(dbMocks.sql.param).toHaveBeenCalledWith(payload.endedAt, expect.anything()) - }) - /** - * A resumed run whose pause state fails to persist can still be `pending`, and - * the duration it banked at the checkpoint must survive the force-fail rather - * than be redefined to include the time it sat waiting. - */ - it('leaves a paused run its checkpoint duration', async () => { - await LoggingSession.markExecutionAsFailed('exec-paused', 'boom', undefined, 'wf-1') - - const durationGuards = dbMocks.sql.mock.calls - .map(([strings]) => String(Array.from(strings))) - .filter((query) => query.includes("= 'pending'")) - expect(durationGuards).toHaveLength(1) + /** + * The duration is the derived SQL fragment, not a number the caller carried + * in — `elapsedDurationMsSql` measures against the row's own `started_at` + * and preserves what a paused row already banked. + */ + expect(String(Array.from(payload.totalDurationMs.strings))).toContain("= 'pending' THEN ") + expect(payload.totalDurationMs.values).toContain(workflowExecutionLogs.totalDurationMs) + + /** + * The end instant is bound through `started_at`'s encoder specifically. + * `endedAt`'s encoder renders identically and would silently subtract the + * timestamp from itself — a duration of zero on every force-failed run. + */ + expect(dbMocks.sql.param).toHaveBeenCalledWith(payload.endedAt, workflowExecutionLogs.startedAt) }) it('clears Redis markers when marking failed (terminal boundary outside completeWorkflowExecution)', async () => { diff --git a/apps/sim/lib/table/__tests__/sql.test.ts b/apps/sim/lib/table/__tests__/sql.test.ts index 1737fddc8ea..8623b06f7cf 100644 --- a/apps/sim/lib/table/__tests__/sql.test.ts +++ b/apps/sim/lib/table/__tests__/sql.test.ts @@ -1078,12 +1078,6 @@ describe('containment operators — operand is read through the column type', () expect(out).toContain('"title":"8"') }) - it('normalizes a date operand the same way the stored cell was normalized', () => { - const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: ' 2024-01-31 ' }] } - const out = render(buildPredicateClause(p, TABLE, DATE)) - expect(out).toContain('"due":"2024-01-31"') - }) - it('reads a formatted amount on a currency column', () => { const p: TablePredicate = { all: [{ field: 'price', op: 'eq', value: '$1,234.56' }] } const out = render(buildPredicateClause(p, TABLE, MONEY)) @@ -1093,53 +1087,112 @@ describe('containment operators — operand is read through the column type', () it('applies to the legacy $-grammar too', () => { const out = render(buildFilterClause({ score: { $eq: '8' } }, TABLE, NUM)) expect(out).toContain('"score":8') - expect(out).toContain('"score":8') + expect(out).not.toContain('"score":"8"') }) it('applies to the legacy equality shorthand', () => { - expect(render(buildFilterClause({ score: '8' }, TABLE, NUM))).toContain('"score":8') + const out = render(buildFilterClause({ score: '8' }, TABLE, NUM)) + expect(out).toContain('"score":8') + expect(out).not.toContain('"score":"8"') }) }) - describe('rejects an operand the column could never hold', () => { + /** + * Coercion is best-effort, never fatal. The v2 predicate grammar is not + * operand-type-checked at the boundary (leaf `value` is `z.unknown()`), so a + * throw here would land inside the background runners that compile the same + * predicate later — including a filter-scoped cancel, which would leave those + * cells uncancellable. An operand the column type refuses therefore compiles + * exactly as it did before: byte-exact, matching nothing. + */ + describe('passes through an operand the column could never hold', () => { it.each(['eq', 'ne'] as const)('%s with an unparseable number', (op) => { const p = { all: [{ field: 'score', op, value: 'eight' }] } as TablePredicate - expect(() => buildPredicateClause(p, TABLE, NUM)).toThrow( - `Operator "${op}" on column "score" (number) requires a number value, got string "eight"` - ) + const out = render(buildPredicateClause(p, TABLE, NUM)) + expect(out).toContain('"score":"eight"') }) - it.each(['in', 'nin'] as const)('%s naming a bad element', (op) => { - const p = { all: [{ field: 'score', op, value: [8, 'eight'] }] } as TablePredicate - expect(() => buildPredicateClause(p, TABLE, NUM)).toThrow(/requires a number value/) + it.each(['in', 'nin'] as const)('%s with a bad element', (op) => { + const p = { all: [{ field: 'score', op, value: ['eight'] }] } as TablePredicate + expect(render(buildPredicateClause(p, TABLE, NUM))).toContain('"score":"eight"') + }) + + it.each(['in', 'nin'] as const)('%s mixing coercible and uncoercible members', (op) => { + const p = { all: [{ field: 'score', op, value: ['8', 'eight'] }] } as TablePredicate + let out = '' + expect(() => { + out = render(buildPredicateClause(p, TABLE, NUM)) + }).not.toThrow() + expect(out).toContain('"score":8') + expect(out).toContain('"score":"eight"') }) - it('rejects a non-boolean on a boolean column', () => { + it('passes through a non-boolean on a boolean column', () => { const p: TablePredicate = { all: [{ field: 'flag', op: 'eq', value: 'yes' }] } - expect(() => buildPredicateClause(p, TABLE, BOOL)).toThrow( - 'Operator "eq" on column "flag" (boolean) requires a boolean value, got string "yes"' - ) + expect(render(buildPredicateClause(p, TABLE, BOOL))).toContain('"flag":"yes"') }) - it('rejects an unparseable date', () => { + it('passes through an unparseable date', () => { const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: 'not-a-date' }] } - expect(() => buildPredicateClause(p, TABLE, DATE)).toThrow(/requires a date value/) + expect(render(buildPredicateClause(p, TABLE, DATE))).toContain('"due":"not-a-date"') }) - it('rejects an object on a string column', () => { + it('passes through an object on a string column', () => { const p = { all: [{ field: 'title', op: 'eq', value: { a: 1 } }], } as unknown as TablePredicate - expect(() => buildPredicateClause(p, TABLE, STR)).toThrow(/requires a string value/) + expect(() => buildPredicateClause(p, TABLE, STR)).not.toThrow() }) - it('rejects it on the legacy $-grammar too', () => { - expect(() => buildFilterClause({ score: { $eq: 'eight' } }, TABLE, NUM)).toThrow( - /requires a number value/ + it('passes through on the legacy $-grammar too', () => { + expect(render(buildFilterClause({ score: { $eq: 'eight' } }, TABLE, NUM))).toContain( + '"score":"eight"' ) }) }) + /** + * `date` is excluded from containment coercion because `date.coerce` is not + * idempotent — `normalizeDateCellValue` drops the sub-second part, so the + * `.000Z` form the write path stores would be rewritten to a string that no + * longer matches the stored bytes. The same leaf compiles the unique and + * upsert probes, whose operands were already coerced once upstream, so a + * rewrite there would silently admit duplicates inside the write transaction. + */ + describe('never rewrites a date operand', () => { + it.each(['eq', 'ne'] as const)('%s keeps the stored .000Z form byte-exact', (op) => { + const p = { + all: [{ field: 'due', op, value: '2024-01-31T10:00:00.000Z' }], + } as TablePredicate + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":"2024-01-31T10:00:00.000Z"') + expect(out).not.toContain('"due":"2024-01-31T10:00:00Z"') + }) + + it.each(['in', 'nin'] as const)('%s keeps every member byte-exact', (op) => { + const p = { + all: [{ field: 'due', op, value: ['2024-01-31T10:00:00.000Z', ' 2024-01-31 '] }], + } as TablePredicate + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":"2024-01-31T10:00:00.000Z"') + expect(out).toContain('"due":" 2024-01-31 "') + }) + + it('does not trim or normalize a loose date operand', () => { + const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: ' 2024-01-31 ' }] } + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":" 2024-01-31 "') + expect(out).not.toContain('"due":"2024-01-31"') + }) + + it('keeps the legacy $-grammar byte-exact too', () => { + const out = render( + buildFilterClause({ due: { $eq: '2024-01-31T10:00:00.000Z' } }, TABLE, DATE) + ) + expect(out).toContain('"due":"2024-01-31T10:00:00.000Z"') + }) + }) + describe('leaves the operands that are not type assertions alone', () => { it('keeps null — a real containment query for a JSON-null cell', () => { const p: TablePredicate = { all: [{ field: 'score', op: 'eq', value: null }] } @@ -1156,15 +1209,57 @@ describe('containment operators — operand is read through the column type', () expect(render(buildPredicateClause(p, TABLE, NO_COLUMNS))).toContain('"adhoc":"8"') }) - it('leaves a select column to its own name→id resolution', () => { + /** + * `select` is excluded from containment coercion wholesale, and an operand + * that is already a declared option id cannot show that: `select.coerce` + * resolves it to itself, so the clause is identical with or without the + * exclusion. What discriminates is an operand `select.coerce` would + * *rewrite* — an option **name**, which `resolveSelectCellValue` turns into + * the option id. Names are already resolved upstream by + * `resolvePredicateSelectValues`, so a second resolution here is a rewrite + * of an operand that was deliberately left alone. + */ + describe('leaves a select column to its own name→id resolution', () => { const statusCol: ColumnDefinition = { id: 'col_status', name: 'status', type: 'select', options: [{ id: 'opt_open', name: 'Open' }], } - const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'opt_open' }] } - expect(render(buildPredicateClause(p, TABLE, [statusCol]))).toContain('"opt_open"') + + it('keeps a declared option id', () => { + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'opt_open' }] } + expect(render(buildPredicateClause(p, TABLE, [statusCol]))).toContain('"opt_open"') + }) + + it('does not re-resolve an option name into its id', () => { + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'Open' }] } + const out = render(buildPredicateClause(p, TABLE, [statusCol])) + expect(out).toContain('"col_status":"Open"') + expect(out).not.toContain('"col_status":"opt_open"') + }) + + it('does not re-resolve names inside an $in list', () => { + const p: TablePredicate = { + all: [{ field: 'col_status', op: 'in', value: ['Open', 'opt_open'] }], + } + const out = render(buildPredicateClause(p, TABLE, [statusCol])) + expect(out).toContain('"col_status":"Open"') + }) + + /** + * A filter for an option deleted since the row was written must still + * compile — the row still stores the id, and the operand reaches the + * clause byte-exact rather than being dropped or refused. + */ + it('keeps an option id no longer in options', () => { + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'opt_ghost' }] } + let out = '' + expect(() => { + out = render(buildPredicateClause(p, TABLE, [statusCol])) + }).not.toThrow() + expect(out).toContain('"col_status":"opt_ghost"') + }) }) }) }) @@ -1227,8 +1322,8 @@ describe('error messages name the caller-facing column, not the storage id', () ) }) - it('names the column on a containment type mismatch', () => { + it('has no message to name on a containment type mismatch — it does not throw', () => { const p: TablePredicate = { all: [{ field: 'col_abc123', op: 'eq', value: 'seven' }] } - expectNamed(() => buildPredicateClause(p, TABLE, [num]), 'overall_score', 'col_abc123') + expect(() => buildPredicateClause(p, TABLE, [num])).not.toThrow() }) }) diff --git a/apps/sim/lib/table/sql.ts b/apps/sim/lib/table/sql.ts index 11218532130..4d5c6c9ab04 100644 --- a/apps/sim/lib/table/sql.ts +++ b/apps/sim/lib/table/sql.ts @@ -404,48 +404,54 @@ function validateComparisonValue( */ const CONTAINMENT_OPS = new Set(['eq', 'ne', 'in', 'nin']) -/** Renders an operand for an error message, bounded. */ -function describeOperand(value: JsonValue): string { - if (typeof value === 'string') return `string "${truncate(value, 64)}"` - return `${typeof value} ${truncate(JSON.stringify(value) ?? String(value), 64)}` -} +/** + * Column types whose containment operand is left byte-exact. + * + * `select` — its operands are option **names**, already resolved to stored ids + * upstream by `resolvePredicateSelectValues` / `resolveFilterSelectValues`, and + * its `coerce` returns an array for a multi-select: the wrong shape for a + * membership clause. + * + * `date` — its `coerce` is **not idempotent**. `normalizeDateCellValue` rebuilds + * the string without a fractional part, so `"2024-01-31T10:00:00.000Z"` — the + * form the write path stores — comes back as `"2024-01-31T10:00:00Z"` and no + * longer matches the stored bytes. `fieldPredicate` is not only used for + * user-facing filters: it also compiles the unique-constraint probes + * (`checkUniqueConstraintsDb`, `checkBatchUniqueConstraintsDb`) and the upsert + * conflict probe, whose operands were **already** coerced by `coerceRowToSchema` + * earlier in the same request. Re-coercing them there would make a unique `date` + * probe stop matching, letting a duplicate row through inside the write + * transaction with no error. Fixing the stored date format is a far larger + * change than a read-path alignment should carry. + */ +const CONTAINMENT_COERCION_EXCLUDED_TYPES = new Set(['select', 'date']) /** * Reads an equality/membership operand the way the **write path** reads a cell, - * so `eq` compares like against like. + * so `eq` compares like against like — best-effort, never fatal. * * The column type's own `coerce` is the single definition of "what this column * can hold": a write of `"8"` to a number column stores `8`, so a filter for - * `"8"` must look for `8` or it reports zero rows for a row that exists. When - * `coerce` refuses, the operand is one the column could never hold — the range - * operators already answer that with a descriptive 400 rather than an empty - * result set, and this is the same answer for the containment operators. + * `"8"` must look for `8` or it reports zero rows for a row that exists. + * + * When `coerce` refuses, the ORIGINAL operand is passed through unchanged and + * the clause compiles exactly as it always did — matching nothing, since JSONB + * containment is exact. Refusing loudly is not an option here: the v2 predicate + * grammar is not operand-type-checked at the boundary (leaf `value` is + * `z.unknown()`), so a throw would land not at submission but inside the + * background runners that compile the same predicate later — a filter-scoped + * cancel that can no longer compile would leave those cells uncancellable. * * `null` and `''` are passed through untouched. Neither is a typed operand: * `null` is a real containment query for a JSON-null cell, and `''` is the - * cleared-cell sentinel the grid writes. Coercing or rejecting either would - * change what an existing caller's filter means rather than fix it. - * - * `select` is excluded: its operands are option **names**, already resolved to - * stored ids upstream by `resolvePredicateSelectValues` / - * `resolveFilterSelectValues`, and its `coerce` returns an array for a - * multi-select — the wrong shape for a membership clause. + * cleared-cell sentinel the grid writes. Coercing either would change what an + * existing caller's filter means rather than fix it. `select` and `date` are + * excluded wholesale — see `CONTAINMENT_COERCION_EXCLUDED_TYPES`. */ -function coerceContainmentOperand( - label: string, - column: ColumnDefinition, - op: FilterOp, - value: JsonValue -): JsonValue { +function coerceContainmentOperand(column: ColumnDefinition, value: JsonValue): JsonValue { if (value === null || value === '') return value const result = columnTypeOf(column).coerce(value, column) - if (!result.ok) { - throw new TableQueryValidationError( - `Operator "${op}" on column "${label}" (${column.type}) requires a ${column.type} value, got ${describeOperand(value)}.`, - 'INVALID_FILTER' - ) - } - return result.value + return result.ok ? (result.value as JsonValue) : value } /** @@ -613,17 +619,23 @@ export function fieldPredicate( } } - // Equality/membership compiles to exact JSONB containment, so a wrongly-typed - // operand is not a narrower match — it is no match at all, reported as an - // empty 200. Read the operand through the column type first, exactly as a - // write would; `coerceContainmentOperand` throws when the column could never - // hold it. Skipped for a field with no schema entry (ad-hoc legacy keys), - // which has no declared type to read it with. + // Equality/membership compiles to exact JSONB containment, so an operand of + // the wrong JS type is not a narrower match — it is no match at all, reported + // as an empty 200 while the row it meant exists. Read the operand through the + // column type first, exactly as a write would. Best-effort only: an operand + // the type refuses passes through unchanged and the clause compiles as it + // always did. Skipped for a field with no schema entry (ad-hoc legacy keys), + // which has no declared type to read it with, and for the types in + // `CONTAINMENT_COERCION_EXCLUDED_TYPES`. + const coercesContainment = + column !== undefined && + !CONTAINMENT_COERCION_EXCLUDED_TYPES.has(column.type) && + CONTAINMENT_OPS.has(op) const containmentValue: JsonValue | undefined = - column && !isSelect && CONTAINMENT_OPS.has(op) + coercesContainment && column ? Array.isArray(value) - ? value.map((v) => coerceContainmentOperand(label, column, op, v as JsonValue)) - : coerceContainmentOperand(label, column, op, value as JsonValue) + ? value.map((v) => coerceContainmentOperand(column, v as JsonValue)) + : coerceContainmentOperand(column, value as JsonValue) : value switch (op) { diff --git a/apps/sim/lib/workflows/executor/execution-queries.test.ts b/apps/sim/lib/workflows/executor/execution-queries.test.ts index 0b01c5b6f15..52750473ef4 100644 --- a/apps/sim/lib/workflows/executor/execution-queries.test.ts +++ b/apps/sim/lib/workflows/executor/execution-queries.test.ts @@ -1,7 +1,7 @@ /** * @vitest-environment node */ -import { queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing/mocks' +import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing/mocks' import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockGetJob, mockGetJobQueue } = vi.hoisted(() => ({ @@ -63,12 +63,87 @@ describe('resolveWorkflowExecutionOwnership', () => { ).resolves.toMatchObject({ workflowGroupWorkspaceId: null }) }) + /** + * A cancel has to tell a live run apart from one that had already finished, + * and the row it would ask for is the row this query already reads. + */ + it('projects the durable status from the same log row it already reads', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, [ + { workflowId: 'workflow-1', status: 'completed' }, + ]) + + await expect( + resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + ).resolves.toMatchObject({ belongsToWorkflow: true, priorStatus: 'completed' }) + }) + + /** + * The row-queue mock returns whatever was queued for a table regardless of the + * predicate, so every other test here passes with the `WHERE` deleted. Execution + * ids are globally unique but nothing in the mock enforces that a lookup keyed + * on the wrong column — or on nothing — would fail, and this resolver is what + * every mutating caller trusts to say which workflow an execution belongs to. + */ + it('keys both durable reads on the execution id', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, [{ workflowId: 'workflow-1' }]) + + await resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + + expect(dbChainMockFns.from).toHaveBeenNthCalledWith(1, schemaMock.workflowExecutionLogs) + expect(dbChainMockFns.from).toHaveBeenNthCalledWith(2, schemaMock.pausedExecutions) + expect(dbChainMockFns.where).toHaveBeenNthCalledWith(1, { + type: 'eq', + left: schemaMock.workflowExecutionLogs.executionId, + right: 'execution-1', + }) + expect(dbChainMockFns.where).toHaveBeenNthCalledWith(2, { + type: 'eq', + left: schemaMock.pausedExecutions.executionId, + right: 'execution-1', + }) + }) + + /** + * A run that paused before its log row landed — or whose log row is gone — is + * still durable, and the paused row's workflow id is the only thing standing + * between it and the queue fallback, which would answer `false` for a run the + * queue no longer holds. Nothing else here queues a `pausedExecutions` row, so + * dropping it from the ownership decision is otherwise invisible. + */ + it('resolves ownership from a paused-only execution', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, []) + queueTableRows(schemaMock.pausedExecutions, [{ workflowId: 'workflow-1' }]) + + await expect( + resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + ).resolves.toMatchObject({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: null, + }) + expect(mockGetJobQueue).not.toHaveBeenCalled() + }) + + it('rejects a paused-only execution bound to another workflow', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, []) + queueTableRows(schemaMock.pausedExecutions, [{ workflowId: 'workflow-2' }]) + + await expect( + resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + ).resolves.toMatchObject({ belongsToWorkflow: false }) + expect(mockGetJobQueue).not.toHaveBeenCalled() + }) + it('checks deterministic queue metadata before the durable log exists', async () => { mockGetJob.mockResolvedValue({ metadata: { workflowId: 'workflow-1' } }) await expect( resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') - ).resolves.toMatchObject({ belongsToWorkflow: true, workflowGroupWorkspaceId: null }) + ).resolves.toMatchObject({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: null, + }) expect(mockGetJob).toHaveBeenCalledWith('workflow-execution:execution-1') }) }) diff --git a/apps/sim/lib/workflows/executor/execution-queries.ts b/apps/sim/lib/workflows/executor/execution-queries.ts index 9c221871f79..6c150192eae 100644 --- a/apps/sim/lib/workflows/executor/execution-queries.ts +++ b/apps/sim/lib/workflows/executor/execution-queries.ts @@ -104,6 +104,11 @@ export interface WorkflowExecutionOwnership { * that has no log row yet, and a paused-only run. */ workflowGroupWorkspaceId: string | null + /** + * Status the durable log row already carried. `null` when there is no log row + * — a queue-only run, or a paused-only run. + */ + priorStatus: string | null } /** @@ -112,10 +117,12 @@ export interface WorkflowExecutionOwnership { * operating on an execution id because execution ids are globally unique, not * nested DB keys under a workflow. * - * The workflow-group origin rides along on the same log row the ownership check - * already reads. A group run owns a table cell sidecar, so cancelling only the - * workflow log would leave the cell stuck as running — and resolving that from a - * second SELECT of the identical row would double the read on every cancel. + * The workflow-group origin and the row's current status both ride along on the + * same log row the ownership check already reads. A group run owns a table cell + * sidecar, so cancelling only the workflow log would leave the cell stuck as + * running; and a cancel has to tell a live run apart from one that had already + * finished. Resolving either from a second SELECT of the identical row would + * double the read on every cancel. */ export async function resolveWorkflowExecutionOwnership( executionId: string, @@ -126,6 +133,7 @@ export async function resolveWorkflowExecutionOwnership( .select({ workflowId: workflowExecutionLogs.workflowId, workspaceId: workflowExecutionLogs.workspaceId, + status: workflowExecutionLogs.status, executionOrigin: workflowExecutionOriginSql(), }) .from(workflowExecutionLogs) @@ -149,6 +157,7 @@ export async function resolveWorkflowExecutionOwnership( return { belongsToWorkflow: durableWorkflowIds.every((value) => value === workflowId), workflowGroupWorkspaceId, + priorStatus: logRow?.status ?? null, } } @@ -157,5 +166,6 @@ export async function resolveWorkflowExecutionOwnership( return { belongsToWorkflow: job?.metadata.workflowId === workflowId, workflowGroupWorkspaceId: null, + priorStatus: null, } } diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts index 0d842abe1a7..aaf109c3cc0 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts @@ -12,6 +12,7 @@ import { import { beforeEach, describe, expect, it, vi } from 'vitest' import { createTimeoutAbortController, getExecutionDeadlineAt } from '@/lib/core/execution-limits' import { abortManualExecution } from '@/lib/execution/manual-cancellation' +import { terminalExecutionLogFields } from '@/lib/logs/execution/cancellation' const { mockReleaseExecutionSlot, mockReplaceLargeValueReferenceKeysWithClient } = vi.hoisted( () => ({ @@ -1567,23 +1568,6 @@ describe('PauseResumeManager terminal resume failure', () => { expect(logUpdate.executionDeadlineAt).toBeNull() expect(JSON.stringify(logUpdate.totalDurationMs)).toContain(logUpdate.endedAt.toISOString()) }) - - /** - * A resume that fails before the row flips back to `running` leaves it - * `pending` with the duration it banked at the checkpoint. Elapsed wall clock - * would redefine that to include the time the run sat waiting. - */ - it('leaves a still-paused run its checkpoint duration', async () => { - queueTableRows(workflowExecutionLogs, [{ status: 'pending' }]) - queueTableRows(pausedExecutions, [{ status: 'paused' }]) - - await markResumeFailed() - - const logUpdate = dbChainMockFns.set.mock.calls.at(-1)?.[0] as { - totalDurationMs: { toSQL: () => { sql: string } } - } - expect(logUpdate.totalDurationMs.toSQL().sql).toContain("= 'pending'") - }) }) describe('PauseResumeManager completed resume transitions', () => { @@ -1592,8 +1576,13 @@ describe('PauseResumeManager completed resume transitions', () => { resetDbChainMock() }) - it('clears the active attempt deadline when sibling pause points remain', async () => { - queueTableRows(workflowExecutionLogs, [{ status: 'running' }]) + interface MockSqlFragment { + values: unknown[] + toSQL: () => { sql: string } + } + + async function markPartialResumeCompleted(logStatus: string): Promise { + queueTableRows(workflowExecutionLogs, [{ status: logStatus }]) queueTableRows(pausedExecutions, [{ status: 'paused' }]) queueTableRows(resumeQueue, [{ status: 'claimed' }]) queueTableRows(pausedExecutions, [{ remaining: 1 }]) @@ -1610,16 +1599,101 @@ describe('PauseResumeManager completed resume transitions', () => { parentExecutionId: 'execution-1', contextId: 'context-1', }) + } - expect(dbChainMockFns.set).toHaveBeenNthCalledWith(3, { - status: 'pending', - executionDeadlineAt: null, - }) + /** The revival is the third write: resume queue, paused execution, then the log. */ + function revivalPayload(): { + status: string + executionDeadlineAt: Date | null + endedAt: MockSqlFragment + totalDurationMs: MockSqlFragment + } { + return dbChainMockFns.set.mock.calls.at(-1)?.[0] + } + + it('clears the active attempt deadline when sibling pause points remain', async () => { + await markPartialResumeCompleted('running') + + expect(dbChainMockFns.set).toHaveBeenCalledTimes(3) + const revival = revivalPayload() + expect(revival.status).toBe('pending') + expect(revival.executionDeadlineAt).toBeNull() expect(dbChainMockFns.from).toHaveBeenNthCalledWith(1, workflowExecutionLogs) expect(dbChainMockFns.from).toHaveBeenNthCalledWith(2, pausedExecutions) expect(dbChainMockFns.from).toHaveBeenNthCalledWith(3, resumeQueue) }) + /** + * The revival claim excludes only `cancelled`, so it also matches a row + * `markResumeFailed` already ended: one context's resume fails, a sibling + * context resumes successfully afterwards, and the run goes live again. It + * must not go live still carrying the end timestamp and duration of the + * attempt that failed — a run waiting on its remaining pause points has not + * ended, and reporting that it has puts it in the `minDurationMs`/ + * `maxDurationMs` filters on `GET /api/v2/logs` with a duration measured at + * something other than its own end. + */ + it('clears the terminal stamp when a partial resume revives a force-failed row', async () => { + await markPartialResumeCompleted('failed') + + const revival = revivalPayload() + expect(Object.keys(revival).sort()).toEqual([ + 'endedAt', + 'executionDeadlineAt', + 'status', + 'totalDurationMs', + ]) + expect(revival.status).toBe('pending') + expect(revival.endedAt.toSQL().sql).toContain("IN ('failed', 'completed') THEN NULL") + expect(revival.totalDurationMs.toSQL().sql).toContain("IN ('failed', 'completed') THEN NULL") + expect(revival.endedAt.values[0]).toBe(workflowExecutionLogs.status) + expect(revival.totalDurationMs.values[0]).toBe(workflowExecutionLogs.status) + }) + + /** + * The opposite case, and the reason the clear is conditional rather than + * unconditional: a row revived from a non-terminal status carries the + * checkpoint `completeWithPause` banked, which is the active duration + * `elapsedDurationMsSql` deliberately preserves for a `pending` row. Nulling + * that would redefine a later terminal duration to include the time the run + * sat waiting. + */ + it('keeps the checkpoint a still-live row banked at its pause', async () => { + await markPartialResumeCompleted('running') + + const revival = revivalPayload() + expect(revival.endedAt.toSQL().sql).toContain('ELSE ?') + expect(revival.totalDurationMs.toSQL().sql).toContain('ELSE ?') + expect(revival.endedAt.values.at(-1)).toBe(workflowExecutionLogs.endedAt) + expect(revival.totalDurationMs.values.at(-1)).toBe(workflowExecutionLogs.totalDurationMs) + }) + + /** + * The compounding half. `elapsedDurationMsSql` preserves a `pending` row's + * `total_duration_ms` and recomputes otherwise, so whatever the revival leaves + * behind is what the next terminal write — a cancel, say — records as the run's + * duration. Leaving the failed resume's frozen value there would freeze the + * cancel at it; leaving `NULL` is what makes the `COALESCE` fall through to the + * elapsed computation. + * + * The link is asserted as a composition rather than executed: the repository + * has no in-memory Postgres, and under the drizzle mock a fragment renders its + * interpolations as `?` with the bound columns on `values`. + */ + it('lets the next terminal write recompute rather than preserve the failed resume duration', async () => { + await markPartialResumeCompleted('failed') + + const revived = revivalPayload().totalDurationMs + expect(revived.toSQL().sql).toContain("IN ('failed', 'completed') THEN NULL") + + const nextTerminalWrite = terminalExecutionLogFields( + 'cancelled', + new Date('2026-08-14T12:00:00.000Z') + ).totalDurationMs as unknown as MockSqlFragment + expect(nextTerminalWrite.toSQL().sql).toContain("= 'pending' THEN ?") + expect(nextTerminalWrite.values).toContain(workflowExecutionLogs.totalDurationMs) + }) + it('fails a claimed resume after cancellation wins the log lock', async () => { queueTableRows(workflowExecutionLogs, [{ status: 'cancelled' }]) queueTableRows(pausedExecutions, [{ status: 'cancelled' }]) diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts index 278be0cf9f3..d6c51041c17 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts @@ -252,6 +252,32 @@ function clearAutomaticResumeWaitingMetadataSql(contextId: string): SQL { END` } +/** + * The terminal columns a `workflow_execution_logs` row keeps when a partial + * resume moves it back to `pending`. + * + * The revival claim excludes only `cancelled`, so it also matches a row + * `markResumeFailed` already ended: one context's resume fails, a sibling + * context resumes successfully afterwards, and the run becomes live again + * carrying the end timestamp and duration of the attempt that failed. A live row + * must not carry a terminal stamp — and because `elapsedDurationMsSql` preserves + * a `pending` row's `total_duration_ms` as its pause checkpoint, leaving it + * there also hands the next terminal write a duration frozen at the failed + * resume rather than one it recomputes. + * + * A row revived from a non-terminal status is the opposite case: its columns are + * the checkpoint `completeWithPause` banked, which is precisely what that + * preservation rule exists to keep, so they survive untouched. The row's own + * status decides, read — like every expression in the same `SET` — against the + * pre-update row. + */ +const revivedExecutionLogStamp = { + endedAt: sql`CASE WHEN ${workflowExecutionLogs.status} IN ('failed', 'completed') THEN NULL ELSE ${workflowExecutionLogs.endedAt} END`, + totalDurationMs: sql< + number | null + >`CASE WHEN ${workflowExecutionLogs.status} IN ('failed', 'completed') THEN NULL ELSE ${workflowExecutionLogs.totalDurationMs} END`, +} + function withoutAutomaticResumeWaitingReason( point: Record ): Record { @@ -2107,7 +2133,7 @@ export class PauseResumeManager { } else { await tx .update(workflowExecutionLogs) - .set({ status: 'pending', executionDeadlineAt: null }) + .set({ status: 'pending', executionDeadlineAt: null, ...revivedExecutionLogStamp }) .where( and( eq(workflowExecutionLogs.executionId, targetParentExecutionId), From c4e6abb8013bda03ec7b9fccdb239cc9ed93ee43 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 14 Aug 2026 13:31:04 -0700 Subject: [PATCH 3/4] fix(execution): report a workflow-group cancellation as the write it performed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cancelling a workflow-group run whose log had already been cancelled, but whose cell sidecar still needed reconciliation, durably cancelled that sidecar and then reported `already_cancelled` with `durablyRecorded: false` — because the terminal-status shortcut answered from the entry snapshot alone and never asked what this request had written. The analytics event, which now gates on that field, stopped firing for a cancellation that really happened. The outcome a cancel reports is the same question whichever path answers it, so there is now one vocabulary for it rather than one the direct claim tracked and one the group transition did not. Every group result maps to that outcome through a total map, so a new group result cannot compile without deciding what it wrote, and the reclassification leads with whether this request wrote at all. A group transition that reports itself already cancelled is deliberately mapped as unknown rather than as a no-op: it leaves the sidecar alone but still terminalizes a log that was active, and the result does not say which happened. That costs nothing today, because the only snapshot that would reclassify proves the log was already terminal. --- .../cancel-workflow-execution.test.ts | 103 ++++++++++++++++++ .../execution/cancel-workflow-execution.ts | 76 +++++++++---- 2 files changed, 155 insertions(+), 24 deletions(-) diff --git a/apps/sim/lib/execution/cancel-workflow-execution.test.ts b/apps/sim/lib/execution/cancel-workflow-execution.test.ts index bc3af8748e5..d39833b9ca6 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.test.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.test.ts @@ -319,6 +319,109 @@ describe('cancelWorkflowExecution', () => { expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('execution-1') }) + /** + * A workflow-group log that is already `cancelled` can still own a cell + * sidecar left in `error`, and reconciling it to `cancelled` is a durable + * write this request performed. The terminal entry snapshot cannot see that + * work, so it must not reinterpret the outcome as a no-op — the API would + * otherwise tell the caller nothing changed and drop the cancellation event. + */ + it('reports a durable write when a cancelled group run still had its sidecar reconciled', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + const cancelled = { + kind: 'cancelled' as const, + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + } + mockCancelWorkflowGroupExecution.mockResolvedValue(cancelled) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockPublishWorkflowGroupCancellationEvent).toHaveBeenCalledWith(cancelled, 'execution-1') + }) + + /** + * The group path terminalizes the workflow log itself when the cell sidecar + * is already gone, so that outcome is a durable write too. + */ + it('reports a durable write when the group path cancels a run whose sidecar is gone', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ kind: 'cancelled_without_sidecar' }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockUpdateSet).not.toHaveBeenCalled() + }) + + /** + * The mirror case: a group run that was already terminal and whose sidecar was + * already `cancelled` leaves both records untouched, so it still reports the + * state it observed rather than a durable write. + */ + it('reports a group run that changed nothing as a no-op', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled', + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: true, + durablyRecorded: false, + reason: 'already_cancelled', + }) + }) + + /** + * The lost-race re-read applies to the group path as well: an entry snapshot + * can still read `running` when the sidecar-less transition finds the log + * already `cancelled` and writes nothing. + */ + it('reports a group run that lost the race to another cancel as a no-op', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled_without_sidecar', + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: true, + durablyRecorded: false, + reason: 'already_cancelled', + }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(2) + }) + it.each([ [{ kind: 'conflict' as const, status: 'completed' }, 'cannot be cancelled while completed'], [{ kind: 'not_workflow_group' as const }, 'no longer the active table execution'], diff --git a/apps/sim/lib/execution/cancel-workflow-execution.ts b/apps/sim/lib/execution/cancel-workflow-execution.ts index 527c14a6e75..c30708bd3d9 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.ts @@ -18,6 +18,7 @@ import { cancelWorkflowGroupExecution, type PublishableWorkflowGroupCancellation, publishWorkflowGroupCancellationEvent, + type WorkflowGroupExecutionCancellationResult, } from '@/lib/table/workflow-group-cancellation' import { WORKFLOW_EXECUTION_JOB_ID_PREFIX } from '@/lib/workflows/executor/execution-job-ids' import { resolveWorkflowExecutionOwnership } from '@/lib/workflows/executor/execution-queries' @@ -79,24 +80,53 @@ function toTerminalExecutionStatus( } /** - * What the direct terminal log claim did: moved the run to `cancelled` here and - * now, matched no row, or never ran — the workflow-group and paused paths write - * their terminal row elsewhere, and a failed statement knows nothing either way. + * What this request's own terminal claim did: moved the run to `cancelled` here + * and now, provably matched no row, or ran on a path that cannot tell. Every + * path that can terminalize the run — the direct log claim and the + * workflow-group transition — answers in this one vocabulary, so the report can + * ask a single question: did this request durably write? */ -type DirectTerminalWriteOutcome = 'applied' | 'no_row' | 'unknown' +type TerminalWriteOutcome = 'applied' | 'no_row' | 'unknown' + +/** + * What a returned workflow-group transition durably wrote. `cancelled` claims + * the cell sidecar and `cancelled_without_sidecar` terminalizes the workflow log + * itself, so both are writes this request performed — including the reconciling + * cancel of a sidecar left in `error` behind an already-`cancelled` log. + * `already_cancelled_without_sidecar` found the log already `cancelled` and + * touched nothing. `already_cancelled` left the sidecar alone but may still have + * terminalized an active workflow log, which the result does not distinguish, so + * it cannot claim either way. `conflict` and `not_workflow_group` never reach + * the report — both throw above — and are mapped only to keep this map total. + */ +const WORKFLOW_GROUP_TERMINAL_WRITES = { + cancelled: 'applied', + cancelled_without_sidecar: 'applied', + already_cancelled: 'unknown', + already_cancelled_without_sidecar: 'no_row', + conflict: 'unknown', + not_workflow_group: 'unknown', +} as const satisfies Record /** * Names the terminal state the cancel could not move, or `null` when it did * real work or when this path cannot tell — in which case the caller keeps the * undifferentiated report rather than guessing. * - * The status read at entry is not enough on its own: a run that finishes - * between that read and the claim leaves a stale non-terminal snapshot behind a - * cancel that wrote nothing. The claim's own row count settles that, and a - * plain post-read cannot: after a successful cancel the row reads `cancelled` - * too, so the state has to be attributed to whoever wrote it. A claim that - * moved no row against a non-terminal snapshot re-reads the row it lost the - * race to, through the same ownership query the entry read came from. + * A request that durably wrote is never a no-op, whatever the entry snapshot + * said. A run can be terminal at entry and still owe this request a real write: + * a workflow-group run whose log is already `cancelled` can carry a sidecar left + * in `error`, and reconciling it is a durable cancellation that the entry + * snapshot cannot see. + * + * The status read at entry is not enough on its own in the other direction + * either: a run that finishes between that read and the claim leaves a stale + * non-terminal snapshot behind a cancel that wrote nothing. The claim's own row + * count settles that, and a plain post-read cannot: after a successful cancel + * the row reads `cancelled` too, so the state has to be attributed to whoever + * wrote it. A claim that moved no row against a non-terminal snapshot re-reads + * the row it lost the race to, through the same ownership query the entry read + * came from. * * Purely observational — it gates no effect, and a read failure falls back to * the undifferentiated report rather than failing the cancel. @@ -105,10 +135,11 @@ async function resolveTerminalNoOpReason( executionId: string, workflowId: string, priorTerminalStatus: TerminalExecutionStatus | null, - directTerminalWrite: DirectTerminalWriteOutcome + terminalWrite: TerminalWriteOutcome ): Promise { + if (terminalWrite === 'applied') return null if (priorTerminalStatus !== null) return TERMINAL_NO_OP_REASONS[priorTerminalStatus] - if (directTerminalWrite !== 'no_row') return null + if (terminalWrite !== 'no_row') return null try { const { priorStatus } = await resolveWorkflowExecutionOwnership(executionId, workflowId) const terminalStatus = toTerminalExecutionStatus(priorStatus) @@ -441,9 +472,10 @@ export async function cancelWorkflowExecution( * The claim's row count is read back only to report it — `returning` changes * what the statement returns, never the row it writes or the rows it matches. */ - let directTerminalWrite: DirectTerminalWriteOutcome = 'unknown' - if ( - groupCancellation === null && + let terminalWrite: TerminalWriteOutcome = 'unknown' + if (groupCancellation !== null) { + terminalWrite = WORKFLOW_GROUP_TERMINAL_WRITES[groupCancellation.kind] + } else if ( (cancellation.durablyRecorded || queuedJobCancelled || locallyAborted) && !pausedCancelled ) { @@ -459,7 +491,7 @@ export async function cancelWorkflowExecution( ) ) .returning({ id: workflowExecutionLogs.id }) - directTerminalWrite = claimedRows.length > 0 ? 'applied' : 'no_row' + terminalWrite = claimedRows.length > 0 ? 'applied' : 'no_row' } catch (dbError) { logger.warn('Failed to update execution log status directly', { executionId, @@ -510,16 +542,12 @@ export async function cancelWorkflowExecution( * run — cancelled, or force-failed with paused state left behind — can still * carry real paused-HITL reconciliation work, and a genuine failure there owes * the caller the step that failed, not a no-op. So only an otherwise-clean - * `recorded` is a candidate, whatever the prior status was. + * `recorded` is a candidate, whatever the prior status was — and only when + * this request wrote nothing durable on any path. */ const terminalNoOpReason = reason === 'recorded' && !pausedCancelled - ? await resolveTerminalNoOpReason( - executionId, - workflowId, - priorTerminalStatus, - directTerminalWrite - ) + ? await resolveTerminalNoOpReason(executionId, workflowId, priorTerminalStatus, terminalWrite) : null return { From f86d73306fed5bce72f0f558ea3f3571183b4347 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 14 Aug 2026 13:48:21 -0700 Subject: [PATCH 4/4] fix(execution): have a workflow-group cancellation report the writes it made MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings landed on the same reporting logic, each a different face of one cause: the caller could not see what the group transaction had written, so it inferred. It inferred from an entry snapshot, then from the returned kind, and the remaining blind spot was the kind that covers two different transactions — a repair that terminalizes an active log, and a genuine no-op — which left a cancel that wrote nothing still claiming a durable write when it lost a race. The transaction now reports both writes it can make, each read from that statement's own returning row and recorded immediately before the throw that already depended on it, so the report cannot drift from the write. The caller derives its outcome from those rather than from the kind, and the kind is back to naming the situation instead of standing in for the work. The group path can now always answer whether it wrote. The only remaining unknown is the direct claim when its update throws or is never attempted, which genuinely has no row count to report. --- .../cancel-workflow-execution.test.ts | 102 ++++++++++++++++-- .../execution/cancel-workflow-execution.ts | 34 +++--- .../table/workflow-group-cancellation.test.ts | 17 +++ .../lib/table/workflow-group-cancellation.ts | 63 +++++++---- 4 files changed, 172 insertions(+), 44 deletions(-) diff --git a/apps/sim/lib/execution/cancel-workflow-execution.test.ts b/apps/sim/lib/execution/cancel-workflow-execution.test.ts index d39833b9ca6..1f51da65463 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.test.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.test.ts @@ -101,6 +101,16 @@ vi.mock('@/lib/workflows/executor/human-in-the-loop-manager', () => ({ import { cancelWorkflowExecution } from '@/lib/execution/cancel-workflow-execution' +/** + * The durable writes a workflow-group transition reports back. The transaction + * updates the workflow log only, the cell sidecar only, or both, so a single + * `kind` cannot answer whether this request wrote anything. + */ +const NO_WRITES = { workflowLogTerminalized: false, sidecarCancelled: false } as const +const LOG_WRITE = { workflowLogTerminalized: true, sidecarCancelled: false } as const +const SIDECAR_WRITE = { workflowLogTerminalized: false, sidecarCancelled: true } as const +const BOTH_WRITES = { workflowLogTerminalized: true, sidecarCancelled: true } as const + const INPUT = { executionId: 'execution-1', workflowId: 'workflow-1', @@ -303,6 +313,7 @@ describe('cancelWorkflowExecution', () => { tableId: 'table-1', rowId: 'row-1', groupId: 'group-1', + writes: BOTH_WRITES, } mockCancelWorkflowGroupExecution.mockResolvedValue(cancelled) @@ -337,6 +348,7 @@ describe('cancelWorkflowExecution', () => { tableId: 'table-1', rowId: 'row-1', groupId: 'group-1', + writes: SIDECAR_WRITE, } mockCancelWorkflowGroupExecution.mockResolvedValue(cancelled) @@ -356,7 +368,10 @@ describe('cancelWorkflowExecution', () => { workflowGroupWorkspaceId: 'workspace-1', priorStatus: 'running', }) - mockCancelWorkflowGroupExecution.mockResolvedValue({ kind: 'cancelled_without_sidecar' }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'cancelled_without_sidecar', + writes: LOG_WRITE, + }) const result = await cancelWorkflowExecution(INPUT) @@ -380,6 +395,71 @@ describe('cancelWorkflowExecution', () => { tableId: 'table-1', rowId: 'row-1', groupId: 'group-1', + writes: NO_WRITES, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: true, + durablyRecorded: false, + reason: 'already_cancelled', + }) + }) + + /** + * The same `already_cancelled` kind covers a transition that left the sidecar + * alone but still terminalized an active workflow log. That log write is + * durable, so the outcome must stay `recorded` and must not re-read a state + * this request itself wrote. + */ + it('reports a durable write when a group run only repaired its workflow log', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled', + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + writes: LOG_WRITE, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledOnce() + }) + + /** + * The lost race the sidecar-bearing kind used to hide: a concurrent cancel + * terminalized both records between the entry snapshot and this transaction, + * which then found the sidecar already `cancelled` and the log already + * `cancelled` and wrote nothing. A non-terminal entry snapshot cannot catch + * that, so the transition's own report of having written nothing is what + * forces the re-read — otherwise the request would claim a durable write and + * fire the v2 cancel analytics gate on a no-op. + */ + it('reports a group run that lost the race with its sidecar already cancelled as a no-op', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled', + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + writes: NO_WRITES, }) const result = await cancelWorkflowExecution(INPUT) @@ -389,6 +469,7 @@ describe('cancelWorkflowExecution', () => { durablyRecorded: false, reason: 'already_cancelled', }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(2) }) /** @@ -410,6 +491,7 @@ describe('cancelWorkflowExecution', () => { }) mockCancelWorkflowGroupExecution.mockResolvedValue({ kind: 'already_cancelled_without_sidecar', + writes: NO_WRITES, }) const result = await cancelWorkflowExecution(INPUT) @@ -423,8 +505,14 @@ describe('cancelWorkflowExecution', () => { }) it.each([ - [{ kind: 'conflict' as const, status: 'completed' }, 'cannot be cancelled while completed'], - [{ kind: 'not_workflow_group' as const }, 'no longer the active table execution'], + [ + { kind: 'conflict' as const, status: 'completed', writes: NO_WRITES }, + 'cannot be cancelled while completed', + ], + [ + { kind: 'not_workflow_group' as const, writes: NO_WRITES }, + 'no longer the active table execution', + ], ])( 'releases the reservation before reporting a refused workflow-group cell claim as a conflict', async (outcome, message) => { @@ -444,8 +532,8 @@ describe('cancelWorkflowExecution', () => { ) it.each([ - [{ kind: 'conflict' as const, status: 'completed' }], - [{ kind: 'not_workflow_group' as const }], + [{ kind: 'conflict' as const, status: 'completed', writes: NO_WRITES }], + [{ kind: 'not_workflow_group' as const, writes: NO_WRITES }], ])( 'keeps the reservation held when a refused claim follows a paused cancellation', async (outcome) => { @@ -462,8 +550,8 @@ describe('cancelWorkflowExecution', () => { ) it.each([ - [{ kind: 'conflict' as const, status: 'completed' }], - [{ kind: 'not_workflow_group' as const }], + [{ kind: 'conflict' as const, status: 'completed', writes: NO_WRITES }], + [{ kind: 'not_workflow_group' as const, writes: NO_WRITES }], ])( 'keeps the reservation held when a refused claim follows a failed cancellation', async (outcome) => { diff --git a/apps/sim/lib/execution/cancel-workflow-execution.ts b/apps/sim/lib/execution/cancel-workflow-execution.ts index c30708bd3d9..10e92517854 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.ts @@ -18,7 +18,7 @@ import { cancelWorkflowGroupExecution, type PublishableWorkflowGroupCancellation, publishWorkflowGroupCancellationEvent, - type WorkflowGroupExecutionCancellationResult, + type WorkflowGroupCancellationWrites, } from '@/lib/table/workflow-group-cancellation' import { WORKFLOW_EXECUTION_JOB_ID_PREFIX } from '@/lib/workflows/executor/execution-job-ids' import { resolveWorkflowExecutionOwnership } from '@/lib/workflows/executor/execution-queries' @@ -85,28 +85,24 @@ function toTerminalExecutionStatus( * path that can terminalize the run — the direct log claim and the * workflow-group transition — answers in this one vocabulary, so the report can * ask a single question: did this request durably write? + * + * Only the direct claim ever answers `unknown`, and only when it could not run + * or its statement failed. The workflow-group transition always knows: it + * reports the writes it performed. */ type TerminalWriteOutcome = 'applied' | 'no_row' | 'unknown' /** - * What a returned workflow-group transition durably wrote. `cancelled` claims - * the cell sidecar and `cancelled_without_sidecar` terminalizes the workflow log - * itself, so both are writes this request performed — including the reconciling - * cancel of a sidecar left in `error` behind an already-`cancelled` log. - * `already_cancelled_without_sidecar` found the log already `cancelled` and - * touched nothing. `already_cancelled` left the sidecar alone but may still have - * terminalized an active workflow log, which the result does not distinguish, so - * it cannot claim either way. `conflict` and `not_workflow_group` never reach - * the report — both throw above — and are mapped only to keep this map total. + * Reads a workflow-group transition's durability off the writes it reported + * rather than off its `kind`. Terminalizing the workflow log and cancelling the + * cell sidecar are each a durable write this request performed, and a single + * `kind` covers both a transition that did one of them and one that did + * neither: `already_cancelled` leaves a sidecar that was already `cancelled` + * alone, but may still have terminalized an active workflow log. */ -const WORKFLOW_GROUP_TERMINAL_WRITES = { - cancelled: 'applied', - cancelled_without_sidecar: 'applied', - already_cancelled: 'unknown', - already_cancelled_without_sidecar: 'no_row', - conflict: 'unknown', - not_workflow_group: 'unknown', -} as const satisfies Record +function toTerminalWriteOutcome(writes: WorkflowGroupCancellationWrites): TerminalWriteOutcome { + return writes.workflowLogTerminalized || writes.sidecarCancelled ? 'applied' : 'no_row' +} /** * Names the terminal state the cancel could not move, or `null` when it did @@ -474,7 +470,7 @@ export async function cancelWorkflowExecution( */ let terminalWrite: TerminalWriteOutcome = 'unknown' if (groupCancellation !== null) { - terminalWrite = WORKFLOW_GROUP_TERMINAL_WRITES[groupCancellation.kind] + terminalWrite = toTerminalWriteOutcome(groupCancellation.writes) } else if ( (cancellation.durablyRecorded || queuedJobCancelled || locallyAborted) && !pausedCancelled diff --git a/apps/sim/lib/table/workflow-group-cancellation.test.ts b/apps/sim/lib/table/workflow-group-cancellation.test.ts index 6514827e2ea..92e400d27e8 100644 --- a/apps/sim/lib/table/workflow-group-cancellation.test.ts +++ b/apps/sim/lib/table/workflow-group-cancellation.test.ts @@ -23,6 +23,8 @@ const OPTIONS = { executionId: 'execution-1', } +const NO_WRITES = { workflowLogTerminalized: false, sidecarCancelled: false } as const + const ACTIVE_TARGET = { tableId: 'table-1', rowId: 'row-1', @@ -64,6 +66,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: true, sidecarCancelled: true }, }) expect(dbChainMockFns.transaction).toHaveBeenCalledOnce() @@ -165,6 +168,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -178,6 +182,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -197,6 +202,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -216,6 +222,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'cancelled_without_sidecar', + writes: { workflowLogTerminalized: true, sidecarCancelled: false }, }) expect(dbChainMockFns.update).toHaveBeenCalledOnce() @@ -249,6 +256,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'already_cancelled_without_sidecar', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -269,6 +277,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -288,6 +297,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status: 'completed', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -318,6 +328,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status: 'completed', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -335,6 +346,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -353,6 +365,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: false, sidecarCancelled: true }, }) expect(dbChainMockFns.update).toHaveBeenCalledOnce() @@ -371,6 +384,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: false, sidecarCancelled: true }, }) const sidecarUpdateValues = collectConditionValues(dbChainMockFns.where.mock.calls[2]?.[0]) @@ -395,6 +409,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status: 'error', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -413,6 +428,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: true, sidecarCancelled: false }, }) expect(dbChainMockFns.update).toHaveBeenCalledOnce() @@ -429,6 +445,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status, + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() diff --git a/apps/sim/lib/table/workflow-group-cancellation.ts b/apps/sim/lib/table/workflow-group-cancellation.ts index 1231cc2b43a..fd739d3aefa 100644 --- a/apps/sim/lib/table/workflow-group-cancellation.ts +++ b/apps/sim/lib/table/workflow-group-cancellation.ts @@ -35,13 +35,28 @@ export type PublishableWorkflowGroupCancellation = | CancelledWorkflowGroupExecution | AlreadyCancelledWorkflowGroupExecution +/** + * The durable terminal writes this request's own transaction performed. Each + * flag is read off that statement's returned row, so it cannot drift from the + * write: the transaction updates the workflow log only, the cell sidecar only, + * or both, and `kind` alone collapses those cases. + */ +export interface WorkflowGroupCancellationWrites { + /** This transaction moved the workflow execution log to `cancelled`. */ + workflowLogTerminalized: boolean + /** This transaction moved the table cell sidecar to `cancelled`. */ + sidecarCancelled: boolean +} + +type WithCancellationWrites = TResult & { writes: WorkflowGroupCancellationWrites } + export type WorkflowGroupExecutionCancellationResult = - | { kind: 'not_workflow_group' } - | { kind: 'conflict'; status: string } - | { kind: 'cancelled_without_sidecar' } - | { kind: 'already_cancelled_without_sidecar' } - | CancelledWorkflowGroupExecution - | AlreadyCancelledWorkflowGroupExecution + | WithCancellationWrites<{ kind: 'not_workflow_group' }> + | WithCancellationWrites<{ kind: 'conflict'; status: string }> + | WithCancellationWrites<{ kind: 'cancelled_without_sidecar' }> + | WithCancellationWrites<{ kind: 'already_cancelled_without_sidecar' }> + | WithCancellationWrites + | WithCancellationWrites interface WorkflowGroupExecutionTarget { tableId: string @@ -90,11 +105,19 @@ function hasDurableWorkflowGroupOrigin(executionData: unknown): boolean { * * This helper only claims durable database state. Publish its `cancelled` result after * the exact execution has been signalled with `publishWorkflowGroupCancellationEvent`. + * + * Every result carries `writes`, the terminal writes this transaction actually + * performed, so a caller reporting durability never has to infer it from `kind`. */ export async function cancelWorkflowGroupExecution( options: CancelWorkflowGroupExecutionOptions ): Promise { const transition = await db.transaction(async (tx) => { + const writes: WorkflowGroupCancellationWrites = { + workflowLogTerminalized: false, + sidecarCancelled: false, + } + const workflowLog = await tx .select({ status: workflowExecutionLogs.status, @@ -134,20 +157,20 @@ export async function cancelWorkflowGroupExecution( .then((rows) => rows[0]) if (!workflowLog) { - return { result: { kind: 'conflict', status: 'no_longer_active' } as const } + return { result: { kind: 'conflict', status: 'no_longer_active' } as const, writes } } if (!target) { if (!hasDurableWorkflowGroupOrigin(workflowLog.executionData)) { - return { result: { kind: 'not_workflow_group' } as const } + return { result: { kind: 'not_workflow_group' } as const, writes } } const workflowLogActive = workflowLog.status === 'running' || workflowLog.status === 'pending' if (!workflowLogActive && workflowLog.status !== 'cancelled') { - return { result: { kind: 'conflict', status: workflowLog.status } as const } + return { result: { kind: 'conflict', status: workflowLog.status } as const, writes } } if (workflowLog.status === 'cancelled') { - return { result: { kind: 'already_cancelled_without_sidecar' } as const } + return { result: { kind: 'already_cancelled_without_sidecar' } as const, writes } } const cancelledAt = new Date() @@ -164,10 +187,11 @@ export async function cancelWorkflowGroupExecution( ) .returning({ status: workflowExecutionLogs.status }) - if (cancelledLog?.status !== 'cancelled') { + writes.workflowLogTerminalized = cancelledLog?.status === 'cancelled' + if (!writes.workflowLogTerminalized) { throw new Error('Workflow-group cancellation lost its locked workflow-log claim') } - return { result: { kind: 'cancelled_without_sidecar' } as const } + return { result: { kind: 'cancelled_without_sidecar' } as const, writes } } const workflowLogActive = workflowLog.status === 'running' || workflowLog.status === 'pending' @@ -179,10 +203,10 @@ export async function cancelWorkflowGroupExecution( const sidecarClaimable = sidecarActive || cancellationOwnedSidecarError if (!workflowLogActive && workflowLog.status !== 'cancelled') { - return { result: { kind: 'conflict', status: workflowLog.status } as const } + return { result: { kind: 'conflict', status: workflowLog.status } as const, writes } } if (!sidecarClaimable && target.status !== 'cancelled') { - return { result: { kind: 'conflict', status: target.status } as const } + return { result: { kind: 'conflict', status: target.status } as const, writes } } const now = new Date() @@ -200,7 +224,8 @@ export async function cancelWorkflowGroupExecution( ) .returning({ status: workflowExecutionLogs.status }) - if (cancelledLog?.status !== 'cancelled') { + writes.workflowLogTerminalized = cancelledLog?.status === 'cancelled' + if (!writes.workflowLogTerminalized) { throw new Error('Workflow-group cancellation lost its locked workflow-log claim') } } @@ -231,7 +256,8 @@ export async function cancelWorkflowGroupExecution( ) .returning({ status: tableRowExecutions.status }) - if (cancelledSidecar?.status !== 'cancelled') { + writes.sidecarCancelled = cancelledSidecar?.status === 'cancelled' + if (!writes.sidecarCancelled) { throw new Error('Workflow-group cancellation lost its locked table-sidecar claim') } } @@ -242,16 +268,17 @@ export async function cancelWorkflowGroupExecution( rowId: target.rowId, groupId: target.groupId, } - return { result, blockErrors: target.blockErrors } + return { result, writes, blockErrors: target.blockErrors } }) if (transition.result.kind !== 'cancelled' && transition.result.kind !== 'already_cancelled') { - return transition.result + return { ...transition.result, writes: transition.writes } } const blockErrors = normalizeBlockErrors(transition.blockErrors) return { ...transition.result, + writes: transition.writes, ...(blockErrors ? { blockErrors } : {}), } }