fix(daemon): stabilize render coordinator lifecycle
- commit
- ea71bd49101d60292661bdd8d91740ed3d31e875
- author
- brice · · 2026-07-11 16:02 UTC
- parent
- 014aa3c
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +226 -13
M internal/usecase/daemon/copymode_test.go +38 -3
| @@ -5,6 +5,7 @@ import ( | |||
| 5 | 5 | "errors" | |
| 6 | 6 | "fmt" | |
| 7 | 7 | "io" | |
| 8 | + | "runtime" | |
| 8 | 9 | "strings" | |
| 9 | 10 | "sync" | |
| 10 | 11 | "testing" | |
| @@ -385,11 +386,45 @@ func TestScrollbackEvictionFeedsCopyModeYank(t *testing.T) { | |||
| 385 | 386 | p.EXPECT().Close().RunAndReturn(func() error { closeOnce.Do(func() { close(readDone) }); return nil }).Maybe() | |
| 386 | 387 | p.EXPECT().Pid().Return(4242).Maybe() | |
| 387 | 388 | ||
| 388 | - | d := newTestDaemon(t, newFactory(t, p), stubClock{}) | |
| 389 | + | // Rendering is coordinator-driven: advance a controllable clock through the | |
| 390 | + | // retained resize timer and its resulting coordinator wake rather than | |
| 391 | + | // relying on wall-clock debounce delivery. | |
| 392 | + | clk := &signalClock{timers: make(chan *signalTimer, 16)} | |
| 393 | + | d := newTestDaemon(t, newFactory(t, p), clk) | |
| 389 | 394 | tr, sends, releaseConn := newConn(t, mustHello(ports.IntentNew, "work", domain.Size{Cols: 16, Rows: 5})) | |
| 395 | + | advanceRender := func() { | |
| 396 | + | for range 4096 { | |
| 397 | + | select { | |
| 398 | + | case timer := <-clk.timers: | |
| 399 | + | timer.ch <- time.Time{} | |
| 400 | + | return | |
| 401 | + | default: | |
| 402 | + | runtime.Gosched() | |
| 403 | + | } | |
| 404 | + | } | |
| 405 | + | t.Fatal("coordinator did not arm a controllable render timer") | |
| 406 | + | } | |
| 407 | + | awaitControlledOutput := func() ports.Frame { | |
| 408 | + | for range 4096 { | |
| 409 | + | select { | |
| 410 | + | case frame := <-sends: | |
| 411 | + | if frame.Type == ports.MsgOutput { | |
| 412 | + | return frame | |
| 413 | + | } | |
| 414 | + | t.Fatalf("unexpected frame type %d while advancing render clock", frame.Type) | |
| 415 | + | case timer := <-clk.timers: | |
| 416 | + | timer.ch <- time.Time{} | |
| 417 | + | default: | |
| 418 | + | runtime.Gosched() | |
| 419 | + | } | |
| 420 | + | } | |
| 421 | + | t.Fatal("controllable timers did not produce an output frame") | |
| 422 | + | return ports.Frame{} | |
| 423 | + | } | |
| 390 | 424 | var hg sync.WaitGroup | |
| 391 | 425 | hg.Go(func() { d.handleConn(tr) }) | |
| 392 | 426 | awaitFrame(t, sends, ports.MsgWelcome) | |
| 427 | + | advanceRender() // initial coordinator invalidation | |
| 393 | 428 | awaitFrame(t, sends, ports.MsgOutput) | |
| 394 | 429 | ||
| 395 | 430 | for i := range 12 { | |
| @@ -420,9 +455,9 @@ func TestScrollbackEvictionFeedsCopyModeYank(t *testing.T) { | |||
| 420 | 455 | ac := sess.client | |
| 421 | 456 | require.NotNil(t, ac) | |
| 422 | 457 | d.handleInput(sess, ac, []byte("\x1b ")) | |
| 423 | - | awaitFrame(t, sends, ports.MsgOutput) | |
| 458 | + | awaitControlledOutput() | |
| 424 | 459 | d.handleInput(sess, ac, []byte("VIS\r")) | |
| 425 | - | awaitFrame(t, sends, ports.MsgOutput) | |
| 460 | + | awaitControlledOutput() | |
| 426 | 461 | d.handleInput(sess, ac, []byte{'g', ' ', 'G', 'y'}) | |
| 427 | 462 | ||
| 428 | 463 | var payload string | |
| @@ -5,6 +5,7 @@ import ( | |||||
| 5 | "errors" | 5 | "errors" | ||
| 6 | "fmt" | 6 | "fmt" | ||
| 7 | "io" | 7 | "io" | ||
| 8 | + | "runtime" | |||
| 8 | "strings" | 9 | "strings" | ||
| 9 | "sync" | 10 | "sync" | ||
| 10 | "testing" | 11 | "testing" | ||
| @@ -385,11 +386,45 @@ func TestScrollbackEvictionFeedsCopyModeYank(t *testing.T) { | |||||
| 385 | p.EXPECT().Close().RunAndReturn(func() error { closeOnce.Do(func() { close(readDone) }); return nil }).Maybe() | 386 | p.EXPECT().Close().RunAndReturn(func() error { closeOnce.Do(func() { close(readDone) }); return nil }).Maybe() | ||
| 386 | p.EXPECT().Pid().Return(4242).Maybe() | 387 | p.EXPECT().Pid().Return(4242).Maybe() | ||
| 387 | 388 | ||||
| 388 | - | d := newTestDaemon(t, newFactory(t, p), stubClock{}) | 389 | + | // Rendering is coordinator-driven: advance a controllable clock through the |
| 390 | + | // retained resize timer and its resulting coordinator wake rather than | |||
| 391 | + | // relying on wall-clock debounce delivery. | |||
| 392 | + | clk := &signalClock{timers: make(chan *signalTimer, 16)} | |||
| 393 | + | d := newTestDaemon(t, newFactory(t, p), clk) | |||
| 389 | tr, sends, releaseConn := newConn(t, mustHello(ports.IntentNew, "work", domain.Size{Cols: 16, Rows: 5})) | 394 | tr, sends, releaseConn := newConn(t, mustHello(ports.IntentNew, "work", domain.Size{Cols: 16, Rows: 5})) | ||
| 395 | + | advanceRender := func() { | |||
| 396 | + | for range 4096 { | |||
| 397 | + | select { | |||
| 398 | + | case timer := <-clk.timers: | |||
| 399 | + | timer.ch <- time.Time{} | |||
| 400 | + | return | |||
| 401 | + | default: | |||
| 402 | + | runtime.Gosched() | |||
| 403 | + | } | |||
| 404 | + | } | |||
| 405 | + | t.Fatal("coordinator did not arm a controllable render timer") | |||
| 406 | + | } | |||
| 407 | + | awaitControlledOutput := func() ports.Frame { | |||
| 408 | + | for range 4096 { | |||
| 409 | + | select { | |||
| 410 | + | case frame := <-sends: | |||
| 411 | + | if frame.Type == ports.MsgOutput { | |||
| 412 | + | return frame | |||
| 413 | + | } | |||
| 414 | + | t.Fatalf("unexpected frame type %d while advancing render clock", frame.Type) | |||
| 415 | + | case timer := <-clk.timers: | |||
| 416 | + | timer.ch <- time.Time{} | |||
| 417 | + | default: | |||
| 418 | + | runtime.Gosched() | |||
| 419 | + | } | |||
| 420 | + | } | |||
| 421 | + | t.Fatal("controllable timers did not produce an output frame") | |||
| 422 | + | return ports.Frame{} | |||
| 423 | + | } | |||
| 390 | var hg sync.WaitGroup | 424 | var hg sync.WaitGroup | ||
| 391 | hg.Go(func() { d.handleConn(tr) }) | 425 | hg.Go(func() { d.handleConn(tr) }) | ||
| 392 | awaitFrame(t, sends, ports.MsgWelcome) | 426 | awaitFrame(t, sends, ports.MsgWelcome) | ||
| 427 | + | advanceRender() // initial coordinator invalidation | |||
| 393 | awaitFrame(t, sends, ports.MsgOutput) | 428 | awaitFrame(t, sends, ports.MsgOutput) | ||
| 394 | 429 | ||||
| 395 | for i := range 12 { | 430 | for i := range 12 { | ||
| @@ -420,9 +455,9 @@ func TestScrollbackEvictionFeedsCopyModeYank(t *testing.T) { | |||||
| 420 | ac := sess.client | 455 | ac := sess.client | ||
| 421 | require.NotNil(t, ac) | 456 | require.NotNil(t, ac) | ||
| 422 | d.handleInput(sess, ac, []byte("\x1b ")) | 457 | d.handleInput(sess, ac, []byte("\x1b ")) | ||
| 423 | - | awaitFrame(t, sends, ports.MsgOutput) | 458 | + | awaitControlledOutput() |
| 424 | d.handleInput(sess, ac, []byte("VIS\r")) | 459 | d.handleInput(sess, ac, []byte("VIS\r")) | ||
| 425 | - | awaitFrame(t, sends, ports.MsgOutput) | 460 | + | awaitControlledOutput() |
| 426 | d.handleInput(sess, ac, []byte{'g', ' ', 'G', 'y'}) | 461 | d.handleInput(sess, ac, []byte{'g', ' ', 'G', 'y'}) | ||
| 427 | 462 | ||||
| 428 | var payload string | 463 | var payload string | ||
M internal/usecase/daemon/render.go +4 -1
| @@ -182,7 +182,6 @@ func (d *Daemon) paintForResizeGeneration(sess *session, ac *attachedClient, res | |||
| 182 | 182 | return | |
| 183 | 183 | } | |
| 184 | 184 | ||
| 185 | - | ac.initOverlays() | |
| 186 | 185 | ac.sendMu.Lock() | |
| 187 | 186 | if ac.currentSession() != sess { | |
| 188 | 187 | ac.sendMu.Unlock() | |
| @@ -192,6 +191,10 @@ func (d *Daemon) paintForResizeGeneration(sess *session, ac *attachedClient, res | |||
| 192 | 191 | ac.sendMu.Unlock() | |
| 193 | 192 | return | |
| 194 | 193 | } | |
| 194 | + | // Composition owns attachment sendMu; initialize its lazy overlay state | |
| 195 | + | // under that same ownership so concurrent fallback paints cannot observe a | |
| 196 | + | // partially published runtime. | |
| 197 | + | ac.initOverlays() | |
| 195 | 198 | if ac.resizePaintPending { | |
| 196 | 199 | ac.resizePaint.stop() | |
| 197 | 200 | ac.resizePaintPending = false | |
| @@ -182,7 +182,6 @@ func (d *Daemon) paintForResizeGeneration(sess *session, ac *attachedClient, res | |||||
| 182 | return | 182 | return | ||
| 183 | } | 183 | } | ||
| 184 | 184 | ||||
| 185 | - | ac.initOverlays() | |||
| 186 | ac.sendMu.Lock() | 185 | ac.sendMu.Lock() | ||
| 187 | if ac.currentSession() != sess { | 186 | if ac.currentSession() != sess { | ||
| 188 | ac.sendMu.Unlock() | 187 | ac.sendMu.Unlock() | ||
| @@ -192,6 +191,10 @@ func (d *Daemon) paintForResizeGeneration(sess *session, ac *attachedClient, res | |||||
| 192 | ac.sendMu.Unlock() | 191 | ac.sendMu.Unlock() | ||
| 193 | return | 192 | return | ||
| 194 | } | 193 | } | ||
| 194 | + | // Composition owns attachment sendMu; initialize its lazy overlay state | |||
| 195 | + | // under that same ownership so concurrent fallback paints cannot observe a | |||
| 196 | + | // partially published runtime. | |||
| 197 | + | ac.initOverlays() | |||
| 195 | if ac.resizePaintPending { | 198 | if ac.resizePaintPending { | ||
| 196 | ac.resizePaint.stop() | 199 | ac.resizePaint.stop() | ||
| 197 | ac.resizePaintPending = false | 200 | ac.resizePaintPending = false | ||
M internal/usecase/daemon/render_coordinator.go +74 -8
| @@ -115,10 +115,19 @@ type renderCoordinator struct { | |||
| 115 | 115 | resize resizeRequestMetadata | |
| 116 | 116 | metrics renderCoordinatorBurstMetrics | |
| 117 | 117 | // generation invalidates callbacks from superseded deadline/watchdog timers. | |
| 118 | - | generation uint64 | |
| 119 | - | armed bool | |
| 120 | - | syncGeneration uint64 | |
| 121 | - | syncWatchdog func() | |
| 118 | + | // Each worker also owns an explicit cancellation channel: fake timers may | |
| 119 | + | // expose a nil or never-firing C channel, so Stop alone cannot release a | |
| 120 | + | // worker waiting in select. | |
| 121 | + | generation uint64 | |
| 122 | + | armed bool | |
| 123 | + | normalTimer ports.Timer | |
| 124 | + | normalCancel chan struct{} | |
| 125 | + | normalWorkerDone chan struct{} | |
| 126 | + | syncGeneration uint64 | |
| 127 | + | syncWatchdog func() | |
| 128 | + | syncTimer ports.Timer | |
| 129 | + | syncCancel chan struct{} | |
| 130 | + | syncWorkerDone chan struct{} | |
| 122 | 131 | } | |
| 123 | 132 | ||
| 124 | 133 | // newRenderCoordinator constructs a coordinator bound to opts. It starts no | |
| @@ -127,6 +136,30 @@ func newRenderCoordinator(opts renderCoordinatorOptions) *renderCoordinator { | |||
| 127 | 136 | return &renderCoordinator{opts: opts} | |
| 128 | 137 | } | |
| 129 | 138 | ||
| 139 | + | // cancelNormalTimerLocked and cancelSyncTimerLocked release timer workers even | |
| 140 | + | // when a test clock supplies an inert channel. Caller holds c.mu. | |
| 141 | + | func (c *renderCoordinator) cancelNormalTimerLocked() { | |
| 142 | + | if c.normalTimer != nil { | |
| 143 | + | c.normalTimer.Stop() | |
| 144 | + | c.normalTimer = nil | |
| 145 | + | } | |
| 146 | + | if c.normalCancel != nil { | |
| 147 | + | close(c.normalCancel) | |
| 148 | + | c.normalCancel = nil | |
| 149 | + | } | |
| 150 | + | } | |
| 151 | + | ||
| 152 | + | func (c *renderCoordinator) cancelSyncTimerLocked() { | |
| 153 | + | if c.syncTimer != nil { | |
| 154 | + | c.syncTimer.Stop() | |
| 155 | + | c.syncTimer = nil | |
| 156 | + | } | |
| 157 | + | if c.syncCancel != nil { | |
| 158 | + | close(c.syncCancel) | |
| 159 | + | c.syncCancel = nil | |
| 160 | + | } | |
| 161 | + | } | |
| 162 | + | ||
| 130 | 163 | // invalidate publishes one producer state transition. Consecutive | |
| 131 | 164 | // invalidations coalesce under a single armed deadline; the latest state and | |
| 132 | 165 | // the stickiest reset win. | |
| @@ -149,6 +182,7 @@ func (c *renderCoordinator) invalidate(inv renderInvalidation) { | |||
| 149 | 182 | arm := !wasPending || (!wasUrgent && c.pendingUrgent) | |
| 150 | 183 | if arm { | |
| 151 | 184 | c.generation++ | |
| 185 | + | c.cancelNormalTimerLocked() | |
| 152 | 186 | } | |
| 153 | 187 | gen := c.generation | |
| 154 | 188 | delay := minOutputRenderDeadline | |
| @@ -157,12 +191,23 @@ func (c *renderCoordinator) invalidate(inv renderInvalidation) { | |||
| 157 | 191 | } | |
| 158 | 192 | clock := c.opts.clock | |
| 159 | 193 | c.armed = c.armed || arm | |
| 160 | - | c.mu.Unlock() | |
| 161 | 194 | if !arm || clock == nil { | |
| 195 | + | c.mu.Unlock() | |
| 162 | 196 | return | |
| 163 | 197 | } | |
| 164 | 198 | timer := clock.NewTimer(delay) | |
| 165 | - | go func() { <-timer.C(); c.fire(gen, false) }() | |
| 199 | + | cancel := make(chan struct{}) | |
| 200 | + | done := make(chan struct{}) | |
| 201 | + | c.normalTimer, c.normalCancel, c.normalWorkerDone = timer, cancel, done | |
| 202 | + | c.mu.Unlock() | |
| 203 | + | go func() { | |
| 204 | + | defer close(done) | |
| 205 | + | select { | |
| 206 | + | case <-timer.C(): | |
| 207 | + | c.fire(gen, false) | |
| 208 | + | case <-cancel: | |
| 209 | + | } | |
| 210 | + | }() | |
| 166 | 211 | // Give fake-clock callbacks a chance to block on C before the test | |
| 167 | 212 | // advances it; real clocks are unaffected. | |
| 168 | 213 | runtime.Gosched() | |
| @@ -186,25 +231,36 @@ func (c *renderCoordinator) noteSyncBegin(gen uint64, force ...func()) { | |||
| 186 | 231 | c.mu.Unlock() | |
| 187 | 232 | return | |
| 188 | 233 | } | |
| 234 | + | c.cancelSyncTimerLocked() | |
| 189 | 235 | c.syncGeneration = gen | |
| 190 | 236 | c.syncWatchdog = nil | |
| 191 | 237 | if len(force) != 0 { | |
| 192 | 238 | c.syncWatchdog = force[0] | |
| 193 | 239 | } | |
| 194 | 240 | clock := c.opts.clock | |
| 195 | - | c.mu.Unlock() | |
| 196 | 241 | if clock == nil { | |
| 242 | + | c.mu.Unlock() | |
| 197 | 243 | return | |
| 198 | 244 | } | |
| 199 | 245 | timer := clock.NewTimer(maxSyncUpdateDuration) | |
| 246 | + | cancel := make(chan struct{}) | |
| 247 | + | done := make(chan struct{}) | |
| 248 | + | c.syncTimer, c.syncCancel, c.syncWorkerDone = timer, cancel, done | |
| 249 | + | c.mu.Unlock() | |
| 200 | 250 | go func() { | |
| 201 | - | <-timer.C() | |
| 251 | + | defer close(done) | |
| 252 | + | select { | |
| 253 | + | case <-timer.C(): | |
| 254 | + | case <-cancel: | |
| 255 | + | return | |
| 256 | + | } | |
| 202 | 257 | c.mu.Lock() | |
| 203 | 258 | valid := !c.torndown && c.syncGeneration == gen | |
| 204 | 259 | force := c.syncWatchdog | |
| 205 | 260 | if valid { | |
| 206 | 261 | c.syncGeneration = 0 | |
| 207 | 262 | c.syncWatchdog = nil | |
| 263 | + | c.cancelSyncTimerLocked() | |
| 208 | 264 | } | |
| 209 | 265 | c.mu.Unlock() | |
| 210 | 266 | if valid { | |
| @@ -226,6 +282,7 @@ func (c *renderCoordinator) noteSyncEnd(gen uint64) { | |||
| 226 | 282 | } | |
| 227 | 283 | c.syncGeneration = 0 | |
| 228 | 284 | c.syncWatchdog = nil | |
| 285 | + | c.cancelSyncTimerLocked() | |
| 229 | 286 | c.mu.Unlock() | |
| 230 | 287 | c.fireCurrent(false) | |
| 231 | 288 | } | |
| @@ -257,6 +314,9 @@ func (c *renderCoordinator) noteDetach(ac *attachedClient) { | |||
| 257 | 314 | c.attachment = nil | |
| 258 | 315 | c.pending = false | |
| 259 | 316 | c.generation++ | |
| 317 | + | c.armed = false | |
| 318 | + | c.cancelNormalTimerLocked() | |
| 319 | + | c.cancelSyncTimerLocked() | |
| 260 | 320 | } | |
| 261 | 321 | c.mu.Unlock() | |
| 262 | 322 | } | |
| @@ -269,6 +329,9 @@ func (c *renderCoordinator) noteReplace(old, replacement *attachedClient) { | |||
| 269 | 329 | c.attachment = replacement | |
| 270 | 330 | c.pending = false | |
| 271 | 331 | c.generation++ | |
| 332 | + | c.armed = false | |
| 333 | + | c.cancelNormalTimerLocked() | |
| 334 | + | c.cancelSyncTimerLocked() | |
| 272 | 335 | } | |
| 273 | 336 | c.mu.Unlock() | |
| 274 | 337 | } | |
| @@ -283,6 +346,9 @@ func (c *renderCoordinator) noteSessionTeardown() { | |||
| 283 | 346 | c.attachment = nil | |
| 284 | 347 | c.pending = false | |
| 285 | 348 | c.generation++ | |
| 349 | + | c.armed = false | |
| 350 | + | c.cancelNormalTimerLocked() | |
| 351 | + | c.cancelSyncTimerLocked() | |
| 286 | 352 | c.mu.Unlock() | |
| 287 | 353 | } | |
| 288 | 354 | ||
| @@ -115,10 +115,19 @@ type renderCoordinator struct { | |||||
| 115 | resize resizeRequestMetadata | 115 | resize resizeRequestMetadata | ||
| 116 | metrics renderCoordinatorBurstMetrics | 116 | metrics renderCoordinatorBurstMetrics | ||
| 117 | // generation invalidates callbacks from superseded deadline/watchdog timers. | 117 | // generation invalidates callbacks from superseded deadline/watchdog timers. | ||
| 118 | - | generation uint64 | 118 | + | // Each worker also owns an explicit cancellation channel: fake timers may |
| 119 | - | armed bool | 119 | + | // expose a nil or never-firing C channel, so Stop alone cannot release a |
| 120 | - | syncGeneration uint64 | 120 | + | // worker waiting in select. |
| 121 | - | syncWatchdog func() | 121 | + | generation uint64 |
| 122 | + | armed bool | |||
| 123 | + | normalTimer ports.Timer | |||
| 124 | + | normalCancel chan struct{} | |||
| 125 | + | normalWorkerDone chan struct{} | |||
| 126 | + | syncGeneration uint64 | |||
| 127 | + | syncWatchdog func() | |||
| 128 | + | syncTimer ports.Timer | |||
| 129 | + | syncCancel chan struct{} | |||
| 130 | + | syncWorkerDone chan struct{} | |||
| 122 | } | 131 | } | ||
| 123 | 132 | ||||
| 124 | // newRenderCoordinator constructs a coordinator bound to opts. It starts no | 133 | // newRenderCoordinator constructs a coordinator bound to opts. It starts no | ||
| @@ -127,6 +136,30 @@ func newRenderCoordinator(opts renderCoordinatorOptions) *renderCoordinator { | |||||
| 127 | return &renderCoordinator{opts: opts} | 136 | return &renderCoordinator{opts: opts} | ||
| 128 | } | 137 | } | ||
| 129 | 138 | ||||
| 139 | + | // cancelNormalTimerLocked and cancelSyncTimerLocked release timer workers even | |||
| 140 | + | // when a test clock supplies an inert channel. Caller holds c.mu. | |||
| 141 | + | func (c *renderCoordinator) cancelNormalTimerLocked() { | |||
| 142 | + | if c.normalTimer != nil { | |||
| 143 | + | c.normalTimer.Stop() | |||
| 144 | + | c.normalTimer = nil | |||
| 145 | + | } | |||
| 146 | + | if c.normalCancel != nil { | |||
| 147 | + | close(c.normalCancel) | |||
| 148 | + | c.normalCancel = nil | |||
| 149 | + | } | |||
| 150 | + | } | |||
| 151 | + | ||||
| 152 | + | func (c *renderCoordinator) cancelSyncTimerLocked() { | |||
| 153 | + | if c.syncTimer != nil { | |||
| 154 | + | c.syncTimer.Stop() | |||
| 155 | + | c.syncTimer = nil | |||
| 156 | + | } | |||
| 157 | + | if c.syncCancel != nil { | |||
| 158 | + | close(c.syncCancel) | |||
| 159 | + | c.syncCancel = nil | |||
| 160 | + | } | |||
| 161 | + | } | |||
| 162 | + | ||||
| 130 | // invalidate publishes one producer state transition. Consecutive | 163 | // invalidate publishes one producer state transition. Consecutive | ||
| 131 | // invalidations coalesce under a single armed deadline; the latest state and | 164 | // invalidations coalesce under a single armed deadline; the latest state and | ||
| 132 | // the stickiest reset win. | 165 | // the stickiest reset win. | ||
| @@ -149,6 +182,7 @@ func (c *renderCoordinator) invalidate(inv renderInvalidation) { | |||||
| 149 | arm := !wasPending || (!wasUrgent && c.pendingUrgent) | 182 | arm := !wasPending || (!wasUrgent && c.pendingUrgent) | ||
| 150 | if arm { | 183 | if arm { | ||
| 151 | c.generation++ | 184 | c.generation++ | ||
| 185 | + | c.cancelNormalTimerLocked() | |||
| 152 | } | 186 | } | ||
| 153 | gen := c.generation | 187 | gen := c.generation | ||
| 154 | delay := minOutputRenderDeadline | 188 | delay := minOutputRenderDeadline | ||
| @@ -157,12 +191,23 @@ func (c *renderCoordinator) invalidate(inv renderInvalidation) { | |||||
| 157 | } | 191 | } | ||
| 158 | clock := c.opts.clock | 192 | clock := c.opts.clock | ||
| 159 | c.armed = c.armed || arm | 193 | c.armed = c.armed || arm | ||
| 160 | - | c.mu.Unlock() | |||
| 161 | if !arm || clock == nil { | 194 | if !arm || clock == nil { | ||
| 195 | + | c.mu.Unlock() | |||
| 162 | return | 196 | return | ||
| 163 | } | 197 | } | ||
| 164 | timer := clock.NewTimer(delay) | 198 | timer := clock.NewTimer(delay) | ||
| 165 | - | go func() { <-timer.C(); c.fire(gen, false) }() | 199 | + | cancel := make(chan struct{}) |
| 200 | + | done := make(chan struct{}) | |||
| 201 | + | c.normalTimer, c.normalCancel, c.normalWorkerDone = timer, cancel, done | |||
| 202 | + | c.mu.Unlock() | |||
| 203 | + | go func() { | |||
| 204 | + | defer close(done) | |||
| 205 | + | select { | |||
| 206 | + | case <-timer.C(): | |||
| 207 | + | c.fire(gen, false) | |||
| 208 | + | case <-cancel: | |||
| 209 | + | } | |||
| 210 | + | }() | |||
| 166 | // Give fake-clock callbacks a chance to block on C before the test | 211 | // Give fake-clock callbacks a chance to block on C before the test | ||
| 167 | // advances it; real clocks are unaffected. | 212 | // advances it; real clocks are unaffected. | ||
| 168 | runtime.Gosched() | 213 | runtime.Gosched() | ||
| @@ -186,25 +231,36 @@ func (c *renderCoordinator) noteSyncBegin(gen uint64, force ...func()) { | |||||
| 186 | c.mu.Unlock() | 231 | c.mu.Unlock() | ||
| 187 | return | 232 | return | ||
| 188 | } | 233 | } | ||
| 234 | + | c.cancelSyncTimerLocked() | |||
| 189 | c.syncGeneration = gen | 235 | c.syncGeneration = gen | ||
| 190 | c.syncWatchdog = nil | 236 | c.syncWatchdog = nil | ||
| 191 | if len(force) != 0 { | 237 | if len(force) != 0 { | ||
| 192 | c.syncWatchdog = force[0] | 238 | c.syncWatchdog = force[0] | ||
| 193 | } | 239 | } | ||
| 194 | clock := c.opts.clock | 240 | clock := c.opts.clock | ||
| 195 | - | c.mu.Unlock() | |||
| 196 | if clock == nil { | 241 | if clock == nil { | ||
| 242 | + | c.mu.Unlock() | |||
| 197 | return | 243 | return | ||
| 198 | } | 244 | } | ||
| 199 | timer := clock.NewTimer(maxSyncUpdateDuration) | 245 | timer := clock.NewTimer(maxSyncUpdateDuration) | ||
| 246 | + | cancel := make(chan struct{}) | |||
| 247 | + | done := make(chan struct{}) | |||
| 248 | + | c.syncTimer, c.syncCancel, c.syncWorkerDone = timer, cancel, done | |||
| 249 | + | c.mu.Unlock() | |||
| 200 | go func() { | 250 | go func() { | ||
| 201 | - | <-timer.C() | 251 | + | defer close(done) |
| 252 | + | select { | |||
| 253 | + | case <-timer.C(): | |||
| 254 | + | case <-cancel: | |||
| 255 | + | return | |||
| 256 | + | } | |||
| 202 | c.mu.Lock() | 257 | c.mu.Lock() | ||
| 203 | valid := !c.torndown && c.syncGeneration == gen | 258 | valid := !c.torndown && c.syncGeneration == gen | ||
| 204 | force := c.syncWatchdog | 259 | force := c.syncWatchdog | ||
| 205 | if valid { | 260 | if valid { | ||
| 206 | c.syncGeneration = 0 | 261 | c.syncGeneration = 0 | ||
| 207 | c.syncWatchdog = nil | 262 | c.syncWatchdog = nil | ||
| 263 | + | c.cancelSyncTimerLocked() | |||
| 208 | } | 264 | } | ||
| 209 | c.mu.Unlock() | 265 | c.mu.Unlock() | ||
| 210 | if valid { | 266 | if valid { | ||
| @@ -226,6 +282,7 @@ func (c *renderCoordinator) noteSyncEnd(gen uint64) { | |||||
| 226 | } | 282 | } | ||
| 227 | c.syncGeneration = 0 | 283 | c.syncGeneration = 0 | ||
| 228 | c.syncWatchdog = nil | 284 | c.syncWatchdog = nil | ||
| 285 | + | c.cancelSyncTimerLocked() | |||
| 229 | c.mu.Unlock() | 286 | c.mu.Unlock() | ||
| 230 | c.fireCurrent(false) | 287 | c.fireCurrent(false) | ||
| 231 | } | 288 | } | ||
| @@ -257,6 +314,9 @@ func (c *renderCoordinator) noteDetach(ac *attachedClient) { | |||||
| 257 | c.attachment = nil | 314 | c.attachment = nil | ||
| 258 | c.pending = false | 315 | c.pending = false | ||
| 259 | c.generation++ | 316 | c.generation++ | ||
| 317 | + | c.armed = false | |||
| 318 | + | c.cancelNormalTimerLocked() | |||
| 319 | + | c.cancelSyncTimerLocked() | |||
| 260 | } | 320 | } | ||
| 261 | c.mu.Unlock() | 321 | c.mu.Unlock() | ||
| 262 | } | 322 | } | ||
| @@ -269,6 +329,9 @@ func (c *renderCoordinator) noteReplace(old, replacement *attachedClient) { | |||||
| 269 | c.attachment = replacement | 329 | c.attachment = replacement | ||
| 270 | c.pending = false | 330 | c.pending = false | ||
| 271 | c.generation++ | 331 | c.generation++ | ||
| 332 | + | c.armed = false | |||
| 333 | + | c.cancelNormalTimerLocked() | |||
| 334 | + | c.cancelSyncTimerLocked() | |||
| 272 | } | 335 | } | ||
| 273 | c.mu.Unlock() | 336 | c.mu.Unlock() | ||
| 274 | } | 337 | } | ||
| @@ -283,6 +346,9 @@ func (c *renderCoordinator) noteSessionTeardown() { | |||||
| 283 | c.attachment = nil | 346 | c.attachment = nil | ||
| 284 | c.pending = false | 347 | c.pending = false | ||
| 285 | c.generation++ | 348 | c.generation++ | ||
| 349 | + | c.armed = false | |||
| 350 | + | c.cancelNormalTimerLocked() | |||
| 351 | + | c.cancelSyncTimerLocked() | |||
| 286 | c.mu.Unlock() | 352 | c.mu.Unlock() | ||
| 287 | } | 353 | } | ||
| 288 | 354 | ||||
M internal/usecase/daemon/render_coordinator_test.go +108 -0
| @@ -4,6 +4,7 @@ import ( | |||
| 4 | 4 | "os" | |
| 5 | 5 | "path/filepath" | |
| 6 | 6 | "runtime" | |
| 7 | + | "sync" | |
| 7 | 8 | "sync/atomic" | |
| 8 | 9 | "testing" | |
| 9 | 10 | "time" | |
| @@ -638,6 +639,113 @@ func TestProducerInvalidationInventory(t *testing.T) { | |||
| 638 | 639 | ||
| 639 | 640 | // --- retained PR #71 resize dispatch ---------------------------------------------- | |
| 640 | 641 | ||
| 642 | + | func TestConcurrentPaintInitializesOverlayUnderSendOwnership(t *testing.T) { | |
| 643 | + | p, release := newBlockingPTY(t) | |
| 644 | + | defer release() | |
| 645 | + | d, sess, ac, _ := newManualSessionWithPTYs(t, p) | |
| 646 | + | // Exercise the original seam: two fallback paints reach lazy initialization | |
| 647 | + | // together before either can compose. | |
| 648 | + | ac.overlays = nil | |
| 649 | + | ac.overlayOnce = sync.Once{} | |
| 650 | + | start := make(chan struct{}) | |
| 651 | + | var wg sync.WaitGroup | |
| 652 | + | wg.Add(2) | |
| 653 | + | for range 2 { | |
| 654 | + | go func() { | |
| 655 | + | defer wg.Done() | |
| 656 | + | <-start | |
| 657 | + | d.paint(sess, ac, true) | |
| 658 | + | }() | |
| 659 | + | } | |
| 660 | + | close(start) | |
| 661 | + | wg.Wait() | |
| 662 | + | require.NotNil(t, ac.overlays) | |
| 663 | + | } | |
| 664 | + | ||
| 665 | + | func TestStartPaneGoroutinesAccountsForOneReader(t *testing.T) { | |
| 666 | + | p, release := newBlockingPTY(t) | |
| 667 | + | d, sess, _, _ := newManualSessionWithPTYs(t, p) | |
| 668 | + | tb := sess.activeTab() | |
| 669 | + | require.NotNil(t, tb) | |
| 670 | + | d.startPaneGoroutines(sess, tb, tb.focusedPane()) | |
| 671 | + | release() | |
| 672 | + | select { | |
| 673 | + | case <-waitGroupDone(&d.sessWg): | |
| 674 | + | case <-time.After(time.Second): | |
| 675 | + | t.Fatal("one reader must balance exactly one WaitGroup count") | |
| 676 | + | } | |
| 677 | + | } | |
| 678 | + | ||
| 679 | + | // inertTimer deliberately has a nil C channel. It verifies coordinator | |
| 680 | + | // lifecycle paths do not strand workers behind fake clocks that never fire. | |
| 681 | + | type inertTimer struct{ stopped atomic.Bool } | |
| 682 | + | ||
| 683 | + | func (t *inertTimer) C() <-chan time.Time { return nil } | |
| 684 | + | func (t *inertTimer) Reset(time.Duration) bool { return false } | |
| 685 | + | func (t *inertTimer) Stop() bool { t.stopped.Store(true); return true } | |
| 686 | + | ||
| 687 | + | type inertClock struct{ timers chan *inertTimer } | |
| 688 | + | ||
| 689 | + | func (c *inertClock) Now() time.Time { return time.Time{} } | |
| 690 | + | func (c *inertClock) NewTimer(time.Duration) ports.Timer { | |
| 691 | + | t := &inertTimer{} | |
| 692 | + | c.timers <- t | |
| 693 | + | return t | |
| 694 | + | } | |
| 695 | + | ||
| 696 | + | func requireWorkerExit(t *testing.T, done <-chan struct{}) { | |
| 697 | + | t.Helper() | |
| 698 | + | for range 4096 { | |
| 699 | + | select { | |
| 700 | + | case <-done: | |
| 701 | + | return | |
| 702 | + | default: | |
| 703 | + | runtime.Gosched() | |
| 704 | + | } | |
| 705 | + | } | |
| 706 | + | t.Fatal("cancelled coordinator timer worker did not exit") | |
| 707 | + | } | |
| 708 | + | ||
| 709 | + | func TestRenderCoordinatorStopsInertTimerWorkers(t *testing.T) { | |
| 710 | + | lifecycle := []struct { | |
| 711 | + | name string | |
| 712 | + | end func(*renderCoordinator, *attachedClient) | |
| 713 | + | }{ | |
| 714 | + | {"detach", func(rc *renderCoordinator, ac *attachedClient) { rc.noteDetach(ac) }}, | |
| 715 | + | {"park", func(rc *renderCoordinator, ac *attachedClient) { rc.notePark(ac) }}, | |
| 716 | + | {"teardown", func(rc *renderCoordinator, _ *attachedClient) { rc.noteSessionTeardown() }}, | |
| 717 | + | } | |
| 718 | + | for _, tc := range lifecycle { | |
| 719 | + | t.Run(tc.name, func(t *testing.T) { | |
| 720 | + | clk := &inertClock{timers: make(chan *inertTimer, 2)} | |
| 721 | + | rc := newRenderCoordinator(renderCoordinatorOptions{clock: clk}) | |
| 722 | + | ac := &attachedClient{} | |
| 723 | + | rc.attach(ac) | |
| 724 | + | rc.invalidate(renderInvalidation{class: invalidateOutput}) | |
| 725 | + | normal := <-clk.timers | |
| 726 | + | rc.mu.Lock() | |
| 727 | + | normalDone := rc.normalWorkerDone | |
| 728 | + | rc.mu.Unlock() | |
| 729 | + | tc.end(rc, ac) | |
| 730 | + | require.True(t, normal.stopped.Load(), "normal timer must be stopped") | |
| 731 | + | requireWorkerExit(t, normalDone) | |
| 732 | + | ||
| 733 | + | // Use a fresh coordinator because detach/teardown intentionally makes | |
| 734 | + | // an attachment ineligible for a new synchronized batch. | |
| 735 | + | rc = newRenderCoordinator(renderCoordinatorOptions{clock: clk}) | |
| 736 | + | rc.attach(ac) | |
| 737 | + | rc.noteSyncBegin(1) | |
| 738 | + | syncTimer := <-clk.timers | |
| 739 | + | rc.mu.Lock() | |
| 740 | + | syncDone := rc.syncWorkerDone | |
| 741 | + | rc.mu.Unlock() | |
| 742 | + | tc.end(rc, ac) | |
| 743 | + | require.True(t, syncTimer.stopped.Load(), "sync watchdog must be stopped") | |
| 744 | + | requireWorkerExit(t, syncDone) | |
| 745 | + | }) | |
| 746 | + | } | |
| 747 | + | } | |
| 748 | + | ||
| 641 | 749 | func TestRenderCoordinatorRetainsPR71ResizeDispatch(t *testing.T) { | |
| 642 | 750 | newResizeFixture := func(t *testing.T) (*Daemon, *session, *attachedClient, chan ports.Frame, *signalClock, chan renderInvalidation) { | |
| 643 | 751 | t.Helper() | |
| @@ -4,6 +4,7 @@ import ( | |||||
| 4 | "os" | 4 | "os" | ||
| 5 | "path/filepath" | 5 | "path/filepath" | ||
| 6 | "runtime" | 6 | "runtime" | ||
| 7 | + | "sync" | |||
| 7 | "sync/atomic" | 8 | "sync/atomic" | ||
| 8 | "testing" | 9 | "testing" | ||
| 9 | "time" | 10 | "time" | ||
| @@ -638,6 +639,113 @@ func TestProducerInvalidationInventory(t *testing.T) { | |||||
| 638 | 639 | ||||
| 639 | // --- retained PR #71 resize dispatch ---------------------------------------------- | 640 | // --- retained PR #71 resize dispatch ---------------------------------------------- | ||
| 640 | 641 | ||||
| 642 | + | func TestConcurrentPaintInitializesOverlayUnderSendOwnership(t *testing.T) { | |||
| 643 | + | p, release := newBlockingPTY(t) | |||
| 644 | + | defer release() | |||
| 645 | + | d, sess, ac, _ := newManualSessionWithPTYs(t, p) | |||
| 646 | + | // Exercise the original seam: two fallback paints reach lazy initialization | |||
| 647 | + | // together before either can compose. | |||
| 648 | + | ac.overlays = nil | |||
| 649 | + | ac.overlayOnce = sync.Once{} | |||
| 650 | + | start := make(chan struct{}) | |||
| 651 | + | var wg sync.WaitGroup | |||
| 652 | + | wg.Add(2) | |||
| 653 | + | for range 2 { | |||
| 654 | + | go func() { | |||
| 655 | + | defer wg.Done() | |||
| 656 | + | <-start | |||
| 657 | + | d.paint(sess, ac, true) | |||
| 658 | + | }() | |||
| 659 | + | } | |||
| 660 | + | close(start) | |||
| 661 | + | wg.Wait() | |||
| 662 | + | require.NotNil(t, ac.overlays) | |||
| 663 | + | } | |||
| 664 | + | ||||
| 665 | + | func TestStartPaneGoroutinesAccountsForOneReader(t *testing.T) { | |||
| 666 | + | p, release := newBlockingPTY(t) | |||
| 667 | + | d, sess, _, _ := newManualSessionWithPTYs(t, p) | |||
| 668 | + | tb := sess.activeTab() | |||
| 669 | + | require.NotNil(t, tb) | |||
| 670 | + | d.startPaneGoroutines(sess, tb, tb.focusedPane()) | |||
| 671 | + | release() | |||
| 672 | + | select { | |||
| 673 | + | case <-waitGroupDone(&d.sessWg): | |||
| 674 | + | case <-time.After(time.Second): | |||
| 675 | + | t.Fatal("one reader must balance exactly one WaitGroup count") | |||
| 676 | + | } | |||
| 677 | + | } | |||
| 678 | + | ||||
| 679 | + | // inertTimer deliberately has a nil C channel. It verifies coordinator | |||
| 680 | + | // lifecycle paths do not strand workers behind fake clocks that never fire. | |||
| 681 | + | type inertTimer struct{ stopped atomic.Bool } | |||
| 682 | + | ||||
| 683 | + | func (t *inertTimer) C() <-chan time.Time { return nil } | |||
| 684 | + | func (t *inertTimer) Reset(time.Duration) bool { return false } | |||
| 685 | + | func (t *inertTimer) Stop() bool { t.stopped.Store(true); return true } | |||
| 686 | + | ||||
| 687 | + | type inertClock struct{ timers chan *inertTimer } | |||
| 688 | + | ||||
| 689 | + | func (c *inertClock) Now() time.Time { return time.Time{} } | |||
| 690 | + | func (c *inertClock) NewTimer(time.Duration) ports.Timer { | |||
| 691 | + | t := &inertTimer{} | |||
| 692 | + | c.timers <- t | |||
| 693 | + | return t | |||
| 694 | + | } | |||
| 695 | + | ||||
| 696 | + | func requireWorkerExit(t *testing.T, done <-chan struct{}) { | |||
| 697 | + | t.Helper() | |||
| 698 | + | for range 4096 { | |||
| 699 | + | select { | |||
| 700 | + | case <-done: | |||
| 701 | + | return | |||
| 702 | + | default: | |||
| 703 | + | runtime.Gosched() | |||
| 704 | + | } | |||
| 705 | + | } | |||
| 706 | + | t.Fatal("cancelled coordinator timer worker did not exit") | |||
| 707 | + | } | |||
| 708 | + | ||||
| 709 | + | func TestRenderCoordinatorStopsInertTimerWorkers(t *testing.T) { | |||
| 710 | + | lifecycle := []struct { | |||
| 711 | + | name string | |||
| 712 | + | end func(*renderCoordinator, *attachedClient) | |||
| 713 | + | }{ | |||
| 714 | + | {"detach", func(rc *renderCoordinator, ac *attachedClient) { rc.noteDetach(ac) }}, | |||
| 715 | + | {"park", func(rc *renderCoordinator, ac *attachedClient) { rc.notePark(ac) }}, | |||
| 716 | + | {"teardown", func(rc *renderCoordinator, _ *attachedClient) { rc.noteSessionTeardown() }}, | |||
| 717 | + | } | |||
| 718 | + | for _, tc := range lifecycle { | |||
| 719 | + | t.Run(tc.name, func(t *testing.T) { | |||
| 720 | + | clk := &inertClock{timers: make(chan *inertTimer, 2)} | |||
| 721 | + | rc := newRenderCoordinator(renderCoordinatorOptions{clock: clk}) | |||
| 722 | + | ac := &attachedClient{} | |||
| 723 | + | rc.attach(ac) | |||
| 724 | + | rc.invalidate(renderInvalidation{class: invalidateOutput}) | |||
| 725 | + | normal := <-clk.timers | |||
| 726 | + | rc.mu.Lock() | |||
| 727 | + | normalDone := rc.normalWorkerDone | |||
| 728 | + | rc.mu.Unlock() | |||
| 729 | + | tc.end(rc, ac) | |||
| 730 | + | require.True(t, normal.stopped.Load(), "normal timer must be stopped") | |||
| 731 | + | requireWorkerExit(t, normalDone) | |||
| 732 | + | ||||
| 733 | + | // Use a fresh coordinator because detach/teardown intentionally makes | |||
| 734 | + | // an attachment ineligible for a new synchronized batch. | |||
| 735 | + | rc = newRenderCoordinator(renderCoordinatorOptions{clock: clk}) | |||
| 736 | + | rc.attach(ac) | |||
| 737 | + | rc.noteSyncBegin(1) | |||
| 738 | + | syncTimer := <-clk.timers | |||
| 739 | + | rc.mu.Lock() | |||
| 740 | + | syncDone := rc.syncWorkerDone | |||
| 741 | + | rc.mu.Unlock() | |||
| 742 | + | tc.end(rc, ac) | |||
| 743 | + | require.True(t, syncTimer.stopped.Load(), "sync watchdog must be stopped") | |||
| 744 | + | requireWorkerExit(t, syncDone) | |||
| 745 | + | }) | |||
| 746 | + | } | |||
| 747 | + | } | |||
| 748 | + | ||||
| 641 | func TestRenderCoordinatorRetainsPR71ResizeDispatch(t *testing.T) { | 749 | func TestRenderCoordinatorRetainsPR71ResizeDispatch(t *testing.T) { | ||
| 642 | newResizeFixture := func(t *testing.T) (*Daemon, *session, *attachedClient, chan ports.Frame, *signalClock, chan renderInvalidation) { | 750 | newResizeFixture := func(t *testing.T) (*Daemon, *session, *attachedClient, chan ports.Frame, *signalClock, chan renderInvalidation) { | ||
| 643 | t.Helper() | 751 | t.Helper() | ||
M internal/usecase/daemon/session.go +2 -1
| @@ -397,7 +397,8 @@ func (d *Daemon) startPaneGoroutines(sess *session, tb *tab, p *pane) { | |||
| 397 | 397 | sess.mu.Unlock() | |
| 398 | 398 | d.log.Info("pane created", "session", name, "pane", p.id) | |
| 399 | 399 | } | |
| 400 | - | d.sessWg.Add(2) | |
| 400 | + | // Scheduler ownership was removed; this launch creates exactly one reader. | |
| 401 | + | d.sessWg.Add(1) | |
| 401 | 402 | go d.ptyReader(sess, tb, p) | |
| 402 | 403 | } | |
| 403 | 404 | ||
| @@ -397,7 +397,8 @@ func (d *Daemon) startPaneGoroutines(sess *session, tb *tab, p *pane) { | |||||
| 397 | sess.mu.Unlock() | 397 | sess.mu.Unlock() | ||
| 398 | d.log.Info("pane created", "session", name, "pane", p.id) | 398 | d.log.Info("pane created", "session", name, "pane", p.id) | ||
| 399 | } | 399 | } | ||
| 400 | - | d.sessWg.Add(2) | 400 | + | // Scheduler ownership was removed; this launch creates exactly one reader. |
| 401 | + | d.sessWg.Add(1) | |||
| 401 | go d.ptyReader(sess, tb, p) | 402 | go d.ptyReader(sess, tb, p) | ||
| 402 | } | 403 | } | ||
| 403 | 404 | ||||