[pfi79]

AbortView stopped the current view, but the controller could restart it through the sync path before the view changer wrote its record. That allowed a commit to land after a NewView or ViewChange record, leaving a WAL sequence that recovery rejects ([NewView, Commit] or [PP, ViewChange, Commit]).

Add AbortViewAndHold/ReleaseViewWrite and a viewChangeLock so no view can be started or write between the abort and the record write. Also enqueue a decision instead of dropping it when the view is aborted, and let AbortView return without waiting for the controller loop.

#736

[HagarMeir]

minRestartView is only set while viewChangeLock is held, and changeView already waits on that lock. ReleaseViewWrite then resets it to 0, so isViewHeld never returns true outside the unit test that calls holdView directly. Could it be removed? If it's meant to stay, the holdView comment ("until the view changer asks for a new one") doesn't match what it does.

(Written by Claude Code.)

[HagarMeir]

The goroutine can put an aborted view's decision on decisionChan after the next view has started, or after the view changer has committed the same in-flight proposal. Is double delivery ruled out somewhere? A decision left in the buffered channel could already do this before the PR, so this may be fine.

(Written by Claude Code.)

[pfi79]

@HagarMeir fixed

[HagarMeir]

Thanks for the fixes. This is a lot of concurrency change in one PR, and I find it hard to follow. Could you split it? Something like:

1. Decide doesn't block or drop a decision when its view is aborted (enqueueDecision)
2. AbortView aborts directly instead of through the controller loop
3. The view changer aborts and holds the view before writing its ViewChange or NewView record
4. sync() aborts the view before saving the NewView record, including the maybePruneInFlight move

Each one would then come with its own tests. Also, ViewController is internal, so AbortViewAndHold/ReleaseViewWrite could go on it directly instead of through the viewHolder type assertion.

(Written by Claude Code.)

[pfi79]

Thanks for the fixes. This is a lot of concurrency change in one PR, and I find it hard to follow. Could you split it? Something like:

1. Decide doesn't block or drop a decision when its view is aborted (enqueueDecision)
2. AbortView aborts directly instead of through the controller loop
3. The view changer aborts and holds the view before writing its ViewChange or NewView record
4. sync() aborts the view before saving the NewView record, including the maybePruneInFlight move

Each one would then come with its own tests. Also, ViewController is internal, so AbortViewAndHold/ReleaseViewWrite could go on it directly instead of through the viewHolder type assertion.

(Written by Claude Code.)

Please take a look, I've simplified the code. But if you think you need to split it into several pull requests, I'll do it.

[HagarMeir]

Thanks for the fixes. This is a lot of concurrency change in one PR, and I find it hard to follow. Could you split it? Something like:

1. Decide doesn't block or drop a decision when its view is aborted (enqueueDecision)
2. AbortView aborts directly instead of through the controller loop
3. The view changer aborts and holds the view before writing its ViewChange or NewView record
4. sync() aborts the view before saving the NewView record, including the maybePruneInFlight move

Each one would then come with its own tests. Also, ViewController is internal, so AbortViewAndHold/ReleaseViewWrite could go on it directly instead of through the viewHolder type assertion.
(Written by Claude Code.)

Please take a look, I've simplified the code. But if you think you need to split it into several pull requests, I'll do it.

yes please try to split the work
thank you