durable: defects found in the platform-redesign review #214

Summary

An adversarial review of durable as the session journal for the voice-ordering platform found defects in fencing, concurrency, replay fidelity, effect dispatch, timers, step numbering and growth. Every defect below has a failing test in #213. Each test asserts what any correct fix must produce, so it can move into the fix's PR unchanged. Line references are main @ 8770dcf. Relates to #179.

Impact

A recovered instance can silently differ from the live one: a different state, a lost acknowledged step, a missing timer, or an effect applied zero times. Two workers on one instance can both commit. Several of these surface only after a crash, deploy or lease handoff, which is exactly when durable is meant to hold. (durable restores the decision history; the platform restores the rest of the session state separately and reconciles with the downstream order system, so these defects surface as a wrong decision history, not a wrong session row.)

Findings (in the agreed fix order)

Fencing

• Two Handles on one instance both get acknowledged Fires, and recovery yields a state neither computed. Append treats a different record for a recorded step as an idempotent no-op (store.go:87, memstore.go:117), and nothing invalidates an older Handle. Test: TestRecover_TwoHandlesOnOneInstance_JournalMatchesAnAcknowledgedHistory
• A Fire whose Append fails stays applied to the live instance (runner.go:360 runs the kernel before the append), so the next acknowledged Fire leaves live ahead of the journal. Test: TestRunner_FireAfterFailedAppend_MatchesJournal

Handle concurrency

• Fire during the documented "Tick from its own timer loop" pattern loses acknowledged steps on 2+ CPUs. Handle has no lock around nextStep (runner.go:372, 389, 415, 437) or the kernel instance. Test: TestRunner_FireDuringTimerLoop_LosesNoAcknowledgedSteps

Replay fidelity

• WithEventData is dropped on recovery: the record keeps only the trace payload (runner.go:381) and replay re-fires without options (runner.go:620). Test: TestRecover_FireWithEventData_MatchesLive
• A service result whose settle was canceled with the turn's context is journaled anyway (service.go:119) and committed on recovery, while the live instance never saw it. Test: TestService_ResultFromCanceledTurn_NotCommittedOnRecover

Effect dispatch

• Only Handle.Fire dispatches domain effects. Effects from Start's initial entry (runner.go:287 absorbs only StartEffects), timer transitions (Tick, runner.go:412), service results (RunService, service.go:119) and actor delivery (actor.go:105) never reach the handler. Tests: TestRunner_EffectDispatch_Start_AppliesEntryEffect, TestRunner_EffectDispatch_TimerTransition_AppliesEffect, TestRunner_EffectDispatch_ServiceSettle_AppliesEffect
• An effect whose dispatch failed is compacted away by the next checkpoint and never retried. Test: TestRunner_EffectDispatch_FailedThenCheckpointed_AppliedOnce

Timers

• Start doesn't arm the initial state's after timer (runner.go:287, same path as above). Test: TestClock_StartInTimedState_ArmsTimer
• A timer persisted by a checkpoint and fired in the tail is missed on replay: persisted timers are re-armed only after the tail replays (runner.go:668). Test: TestTimerCheckpoint_FiredAfterCheckpoint_ByteIdentical
• A timer armed by a service result is lost on replay: RunService records no clock read for the arm (service.go:119) and replay doesn't absorb the settle's timer effects (runner.go:589). Test: TestService_TimerArmedBySettle_ReplaysOnRecover

Step numbering

• When a checkpoint compacts the tail, recovery derives the next step from the trace count (runner.go:698), which undercounts tick barriers. The next Fire reuses a recorded step and is dropped with a nil error. Test: TestRecover_StepAfterTickCheckpoint_IsPersisted

Telemetry on replay

• Middleware (the sink/bridge seam) runs again for every replayed step (runner.go:620), so recovery re-emits spans and sink events. No test until open question 7 is decided.

Growth

• Every tick appends a record even when no timer is due: Scheduler.Tick always reads the clock (state/driver.go:116), so the "records nothing" guard (runner.go:429) never applies. Test: TestClock_TickWithNothingDue_AppendsNoRecord
• Checkpoints carry the full trace history (runner.go:258 forces unbounded history): 8 KB at step 50, 157 KB at step 1000. No test until open question 9 is decided.

Open questions

These are contract decisions the fixes depend on. The tests deliberately accept either answer where one exists. 1, 10 and 11 block the first checkpoints.

1. Fencing: should Recover take over the instance (an older Handle's commits fail), or should commits use optimistic concurrency on the step number? Both pass the test. A Recover that refuses while a Handle is held would also need a release API.
2. Append contract: keep first-writer-wins, or reject a different record for a recorded step? ..._Append_FirstWriterWins in the MemStore and FileStore tests asserts today's behavior.
3. Store without the dispatch set: keep the documented no-op (effect.go:214, TestRunner_EffectDispatch_NonDispatchStore_NoOp), or refuse the configuration?
4. Recover while the effect handler is down: keep failing (no worker can resume until the downstream is back), or recover and redispatch later?
5. Effect ids (step#ordinal#kind, effect.go:150) repeat across instances, so they can't serve as downstream idempotency keys. Add the instance id (needs a dispatched-set migration), or pass the instance to the handler?
6. Machine version on recovery: Recover hard-codes its Restore options (runner.go:542), so RejectMachineVersionMismatch can't be enabled and a deploy replays history through new code. Expose the opt-in, or make it strict?
7. Telemetry on replay: skip middleware while replaying, or run it with a replay marker in the context?
8. A timer whose transition fails is consumed, and the error appears only in results[i].Err. Re-arm it, return the error from Tick, or leave it?
9. Checkpoint size: bound the trace history in checkpoints? Doing so changes the byte-identical recovery guarantee the existing tests assert.
10. Handle concurrency: is a Handle meant to support Fire and Tick from different goroutines (as drainClock's comments describe), or is single-goroutine ownership the contract? The concurrency test assumes the former.
11. Event data: journaling WithEventData needs a way to read it from state (fireConfig is unexported, state/options.go:261), which is an additive API on the frozen v1 module. Add it, or have a Handle refuse the option? The test accepts either.

Notes

• state stays at its frozen v1 contract. Question 11 is the only one that touches it, and only additively.
• Struct service results coming back as JSON maps is the documented caveat (doc.go, Caveats) and is not listed here.