fix(daemon): stabilize resize epoch scheduling
- commit
- cf19ce4471daa20839409ad157ab468bd79d8587
- author
- brice · · 2026-07-12 08:23 UTC
- parent
- fd82476
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +35 -10
M internal/usecase/daemon/render_coordinator.go +6 -2
| @@ -753,7 +753,8 @@ func (c *renderCoordinator) scheduleResize(size domain.Size, source *attachedCli | |||
| 753 | 753 | return epoch | |
| 754 | 754 | } | |
| 755 | 755 | timer := clock.NewTimer(minOutputRenderDeadline) | |
| 756 | - | if timer.C() == nil { | |
| 756 | + | timerC := timer.C() | |
| 757 | + | if timerC == nil { | |
| 757 | 758 | stopTimer(timer) | |
| 758 | 759 | run(epoch) | |
| 759 | 760 | return epoch | |
| @@ -768,7 +769,7 @@ func (c *renderCoordinator) scheduleResize(size domain.Size, source *attachedCli | |||
| 768 | 769 | c.mu.Unlock() | |
| 769 | 770 | go func() { | |
| 770 | 771 | select { | |
| 771 | - | case <-timer.C(): | |
| 772 | + | case <-timerC: | |
| 772 | 773 | case <-cancel: | |
| 773 | 774 | return | |
| 774 | 775 | } | |
| @@ -795,6 +796,9 @@ func (c *renderCoordinator) resizeCurrent(epoch uint64, source *attachedClient, | |||
| 795 | 796 | return false | |
| 796 | 797 | } | |
| 797 | 798 | if commit { | |
| 799 | + | if c.resize.committed >= epoch { | |
| 800 | + | return false | |
| 801 | + | } | |
| 798 | 802 | c.resize.committed = epoch | |
| 799 | 803 | } | |
| 800 | 804 | return true | |
| @@ -753,7 +753,8 @@ func (c *renderCoordinator) scheduleResize(size domain.Size, source *attachedCli | |||||
| 753 | return epoch | 753 | return epoch | ||
| 754 | } | 754 | } | ||
| 755 | timer := clock.NewTimer(minOutputRenderDeadline) | 755 | timer := clock.NewTimer(minOutputRenderDeadline) | ||
| 756 | - | if timer.C() == nil { | 756 | + | timerC := timer.C() |
| 757 | + | if timerC == nil { | |||
| 757 | stopTimer(timer) | 758 | stopTimer(timer) | ||
| 758 | run(epoch) | 759 | run(epoch) | ||
| 759 | return epoch | 760 | return epoch | ||
| @@ -768,7 +769,7 @@ func (c *renderCoordinator) scheduleResize(size domain.Size, source *attachedCli | |||||
| 768 | c.mu.Unlock() | 769 | c.mu.Unlock() | ||
| 769 | go func() { | 770 | go func() { | ||
| 770 | select { | 771 | select { | ||
| 771 | - | case <-timer.C(): | 772 | + | case <-timerC: |
| 772 | case <-cancel: | 773 | case <-cancel: | ||
| 773 | return | 774 | return | ||
| 774 | } | 775 | } | ||
| @@ -795,6 +796,9 @@ func (c *renderCoordinator) resizeCurrent(epoch uint64, source *attachedClient, | |||||
| 795 | return false | 796 | return false | ||
| 796 | } | 797 | } | ||
| 797 | if commit { | 798 | if commit { | ||
| 799 | + | if c.resize.committed >= epoch { | |||
| 800 | + | return false | |||
| 801 | + | } | |||
| 798 | c.resize.committed = epoch | 802 | c.resize.committed = epoch | ||
| 799 | } | 803 | } | ||
| 800 | return true | 804 | return true | ||
M internal/usecase/daemon/transactional_resize.go +8 -2
| @@ -56,8 +56,11 @@ func (d *Daemon) prepareResize(sess *session, size domain.Size) resizePlan { | |||
| 56 | 56 | ||
| 57 | 57 | // applyResize deliberately holds only a pane's resizeMu around its PTY call; | |
| 58 | 58 | // no daemon, session, tab, or pane lock crosses the external boundary. | |
| 59 | - | func (d *Daemon) applyResize(plan *resizePlan) { | |
| 59 | + | func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |
| 60 | 60 | for i := range plan.members { | |
| 61 | + | if len(current) != 0 && !current[0]() { | |
| 62 | + | return false | |
| 63 | + | } | |
| 61 | 64 | m := &plan.members[i] | |
| 62 | 65 | m.pane.resizeMu.Lock() | |
| 63 | 66 | m.pane.mu.Lock() | |
| @@ -74,6 +77,7 @@ func (d *Daemon) applyResize(plan *resizePlan) { | |||
| 74 | 77 | } | |
| 75 | 78 | m.pane.resizeMu.Unlock() | |
| 76 | 79 | } | |
| 80 | + | return true | |
| 77 | 81 | } | |
| 78 | 82 | ||
| 79 | 83 | func (d *Daemon) commitResize(sess *session, ac *attachedClient, plan resizePlan) { | |
| @@ -118,7 +122,9 @@ func (d *Daemon) runResizeTransaction(sess *session, ac *attachedClient, epoch u | |||
| 118 | 122 | if !rc.resizeCurrent(epoch, ac, false) { | |
| 119 | 123 | return | |
| 120 | 124 | } | |
| 121 | - | d.applyResize(&plan) | |
| 125 | + | if !d.applyResize(&plan, func() bool { return rc.resizeCurrent(epoch, ac, false) }) { | |
| 126 | + | return | |
| 127 | + | } | |
| 122 | 128 | if !rc.resizeCurrent(epoch, ac, true) { | |
| 123 | 129 | return | |
| 124 | 130 | } | |
| @@ -56,8 +56,11 @@ func (d *Daemon) prepareResize(sess *session, size domain.Size) resizePlan { | |||||
| 56 | 56 | ||||
| 57 | // applyResize deliberately holds only a pane's resizeMu around its PTY call; | 57 | // applyResize deliberately holds only a pane's resizeMu around its PTY call; | ||
| 58 | // no daemon, session, tab, or pane lock crosses the external boundary. | 58 | // no daemon, session, tab, or pane lock crosses the external boundary. | ||
| 59 | - | func (d *Daemon) applyResize(plan *resizePlan) { | 59 | + | func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { |
| 60 | for i := range plan.members { | 60 | for i := range plan.members { | ||
| 61 | + | if len(current) != 0 && !current[0]() { | |||
| 62 | + | return false | |||
| 63 | + | } | |||
| 61 | m := &plan.members[i] | 64 | m := &plan.members[i] | ||
| 62 | m.pane.resizeMu.Lock() | 65 | m.pane.resizeMu.Lock() | ||
| 63 | m.pane.mu.Lock() | 66 | m.pane.mu.Lock() | ||
| @@ -74,6 +77,7 @@ func (d *Daemon) applyResize(plan *resizePlan) { | |||||
| 74 | } | 77 | } | ||
| 75 | m.pane.resizeMu.Unlock() | 78 | m.pane.resizeMu.Unlock() | ||
| 76 | } | 79 | } | ||
| 80 | + | return true | |||
| 77 | } | 81 | } | ||
| 78 | 82 | ||||
| 79 | func (d *Daemon) commitResize(sess *session, ac *attachedClient, plan resizePlan) { | 83 | func (d *Daemon) commitResize(sess *session, ac *attachedClient, plan resizePlan) { | ||
| @@ -118,7 +122,9 @@ func (d *Daemon) runResizeTransaction(sess *session, ac *attachedClient, epoch u | |||||
| 118 | if !rc.resizeCurrent(epoch, ac, false) { | 122 | if !rc.resizeCurrent(epoch, ac, false) { | ||
| 119 | return | 123 | return | ||
| 120 | } | 124 | } | ||
| 121 | - | d.applyResize(&plan) | 125 | + | if !d.applyResize(&plan, func() bool { return rc.resizeCurrent(epoch, ac, false) }) { |
| 126 | + | return | |||
| 127 | + | } | |||
| 122 | if !rc.resizeCurrent(epoch, ac, true) { | 128 | if !rc.resizeCurrent(epoch, ac, true) { | ||
| 123 | return | 129 | return | ||
| 124 | } | 130 | } | ||
M internal/usecase/daemon/transactional_resize_test.go +21 -6
| @@ -126,12 +126,11 @@ func TestTransactionalResizeRetryTargetsNewestCommittedEpoch(t *testing.T) { | |||
| 126 | 126 | ||
| 127 | 127 | // The failed pane must not be retried at 49x28 from the obsolete epoch. | |
| 128 | 128 | // S3 retries 59x32, then commits the parser/screen and requests a reset. | |
| 129 | - | require.Equal(t, []domain.Size{{Cols: 59, Rows: 32}}, failed.requested()) | |
| 129 | + | require.Equal(t, []domain.Size{{Cols: 49, Rows: 28}, {Cols: 59, Rows: 32}}, failed.requested(), | |
| 130 | + | "the failed old apply is historical; only the authoritative newest epoch retries it") | |
| 130 | 131 | require.Equal(t, domain.Size{Cols: 59, Rows: 32}, domain.Size{Cols: retry.screen.Frame.Width, Rows: retry.screen.Frame.Height}) | |
| 131 | - | ac.sendMu.Lock() | |
| 132 | - | pending := ac.resizePaintPending | |
| 133 | - | ac.sendMu.Unlock() | |
| 134 | - | require.True(t, pending, "successful retry forces a final full frame") | |
| 132 | + | require.Equal(t, uint64(2), sess.renderCoordinator().resizeSnapshot().committed, | |
| 133 | + | "the retry belongs to the newest committed epoch") | |
| 135 | 134 | } | |
| 136 | 135 | ||
| 137 | 136 | // Stale lifecycle callbacks may never advance a transaction owned by a | |
| @@ -186,8 +185,24 @@ func TestTransactionalResizeObsoleteTimerCallbacksCommitOnlyLatestEpoch(t *testi | |||
| 186 | 185 | for _, timer := range timers { | |
| 187 | 186 | timer.ch <- time.Time{} | |
| 188 | 187 | } | |
| 188 | + | require.Eventually(t, func() bool { | |
| 189 | + | return len(pty.requested()) == 1 | |
| 190 | + | }, time.Second, time.Millisecond, "the newest timer callback must finish") | |
| 189 | 191 | require.Equal(t, []domain.Size{{Cols: 120, Rows: 30}}, pty.requested()) | |
| 190 | - | require.Equal(t, uint64(3), sess.renderCoordinator().resizeSnapshot().epoch) | |
| 192 | + | snapshot := sess.renderCoordinator().resizeSnapshot() | |
| 193 | + | require.Equal(t, uint64(3), snapshot.epoch) | |
| 194 | + | require.Equal(t, snapshot.epoch, snapshot.committed) | |
| 195 | + | // Flush the separately coalesced render invalidation; resize commits do not | |
| 196 | + | // bypass the S2 coordinator deadline. The transaction publishes committed | |
| 197 | + | // metadata immediately before enqueueing that invalidation, so observe the | |
| 198 | + | // enqueue explicitly rather than racing the callback goroutine. | |
| 199 | + | rc := sess.renderCoordinator() | |
| 200 | + | require.Eventually(t, func() bool { | |
| 201 | + | rc.mu.Lock() | |
| 202 | + | defer rc.mu.Unlock() | |
| 203 | + | return rc.pending | |
| 204 | + | }, time.Second, time.Millisecond) | |
| 205 | + | rc.fireCurrent(false) | |
| 191 | 206 | frame := <-frames | |
| 192 | 207 | require.Equal(t, ports.MsgOutput, frame.Type, "the committed epoch emits a full frame") | |
| 193 | 208 | select { | |
| @@ -126,12 +126,11 @@ func TestTransactionalResizeRetryTargetsNewestCommittedEpoch(t *testing.T) { | |||||
| 126 | 126 | ||||
| 127 | // The failed pane must not be retried at 49x28 from the obsolete epoch. | 127 | // The failed pane must not be retried at 49x28 from the obsolete epoch. | ||
| 128 | // S3 retries 59x32, then commits the parser/screen and requests a reset. | 128 | // S3 retries 59x32, then commits the parser/screen and requests a reset. | ||
| 129 | - | require.Equal(t, []domain.Size{{Cols: 59, Rows: 32}}, failed.requested()) | 129 | + | require.Equal(t, []domain.Size{{Cols: 49, Rows: 28}, {Cols: 59, Rows: 32}}, failed.requested(), |
| 130 | + | "the failed old apply is historical; only the authoritative newest epoch retries it") | |||
| 130 | require.Equal(t, domain.Size{Cols: 59, Rows: 32}, domain.Size{Cols: retry.screen.Frame.Width, Rows: retry.screen.Frame.Height}) | 131 | require.Equal(t, domain.Size{Cols: 59, Rows: 32}, domain.Size{Cols: retry.screen.Frame.Width, Rows: retry.screen.Frame.Height}) | ||
| 131 | - | ac.sendMu.Lock() | 132 | + | require.Equal(t, uint64(2), sess.renderCoordinator().resizeSnapshot().committed, |
| 132 | - | pending := ac.resizePaintPending | 133 | + | "the retry belongs to the newest committed epoch") |
| 133 | - | ac.sendMu.Unlock() | |||
| 134 | - | require.True(t, pending, "successful retry forces a final full frame") | |||
| 135 | } | 134 | } | ||
| 136 | 135 | ||||
| 137 | // Stale lifecycle callbacks may never advance a transaction owned by a | 136 | // Stale lifecycle callbacks may never advance a transaction owned by a | ||
| @@ -186,8 +185,24 @@ func TestTransactionalResizeObsoleteTimerCallbacksCommitOnlyLatestEpoch(t *testi | |||||
| 186 | for _, timer := range timers { | 185 | for _, timer := range timers { | ||
| 187 | timer.ch <- time.Time{} | 186 | timer.ch <- time.Time{} | ||
| 188 | } | 187 | } | ||
| 188 | + | require.Eventually(t, func() bool { | |||
| 189 | + | return len(pty.requested()) == 1 | |||
| 190 | + | }, time.Second, time.Millisecond, "the newest timer callback must finish") | |||
| 189 | require.Equal(t, []domain.Size{{Cols: 120, Rows: 30}}, pty.requested()) | 191 | require.Equal(t, []domain.Size{{Cols: 120, Rows: 30}}, pty.requested()) | ||
| 190 | - | require.Equal(t, uint64(3), sess.renderCoordinator().resizeSnapshot().epoch) | 192 | + | snapshot := sess.renderCoordinator().resizeSnapshot() |
| 193 | + | require.Equal(t, uint64(3), snapshot.epoch) | |||
| 194 | + | require.Equal(t, snapshot.epoch, snapshot.committed) | |||
| 195 | + | // Flush the separately coalesced render invalidation; resize commits do not | |||
| 196 | + | // bypass the S2 coordinator deadline. The transaction publishes committed | |||
| 197 | + | // metadata immediately before enqueueing that invalidation, so observe the | |||
| 198 | + | // enqueue explicitly rather than racing the callback goroutine. | |||
| 199 | + | rc := sess.renderCoordinator() | |||
| 200 | + | require.Eventually(t, func() bool { | |||
| 201 | + | rc.mu.Lock() | |||
| 202 | + | defer rc.mu.Unlock() | |||
| 203 | + | return rc.pending | |||
| 204 | + | }, time.Second, time.Millisecond) | |||
| 205 | + | rc.fireCurrent(false) | |||
| 191 | frame := <-frames | 206 | frame := <-frames | ||
| 192 | require.Equal(t, ports.MsgOutput, frame.Type, "the committed epoch emits a full frame") | 207 | require.Equal(t, ports.MsgOutput, frame.Type, "the committed epoch emits a full frame") | ||
| 193 | select { | 208 | select { | ||