Code Review Analysis

Critical

1. Potential deadlock between NewTerm and becomeFollower callback
follower_controller.go — The becomeFollower callback (line 275-279) acquires fc.rwMutex.Lock() inside the bgAppender goroutine. Meanwhile, NewTerm() acquires fc.rwMutex.Lock() (line 297), then calls fc.logSynchronizer.Close() which calls ls.waitGroup.Wait(). If bgAppender is inside becomeFollower() waiting for the lock, and NewTerm holds the lock waiting for bgAppender to finish — deadlock.

2. Race condition: term check before lock in NewTerm allows term regression
follower_controller.go:293-297 — The term comparison newTerm < fc.term.Load() happens before acquiring rwMutex. Between the check and the lock, another concurrent NewTerm could set a higher term. The stale check then allows overwriting with a lower term, violating the monotonically-increasing term invariant.

High

3. Delete() uses WAL and DB after Close() already closed them
follower_controller.go:643-652 — Close() on line 643 closes fc.wal and fc.db, then lines 651-652 call fc.wal.Delete() and fc.db.Delete() on the already-closed resources. This could panic or silently fail to delete data.

4. initDatabase silently ignores ReadTerm error
follower_controller.go:125 — The error from db.ReadTerm() is never checked. The := shadows the named return err, so execution continues with potentially undefined term/options values.

Medium

5. Unbuffered signal channels cause lost notifications
follower_controller.go:192 — stateApplierCond is created with zero buffer. Signals sent via PushNoBlock are dropped when the receiver is busy. Same issue with syncCond in log_synchronizer.go:166. This can delay entry application or WAL sync indefinitely if no new appends arrive.

6. InstallSnapshot holds write lock during entire stream receive
follower_controller.go:483-484 — The lock is held for the entire snapshot transfer including all stream.Recv() calls, blocking GetStatus, NewTerm, Truncate, and AppendEntries for potentially minutes.

Low

7. InstallSnapshot does not update fc.status on success — Status remains whatever it was before (e.g. FENCED), which may prevent accepting append entries without an additional NewTerm cycle.

8. Test TestFollower_DisconnectLeader reuses an already-cancelled stream (line 851-854) — The test passes for the wrong reason since the stream context is already cancelled before AppendEntries is called.

Updated review after detailed analysis against main:

Items from the original review — status:

1. Deadlock between NewTerm and onAppend — Fixed in 4883d79 (status is now atomic, no lock in callback).
2. Term regression race in NewTerm — Fixed in 06c2cb8 (double-check after lock acquisition).
3. Delete() uses WAL/DB after Close() — Same pattern as main (original also closed before deleting via saved references). Acceptable.
4. initDatabase ignores ReadTerm error — Not an issue, error is checked on line 128.
5. Unbuffered signal channels — Fixed in 224bfc1 and c8d6299 (channels are now buffered).
6. InstallSnapshot holds lock during stream — By design; same as main which held sync.Mutex throughout handleSnapshot.
7. InstallSnapshot doesn't update status — Acceptable; coordinator always calls NewTerm after snapshot.
8. Test reuses cancelled stream — Fixed in test changes (new stream created after NewTerm).

Two regressions found and fixed:

• InstallSnapshot was persisting term after sending ack — crash could lose term (037bd6d).
• InstallSnapshot was overwriting TermOptions with empty defaults — removed redundant UpdateTerm since snapshot DB already contains the leader's term and options (54ea495).