feat(workflow-engine): Adds pagination and optimizes db hot path(s)#18366
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces pagination support to the workflow engine's active workflows listing API endpoint. The changes replace single-response calls with paginated results including metadata ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Endpoint as EngineEndpoints
participant Repo as EngineRepository
participant DB as Database
Client->>Endpoint: GET /workflows?page=1&pageSize=25
Endpoint->>Endpoint: Apply pagination settings<br/>(clamp pageSize to max)
Endpoint->>Repo: GetActiveWorkflowsPaginated<br/>(page=1, pageSize=25)
Repo->>DB: SELECT COUNT(*) FROM workflows<br/>(with filters)
DB-->>Repo: totalCount
alt totalCount == 0
Repo-->>Endpoint: Return 204 NoContent
Endpoint-->>Client: 204 No Content
else totalCount > 0
Repo->>DB: SELECT * FROM workflows<br/>ORDER BY CreatedAt<br/>SKIP 0 TAKE 25
DB-->>Repo: workflows (page data)
Repo-->>Endpoint: (workflows, totalCount)
Endpoint->>Endpoint: Map to PaginatedResponse<br/>(data, page, pageSize,<br/>totalCount, totalPages)
Endpoint-->>Client: 200 OK + PaginatedResponse JSON
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
skip-releasenotes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/Fixtures/WorkflowTestHelper.cs (1)
13-23:⚠️ Potential issue | 🟡 MinorAvoid silently overriding
metadata.Namespacein the test helper.
CreateRequest()now returns namespace-bearing metadata, butEnqueueWorkflows()still defaultsnsto"test-namespace"and overwrites the metadata at Line 23. Any test that passes onlymetadatawill enqueue into the wrong namespace and stop exercising the intended case.Suggested fix
public static async Task<BatchEnqueueResult[]> EnqueueWorkflows( IEngineRepository repository, WorkflowRequestMetadata metadata, IReadOnlyList<WorkflowRequest> workflows, string? idempotencyKey = null, - string ns = "test-namespace", + string? ns = null, Dictionary<string, string>? labels = null ) { + ns ??= metadata.Namespace; var resolvedKey = idempotencyKey ?? (string.IsNullOrEmpty(metadata.IdempotencyKey) ? Guid.NewGuid().ToString("N") : metadata.IdempotencyKey); var resolvedMetadata = metadata with { Namespace = ns, IdempotencyKey = resolvedKey };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/Fixtures/WorkflowTestHelper.cs` around lines 13 - 23, The helper currently overwrites metadata.Namespace unconditionally; change the logic in EnqueueWorkflows so resolvedMetadata preserves an existing metadata.Namespace when present instead of always using the ns parameter. Concretely, when constructing resolvedMetadata (the spot that sets Namespace and IdempotencyKey), set Namespace to metadata.Namespace if it is non-empty, otherwise fall back to the ns parameter, and still apply the resolvedKey to IdempotencyKey; this keeps CreateRequest-provided namespaces intact while preserving existing idempotency handling (look for the resolvedKey and resolvedMetadata variables in the method).src/Runtime/workflow-engine/docs/technical-guide.md (1)
361-369:⚠️ Potential issue | 🟡 MinorDocumentation example inconsistent with implementation.
The request body example still shows
idempotencyKeyandcorrelationIdas JSON body fields, but according to the PR changes and HTTP snapshots, these are now passed via query parameters (?idempotencyKey=...&correlationId=...) or headers rather than in the request body.Consider updating the example to reflect the new API contract:
📝 Suggested documentation update
-POST /api/v1/{namespace}/workflows +POST /api/v1/{namespace}/workflows?idempotencyKey=process-next-abc123&correlationId=a1b2c3d4-e5f6-7890-abcd-ef1234567890And remove
idempotencyKeyandcorrelationIdfrom the JSON body example, or add a note explaining these are extracted from query parameters/headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/docs/technical-guide.md` around lines 361 - 369, The JSON example in the technical-guide shows idempotencyKey and correlationId as body fields (see the example block containing "idempotencyKey" and "correlationId" and the sample id "process-next-abc123"), but the implementation extracts those values from query parameters or headers; update the example to remove idempotencyKey and correlationId from the JSON payload (leave the "labels" object/other body fields intact) and either show the request URL with ?idempotencyKey=...&correlationId=... or add a short note stating that idempotencyKey and correlationId are provided via query parameters or headers as per the new API contract.src/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cs (1)
131-154:⚠️ Potential issue | 🟠 MajorKeep workflow reads namespace-aware as well.
A workflow created via
Enqueue(..., ns: otherNs)can no longer be fetched or awaited with the same client instance, because these reads always go to_defaultNamespace. That turns valid non-default-namespace workflows into 404s and polling time-outs.🔧 Possible fix
-public Task<HttpResponseMessage> GetWorkflowRaw(Guid workflowId) => - _client.GetAsync($"{GetBasePath()}/{workflowId}", CancellationToken.None); +public Task<HttpResponseMessage> GetWorkflowRaw(Guid workflowId, string? ns = null) => + _client.GetAsync($"{GetBasePath(ns)}/{workflowId}", CancellationToken.None); -public async Task<WorkflowStatusResponse?> GetWorkflow(Guid workflowId) +public async Task<WorkflowStatusResponse?> GetWorkflow(Guid workflowId, string? ns = null) { - using var response = await GetWorkflowRaw(workflowId); + using var response = await GetWorkflowRaw(workflowId, ns);Then thread the same optional
nsthroughWaitForWorkflowStatus(...), otherwise polling still stays pinned to the default namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cs` around lines 131 - 154, GetWorkflow/GetWorkflowRaw always call GetBasePath() which uses the default namespace, causing reads of workflows created with Enqueue(..., ns: otherNs) to 404; make namespace-aware reads by adding an optional ns parameter to GetWorkflowRaw and GetWorkflow (and thread that ns through any polling helper like WaitForWorkflowStatus), update GetBasePath usage to include the ns when provided (e.g., GetBasePath(ns)), and ensure callers (including tests that call GetWorkflow/WaitForWorkflowStatus) pass the same ns used in Enqueue so polling and direct fetches target the correct namespace.
🧹 Nitpick comments (8)
src/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Integration/AppCommandIntegrationTests.cs (1)
159-173: Assert the callback side effect once as well.This only proves deduplicated creation. A regression could still execute the same workflow twice and this test would stay green. Given the test name, I'd also wait for completion and assert a single WireMock hit.
Suggested test hardening
var response1 = await _client.Enqueue(request, idempotencyKey: "idempotent-test-key"); var response2 = await _client.Enqueue(request, idempotencyKey: "idempotent-test-key"); // Both should return the same workflow ID - Assert.Equal(response1.Workflows.Single().DatabaseId, response2.Workflows.Single().DatabaseId); + var workflowId = response1.Workflows.Single().DatabaseId; + Assert.Equal(workflowId, response2.Workflows.Single().DatabaseId); + await _client.WaitForWorkflowStatus(workflowId, PersistentItemStatus.Completed); + Assert.Single(fixture.WireMock.LogEntries);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Integration/AppCommandIntegrationTests.cs` around lines 159 - 173, The test AppCommand_IdempotentResubmit_DoesNotDuplicate only asserts deduplicated creation but not that the workflow executed once; after enqueuing (using _client.Enqueue and inspecting response1.Workflows.Single().DatabaseId) wait for the workflow to complete (use the existing test helper that polls by DatabaseId or implement a short retry loop against the workflow status) and then assert the external callback/mock was hit exactly once (assert the WireMock/mock server hit count == 1) to ensure the workflow side effect ran only once.src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/query.js (1)
76-79: Avoid silently dropping namespace filters when more than one value exists.If
values.size > 1, the current branch emits no namespace constraint at all. Consider a fallback so filter intent is preserved.Defensive fallback
- if (key === 'namespace') { - // Send as dedicated namespace param when a single namespace is selected - if (values.size === 1) params.set('namespace', [...values][0]); + if (key === 'namespace') { + // Dedicated param supports one namespace; preserve filtering intent for multi-value sets + if (values.size === 1) { + params.set('namespace', [...values][0]); + } else { + for (const v of values) labelPairs.push(`namespace:${v}`); + } } else if (key === 'correlationId') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/query.js` around lines 76 - 79, The current namespace branch only sets params.set('namespace', ...) when values.size === 1 and drops the filter for multi-value sets; update the branch in query.js (the block checking if (key === 'namespace')) to preserve intent by falling back for values.size > 1 — e.g., serialize multiple namespaces into the query (use params.set('namespace', Array.from(values).join(',')) or alternatively append each value with params.append('namespace', v)) while keeping the existing single-value behavior so callers that read 'namespace' still receive a clear constraint.src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/index.html (1)
39-47: Consider adding basic ARIA wiring for the new namespace dropdown.A small a11y improvement: connect the toggle/panel and give the search box an explicit accessible name.
Possible markup improvement
- <button class="dropdown-toggle" onclick="toggleDropdown('ns-dropdown')"> + <button class="dropdown-toggle" onclick="toggleDropdown('ns-dropdown')" aria-controls="ns-panel" aria-expanded="false"> @@ - <div class="dropdown-panel"> - <input class="dropdown-search" type="text" placeholder="Search..." autocomplete="off" spellcheck="false"> + <div class="dropdown-panel" id="ns-panel"> + <input class="dropdown-search" type="text" placeholder="Search..." aria-label="Search namespaces" autocomplete="off" spellcheck="false">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/index.html` around lines 39 - 47, Add basic ARIA wiring for the namespace dropdown: give the panel element the id referenced by the toggle (use id="ns-dropdown" on the div.dropdown-panel), add aria-controls="ns-dropdown", aria-expanded (update true/false when toggleDropdown runs) and aria-haspopup="listbox" to the button.dropdown-toggle, give the search input.dropdown-search an explicit accessible name (e.g., aria-label="Search namespaces"), and mark the list container (div#ns-list) with role="listbox" and ensure the selected span (id="ns-selected") is announced (e.g., aria-live="polite" or aria-atomic) so screen readers know selection changes; update toggleDropdown to flip aria-expanded accordingly.src/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.WebhookCommand_FullHttpChatter_DocumentsExchange.http (1)
77-77: Clarify: Double-slash in step idempotency keys is intentional?The step idempotency keys show
chatter-idem-key//step-1andchatter-idem-key//step-2(double slash). This appears to result from the operationId being/step-1(with leading slash), combined with the format{workflowIdempotencyKey}/{stepOperationId}.If this is intentional, consider adding a brief comment in the snapshot or test explaining the double-slash pattern to avoid future confusion.
Also applies to: 93-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.WebhookCommand_FullHttpChatter_DocumentsExchange.http` at line 77, The snapshot shows an Idempotency-Key value with a double slash ("chatter-idem-key//step-1") caused by concatenating workflowIdempotencyKey and stepOperationId where stepOperationId comes from an operationId with a leading slash (e.g., "/step-1"); either normalize the step id before concatenation by trimming any leading slashes (so the code that builds the header removes leading '/' from stepOperationId/operationId), or if the double-slash is intentional, add a short explanatory comment in the test/snapshot near the Idempotency-Key to document that "{workflowIdempotencyKey}/{stepOperationId}" can produce a double-slash when operationId starts with '/'.src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/state-modal.js (1)
72-76: JSDoc missingwfNamespaceparameter documentation.The function signature was updated to include
wfNamespace, but the JSDoc comment only documentswfId.📝 Proposed fix
/** * Fetch and render the state trail. * `@param` {string} wfId - workflow databaseId + * `@param` {string} wfNamespace - workflow namespace */ const fetchAndRender = async (wfId, wfNamespace) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/state-modal.js` around lines 72 - 76, The JSDoc for fetchAndRender is missing documentation for the new wfNamespace parameter; update the comment block above the function (fetchAndRender) to add a `@param` entry for wfNamespace describing its purpose (e.g., workflow namespace or context) alongside the existing `@param` for wfId so the signature and documentation match.src/Runtime/workflow-engine/.k6/lib/helpers.js (1)
130-135: Spin-wait sleep implementation is CPU-intensive.The
sleepMsfunction uses a busy-wait loop which consumes CPU cycles unnecessarily. While k6 lacks a native millisecond sleep, consider usingsleep()from k6 with fractional seconds.🔧 Alternative using k6's sleep
+import { sleep } from 'k6'; -function sleepMs(ms) { - const end = Date.now() + ms; - while (Date.now() < end) { - // spin — k6 doesn't have a millisecond sleep - } -} +function sleepMs(ms) { + sleep(ms / 1000); +}k6's
sleep()accepts fractional seconds (e.g.,sleep(0.5)for 500ms), which is more efficient than spinning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/.k6/lib/helpers.js` around lines 130 - 135, The current sleepMs function uses a CPU-intensive busy-wait loop; replace its implementation to call k6's sleep with fractional seconds instead of spinning (update the sleepMs function to convert milliseconds to seconds and call sleep(ms/1000)). Locate the sleepMs function in this file and ensure the k6 sleep import is present where needed so callers of sleepMs continue to work with millisecond input converted to seconds.src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/modal.js (1)
447-454: Consider clearing_openWfNamespaceon modal close.
_openWfIdand_openStepKeyare cleared but_openWfNamespaceis not. This creates inconsistency and could lead to stale namespace values if the modal is reopened.🔧 Suggested fix
window.closeModal = () => { _openWfId = ''; + _openWfNamespace = ''; _openStepKey = ''; clearTimeout(_refreshTimer); dom.modal.classList.remove('open');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/modal.js` around lines 447 - 454, The modal close handler (window.closeModal) clears _openWfId and _openStepKey but omits _openWfNamespace, leaving a stale namespace; update the window.closeModal implementation to also reset _openWfNamespace (alongside _openWfId and _openStepKey), and ensure any associated state cleanup (e.g., clearTimeout(_refreshTimer), hiding dom.modalTabs/dom.modalSubtabs) remains intact so the namespace cannot persist when the modal is reopened.src/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/DashboardEndpoints.cs (1)
579-588: Inconsistent variable naming:nsProp2vsnsProp.The variable is named
nsProp2but there's nonsPropin this handler's scope (unlike the retry handler). This appears to be copy-paste residue.🔧 Rename for consistency
if ( - !doc.RootElement.TryGetProperty("namespace", out var nsProp2) - || nsProp2.ValueKind != JsonValueKind.String - || string.IsNullOrWhiteSpace(nsProp2.GetString()) + !doc.RootElement.TryGetProperty("namespace", out var nsProp) + || nsProp.ValueKind != JsonValueKind.String + || string.IsNullOrWhiteSpace(nsProp.GetString()) ) { return Results.BadRequest("Missing namespace"); } - string ns = nsProp2.GetString() ?? throw new UnreachableException(); + string ns = nsProp.GetString() ?? throw new UnreachableException();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/DashboardEndpoints.cs` around lines 579 - 588, The handler contains a leftover variable name nsProp2 (used when reading the "namespace" property) which is inconsistent with other handlers that use nsProp; rename nsProp2 to nsProp and update its usages (including the subsequent GetString() call that assigns string ns) so the variable naming matches the rest of the code and removes the copy-paste residue in the JSON property parsing block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Fixtures/AppTestHelpers.cs`:
- Around line 89-93: The test helper assigns a nullable parameter using the
null-forgiving operator to the required non-nullable property
AppWorkflowContext.LockToken; change the helper method signature to make the
lockToken parameter non-nullable (remove the '?' from its type) and stop using
the null-forgiving operator so you can assign LockToken = lockToken directly, or
alternatively add an explicit guard (e.g., throw ArgumentNullException if
lockToken is null) before assigning to AppWorkflowContext.LockToken to satisfy
the nullability contract.
In `@src/Runtime/workflow-engine/docs/presentation-technical.md`:
- Around line 266-267: Update the documentation for the list endpoint GET
/api/v1/{namespace}/workflows to explicitly describe the new pagination
contract: document the request query parameters page and pageSize (including
defaults and limits if any) and show the paginated response envelope shape with
keys { data, page, pageSize, totalCount, totalPages }; also give a small example
request and response snippet illustrating a filtered, paginated call and a
response containing the data array and those pagination fields so consumers know
how to page through results.
In `@src/Runtime/workflow-engine/README.md`:
- Line 57: Update the documentation for the GET /api/v1/{namespace}/workflows
endpoint to reflect the new pagination contract: state that the endpoint accepts
optional query parameters ?page and ?pageSize, and that responses return a
paginated envelope with the fields data (array of workflows), page, pageSize,
totalCount, and totalPages; update the description text where GET
/api/v1/{namespace}/workflows is listed and add a short example or sentence
showing the query params and the envelope field names (data, page, pageSize,
totalCount, totalPages).
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/filters.js`:
- Around line 119-148: The event listener is attached to dom.nsList without
checking it exists, which throws if the element is not in the DOM; guard the
attachment by first checking dom.nsList (e.g., if (!dom.nsList) return or wrap
the listener registration in if (dom.nsList) { ... }) before calling
dom.nsList.addEventListener, then keep the existing handler logic (uses
state.namespaceFilter, syncNamespaceToLabelFilter, rebuildNamespaceDropdown)
unchanged.
In `@src/Runtime/workflow-engine/src/WorkflowEngine.Models/PaginatedResponse.cs`:
- Around line 25-38: The TotalPages getter trusts PageSize and can divide by
zero or negative values; validate PageSize on assignment to ensure it's
positive. Update the PaginatedResponse model so PageSize (the required int
PageSize { get; init; }) enforces >0 (throw ArgumentOutOfRangeException or
similar during init) or replace the auto-init with a validated init/backing
field, and keep TotalPages as (int)Math.Ceiling((double)TotalCount / PageSize)
assuming PageSize is guaranteed positive.
In `@src/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cs`:
- Around line 221-224: ListActiveWorkflows currently returns only the first page
from ListActiveWorkflowsPaginated (dropping subsequent pages). Update
EngineApiClient.ListActiveWorkflows to aggregate all pages: call
ListActiveWorkflowsPaginated to get the first page, append its Data, then
iterate using the paginated response's next-page mechanism (e.g., next token /
page fetch or a provided Pages/HasMore iterator) to fetch and append Data from
each subsequent page until no more pages remain; return the full concatenated
List<WorkflowStatusResponse>. If first-page-only behavior was intended, rename
the method to make that explicit.
---
Outside diff comments:
In `@src/Runtime/workflow-engine/docs/technical-guide.md`:
- Around line 361-369: The JSON example in the technical-guide shows
idempotencyKey and correlationId as body fields (see the example block
containing "idempotencyKey" and "correlationId" and the sample id
"process-next-abc123"), but the implementation extracts those values from query
parameters or headers; update the example to remove idempotencyKey and
correlationId from the JSON payload (leave the "labels" object/other body fields
intact) and either show the request URL with
?idempotencyKey=...&correlationId=... or add a short note stating that
idempotencyKey and correlationId are provided via query parameters or headers as
per the new API contract.
In
`@src/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/Fixtures/WorkflowTestHelper.cs`:
- Around line 13-23: The helper currently overwrites metadata.Namespace
unconditionally; change the logic in EnqueueWorkflows so resolvedMetadata
preserves an existing metadata.Namespace when present instead of always using
the ns parameter. Concretely, when constructing resolvedMetadata (the spot that
sets Namespace and IdempotencyKey), set Namespace to metadata.Namespace if it is
non-empty, otherwise fall back to the ns parameter, and still apply the
resolvedKey to IdempotencyKey; this keeps CreateRequest-provided namespaces
intact while preserving existing idempotency handling (look for the resolvedKey
and resolvedMetadata variables in the method).
In `@src/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cs`:
- Around line 131-154: GetWorkflow/GetWorkflowRaw always call GetBasePath()
which uses the default namespace, causing reads of workflows created with
Enqueue(..., ns: otherNs) to 404; make namespace-aware reads by adding an
optional ns parameter to GetWorkflowRaw and GetWorkflow (and thread that ns
through any polling helper like WaitForWorkflowStatus), update GetBasePath usage
to include the ns when provided (e.g., GetBasePath(ns)), and ensure callers
(including tests that call GetWorkflow/WaitForWorkflowStatus) pass the same ns
used in Enqueue so polling and direct fetches target the correct namespace.
---
Nitpick comments:
In
`@src/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Integration/AppCommandIntegrationTests.cs`:
- Around line 159-173: The test AppCommand_IdempotentResubmit_DoesNotDuplicate
only asserts deduplicated creation but not that the workflow executed once;
after enqueuing (using _client.Enqueue and inspecting
response1.Workflows.Single().DatabaseId) wait for the workflow to complete (use
the existing test helper that polls by DatabaseId or implement a short retry
loop against the workflow status) and then assert the external callback/mock was
hit exactly once (assert the WireMock/mock server hit count == 1) to ensure the
workflow side effect ran only once.
In `@src/Runtime/workflow-engine/.k6/lib/helpers.js`:
- Around line 130-135: The current sleepMs function uses a CPU-intensive
busy-wait loop; replace its implementation to call k6's sleep with fractional
seconds instead of spinning (update the sleepMs function to convert milliseconds
to seconds and call sleep(ms/1000)). Locate the sleepMs function in this file
and ensure the k6 sleep import is present where needed so callers of sleepMs
continue to work with millisecond input converted to seconds.
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/DashboardEndpoints.cs`:
- Around line 579-588: The handler contains a leftover variable name nsProp2
(used when reading the "namespace" property) which is inconsistent with other
handlers that use nsProp; rename nsProp2 to nsProp and update its usages
(including the subsequent GetString() call that assigns string ns) so the
variable naming matches the rest of the code and removes the copy-paste residue
in the JSON property parsing block.
In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/index.html`:
- Around line 39-47: Add basic ARIA wiring for the namespace dropdown: give the
panel element the id referenced by the toggle (use id="ns-dropdown" on the
div.dropdown-panel), add aria-controls="ns-dropdown", aria-expanded (update
true/false when toggleDropdown runs) and aria-haspopup="listbox" to the
button.dropdown-toggle, give the search input.dropdown-search an explicit
accessible name (e.g., aria-label="Search namespaces"), and mark the list
container (div#ns-list) with role="listbox" and ensure the selected span
(id="ns-selected") is announced (e.g., aria-live="polite" or aria-atomic) so
screen readers know selection changes; update toggleDropdown to flip
aria-expanded accordingly.
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/modal.js`:
- Around line 447-454: The modal close handler (window.closeModal) clears
_openWfId and _openStepKey but omits _openWfNamespace, leaving a stale
namespace; update the window.closeModal implementation to also reset
_openWfNamespace (alongside _openWfId and _openStepKey), and ensure any
associated state cleanup (e.g., clearTimeout(_refreshTimer), hiding
dom.modalTabs/dom.modalSubtabs) remains intact so the namespace cannot persist
when the modal is reopened.
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/query.js`:
- Around line 76-79: The current namespace branch only sets
params.set('namespace', ...) when values.size === 1 and drops the filter for
multi-value sets; update the branch in query.js (the block checking if (key ===
'namespace')) to preserve intent by falling back for values.size > 1 — e.g.,
serialize multiple namespaces into the query (use params.set('namespace',
Array.from(values).join(',')) or alternatively append each value with
params.append('namespace', v)) while keeping the existing single-value behavior
so callers that read 'namespace' still receive a clear constraint.
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/state-modal.js`:
- Around line 72-76: The JSDoc for fetchAndRender is missing documentation for
the new wfNamespace parameter; update the comment block above the function
(fetchAndRender) to add a `@param` entry for wfNamespace describing its purpose
(e.g., workflow namespace or context) alongside the existing `@param` for wfId so
the signature and documentation match.
In
`@src/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.WebhookCommand_FullHttpChatter_DocumentsExchange.http`:
- Line 77: The snapshot shows an Idempotency-Key value with a double slash
("chatter-idem-key//step-1") caused by concatenating workflowIdempotencyKey and
stepOperationId where stepOperationId comes from an operationId with a leading
slash (e.g., "/step-1"); either normalize the step id before concatenation by
trimming any leading slashes (so the code that builds the header removes leading
'/' from stepOperationId/operationId), or if the double-slash is intentional,
add a short explanatory comment in the test/snapshot near the Idempotency-Key to
document that "{workflowIdempotencyKey}/{stepOperationId}" can produce a
double-slash when operationId starts with '/'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 711ea517-ab7d-4a8f-be37-255ec7f43ffb
📒 Files selected for processing (80)
src/Runtime/workflow-engine-app/.k6/README.mdsrc/Runtime/workflow-engine-app/.k6/constant-rate.jssrc/Runtime/workflow-engine-app/.k6/continuous-process-next.jssrc/Runtime/workflow-engine-app/.k6/payloads/process-next.jsonsrc/Runtime/workflow-engine-app/.k6/stress-test.jssrc/Runtime/workflow-engine-app/Makefilesrc/Runtime/workflow-engine-app/src/WorkflowEngine.App/Commands/AppCommand/AppCommand.cssrc/Runtime/workflow-engine-app/src/WorkflowEngine.App/appsettings.Docker.jsonsrc/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/.snapshots/AppCommandIntegrationTests.AppCommand_FullHttpChatter_DocumentsExchange.httpsrc/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Fixtures/AppTestHelpers.cssrc/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Integration/AppCommandIntegrationTests.HttpChatter.cssrc/Runtime/workflow-engine-app/tests/WorkflowEngine.App.Tests/Integration/AppCommandIntegrationTests.cssrc/Runtime/workflow-engine/.k6/README.mdsrc/Runtime/workflow-engine/.k6/constant-rate.jssrc/Runtime/workflow-engine/.k6/lib/helpers.jssrc/Runtime/workflow-engine/.k6/payloads/webhook.jsonsrc/Runtime/workflow-engine/.k6/stress-test.jssrc/Runtime/workflow-engine/AGENTS.mdsrc/Runtime/workflow-engine/Makefilesrc/Runtime/workflow-engine/README.mdsrc/Runtime/workflow-engine/docs/presentation-technical.mdsrc/Runtime/workflow-engine/docs/technical-guide.mdsrc/Runtime/workflow-engine/src/WorkflowEngine.Commands/Extensions/OutboundHeaderExtensions.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Commands/Webhook/WebhookCommand.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/DashboardEndpoints.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/EngineEndpoints.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Engine.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Extensions/WorkflowEngineBuilderExtensions.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Metadata/InboundMetadata.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Metadata/MetadataExtractor.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Metadata/WorkflowMetadataOperationTransformer.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/WorkflowWriteBuffer.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/index.htmlsrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/core/state.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/filters.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/modal.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/query.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/state-modal.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/url.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/shared/cards.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/shared/dropdown.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/shared/pipeline.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Constants/WorkflowNamespace.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Extensions/WorkflowRequestExtensions.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Writes.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/IEngineRepository.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/PaginatedResponse.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/StepStatusResponse.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/WorkflowEnqueueRequest.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/WorkflowMetadataConstants.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/WorkflowRequestMetadata.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/WorkflowStatusResponse.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/WorkflowWriteBufferTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_GetWorkflow_AfterRetries_ShowsRetryState.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_GetWorkflow_CompletedWebhook_ReturnsFullDetailsShape.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_GetWorkflow_MultipleSteps_ReturnsAllSteps.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_GetWorkflow_WithDependencies_ShowsDependencyStatus.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_GetWorkflow_WithStepLabels_ReturnsLabelsInShape.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_ListActiveWorkflows_WhileProcessing_ReturnsWorkflowsShape.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.WebhookCommand_FullHttpChatter_DocumentsExchange.httpsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/DashboardEndpointTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/DashboardRetryTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/EngineCancellationTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/EngineGracefulShutdownTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/EngineResumeTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/EngineTests.HttpChatter.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/EngineTests.Responses.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/EngineTests.Validation.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/TelemetryTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/Fixtures/WorkflowTestHelper.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/QueryPlanTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/WorkflowCrudTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/WorkflowQueryTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineAppFixture.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/HttpChatterHelpers.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/HttpExchangeRecorder.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/TestHelpers.cs
💤 Files with no reviewable changes (3)
- src/Runtime/workflow-engine/.k6/payloads/webhook.json
- src/Runtime/workflow-engine-app/.k6/payloads/process-next.json
- src/Runtime/workflow-engine/src/WorkflowEngine.Models/WorkflowEnqueueRequest.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs (1)
21-36: Use non-default pagination values in these tests.These assertions still pass if the handler keeps hardcoded
25/100, because_defaultSettingsinherits the model defaults. Please overridePaginationwith non-default values in the test options and assert against those, so the configurable behaviour added in this PR is actually exercised.Also applies to: 333-352, 427-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs` around lines 21 - 36, The test options created in _defaultSettings use EngineSettings but don't override the Pagination property, so tests can pass with hardcoded defaults; update the EngineSettings instance used in _defaultSettings to set a non-default Pagination value (e.g., specify Pagination = new PaginationSettings { PageSize = X, PageLimit = Y } or whatever the project uses) and then update assertions in the tests that reference pagination behavior to assert against these non-default values; modify all similar test option blocks mentioned (lines around 333-352 and 427-507) to set explicit, non-default Pagination on the EngineSettings so the configurable pagination behavior is actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/EngineEndpoints.cs`:
- Around line 142-149: The effectivePage value can still be so large that
(page-1)*pageSize overflows in repository.GetActiveWorkflowsPaginated; clamp
effectivePage to a safe maximum before the call (e.g., compute safeMaxPage =
Math.Max(1, int.MaxValue / effectivePageSize) and set effectivePage =
Math.Min(effectivePage, safeMaxPage)) or add the same validation inside
GetActiveWorkflowsPaginated so that the Skip offset cannot overflow; update the
code paths that call effectivePage (the variables effectivePage,
effectivePageSize and the call to repository.GetActiveWorkflowsPaginated)
accordingly.
---
Nitpick comments:
In
`@src/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs`:
- Around line 21-36: The test options created in _defaultSettings use
EngineSettings but don't override the Pagination property, so tests can pass
with hardcoded defaults; update the EngineSettings instance used in
_defaultSettings to set a non-default Pagination value (e.g., specify Pagination
= new PaginationSettings { PageSize = X, PageLimit = Y } or whatever the project
uses) and then update assertions in the tests that reference pagination behavior
to assert against these non-default values; modify all similar test option
blocks mentioned (lines around 333-352 and 427-507) to set explicit, non-default
Pagination on the EngineSettings so the configurable pagination behavior is
actually exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4780488c-a305-45e0-be04-51ae9b99ed3c
📒 Files selected for processing (3)
src/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/EngineEndpoints.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/EngineSettings.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cs (2)
567-570: Consider defensive validation for pagination parameters.If
page < 1is passed,Skip((page - 1) * pageSize)produces a negative offset, which throwsArgumentOutOfRangeExceptionin EF Core. While the endpoint likely validates these parameters, adding a guard clause here would make the repository more defensive.💡 Suggested defensive guard
try { logger.FetchingWorkflows("active (paginated)"); + + if (page < 1) + throw new ArgumentOutOfRangeException(nameof(page), "Page must be >= 1"); + if (pageSize < 1) + throw new ArgumentOutOfRangeException(nameof(pageSize), "PageSize must be >= 1"); await using var context = await dbContextFactory.CreateDbContextAsync(cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cs` around lines 567 - 570, The repository method using baseQuery with OrderBy(...).Skip((page - 1) * pageSize).Take(pageSize) should defensively validate pagination inputs: ensure page is at least 1 and pageSize is positive before computing Skip/Take (e.g., clamp page to 1 or throw an ArgumentException), so update the method around the baseQuery/Skip/Take usage (referencing baseQuery, page, pageSize, OrderBy, Skip, Take, and the workflows variable) to validate or normalize those parameters and avoid passing a negative skip value into EF Core.
149-176: Missing success logging for consistency with other methods.Other read methods in this file (e.g.,
GetScheduledWorkflows,GetDistinctLabelValues) log success vialogger.SuccessfullyFetchedWorkflows(...)after completing the query. This method lacks that logging, which creates an observability gap.💡 Suggested fix
await using var dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); - return await dbContext + var result = await dbContext .Workflows.Select(w => w.Namespace) .Distinct() .OrderBy(n => n) .ToListAsync(cancellationToken); + + logger.SuccessfullyFetchedWorkflows(result.Count); + + return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cs` around lines 149 - 176, GetDistinctNamespaces currently returns the distinct namespace list but does not emit the success metric/log like other reads; after the ToListAsync call and before returning, call logger.SuccessfullyFetchedWorkflows with an appropriate argument (e.g., "namespaces" or the resulting list count) and ensure activity remains consistent (use activity?.SetTag or leave as-is), so add the logger.SuccessfullyFetchedWorkflows(...) invocation in the GetDistinctNamespaces method right after obtaining the result and before the return, mirroring how GetScheduledWorkflows/GetDistinctLabelValues do it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cs`:
- Around line 567-570: The repository method using baseQuery with
OrderBy(...).Skip((page - 1) * pageSize).Take(pageSize) should defensively
validate pagination inputs: ensure page is at least 1 and pageSize is positive
before computing Skip/Take (e.g., clamp page to 1 or throw an
ArgumentException), so update the method around the baseQuery/Skip/Take usage
(referencing baseQuery, page, pageSize, OrderBy, Skip, Take, and the workflows
variable) to validate or normalize those parameters and avoid passing a negative
skip value into EF Core.
- Around line 149-176: GetDistinctNamespaces currently returns the distinct
namespace list but does not emit the success metric/log like other reads; after
the ToListAsync call and before returning, call
logger.SuccessfullyFetchedWorkflows with an appropriate argument (e.g.,
"namespaces" or the resulting list count) and ensure activity remains consistent
(use activity?.SetTag or leave as-is), so add the
logger.SuccessfullyFetchedWorkflows(...) invocation in the GetDistinctNamespaces
method right after obtaining the result and before the return, mirroring how
GetScheduledWorkflows/GetDistinctLabelValues do it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbf5613c-e6dd-4868-97c8-9aa4dea7441c
📒 Files selected for processing (1)
src/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cs
The GET /api/v1/{namespace}/workflows endpoint now supports offset-based
pagination via ?page and ?pageSize query params, returning a PaginatedResponse
envelope with data, page, pageSize, totalCount, and totalPages. This prevents
unbounded responses during high-volume scenarios (e.g. k6 stress tests with
10k workflows) and gives frontends the metadata needed for page controls.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…neSettings Adds PaginationSettings (DefaultPageSize, MaxPageSize) to EngineSettings so hosts can tune pagination limits through configuration rather than relying on hardcoded values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… on empty count EF Core strips Include() for aggregate queries, so the count is already lightweight. Add early return when totalCount is 0 to skip the data query entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r, add pagination repo tests - ListActiveWorkflows now iterates through all pages instead of returning only the first page, matching the "full list" contract callers expect - Add 5 repository-level pagination tests verifying total count, complete non-overlapping iteration, empty results, out-of-range pages, and terminal workflow exclusion - Update docs (README, AGENTS, presentation) to reflect paginated endpoint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
danielskovli
force-pushed
the
feature/workflow-engine-pagination
branch
from
3209fbe to
3f07aee
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs (1)
21-36: Consider explicitly settingPaginationin test settings for clarity.The
_defaultSettingsfixture doesn't explicitly set thePaginationproperty, yet tests assert specific values likePageSize == 25andMaxPageSize == 100. While this works ifEngineSettings.Paginationhas default initialisers, explicitly setting the pagination configuration would make the test assumptions clearer and more resilient to future changes in default values.💡 Suggested improvement
private static readonly IOptions<EngineSettings> _defaultSettings = Options.Create( new EngineSettings { MaxWorkflowsPerRequest = 100, MaxStepsPerWorkflow = 50, MaxLabels = 10, MetricsCollectionInterval = TimeSpan.FromSeconds(10), DefaultStepCommandTimeout = TimeSpan.FromSeconds(30), DefaultStepRetryStrategy = new() { MaxDelay = TimeSpan.FromMinutes(5) }, DatabaseCommandTimeout = TimeSpan.FromSeconds(30), DatabaseRetryStrategy = new() { MaxDelay = TimeSpan.FromMinutes(1) }, HeartbeatInterval = TimeSpan.FromSeconds(10), StaleWorkflowThreshold = TimeSpan.FromMinutes(1), MaxReclaimCount = 3, + Pagination = new() { DefaultPageSize = 25, MaxPageSize = 100 }, } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs` around lines 21 - 36, The test fixture _defaultSettings should explicitly set the Pagination property on the EngineSettings instance so assertions about PageSize and MaxPageSize are clear and robust; update the new EngineSettings initializer to include Pagination = new PaginationSettings { PageSize = 25, MaxPageSize = 100 } (or whatever values the tests assert) so the tests no longer rely on EngineSettings.Pagination defaults when referencing PageSize and MaxPageSize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cs`:
- Around line 21-36: The test fixture _defaultSettings should explicitly set the
Pagination property on the EngineSettings instance so assertions about PageSize
and MaxPageSize are clear and robust; update the new EngineSettings initializer
to include Pagination = new PaginationSettings { PageSize = 25, MaxPageSize =
100 } (or whatever values the tests assert) so the tests no longer rely on
EngineSettings.Pagination defaults when referencing PageSize and MaxPageSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ab523a5-bc3d-44fb-903e-59232352b640
📒 Files selected for processing (14)
src/Runtime/workflow-engine/.k6/lib/helpers.jssrc/Runtime/workflow-engine/AGENTS.mdsrc/Runtime/workflow-engine/README.mdsrc/Runtime/workflow-engine/docs/presentation-technical.mdsrc/Runtime/workflow-engine/src/WorkflowEngine.Core/Endpoints/EngineEndpoints.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/EngineRepository.Reads.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/IEngineRepository.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/EngineSettings.cssrc/Runtime/workflow-engine/src/WorkflowEngine.Models/PaginatedResponse.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Core.Tests/Endpoints/EngineEndpointTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_ListActiveWorkflows_WhileProcessing_ReturnsWorkflowsShape.verified.txtsrc/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/TelemetryTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/WorkflowQueryTests.cssrc/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cs
✅ Files skipped from review due to trivial changes (5)
- src/Runtime/workflow-engine/README.md
- src/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/TelemetryTests.cs
- src/Runtime/workflow-engine/docs/presentation-technical.md
- src/Runtime/workflow-engine/tests/WorkflowEngine.Integration.Tests/.snapshots/EngineTests.Response_ListActiveWorkflows_WhileProcessing_ReturnsWorkflowsShape.verified.txt
- src/Runtime/workflow-engine/src/WorkflowEngine.Models/PaginatedResponse.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Runtime/workflow-engine/.k6/lib/helpers.js
- src/Runtime/workflow-engine/AGENTS.md
- src/Runtime/workflow-engine/src/WorkflowEngine.Models/EngineSettings.cs
- src/Runtime/workflow-engine/tests/WorkflowEngine.Repository.Tests/WorkflowQueryTests.cs
- src/Runtime/workflow-engine/tests/WorkflowEngine.TestKit/EngineApiClient.cs
- src/Runtime/workflow-engine/src/WorkflowEngine.Data/Repository/IEngineRepository.cs
bjorntore
approved these changes
Apr 8, 2026
danielskovli
commented
Apr 8, 2026
Piggyback heartbeats onto status writes to eliminate deadlocks between HeartbeatService and WorkflowUpdateBuffer. Simplify update buffer to single-flush with batch deduplication and fire-and-forget step-started updates. Make EF Core/Npgsql instrumentation opt-in. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
danielskovli
changed the title
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- SubmitAndForget now logs + emits a metric when TryWrite fails due to a full channel - Clarify dedup safety invariant in WorkflowUpdateBuffer comment - Fix two stale ExecuteAsync references in technical-guide.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…irectly specifying updated steps
danielskovli
temporarily deployed
to
runtime_tt_ring1
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
data,nextCursor, and optionaltotalCountPagination
?pageSize=25&cursor=<guid>) onGET /api/v1/{namespace}/workflowsand related endpointsPaginationSettingsinEngineSettingscontrols default/max page sizes?pageSize=1for lightweight pollingStampede / DB contention improvements
HeartbeatAt = @nowfor Processing workflows, eliminating a deadlock vector between the HeartbeatService and UpdateBufferUpdatedAt, avoiding redundant row locks on actively-processing workflowsSubmitAndForget()for step Processing status — workers proceed to execution immediately without awaiting the DB writeEnableDatabaseInstrumentation(defaults to false)Stress test results (50k workflows, 550k steps)
Breaking change
The response shape for workflow list endpoints changes from a bare JSON array to a paginated envelope object. Consumers will need to read
.datafor the workflow list.Test plan
Submit_DeduplicatesSameWorkflowInBatch🤖 Generated with Claude Code