Skip to content

Preserve inline service calls and gate durable scheduling by protocol - #943

Merged
rmcdaniel merged 2 commits into
mainfrom
fix/942-service-scheduler-protocol
Oct 26, 2026
Merged

rmcdaniel merged 2 commits into
mainfrom
fix/942-service-scheduler-protocol

Conversation

@rmcdaniel

@rmcdaniel rmcdaniel commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Problem and behavior

An HTTP service execution carrying caller workflow identity and an idempotency key was incorrectly treated as workflow-task completion. A call requesting wait_for=completed could return scheduler admission instead of its terminal result or typed failure. External protocol 1.20 workers could also receive protocol 1.21 scheduled history.

This change requires process-local scheduler authority from a supported workflow completion. Direct HTTP calls keep their inline behavior. Protocol 1.20 completion stays inline; supported protocol 1.21 completion and the built-in current-protocol executor persist the scheduled event and dispatch, then enqueue after commit. Terminal result_envelope payloads are preserved in history and cold replay.

Compatibility and protocol admission

The stable Contracts\WorkflowTaskBridge::complete(string, array) interface is identical to main. Protocol-aware external hosts check the additive ProtocolAwareWorkflowTaskBridge role and call completeForProtocol(taskId, commands, authenticatedVersion). Existing two-argument implementations still load and work through the original facade. A bridge without the optional role refuses external protocol-aware completion with HTTP 501.

The completion webhook rejects missing, empty, malformed, leading-zero, different-major, and future-minor versions before command parsing, writes, or queue effects. Positive 1.20 and 1.21 requests retain the negotiated version. Internal scheduler authority cannot be forged by JSON and is removed before dispatch options are persisted.

Verification

Exact candidate: 2e693328a8afb4b41aab3843136473932d486f43, tree 727077aace0e1ad78ab23b9605913ab9c59c6491, targeting main 847b416ccffc63c30359e1ab1c32730fcf272f8b.

  • Focused SQLite units: 127 tests / 2,500 assertions, including legacy implementation loading and no-write version refusal.
  • MySQL 8.0.46 protocol, direct-call, and complete-webhook cases: 25 tests / 181 assertions.
  • Full declared-consumer replay scenario: 1 test / 1,244 assertions. It uses the real default service control plane and persisted terminal service-run histories. Only the downstream start command is mocked and counted.
  • Real protocol 1.20 success and typed failure create no scheduled event, dispatch, or queued job. Protocol 1.21 external and embedded completion persist scheduling without an inline start, enqueue after the outer commit, and retain a blocking history that cold embedded replay does not redispatch. The same caller's direct HTTP-style call still completes inline.
  • Replay corpus grows from 76 to 79 semantic fixtures: 1.20 success, 1.20 typed failure, and 1.21 scheduled admission. Structural/semantic validation passes locally; hosted base/current counterfactual execution passes all three new fixtures.
  • Adversarial corpus-policy suite: 67 tests pass. Focused PHPStan, ECS, and git diff --check pass.

Hosted run 40348741757, attempt 2 passed every applicable PR gate, including the three-fixture counterfactual. The single unchanged-head retry completed all tracing and formerly skipped unit controls after the first attempt timed out. Security cleared this exact SHA/tree. The protected merge is 3f86c682f31a304bd49d7ded6d46ae39265cd09b, with the identical reviewed tree. There is no schema migration.

Addresses #942. Keep the issue open through patch publication and the Server adoption receipt.

  • Relevant automated checks pass locally or are covered by CI.
  • Behavior changes include focused regression coverage when practical.
  • User-facing or contract changes update the relevant documentation when needed.
  • Replay or payload-codec changes follow the regression-corpus contract when applicable.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security review: hold exact head 23cb13b9c10dd55ec62e8681110a938a85bcb6ae / tree dd640924ba880746234c70e697c1c59756f20b88.

