feat(swf): add Simple Workflow Service emulation - #2257

🎉 Thanks for your first pull request to Floci!

Your CI checks need a maintainer to approve them before they run. That is GitHub's standard gate on first-time contributors, not a problem with your PR — so if the checks look like they are doing nothing, that is why. Once a maintainer approves, CI and the compatibility suite start automatically. Nothing is needed from you in the meantime.

While you wait, a couple of things that make review faster:

• Link the issue this fixes with Closes #N in the description
• Commits follow Conventional Commits (feat(s3): ..., fix(dynamodb): ...)
• Behaviour changes come with a test — see CONTRIBUTING.md

Come join us in Slack — it is the fastest way to reach maintainers if you get stuck, or want feedback on an approach before investing more time in it.

sequenceDiagram
    participant SDK as AWS SDK / CLI
    participant C as AwsJsonController
    participant H as SwfJsonHandler
    participant S as SwfService
    participant Store as StorageBackend
    participant L as Lambda Service
    SDK->>C: JSON 1.0 request
    C->>H: Dispatch SimpleWorkflowService action
    H->>S: Parsed operation request
    S->>Store: Read or mutate region-scoped SWF state
    opt ScheduleLambdaFunction
        S->>L: Invoke configured Lambda
        L-->>S: Invocation result
        S->>Store: Append terminal Lambda event
    end
    Store-->>S: Updated state
    S-->>H: Operation result
    H-->>SDK: AWS-compatible JSON response

_{Reviews (15): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile}

Thanks for the review — all three P1s were real. Fixed in 43cdb1a4, with replies on each thread.

The first two shared a root cause worth naming: mutation was serialized per execution, but several SWF invariants span an execution family. StartWorkflowExecution has to see a concurrently inserted run of the same workflowId, and a closing execution mutates its parent and its children. Taking the related execution's lock would deadlock (a closing child wants its parent; a closing parent wants its children), so the lock is now per domain — a parent and a child always share one, which covers the family without nesting.

Both were reproduced before being fixed, and each guard fails on the previous code:

The duplicate-start fix is also confirmed over the wire, not just in-process: 8 concurrent StartWorkflowExecution calls yield 1 success, 7 WorkflowExecutionAlreadyStartedFault, and 1 open run.

For the sweep interval I chose a warned fallback to 1s rather than failing fast, since taking every other service down over one misconfigured SWF setting is the worse outcome for a local emulator. Happy to switch it to a hard startup failure if you'd prefer that convention.

Also worth flagging from your summary: earlier commits on this branch fixed three wire-level divergences found by continuing to diff captured live-service traffic against the implementation — child-workflow correlation ids, the closing-decision-must-be-last rejection, and the Unknown activity, scheduledEventId = N fault. Those are folded into the description rather than left as a running commentary; the branch history has the detail per commit.

Current state: SwfIntegrationTest 33/33, SwfServiceTest 33/33, SwfTimeoutSweeperTest 2/2, SDK v2 SwfTest 9/9, full suite 9349 tests / 0 failures (the 2 errors are the pre-existing Port already bound: 8444 conflict, reproducible on unmodified ea74b979 on this host). Docker image rebuilt at the final commit and re-verified.

Thank you — both gaps were real, and I measured each against the live service before fixing it. Both are addressed in 24ed45e8.

1. Region scoping

Confirmed exactly as you described: keys were the bare name, so orders in eu-west-1 collided with us-east-1. The live-service measurement:

Keys are now region-qualified — <region>:<domain>, <region>:<domain>/<name>/<version>, <region>:<domain>/<workflowId>/<runId> — and every store scan is prefixed by region.

Two consequences worth calling out, since neither was in the report:

• The mutation lock had to move too. domainLock keyed on the bare domain would have serialized two regions against each other, and worse, it would have disagreed with the sweeper's lock, which derives its scope from the storage key. Both now resolve to the same region-qualified domain.
• Task tokens needed no separate change, because they resolve through the (now region-scoped) execution key — so a token issued in one region cannot address another's execution. There is a test for that.

2. Registration-list pagination

Also confirmed. Four details came from the captures rather than the docs, and I would have gotten two of them wrong by guessing:

• nextPageToken is absent on the final page even when that page is exactly full (6 items at size 2 → 3 pages, no token on the third).
• maximumPageSize: 0 means "no caller limit", not "empty page".
• Above 1000 is rejected, not clamped: Value '1001' at 'maximumPageSize' failed to satisfy constraint: Member must have value less than or equal to 1000.
• A corrupt token answers ValidationException: Invalid token. This one matters most — the previous helper silently restarted at offset 0, which turns a paging loop into an infinite one. The three list operations now share one paging helper with the history and execution listings.

