Skip to content

fix(durable): lock the Handle and refuse steps after a failed Append - #233

Closed
reka18 wants to merge 3 commits into
mainfrom
fix/durable-handle-lock-failed-append
Closed

reka18 wants to merge 3 commits into
mainfrom
fix/durable-handle-lock-failed-append

Conversation

@reka18

@reka18 reka18 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

What this change does

Two #214 fixes in the same code path. Rebased onto main now that #232 has merged.

  • 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

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 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:replay-determinism ACR pending: replay-determinism wait:api-compatibility ACR pending: api-compatibility wait:missed-context ACR pending: missed-context wait:human-readability ACR pending: human-readability labels Nov 5, 2026
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from fa0fe3f to 69ef146 Compare November 5, 2026 16:38
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from f0650c2 to 12eb6e2 Compare November 5, 2026 16:38
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 69ef146 to aeac33f Compare November 5, 2026 16:40
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 12eb6e2 to 1bb1896 Compare November 5, 2026 16:40
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from aeac33f to 04b5e9d Compare November 5, 2026 16:53
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 1bb1896 to 651b18b Compare November 5, 2026 16:53
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 04b5e9d to b1c7167 Compare November 5, 2026 17:09
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 651b18b to 3c76b62 Compare November 5, 2026 17:09
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from b1c7167 to 5d7a713 Compare November 5, 2026 17:14
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 3c76b62 to 474cfab Compare November 5, 2026 17:14
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 5d7a713 to 3d338cd Compare November 5, 2026 17:22
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 474cfab to 6b0cfa0 Compare November 5, 2026 17:22
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 3d338cd to 8b74a6b Compare November 5, 2026 17:33
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 6b0cfa0 to f0c458f Compare November 5, 2026 17:33
@reka18
reka18 added this pull request to stack #238 November 5, 2026 17:34
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 8b74a6b to de02c29 Compare November 5, 2026 17:42
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from f0c458f to 1b647b2 Compare November 5, 2026 17:42
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from de02c29 to 826c383 Compare November 5, 2026 18:04
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 1b647b2 to ec0d8cc Compare November 5, 2026 18:04
@reka18 reka18 added ready All ACR dimensions confirmed - clear to merge confirmed:correctness ACR clean: correctness confirmed:regression-safety ACR clean: regression-safety confirmed:concurrency-discipline ACR clean: concurrency-discipline confirmed:replay-determinism ACR clean: replay-determinism confirmed:api-compatibility ACR clean: api-compatibility confirmed:missed-context ACR clean: missed-context confirmed:human-readability ACR clean: human-readability and removed wait:correctness ACR pending: correctness wait:regression-safety ACR pending: regression-safety wait:concurrency-discipline ACR pending: concurrency-discipline wait:replay-determinism ACR pending: replay-determinism wait:api-compatibility ACR pending: api-compatibility wait:missed-context ACR pending: missed-context wait:human-readability ACR pending: human-readability labels Nov 5, 2026
@reka18

reka18 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Review results. One reviewer per dimension, each reading the others' findings. All seven dimensions have zero blockers on the latest round.

Fixed along the way: the Handle lock and failed-Append refusal, with context checks before each step. The timer-loop test moved here from #213 and now also fails if a Fire or Tick errors or no timer fires.

Known limit: that test is reliable under -race; a plain run can pass by luck.

@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from f7b771b to 4ee6c01 Compare November 5, 2026 21:25
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 9f0db01 to ad2b6c6 Compare November 5, 2026 22:00
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 4ee6c01 to 26b512f Compare November 5, 2026 22:00
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from ad2b6c6 to 831afc6 Compare November 5, 2026 22:12
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch 2 times, most recently from 7fd91fe to 2ab1add Compare November 5, 2026 22:17
@reka18
reka18 force-pushed the fix/durable-step-numbering branch from 831afc6 to aef9065 Compare November 5, 2026 22:17
WithEventData payloads are recorded and supplied again by Recover and StateAt,
through the additive state.FireEventData. The live Fire now uses the decoded
payload too, so the live run and a replay can't differ. An error payload is
rebuilt from its message. A payload that can't be recorded, or is over
WithMaxEventDataBytes, refuses the Fire before the kernel runs. FileStore
refuses a Record too large for it to read back.

Relates to #214.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
Recovery derived the next step from the restored Trace count when a checkpoint
had compacted the tail. A tick barrier consumes an ordinal but leaves no Trace,
so the count fell short and the next Fire reused a recorded step. The checkpoint
envelope now records the step that follows it. A checkpoint written before this
change keeps the old fallback.

Relates to #214.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
Fire, Tick, RunService and DeliverToActor now hold a lock for the whole step,
so a host's timer loop no longer loses acknowledged steps. A step that applied
to the kernel but wasn't appended leaves the instance ahead of the journal, so
the Handle refuses further steps with ErrHandleFailed until it's recovered.

Relates to #214.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18
reka18 force-pushed the fix/durable-handle-lock-failed-append branch from 2ab1add to 67d9182 Compare November 6, 2026 15:41
@reka18

reka18 commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Replaced by #243. GitHub couldn't reopen this PR after its branch was rebased onto main. #243 has the same branch, now at 2f8b57c.

@reka18

reka18 commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

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.

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.

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.

1 participant