Skip to content

fix(durable): journal WithEventData payloads for recovery - #230

Merged
reka18 merged 3 commits into
mainfrom
fix/durable-event-data
Nov 9, 2026
Merged

reka18 merged 3 commits into
mainfrom
fix/durable-event-data

Conversation

@reka18

@reka18 reka18 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

What this change does

WithEventData payloads now survive recovery, and a live run and a replay see the same payload, apart from the known cases listed below. It sits on main as the bottom of stack #239.

  • Handle.Fire records the payload in the new Record.EventData, and Recover and StateAt supply it on replay.
  • state.FireEventData is a new additive function in the frozen state module, so a host can read the payload from FireOptions.
  • The live Fire now uses the decoded payload too. Assign sees the same value live and on replay: a struct arrives as a map and an integer as a float64.
  • An error payload is journaled by its message and rebuilt with errors.New, live and on replay. The service and actor paths rebuild errors the same way on replay. errors.Is no longer matches it.
  • A payload with no JSON form that decodes again, one that panics while encoding (such as an error that's a typed nil), or a payload over the new WithMaxEventDataBytes (8 MiB by default), refuses the Fire with the new ErrEventDataEncoding before the kernel runs. The Handle stays usable.
  • The two new sentinels follow the durable: error text from fix(durable): add write fencing and reject conflicting steps #229.
  • FileStore.Append refuses a Record over its 16 MiB line limit with the new ErrRecordTooLarge, instead of writing a line Load can't read. An oversized line drops its Snapshot first, which recovery never reads, so large checkpoints keep working.

Please check these:

  • Decoding live changes what reducers see, so in.Event.(int) now fails at Fire. That assertion already failed on recovery. Six of seven reviewers backed this, over keeping the live value and documenting the difference.
  • durable needs state.FireEventData, which isn't in state 1.0.0. state releases v1.1.0 first, and durable's require moves there before a durable tag.
  • The cap covers the payload only. A Record that still exceeds a FileStore line, from a raised cap or a large Event, Entries or Effects, gets ErrRecordTooLarge after the kernel ran, which leaves the Handle out of step with the journal. fix(durable): lock the Handle and refuse steps after a failed Append #233 makes that Handle refuse further steps.
  • A Store that encodes fields one by one must keep the two new Record fields, or payloads are lost.
  • Known and left open: a payload nested about 10,000 levels deep can break a later Recover or checkpoint, and a map with colliding MarshalText keys encodes differently between runs, though the journal still matches live. An event with no JSON form, such as +Inf as a float64 event, journals an empty Event, so replay skips that step and its payload. That predates this PR. An error kept in context becomes a map once a checkpoint carries it, and in an error-typed field Recover then fails. A pointer chain millions deep overflows the stack in json.Marshal, which Go can't recover.

Why

Payloads passed with WithEventData were lost on recovery, so a transition's Assign saw different data on replay than it did live. Journaling alone isn't enough, because live saw the Go value and replay its JSON form.

Relates to: #214

Evidence

Run locally on Go 1.27.3 at 43edd5e:

Command Result
mage check Pass: lint, race tests and govulncheck across the suite
go test ./... in durable Pass, including TestRecover_FireWithEventData_MatchesLive
go test -race ./... in durable Pass
go test ./... in state Pass
GOWORK=off go build ./... in durable Pass

CI is green at the same commit: 112 of 112 checks pass.

Checklist

  • Commits are signed off (git commit -s) per the DCO
  • Conventional commit messages (type: subject)
  • mage check passes locally (lint + race tests + govulncheck)
  • Tests added/updated for the change: durable/event_data_test.go, state/fire_event_data_test.go
  • 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-event-data branch from ad5c962 to 1047e24 Compare November 5, 2026 16:38
@reka18
reka18 force-pushed the fix/durable-event-data branch from 1047e24 to 05d4a81 Compare November 5, 2026 16:40
@reka18
reka18 force-pushed the fix/durable-event-data branch from 05d4a81 to 6157108 Compare November 5, 2026 16:53
@reka18
reka18 force-pushed the fix/durable-event-data branch 3 times, most recently from b953e8c to a9d0723 Compare November 5, 2026 17:22
@reka18
reka18 force-pushed the fix/durable-event-data branch from a9d0723 to 57218cc 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-event-data branch from 57218cc to cb86fd7 Compare November 5, 2026 17:42
@reka18

reka18 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor Author

ACR on this PR (one reviewer per dimension)

Round 2 combined missed-context and human-readability in one reviewer; every other round ran one per dimension. The reviewers reconciled their findings after round 1, before code changed.

Dimension Rounds Latest round
correctness 6, the last verify-only 0 blockers
regression-safety 4 0 blockers
concurrency-discipline 1 0 blockers
replay-determinism 5 see below
api-compatibility 4 0 blockers
missed-context 3 0 blockers
human-readability 3 0 blockers

Main changes the reviews drove:

  • The live Fire now uses the decoded payload, so int and struct payloads can't differ between live and replay. Six of seven reviewers backed this; the maintainer decided.
  • Error payloads are rebuilt from their message, and the message goes through JSON first.
  • Payloads that can't be recorded, panic while encoding, or exceed the new WithMaxEventDataBytes are refused before the kernel runs.
  • FileStore never keeps a journal line Load can't read: an oversized Record drops its Snapshot, else returns ErrRecordTooLarge.

replay-determinism's last round: it reported three blockers. Two are the torn journal and non-atomic checkpoint bugs tracked in #228, which reproduce on the parent commit, so they're out of scope here. The third, an error kept in an error-typed context field, is now stated in the Handle.Fire doc and this PR. It's confirmed on that basis, not on a clean round.

Known and left open: listed under "Please check these" in the description.

All seven dimensions are confirmed. ready means review is complete; merging still depends on the PRs below this one in the stack.

@reka18 reka18 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:replay-determinism ACR pending: replay-determinism wait:api-compatibility ACR pending: api-compatibility labels Nov 5, 2026
@reka18
reka18 force-pushed the fix/durable-event-data branch 2 times, most recently from 29b6cc0 to 53ce7e5 Compare November 5, 2026 19:05
@reka18
reka18 force-pushed the fix/durable-event-data branch from 53ce7e5 to 90b2432 Compare November 5, 2026 19:19
@reka18
reka18 force-pushed the fix/durable-event-data branch from 90b2432 to 33be29d Compare November 5, 2026 20:23
@reka18
reka18 removed this pull request from stack #238 November 5, 2026 20:26
@reka18
reka18 added this pull request to stack #239 November 5, 2026 20:26
@reka18
reka18 force-pushed the fix/durable-event-data branch from 33be29d to 3679dcd Compare November 5, 2026 21:02
@reka18

reka18 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Update since review. #213 is closed. This PR now carries the tests it turns green, and it's stacked on #231 in stack #239. The code under review didn't change, and durable passes with -race and lint at this head.

@reka18
reka18 force-pushed the fix/durable-event-data branch from 3679dcd to a67e2a1 Compare November 5, 2026 21:25
@reka18
reka18 force-pushed the fix/durable-event-data branch from a67e2a1 to c773966 Compare November 5, 2026 22:00
@reka18
reka18 force-pushed the fix/durable-event-data branch 2 times, most recently from 1836226 to fd16d32 Compare November 5, 2026 22:17
Base automatically changed from fix/durable-fence-and-event-data to main November 6, 2026 15:41
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>
@reka18
reka18 force-pushed the fix/durable-event-data branch from fd16d32 to 27c5701 Compare November 6, 2026 15:41
Matches the change on main: sentinel text in private constants, and the
"durable: " prefix rather than "crucible/durable: ".

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18
reka18 marked this pull request as ready for review November 6, 2026 16:45
@reka18
reka18 requested a review from joshua-temple November 6, 2026 16:45
Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18
reka18 merged commit c6243fd into main Nov 9, 2026
112 checks passed
@reka18
reka18 deleted the fix/durable-event-data branch November 9, 2026 15:52
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