Tests

SwfRegionPaginationTest in the compatibility suite drives two SDK clients at different regions against the same emulator, as you asked. Against the previous implementation all five fail, reproducing your report:

DomainAlreadyExists sdk-region-… (Status Code: 400)      <- the duplicate-region bug
maximumPageSize must cap the page: 5 ==> expected: <true> but was: <false>
Expected SwfException to be thrown, but nothing was thrown   <- 1001 accepted
expected: <3> but was: <4>                                  <- page size ignored

Plus wire-level integration tests and unit tests for the page-size limits. Current SWF totals: SwfIntegrationTest 40, SwfServiceTest 42, SwfLambdaIntegrationTest 3, SwfTimeoutSweeperTest 2, and SDK SwfTest 9 / SwfLambdaTest 1 / SwfRegionPaginationTest 5.

One incidental find while sweeping for unscoped keys: listExecutions had also been scanning on an unqualified prefix, so it would have leaked across regions once domains were shared. Fixed in the same pass and covered by typesAndExecutionsAreIsolatedPerRegion.

Want your agent to iterate on Greptile's feedback? Try greploops.

Thank you for the very thorough follow-up here. I re-reviewed the full change at 4cea487e, including the later commits and all of the discussion. The two items from my earlier review—regional isolation and registration-list pagination—look well addressed, and I appreciate the live-service measurements plus the SDK and signed-wire coverage you added.

I did find four remaining correctness issues that I think should be addressed before this is ready:

1. PollForDecisionTask continuation paging does not return the same task. The handler always calls service.pollForDecisionTask(...) before interpreting nextPageToken. After the first page has claimed the task, a continuation request therefore returns an empty/new task instead of the next history page for the original task. AWS explicitly says that calling PollForDecisionTask with the returned token must not return a new decision task. startAtPreviousStartedEvent also appears to be ignored.

2. A child workflow that continues as new loses its parent relationship. continueAsNew creates the successor and sets continuedExecutionRunId, but it does not carry over the parent identifiers or update the parent's childExecutions entry to the successor run. In a focused reproduction, the successor's parent was null. That leaves the parent tracking the now-closed original run, so it can stall and never receive the successor's eventual outcome or apply child policy to it.

3. An invalid decision later in a batch can still consume the task and apply earlier decisions. respondDecisionTaskCompleted appends DecisionTaskCompleted, removes the token, and applies decisions before every decision type has been validated. With [RecordMarker, NotARealDecision], the request failed, but the history still grew from 3 to 5 with both DecisionTaskCompleted and MarkerRecorded, and the original task token was no longer retryable. The batch should be validated before any state is mutated, as is already done for the “close must be last” rule.

4. The synchronous Lambda invocation still holds the domain-wide lock. This matches the current Greptile thread. A blocking invoker prevented a signal to an unrelated workflow in the same domain from completing until the Lambda was released. The invocation needs to happen outside the domain monitor while preserving an atomic state transition around the recorded result.

I reproduced all four with small focused regression probes, then removed those probes from the checkout. The existing focused SWF suite still passes all 89 tests, including the Docker-backed Lambda tests, and the current CI is green—these are gaps that the current coverage does not exercise.

This is a substantial and thoughtfully tested addition, and the response to the earlier feedback was excellent. I think it is close; these remaining state-machine and polling cases are the pieces I would want pinned down before merging.

@abanna @pgermosen — all four confirmed, all four fixed in e4efddc4, plus a simplification pass in 8fbb7165. Thank you both; @pgermosen, the independent read and the priority ordering were genuinely useful, and your note that #3 could not be fixed with a try/catch was correct and saved me starting down that path.

I measured each case against the live service before changing anything. Two came back differently from the report, so those are worth flagging up front.

#3 — batch validation (took it first, as suggested)

Confirmed exactly: [RecordMarker, NotARealDecision] grew the history 3 → 5 and burned the token. The live service rejects the batch, persists nothing, and leaves the token claimable — verified:

history before: [WorkflowExecutionStarted, DecisionTaskScheduled, DecisionTaskStarted]
[RecordMarker, NotARealDecision] -> HTTP 400 ValidationException
history after:  [WorkflowExecutionStarted, DecisionTaskScheduled, DecisionTaskStarted]
SAME token retried with a valid batch -> HTTP 200
markers recorded: ['retry-after-reject']

