[reka18]

What this change does

Two #214 fixes in the same code path. Rebased onto main now that #232 has merged. This replaces #233, which GitHub couldn't reopen after the rebase.

• Handle lock. Fire, Tick, RunService and DeliverToActor hold a lock for the whole step, so they're safe from different goroutines. This is the contract in my answer to question 10 on #214, which is a working answer, not a ruling.
• Failed Append. A step that applied to the kernel but may not have been recorded fails the Handle. That call and every later one return the new ErrHandleFailed, wrapping the cause, until a new Handle is recovered. nextStep now advances right after the append, so a failed checkpoint no longer makes the next step conflict.
• Ended contexts. Tick, RunService and DeliverToActor return ctx.Err() once they hold the lock, so a deadline that ended in the lock queue changes nothing. Before, Tick consumed a due timer the kernel then refused.

Please check these:

• Assumption: the per-step Append stays inline, on the session's single owner goroutine. If it moves off that goroutine, the lock scope and this failure handling change.
• The lock covers the Store write, effect dispatch and a running service, so a slow service blocks other steps, and a waiting step can't be bounded by its own context. state.ServiceRunner runs and settles in one call, so I couldn't narrow it here.
• Any host callback that calls back into its own Handle deadlocks. The Handle doc says so.
• Instance() returns the live instance outside the lock.
• If the cause is ErrFenced, a newer owner holds the instance, so Recover won't help. The docs say to stop.
• After a failed Append, a service whose result wasn't recorded is in flight again on the recovered Handle, so RunService runs it a second time. An InvokeActor actor restarts from its initial state, and a Spawn actor doesn't return.

Known and left open:

• A context canceled partway through Tick or DeliverToActor can still journal a step the kernel refused, and a canceled delivery can make Recover fail. An already-ended context no longer reaches that path.
• A Store that panics mid-step skips the failure latch, if the host recovers the panic.
• Stateless Runner.Fire calls on one id aren't serialized. The doc says so.
• A refused Fire leaves a Trace only in the live instance, so a later snapshot can differ byte for byte.

Why

A host's timer loop racing Fire lost acknowledged steps, and a failed Append left the live instance ahead of the journal.

Relates to: #214, #245

Evidence

On GOTOOLCHAIN=go1.27.3, in durable:

Command	Result
go test ./...	Pass, including TestRunner_FireDuringTimerLoop_LosesNoAcknowledgedSteps, TestRunner_FireAfterFailedAppend_MatchesJournal, and the failure-latch and ended-context tests for RunService and DeliverToActor in handle_failure_test.go, each checked to fail with its guard removed. The timer-loop test now also fails if a Fire or Tick errors or no timer fires. It's reliable under -race; a plain run can pass by luck
go test -race ./...	0 race warnings, with no test skipped
Pinned golangci-lint	0 issues

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, and it cannot pass until the rest of the chain lands
• Tests added/updated for the change
• Public API changes documented (godoc) and, if applicable, noted for the changelog

[reka18]

Findings from Temple's review of #233

Temple reviewed #233, the PR this one replaces. These are his findings and where each one is handled. Line numbers are from #233's old head (67d9182).

High

• Effect handlers that call back into the Handle deadlock (runner.go:457). This is real, and this PR causes it. Fix in this PR: the lock will cover only the kernel step and the Append, and effects will dispatch after it's released. A checkpoint won't compact past an effect that hasn't been dispatched. A call back in on the step's own context, from a service function or actor behavior that has to stay under the lock, will return an error instead of hanging.
• A checkpoint saved before fix(durable): carry the next step through a checkpoint #232 can recover with the wrong step number (runner.go:783). This is real. fix(durable): carry the next step through a checkpoint #232 added the NextStep field. A checkpoint from before fix(durable): carry the next step through a checkpoint #232 doesn't carry it, so recovery falls back to counting Traces. A tick barrier or an actor delivery throws that count off, the next Append conflicts, and every later Recover hits the same conflict. Fix in this PR: the built-in Stores already track the highest recorded step, so recovery will ask them for it. It will fall back to the Trace count only for a Store that can't report it.
• A Tick whose context ends partway replays differently from the live run (runner.go:487). This PR lists it as known. Fixed in fix(durable): don't journal a step whose context ended partway #234: a Tick, RunService or DeliverToActor whose context ends partway fails the Handle and records nothing, so the recovered instance matches the journal.

Medium

• A step waiting for the lock ignores its context deadline (:104). Fix in this PR: the lock becomes one a waiting step can leave, so it returns ctx.Err() once its context ends.
• Live Fire passes event data as decoded JSON, not the caller's Go value (:853). This came from fix(durable): journal WithEventData payloads for recovery #230. It was done so a live step and its replay see the same value, but a reducer that expects its own struct type gets a map. To fix here: let the Runner decode event data into the caller's type, so live and replay both see that struct. I'll post the exact shape before it lands.
• Instance() isn't covered by the lock (:270). The doc warns about it, but it's an easy race. Fix in this PR: a locked read on the Handle, so a host can read the instance without racing a step.

