Skip to content

feat(state): pass the step's context to callbacks; skip CI on drafts - #245

Open
reka18 wants to merge 9 commits into
mainfrom
feat/state-callback-context
Open

reka18 wants to merge 9 commits into
mainfrom
feat/state-callback-context

Conversation

@reka18

@reka18 reka18 commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

What this change does

Guards, actions and reducers now get the context of the step running them, so a host can tell when one of them calls back into it.

  • GuardCtx, ActionCtx and AssignCtx have a new Ctx field: the context the Fire was given. Outside a Fire, such as during PlanPath, it's context.Background(), never nil. A Fire nested inside a callback on the same instance restores the outer context when it returns.
  • The in-process bindings pass the context they're given (EvalGuard, EvalAction, EvalAssign) through to the function they wrap.
  • WithContext sets the context Cast gives the guards, actions and reducers that settle the initial state.
  • ActorSystem.RegisterContext registers a ContextActorBehavior, an actor factory that gets the context of the step spawning or restoring the actor, so it can Cast its child WithContext. A child's own callbacks already get the context its delivery carries. Register is unchanged.
  • A guard registered with BindGuard gets the Fire's context through EvalGuard, where it used to get a background one, and callbacks see the context as the middleware chain passes it down.

It's additive: no existing signature changes.

CI on drafts. This PR also stops CI and CodeQL running on draft PRs. A draft runs only the required gate check, which fails in a few seconds with "Draft PR: CI runs when it's marked ready for review". Marking a PR ready runs the full CI and CodeQL, as does every later push to it. The gate fails rather than skips on a draft: a skipped required check counts as passing for that commit, so a PR could otherwise be marked ready and merged before its CI ran. CONTRIBUTING says so.

Why

#243 makes a durable Handle hold a lock for each step. Code that runs under that lock and calls back into the same Handle would wait forever. A service gets the step's context, so #243 can recognize a call carrying it and return ErrReentrantStep instead. Guards, actions, reducers and actor factories had no context to pass back, so the same mistake there still deadlocked. With this change, a callback that passes in.Ctx back into its Handle gets ErrReentrantStep too. Once both land, #243's docs will say so.

It also lets work under review stay a draft without spending CI time, as agreed with the maintainers.

Relates to: #214, #243

Evidence

On GOTOOLCHAIN=go1.27.3:

Check Result
go test -race ./state/... ./durable/... ./cluster/... Pass
go test ./... in examples/dispatch and e2e Pass
go vet ./... in every workspace module Clean
Pinned golangci-lint on state 0 issues
actionlint on ci.yml and codeql.yml Clean
This PR as a draft, after the CI change Only gate ran (3 s, failed with the draft message); CodeQL skipped

The new tests in state/callback_context_test.go cover a guard, an action and a reducer seeing the Fire's context, an entry reducer on a later Fire, Cast with and without WithContext, PlanPath, a nested Fire, and RegisterContext through spawn, Cast and delivery, a BindGuard binding, middleware, an eventless transition settling during Cast, and a parallel region. Each fails when the code it covers is removed.

Checklist

  • Commits are signed off (git commit -s) per the DCO
  • Conventional commit messages (type: subject)
  • mage check passes locally (lint + race tests + govulncheck): not run as a whole; the parts above were run individually
  • Tests added/updated for the change
  • Public API changes documented (godoc) and, if applicable, noted for the changelog

A guard, action or reducer had no context, so a host couldn't tell a call
it made back into the host from a call on another goroutine. GuardCtx,
ActionCtx and AssignCtx now carry Ctx: the context the Fire was given, or
context.Background() outside a Fire. The in-process bindings pass their
own context through.

WithContext sets the context Cast gives the reducers that settle the
initial state. ActorSystem.RegisterContext registers a factory that gets
the spawning step's context, so it can Cast its child with it. A child's
own callbacks already get the context its delivery carries.