Fixed by extending the requireCloseIsLastDecision shape you pointed at, rather than inventing a pattern: both rules now live in one requireValidBatch that runs before any mutation, and the accepted set comes from SwfConstants.DECISION_TYPES so the check and the enum list in the fault message cannot drift. applyDecision's default arm became unreachable, so it is now an IllegalStateException marking the invariant rather than a user-facing fault.

Guard fails on the previous code with a rejected batch must not grow the history ==> expected: <3> but was: <5>.

#1 — poll continuation

Confirmed, and AWS does state it outright. One correction to the report worth having: on the live service the continuation returns a different taskToken but the same startedEventId, previousStartedEventId and workflowExecution, with the next contiguous page:

page 1: events=[1,2,3]  startedEventId=23  previousStartedEventId=18
page 2: events=[4,5,6]  startedEventId=23 (SAME)  workflowExecution identical

So "same task, next page" is the invariant; the token itself is not stable. The continuation token now names the task it belongs to, and a continuation resolves that task instead of polling again.

startAtPreviousStartedEvent also measured differently than I expected — it starts at previousStartedEventId, inclusive, not after it:

default:                        events=13  eventIds=[1..13]
startAtPreviousStartedEvent:    events=6   eventIds=[8,9,10,11,12,13]   previousStartedEventId=8

I would have written > from and been off by one event. Now implemented as >= from.

#2 — continue-as-new parent

Confirmed; the successor's parent was null. The live service keeps the link across the boundary:

successor WorkflowExecutionStarted:
  parentWorkflowExecution = {'workflowId': 'wf-parent', 'runId': '22jxFdTId...'}
  parentInitiatedEventId  = 5
  continuedExecutionRunId = 220h5FT9D...

carryParentTo moves the parent identifiers and repoints the parent's childExecutions entry, before the successor's WorkflowExecutionStarted is appended so that event names the parent as the live service does. Guard fails on the previous code with the successor of a child must keep its parent ==> expected: <wf-cn-parent> but was: <null>.

#4 — Lambda under the domain lock

I fixed this rather than deferring it — the guard was quick to write and it is the kind of thing that gets forgotten. A decision now queues the invocation, records LambdaFunctionScheduled/LambdaFunctionStarted under the lock, runs the invocation with no lock held, then re-takes the lock to record the outcome, so the transition around the result stays atomic.

The guard blocks inside the invoker and signals an unrelated execution in the same domain. On the previous code it fails with signalling an unrelated execution blocked on the Lambda invocation. If you would still rather this were a separate PR, say so and I will split it back out — but it is small and covered.

Simplification pass (8fbb7165)

Separately from the fixes, since @AndreKurait asked me to look for reducible code: four page-token codecs and three copies of the same offset/limit/token block had accumulated. Now one Page record with one encode/decode pair (the task token is just an optional field, which is all the poll needed) and one pagedList helper the history and execution listings reuse. Net 62 fewer lines in the service and handler.

That collapse also surfaced a real divergence rather than only shortening code: historyPageSize clamped an oversized maximumPageSize while the registration lists rejected it. The live service rejects it everywhere:

GetWorkflowExecutionHistory maximumPageSize=1001 -> 400  ... less than or equal to 1000
ListOpenWorkflowExecutions  maximumPageSize=1001 -> 400  ... less than or equal to 1000
PollForDecisionTask         maximumPageSize=1001 -> 400  ... less than or equal to 1000
GetWorkflowExecutionHistory maximumPageSize=-1   -> 400  ... greater than or equal to 0

One pageSize method now serves all of them, including the distinct negative-value message.

Coverage

SWF is now 45 wire + 45 unit + 3 Docker-backed Lambda + 2 sweeper locally, plus 15 in the SDK compatibility suite. Every fix above ships a guard that fails on the commit before it, with the failure message quoted rather than described.

One process note in the interest of not overclaiming: my first attempt at proving these baselines used git checkout HEAD -- src/main/... to restore after reverting, which restores the commit — it silently destroyed the four uncommitted fixes and I had to re-apply them. The fixes are committed now and the baseline script refuses to run against a dirty tree, but it is the kind of thing worth admitting rather than quietly redoing.

One more wire divergence, found by re-reading my own capture rather than reported: fb252121.

The rejection message for an unknown decisionType listed the enum members in the API model's order. The live service uses a different order:

service: [CompleteWorkflowExecution, StartTimer, RequestCancelExternalWorkflowExecution, ...
model:   [ScheduleActivityTask, RequestCancelActivityTask, CompleteWorkflowExecution, ...

I had built that string from service-2.json instead of from a response, which is exactly the mistake this PR has otherwise avoided. Sampled four live rejections to check it was not arbitrary — the service's order was identical every time, so a client comparing the message byte-for-byte would have seen a mismatch.

The guard now asserts the entire message rather than a substring, and fails on the previous order with both strings diffed in full. Verified over the wire as byte-identical to the live capture:

emulator: 1 validation error detected: Value 'NotARealDecision' at 'decisions.2.member.decisionType' faile ...
live AWS: 1 validation error detected: Value 'NotARealDecision' at 'decisions.2.member.decisionType' faile ...
BYTE-IDENTICAL: PASS

SWF is now 95 tests locally plus 15 in the SDK suite. This does not change any behaviour the four review findings covered — it is only the text of that one fault.

Both addressed in 43dc56e4 (fix), f2128943 (guard) and 1d8ee47c (merge of main).

P1: failed batches retained Lambda work

Confirmed, and the fix is structural rather than a finally. The queue was a ThreadLocal drained after the domain lock released; on a pooled request thread that outlives the request, anything thrown between queueing and draining left the work behind for whichever request reused the thread.

It is now a plain local list created per call and threaded through applyDecisions, so the drain is only reachable after the batch has been applied and stored. A request that throws leaves the list unreferenced — there is no cross-request carrier left to leak through. That also deleted drainPendingLambdas and the ThreadLocal field outright, so the class of bug is gone rather than guarded against.

I want to be precise about severity, because my first attempt at a guard passed on the pre-fix code and I nearly shipped it as proof. No decision handler throws today, and WalStorage.appendPut swallows its IOException, so the leak was not reachable through a normal AWS-shaped rejection — Close must be last is rejected before anything is queued. Reaching it needs a throw from the store between the queue and the drain.

So the guard injects exactly that: a storage backend whose put fails once, after a Lambda has been queued and the history mutated, followed by a second request on the same thread. On fb252121 it fails with the leaked invocation named:

a later request must not run a failed request's queued Lambda: [us-east-1|leaked-fn|{}]
==> expected: <true> but was: <false>

That is the reviewer's scenario reproduced end to end: an external side effect from a request that had already failed, executed by an unrelated later one.

The CI compile failure was staleness, not this branch

The Ec2Service constructor error is not from my changes — git diff --name-only across my whole branch touches zero EC2 files. My branch was 35 commits behind upstream, and two of those changed Ec2Service's constructor:

2ab795c6 fix(ec2): assign a public IP only when the subnet opts in via MapPublicIpOnLaunch (#2023)
fc74987b fix(ec2): delete key pairs by name through the storage backend (#2187)

CI builds the merge of the PR into current main, so it saw the new call site against my older constructor while a local build of my branch alone compiled fine. Merged main in — no conflicts — and it compiles clean.

SWF is 96 tests locally after the merge, all passing, and make docs-check is clean. Full suite against merged main:

Tests run: 9870, Failures: 0, Errors: 2, Skipped: 9

9870 rather than the 9380 I reported earlier because the merge brought 490 new tests with it; all of them pass. The 2 errors are the Port already bound: 8444 pair, which is this host, not the branch.

I also tidied the PR while I was in here: the compatibility table had grown into a chronological log of review rounds, so it is now grouped by theme with the self-evident rows dropped, and I deleted my own superseded per-commit verification comments. The thread should read as one reply per review round now.

@pgermosen @abanna can you take a look?

Thanks for the substantial SWF implementation and for addressing the earlier review feedback — the region scoping, pagination, decision-batch atomicity, child continuation, and Lambda lock changes are thoughtful, and the focused tests are green.

I did find a few follow-ups on the current head (1d8ee47) worth addressing before merge:

• Lambda lifecycle: SwfService.java queues Lambda work without representing it as open execution state. A batch containing ScheduleLambdaFunction followed by CompleteWorkflowExecution can close the workflow, invoke the function afterward, and leave no terminal Lambda event. This also leaves openLambdaFunctions at zero and does not enforce the Lambda timeout.

• Multi-account timeout sweep: the background sweep scans the account-aware store outside request scope, where it falls back to the default account. Expired executions in other accounts therefore remain open.

• Tag persistence key: tagging reads a region-qualified domain but writes under the bare domain name, creating an orphan key and risking lost updates with WAL storage after restart.

• List/count validation and ordering: the execution filters accept mutually exclusive filters together and allow missing required time filters. Closed listings also always sort by start time instead of close time when closeTimeFilter is used.

• Concurrent registration: domain and workflow/activity type registration check then put without synchronization; concurrent requests can all report success instead of returning the expected already-exists fault.

These should be straightforward to cover with focused regressions. Thanks again for the care in this PR — it is a strong foundation, and these state/storage edges are the main things holding it back for me.

🎉 This PR is included in version 1.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