fix(daemon): republish synchronized completion
- commit
- 7f0463cabce389ea4aaf4fb58a45e5b3e323ebb4
- author
- brice · · 2026-07-11 21:56 UTC
- committer
- brice · · 2026-07-11 22:09 UTC
- parent
- fcb1644
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +30 -9
M internal/usecase/daemon/render.go +5 -3
| @@ -186,9 +186,11 @@ func (d *Daemon) ptyReader(sess *session, tb *tab, p *pane) { | |||
| 186 | 186 | rc.invalidate(renderInvalidation{class: invalidateOutput, producer: "render.go"}) | |
| 187 | 187 | } | |
| 188 | 188 | } else if syncEnded && renderable { | |
| 189 | - | // State removal happened under pane.mu; only the external wake is | |
| 190 | - | // deferred until after unlock to preserve lock order. | |
| 191 | - | rc.fireCurrent(false) | |
| 189 | + | // State removal happened under pane.mu. Publish a new urgent | |
| 190 | + | // completion after unlock: detach/replace may have cleared the | |
| 191 | + | // pending batch work, so fireCurrent alone could not wake a | |
| 192 | + | // headless picker preview or replacement attachment. | |
| 193 | + | rc.invalidate(renderInvalidation{class: invalidateUrgent, producer: "render.go"}) | |
| 192 | 194 | } | |
| 193 | 195 | } | |
| 194 | 196 | if completeSyncRead && renderable { | |
| @@ -186,9 +186,11 @@ func (d *Daemon) ptyReader(sess *session, tb *tab, p *pane) { | |||||
| 186 | rc.invalidate(renderInvalidation{class: invalidateOutput, producer: "render.go"}) | 186 | rc.invalidate(renderInvalidation{class: invalidateOutput, producer: "render.go"}) | ||
| 187 | } | 187 | } | ||
| 188 | } else if syncEnded && renderable { | 188 | } else if syncEnded && renderable { | ||
| 189 | - | // State removal happened under pane.mu; only the external wake is | 189 | + | // State removal happened under pane.mu. Publish a new urgent |
| 190 | - | // deferred until after unlock to preserve lock order. | 190 | + | // completion after unlock: detach/replace may have cleared the |
| 191 | - | rc.fireCurrent(false) | 191 | + | // pending batch work, so fireCurrent alone could not wake a |
| 192 | + | // headless picker preview or replacement attachment. | |||
| 193 | + | rc.invalidate(renderInvalidation{class: invalidateUrgent, producer: "render.go"}) | |||
| 192 | } | 194 | } | ||
| 193 | } | 195 | } | ||
| 194 | if completeSyncRead && renderable { | 196 | if completeSyncRead && renderable { | ||
M internal/usecase/daemon/render_coordinator.go +4 -1
| @@ -272,7 +272,10 @@ func (c *renderCoordinator) invalidateForAttachment(source *attachedClient, inv | |||
| 272 | 272 | c.pendingUrgent = c.pendingUrgent || inv.class == invalidateUrgent | |
| 273 | 273 | c.pendingPreview = c.pendingPreview || c.previewWake != nil || len(c.previewWakes) != 0 | |
| 274 | 274 | c.coalesced++ | |
| 275 | - | arm := !wasPending || (!wasUrgent && c.pendingUrgent) || (!wasPreviewPending && c.pendingPreview) | |
| 275 | + | // A deadline may have expired while synchronized output still gated the | |
| 276 | + | // pending work. Completion republishes urgently and must reserve a fresh | |
| 277 | + | // deadline rather than leaving that already-fired timer as the only arm. | |
| 278 | + | arm := !wasPending || (!wasUrgent && c.pendingUrgent) || (!wasPreviewPending && c.pendingPreview) || c.deadlineDue | |
| 276 | 279 | var old ports.Timer | |
| 277 | 280 | if arm { | |
| 278 | 281 | c.generation++ | |
| @@ -272,7 +272,10 @@ func (c *renderCoordinator) invalidateForAttachment(source *attachedClient, inv | |||||
| 272 | c.pendingUrgent = c.pendingUrgent || inv.class == invalidateUrgent | 272 | c.pendingUrgent = c.pendingUrgent || inv.class == invalidateUrgent | ||
| 273 | c.pendingPreview = c.pendingPreview || c.previewWake != nil || len(c.previewWakes) != 0 | 273 | c.pendingPreview = c.pendingPreview || c.previewWake != nil || len(c.previewWakes) != 0 | ||
| 274 | c.coalesced++ | 274 | c.coalesced++ | ||
| 275 | - | arm := !wasPending || (!wasUrgent && c.pendingUrgent) || (!wasPreviewPending && c.pendingPreview) | 275 | + | // A deadline may have expired while synchronized output still gated the |
| 276 | + | // pending work. Completion republishes urgently and must reserve a fresh | |||
| 277 | + | // deadline rather than leaving that already-fired timer as the only arm. | |||
| 278 | + | arm := !wasPending || (!wasUrgent && c.pendingUrgent) || (!wasPreviewPending && c.pendingPreview) || c.deadlineDue | |||
| 276 | var old ports.Timer | 279 | var old ports.Timer | ||
| 277 | if arm { | 280 | if arm { | ||
| 278 | c.generation++ | 281 | c.generation++ | ||
M internal/usecase/daemon/render_coordinator_test.go +18 -3
| @@ -657,9 +657,14 @@ func TestRenderCoordinatorSynchronizedOutput(t *testing.T) { | |||
| 657 | 657 | require.Equal(t, maxSyncUpdateDuration, watchdogs[len(watchdogs)-1].duration) | |
| 658 | 658 | ||
| 659 | 659 | h.rc.invalidate(renderInvalidation{class: invalidateOutput, reset: true}) | |
| 660 | - | for _, timer := range h.armedTimers(t) { | |
| 661 | - | timer.ch <- time.Time{} | |
| 662 | - | } | |
| 660 | + | timers := h.armedTimers(t) | |
| 661 | + | require.Len(t, timers, 1) | |
| 662 | + | h.rc.mu.Lock() | |
| 663 | + | normalWorkerDone := h.rc.normalWorkerDone | |
| 664 | + | h.rc.mu.Unlock() | |
| 665 | + | require.NotNil(t, normalWorkerDone) | |
| 666 | + | timers[0].ch <- time.Time{} | |
| 667 | + | requireWorkerExit(t, normalWorkerDone) | |
| 663 | 668 | requireNoWake(t, h.wakes) | |
| 664 | 669 | ||
| 665 | 670 | h.syncActive.Store(false) | |
| @@ -1354,7 +1359,12 @@ func TestRenderCoordinatorSyncBatchSurvivesAttachmentLifecycle(t *testing.T) { | |||
| 1354 | 1359 | h.rc.invalidate(renderInvalidation{class: invalidateOutput, producer: "complete preview"}) | |
| 1355 | 1360 | timers := h.armedTimers(t) | |
| 1356 | 1361 | require.Len(t, timers, 1) | |
| 1362 | + | h.rc.mu.Lock() | |
| 1363 | + | normalWorkerDone := h.rc.normalWorkerDone | |
| 1364 | + | h.rc.mu.Unlock() | |
| 1365 | + | require.NotNil(t, normalWorkerDone) | |
| 1357 | 1366 | timers[0].ch <- time.Time{} | |
| 1367 | + | requireWorkerExit(t, normalWorkerDone) | |
| 1358 | 1368 | requireNoWake(t, h.previews) | |
| 1359 | 1369 | ||
| 1360 | 1370 | h.rc.mu.Lock() | |
| @@ -1386,7 +1396,12 @@ func TestRenderCoordinatorSyncBatchSurvivesAttachmentLifecycle(t *testing.T) { | |||
| 1386 | 1396 | h.rc.invalidateForAttachment(replacement, renderInvalidation{class: invalidateUrgent, reset: true, producer: "replacement first paint"}) | |
| 1387 | 1397 | timers := h.armedTimers(t) | |
| 1388 | 1398 | require.Len(t, timers, 1) | |
| 1399 | + | h.rc.mu.Lock() | |
| 1400 | + | normalWorkerDone := h.rc.normalWorkerDone | |
| 1401 | + | h.rc.mu.Unlock() | |
| 1402 | + | require.NotNil(t, normalWorkerDone) | |
| 1389 | 1403 | timers[0].ch <- time.Time{} | |
| 1404 | + | requireWorkerExit(t, normalWorkerDone) | |
| 1390 | 1405 | requireNoWake(t, h.wakes) | |
| 1391 | 1406 | ||
| 1392 | 1407 | h.rc.mu.Lock() | |
| @@ -657,9 +657,14 @@ func TestRenderCoordinatorSynchronizedOutput(t *testing.T) { | |||||
| 657 | require.Equal(t, maxSyncUpdateDuration, watchdogs[len(watchdogs)-1].duration) | 657 | require.Equal(t, maxSyncUpdateDuration, watchdogs[len(watchdogs)-1].duration) | ||
| 658 | 658 | ||||
| 659 | h.rc.invalidate(renderInvalidation{class: invalidateOutput, reset: true}) | 659 | h.rc.invalidate(renderInvalidation{class: invalidateOutput, reset: true}) | ||
| 660 | - | for _, timer := range h.armedTimers(t) { | 660 | + | timers := h.armedTimers(t) |
| 661 | - | timer.ch <- time.Time{} | 661 | + | require.Len(t, timers, 1) |
| 662 | - | } | 662 | + | h.rc.mu.Lock() |
| 663 | + | normalWorkerDone := h.rc.normalWorkerDone | |||
| 664 | + | h.rc.mu.Unlock() | |||
| 665 | + | require.NotNil(t, normalWorkerDone) | |||
| 666 | + | timers[0].ch <- time.Time{} | |||
| 667 | + | requireWorkerExit(t, normalWorkerDone) | |||
| 663 | requireNoWake(t, h.wakes) | 668 | requireNoWake(t, h.wakes) | ||
| 664 | 669 | ||||
| 665 | h.syncActive.Store(false) | 670 | h.syncActive.Store(false) | ||
| @@ -1354,7 +1359,12 @@ func TestRenderCoordinatorSyncBatchSurvivesAttachmentLifecycle(t *testing.T) { | |||||
| 1354 | h.rc.invalidate(renderInvalidation{class: invalidateOutput, producer: "complete preview"}) | 1359 | h.rc.invalidate(renderInvalidation{class: invalidateOutput, producer: "complete preview"}) | ||
| 1355 | timers := h.armedTimers(t) | 1360 | timers := h.armedTimers(t) | ||
| 1356 | require.Len(t, timers, 1) | 1361 | require.Len(t, timers, 1) | ||
| 1362 | + | h.rc.mu.Lock() | |||
| 1363 | + | normalWorkerDone := h.rc.normalWorkerDone | |||
| 1364 | + | h.rc.mu.Unlock() | |||
| 1365 | + | require.NotNil(t, normalWorkerDone) | |||
| 1357 | timers[0].ch <- time.Time{} | 1366 | timers[0].ch <- time.Time{} | ||
| 1367 | + | requireWorkerExit(t, normalWorkerDone) | |||
| 1358 | requireNoWake(t, h.previews) | 1368 | requireNoWake(t, h.previews) | ||
| 1359 | 1369 | ||||
| 1360 | h.rc.mu.Lock() | 1370 | h.rc.mu.Lock() | ||
| @@ -1386,7 +1396,12 @@ func TestRenderCoordinatorSyncBatchSurvivesAttachmentLifecycle(t *testing.T) { | |||||
| 1386 | h.rc.invalidateForAttachment(replacement, renderInvalidation{class: invalidateUrgent, reset: true, producer: "replacement first paint"}) | 1396 | h.rc.invalidateForAttachment(replacement, renderInvalidation{class: invalidateUrgent, reset: true, producer: "replacement first paint"}) | ||
| 1387 | timers := h.armedTimers(t) | 1397 | timers := h.armedTimers(t) | ||
| 1388 | require.Len(t, timers, 1) | 1398 | require.Len(t, timers, 1) | ||
| 1399 | + | h.rc.mu.Lock() | |||
| 1400 | + | normalWorkerDone := h.rc.normalWorkerDone | |||
| 1401 | + | h.rc.mu.Unlock() | |||
| 1402 | + | require.NotNil(t, normalWorkerDone) | |||
| 1389 | timers[0].ch <- time.Time{} | 1403 | timers[0].ch <- time.Time{} | ||
| 1404 | + | requireWorkerExit(t, normalWorkerDone) | |||
| 1390 | requireNoWake(t, h.wakes) | 1405 | requireNoWake(t, h.wakes) | ||
| 1391 | 1406 | ||||
| 1392 | h.rc.mu.Lock() | 1407 | h.rc.mu.Lock() | ||
M internal/usecase/daemon/render_test.go +3 -2
| @@ -194,6 +194,7 @@ func TestPTYReaderSyncVisibilityTransitions(t *testing.T) { | |||
| 194 | 194 | ||
| 195 | 195 | inactiveSteps <- channelPTYStep{data: []byte(" complete\x1b[?2026l")} | |
| 196 | 196 | awaitPTYReadProcessed(t, inactiveProcessed) | |
| 197 | + | fireCoordinatorTimer(t, drainCoordinatorTimers(clock), urgentRenderDeadline) | |
| 197 | 198 | frame := awaitOutputFrameWithoutSleep(t, sends) | |
| 198 | 199 | output, err := ports.UnmarshalOutput(frame.Payload) | |
| 199 | 200 | require.NoError(t, err) | |
| @@ -277,7 +278,7 @@ func TestPTYReaderRepublishesSynchronizedCompletionAfterAttachmentLifecycle(t *t | |||
| 277 | 278 | fireCoordinatorTimer(t, drainCoordinatorTimers(clock), urgentRenderDeadline) | |
| 278 | 279 | wake := <-previews | |
| 279 | 280 | require.True(t, wake.urgent) | |
| 280 | - | require.Equal(t, 1, wake.coalesced) | |
| 281 | + | require.Equal(t, 2, wake.coalesced, "completion retains coordinator coalescing across the cleared attachment work") | |
| 281 | 282 | select { | |
| 282 | 283 | case duplicate := <-previews: | |
| 283 | 284 | t.Fatalf("sync completion must publish exactly one preview wake: %#v", duplicate) | |
| @@ -320,7 +321,7 @@ func TestPTYReaderRepublishesSynchronizedCompletionAfterAttachmentLifecycle(t *t | |||
| 320 | 321 | wake := <-wakes | |
| 321 | 322 | require.True(t, wake.urgent) | |
| 322 | 323 | require.True(t, wake.reset, "the replacement's cleared batch must repaint a complete frame") | |
| 323 | - | require.Equal(t, 2, wake.coalesced, "completion must coalesce with the replacement reset") | |
| 324 | + | require.Equal(t, 3, wake.coalesced, "completion must retain the replacement reset and coordinator coalescing") | |
| 324 | 325 | require.Same(t, replacement, wake.attachment) | |
| 325 | 326 | select { | |
| 326 | 327 | case duplicate := <-wakes: | |
| @@ -194,6 +194,7 @@ func TestPTYReaderSyncVisibilityTransitions(t *testing.T) { | |||||
| 194 | 194 | ||||
| 195 | inactiveSteps <- channelPTYStep{data: []byte(" complete\x1b[?2026l")} | 195 | inactiveSteps <- channelPTYStep{data: []byte(" complete\x1b[?2026l")} | ||
| 196 | awaitPTYReadProcessed(t, inactiveProcessed) | 196 | awaitPTYReadProcessed(t, inactiveProcessed) | ||
| 197 | + | fireCoordinatorTimer(t, drainCoordinatorTimers(clock), urgentRenderDeadline) | |||
| 197 | frame := awaitOutputFrameWithoutSleep(t, sends) | 198 | frame := awaitOutputFrameWithoutSleep(t, sends) | ||
| 198 | output, err := ports.UnmarshalOutput(frame.Payload) | 199 | output, err := ports.UnmarshalOutput(frame.Payload) | ||
| 199 | require.NoError(t, err) | 200 | require.NoError(t, err) | ||
| @@ -277,7 +278,7 @@ func TestPTYReaderRepublishesSynchronizedCompletionAfterAttachmentLifecycle(t *t | |||||
| 277 | fireCoordinatorTimer(t, drainCoordinatorTimers(clock), urgentRenderDeadline) | 278 | fireCoordinatorTimer(t, drainCoordinatorTimers(clock), urgentRenderDeadline) | ||
| 278 | wake := <-previews | 279 | wake := <-previews | ||
| 279 | require.True(t, wake.urgent) | 280 | require.True(t, wake.urgent) | ||
| 280 | - | require.Equal(t, 1, wake.coalesced) | 281 | + | require.Equal(t, 2, wake.coalesced, "completion retains coordinator coalescing across the cleared attachment work") |
| 281 | select { | 282 | select { | ||
| 282 | case duplicate := <-previews: | 283 | case duplicate := <-previews: | ||
| 283 | t.Fatalf("sync completion must publish exactly one preview wake: %#v", duplicate) | 284 | t.Fatalf("sync completion must publish exactly one preview wake: %#v", duplicate) | ||
| @@ -320,7 +321,7 @@ func TestPTYReaderRepublishesSynchronizedCompletionAfterAttachmentLifecycle(t *t | |||||
| 320 | wake := <-wakes | 321 | wake := <-wakes | ||
| 321 | require.True(t, wake.urgent) | 322 | require.True(t, wake.urgent) | ||
| 322 | require.True(t, wake.reset, "the replacement's cleared batch must repaint a complete frame") | 323 | require.True(t, wake.reset, "the replacement's cleared batch must repaint a complete frame") | ||
| 323 | - | require.Equal(t, 2, wake.coalesced, "completion must coalesce with the replacement reset") | 324 | + | require.Equal(t, 3, wake.coalesced, "completion must retain the replacement reset and coordinator coalescing") |
| 324 | require.Same(t, replacement, wake.attachment) | 325 | require.Same(t, replacement, wake.attachment) | ||
| 325 | select { | 326 | select { | ||
| 326 | case duplicate := <-wakes: | 327 | case duplicate := <-wakes: | ||