Uh oh!
There was an error while loading. Please reload this page.
Add FORK_JOIN task support - #227
Open
rennolaj wants to merge 1 commit into
Open
Conversation
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Objective
Add support for Conductor's static
FORK_JOINtask type. The engine already supportedFORK_JOIN_DYNAMIC(runtime-determined branches), but had no builder/model for the build-time, fixed-branchFORK_JOIN— even though the client SDK already carries the enum values andForkTasks/JoinOnfields needed for it (Client/Generated/Models.cs).Approach
Builders/ForkJoinTaskBuilder.cs—AddTask(reference, input, params branches), where each branch is anAction<ITaskSequenceBuilder<TWorkflow>>. Builds oneFORK_JOINtask (ForkTasks= each branch's built tasks, in order) plus an auto-generated companionJOINtask (JoinOn= the last task reference of each branch) — mirroring howDynamicForkJoinTaskBuilderemits a fork+join pair, and reusingSwitchTaskBuilder's pattern for collecting multiple sub-branches into one builder.Model/ForkJoinTaskModel.cs—ForkJoinInput/ForkJoinTaskModel, following the sameTaskModel<TInput, NoOutput>shape as every other operator.InvalidOperationExceptionrather than letting an empty branch reach a bare LINQSequence contains no elementsexception (an actual gap found and fixed during review)..cs+ expected.json(captured from the real serializer output, not hand-written) +csprojEmbeddedResourceentry +[Fact]s inIntegration/WorkflowBuilderTests.cs.ForkJoinTask.cs) deliberately gives one branch two tasks using synthetic, non-business-flavoredPassThroughTaskExtensions.AddTaskscalls, specifically so the test can distinguish "picks the branch's last task" from "picks its first task" forjoinOn— a single-task-per-branch fixture can't tell those apart, since first and last are the same task.README.md/SKILL.mdchanges — confirmed no prior operator-addition PR ever touches docs (they're handled in separate bulk-doc PRs), so this follows that same precedent.Modified files (8 files, +287)
src/ConductorSharp.Engine/Builders/ForkJoinTaskBuilder.cs(new)src/ConductorSharp.Engine/Model/ForkJoinTaskModel.cs(new)test/ConductorSharp.Engine.Tests/Samples/Workflows/ForkJoinTask.cs+.json(new)test/ConductorSharp.Engine.Tests/Samples/Workflows/EmptyForkJoinTask.cs(new — zero-branches case)test/ConductorSharp.Engine.Tests/Samples/Workflows/EmptyBranchForkJoinTask.cs(new — empty-branch case)test/ConductorSharp.Engine.Tests/ConductorSharp.Engine.Tests.csproj(+1EmbeddedResource)test/ConductorSharp.Engine.Tests/Integration/WorkflowBuilderTests.cs(+3[Fact]s)Checks
dotnet test test/ConductorSharp.Engine.Tests/ConductorSharp.Engine.Tests.csproj— passed, 73/73 (3 new: happy path, zero-branches exception, empty-branch exception).dotnet build ConductorSharp.sln— passed.dotnet csharpier --checkon all changed files — passed (repo's enforced pre-commit formatter).Risks
params Action<ITaskSequenceBuilder<TWorkflow>>[] branches) is a new design choice — no direct precedent existed for "one property reference → N parallel branches" prior to this PR (FORK_JOIN_DYNAMICdoesn't need one, since its branches are runtime data, not C# code).JoinOnis computed as each branch's last built task's reference name. This is standard Conductor convention, but only exercised end-to-end via generated JSON assertions here, not against a live Conductor server.DOTNET_ROLL_FORWARD=LatestMajorandHUSKY=0(to skip thehusky installpre-build hook, which also needs net8.0). Worth confirming your CI/dev environment has net6.0 so neither workaround is needed there.Review focus
ForkJoinTaskBuilder<TWorkflow>branch-collection API inForkJoinTaskBuilder.cs— isparams Action<ITaskSequenceBuilder<TWorkflow>>[] branchesthe shape you'd want, or would you prefer something else?Name/TaskReferenceNameare both set to the same prefixed string (FORK_JOIN_<ref>,JOIN_<ref>) on the two emitted tasks, matchingDynamicForkJoinTaskBuilder's existing convention for two-task-emission builders (single-task builders likeWaitTaskBuilderinstead leaveTaskReferenceNameunprefixed) — confirm this reasoning holds.Out of scope
FORK_JOIN_DYNAMIC,DO_WHILE,SWITCH, and every other existing operator — untouched.Client/Generated/Models.cs) — already had what was needed.README.md/SKILL.mddocumentation — deliberately excluded per the operator-addition precedent (see Approach).