Low

• The new errors start with "crucible/durable:". Already fixed: the rebased errors use the package's durable: prefix.
• Fire advances its step number differently from the other three steps. Fixed in fix(durable): dispatch effects from Start, Tick and RunService #235: all four steps go through one commitStep.
• The file store copies each record twice per save. This came from fix(durable): journal WithEventData payloads for recovery #230. Fix in this PR: FileStore.Append will clone the record once.

A fresh review round runs on this PR before it changes, covering correctness, concurrency and replay determinism. The PRs above it will be rebased once it merges.

[reka18]

Review of the rebased change (494c78a..2f8b57c)

The change is rebased onto main after #232 merged, plus one fix commit (2f8b57c). All seven dimensions are confirmed, so the PR is ready.

The reviewers (AI agents, one per dimension, each starting with a clean context) could only read code: they traced the code paths but couldn't run tests. I ran the tests and the mutation checks myself.

Dimension	Round 1	Round 2	Status
Correctness	Blockers, all pre-existing (see below)	0 blockers	Confirmed
Concurrency discipline	Blockers, all pre-existing	0 blockers	Confirmed
Replay determinism	Blockers, all pre-existing	1 blocker, pre-existing, fixed in #236	Confirmed (my ruling)
Regression safety	1 blocker	0 blockers	Confirmed
API compatibility	0 blockers	Not rerun	Confirmed
Missed context	0 blockers	Not rerun	Confirmed
Human readability	0 blockers	Not rerun	Confirmed

Fixed in 2f8b57c

• Blocker: the RunService and DeliverToActor docs said a step that wasn't recorded returns false. It returns true along with ErrHandleFailed, and a failed checkpoint returns true along with that error. The docs now say so.
• Tests: RunService and DeliverToActor had no tests for the failure latch or the ended-context guard. Four tests in handle_failure_test.go now cover them, and each fails when its guard is removed. The Tick ended-context test now also checks that nothing was appended.
• Docs:
  • The Handle doc says a context that ends during the Store write can also fail the Handle.
  • A refused Fire now "journals nothing".
  • A comment at persistStep states the assumption that Append runs inline.
  • The "kernel transition itself does not fail" claim on RunService is removed.
  • The sentinel group comment covers the Handle.

Rejected: pre-existing on main, fixed higher in the stack

These come from code this PR doesn't change. Fixing them here would mean fixing more than the bottom PR of the stack.

• Tick, RunService and DeliverToActor don't dispatch domain effects, and the absorbDrivers doc names steps that don't start drivers. Fixed in fix(durable): dispatch effects from Start, Tick and RunService #235.
• RunService leaks a clock read into the next Record, and recovery loses timers that a settle armed. Fixed in fix(durable): keep timers across recovery and skip idle ticks #236.
• Round 2's blocker: after a tick, the Handle's timer table can keep a timer the scheduler already fired. The table is pruned by the tick's first clock read, but the scheduler uses each iteration's own read. A checkpoint can then save that timer, so recovery fires it again, and the comment that says the table matches the scheduler claims too much. fix(durable): keep timers across recovery and skip idle ticks #236 makes the table follow the scheduler. Disagreement: the reviewer called this a blocker. I rejected it for this PR because fix(durable): keep timers across recovery and skip idle ticks #236 fixes it.
• RunService journals a settle that the kernel refused on a canceled context. Fixed in fix(durable): don't journal a step whose context ended partway #234.
• The idle-tick comment on Tick describes a case that can't happen. Fixed in fix(durable): keep timers across recovery and skip idle ticks #236.

Pre-existing, possibly not tracked yet

• When Fire spawns an actor and the spawn fails, the parent's error transition runs on the live instance but isn't journaled.
• When a delivery to an actor drives a parent transition into a state with an after timer or an invoked service, that timer or service isn't started.
• A clock read from outside a step can be counted in the step that's open at the time.
• DeliverToActor writes an empty Record when the delivery drove no parent transition.

Non-blockers left as they are

• ActorRef reads outside the lock, and its docs don't say so the way the Instance() docs do.
• The failed-Append path of Tick itself has no direct test. The tests cover the same path through Fire, RunService and DeliverToActor.
• Tick could call persistStep instead of repeating its steps inline.
• The timer-loop race test triggers the race by chance, though it reliably catches the race when it happens.
• Recover's doc doesn't repeat the rule against overlapping a live Handle.
• A Tick that never stops re-arming now holds the lock indefinitely.

Verification

On GOTOOLCHAIN=go1.27.3:

• go test -race ./... passes in durable.
• go test ./... passes in state and examples/dispatch.

[joshua-temple]

Should we just fold ctx.Err() into refuse() ?

[reka18]

#243 (comment)

[reka18]

#245 passes the step's context to guards, actions, reducers and actor factories. Today only a service's call back into its own Handle is caught with ErrReentrantStep, because only a service gets the step's context; the same call from a guard, action, reducer or actor factory deadlocks. With #245, one that passes in.Ctx back gets ErrReentrantStep too.