The change is additive.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18 reka18 added not-ready On every new PR until all review dimensions are confirmed wait:correctness ACR pending: correctness wait:regression-safety ACR pending: regression-safety wait:concurrency-discipline ACR pending: concurrency-discipline wait:api-compatibility ACR pending: api-compatibility wait:missed-context ACR pending: missed-context wait:human-readability ACR pending: human-readability labels Nov 10, 2026
A draft PR now runs only the gate, which fails with a note that CI runs
once the PR is marked ready. Marking it ready (ready_for_review) runs the
full CI and CodeQL, as does every later push to a ready PR.

The gate fails rather than skips on a draft: a skipped required check
counts as passing for that commit, so a PR could otherwise be marked
ready and merged before its CI had run.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
Review found that a guard registered with BindGuard was adapted with a
background context, dropping the Fire's context the kernel now passes.
Its EvalGuard now gets the Fire's context.

Callbacks also see the context as the middleware chain passes it down,
not only the one Fire was called with.

Docs: WithContext reaches the guards and actions that settle the initial
state as well as its reducers, and Cast settles to completion even if
that context has ended. The ActorSystem doc says the parent Instance
still needs one goroutine at a time. The site docs mention Ctx.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
…in regions

Review follow-ups:
- Cast clears the callbacks' context with defer, as Fire does.
- Tests: an eventless transition settling during Cast, and a transition
  inside a parallel region, see the expected context.
- Ctx's doc no longer suggests PlanPath runs actions or reducers.
- The guards and actors pages mention Ctx and RegisterContext, the
  deferred-surface notes record Cast's WithContext, and CONTRIBUTING says
  CI runs on ready PRs.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18 reka18 changed the title feat(state): pass the step's context to guards, actions and reducers feat(state): pass the step's context to callbacks; skip CI on drafts Nov 10, 2026
@reka18 reka18 added ready All ACR dimensions confirmed - clear to merge confirmed:correctness ACR clean: correctness confirmed:api-compatibility ACR clean: api-compatibility and removed not-ready On every new PR until all review dimensions are confirmed wait:correctness ACR pending: correctness wait:regression-safety ACR pending: regression-safety wait:concurrency-discipline ACR pending: concurrency-discipline wait:api-compatibility ACR pending: api-compatibility wait:missed-context ACR pending: missed-context wait:human-readability ACR pending: human-readability labels Nov 10, 2026
@reka18

reka18 commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Review

Each dimension was attacked by an AI reviewer working from a clean context. The reviewers could only read code; they traced paths by hand. I ran the tests and mutation checks. All seven dimensions are confirmed.

Dimension Round 1 Round 2 Status
Correctness 0 blockers Not rerun Confirmed
Concurrency discipline 1 blocker 0 blockers Confirmed
API compatibility 0 blockers Not rerun Confirmed
Regression safety 1 blocker 0 blockers Confirmed
Missed context 1 blocker 0 blockers Confirmed
Human readability 0 blockers Not rerun Confirmed
CI gating (the workflow change) 0 blockers Not rerun Confirmed

Fixed

  • Blocker, found by three reviewers: a guard registered with BindGuard, the seam CEL and wasm guards use, was adapted with context.Background(), so it never saw the step's context. Its EvalGuard now gets the Fire's context, and a test proves it.
  • Callbacks now see the context as the middleware chain passes it down.
  • Cast restores its context with defer, as Fire does.
  • Tests added for an eventless transition settling during Cast and a transition in a parallel region.
  • Docs: WithContext reaches the guards and actions settling the initial state, not just its reducers; Cast settles to completion even if that context has ended; the ActorSystem doc says the parent Instance still needs one goroutine at a time; the guards, actors, actions and assigns pages mention Ctx; CONTRIBUTING says CI runs on ready PRs.

Not changed

  • durable.Runner.Start doesn't pass WithContext yet, and cluster.WithActorBehaviors has no context-aware form. Both are follow-ups in those packages.
  • Pre-existing and unchanged: a nested Fire on the same instance restores only the outer Fire's context, not its other per-Fire state; the actor system runs a spawn's behavior outside its mutex; a stacked PR whose base is retargeted to main gets no CI event until its next push; CodeQL's weekly scan can be cancelled by a push to main.