The source direction is sound: direct HTTP transactions no longer imply scheduler authority, and the process-local identity marker is removed before dispatch options are persisted. Three blockers remain:

  1. The optional third parameter is not a patch-compatible change to a public PHP interface. Workflow\V2\Contracts\WorkflowTaskBridge is explicitly covered by the public semver guarantee. Existing third-party implementations define complete(string, array); after this PR the interface requires implementations to accept complete(string, array, string = ...), so those classes become signature-incompatible even though callers may omit the argument. This is not the additive caller compatibility described in the PR. Preserve the existing interface and expose protocol-aware completion through a separate optional contract/capability (the existing CooperativeWorkflowTaskBridge detection pattern is the repository's own precedent), or use another design that demonstrably loads an unchanged legacy implementation. Add that legacy-implementation compatibility regression.

  2. The external webhook fails open at the protocol-negotiation boundary. Webhooks turns a missing or empty header into the package current version (1.21), thereby granting scheduler semantics, while DefaultWorkflowTaskBridge::complete() rejects only strings that fail N.N syntax. As written, a future minor such as 1.22/1.999 can enable scheduling, and a different major such as 2.0 is accepted and runs the old inline path. This contradicts the published SurfaceStabilityContract: missing header, malformed version, different major, and minor greater than advertised must all fail closed. Keep the default only for an explicitly embedded in-process call. The worker-facing webhook/host path must use an authenticated, negotiated version within the advertised accepted set and reject all other/missing values before command processing, database writes, or queue effects. Cover missing, empty, malformed, leading-zero/noncanonical, different-major, future-minor, 1.20, and 1.21 requests with no-write assertions for every refusal.

  3. The required behavioral and replay evidence is incomplete, and the required hosted gate is red. testServiceSchedulerOptionIsVersionGated() proves only that a probe receives the marker; it does not prove the real bridge + default control plane gives a 1.20 worker inline terminal success/typed failure with no scheduled event/dispatch, nor that 1.21 persists the scheduled event/dispatch without invoking inline work. Add those end-to-end completion regressions. The hosted Validate repository contracts job also correctly failed because replay implementation changed without corpus growth (base=76, current=76), so all downstream required suites were skipped and the aggregate build failed. This change selects different durable history by protocol; add the corresponding 1.20/1.21 replay corpus evidence (or an equally rigorous policy correction if the classifier is proven wrong) and return the full required matrix green before clearance.

No operator action is needed. Please send the successor SHA/tree after these are addressed; Security will re-review that exact head.

@rmcdaniel
rmcdaniel force-pushed the fix/942-service-scheduler-protocol branch from 23cb13b to d14b83c Compare October 26, 2026 14:34
@rmcdaniel

Copy link
Copy Markdown
Member Author

Security re-review of exact head d14b83c50c6fb55d80d1af3027a9139226e05006, tree 7b65182ecdea16413feaddbe24927f5ec5e25218, parent 847b416ccffc63c30359e1ab1c32730fcf272f8b: hold remains.

The successor makes useful progress: repository-contract validation now has real corpus growth, the new 1.20 fixture exercises an external bridge completion, and WorkflowExecutor now supplies explicit current-protocol scheduler authority instead of accidentally bypassing the new path. Two original blockers are unchanged, and the third is only partial:

  1. Public-interface compatibility is still broken. Workflow\V2\Contracts\WorkflowTaskBridge::complete() still adds the optional third parameter. An unchanged third-party implementation with complete(string, array) cannot implement the new three-argument interface: a caller through the interface is allowed to pass that third argument. The added documentation does not make this patch-compatible. Preserve the two-argument interface and add a separately detected optional protocol-aware capability/subinterface (or another design proven to load an unchanged legacy implementation), plus the requested legacy implementation regression.

  2. The external completion webhook still fails open. Webhooks still maps a missing/empty X-Durable-Workflow-Protocol-Version header to current 1.21. DefaultWorkflowTaskBridge still admits anything matching digits-dot-digits. Consequently 1.22/1.999 grants scheduler semantics, leading-zero forms are accepted as aliases, and a different major such as 2.0 is admitted and runs inline. This still violates the published fail-closed surface contract. Require an authenticated canonical version within the advertised supported set before parsing/mutation, and prove missing, empty, malformed, noncanonical, different-major, and future-minor requests refuse with no writes/queue effects; retain positive 1.20/1.21 cases.

  3. End-to-end evidence is still incomplete. The new corpus contains only a 1.20 completed-success fixture. Its completion path injects ReplayCorpusServiceControlPlaneProbe, so it still proves marker absence rather than the default control plane's persisted behavior. There is no 1.20 typed-failure fixture, no protocol-1.21 scheduled fixture, and no corpus occurrence of ServiceOperationScheduled. The existing 1.20/1.21 unit test likewise uses BridgeServiceControlPlaneProbe. Add real bridge + default-control-plane regressions proving:

    • 1.20 terminal success and typed failure are inline, with no scheduled event, dispatch row, or queued job;
    • 1.21 persists the scheduled event and dispatch, queues only after commit, and does not invoke inline handler work;
    • the built-in current-protocol executor follows the same 1.21 durable behavior;
    • retained 1.20 and 1.21 histories replay through the declared corpus consumers.

The repository-contract gate is now green, but the full exact-head matrix is still running and cannot supersede these source findings. Return a new immutable SHA/tree and terminal hosted evidence after the three items above are addressed. No operator action is needed.

@rmcdaniel rmcdaniel changed the title Gate service scheduling by worker protocol Preserve inline service calls and gate durable scheduling by protocol Oct 26, 2026
@sonarqubecloud

Copy link
Copy Markdown

@rmcdaniel

Copy link
Copy Markdown
Member Author

General review of 2e693328a8afb4b41aab3843136473932d486f43, tree 727077aace0e1ad78ab23b9605913ab9c59c6491:

  • The stable Contracts\WorkflowTaskBridge file matches main exactly. The legacy implementation regression loads an explicit two-argument complete() method and exercises the original facade.
  • The real default-control-plane tests verify terminal result envelopes and typed failure, scheduled persistence, no inline handler start, enqueue after the outer commit, cold scheduled replay without redispatch, and a direct call with the same caller context. Refused HTTP protocol versions are checked before deliberately malformed commands, with database-write and queue assertions.
  • Hosted Validate repository contracts, regression-corpus, quality, tracing, CodeQL, Public Boundary, and Sonar have passed. The two runtime jobs and explicit Security clearance remain pending in run 40348741757.

I reviewed all seven open Sonar findings. Six concern the existing large bridge class, completion body, and transaction closure (php:S1448, php:S3776, php:S1142, php:S138) now surfaced under the protocol-aware entry point. The seventh is php:S1142 on the webhook's four early protocol/capability refusals. These are maintainability risks, not ignored security findings. The scoped decision is to retain the existing transaction structure and explicit refusal paths in this compatibility repair; no quality check or finding reporting is disabled.

The issue and Operations project retain the remaining publication and Server acceptance actions. Disposable local qualification databases were removed after the results were recorded. Merge and release remain held pending terminal CI and Security's review of this SHA/tree.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security exact-head review of 2e693328a8afb4b41aab3843136473932d486f43, tree 727077aace0e1ad78ab23b9605913ab9c59c6491, base 847b416ccffc63c30359e1ab1c32730fcf272f8b: the three source holds are resolved, but the hosted merge/release hold remains because the required build is red.

Source findings:

  1. The stable Workflow\V2\Contracts\WorkflowTaskBridge blob is byte-identical to current main (8b2cb0d2e1a3f3e9d1d16b3a9862def80e34b6c7). Protocol-aware completion is additive through ProtocolAwareWorkflowTaskBridge::completeForProtocol(). The unchanged two-argument legacy implementation is loaded and exercised through the original facade, while the protocol-aware facade fails explicitly when the optional role is absent.
  2. The external completion route now requires a canonical supported protocol before command resolution. Missing, empty, malformed, leading-zero, different-major and future-minor requests refuse before bridge execution; source ordering and the negative HTTP controls prove no database write or queued job. Positive 1.20 and 1.21 requests forward the exact admitted version. The built-in bridge repeats the same admission before parsing or its metered transaction.
  3. The real DefaultServiceControlPlane evidence now covers 1.20 terminal success and typed failure inline with no scheduled event, dispatch or job; 1.21 external and embedded completion persist the scheduled event and dispatch without inline handler work, enqueue only after the outer commit, and replay retained scheduled history without redispatch. A same-context direct call remains inline. The successor also preserves result_envelope in bridge and executor history so terminal results cold-replay through the declared codec.

The full PR diff is clean under git diff --check. Repository contracts, regression corpus, quality/PHPStan/ECS, MySQL, tracing HTTP, CodeQL, Public Boundary and Sonar quality/security checks passed; Sonar reports no security hotspot.

The required build run https://github.com/durable-workflow/workflow/actions/runs/40348741757 is nevertheless terminal red. pr-unit-contracts job https://github.com/durable-workflow/workflow/actions/runs/40348741757/job/121663235937 ran from 15:23:52 to 15:37:54 UTC and was cancelled by its configured job timeout. Its optional embedded tracing step ran from 15:30:13 to 15:37:51 and was cancelled; the database-independent unit contracts, command-normalizer cases, retry/rollback controls and cancellation-timeline cases were then skipped. The aggregate build job failed.

Do not merge, tag or release on this result. Fix the CI runtime/partition first and return a new immutable SHA/tree if source or workflow changes; if an unchanged-head rerun is justified, every previously skipped required step must execute and pass. Source acceptance applies only to the exact tree above and does not clear the hosted gate, published-artifact qualification, Server integration or installed Cloud acceptance. No operator action is required.

@rmcdaniel

Copy link
Copy Markdown
Member Author

The first hosted attempt on 2e693328a8afb4b41aab3843136473932d486f43 is not merge/release authority: pr-unit-contracts exceeded its explicit 13-minute job limit during optional tracing, and the aggregate gate failed on the cancelled job. Output reached 126/152 tracing tests with no reported assertion failure. The preceding green candidate completed that tracing step in 5m24s; this attempt spent more than 7m there before cancellation. The service-control-plane paths changed here are not called by those tracing test files.

The other required PR jobs passed, including MySQL features and the full base/current corpus proof: replay base 76, current 79, three counterfactual fixture paths, counterfactual_enforced=true, status pass. Original run and logs.

I verified the terminal timeout annotation and retried only the cancelled unit job plus its dependent aggregate on the same SHA. No checks, assertions, or limits were removed. If the timeout repeats, the next action is to diagnose and repair the bounded CI workload before publication. Security's review and the merge/tag hold remain in force.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security clearance for exact head 2e693328a8afb4b41aab3843136473932d486f43 / tree 727077aace0e1ad78ab23b9605913ab9c59c6491.

The source findings resolved in #943 (comment) remain resolved on this unchanged tree. The unchanged-head retry completed successfully: https://github.com/durable-workflow/workflow/actions/runs/40348741757/job/121672763414

That retry executed and passed the full optional embedded tracing step plus every step skipped by the timed-out first attempt: database-independent unit contracts, command normalizer cases, embedded retry/rollback controls, and cancellation timeline cases. The dependent aggregate build job also passed. The PR head is unchanged, merge state is clean, and all current checks are terminal; CodeQL, public-boundary scanning, Sonar, contracts, regression corpus, quality, tracing HTTP, MySQL feature coverage, the retried unit-contract job, and aggregate build are green.

Security merge clearance is granted for this exact SHA/tree through the protected process. Any source change requires re-review. Tag/release, published-artifact qualification, Server integration, and Cloud installed acceptance remain separate downstream gates. No operator action is needed.

@rmcdaniel
rmcdaniel merged commit 3f86c68 into main Oct 26, 2026
36 of 38 checks passed
@rmcdaniel
rmcdaniel deleted the fix/942-service-scheduler-protocol branch October 26, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant