Repository navigation
Conversation
6329c7f to
aa13246
Compare
HagarMeir
left a comment
HagarMeir
left a comment
There was a problem hiding this comment.
Thanks for this. I left 10 inline comments. The main concern: nothing stops the controller from restarting a view after the ViewChanger has left it (via the sync → changeView path). That leaves the new truncation logic open to WAL sequences that fail recovery ([NewView, Commit] / [PP, ViewChange, Commit]). It also accounts for the timing dependence in state.go. Separately, the new AbortChan case in Decide can drop committed decisions, AbortView is now blocking, and clearing InFlight under Condition B discards prepared state. The last three comments are nits.
This review was written by Claude (an AI assistant) on behalf of @HagarMeir.
| // irrelevant, and either way there is nothing left to recover from it. Clearing it here | ||
| // makes the new view record a truncation point, by the rule PersistedState.Save applies. | ||
| if v.InFlight != nil { | ||
| v.InFlight.clear() |
There was a problem hiding this comment.
Race: the aborted view can be restarted before this NewView is saved.
processNewViewMsg relies on AbortView to keep the old view stopped while it clears InFlight and writes a truncating NewView record. Nothing stops the controller from restarting that view after AbortView returns and before the save.
Scenario: AbortView returns, then the heartbeat monitor calls Sync(). On the controller loop, sync() returns (0,0,0) and run() calls changeView(currentViewNumber, ...), which restarts the stopped view. The restarted view gets a pre-prepare and saves a ProposedRecord, which stores the in-flight. The VC then clears InFlight and saves NewView with truncateTo=true, which drops that pre-prepare, and the restarted view appends a Commit. The WAL now ends [NewView, Commit]. On restart recoverPrepared returns "expected second last message to be a pre-prepare" and NewProposer panics. The in-flight the live view still needs has also been cleared.
I think this needs a guard that keeps the controller from restarting a view the view-changer has already left.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // pre-prepare in the WAL. AbortView waits for the view to stop, and it also stops | ||
| // the view before the view data message is prepared below. If the controller has | ||
| // already moved to a newer view, this view change is stale. | ||
| if !v.Controller.AbortView(v.currView) { |
There was a problem hiding this comment.
The ordering this comment claims isn't enforced.
Moving AbortView before the ViewChange save is meant to stop a commit from landing after that record, but the controller can still restart the aborted view (the sync/changeView path) before Save runs.
Scenario: AbortView(v.currView) returns true. The controller loop takes a pending syncChan token and changeView restarts the stopped view at the same number. That view writes PP(s), the VC writes ViewChange(v), then the view writes Commit(s). The WAL ends [PP, ViewChange, Commit], and recovery fails because entries[-2] is not a pre-prepare.
This is the same root cause as the comment on processNewViewMsg. A left-view guard would cover both.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // the view before the view data message is prepared below. If the controller has | ||
| // already moved to a newer view, this view change is stale. | ||
| if !v.Controller.AbortView(v.currView) { | ||
| return |
There was a problem hiding this comment.
The restore path can silently drop a restored view change.
With restore=true and SyncOnStart=true: the node restarts with a ViewChange as the last WAL record, and the controller's syncOnStart advances to view w > NextView and calls InformNewView, but the VC run loop's select picks Restore first. AbortView(NextView) returns false (view < currView), so this early return exits after startViewChange has already set nextView/checkTimeout, without sending ViewData or clearing votes. Before this change the code continued past this point.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // In case we are in the middle of shutting down, | ||
| // abort deciding. | ||
| return | ||
| case <-view.AbortChan(): |
There was a problem hiding this comment.
A committed decision can be dropped.
Adding case <-view.AbortChan() to the first select of Decide drops a committed decision whenever the view is aborted, even when decisionChan has buffer room and the decision would previously have been queued and delivered.
Scenario: the view reaches decide() (startNextSeq has already run and the Commit is persisted) just as the controller calls Abort. Both select cases are ready and Go picks one at random, so about half the time the decision is discarded: Deliver is not called, the checkpoint stays at s-1, and ViewSequences already says s+1. The node then needs a view change or sync to deliver s. On the sync path (abortCurrentView) there is no view change to "recover" the proposal, so the comment's claim does not hold there.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // not be overtaken by a record written by the view, such as a truncating new | ||
| // view record. It returns false when the view is older than the current one or | ||
| // when the controller is stopping. | ||
| func (c *Controller) AbortView(view uint64) bool { |
There was a problem hiding this comment.
AbortView now blocks the ViewChanger run loop.
AbortView used to be a buffered fire-and-forget send. Now it blocks until the controller loop handles the request, and the controller loop can be inside a long Synchronizer.Sync()/fetchState or an application Deliver.
Scenario: the controller is in sync() doing a long state transfer when a ViewChange quorum arrives. The VC loop blocks in AbortView, so ticks and view-change timeouts are not processed and incMsgs fills. HandleMessage then blocks the comm goroutine that also feeds the Collector, so fetchState's StateTransferResponses cannot arrive and it times out.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| } | ||
|
|
||
| // The quorum has decided the fate of the proposal in flight: it is either committed or | ||
| // irrelevant, and either way there is nothing left to recover from it. Clearing it here |
There was a problem hiding this comment.
This clears the node's own prepared in-flight proposal.
InFlight is now cleared unconditionally, including under Condition B (noInFlight). That throws away this node's own prepared in-flight proposal, a protocol-relevant change made only to make the NewView record a truncation point.
Scenario: the node is prepared on X at seq s in view v, and the NewView for v+1 is accepted under Condition B (a quorum excluding this node reported nothing prepared). InFlight is cleared, so if v+1 fails before any proposal, this node's ViewData for v+2 no longer carries X/prepared. This seems to conflict with the in-flight carry-over rules in the GHSA-x46p fix (don't drop prepared state), and it ties WAL cleanup to view-change safety. Could the truncation decision be made without mutating InFlight here?
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // A commit never is: its pre-prepare has to survive even when another goroutine prunes the | ||
| // in-flight proposal (Controller.sync, through maybePruneInFlight) while this save is running, | ||
| // so the flag for a commit is not read off that shared state. | ||
| truncateTo := msgToSave.GetProposedRecord() != nil || |
There was a problem hiding this comment.
The truncation decision depends on goroutine timing.
The truncation flag for NewView/ViewChange is read from shared InFlightData, which the VC (clear) and the controller (maybePruneInFlight) both change outside the controller loop.
Scenario: after the VC's AbortView returns, the controller loop can run sync(), whose maybePruneInFlight clears in-flight while the VC is saving a ViewChange, or the reverse. Whether that record truncates the WAL then depends on scheduling rather than on a stable checkpoint. A ProposedRecord saved by a restarted view between the read and the Append can be truncated away.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // abortCurrentView stops the current view and waits for it to finish. It is | ||
| // called from the controller loop, which also owns abortViewChan, so it must | ||
| // not post an abort request to that channel. | ||
| func (c *Controller) abortCurrentView() { |
There was a problem hiding this comment.
nit: abortCurrentView re-implements abortView (relinquishLeaderToken + currView.Abort) without the view-number check. Could it call c.abortView(c.getCurrentViewNumber()) instead? Otherwise any future change to abortView, such as logging, metrics or a left-view guard, won't apply to the sync path.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // | ||
| // The last file of the WAL is kept even when it is an anchor-only one: an empty WAL still has to be | ||
| // a WAL of at least one file, both for Open() to have something to open and for ReadAll() to read. | ||
| func (w *WriteAheadLogFile) removeAnchorOnlyTailFiles(dirPath string, walNames []string) ([]string, error) { |
There was a problem hiding this comment.
nit: removeAnchorOnlyTailFiles keeps walNames and activeIndexes in parallel only to recompute w.index from walNames[0] (which it never changes), and it reopens every tail file that checkWalFiles has just opened. The root cause, ReadAll always creating a fresh file, is left in place. Would it be simpler for ReadAll/deleteAndCreateFile to reuse the last file when it's anchor-only instead of creating index+1? That would stop the accumulation at its source and make this cleanup pass on Open unnecessary.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
| // ViewController controls the view | ||
| // | ||
| //go:generate mockery -dir . -name ViewController -case underscore -output ./mocks/ | ||
| //go:generate mockery --dir . --name ViewController --case underscore --output ./mocks/ |
There was a problem hiding this comment.
nit: only this go:generate line was switched to mockery v2 flag syntax, and only its mock was regenerated (with v2.53.8). Every other directive in the package still uses v1 single-dash flags and v1-generated mocks, so go generate ./internal/bft/... now needs two incompatible mockery versions (v2 rejects -dir/-name, v1 rejects --dir). Either keep v1 here or migrate them all together.
(Written by Claude, an AI assistant, on behalf of @HagarMeir.)
|
Thanks for looking into the WAL file count. Some thoughts on the description and on how to move forward: Could you open an issue with a reproduction? It would help to see the I think the cause is different from what the description says. The description says "a node that never proposed never deleted a file", but the truncation point is not leader-only. Every replica saves the pre-prepare with What I believe actually grows the file count is restarts with no pre-prepare between them. On each restart Suggestion: split the PR in two.
Happy to review either part. This comment was written by Claude (an AI assistant) on behalf of @HagarMeir. |
|
To check the restart theory, I ran a quick experiment on
So the growth is real, but it happens only when a node restarts without a pre-prepare between restarts: a partitioned or crash-looping node, one that only catches up via sync, or one stuck in view changes. A node that keeps accepting pre-prepares, leader or follower, doesn't grow, and the accumulated files are deleted as soon as one pre-prepare arrives. That makes me think the This comment was written by Claude (an AI assistant) on behalf of @HagarMeir. |
aa13246 to
2467d9d
Compare
Your AI agent is looking at the repository in an interesting way.
|
no special prompt |
2467d9d to
52b8c55
Compare
|
The WAL part of this PR is now in #739. Once that merges, could you rebase this one so it carries only the internal/bft truncation changes? Written by Claude (an AI assistant) on behalf of @HagarMeir. |
I do rebase every time. but I removed it in the draft in order to try to do all the work in small words more understandable. |
e4f77c5 to
a24e0ad
Compare
The WAL only freed space at a truncation point, which only a pre-prepare carried. A node that never proposed never deleted a file: files rotated by size and truncateIndex stayed zero. PersistedState.Save now derives the truncation flag from whether a proposal is in flight rather than from the message type, so NewView after sync or view change and ViewChange with nothing in flight also truncate. A commit never does, keeping a pre-prepare and its commit together. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
a24e0ad to
b853746
Compare
Watching the stands, I found that some nodes are accumulating wal files, while they are quite working and efficient. The height of the blocks is normal. I think it's just that there are no signals for such nodes to trim the wal files. Below is my suggestion to change the situation:
The WAL only freed space at a truncation point, which only a pre-prepare carried. A node that never proposed never deleted a file: files rotated by size, truncateIndex stayed zero, and restarts accumulated anchor-only files.
PersistedState.Save now derives the truncation flag from whether a proposal is in flight rather than from the message type, so NewView after sync or view change and ViewChange with nothing in flight also truncate. A commit never does, keeping a pre-prepare and its commit together. Open also drops trailing anchor-only files, so restarts stop adding files.
#736