@reka18
reka18 marked this pull request as ready for review November 10, 2026 20:15
@reka18
reka18 requested a review from joshua-temple November 10, 2026 20:15
@joshua-temple

joshua-temple commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Review at 1aea966. Build, vet, tests pass. One fix needed before merge.

Must fix

  • state/binding.go:186-189: cancelled Fire ctx makes a bound guard return an error, counted as false
    • A fallback transition can commit before the next cancel check at fire.go:253
    • Durable replay runs with a live ctx, takes the primary branch, diverges from live
    • Fix: if Ctx.Err() != nil, surface the error, not a false verdict
    • Move the CHANGELOG entry from Added to Changed
    • Add a test: guard under a cancelled ctx

Should fix

  • kernel.go:561,576, assign.go:35: Ctx godoc should say callbacks must not branch on it (replay uses the recovery ctx)
  • actor_snapshot.go:279: no test for the RestoreActors path
  • state/CHANGELOG.md:~728: reserved WithContext promise clashes with Cast's WithContext; reword

Nits

  • fire.go:69: comment that Fire's defer restores callCtx

Review follow-ups:
- A guard registered with BindGuard whose EvalGuard fails after the
  Fire's context ended now stops the Fire with the context's error. It
  was read as false, so another transition was chosen and its callbacks
  ran under the ended context until the next cancellation check rolled
  the Fire back. An eventless transition chosen while the context ended
  isn't committed either. The CHANGELOG lists this under Changed.
- Ctx's doc says not to branch on it: a replay runs the same callbacks
  under a different context.
- A test covers RestoreActors through RegisterContext.
- The deferred-surface note no longer reads as a promise that clashes
  with Cast's WithContext.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18 reka18 added not-ready On every new PR until all review dimensions are confirmed wait:correctness ACR pending: correctness wait:regression-safety ACR pending: regression-safety wait:human-readability ACR pending: human-readability wait:replay-determinism ACR pending: replay-determinism and removed ready All ACR dimensions confirmed - clear to merge confirmed:correctness ACR clean: correctness confirmed:regression-safety ACR clean: regression-safety confirmed:human-readability ACR clean: human-readability labels Nov 10, 2026
…rds alike

Review follow-ups:
- Cast gives its callbacks WithContext's values without its cancellation
  or deadline. A bound guard that honored an ended context could fail
  there and quietly send the settle down another transition, because
  Cast settles to completion and has no error to report.
- A guard stopped by an ended context no longer reports a guard panic or
  failure in the Trace on any path: named guards, guard expressions,
  parallel regions and the parallel state itself.
- The Fire's error names the bound guard and keeps its own error, while
  still matching the context's.
- Tests pin each of these, and a Fire whose eventless guard was canceled
  with nothing chosen.
- Docs: ContextActorBehavior covers restore; the site pages say not to
  branch on Ctx.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
An eventless transition's guard stopped by an ended context was read as
not passing, so the Fire reported a bare context error, without the
guard's name or the binding's own error. Eventless selection now stops
with the guard's error, as event-driven evaluation does. A test covers
both.

Docs: a nested Fire clears the outer Fire's raised events; a bound
guard's cancellation check reads the context it got, as middleware
passed it down; at Cast, Ctx carries the values of WithContext's
context.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
The Fire's error embedded the binding's error as text only, so a host couldn't match its sentinel with errors.Is, though the CHANGELOG said it was wrapped. It now wraps both the context's error and the binding's. The test checks errors.Is for each.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
… error

A guard that panicked with a value wrapping context.Canceled was taken for a guard stopped by its ended context. Canceled guards now carry the kernel's own marker, so only those skip the guard-panic outcome. Tests: that case, a region's eventless transition settling during Cast, and a note that a nil Ctx fails the context tests.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18 reka18 added ready All ACR dimensions confirmed - clear to merge confirmed:correctness ACR clean: correctness confirmed:regression-safety ACR clean: regression-safety confirmed:human-readability ACR clean: human-readability confirmed:replay-determinism ACR clean: replay-determinism and removed not-ready On every new PR until all review dimensions are confirmed wait:correctness ACR pending: correctness wait:regression-safety ACR pending: regression-safety wait:replay-determinism ACR pending: replay-determinism wait:human-readability ACR pending: human-readability labels Nov 11, 2026
@reka18

reka18 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed in 2f3c928 through f2eb64c.

Your must-fix: done. A BindGuard binding whose EvalGuard fails after the Fire's context ended now stops the Fire with the context's error, on event-driven and eventless transitions alike. The error names the guard and wraps both the context's error and the binding's own, so errors.Is matches either. The CHANGELOG entry is under Changed, and tests pin both kinds of transition. One correction: the divergence couldn't persist. The cancellation check after each microstep already rolled the Fire back, with nothing journaled. What the fix removes is the fallback's actions and reducers running under the ended context before that rollback.

Reviewing the fix also turned up a Cast case. Cast settles to completion and can't abort, so with an ended WithContext context, a bound guard that honoured cancellation sent the settle down another transition with no error. Cast's callbacks now get WithContext's values without its cancellation or deadline.

Your should-fix items: all done.

  • The Ctx docs say not to branch on it, because a replay runs the same callbacks under a different context. The guards, actions and assigns pages say so too.
  • There's now a RestoreActors test through RegisterContext.
  • The reserved WithContext note in the CHANGELOG is reworded.

Your nit: done. The comment at fire.go:69 says Fire's deferred restore puts callCtx back.

Also: a cancelled guard no longer shows as a guard panic or failure in the Trace on any path. The review summary below this comment has the details.

@reka18

reka18 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Review after the maintainer's comments

The maintainer's review is addressed in 2f3c928 through f2eb64c. Each changed dimension was attacked again by an AI reviewer working from a clean context, until it had no blockers. The reviewers could only read code; I ran the tests and the mutation checks. All dimensions are confirmed.

Dimension Rounds since the maintainer's review Status
Correctness 2 rounds with blockers, then a verify-only round Confirmed
Replay determinism 2 rounds with blockers, then a verify-only round Confirmed
Regression safety 0 blockers Confirmed
Human readability 0 blockers Confirmed
Concurrency, API compatibility, missed context, CI gating Unchanged since their earlier confirmation Confirmed

Fixed

  • A bound guard on an ended context: a BindGuard binding whose EvalGuard fails after the Fire's context ended stops the Fire with the context's error, on event-driven and eventless transitions. The error names the guard and wraps both the context's error and the binding's own. The fallback transition's callbacks never run.
  • Cast with an ended context: Cast settles to completion, so a bound guard that honoured cancellation could quietly send the settle down another transition. Cast's callbacks now get WithContext's values without its cancellation or deadline.
  • Trace outcome: a cancelled guard is no longer reported as a guard panic or failure, whether it's a named guard, a guard expression, or in a parallel region or the parallel state. A guard that panics with a value wrapping a context error is still reported as a panic.
  • Docs: don't branch on Ctx, since a replay runs the same callbacks under a different context; a nested Fire clears the outer Fire's raised events; ContextActorBehavior covers restore.
  • Tests: each fix has a test that fails when the fix is removed, plus RestoreActors through RegisterContext and a region's eventless transition settling during Cast.

Not changed

  • A Fire stopped by a cancelled guard during eventless selection keeps the earlier outcome in its Trace instead of the cancellation outcome. The error is right; the outcome is a consistency follow-up.
  • A Fire rolled back at a cancellation check keeps a Trace that reads success. This predates this PR.
  • A plain guard that checks its context and returns false can still steer a transition until the rollback. That's covered by the rule against branching on Ctx.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

confirmed:api-compatibility ACR clean: api-compatibility confirmed:concurrency-discipline ACR clean: concurrency-discipline confirmed:correctness ACR clean: correctness confirmed:human-readability ACR clean: human-readability confirmed:missed-context ACR clean: missed-context confirmed:regression-safety ACR clean: regression-safety confirmed:replay-determinism ACR clean: replay-determinism ready All ACR dimensions confirmed - clear to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants