fix(remote): address picker review findings
- commit
- bd9d2c003b2bc934a3a17328718000e91e82fab7
- author
- brice · · 2026-07-31 17:26 UTC
- committer
- brice · · 2026-07-31 17:56 UTC
- parent
- 1fe4915
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
14 files · +423 -78
M internal/usecase/daemon/client_transport_errors.go +2 -2
| @@ -54,7 +54,6 @@ func (d *Daemon) finishClientGone(sess *session, ac *attachedClient, failed port | |||
| 54 | 54 | if rc := sess.renderCoordinator(); rc != nil { | |
| 55 | 55 | rc.noteDetach(ac) | |
| 56 | 56 | } | |
| 57 | - | d.unregisterPreview(ac) | |
| 58 | 57 | sess.mu.Lock() | |
| 59 | 58 | ephemeral := sess.ephemeral | |
| 60 | 59 | sess.mu.Unlock() | |
| @@ -75,6 +74,7 @@ func (d *Daemon) finishClientGone(sess *session, ac *attachedClient, failed port | |||
| 75 | 74 | // attachment parking marker left by a raced non-explicit teardown so | |
| 76 | 75 | // IntentResume waiters are not stranded on a never-published park. | |
| 77 | 76 | d.clearParkingInFlight(ac.resumeToken, ac) | |
| 77 | + | d.closePicker(ac) | |
| 78 | 78 | ||
| 79 | 79 | d.resetScreenDefaultColors(sess) | |
| 80 | 80 | ac.clearPreviousSession() | |
| @@ -176,13 +176,13 @@ func (d *Daemon) finishSendErrorDetach(sess *session, ac *attachedClient, failed | |||
| 176 | 176 | if rc := sess.renderCoordinator(); rc != nil { | |
| 177 | 177 | rc.noteDetach(ac) | |
| 178 | 178 | } | |
| 179 | - | d.unregisterPreview(ac) | |
| 180 | 179 | if d.parkAttachment(sess, ac) { | |
| 181 | 180 | _ = ac.closeCapturedTransport(failed) | |
| 182 | 181 | d.log.Warn("parked client after send error", "session", sess.name) | |
| 183 | 182 | return | |
| 184 | 183 | } | |
| 185 | 184 | d.clearParkingInFlight(ac.resumeToken, ac) | |
| 185 | + | d.closePicker(ac) | |
| 186 | 186 | d.resetScreenDefaultColors(sess) | |
| 187 | 187 | ac.clearPreviousSession() | |
| 188 | 188 | _ = ac.closeCapturedTransport(failed) | |
| @@ -54,7 +54,6 @@ func (d *Daemon) finishClientGone(sess *session, ac *attachedClient, failed port | |||||
| 54 | if rc := sess.renderCoordinator(); rc != nil { | 54 | if rc := sess.renderCoordinator(); rc != nil { | ||
| 55 | rc.noteDetach(ac) | 55 | rc.noteDetach(ac) | ||
| 56 | } | 56 | } | ||
| 57 | - | d.unregisterPreview(ac) | |||
| 58 | sess.mu.Lock() | 57 | sess.mu.Lock() | ||
| 59 | ephemeral := sess.ephemeral | 58 | ephemeral := sess.ephemeral | ||
| 60 | sess.mu.Unlock() | 59 | sess.mu.Unlock() | ||
| @@ -75,6 +74,7 @@ func (d *Daemon) finishClientGone(sess *session, ac *attachedClient, failed port | |||||
| 75 | // attachment parking marker left by a raced non-explicit teardown so | 74 | // attachment parking marker left by a raced non-explicit teardown so | ||
| 76 | // IntentResume waiters are not stranded on a never-published park. | 75 | // IntentResume waiters are not stranded on a never-published park. | ||
| 77 | d.clearParkingInFlight(ac.resumeToken, ac) | 76 | d.clearParkingInFlight(ac.resumeToken, ac) | ||
| 77 | + | d.closePicker(ac) | |||
| 78 | 78 | ||||
| 79 | d.resetScreenDefaultColors(sess) | 79 | d.resetScreenDefaultColors(sess) | ||
| 80 | ac.clearPreviousSession() | 80 | ac.clearPreviousSession() | ||
| @@ -176,13 +176,13 @@ func (d *Daemon) finishSendErrorDetach(sess *session, ac *attachedClient, failed | |||||
| 176 | if rc := sess.renderCoordinator(); rc != nil { | 176 | if rc := sess.renderCoordinator(); rc != nil { | ||
| 177 | rc.noteDetach(ac) | 177 | rc.noteDetach(ac) | ||
| 178 | } | 178 | } | ||
| 179 | - | d.unregisterPreview(ac) | |||
| 180 | if d.parkAttachment(sess, ac) { | 179 | if d.parkAttachment(sess, ac) { | ||
| 181 | _ = ac.closeCapturedTransport(failed) | 180 | _ = ac.closeCapturedTransport(failed) | ||
| 182 | d.log.Warn("parked client after send error", "session", sess.name) | 181 | d.log.Warn("parked client after send error", "session", sess.name) | ||
| 183 | return | 182 | return | ||
| 184 | } | 183 | } | ||
| 185 | d.clearParkingInFlight(ac.resumeToken, ac) | 184 | d.clearParkingInFlight(ac.resumeToken, ac) | ||
| 185 | + | d.closePicker(ac) | |||
| 186 | d.resetScreenDefaultColors(sess) | 186 | d.resetScreenDefaultColors(sess) | ||
| 187 | ac.clearPreviousSession() | 187 | ac.clearPreviousSession() | ||
| 188 | _ = ac.closeCapturedTransport(failed) | 188 | _ = ac.closeCapturedTransport(failed) | ||
M internal/usecase/daemon/daemon.go +8 -7
| @@ -334,12 +334,13 @@ type Daemon struct { | |||
| 334 | 334 | } | |
| 335 | 335 | ||
| 336 | 336 | type parkedAttachment struct { | |
| 337 | - | sess *session | |
| 338 | - | ac *attachedClient | |
| 339 | - | role attachmentRole | |
| 340 | - | timer ports.Timer | |
| 341 | - | done chan struct{} | |
| 342 | - | doneOnce sync.Once | |
| 337 | + | sess *session | |
| 338 | + | ac *attachedClient | |
| 339 | + | role attachmentRole | |
| 340 | + | pickerGeneration uint64 | |
| 341 | + | timer ports.Timer | |
| 342 | + | done chan struct{} | |
| 343 | + | doneOnce sync.Once | |
| 343 | 344 | } | |
| 344 | 345 | ||
| 345 | 346 | // parkingAttachment is the observable detach→park lifecycle for one resume | |
| @@ -804,7 +805,7 @@ func (d *Daemon) shutdownAllWithSnapshotDeadline(reason uint8, deadline *snapsho | |||
| 804 | 805 | snapshot := d.sessionsSnapshotLocked() | |
| 805 | 806 | empty := len(snapshot) == 0 | |
| 806 | 807 | d.mu.Unlock() | |
| 807 | - | finishParkedAttachmentRetirements(parkedRetirements) | |
| 808 | + | d.finishParkedAttachmentRetirements(parkedRetirements) | |
| 808 | 809 | d.log.Info("graceful shutdown begin", "reason", reason, "sessions", len(snapshot)) | |
| 809 | 810 | if empty { | |
| 810 | 811 | d.doneOnce.Do(func() { close(d.done) }) | |
| @@ -334,12 +334,13 @@ type Daemon struct { | |||||
| 334 | } | 334 | } | ||
| 335 | 335 | ||||
| 336 | type parkedAttachment struct { | 336 | type parkedAttachment struct { | ||
| 337 | - | sess *session | 337 | + | sess *session |
| 338 | - | ac *attachedClient | 338 | + | ac *attachedClient |
| 339 | - | role attachmentRole | 339 | + | role attachmentRole |
| 340 | - | timer ports.Timer | 340 | + | pickerGeneration uint64 |
| 341 | - | done chan struct{} | 341 | + | timer ports.Timer |
| 342 | - | doneOnce sync.Once | 342 | + | done chan struct{} |
| 343 | + | doneOnce sync.Once | |||
| 343 | } | 344 | } | ||
| 344 | 345 | ||||
| 345 | // parkingAttachment is the observable detach→park lifecycle for one resume | 346 | // parkingAttachment is the observable detach→park lifecycle for one resume | ||
| @@ -804,7 +805,7 @@ func (d *Daemon) shutdownAllWithSnapshotDeadline(reason uint8, deadline *snapsho | |||||
| 804 | snapshot := d.sessionsSnapshotLocked() | 805 | snapshot := d.sessionsSnapshotLocked() | ||
| 805 | empty := len(snapshot) == 0 | 806 | empty := len(snapshot) == 0 | ||
| 806 | d.mu.Unlock() | 807 | d.mu.Unlock() | ||
| 807 | - | finishParkedAttachmentRetirements(parkedRetirements) | 808 | + | d.finishParkedAttachmentRetirements(parkedRetirements) |
| 808 | d.log.Info("graceful shutdown begin", "reason", reason, "sessions", len(snapshot)) | 809 | d.log.Info("graceful shutdown begin", "reason", reason, "sessions", len(snapshot)) | ||
| 809 | if empty { | 810 | if empty { | ||
| 810 | d.doneOnce.Do(func() { close(d.done) }) | 811 | d.doneOnce.Do(func() { close(d.done) }) | ||
M internal/usecase/daemon/move_pane_postcommit.go +1 -1
| @@ -132,7 +132,7 @@ func (p movePanePostcommitPlan) execute(d *Daemon) { | |||
| 132 | 132 | d.unregisterPreview(retirement.parked.ac) | |
| 133 | 133 | retirement.parked.ac.clearCaptureFrames() | |
| 134 | 134 | } | |
| 135 | - | finishParkedAttachmentRetirements(p.retiredParked) | |
| 135 | + | d.finishParkedAttachmentRetirements(p.retiredParked) | |
| 136 | 136 | if p.sourceEmpty && p.sourceMetadataValid && d.persistEnabled { | |
| 137 | 137 | if err := d.beginSnapshotPurge(p.source.name, p.source.incarnation); err == nil { | |
| 138 | 138 | if err := d.finishSnapshotPurge(d.serveCtx, p.source.name, p.source.incarnation); err != nil { | |
| @@ -132,7 +132,7 @@ func (p movePanePostcommitPlan) execute(d *Daemon) { | |||||
| 132 | d.unregisterPreview(retirement.parked.ac) | 132 | d.unregisterPreview(retirement.parked.ac) | ||
| 133 | retirement.parked.ac.clearCaptureFrames() | 133 | retirement.parked.ac.clearCaptureFrames() | ||
| 134 | } | 134 | } | ||
| 135 | - | finishParkedAttachmentRetirements(p.retiredParked) | 135 | + | d.finishParkedAttachmentRetirements(p.retiredParked) |
| 136 | if p.sourceEmpty && p.sourceMetadataValid && d.persistEnabled { | 136 | if p.sourceEmpty && p.sourceMetadataValid && d.persistEnabled { | ||
| 137 | if err := d.beginSnapshotPurge(p.source.name, p.source.incarnation); err == nil { | 137 | if err := d.beginSnapshotPurge(p.source.name, p.source.incarnation); err == nil { | ||
| 138 | if err := d.finishSnapshotPurge(d.serveCtx, p.source.name, p.source.incarnation); err != nil { | 138 | if err := d.finishSnapshotPurge(d.serveCtx, p.source.name, p.source.incarnation); err != nil { | ||
M internal/usecase/daemon/move_picker.go +1 -1
| @@ -176,7 +176,7 @@ func (d *Daemon) refreshPickerOpts(ac *attachedClient, opts pickerRefreshOptions | |||
| 176 | 176 | } | |
| 177 | 177 | if intent != pickerNavigate { | |
| 178 | 178 | if _, ok := model.Selected(); !ok { | |
| 179 | - | d.closePicker(ac) | |
| 179 | + | d.closePickerIfCurrent(ac, observedModel, observedGeneration) | |
| 180 | 180 | return | |
| 181 | 181 | } | |
| 182 | 182 | } | |
| @@ -176,7 +176,7 @@ func (d *Daemon) refreshPickerOpts(ac *attachedClient, opts pickerRefreshOptions | |||||
| 176 | } | 176 | } | ||
| 177 | if intent != pickerNavigate { | 177 | if intent != pickerNavigate { | ||
| 178 | if _, ok := model.Selected(); !ok { | 178 | if _, ok := model.Selected(); !ok { | ||
| 179 | - | d.closePicker(ac) | 179 | + | d.closePickerIfCurrent(ac, observedModel, observedGeneration) |
| 180 | return | 180 | return | ||
| 181 | } | 181 | } | ||
| 182 | } | 182 | } | ||
M internal/usecase/daemon/move_picker_test.go +42 -0
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | ||
| 10 | 10 | "github.com/bnema/vev/internal/domain" | |
| 11 | 11 | "github.com/bnema/vev/internal/ports" | |
| 12 | + | "github.com/bnema/vev/internal/usecase/picker" | |
| 12 | 13 | ) | |
| 13 | 14 | ||
| 14 | 15 | func setupMovePickerSessions(t *testing.T, extraDestinationTabs int) (*Daemon, *session, *attachedClient, *session, *tab, []func()) { | |
| @@ -314,6 +315,47 @@ func TestMovePickerFinalSourceClosesBeforeSnatchFinalization(t *testing.T) { | |||
| 314 | 315 | require.Equal(t, ports.MsgOutput, frames[0].Type) | |
| 315 | 316 | } | |
| 316 | 317 | ||
| 318 | + | func TestMovePickerRefreshCloseKeepsReplacementPicker(t *testing.T) { | |
| 319 | + | d, source, ac, destination, _, releases := setupMovePickerSessions(t, 0) | |
| 320 | + | defer releaseAll(releases) | |
| 321 | + | require.NoError(t, d.enterPickerForIntent(source, ac, pickerMovePane, moveSourceLocator{ | |
| 322 | + | Session: moveSessionLocator{ID: source.id, Incarnation: source.incarnation, Name: source.name}, | |
| 323 | + | TabID: "source-tab", PaneID: "source-pane", | |
| 324 | + | })) | |
| 325 | + | d.mu.Lock() | |
| 326 | + | delete(d.sessions, destination.id) | |
| 327 | + | d.mu.Unlock() | |
| 328 | + | ||
| 329 | + | rebuildReached := make(chan struct{}) | |
| 330 | + | allowClose := make(chan struct{}) | |
| 331 | + | refreshed := make(chan struct{}) | |
| 332 | + | ac.overlays.afterPickerRefreshBuild = func(*picker.Model) { | |
| 333 | + | close(rebuildReached) | |
| 334 | + | <-allowClose | |
| 335 | + | } | |
| 336 | + | go func() { | |
| 337 | + | d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1}) | |
| 338 | + | close(refreshed) | |
| 339 | + | }() | |
| 340 | + | select { | |
| 341 | + | case <-rebuildReached: | |
| 342 | + | case <-time.After(time.Second): | |
| 343 | + | t.Fatal("timed out waiting for move-picker refresh rebuild") | |
| 344 | + | } | |
| 345 | + | ||
| 346 | + | replacement := d.newPickerModel(source, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |
| 347 | + | d.publishPicker(source, ac, replacement, pickerNavigate, moveSourceLocator{}) | |
| 348 | + | close(allowClose) | |
| 349 | + | select { | |
| 350 | + | case <-refreshed: | |
| 351 | + | case <-time.After(time.Second): | |
| 352 | + | t.Fatal("timed out waiting for move-picker refresh") | |
| 353 | + | } | |
| 354 | + | ac.overlays.pickerMu.Lock() | |
| 355 | + | require.Same(t, replacement, ac.overlays.picker) | |
| 356 | + | ac.overlays.pickerMu.Unlock() | |
| 357 | + | } | |
| 358 | + | ||
| 317 | 359 | func TestMovePickerDispatchMuNotHeldWhileOpen(t *testing.T) { | |
| 318 | 360 | d, source, ac, _, _, releases := setupMovePickerSessions(t, 0) | |
| 319 | 361 | defer releaseAll(releases) | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | 9 | ||||
| 10 | "github.com/bnema/vev/internal/domain" | 10 | "github.com/bnema/vev/internal/domain" | ||
| 11 | "github.com/bnema/vev/internal/ports" | 11 | "github.com/bnema/vev/internal/ports" | ||
| 12 | + | "github.com/bnema/vev/internal/usecase/picker" | |||
| 12 | ) | 13 | ) | ||
| 13 | 14 | ||||
| 14 | func setupMovePickerSessions(t *testing.T, extraDestinationTabs int) (*Daemon, *session, *attachedClient, *session, *tab, []func()) { | 15 | func setupMovePickerSessions(t *testing.T, extraDestinationTabs int) (*Daemon, *session, *attachedClient, *session, *tab, []func()) { | ||
| @@ -314,6 +315,47 @@ func TestMovePickerFinalSourceClosesBeforeSnatchFinalization(t *testing.T) { | |||||
| 314 | require.Equal(t, ports.MsgOutput, frames[0].Type) | 315 | require.Equal(t, ports.MsgOutput, frames[0].Type) | ||
| 315 | } | 316 | } | ||
| 316 | 317 | ||||
| 318 | + | func TestMovePickerRefreshCloseKeepsReplacementPicker(t *testing.T) { | |||
| 319 | + | d, source, ac, destination, _, releases := setupMovePickerSessions(t, 0) | |||
| 320 | + | defer releaseAll(releases) | |||
| 321 | + | require.NoError(t, d.enterPickerForIntent(source, ac, pickerMovePane, moveSourceLocator{ | |||
| 322 | + | Session: moveSessionLocator{ID: source.id, Incarnation: source.incarnation, Name: source.name}, | |||
| 323 | + | TabID: "source-tab", PaneID: "source-pane", | |||
| 324 | + | })) | |||
| 325 | + | d.mu.Lock() | |||
| 326 | + | delete(d.sessions, destination.id) | |||
| 327 | + | d.mu.Unlock() | |||
| 328 | + | ||||
| 329 | + | rebuildReached := make(chan struct{}) | |||
| 330 | + | allowClose := make(chan struct{}) | |||
| 331 | + | refreshed := make(chan struct{}) | |||
| 332 | + | ac.overlays.afterPickerRefreshBuild = func(*picker.Model) { | |||
| 333 | + | close(rebuildReached) | |||
| 334 | + | <-allowClose | |||
| 335 | + | } | |||
| 336 | + | go func() { | |||
| 337 | + | d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1}) | |||
| 338 | + | close(refreshed) | |||
| 339 | + | }() | |||
| 340 | + | select { | |||
| 341 | + | case <-rebuildReached: | |||
| 342 | + | case <-time.After(time.Second): | |||
| 343 | + | t.Fatal("timed out waiting for move-picker refresh rebuild") | |||
| 344 | + | } | |||
| 345 | + | ||||
| 346 | + | replacement := d.newPickerModel(source, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |||
| 347 | + | d.publishPicker(source, ac, replacement, pickerNavigate, moveSourceLocator{}) | |||
| 348 | + | close(allowClose) | |||
| 349 | + | select { | |||
| 350 | + | case <-refreshed: | |||
| 351 | + | case <-time.After(time.Second): | |||
| 352 | + | t.Fatal("timed out waiting for move-picker refresh") | |||
| 353 | + | } | |||
| 354 | + | ac.overlays.pickerMu.Lock() | |||
| 355 | + | require.Same(t, replacement, ac.overlays.picker) | |||
| 356 | + | ac.overlays.pickerMu.Unlock() | |||
| 357 | + | } | |||
| 358 | + | ||||
| 317 | func TestMovePickerDispatchMuNotHeldWhileOpen(t *testing.T) { | 359 | func TestMovePickerDispatchMuNotHeldWhileOpen(t *testing.T) { | ||
| 318 | d, source, ac, _, _, releases := setupMovePickerSessions(t, 0) | 360 | d, source, ac, _, _, releases := setupMovePickerSessions(t, 0) | ||
| 319 | defer releaseAll(releases) | 361 | defer releaseAll(releases) | ||
M internal/usecase/daemon/overlay_runtime.go +3 -2
| @@ -31,8 +31,9 @@ type overlayRuntime struct { | |||
| 31 | 31 | pickerPending []byte | |
| 32 | 32 | pickerESC pendingByteTimer | |
| 33 | 33 | ||
| 34 | - | // Deterministic lifecycle seams used by ownership race tests. Hooks run | |
| 35 | - | // without pickerMu or remoteCatalog.mu held. | |
| 34 | + | // Test-only, unsynchronized lifecycle seams. Assign them before picker | |
| 35 | + | // publication or goroutine startup. Hooks run without pickerMu or | |
| 36 | + | // remoteCatalog.mu held. | |
| 36 | 37 | beforeRemotePickerRegistration func() | |
| 37 | 38 | afterPickerRefreshBuild func(*picker.Model) | |
| 38 | 39 | ||
| @@ -31,8 +31,9 @@ type overlayRuntime struct { | |||||
| 31 | pickerPending []byte | 31 | pickerPending []byte | ||
| 32 | pickerESC pendingByteTimer | 32 | pickerESC pendingByteTimer | ||
| 33 | 33 | ||||
| 34 | - | // Deterministic lifecycle seams used by ownership race tests. Hooks run | 34 | + | // Test-only, unsynchronized lifecycle seams. Assign them before picker |
| 35 | - | // without pickerMu or remoteCatalog.mu held. | 35 | + | // publication or goroutine startup. Hooks run without pickerMu or |
| 36 | + | // remoteCatalog.mu held. | |||
| 36 | beforeRemotePickerRegistration func() | 37 | beforeRemotePickerRegistration func() | ||
| 37 | afterPickerRefreshBuild func(*picker.Model) | 38 | afterPickerRefreshBuild func(*picker.Model) | ||
| 38 | 39 | ||||
M internal/usecase/daemon/picker.go +27 -6
| @@ -86,10 +86,12 @@ func (d *Daemon) publishPicker(sess *session, ac *attachedClient, model *picker. | |||
| 86 | 86 | func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceFilter) { | |
| 87 | 87 | d.mu.Lock() | |
| 88 | 88 | sessions := make([]attachmentSession, 0, len(d.sessions)) | |
| 89 | - | registry := make(map[domain.SessionID]attachmentSession, len(d.sessions)) | |
| 89 | + | proxies := make(map[domain.SessionID]*proxySession) | |
| 90 | 90 | for id, entry := range d.sessions { | |
| 91 | 91 | sessions = append(sessions, entry) | |
| 92 | - | registry[id] = entry | |
| 92 | + | if proxy, ok := entry.(*proxySession); ok { | |
| 93 | + | proxies[id] = proxy | |
| 94 | + | } | |
| 93 | 95 | } | |
| 94 | 96 | stopped := make([]stoppedSession, 0, len(d.stopped)) | |
| 95 | 97 | for _, s := range d.stopped { | |
| @@ -149,7 +151,14 @@ func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceF | |||
| 149 | 151 | catalog := d.remotePickerCatalogSnapshot() | |
| 150 | 152 | sortRemotePickerCatalog(catalog, d.remotePickerHostRanks()) | |
| 151 | 153 | ||
| 152 | - | views := make([]picker.SessionView, 0, len(live)+len(stopped)) | |
| 154 | + | catalogRows := 0 | |
| 155 | + | for _, host := range catalog { | |
| 156 | + | catalogRows += len(host.entry.Sessions) | |
| 157 | + | if len(host.entry.Sessions) == 0 && (host.status == remoteHostUnreachable || host.status == remoteHostVersionMismatch) { | |
| 158 | + | catalogRows++ | |
| 159 | + | } | |
| 160 | + | } | |
| 161 | + | views := make([]picker.SessionView, 0, len(live)+len(stopped)+catalogRows) | |
| 153 | 162 | for _, item := range live { | |
| 154 | 163 | if proxy, ok := item.entry.(*proxySession); ok { | |
| 155 | 164 | views = append(views, remoteProxyPickerView(proxy.key, item.view)) | |
| @@ -163,7 +172,7 @@ func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceF | |||
| 163 | 172 | if key.Validate() != nil { | |
| 164 | 173 | continue | |
| 165 | 174 | } | |
| 166 | - | if proxy, ok := registry[key.ID()].(*proxySession); ok && proxy.key == key { | |
| 175 | + | if proxy, ok := proxies[key.ID()]; ok && proxy.key == key { | |
| 167 | 176 | continue | |
| 168 | 177 | } | |
| 169 | 178 | views = append(views, remotePickerView(key, session, host.status, host.entry.FetchedAt)) | |
| @@ -496,7 +505,18 @@ func (d *Daemon) clearDestroyedTabPreview(tb *tab) { | |||
| 496 | 505 | } | |
| 497 | 506 | ||
| 498 | 507 | func (d *Daemon) closePicker(ac *attachedClient) { | |
| 508 | + | d.closePickerIfCurrent(ac, nil, 0) | |
| 509 | + | } | |
| 510 | + | ||
| 511 | + | // closePickerIfCurrent closes only the observed picker lifecycle. A nonzero | |
| 512 | + | // generation guards terminal parked cleanup even when a refresh replaced the | |
| 513 | + | // model within that generation. | |
| 514 | + | func (d *Daemon) closePickerIfCurrent(ac *attachedClient, model *picker.Model, generation uint64) bool { | |
| 499 | 515 | ac.overlays.pickerMu.Lock() | |
| 516 | + | if model != nil && (ac.overlays.picker != model || ac.overlays.pickerGeneration != generation) || model == nil && generation != 0 && ac.overlays.pickerGeneration != generation { | |
| 517 | + | ac.overlays.pickerMu.Unlock() | |
| 518 | + | return false | |
| 519 | + | } | |
| 500 | 520 | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: ac.overlays.picker} | |
| 501 | 521 | ac.overlays.picker = nil | |
| 502 | 522 | ac.overlays.pickerTitle = "" | |
| @@ -504,12 +524,13 @@ func (d *Daemon) closePicker(ac *attachedClient) { | |||
| 504 | 524 | ac.overlays.pickerSource = moveSourceLocator{} | |
| 505 | 525 | ac.overlays.pickerPending = nil | |
| 506 | 526 | ac.overlays.pickerESC.stop() | |
| 507 | - | generation := ac.overlays.pickerPreviewGeneration | |
| 527 | + | previewGeneration := ac.overlays.pickerPreviewGeneration | |
| 508 | 528 | ac.overlays.pickerMu.Unlock() | |
| 509 | - | d.clearPreviewGeneration(ac, generation) | |
| 529 | + | d.clearPreviewGeneration(ac, previewGeneration) | |
| 510 | 530 | if instance.model != nil { | |
| 511 | 531 | d.remotePickerClosed(instance) | |
| 512 | 532 | } | |
| 533 | + | return true | |
| 513 | 534 | } | |
| 514 | 535 | ||
| 515 | 536 | func pickerTargetsEqual(left, right picker.Target) bool { | |
| @@ -86,10 +86,12 @@ func (d *Daemon) publishPicker(sess *session, ac *attachedClient, model *picker. | |||||
| 86 | func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceFilter) { | 86 | func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceFilter) { | ||
| 87 | d.mu.Lock() | 87 | d.mu.Lock() | ||
| 88 | sessions := make([]attachmentSession, 0, len(d.sessions)) | 88 | sessions := make([]attachmentSession, 0, len(d.sessions)) | ||
| 89 | - | registry := make(map[domain.SessionID]attachmentSession, len(d.sessions)) | 89 | + | proxies := make(map[domain.SessionID]*proxySession) |
| 90 | for id, entry := range d.sessions { | 90 | for id, entry := range d.sessions { | ||
| 91 | sessions = append(sessions, entry) | 91 | sessions = append(sessions, entry) | ||
| 92 | - | registry[id] = entry | 92 | + | if proxy, ok := entry.(*proxySession); ok { |
| 93 | + | proxies[id] = proxy | |||
| 94 | + | } | |||
| 93 | } | 95 | } | ||
| 94 | stopped := make([]stoppedSession, 0, len(d.stopped)) | 96 | stopped := make([]stoppedSession, 0, len(d.stopped)) | ||
| 95 | for _, s := range d.stopped { | 97 | for _, s := range d.stopped { | ||
| @@ -149,7 +151,14 @@ func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceF | |||||
| 149 | catalog := d.remotePickerCatalogSnapshot() | 151 | catalog := d.remotePickerCatalogSnapshot() | ||
| 150 | sortRemotePickerCatalog(catalog, d.remotePickerHostRanks()) | 152 | sortRemotePickerCatalog(catalog, d.remotePickerHostRanks()) | ||
| 151 | 153 | ||||
| 152 | - | views := make([]picker.SessionView, 0, len(live)+len(stopped)) | 154 | + | catalogRows := 0 |
| 155 | + | for _, host := range catalog { | |||
| 156 | + | catalogRows += len(host.entry.Sessions) | |||
| 157 | + | if len(host.entry.Sessions) == 0 && (host.status == remoteHostUnreachable || host.status == remoteHostVersionMismatch) { | |||
| 158 | + | catalogRows++ | |||
| 159 | + | } | |||
| 160 | + | } | |||
| 161 | + | views := make([]picker.SessionView, 0, len(live)+len(stopped)+catalogRows) | |||
| 153 | for _, item := range live { | 162 | for _, item := range live { | ||
| 154 | if proxy, ok := item.entry.(*proxySession); ok { | 163 | if proxy, ok := item.entry.(*proxySession); ok { | ||
| 155 | views = append(views, remoteProxyPickerView(proxy.key, item.view)) | 164 | views = append(views, remoteProxyPickerView(proxy.key, item.view)) | ||
| @@ -163,7 +172,7 @@ func (d *Daemon) pickerViews(cur *session) ([]picker.SessionView, picker.SourceF | |||||
| 163 | if key.Validate() != nil { | 172 | if key.Validate() != nil { | ||
| 164 | continue | 173 | continue | ||
| 165 | } | 174 | } | ||
| 166 | - | if proxy, ok := registry[key.ID()].(*proxySession); ok && proxy.key == key { | 175 | + | if proxy, ok := proxies[key.ID()]; ok && proxy.key == key { |
| 167 | continue | 176 | continue | ||
| 168 | } | 177 | } | ||
| 169 | views = append(views, remotePickerView(key, session, host.status, host.entry.FetchedAt)) | 178 | views = append(views, remotePickerView(key, session, host.status, host.entry.FetchedAt)) | ||
| @@ -496,7 +505,18 @@ func (d *Daemon) clearDestroyedTabPreview(tb *tab) { | |||||
| 496 | } | 505 | } | ||
| 497 | 506 | ||||
| 498 | func (d *Daemon) closePicker(ac *attachedClient) { | 507 | func (d *Daemon) closePicker(ac *attachedClient) { | ||
| 508 | + | d.closePickerIfCurrent(ac, nil, 0) | |||
| 509 | + | } | |||
| 510 | + | ||||
| 511 | + | // closePickerIfCurrent closes only the observed picker lifecycle. A nonzero | |||
| 512 | + | // generation guards terminal parked cleanup even when a refresh replaced the | |||
| 513 | + | // model within that generation. | |||
| 514 | + | func (d *Daemon) closePickerIfCurrent(ac *attachedClient, model *picker.Model, generation uint64) bool { | |||
| 499 | ac.overlays.pickerMu.Lock() | 515 | ac.overlays.pickerMu.Lock() | ||
| 516 | + | if model != nil && (ac.overlays.picker != model || ac.overlays.pickerGeneration != generation) || model == nil && generation != 0 && ac.overlays.pickerGeneration != generation { | |||
| 517 | + | ac.overlays.pickerMu.Unlock() | |||
| 518 | + | return false | |||
| 519 | + | } | |||
| 500 | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: ac.overlays.picker} | 520 | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: ac.overlays.picker} | ||
| 501 | ac.overlays.picker = nil | 521 | ac.overlays.picker = nil | ||
| 502 | ac.overlays.pickerTitle = "" | 522 | ac.overlays.pickerTitle = "" | ||
| @@ -504,12 +524,13 @@ func (d *Daemon) closePicker(ac *attachedClient) { | |||||
| 504 | ac.overlays.pickerSource = moveSourceLocator{} | 524 | ac.overlays.pickerSource = moveSourceLocator{} | ||
| 505 | ac.overlays.pickerPending = nil | 525 | ac.overlays.pickerPending = nil | ||
| 506 | ac.overlays.pickerESC.stop() | 526 | ac.overlays.pickerESC.stop() | ||
| 507 | - | generation := ac.overlays.pickerPreviewGeneration | 527 | + | previewGeneration := ac.overlays.pickerPreviewGeneration |
| 508 | ac.overlays.pickerMu.Unlock() | 528 | ac.overlays.pickerMu.Unlock() | ||
| 509 | - | d.clearPreviewGeneration(ac, generation) | 529 | + | d.clearPreviewGeneration(ac, previewGeneration) |
| 510 | if instance.model != nil { | 530 | if instance.model != nil { | ||
| 511 | d.remotePickerClosed(instance) | 531 | d.remotePickerClosed(instance) | ||
| 512 | } | 532 | } | ||
| 533 | + | return true | |||
| 513 | } | 534 | } | ||
| 514 | 535 | ||||
| 515 | func pickerTargetsEqual(left, right picker.Target) bool { | 536 | func pickerTargetsEqual(left, right picker.Target) bool { | ||
M internal/usecase/daemon/remote_picker.go +3 -3
| @@ -313,6 +313,9 @@ func (d *Daemon) startRemotePickerRefresh(instance remotePickerInstance) uint64 | |||
| 313 | 313 | if previous != nil { | |
| 314 | 314 | previous() | |
| 315 | 315 | } | |
| 316 | + | if d.remoteCatalogClient == nil { | |
| 317 | + | return generation | |
| 318 | + | } | |
| 316 | 319 | ||
| 317 | 320 | hosts, err := d.remotePickerHosts() | |
| 318 | 321 | if err != nil { | |
| @@ -349,9 +352,6 @@ func (d *Daemon) startRemotePickerRefresh(instance remotePickerInstance) uint64 | |||
| 349 | 352 | d.refreshRemoteOpenPickers() | |
| 350 | 353 | } | |
| 351 | 354 | ||
| 352 | - | if d.remoteCatalogClient == nil { | |
| 353 | - | return generation | |
| 354 | - | } | |
| 355 | 355 | for _, host := range hosts { | |
| 356 | 356 | d.sessWg.Go(func() { | |
| 357 | 357 | catalog, listErr := d.remoteCatalogClient.List(ctx, host) | |
| @@ -313,6 +313,9 @@ func (d *Daemon) startRemotePickerRefresh(instance remotePickerInstance) uint64 | |||||
| 313 | if previous != nil { | 313 | if previous != nil { | ||
| 314 | previous() | 314 | previous() | ||
| 315 | } | 315 | } | ||
| 316 | + | if d.remoteCatalogClient == nil { | |||
| 317 | + | return generation | |||
| 318 | + | } | |||
| 316 | 319 | ||||
| 317 | hosts, err := d.remotePickerHosts() | 320 | hosts, err := d.remotePickerHosts() | ||
| 318 | if err != nil { | 321 | if err != nil { | ||
| @@ -349,9 +352,6 @@ func (d *Daemon) startRemotePickerRefresh(instance remotePickerInstance) uint64 | |||||
| 349 | d.refreshRemoteOpenPickers() | 352 | d.refreshRemoteOpenPickers() | ||
| 350 | } | 353 | } | ||
| 351 | 354 | ||||
| 352 | - | if d.remoteCatalogClient == nil { | |||
| 353 | - | return generation | |||
| 354 | - | } | |||
| 355 | for _, host := range hosts { | 355 | for _, host := range hosts { | ||
| 356 | d.sessWg.Go(func() { | 356 | d.sessWg.Go(func() { | ||
| 357 | catalog, listErr := d.remoteCatalogClient.List(ctx, host) | 357 | catalog, listErr := d.remoteCatalogClient.List(ctx, host) | ||
M internal/usecase/daemon/remote_picker_test.go +306 -40
| @@ -3,6 +3,7 @@ package daemon | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | + | "fmt" | |
| 6 | 7 | "io" | |
| 7 | 8 | "log/slog" | |
| 8 | 9 | "sync" | |
| @@ -17,6 +18,40 @@ import ( | |||
| 17 | 18 | "github.com/bnema/vev/internal/usecase/picker" | |
| 18 | 19 | ) | |
| 19 | 20 | ||
| 21 | + | const remotePickerReceiveTimeout = time.Second | |
| 22 | + | ||
| 23 | + | func receiveRemotePicker[T any](t *testing.T, ch <-chan T, what string) T { | |
| 24 | + | t.Helper() | |
| 25 | + | select { | |
| 26 | + | case value, ok := <-ch: | |
| 27 | + | if !ok { | |
| 28 | + | t.Fatalf("channel closed unexpectedly while waiting for %s", what) | |
| 29 | + | } | |
| 30 | + | return value | |
| 31 | + | case <-time.After(remotePickerReceiveTimeout): | |
| 32 | + | t.Fatalf("timed out waiting for %s", what) | |
| 33 | + | var zero T | |
| 34 | + | return zero | |
| 35 | + | } | |
| 36 | + | } | |
| 37 | + | ||
| 38 | + | func waitRemotePickerClose(ch <-chan struct{}, what string) error { | |
| 39 | + | select { | |
| 40 | + | case _, ok := <-ch: | |
| 41 | + | if ok { | |
| 42 | + | return fmt.Errorf("received signal instead of closure while waiting for %s", what) | |
| 43 | + | } | |
| 44 | + | return nil | |
| 45 | + | case <-time.After(remotePickerReceiveTimeout): | |
| 46 | + | return fmt.Errorf("timed out waiting for %s", what) | |
| 47 | + | } | |
| 48 | + | } | |
| 49 | + | ||
| 50 | + | func receiveRemotePickerClose(t *testing.T, ch <-chan struct{}, what string) { | |
| 51 | + | t.Helper() | |
| 52 | + | require.NoError(t, waitRemotePickerClose(ch, what)) | |
| 53 | + | } | |
| 54 | + | ||
| 20 | 55 | func newRemotePickerDaemon(store ports.RemoteHostStore) *Daemon { | |
| 21 | 56 | return New(nil, stubClock{}, slog.New(slog.NewTextHandler(io.Discard, nil)), WithRemoteDiscovery(store, nil, nil, nil, ports.RemoteTransportUDP)) | |
| 22 | 57 | } | |
| @@ -248,23 +283,25 @@ type remoteRefreshResult struct { | |||
| 248 | 283 | } | |
| 249 | 284 | ||
| 250 | 285 | type channelRemoteCatalog struct { | |
| 251 | - | d *Daemon | |
| 252 | - | requests chan remoteRefreshRequest | |
| 253 | - | probeMu sync.Mutex | |
| 286 | + | d *Daemon | |
| 287 | + | requests chan remoteRefreshRequest | |
| 288 | + | probeMu sync.Mutex | |
| 289 | + | probes bool | |
| 290 | + | violations []string | |
| 254 | 291 | } | |
| 255 | 292 | ||
| 256 | 293 | func (c *channelRemoteCatalog) List(ctx context.Context, host string) (ports.RemoteCatalog, error) { | |
| 257 | 294 | c.probeMu.Lock() | |
| 258 | - | if !c.d.mu.TryLock() { | |
| 259 | - | c.probeMu.Unlock() | |
| 260 | - | return ports.RemoteCatalog{}, errors.New("daemon lock held during remote catalog I/O") | |
| 295 | + | if c.probes && !c.d.mu.TryLock() { | |
| 296 | + | c.violations = append(c.violations, "daemon lock held during remote catalog I/O") | |
| 297 | + | } else if c.probes { | |
| 298 | + | c.d.mu.Unlock() | |
| 261 | 299 | } | |
| 262 | - | c.d.mu.Unlock() | |
| 263 | - | if !c.d.remoteCatalog.mu.TryLock() { | |
| 264 | - | c.probeMu.Unlock() | |
| 265 | - | return ports.RemoteCatalog{}, errors.New("remote catalog lock held during remote catalog I/O") | |
| 300 | + | if c.probes && !c.d.remoteCatalog.mu.TryLock() { | |
| 301 | + | c.violations = append(c.violations, "remote catalog lock held during remote catalog I/O") | |
| 302 | + | } else if c.probes { | |
| 303 | + | c.d.remoteCatalog.mu.Unlock() | |
| 266 | 304 | } | |
| 267 | - | c.d.remoteCatalog.mu.Unlock() | |
| 268 | 305 | c.probeMu.Unlock() | |
| 269 | 306 | request := remoteRefreshRequest{ctx: ctx, host: host, result: make(chan remoteRefreshResult, 1)} | |
| 270 | 307 | c.requests <- request | |
| @@ -277,21 +314,28 @@ func (c *channelRemoteCatalog) List(ctx context.Context, host string) (ports.Rem | |||
| 277 | 314 | } | |
| 278 | 315 | ||
| 279 | 316 | type recordingRemoteCache struct { | |
| 280 | - | d *Daemon | |
| 281 | - | stores chan []ports.RemoteCatalogCacheEntry | |
| 317 | + | d *Daemon | |
| 318 | + | stores chan []ports.RemoteCatalogCacheEntry | |
| 319 | + | probeMu sync.Mutex | |
| 320 | + | probes bool | |
| 321 | + | violations []string | |
| 282 | 322 | } | |
| 283 | 323 | ||
| 284 | 324 | func (*recordingRemoteCache) Load() ([]ports.RemoteCatalogCacheEntry, error) { return nil, nil } | |
| 285 | 325 | ||
| 286 | 326 | func (c *recordingRemoteCache) Store(entries []ports.RemoteCatalogCacheEntry) error { | |
| 287 | - | if !c.d.mu.TryLock() { | |
| 288 | - | return errors.New("daemon lock held during remote cache write") | |
| 327 | + | c.probeMu.Lock() | |
| 328 | + | if c.probes && !c.d.mu.TryLock() { | |
| 329 | + | c.violations = append(c.violations, "daemon lock held during remote cache write") | |
| 330 | + | } else if c.probes { | |
| 331 | + | c.d.mu.Unlock() | |
| 289 | 332 | } | |
| 290 | - | c.d.mu.Unlock() | |
| 291 | - | if !c.d.remoteCatalog.mu.TryLock() { | |
| 292 | - | return errors.New("remote catalog lock held during remote cache write") | |
| 333 | + | if c.probes && !c.d.remoteCatalog.mu.TryLock() { | |
| 334 | + | c.violations = append(c.violations, "remote catalog lock held during remote cache write") | |
| 335 | + | } else if c.probes { | |
| 336 | + | c.d.remoteCatalog.mu.Unlock() | |
| 293 | 337 | } | |
| 294 | - | c.d.remoteCatalog.mu.Unlock() | |
| 338 | + | c.probeMu.Unlock() | |
| 295 | 339 | copyEntries := make([]ports.RemoteCatalogCacheEntry, len(entries)) | |
| 296 | 340 | for i, entry := range entries { | |
| 297 | 341 | copyEntries[i] = cloneRemoteCatalogEntry(entry) | |
| @@ -305,6 +349,20 @@ type fixedRemoteRefreshClock struct{ now time.Time } | |||
| 305 | 349 | func (c fixedRemoteRefreshClock) Now() time.Time { return c.now } | |
| 306 | 350 | func (fixedRemoteRefreshClock) NewTimer(time.Duration) ports.Timer { return stubTimer{} } | |
| 307 | 351 | ||
| 352 | + | func requireNoRemoteLockViolations(t *testing.T, catalog *channelRemoteCatalog, cache *recordingRemoteCache) { | |
| 353 | + | t.Helper() | |
| 354 | + | if catalog != nil { | |
| 355 | + | catalog.probeMu.Lock() | |
| 356 | + | require.Empty(t, catalog.violations) | |
| 357 | + | catalog.probeMu.Unlock() | |
| 358 | + | } | |
| 359 | + | if cache != nil { | |
| 360 | + | cache.probeMu.Lock() | |
| 361 | + | require.Empty(t, cache.violations) | |
| 362 | + | cache.probeMu.Unlock() | |
| 363 | + | } | |
| 364 | + | } | |
| 365 | + | ||
| 308 | 366 | func newRemoteRefreshDaemon(t *testing.T, hosts *remoteRefreshHostStore, now time.Time) (*Daemon, *channelRemoteCatalog, *recordingRemoteCache) { | |
| 309 | 367 | t.Helper() | |
| 310 | 368 | catalog := &channelRemoteCatalog{requests: make(chan remoteRefreshRequest, 8)} | |
| @@ -333,11 +391,12 @@ func TestRemoteRefreshStartsPerHostAndCancelsSupersededGeneration(t *testing.T) | |||
| 333 | 391 | firstGeneration := d.startRemotePickerRefresh(instance) | |
| 334 | 392 | first := map[string]remoteRefreshRequest{} | |
| 335 | 393 | for range 2 { | |
| 336 | - | request := <-catalog.requests | |
| 394 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |
| 337 | 395 | first[request.host] = request | |
| 338 | 396 | } | |
| 339 | 397 | require.Contains(t, first, "arch") | |
| 340 | 398 | require.Contains(t, first, "mule") | |
| 399 | + | requireNoRemoteLockViolations(t, catalog, nil) | |
| 341 | 400 | d.remoteCatalog.mu.Lock() | |
| 342 | 401 | require.Equal(t, []ports.RemoteCatalogSession{{Name: "cached", State: "running"}}, d.remoteCatalog.cache["arch"].Sessions, "cached rows publish before any remote completion") | |
| 343 | 402 | require.Equal(t, remoteHostRefreshing, d.remoteCatalog.status["arch"]) | |
| @@ -346,15 +405,16 @@ func TestRemoteRefreshStartsPerHostAndCancelsSupersededGeneration(t *testing.T) | |||
| 346 | 405 | secondGeneration := d.startRemotePickerRefresh(instance) | |
| 347 | 406 | require.Greater(t, secondGeneration, firstGeneration) | |
| 348 | 407 | for _, request := range first { | |
| 349 | - | <-request.ctx.Done() | |
| 408 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") | |
| 350 | 409 | } | |
| 351 | 410 | for range 2 { | |
| 352 | - | request := <-catalog.requests | |
| 411 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |
| 353 | 412 | request.result <- remoteRefreshResult{catalog: ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}} | |
| 354 | 413 | } | |
| 355 | 414 | for range 2 { | |
| 356 | - | <-cache.stores | |
| 415 | + | receiveRemotePicker(t, cache.stores, "remote cache store") | |
| 357 | 416 | } | |
| 417 | + | requireNoRemoteLockViolations(t, nil, cache) | |
| 358 | 418 | ||
| 359 | 419 | d.remoteCatalog.mu.Lock() | |
| 360 | 420 | require.Equal(t, secondGeneration, d.remoteCatalog.refresh) | |
| @@ -363,6 +423,46 @@ func TestRemoteRefreshStartsPerHostAndCancelsSupersededGeneration(t *testing.T) | |||
| 363 | 423 | d.remoteCatalog.mu.Unlock() | |
| 364 | 424 | } | |
| 365 | 425 | ||
| 426 | + | func TestRemoteRefreshLockProbesSerialized(t *testing.T) { | |
| 427 | + | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |
| 428 | + | d, catalog, cache := newRemoteRefreshDaemon(t, hosts, time.Unix(125, 0)) | |
| 429 | + | catalog.probes, cache.probes = true, true | |
| 430 | + | ac := &attachedClient{} | |
| 431 | + | ac.initOverlays() | |
| 432 | + | model := picker.New(nil, picker.SelectionConfig{}) | |
| 433 | + | ac.overlays.pickerMu.Lock() | |
| 434 | + | ac.overlays.picker = model | |
| 435 | + | ac.overlays.pickerGeneration++ | |
| 436 | + | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: model} | |
| 437 | + | ac.overlays.pickerMu.Unlock() | |
| 438 | + | require.True(t, d.registerRemotePicker(instance)) | |
| 439 | + | ||
| 440 | + | d.startRemotePickerRefresh(instance) | |
| 441 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |
| 442 | + | request.result <- remoteRefreshResult{catalog: ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}} | |
| 443 | + | receiveRemotePicker(t, cache.stores, "remote cache store") | |
| 444 | + | requireNoRemoteLockViolations(t, catalog, cache) | |
| 445 | + | } | |
| 446 | + | ||
| 447 | + | func TestRemoteRefreshWithoutCatalogClientDoesNotMarkHostsRefreshing(t *testing.T) { | |
| 448 | + | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |
| 449 | + | d := newRemotePickerDaemon(hosts) | |
| 450 | + | ac := &attachedClient{} | |
| 451 | + | ac.initOverlays() | |
| 452 | + | model := picker.New(nil, picker.SelectionConfig{}) | |
| 453 | + | ac.overlays.pickerMu.Lock() | |
| 454 | + | ac.overlays.picker = model | |
| 455 | + | ac.overlays.pickerGeneration++ | |
| 456 | + | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: model} | |
| 457 | + | ac.overlays.pickerMu.Unlock() | |
| 458 | + | require.True(t, d.registerRemotePicker(instance)) | |
| 459 | + | ||
| 460 | + | require.NotZero(t, d.startRemotePickerRefresh(instance)) | |
| 461 | + | d.remoteCatalog.mu.Lock() | |
| 462 | + | require.NotEqual(t, remoteHostRefreshing, d.remoteCatalog.status["arch"]) | |
| 463 | + | d.remoteCatalog.mu.Unlock() | |
| 464 | + | } | |
| 465 | + | ||
| 366 | 466 | func TestRemoteRefreshResultPreservesFailuresAndEvictsSuccessfulOmissions(t *testing.T) { | |
| 367 | 467 | now := time.Unix(200, 0) | |
| 368 | 468 | hosts := &remoteRefreshHostStore{hosts: []string{"arch", "mule"}} | |
| @@ -383,11 +483,12 @@ func TestRemoteRefreshResultPreservesFailuresAndEvictsSuccessfulOmissions(t *tes | |||
| 383 | 483 | d.remoteCatalog.mu.Unlock() | |
| 384 | 484 | ||
| 385 | 485 | require.True(t, d.applyRemoteRefreshResult(7, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}, nil)) | |
| 386 | - | stored := <-cache.stores | |
| 486 | + | stored := receiveRemotePicker(t, cache.stores, "remote cache store") | |
| 387 | 487 | require.Equal(t, []ports.RemoteCatalogCacheEntry{ | |
| 388 | 488 | {Host: "arch", FetchedAt: now, Sessions: []ports.RemoteCatalogSession{}}, | |
| 389 | 489 | {Host: "mule", FetchedAt: time.Unix(20, 0), Sessions: []ports.RemoteCatalogSession{{Name: "stale", State: "running"}}}, | |
| 390 | 490 | }, stored, "every successful write persists the newest full cache atomically") | |
| 491 | + | requireNoRemoteLockViolations(t, nil, cache) | |
| 391 | 492 | d.remoteCatalog.mu.Lock() | |
| 392 | 493 | require.Empty(t, d.remoteCatalog.cache["arch"].Sessions, "a successful omission authoritatively evicts old rows") | |
| 393 | 494 | require.Equal(t, remoteHostFresh, d.remoteCatalog.status["arch"]) | |
| @@ -407,7 +508,8 @@ func TestRemoteRefreshRejectsRemovedHostAndLateGeneration(t *testing.T) { | |||
| 407 | 508 | require.False(t, d.applyRemoteRefreshResult(8, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion, Sessions: []ports.RemoteCatalogSession{{Name: "late", State: "running"}}}, nil)) | |
| 408 | 509 | hosts.set() | |
| 409 | 510 | require.True(t, d.applyRemoteRefreshResult(9, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion, Sessions: []ports.RemoteCatalogSession{{Name: "removed", State: "running"}}}, nil)) | |
| 410 | - | require.Empty(t, <-cache.stores) | |
| 511 | + | require.Empty(t, receiveRemotePicker(t, cache.stores, "remote cache store")) | |
| 512 | + | requireNoRemoteLockViolations(t, nil, cache) | |
| 411 | 513 | d.remoteCatalog.mu.Lock() | |
| 412 | 514 | require.NotContains(t, d.remoteCatalog.cache, "arch") | |
| 413 | 515 | require.NotContains(t, d.remoteCatalog.status, "arch") | |
| @@ -446,7 +548,7 @@ func addRemoteRefreshPickerOwner(t *testing.T, d *Daemon, id domain.SessionID) ( | |||
| 446 | 548 | ||
| 447 | 549 | func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |
| 448 | 550 | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |
| 449 | - | d, _, cache := newRemoteRefreshDaemon(t, hosts, time.Unix(450, 0)) | |
| 551 | + | d, catalog, cache := newRemoteRefreshDaemon(t, hosts, time.Unix(450, 0)) | |
| 450 | 552 | firstSession, first, _ := addRemoteRefreshPickerOwner(t, d, "first") | |
| 451 | 553 | _, second, _ := addRemoteRefreshPickerOwner(t, d, "second") | |
| 452 | 554 | before := make(map[*attachedClient]*picker.Model) | |
| @@ -455,11 +557,11 @@ func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |||
| 455 | 557 | owner.overlays.pickerMu.Lock() | |
| 456 | 558 | owner.overlays.picker = d.newPickerModel(firstSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{Session: "second", TabID: "second-tab"}) | |
| 457 | 559 | owner.overlays.pickerIntent = pickerNavigate | |
| 560 | + | owner.overlays.pickerGeneration++ | |
| 561 | + | instance := remotePickerInstance{ac: owner, generation: owner.overlays.pickerGeneration, model: owner.overlays.picker} | |
| 458 | 562 | before[owner] = owner.overlays.picker | |
| 459 | 563 | owner.overlays.pickerMu.Unlock() | |
| 460 | - | d.remoteCatalog.mu.Lock() | |
| 461 | - | d.remoteCatalog.pickers[owner] = struct{}{} | |
| 462 | - | d.remoteCatalog.mu.Unlock() | |
| 564 | + | require.True(t, d.registerRemotePicker(instance)) | |
| 463 | 565 | } | |
| 464 | 566 | d.remoteCatalog.mu.Lock() | |
| 465 | 567 | d.remoteCatalog.refresh = 4 | |
| @@ -469,7 +571,8 @@ func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |||
| 469 | 571 | ProtocolVersion: ports.ProtocolVersion, | |
| 470 | 572 | Sessions: []ports.RemoteCatalogSession{{Name: "work", State: "running", Tabs: 2}}, | |
| 471 | 573 | }, nil)) | |
| 472 | - | <-cache.stores | |
| 574 | + | receiveRemotePicker(t, cache.stores, "remote cache store") | |
| 575 | + | requireNoRemoteLockViolations(t, catalog, cache) | |
| 473 | 576 | ||
| 474 | 577 | for _, owner := range []*attachedClient{first, second} { | |
| 475 | 578 | owner.overlays.pickerMu.Lock() | |
| @@ -482,6 +585,165 @@ func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |||
| 482 | 585 | } | |
| 483 | 586 | } | |
| 484 | 587 | ||
| 588 | + | func TestRemotePickerTeardownLifecycle(t *testing.T) { | |
| 589 | + | for _, test := range []struct { | |
| 590 | + | name string | |
| 591 | + | resumeCapable bool | |
| 592 | + | teardown func(*Daemon, *session, *attachedClient) | |
| 593 | + | }{ | |
| 594 | + | {name: "client gone parks resumable attachment", resumeCapable: true, teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.clientGone(sess, ac, ac.transport(), false) }}, | |
| 595 | + | {name: "send error parks resumable attachment", resumeCapable: true, teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.detachOnSendError(sess, ac, ac.transport()) }}, | |
| 596 | + | {name: "client gone removes non-resumable attachment", teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.clientGone(sess, ac, ac.transport(), false) }}, | |
| 597 | + | {name: "send error removes non-resumable attachment", teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.detachOnSendError(sess, ac, ac.transport()) }}, | |
| 598 | + | } { | |
| 599 | + | t.Run(test.name, func(t *testing.T) { | |
| 600 | + | d := newRemotePickerDaemon(nil) | |
| 601 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |
| 602 | + | ac.resumeCapable = test.resumeCapable | |
| 603 | + | model := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |
| 604 | + | d.publishPicker(sess, ac, model, pickerNavigate, moveSourceLocator{}) | |
| 605 | + | ac.overlays.pickerMu.Lock() | |
| 606 | + | generation := ac.overlays.pickerGeneration | |
| 607 | + | ac.overlays.pickerMu.Unlock() | |
| 608 | + | ||
| 609 | + | test.teardown(d, sess, ac) | |
| 610 | + | ||
| 611 | + | d.remoteCatalog.mu.Lock() | |
| 612 | + | _, registered := d.remoteCatalog.pickers[ac] | |
| 613 | + | d.remoteCatalog.mu.Unlock() | |
| 614 | + | ac.overlays.pickerMu.Lock() | |
| 615 | + | current, currentGeneration := ac.overlays.picker, ac.overlays.pickerGeneration | |
| 616 | + | ac.overlays.pickerMu.Unlock() | |
| 617 | + | if !test.resumeCapable { | |
| 618 | + | require.False(t, registered) | |
| 619 | + | require.Nil(t, current) | |
| 620 | + | return | |
| 621 | + | } | |
| 622 | + | require.True(t, ac.parked) | |
| 623 | + | require.True(t, registered) | |
| 624 | + | require.Same(t, model, current) | |
| 625 | + | require.Equal(t, generation, currentGeneration) | |
| 626 | + | d.mu.Lock() | |
| 627 | + | parked := d.parked[ac.resumeToken] | |
| 628 | + | d.mu.Unlock() | |
| 629 | + | require.NotNil(t, parked) | |
| 630 | + | require.Same(t, ac, parked.ac) | |
| 631 | + | }) | |
| 632 | + | } | |
| 633 | + | } | |
| 634 | + | ||
| 635 | + | func TestParkedPickerExpiryClosesCapturedGenerationAndCancelsRefresh(t *testing.T) { | |
| 636 | + | clock := &signalClock{timers: make(chan *signalTimer, 1)} | |
| 637 | + | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |
| 638 | + | d, catalog, _ := newRemoteRefreshDaemon(t, hosts, time.Unix(475, 0)) | |
| 639 | + | d.clock = clock | |
| 640 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |
| 641 | + | ac.resumeCapable = true | |
| 642 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |
| 643 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |
| 644 | + | ||
| 645 | + | require.True(t, d.parkAttachment(sess, ac)) | |
| 646 | + | timer := receiveRemotePicker(t, clock.timers, "park expiry timer") | |
| 647 | + | timer.ch <- clock.Now() | |
| 648 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") | |
| 649 | + | ||
| 650 | + | ac.overlays.pickerMu.Lock() | |
| 651 | + | require.Nil(t, ac.overlays.picker) | |
| 652 | + | ac.overlays.pickerMu.Unlock() | |
| 653 | + | d.remoteCatalog.mu.Lock() | |
| 654 | + | require.NotContains(t, d.remoteCatalog.pickers, ac) | |
| 655 | + | d.remoteCatalog.mu.Unlock() | |
| 656 | + | } | |
| 657 | + | ||
| 658 | + | func TestParkedPickerRetirementClosesCapturedGenerationOnly(t *testing.T) { | |
| 659 | + | d := newRemotePickerDaemon(nil) | |
| 660 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |
| 661 | + | ac.resumeCapable = true | |
| 662 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |
| 663 | + | d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1}) | |
| 664 | + | ac.overlays.pickerMu.Lock() | |
| 665 | + | refreshed := ac.overlays.picker | |
| 666 | + | ac.overlays.pickerMu.Unlock() | |
| 667 | + | require.NotNil(t, refreshed) | |
| 668 | + | require.True(t, d.parkAttachment(sess, ac)) | |
| 669 | + | token := ac.resumeToken | |
| 670 | + | d.mu.Lock() | |
| 671 | + | parked := d.parked[token] | |
| 672 | + | d.mu.Unlock() | |
| 673 | + | require.NotNil(t, parked) | |
| 674 | + | ||
| 675 | + | d.mu.Lock() | |
| 676 | + | retirement := d.retireParkedAttachmentLocked(token, parked) | |
| 677 | + | d.mu.Unlock() | |
| 678 | + | d.finishParkedAttachmentRetirements([]parkedAttachmentRetirement{retirement}) | |
| 679 | + | ac.overlays.pickerMu.Lock() | |
| 680 | + | require.Nil(t, ac.overlays.picker, "terminal retirement closes a refreshed model in its captured generation") | |
| 681 | + | ac.overlays.pickerMu.Unlock() | |
| 682 | + | } | |
| 683 | + | ||
| 684 | + | func TestParkedPickerTokenReplacementRetiresPreviousGeneration(t *testing.T) { | |
| 685 | + | d := newRemotePickerDaemon(nil) | |
| 686 | + | firstSession, first, _ := addRemoteRefreshPickerOwner(t, d, "first") | |
| 687 | + | secondSession, second, _ := addRemoteRefreshPickerOwner(t, d, "second") | |
| 688 | + | first.resumeCapable, second.resumeCapable = true, true | |
| 689 | + | d.publishPicker(firstSession, first, d.newPickerModel(firstSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |
| 690 | + | require.True(t, d.parkAttachment(firstSession, first)) | |
| 691 | + | token := first.resumeToken | |
| 692 | + | d.publishPicker(secondSession, second, d.newPickerModel(secondSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |
| 693 | + | second.resumeToken = token | |
| 694 | + | require.True(t, d.parkAttachment(secondSession, second)) | |
| 695 | + | ||
| 696 | + | first.overlays.pickerMu.Lock() | |
| 697 | + | require.Nil(t, first.overlays.picker) | |
| 698 | + | first.overlays.pickerMu.Unlock() | |
| 699 | + | second.overlays.pickerMu.Lock() | |
| 700 | + | require.NotNil(t, second.overlays.picker) | |
| 701 | + | second.overlays.pickerMu.Unlock() | |
| 702 | + | } | |
| 703 | + | ||
| 704 | + | func TestParkedPickerResumePreservesGeneration(t *testing.T) { | |
| 705 | + | d := newRemotePickerDaemon(nil) | |
| 706 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |
| 707 | + | ac.resumeCapable = true | |
| 708 | + | ac.clientID = [16]byte{1, 2, 3, 4} | |
| 709 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |
| 710 | + | ac.overlays.pickerMu.Lock() | |
| 711 | + | model, generation := ac.overlays.picker, ac.overlays.pickerGeneration | |
| 712 | + | ac.overlays.pickerMu.Unlock() | |
| 713 | + | d.clientGone(sess, ac, ac.transport(), false) | |
| 714 | + | token := ac.resumeToken | |
| 715 | + | tr := &closeTrackingTransport{} | |
| 716 | + | resumedSess, resumedAC, ok, err := d.resumeParked(helloResumeCapable(ports.IntentResume, sess.name, token), tr, domain.Size{Cols: 80, Rows: 24}) | |
| 717 | + | require.NoError(t, err) | |
| 718 | + | require.True(t, ok) | |
| 719 | + | require.Same(t, sess, resumedSess) | |
| 720 | + | require.Same(t, ac, resumedAC) | |
| 721 | + | ac.overlays.pickerMu.Lock() | |
| 722 | + | require.Same(t, model, ac.overlays.picker) | |
| 723 | + | require.Equal(t, generation, ac.overlays.pickerGeneration) | |
| 724 | + | ac.overlays.pickerMu.Unlock() | |
| 725 | + | } | |
| 726 | + | ||
| 727 | + | func TestStaleParkedPickerExpiryPreservesNewGeneration(t *testing.T) { | |
| 728 | + | d := newRemotePickerDaemon(nil) | |
| 729 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |
| 730 | + | ac.resumeCapable = true | |
| 731 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |
| 732 | + | require.True(t, d.parkAttachment(sess, ac)) | |
| 733 | + | token := ac.resumeToken | |
| 734 | + | d.mu.Lock() | |
| 735 | + | parked := d.parked[token] | |
| 736 | + | d.mu.Unlock() | |
| 737 | + | require.NotNil(t, parked) | |
| 738 | + | ||
| 739 | + | newer := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |
| 740 | + | d.publishPicker(sess, ac, newer, pickerNavigate, moveSourceLocator{}) | |
| 741 | + | d.expireParked(token, parked) | |
| 742 | + | ac.overlays.pickerMu.Lock() | |
| 743 | + | require.Same(t, newer, ac.overlays.picker) | |
| 744 | + | ac.overlays.pickerMu.Unlock() | |
| 745 | + | } | |
| 746 | + | ||
| 485 | 747 | func TestRemoteRefreshPreservesRemoteCursorAcrossCacheToProxyUpgrade(t *testing.T) { | |
| 486 | 748 | d := newRemotePickerDaemon(nil) | |
| 487 | 749 | ownerSession, owner, _ := addRemoteRefreshPickerOwner(t, d, "local") | |
| @@ -542,10 +804,10 @@ func TestRemoteRefreshCancellationWhenLastPickerCloses(t *testing.T) { | |||
| 542 | 804 | } | |
| 543 | 805 | first, second := newInstance(), newInstance() | |
| 544 | 806 | d.remotePickerOpened(first) | |
| 545 | - | request := <-catalog.requests | |
| 807 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |
| 546 | 808 | d.remotePickerOpened(second) | |
| 547 | - | <-request.ctx.Done() | |
| 548 | - | request = <-catalog.requests | |
| 809 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") | |
| 810 | + | request = receiveRemotePicker(t, catalog.requests, "catalog request") | |
| 549 | 811 | ||
| 550 | 812 | closeInstance(first) | |
| 551 | 813 | select { | |
| @@ -554,7 +816,7 @@ func TestRemoteRefreshCancellationWhenLastPickerCloses(t *testing.T) { | |||
| 554 | 816 | default: | |
| 555 | 817 | } | |
| 556 | 818 | closeInstance(second) | |
| 557 | - | <-request.ctx.Done() | |
| 819 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") | |
| 558 | 820 | ||
| 559 | 821 | d.remoteCatalog.mu.Lock() | |
| 560 | 822 | require.Empty(t, d.remoteCatalog.pickers) | |
| @@ -606,16 +868,17 @@ func TestRemotePickerPublishCannotRegisterAfterSnatchClear(t *testing.T) { | |||
| 606 | 868 | model := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |
| 607 | 869 | registrationReached := make(chan struct{}) | |
| 608 | 870 | allowRegistration := make(chan struct{}) | |
| 871 | + | registrationWait := make(chan error, 1) | |
| 609 | 872 | ac.overlays.beforeRemotePickerRegistration = func() { | |
| 610 | 873 | close(registrationReached) | |
| 611 | - | <-allowRegistration | |
| 874 | + | registrationWait <- waitRemotePickerClose(allowRegistration, "picker registration release") | |
| 612 | 875 | } | |
| 613 | 876 | published := make(chan struct{}) | |
| 614 | 877 | go func() { | |
| 615 | 878 | d.publishPicker(sess, ac, model, pickerNavigate, moveSourceLocator{}) | |
| 616 | 879 | close(published) | |
| 617 | 880 | }() | |
| 618 | - | <-registrationReached | |
| 881 | + | receiveRemotePickerClose(t, registrationReached, "picker registration") | |
| 619 | 882 | ||
| 620 | 883 | token := attachmentRoleToken{ | |
| 621 | 884 | sess: sess, ac: ac, role: attachmentSnatched, | |
| @@ -623,7 +886,8 @@ func TestRemotePickerPublishCannotRegisterAfterSnatchClear(t *testing.T) { | |||
| 623 | 886 | } | |
| 624 | 887 | require.True(t, d.clearForSnatch(token)) | |
| 625 | 888 | close(allowRegistration) | |
| 626 | - | <-published | |
| 889 | + | require.NoError(t, receiveRemotePicker(t, registrationWait, "picker registration release")) | |
| 890 | + | receiveRemotePickerClose(t, published, "picker publication") | |
| 627 | 891 | ||
| 628 | 892 | d.remoteCatalog.mu.Lock() | |
| 629 | 893 | _, registered := d.remoteCatalog.pickers[ac] | |
| @@ -663,22 +927,24 @@ func TestRemotePickerStaleRefreshCannotOverwriteReopenedModel(t *testing.T) { | |||
| 663 | 927 | ||
| 664 | 928 | rebuildReached := make(chan struct{}) | |
| 665 | 929 | allowPublication := make(chan struct{}) | |
| 930 | + | publicationWait := make(chan error, 1) | |
| 666 | 931 | ac.overlays.afterPickerRefreshBuild = func(*picker.Model) { | |
| 667 | 932 | close(rebuildReached) | |
| 668 | - | <-allowPublication | |
| 933 | + | publicationWait <- waitRemotePickerClose(allowPublication, "picker publication release") | |
| 669 | 934 | } | |
| 670 | 935 | refreshed := make(chan struct{}) | |
| 671 | 936 | go func() { | |
| 672 | 937 | d.refreshRemoteOpenPickers() | |
| 673 | 938 | close(refreshed) | |
| 674 | 939 | }() | |
| 675 | - | <-rebuildReached | |
| 940 | + | receiveRemotePickerClose(t, rebuildReached, "picker rebuild") | |
| 676 | 941 | ||
| 677 | 942 | d.closePicker(ac) | |
| 678 | 943 | reopened := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |
| 679 | 944 | d.publishPicker(sess, ac, reopened, pickerNavigate, moveSourceLocator{}) | |
| 680 | 945 | close(allowPublication) | |
| 681 | - | <-refreshed | |
| 946 | + | require.NoError(t, receiveRemotePicker(t, publicationWait, "picker publication release")) | |
| 947 | + | receiveRemotePickerClose(t, refreshed, "picker refresh") | |
| 682 | 948 | ||
| 683 | 949 | ac.overlays.pickerMu.Lock() | |
| 684 | 950 | require.Same(t, reopened, ac.overlays.picker, "a rebuild for the old picker must not publish into its replacement") | |
| @@ -3,6 +3,7 @@ package daemon | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | + | "fmt" | |||
| 6 | "io" | 7 | "io" | ||
| 7 | "log/slog" | 8 | "log/slog" | ||
| 8 | "sync" | 9 | "sync" | ||
| @@ -17,6 +18,40 @@ import ( | |||||
| 17 | "github.com/bnema/vev/internal/usecase/picker" | 18 | "github.com/bnema/vev/internal/usecase/picker" | ||
| 18 | ) | 19 | ) | ||
| 19 | 20 | ||||
| 21 | + | const remotePickerReceiveTimeout = time.Second | |||
| 22 | + | ||||
| 23 | + | func receiveRemotePicker[T any](t *testing.T, ch <-chan T, what string) T { | |||
| 24 | + | t.Helper() | |||
| 25 | + | select { | |||
| 26 | + | case value, ok := <-ch: | |||
| 27 | + | if !ok { | |||
| 28 | + | t.Fatalf("channel closed unexpectedly while waiting for %s", what) | |||
| 29 | + | } | |||
| 30 | + | return value | |||
| 31 | + | case <-time.After(remotePickerReceiveTimeout): | |||
| 32 | + | t.Fatalf("timed out waiting for %s", what) | |||
| 33 | + | var zero T | |||
| 34 | + | return zero | |||
| 35 | + | } | |||
| 36 | + | } | |||
| 37 | + | ||||
| 38 | + | func waitRemotePickerClose(ch <-chan struct{}, what string) error { | |||
| 39 | + | select { | |||
| 40 | + | case _, ok := <-ch: | |||
| 41 | + | if ok { | |||
| 42 | + | return fmt.Errorf("received signal instead of closure while waiting for %s", what) | |||
| 43 | + | } | |||
| 44 | + | return nil | |||
| 45 | + | case <-time.After(remotePickerReceiveTimeout): | |||
| 46 | + | return fmt.Errorf("timed out waiting for %s", what) | |||
| 47 | + | } | |||
| 48 | + | } | |||
| 49 | + | ||||
| 50 | + | func receiveRemotePickerClose(t *testing.T, ch <-chan struct{}, what string) { | |||
| 51 | + | t.Helper() | |||
| 52 | + | require.NoError(t, waitRemotePickerClose(ch, what)) | |||
| 53 | + | } | |||
| 54 | + | ||||
| 20 | func newRemotePickerDaemon(store ports.RemoteHostStore) *Daemon { | 55 | func newRemotePickerDaemon(store ports.RemoteHostStore) *Daemon { | ||
| 21 | return New(nil, stubClock{}, slog.New(slog.NewTextHandler(io.Discard, nil)), WithRemoteDiscovery(store, nil, nil, nil, ports.RemoteTransportUDP)) | 56 | return New(nil, stubClock{}, slog.New(slog.NewTextHandler(io.Discard, nil)), WithRemoteDiscovery(store, nil, nil, nil, ports.RemoteTransportUDP)) | ||
| 22 | } | 57 | } | ||
| @@ -248,23 +283,25 @@ type remoteRefreshResult struct { | |||||
| 248 | } | 283 | } | ||
| 249 | 284 | ||||
| 250 | type channelRemoteCatalog struct { | 285 | type channelRemoteCatalog struct { | ||
| 251 | - | d *Daemon | 286 | + | d *Daemon |
| 252 | - | requests chan remoteRefreshRequest | 287 | + | requests chan remoteRefreshRequest |
| 253 | - | probeMu sync.Mutex | 288 | + | probeMu sync.Mutex |
| 289 | + | probes bool | |||
| 290 | + | violations []string | |||
| 254 | } | 291 | } | ||
| 255 | 292 | ||||
| 256 | func (c *channelRemoteCatalog) List(ctx context.Context, host string) (ports.RemoteCatalog, error) { | 293 | func (c *channelRemoteCatalog) List(ctx context.Context, host string) (ports.RemoteCatalog, error) { | ||
| 257 | c.probeMu.Lock() | 294 | c.probeMu.Lock() | ||
| 258 | - | if !c.d.mu.TryLock() { | 295 | + | if c.probes && !c.d.mu.TryLock() { |
| 259 | - | c.probeMu.Unlock() | 296 | + | c.violations = append(c.violations, "daemon lock held during remote catalog I/O") |
| 260 | - | return ports.RemoteCatalog{}, errors.New("daemon lock held during remote catalog I/O") | 297 | + | } else if c.probes { |
| 298 | + | c.d.mu.Unlock() | |||
| 261 | } | 299 | } | ||
| 262 | - | c.d.mu.Unlock() | 300 | + | if c.probes && !c.d.remoteCatalog.mu.TryLock() { |
| 263 | - | if !c.d.remoteCatalog.mu.TryLock() { | 301 | + | c.violations = append(c.violations, "remote catalog lock held during remote catalog I/O") |
| 264 | - | c.probeMu.Unlock() | 302 | + | } else if c.probes { |
| 265 | - | return ports.RemoteCatalog{}, errors.New("remote catalog lock held during remote catalog I/O") | 303 | + | c.d.remoteCatalog.mu.Unlock() |
| 266 | } | 304 | } | ||
| 267 | - | c.d.remoteCatalog.mu.Unlock() | |||
| 268 | c.probeMu.Unlock() | 305 | c.probeMu.Unlock() | ||
| 269 | request := remoteRefreshRequest{ctx: ctx, host: host, result: make(chan remoteRefreshResult, 1)} | 306 | request := remoteRefreshRequest{ctx: ctx, host: host, result: make(chan remoteRefreshResult, 1)} | ||
| 270 | c.requests <- request | 307 | c.requests <- request | ||
| @@ -277,21 +314,28 @@ func (c *channelRemoteCatalog) List(ctx context.Context, host string) (ports.Rem | |||||
| 277 | } | 314 | } | ||
| 278 | 315 | ||||
| 279 | type recordingRemoteCache struct { | 316 | type recordingRemoteCache struct { | ||
| 280 | - | d *Daemon | 317 | + | d *Daemon |
| 281 | - | stores chan []ports.RemoteCatalogCacheEntry | 318 | + | stores chan []ports.RemoteCatalogCacheEntry |
| 319 | + | probeMu sync.Mutex | |||
| 320 | + | probes bool | |||
| 321 | + | violations []string | |||
| 282 | } | 322 | } | ||
| 283 | 323 | ||||
| 284 | func (*recordingRemoteCache) Load() ([]ports.RemoteCatalogCacheEntry, error) { return nil, nil } | 324 | func (*recordingRemoteCache) Load() ([]ports.RemoteCatalogCacheEntry, error) { return nil, nil } | ||
| 285 | 325 | ||||
| 286 | func (c *recordingRemoteCache) Store(entries []ports.RemoteCatalogCacheEntry) error { | 326 | func (c *recordingRemoteCache) Store(entries []ports.RemoteCatalogCacheEntry) error { | ||
| 287 | - | if !c.d.mu.TryLock() { | 327 | + | c.probeMu.Lock() |
| 288 | - | return errors.New("daemon lock held during remote cache write") | 328 | + | if c.probes && !c.d.mu.TryLock() { |
| 329 | + | c.violations = append(c.violations, "daemon lock held during remote cache write") | |||
| 330 | + | } else if c.probes { | |||
| 331 | + | c.d.mu.Unlock() | |||
| 289 | } | 332 | } | ||
| 290 | - | c.d.mu.Unlock() | 333 | + | if c.probes && !c.d.remoteCatalog.mu.TryLock() { |
| 291 | - | if !c.d.remoteCatalog.mu.TryLock() { | 334 | + | c.violations = append(c.violations, "remote catalog lock held during remote cache write") |
| 292 | - | return errors.New("remote catalog lock held during remote cache write") | 335 | + | } else if c.probes { |
| 336 | + | c.d.remoteCatalog.mu.Unlock() | |||
| 293 | } | 337 | } | ||
| 294 | - | c.d.remoteCatalog.mu.Unlock() | 338 | + | c.probeMu.Unlock() |
| 295 | copyEntries := make([]ports.RemoteCatalogCacheEntry, len(entries)) | 339 | copyEntries := make([]ports.RemoteCatalogCacheEntry, len(entries)) | ||
| 296 | for i, entry := range entries { | 340 | for i, entry := range entries { | ||
| 297 | copyEntries[i] = cloneRemoteCatalogEntry(entry) | 341 | copyEntries[i] = cloneRemoteCatalogEntry(entry) | ||
| @@ -305,6 +349,20 @@ type fixedRemoteRefreshClock struct{ now time.Time } | |||||
| 305 | func (c fixedRemoteRefreshClock) Now() time.Time { return c.now } | 349 | func (c fixedRemoteRefreshClock) Now() time.Time { return c.now } | ||
| 306 | func (fixedRemoteRefreshClock) NewTimer(time.Duration) ports.Timer { return stubTimer{} } | 350 | func (fixedRemoteRefreshClock) NewTimer(time.Duration) ports.Timer { return stubTimer{} } | ||
| 307 | 351 | ||||
| 352 | + | func requireNoRemoteLockViolations(t *testing.T, catalog *channelRemoteCatalog, cache *recordingRemoteCache) { | |||
| 353 | + | t.Helper() | |||
| 354 | + | if catalog != nil { | |||
| 355 | + | catalog.probeMu.Lock() | |||
| 356 | + | require.Empty(t, catalog.violations) | |||
| 357 | + | catalog.probeMu.Unlock() | |||
| 358 | + | } | |||
| 359 | + | if cache != nil { | |||
| 360 | + | cache.probeMu.Lock() | |||
| 361 | + | require.Empty(t, cache.violations) | |||
| 362 | + | cache.probeMu.Unlock() | |||
| 363 | + | } | |||
| 364 | + | } | |||
| 365 | + | ||||
| 308 | func newRemoteRefreshDaemon(t *testing.T, hosts *remoteRefreshHostStore, now time.Time) (*Daemon, *channelRemoteCatalog, *recordingRemoteCache) { | 366 | func newRemoteRefreshDaemon(t *testing.T, hosts *remoteRefreshHostStore, now time.Time) (*Daemon, *channelRemoteCatalog, *recordingRemoteCache) { | ||
| 309 | t.Helper() | 367 | t.Helper() | ||
| 310 | catalog := &channelRemoteCatalog{requests: make(chan remoteRefreshRequest, 8)} | 368 | catalog := &channelRemoteCatalog{requests: make(chan remoteRefreshRequest, 8)} | ||
| @@ -333,11 +391,12 @@ func TestRemoteRefreshStartsPerHostAndCancelsSupersededGeneration(t *testing.T) | |||||
| 333 | firstGeneration := d.startRemotePickerRefresh(instance) | 391 | firstGeneration := d.startRemotePickerRefresh(instance) | ||
| 334 | first := map[string]remoteRefreshRequest{} | 392 | first := map[string]remoteRefreshRequest{} | ||
| 335 | for range 2 { | 393 | for range 2 { | ||
| 336 | - | request := <-catalog.requests | 394 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") |
| 337 | first[request.host] = request | 395 | first[request.host] = request | ||
| 338 | } | 396 | } | ||
| 339 | require.Contains(t, first, "arch") | 397 | require.Contains(t, first, "arch") | ||
| 340 | require.Contains(t, first, "mule") | 398 | require.Contains(t, first, "mule") | ||
| 399 | + | requireNoRemoteLockViolations(t, catalog, nil) | |||
| 341 | d.remoteCatalog.mu.Lock() | 400 | d.remoteCatalog.mu.Lock() | ||
| 342 | require.Equal(t, []ports.RemoteCatalogSession{{Name: "cached", State: "running"}}, d.remoteCatalog.cache["arch"].Sessions, "cached rows publish before any remote completion") | 401 | require.Equal(t, []ports.RemoteCatalogSession{{Name: "cached", State: "running"}}, d.remoteCatalog.cache["arch"].Sessions, "cached rows publish before any remote completion") | ||
| 343 | require.Equal(t, remoteHostRefreshing, d.remoteCatalog.status["arch"]) | 402 | require.Equal(t, remoteHostRefreshing, d.remoteCatalog.status["arch"]) | ||
| @@ -346,15 +405,16 @@ func TestRemoteRefreshStartsPerHostAndCancelsSupersededGeneration(t *testing.T) | |||||
| 346 | secondGeneration := d.startRemotePickerRefresh(instance) | 405 | secondGeneration := d.startRemotePickerRefresh(instance) | ||
| 347 | require.Greater(t, secondGeneration, firstGeneration) | 406 | require.Greater(t, secondGeneration, firstGeneration) | ||
| 348 | for _, request := range first { | 407 | for _, request := range first { | ||
| 349 | - | <-request.ctx.Done() | 408 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") |
| 350 | } | 409 | } | ||
| 351 | for range 2 { | 410 | for range 2 { | ||
| 352 | - | request := <-catalog.requests | 411 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") |
| 353 | request.result <- remoteRefreshResult{catalog: ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}} | 412 | request.result <- remoteRefreshResult{catalog: ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}} | ||
| 354 | } | 413 | } | ||
| 355 | for range 2 { | 414 | for range 2 { | ||
| 356 | - | <-cache.stores | 415 | + | receiveRemotePicker(t, cache.stores, "remote cache store") |
| 357 | } | 416 | } | ||
| 417 | + | requireNoRemoteLockViolations(t, nil, cache) | |||
| 358 | 418 | ||||
| 359 | d.remoteCatalog.mu.Lock() | 419 | d.remoteCatalog.mu.Lock() | ||
| 360 | require.Equal(t, secondGeneration, d.remoteCatalog.refresh) | 420 | require.Equal(t, secondGeneration, d.remoteCatalog.refresh) | ||
| @@ -363,6 +423,46 @@ func TestRemoteRefreshStartsPerHostAndCancelsSupersededGeneration(t *testing.T) | |||||
| 363 | d.remoteCatalog.mu.Unlock() | 423 | d.remoteCatalog.mu.Unlock() | ||
| 364 | } | 424 | } | ||
| 365 | 425 | ||||
| 426 | + | func TestRemoteRefreshLockProbesSerialized(t *testing.T) { | |||
| 427 | + | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |||
| 428 | + | d, catalog, cache := newRemoteRefreshDaemon(t, hosts, time.Unix(125, 0)) | |||
| 429 | + | catalog.probes, cache.probes = true, true | |||
| 430 | + | ac := &attachedClient{} | |||
| 431 | + | ac.initOverlays() | |||
| 432 | + | model := picker.New(nil, picker.SelectionConfig{}) | |||
| 433 | + | ac.overlays.pickerMu.Lock() | |||
| 434 | + | ac.overlays.picker = model | |||
| 435 | + | ac.overlays.pickerGeneration++ | |||
| 436 | + | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: model} | |||
| 437 | + | ac.overlays.pickerMu.Unlock() | |||
| 438 | + | require.True(t, d.registerRemotePicker(instance)) | |||
| 439 | + | ||||
| 440 | + | d.startRemotePickerRefresh(instance) | |||
| 441 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |||
| 442 | + | request.result <- remoteRefreshResult{catalog: ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}} | |||
| 443 | + | receiveRemotePicker(t, cache.stores, "remote cache store") | |||
| 444 | + | requireNoRemoteLockViolations(t, catalog, cache) | |||
| 445 | + | } | |||
| 446 | + | ||||
| 447 | + | func TestRemoteRefreshWithoutCatalogClientDoesNotMarkHostsRefreshing(t *testing.T) { | |||
| 448 | + | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |||
| 449 | + | d := newRemotePickerDaemon(hosts) | |||
| 450 | + | ac := &attachedClient{} | |||
| 451 | + | ac.initOverlays() | |||
| 452 | + | model := picker.New(nil, picker.SelectionConfig{}) | |||
| 453 | + | ac.overlays.pickerMu.Lock() | |||
| 454 | + | ac.overlays.picker = model | |||
| 455 | + | ac.overlays.pickerGeneration++ | |||
| 456 | + | instance := remotePickerInstance{ac: ac, generation: ac.overlays.pickerGeneration, model: model} | |||
| 457 | + | ac.overlays.pickerMu.Unlock() | |||
| 458 | + | require.True(t, d.registerRemotePicker(instance)) | |||
| 459 | + | ||||
| 460 | + | require.NotZero(t, d.startRemotePickerRefresh(instance)) | |||
| 461 | + | d.remoteCatalog.mu.Lock() | |||
| 462 | + | require.NotEqual(t, remoteHostRefreshing, d.remoteCatalog.status["arch"]) | |||
| 463 | + | d.remoteCatalog.mu.Unlock() | |||
| 464 | + | } | |||
| 465 | + | ||||
| 366 | func TestRemoteRefreshResultPreservesFailuresAndEvictsSuccessfulOmissions(t *testing.T) { | 466 | func TestRemoteRefreshResultPreservesFailuresAndEvictsSuccessfulOmissions(t *testing.T) { | ||
| 367 | now := time.Unix(200, 0) | 467 | now := time.Unix(200, 0) | ||
| 368 | hosts := &remoteRefreshHostStore{hosts: []string{"arch", "mule"}} | 468 | hosts := &remoteRefreshHostStore{hosts: []string{"arch", "mule"}} | ||
| @@ -383,11 +483,12 @@ func TestRemoteRefreshResultPreservesFailuresAndEvictsSuccessfulOmissions(t *tes | |||||
| 383 | d.remoteCatalog.mu.Unlock() | 483 | d.remoteCatalog.mu.Unlock() | ||
| 384 | 484 | ||||
| 385 | require.True(t, d.applyRemoteRefreshResult(7, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}, nil)) | 485 | require.True(t, d.applyRemoteRefreshResult(7, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion}, nil)) | ||
| 386 | - | stored := <-cache.stores | 486 | + | stored := receiveRemotePicker(t, cache.stores, "remote cache store") |
| 387 | require.Equal(t, []ports.RemoteCatalogCacheEntry{ | 487 | require.Equal(t, []ports.RemoteCatalogCacheEntry{ | ||
| 388 | {Host: "arch", FetchedAt: now, Sessions: []ports.RemoteCatalogSession{}}, | 488 | {Host: "arch", FetchedAt: now, Sessions: []ports.RemoteCatalogSession{}}, | ||
| 389 | {Host: "mule", FetchedAt: time.Unix(20, 0), Sessions: []ports.RemoteCatalogSession{{Name: "stale", State: "running"}}}, | 489 | {Host: "mule", FetchedAt: time.Unix(20, 0), Sessions: []ports.RemoteCatalogSession{{Name: "stale", State: "running"}}}, | ||
| 390 | }, stored, "every successful write persists the newest full cache atomically") | 490 | }, stored, "every successful write persists the newest full cache atomically") | ||
| 491 | + | requireNoRemoteLockViolations(t, nil, cache) | |||
| 391 | d.remoteCatalog.mu.Lock() | 492 | d.remoteCatalog.mu.Lock() | ||
| 392 | require.Empty(t, d.remoteCatalog.cache["arch"].Sessions, "a successful omission authoritatively evicts old rows") | 493 | require.Empty(t, d.remoteCatalog.cache["arch"].Sessions, "a successful omission authoritatively evicts old rows") | ||
| 393 | require.Equal(t, remoteHostFresh, d.remoteCatalog.status["arch"]) | 494 | require.Equal(t, remoteHostFresh, d.remoteCatalog.status["arch"]) | ||
| @@ -407,7 +508,8 @@ func TestRemoteRefreshRejectsRemovedHostAndLateGeneration(t *testing.T) { | |||||
| 407 | require.False(t, d.applyRemoteRefreshResult(8, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion, Sessions: []ports.RemoteCatalogSession{{Name: "late", State: "running"}}}, nil)) | 508 | require.False(t, d.applyRemoteRefreshResult(8, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion, Sessions: []ports.RemoteCatalogSession{{Name: "late", State: "running"}}}, nil)) | ||
| 408 | hosts.set() | 509 | hosts.set() | ||
| 409 | require.True(t, d.applyRemoteRefreshResult(9, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion, Sessions: []ports.RemoteCatalogSession{{Name: "removed", State: "running"}}}, nil)) | 510 | require.True(t, d.applyRemoteRefreshResult(9, "arch", ports.RemoteCatalog{ProtocolVersion: ports.ProtocolVersion, Sessions: []ports.RemoteCatalogSession{{Name: "removed", State: "running"}}}, nil)) | ||
| 410 | - | require.Empty(t, <-cache.stores) | 511 | + | require.Empty(t, receiveRemotePicker(t, cache.stores, "remote cache store")) |
| 512 | + | requireNoRemoteLockViolations(t, nil, cache) | |||
| 411 | d.remoteCatalog.mu.Lock() | 513 | d.remoteCatalog.mu.Lock() | ||
| 412 | require.NotContains(t, d.remoteCatalog.cache, "arch") | 514 | require.NotContains(t, d.remoteCatalog.cache, "arch") | ||
| 413 | require.NotContains(t, d.remoteCatalog.status, "arch") | 515 | require.NotContains(t, d.remoteCatalog.status, "arch") | ||
| @@ -446,7 +548,7 @@ func addRemoteRefreshPickerOwner(t *testing.T, d *Daemon, id domain.SessionID) ( | |||||
| 446 | 548 | ||||
| 447 | func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | 549 | func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | ||
| 448 | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | 550 | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | ||
| 449 | - | d, _, cache := newRemoteRefreshDaemon(t, hosts, time.Unix(450, 0)) | 551 | + | d, catalog, cache := newRemoteRefreshDaemon(t, hosts, time.Unix(450, 0)) |
| 450 | firstSession, first, _ := addRemoteRefreshPickerOwner(t, d, "first") | 552 | firstSession, first, _ := addRemoteRefreshPickerOwner(t, d, "first") | ||
| 451 | _, second, _ := addRemoteRefreshPickerOwner(t, d, "second") | 553 | _, second, _ := addRemoteRefreshPickerOwner(t, d, "second") | ||
| 452 | before := make(map[*attachedClient]*picker.Model) | 554 | before := make(map[*attachedClient]*picker.Model) | ||
| @@ -455,11 +557,11 @@ func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |||||
| 455 | owner.overlays.pickerMu.Lock() | 557 | owner.overlays.pickerMu.Lock() | ||
| 456 | owner.overlays.picker = d.newPickerModel(firstSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{Session: "second", TabID: "second-tab"}) | 558 | owner.overlays.picker = d.newPickerModel(firstSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{Session: "second", TabID: "second-tab"}) | ||
| 457 | owner.overlays.pickerIntent = pickerNavigate | 559 | owner.overlays.pickerIntent = pickerNavigate | ||
| 560 | + | owner.overlays.pickerGeneration++ | |||
| 561 | + | instance := remotePickerInstance{ac: owner, generation: owner.overlays.pickerGeneration, model: owner.overlays.picker} | |||
| 458 | before[owner] = owner.overlays.picker | 562 | before[owner] = owner.overlays.picker | ||
| 459 | owner.overlays.pickerMu.Unlock() | 563 | owner.overlays.pickerMu.Unlock() | ||
| 460 | - | d.remoteCatalog.mu.Lock() | 564 | + | require.True(t, d.registerRemotePicker(instance)) |
| 461 | - | d.remoteCatalog.pickers[owner] = struct{}{} | |||
| 462 | - | d.remoteCatalog.mu.Unlock() | |||
| 463 | } | 565 | } | ||
| 464 | d.remoteCatalog.mu.Lock() | 566 | d.remoteCatalog.mu.Lock() | ||
| 465 | d.remoteCatalog.refresh = 4 | 567 | d.remoteCatalog.refresh = 4 | ||
| @@ -469,7 +571,8 @@ func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |||||
| 469 | ProtocolVersion: ports.ProtocolVersion, | 571 | ProtocolVersion: ports.ProtocolVersion, | ||
| 470 | Sessions: []ports.RemoteCatalogSession{{Name: "work", State: "running", Tabs: 2}}, | 572 | Sessions: []ports.RemoteCatalogSession{{Name: "work", State: "running", Tabs: 2}}, | ||
| 471 | }, nil)) | 573 | }, nil)) | ||
| 472 | - | <-cache.stores | 574 | + | receiveRemotePicker(t, cache.stores, "remote cache store") |
| 575 | + | requireNoRemoteLockViolations(t, catalog, cache) | |||
| 473 | 576 | ||||
| 474 | for _, owner := range []*attachedClient{first, second} { | 577 | for _, owner := range []*attachedClient{first, second} { | ||
| 475 | owner.overlays.pickerMu.Lock() | 578 | owner.overlays.pickerMu.Lock() | ||
| @@ -482,6 +585,165 @@ func TestRemoteRefreshUpdatesAllOpenPickersPreservingSelection(t *testing.T) { | |||||
| 482 | } | 585 | } | ||
| 483 | } | 586 | } | ||
| 484 | 587 | ||||
| 588 | + | func TestRemotePickerTeardownLifecycle(t *testing.T) { | |||
| 589 | + | for _, test := range []struct { | |||
| 590 | + | name string | |||
| 591 | + | resumeCapable bool | |||
| 592 | + | teardown func(*Daemon, *session, *attachedClient) | |||
| 593 | + | }{ | |||
| 594 | + | {name: "client gone parks resumable attachment", resumeCapable: true, teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.clientGone(sess, ac, ac.transport(), false) }}, | |||
| 595 | + | {name: "send error parks resumable attachment", resumeCapable: true, teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.detachOnSendError(sess, ac, ac.transport()) }}, | |||
| 596 | + | {name: "client gone removes non-resumable attachment", teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.clientGone(sess, ac, ac.transport(), false) }}, | |||
| 597 | + | {name: "send error removes non-resumable attachment", teardown: func(d *Daemon, sess *session, ac *attachedClient) { d.detachOnSendError(sess, ac, ac.transport()) }}, | |||
| 598 | + | } { | |||
| 599 | + | t.Run(test.name, func(t *testing.T) { | |||
| 600 | + | d := newRemotePickerDaemon(nil) | |||
| 601 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |||
| 602 | + | ac.resumeCapable = test.resumeCapable | |||
| 603 | + | model := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |||
| 604 | + | d.publishPicker(sess, ac, model, pickerNavigate, moveSourceLocator{}) | |||
| 605 | + | ac.overlays.pickerMu.Lock() | |||
| 606 | + | generation := ac.overlays.pickerGeneration | |||
| 607 | + | ac.overlays.pickerMu.Unlock() | |||
| 608 | + | ||||
| 609 | + | test.teardown(d, sess, ac) | |||
| 610 | + | ||||
| 611 | + | d.remoteCatalog.mu.Lock() | |||
| 612 | + | _, registered := d.remoteCatalog.pickers[ac] | |||
| 613 | + | d.remoteCatalog.mu.Unlock() | |||
| 614 | + | ac.overlays.pickerMu.Lock() | |||
| 615 | + | current, currentGeneration := ac.overlays.picker, ac.overlays.pickerGeneration | |||
| 616 | + | ac.overlays.pickerMu.Unlock() | |||
| 617 | + | if !test.resumeCapable { | |||
| 618 | + | require.False(t, registered) | |||
| 619 | + | require.Nil(t, current) | |||
| 620 | + | return | |||
| 621 | + | } | |||
| 622 | + | require.True(t, ac.parked) | |||
| 623 | + | require.True(t, registered) | |||
| 624 | + | require.Same(t, model, current) | |||
| 625 | + | require.Equal(t, generation, currentGeneration) | |||
| 626 | + | d.mu.Lock() | |||
| 627 | + | parked := d.parked[ac.resumeToken] | |||
| 628 | + | d.mu.Unlock() | |||
| 629 | + | require.NotNil(t, parked) | |||
| 630 | + | require.Same(t, ac, parked.ac) | |||
| 631 | + | }) | |||
| 632 | + | } | |||
| 633 | + | } | |||
| 634 | + | ||||
| 635 | + | func TestParkedPickerExpiryClosesCapturedGenerationAndCancelsRefresh(t *testing.T) { | |||
| 636 | + | clock := &signalClock{timers: make(chan *signalTimer, 1)} | |||
| 637 | + | hosts := &remoteRefreshHostStore{hosts: []string{"arch"}} | |||
| 638 | + | d, catalog, _ := newRemoteRefreshDaemon(t, hosts, time.Unix(475, 0)) | |||
| 639 | + | d.clock = clock | |||
| 640 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |||
| 641 | + | ac.resumeCapable = true | |||
| 642 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |||
| 643 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") | |||
| 644 | + | ||||
| 645 | + | require.True(t, d.parkAttachment(sess, ac)) | |||
| 646 | + | timer := receiveRemotePicker(t, clock.timers, "park expiry timer") | |||
| 647 | + | timer.ch <- clock.Now() | |||
| 648 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") | |||
| 649 | + | ||||
| 650 | + | ac.overlays.pickerMu.Lock() | |||
| 651 | + | require.Nil(t, ac.overlays.picker) | |||
| 652 | + | ac.overlays.pickerMu.Unlock() | |||
| 653 | + | d.remoteCatalog.mu.Lock() | |||
| 654 | + | require.NotContains(t, d.remoteCatalog.pickers, ac) | |||
| 655 | + | d.remoteCatalog.mu.Unlock() | |||
| 656 | + | } | |||
| 657 | + | ||||
| 658 | + | func TestParkedPickerRetirementClosesCapturedGenerationOnly(t *testing.T) { | |||
| 659 | + | d := newRemotePickerDaemon(nil) | |||
| 660 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |||
| 661 | + | ac.resumeCapable = true | |||
| 662 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |||
| 663 | + | d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1}) | |||
| 664 | + | ac.overlays.pickerMu.Lock() | |||
| 665 | + | refreshed := ac.overlays.picker | |||
| 666 | + | ac.overlays.pickerMu.Unlock() | |||
| 667 | + | require.NotNil(t, refreshed) | |||
| 668 | + | require.True(t, d.parkAttachment(sess, ac)) | |||
| 669 | + | token := ac.resumeToken | |||
| 670 | + | d.mu.Lock() | |||
| 671 | + | parked := d.parked[token] | |||
| 672 | + | d.mu.Unlock() | |||
| 673 | + | require.NotNil(t, parked) | |||
| 674 | + | ||||
| 675 | + | d.mu.Lock() | |||
| 676 | + | retirement := d.retireParkedAttachmentLocked(token, parked) | |||
| 677 | + | d.mu.Unlock() | |||
| 678 | + | d.finishParkedAttachmentRetirements([]parkedAttachmentRetirement{retirement}) | |||
| 679 | + | ac.overlays.pickerMu.Lock() | |||
| 680 | + | require.Nil(t, ac.overlays.picker, "terminal retirement closes a refreshed model in its captured generation") | |||
| 681 | + | ac.overlays.pickerMu.Unlock() | |||
| 682 | + | } | |||
| 683 | + | ||||
| 684 | + | func TestParkedPickerTokenReplacementRetiresPreviousGeneration(t *testing.T) { | |||
| 685 | + | d := newRemotePickerDaemon(nil) | |||
| 686 | + | firstSession, first, _ := addRemoteRefreshPickerOwner(t, d, "first") | |||
| 687 | + | secondSession, second, _ := addRemoteRefreshPickerOwner(t, d, "second") | |||
| 688 | + | first.resumeCapable, second.resumeCapable = true, true | |||
| 689 | + | d.publishPicker(firstSession, first, d.newPickerModel(firstSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |||
| 690 | + | require.True(t, d.parkAttachment(firstSession, first)) | |||
| 691 | + | token := first.resumeToken | |||
| 692 | + | d.publishPicker(secondSession, second, d.newPickerModel(secondSession, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |||
| 693 | + | second.resumeToken = token | |||
| 694 | + | require.True(t, d.parkAttachment(secondSession, second)) | |||
| 695 | + | ||||
| 696 | + | first.overlays.pickerMu.Lock() | |||
| 697 | + | require.Nil(t, first.overlays.picker) | |||
| 698 | + | first.overlays.pickerMu.Unlock() | |||
| 699 | + | second.overlays.pickerMu.Lock() | |||
| 700 | + | require.NotNil(t, second.overlays.picker) | |||
| 701 | + | second.overlays.pickerMu.Unlock() | |||
| 702 | + | } | |||
| 703 | + | ||||
| 704 | + | func TestParkedPickerResumePreservesGeneration(t *testing.T) { | |||
| 705 | + | d := newRemotePickerDaemon(nil) | |||
| 706 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |||
| 707 | + | ac.resumeCapable = true | |||
| 708 | + | ac.clientID = [16]byte{1, 2, 3, 4} | |||
| 709 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |||
| 710 | + | ac.overlays.pickerMu.Lock() | |||
| 711 | + | model, generation := ac.overlays.picker, ac.overlays.pickerGeneration | |||
| 712 | + | ac.overlays.pickerMu.Unlock() | |||
| 713 | + | d.clientGone(sess, ac, ac.transport(), false) | |||
| 714 | + | token := ac.resumeToken | |||
| 715 | + | tr := &closeTrackingTransport{} | |||
| 716 | + | resumedSess, resumedAC, ok, err := d.resumeParked(helloResumeCapable(ports.IntentResume, sess.name, token), tr, domain.Size{Cols: 80, Rows: 24}) | |||
| 717 | + | require.NoError(t, err) | |||
| 718 | + | require.True(t, ok) | |||
| 719 | + | require.Same(t, sess, resumedSess) | |||
| 720 | + | require.Same(t, ac, resumedAC) | |||
| 721 | + | ac.overlays.pickerMu.Lock() | |||
| 722 | + | require.Same(t, model, ac.overlays.picker) | |||
| 723 | + | require.Equal(t, generation, ac.overlays.pickerGeneration) | |||
| 724 | + | ac.overlays.pickerMu.Unlock() | |||
| 725 | + | } | |||
| 726 | + | ||||
| 727 | + | func TestStaleParkedPickerExpiryPreservesNewGeneration(t *testing.T) { | |||
| 728 | + | d := newRemotePickerDaemon(nil) | |||
| 729 | + | sess, ac, _ := addRemoteRefreshPickerOwner(t, d, "owner") | |||
| 730 | + | ac.resumeCapable = true | |||
| 731 | + | d.publishPicker(sess, ac, d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}), pickerNavigate, moveSourceLocator{}) | |||
| 732 | + | require.True(t, d.parkAttachment(sess, ac)) | |||
| 733 | + | token := ac.resumeToken | |||
| 734 | + | d.mu.Lock() | |||
| 735 | + | parked := d.parked[token] | |||
| 736 | + | d.mu.Unlock() | |||
| 737 | + | require.NotNil(t, parked) | |||
| 738 | + | ||||
| 739 | + | newer := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | |||
| 740 | + | d.publishPicker(sess, ac, newer, pickerNavigate, moveSourceLocator{}) | |||
| 741 | + | d.expireParked(token, parked) | |||
| 742 | + | ac.overlays.pickerMu.Lock() | |||
| 743 | + | require.Same(t, newer, ac.overlays.picker) | |||
| 744 | + | ac.overlays.pickerMu.Unlock() | |||
| 745 | + | } | |||
| 746 | + | ||||
| 485 | func TestRemoteRefreshPreservesRemoteCursorAcrossCacheToProxyUpgrade(t *testing.T) { | 747 | func TestRemoteRefreshPreservesRemoteCursorAcrossCacheToProxyUpgrade(t *testing.T) { | ||
| 486 | d := newRemotePickerDaemon(nil) | 748 | d := newRemotePickerDaemon(nil) | ||
| 487 | ownerSession, owner, _ := addRemoteRefreshPickerOwner(t, d, "local") | 749 | ownerSession, owner, _ := addRemoteRefreshPickerOwner(t, d, "local") | ||
| @@ -542,10 +804,10 @@ func TestRemoteRefreshCancellationWhenLastPickerCloses(t *testing.T) { | |||||
| 542 | } | 804 | } | ||
| 543 | first, second := newInstance(), newInstance() | 805 | first, second := newInstance(), newInstance() | ||
| 544 | d.remotePickerOpened(first) | 806 | d.remotePickerOpened(first) | ||
| 545 | - | request := <-catalog.requests | 807 | + | request := receiveRemotePicker(t, catalog.requests, "catalog request") |
| 546 | d.remotePickerOpened(second) | 808 | d.remotePickerOpened(second) | ||
| 547 | - | <-request.ctx.Done() | 809 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") |
| 548 | - | request = <-catalog.requests | 810 | + | request = receiveRemotePicker(t, catalog.requests, "catalog request") |
| 549 | 811 | ||||
| 550 | closeInstance(first) | 812 | closeInstance(first) | ||
| 551 | select { | 813 | select { | ||
| @@ -554,7 +816,7 @@ func TestRemoteRefreshCancellationWhenLastPickerCloses(t *testing.T) { | |||||
| 554 | default: | 816 | default: | ||
| 555 | } | 817 | } | ||
| 556 | closeInstance(second) | 818 | closeInstance(second) | ||
| 557 | - | <-request.ctx.Done() | 819 | + | receiveRemotePickerClose(t, request.ctx.Done(), "catalog request cancellation") |
| 558 | 820 | ||||
| 559 | d.remoteCatalog.mu.Lock() | 821 | d.remoteCatalog.mu.Lock() | ||
| 560 | require.Empty(t, d.remoteCatalog.pickers) | 822 | require.Empty(t, d.remoteCatalog.pickers) | ||
| @@ -606,16 +868,17 @@ func TestRemotePickerPublishCannotRegisterAfterSnatchClear(t *testing.T) { | |||||
| 606 | model := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | 868 | model := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | ||
| 607 | registrationReached := make(chan struct{}) | 869 | registrationReached := make(chan struct{}) | ||
| 608 | allowRegistration := make(chan struct{}) | 870 | allowRegistration := make(chan struct{}) | ||
| 871 | + | registrationWait := make(chan error, 1) | |||
| 609 | ac.overlays.beforeRemotePickerRegistration = func() { | 872 | ac.overlays.beforeRemotePickerRegistration = func() { | ||
| 610 | close(registrationReached) | 873 | close(registrationReached) | ||
| 611 | - | <-allowRegistration | 874 | + | registrationWait <- waitRemotePickerClose(allowRegistration, "picker registration release") |
| 612 | } | 875 | } | ||
| 613 | published := make(chan struct{}) | 876 | published := make(chan struct{}) | ||
| 614 | go func() { | 877 | go func() { | ||
| 615 | d.publishPicker(sess, ac, model, pickerNavigate, moveSourceLocator{}) | 878 | d.publishPicker(sess, ac, model, pickerNavigate, moveSourceLocator{}) | ||
| 616 | close(published) | 879 | close(published) | ||
| 617 | }() | 880 | }() | ||
| 618 | - | <-registrationReached | 881 | + | receiveRemotePickerClose(t, registrationReached, "picker registration") |
| 619 | 882 | ||||
| 620 | token := attachmentRoleToken{ | 883 | token := attachmentRoleToken{ | ||
| 621 | sess: sess, ac: ac, role: attachmentSnatched, | 884 | sess: sess, ac: ac, role: attachmentSnatched, | ||
| @@ -623,7 +886,8 @@ func TestRemotePickerPublishCannotRegisterAfterSnatchClear(t *testing.T) { | |||||
| 623 | } | 886 | } | ||
| 624 | require.True(t, d.clearForSnatch(token)) | 887 | require.True(t, d.clearForSnatch(token)) | ||
| 625 | close(allowRegistration) | 888 | close(allowRegistration) | ||
| 626 | - | <-published | 889 | + | require.NoError(t, receiveRemotePicker(t, registrationWait, "picker registration release")) |
| 890 | + | receiveRemotePickerClose(t, published, "picker publication") | |||
| 627 | 891 | ||||
| 628 | d.remoteCatalog.mu.Lock() | 892 | d.remoteCatalog.mu.Lock() | ||
| 629 | _, registered := d.remoteCatalog.pickers[ac] | 893 | _, registered := d.remoteCatalog.pickers[ac] | ||
| @@ -663,22 +927,24 @@ func TestRemotePickerStaleRefreshCannotOverwriteReopenedModel(t *testing.T) { | |||||
| 663 | 927 | ||||
| 664 | rebuildReached := make(chan struct{}) | 928 | rebuildReached := make(chan struct{}) | ||
| 665 | allowPublication := make(chan struct{}) | 929 | allowPublication := make(chan struct{}) | ||
| 930 | + | publicationWait := make(chan error, 1) | |||
| 666 | ac.overlays.afterPickerRefreshBuild = func(*picker.Model) { | 931 | ac.overlays.afterPickerRefreshBuild = func(*picker.Model) { | ||
| 667 | close(rebuildReached) | 932 | close(rebuildReached) | ||
| 668 | - | <-allowPublication | 933 | + | publicationWait <- waitRemotePickerClose(allowPublication, "picker publication release") |
| 669 | } | 934 | } | ||
| 670 | refreshed := make(chan struct{}) | 935 | refreshed := make(chan struct{}) | ||
| 671 | go func() { | 936 | go func() { | ||
| 672 | d.refreshRemoteOpenPickers() | 937 | d.refreshRemoteOpenPickers() | ||
| 673 | close(refreshed) | 938 | close(refreshed) | ||
| 674 | }() | 939 | }() | ||
| 675 | - | <-rebuildReached | 940 | + | receiveRemotePickerClose(t, rebuildReached, "picker rebuild") |
| 676 | 941 | ||||
| 677 | d.closePicker(ac) | 942 | d.closePicker(ac) | ||
| 678 | reopened := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | 943 | reopened := d.newPickerModel(sess, pickerNavigate, moveSourceLocator{}, picker.SourceFilter{}) | ||
| 679 | d.publishPicker(sess, ac, reopened, pickerNavigate, moveSourceLocator{}) | 944 | d.publishPicker(sess, ac, reopened, pickerNavigate, moveSourceLocator{}) | ||
| 680 | close(allowPublication) | 945 | close(allowPublication) | ||
| 681 | - | <-refreshed | 946 | + | require.NoError(t, receiveRemotePicker(t, publicationWait, "picker publication release")) |
| 947 | + | receiveRemotePickerClose(t, refreshed, "picker refresh") | |||
| 682 | 948 | ||||
| 683 | ac.overlays.pickerMu.Lock() | 949 | ac.overlays.pickerMu.Lock() | ||
| 684 | require.Same(t, reopened, ac.overlays.picker, "a rebuild for the old picker must not publish into its replacement") | 950 | require.Same(t, reopened, ac.overlays.picker, "a rebuild for the old picker must not publish into its replacement") | ||
M internal/usecase/daemon/resume.go +19 -9
| @@ -216,6 +216,12 @@ func (d *Daemon) parkAttachmentAs(sess *session, ac *attachedClient, role attach | |||
| 216 | 216 | // cannot leave closed panes retained through its capture cache. This must | |
| 217 | 217 | // remain outside d.mu: sendMu is ordered before the daemon lock. | |
| 218 | 218 | ac.clearCaptureFrames() | |
| 219 | + | var pickerGeneration uint64 | |
| 220 | + | if ac.overlays != nil { | |
| 221 | + | ac.overlays.pickerMu.Lock() | |
| 222 | + | pickerGeneration = ac.overlays.pickerGeneration | |
| 223 | + | ac.overlays.pickerMu.Unlock() | |
| 224 | + | } | |
| 219 | 225 | d.mu.Lock() | |
| 220 | 226 | if d.closing || d.sessions[sess.id] != sess { | |
| 221 | 227 | d.clearParkingInFlightLocked(ac.resumeToken, ac) | |
| @@ -244,13 +250,13 @@ func (d *Daemon) parkAttachmentAs(sess *session, ac *attachedClient, role attach | |||
| 244 | 250 | } | |
| 245 | 251 | grace := d.resumeParkGrace | |
| 246 | 252 | timer := d.clock.NewTimer(grace) | |
| 247 | - | parked := &parkedAttachment{sess: sess, ac: ac, role: role, timer: timer, done: make(chan struct{})} | |
| 253 | + | parked := &parkedAttachment{sess: sess, ac: ac, role: role, pickerGeneration: pickerGeneration, timer: timer, done: make(chan struct{})} | |
| 248 | 254 | ac.parked = true | |
| 249 | 255 | d.parked[token] = parked | |
| 250 | 256 | d.clearParkingInFlightLocked(token, ac) | |
| 251 | 257 | d.mu.Unlock() | |
| 252 | 258 | if oldRetirement != nil { | |
| 253 | - | finishParkedAttachmentRetirements([]parkedAttachmentRetirement{*oldRetirement}) | |
| 259 | + | d.finishParkedAttachmentRetirements([]parkedAttachmentRetirement{*oldRetirement}) | |
| 254 | 260 | } | |
| 255 | 261 | if oldSame != nil { | |
| 256 | 262 | if oldSame.timer != nil { | |
| @@ -275,6 +281,7 @@ func (d *Daemon) expireParked(token uint64, parked *parkedAttachment) { | |||
| 275 | 281 | if d.parked[token] == parked { | |
| 276 | 282 | d.removeParkedLocked(token, parked) | |
| 277 | 283 | d.mu.Unlock() | |
| 284 | + | d.closePickerIfCurrent(parked.ac, nil, parked.pickerGeneration) | |
| 278 | 285 | d.log.Warn("parked client expired", "session", parked.sess.name) | |
| 279 | 286 | return | |
| 280 | 287 | } | |
| @@ -312,8 +319,9 @@ func (d *Daemon) demoteParkedActiveForSessionLocked(sess *session) { | |||
| 312 | 319 | } | |
| 313 | 320 | ||
| 314 | 321 | type parkedAttachmentRetirement struct { | |
| 315 | - | parked *parkedAttachment | |
| 316 | - | transport transportSnapshot | |
| 322 | + | parked *parkedAttachment | |
| 323 | + | pickerGeneration uint64 | |
| 324 | + | transport transportSnapshot | |
| 317 | 325 | } | |
| 318 | 326 | ||
| 319 | 327 | func (d *Daemon) retireParkedAttachmentLocked(token uint64, parked *parkedAttachment) parkedAttachmentRetirement { | |
| @@ -324,8 +332,9 @@ func (d *Daemon) retireParkedAttachmentLocked(token uint64, parked *parkedAttach | |||
| 324 | 332 | parked.ac.roleGeneration.Add(1) | |
| 325 | 333 | parked.ac.setSession(nil) | |
| 326 | 334 | return parkedAttachmentRetirement{ | |
| 327 | - | parked: parked, | |
| 328 | - | transport: parked.ac.transportSnapshot(), | |
| 335 | + | parked: parked, | |
| 336 | + | pickerGeneration: parked.pickerGeneration, | |
| 337 | + | transport: parked.ac.transportSnapshot(), | |
| 329 | 338 | } | |
| 330 | 339 | } | |
| 331 | 340 | ||
| @@ -350,15 +359,16 @@ func (d *Daemon) purgeAllParkedLocked() []parkedAttachmentRetirement { | |||
| 350 | 359 | return retirements | |
| 351 | 360 | } | |
| 352 | 361 | ||
| 353 | - | // finishParkedAttachmentRetirements stops timers and closes transports without | |
| 354 | - | // holding daemon, session, coordinator, or pane locks. | |
| 355 | - | func finishParkedAttachmentRetirements(retirements []parkedAttachmentRetirement) { | |
| 362 | + | // finishParkedAttachmentRetirements stops timers and retires picker ownership | |
| 363 | + | // and transports without holding daemon, session, coordinator, or pane locks. | |
| 364 | + | func (d *Daemon) finishParkedAttachmentRetirements(retirements []parkedAttachmentRetirement) { | |
| 356 | 365 | for _, retirement := range retirements { | |
| 357 | 366 | if retirement.parked.timer != nil { | |
| 358 | 367 | retirement.parked.timer.Stop() | |
| 359 | 368 | } | |
| 360 | 369 | retirement.parked.closeDone() | |
| 361 | 370 | ac := retirement.parked.ac | |
| 371 | + | d.closePickerIfCurrent(ac, nil, retirement.pickerGeneration) | |
| 362 | 372 | _ = ac.closeCapturedTransport(ac.revokeTransport(retirement.transport.transport)) | |
| 363 | 373 | } | |
| 364 | 374 | } | |
| @@ -216,6 +216,12 @@ func (d *Daemon) parkAttachmentAs(sess *session, ac *attachedClient, role attach | |||||
| 216 | // cannot leave closed panes retained through its capture cache. This must | 216 | // cannot leave closed panes retained through its capture cache. This must | ||
| 217 | // remain outside d.mu: sendMu is ordered before the daemon lock. | 217 | // remain outside d.mu: sendMu is ordered before the daemon lock. | ||
| 218 | ac.clearCaptureFrames() | 218 | ac.clearCaptureFrames() | ||
| 219 | + | var pickerGeneration uint64 | |||
| 220 | + | if ac.overlays != nil { | |||
| 221 | + | ac.overlays.pickerMu.Lock() | |||
| 222 | + | pickerGeneration = ac.overlays.pickerGeneration | |||
| 223 | + | ac.overlays.pickerMu.Unlock() | |||
| 224 | + | } | |||
| 219 | d.mu.Lock() | 225 | d.mu.Lock() | ||
| 220 | if d.closing || d.sessions[sess.id] != sess { | 226 | if d.closing || d.sessions[sess.id] != sess { | ||
| 221 | d.clearParkingInFlightLocked(ac.resumeToken, ac) | 227 | d.clearParkingInFlightLocked(ac.resumeToken, ac) | ||
| @@ -244,13 +250,13 @@ func (d *Daemon) parkAttachmentAs(sess *session, ac *attachedClient, role attach | |||||
| 244 | } | 250 | } | ||
| 245 | grace := d.resumeParkGrace | 251 | grace := d.resumeParkGrace | ||
| 246 | timer := d.clock.NewTimer(grace) | 252 | timer := d.clock.NewTimer(grace) | ||
| 247 | - | parked := &parkedAttachment{sess: sess, ac: ac, role: role, timer: timer, done: make(chan struct{})} | 253 | + | parked := &parkedAttachment{sess: sess, ac: ac, role: role, pickerGeneration: pickerGeneration, timer: timer, done: make(chan struct{})} |
| 248 | ac.parked = true | 254 | ac.parked = true | ||
| 249 | d.parked[token] = parked | 255 | d.parked[token] = parked | ||
| 250 | d.clearParkingInFlightLocked(token, ac) | 256 | d.clearParkingInFlightLocked(token, ac) | ||
| 251 | d.mu.Unlock() | 257 | d.mu.Unlock() | ||
| 252 | if oldRetirement != nil { | 258 | if oldRetirement != nil { | ||
| 253 | - | finishParkedAttachmentRetirements([]parkedAttachmentRetirement{*oldRetirement}) | 259 | + | d.finishParkedAttachmentRetirements([]parkedAttachmentRetirement{*oldRetirement}) |
| 254 | } | 260 | } | ||
| 255 | if oldSame != nil { | 261 | if oldSame != nil { | ||
| 256 | if oldSame.timer != nil { | 262 | if oldSame.timer != nil { | ||
| @@ -275,6 +281,7 @@ func (d *Daemon) expireParked(token uint64, parked *parkedAttachment) { | |||||
| 275 | if d.parked[token] == parked { | 281 | if d.parked[token] == parked { | ||
| 276 | d.removeParkedLocked(token, parked) | 282 | d.removeParkedLocked(token, parked) | ||
| 277 | d.mu.Unlock() | 283 | d.mu.Unlock() | ||
| 284 | + | d.closePickerIfCurrent(parked.ac, nil, parked.pickerGeneration) | |||
| 278 | d.log.Warn("parked client expired", "session", parked.sess.name) | 285 | d.log.Warn("parked client expired", "session", parked.sess.name) | ||
| 279 | return | 286 | return | ||
| 280 | } | 287 | } | ||
| @@ -312,8 +319,9 @@ func (d *Daemon) demoteParkedActiveForSessionLocked(sess *session) { | |||||
| 312 | } | 319 | } | ||
| 313 | 320 | ||||
| 314 | type parkedAttachmentRetirement struct { | 321 | type parkedAttachmentRetirement struct { | ||
| 315 | - | parked *parkedAttachment | 322 | + | parked *parkedAttachment |
| 316 | - | transport transportSnapshot | 323 | + | pickerGeneration uint64 |
| 324 | + | transport transportSnapshot | |||
| 317 | } | 325 | } | ||
| 318 | 326 | ||||
| 319 | func (d *Daemon) retireParkedAttachmentLocked(token uint64, parked *parkedAttachment) parkedAttachmentRetirement { | 327 | func (d *Daemon) retireParkedAttachmentLocked(token uint64, parked *parkedAttachment) parkedAttachmentRetirement { | ||
| @@ -324,8 +332,9 @@ func (d *Daemon) retireParkedAttachmentLocked(token uint64, parked *parkedAttach | |||||
| 324 | parked.ac.roleGeneration.Add(1) | 332 | parked.ac.roleGeneration.Add(1) | ||
| 325 | parked.ac.setSession(nil) | 333 | parked.ac.setSession(nil) | ||
| 326 | return parkedAttachmentRetirement{ | 334 | return parkedAttachmentRetirement{ | ||
| 327 | - | parked: parked, | 335 | + | parked: parked, |
| 328 | - | transport: parked.ac.transportSnapshot(), | 336 | + | pickerGeneration: parked.pickerGeneration, |
| 337 | + | transport: parked.ac.transportSnapshot(), | |||
| 329 | } | 338 | } | ||
| 330 | } | 339 | } | ||
| 331 | 340 | ||||
| @@ -350,15 +359,16 @@ func (d *Daemon) purgeAllParkedLocked() []parkedAttachmentRetirement { | |||||
| 350 | return retirements | 359 | return retirements | ||
| 351 | } | 360 | } | ||
| 352 | 361 | ||||
| 353 | - | // finishParkedAttachmentRetirements stops timers and closes transports without | 362 | + | // finishParkedAttachmentRetirements stops timers and retires picker ownership |
| 354 | - | // holding daemon, session, coordinator, or pane locks. | 363 | + | // and transports without holding daemon, session, coordinator, or pane locks. |
| 355 | - | func finishParkedAttachmentRetirements(retirements []parkedAttachmentRetirement) { | 364 | + | func (d *Daemon) finishParkedAttachmentRetirements(retirements []parkedAttachmentRetirement) { |
| 356 | for _, retirement := range retirements { | 365 | for _, retirement := range retirements { | ||
| 357 | if retirement.parked.timer != nil { | 366 | if retirement.parked.timer != nil { | ||
| 358 | retirement.parked.timer.Stop() | 367 | retirement.parked.timer.Stop() | ||
| 359 | } | 368 | } | ||
| 360 | retirement.parked.closeDone() | 369 | retirement.parked.closeDone() | ||
| 361 | ac := retirement.parked.ac | 370 | ac := retirement.parked.ac | ||
| 371 | + | d.closePickerIfCurrent(ac, nil, retirement.pickerGeneration) | |||
| 362 | _ = ac.closeCapturedTransport(ac.revokeTransport(retirement.transport.transport)) | 372 | _ = ac.closeCapturedTransport(ac.revokeTransport(retirement.transport.transport)) | ||
| 363 | } | 373 | } | ||
| 364 | } | 374 | } | ||
M internal/usecase/daemon/resume_test.go +1 -1
| @@ -1702,7 +1702,7 @@ func TestDiscardingParkedAttachmentClearsPreviousSession(t *testing.T) { | |||
| 1702 | 1702 | require.NotNil(t, parked) | |
| 1703 | 1703 | retirements := tc.discard(d, ac.resumeToken, parked) | |
| 1704 | 1704 | d.mu.Unlock() | |
| 1705 | - | finishParkedAttachmentRetirements(retirements) | |
| 1705 | + | d.finishParkedAttachmentRetirements(retirements) | |
| 1706 | 1706 | ||
| 1707 | 1707 | require.Nil(t, ac.previousSession.Get()) | |
| 1708 | 1708 | }) | |
| @@ -1702,7 +1702,7 @@ func TestDiscardingParkedAttachmentClearsPreviousSession(t *testing.T) { | |||||
| 1702 | require.NotNil(t, parked) | 1702 | require.NotNil(t, parked) | ||
| 1703 | retirements := tc.discard(d, ac.resumeToken, parked) | 1703 | retirements := tc.discard(d, ac.resumeToken, parked) | ||
| 1704 | d.mu.Unlock() | 1704 | d.mu.Unlock() | ||
| 1705 | - | finishParkedAttachmentRetirements(retirements) | 1705 | + | d.finishParkedAttachmentRetirements(retirements) |
| 1706 | 1706 | ||||
| 1707 | require.Nil(t, ac.previousSession.Get()) | 1707 | require.Nil(t, ac.previousSession.Get()) | ||
| 1708 | }) | 1708 | }) | ||
M internal/usecase/daemon/session.go +1 -1
| @@ -1608,7 +1608,7 @@ func (d *Daemon) killSessionWithSnapshotDeadline(sess *session, reason uint8, pu | |||
| 1608 | 1608 | d.closing = true | |
| 1609 | 1609 | } | |
| 1610 | 1610 | d.mu.Unlock() | |
| 1611 | - | finishParkedAttachmentRetirements(parkedRetirements) | |
| 1611 | + | d.finishParkedAttachmentRetirements(parkedRetirements) | |
| 1612 | 1612 | frozen.unfreeze() | |
| 1613 | 1613 | frozen = frozenRoleEffectGates{} | |
| 1614 | 1614 | d.log.Info("session closed", "session", stoppedName, "id", sess.id, "ephemeral", ephemeral, "purge", purge, "reason", reason) | |
| @@ -1608,7 +1608,7 @@ func (d *Daemon) killSessionWithSnapshotDeadline(sess *session, reason uint8, pu | |||||
| 1608 | d.closing = true | 1608 | d.closing = true | ||
| 1609 | } | 1609 | } | ||
| 1610 | d.mu.Unlock() | 1610 | d.mu.Unlock() | ||
| 1611 | - | finishParkedAttachmentRetirements(parkedRetirements) | 1611 | + | d.finishParkedAttachmentRetirements(parkedRetirements) |
| 1612 | frozen.unfreeze() | 1612 | frozen.unfreeze() | ||
| 1613 | frozen = frozenRoleEffectGates{} | 1613 | frozen = frozenRoleEffectGates{} | ||
| 1614 | d.log.Info("session closed", "session", stoppedName, "id", sess.id, "ephemeral", ephemeral, "purge", purge, "reason", reason) | 1614 | d.log.Info("session closed", "session", stoppedName, "id", sess.id, "ephemeral", ephemeral, "purge", purge, "reason", reason) | ||
M internal/usecase/picker/model.go +6 -2
| @@ -254,7 +254,7 @@ func rowsForSession(session SessionView, config SelectionConfig) []row { | |||
| 254 | 254 | if common.remote { | |
| 255 | 255 | header.detail = remoteRowDetail(session) | |
| 256 | 256 | header.selectable = common.hasRemoteKey && config.Mode == SelectNavigationTab && session.ConnectOnly && remoteSelectable(session) | |
| 257 | - | header.dim = !session.RemoteAttachReady || session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch | |
| 257 | + | header.dim = remoteDim(session) | |
| 258 | 258 | } | |
| 259 | 259 | if config.Mode != SelectNavigationTab && session.CannotAcceptMoves { | |
| 260 | 260 | header.selectable, header.dim = false, true | |
| @@ -271,7 +271,7 @@ func rowsForSession(session SessionView, config SelectionConfig) []row { | |||
| 271 | 271 | tabRow.selectable = tabRow.kind.selectable(config.Mode) | |
| 272 | 272 | if common.remote { | |
| 273 | 273 | tabRow.selectable = false | |
| 274 | - | tabRow.dim = !session.RemoteAttachReady || session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch | |
| 274 | + | tabRow.dim = remoteDim(session) | |
| 275 | 275 | } | |
| 276 | 276 | if config.Mode != SelectNavigationTab && session.CannotAcceptMoves { | |
| 277 | 277 | tabRow.selectable, tabRow.dim = false, true | |
| @@ -289,6 +289,10 @@ func remoteSelectable(session SessionView) bool { | |||
| 289 | 289 | return session.RemoteAttachReady && session.RemoteAvailability != RemoteVersionMismatch | |
| 290 | 290 | } | |
| 291 | 291 | ||
| 292 | + | func remoteDim(session SessionView) bool { | |
| 293 | + | return !session.RemoteAttachReady || session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch | |
| 294 | + | } | |
| 295 | + | ||
| 292 | 296 | func remoteRowDetail(session SessionView) string { | |
| 293 | 297 | if session.RemoteDetail != "" && (session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch) { | |
| 294 | 298 | return session.RemoteDetail | |
| @@ -254,7 +254,7 @@ func rowsForSession(session SessionView, config SelectionConfig) []row { | |||||
| 254 | if common.remote { | 254 | if common.remote { | ||
| 255 | header.detail = remoteRowDetail(session) | 255 | header.detail = remoteRowDetail(session) | ||
| 256 | header.selectable = common.hasRemoteKey && config.Mode == SelectNavigationTab && session.ConnectOnly && remoteSelectable(session) | 256 | header.selectable = common.hasRemoteKey && config.Mode == SelectNavigationTab && session.ConnectOnly && remoteSelectable(session) | ||
| 257 | - | header.dim = !session.RemoteAttachReady || session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch | 257 | + | header.dim = remoteDim(session) |
| 258 | } | 258 | } | ||
| 259 | if config.Mode != SelectNavigationTab && session.CannotAcceptMoves { | 259 | if config.Mode != SelectNavigationTab && session.CannotAcceptMoves { | ||
| 260 | header.selectable, header.dim = false, true | 260 | header.selectable, header.dim = false, true | ||
| @@ -271,7 +271,7 @@ func rowsForSession(session SessionView, config SelectionConfig) []row { | |||||
| 271 | tabRow.selectable = tabRow.kind.selectable(config.Mode) | 271 | tabRow.selectable = tabRow.kind.selectable(config.Mode) | ||
| 272 | if common.remote { | 272 | if common.remote { | ||
| 273 | tabRow.selectable = false | 273 | tabRow.selectable = false | ||
| 274 | - | tabRow.dim = !session.RemoteAttachReady || session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch | 274 | + | tabRow.dim = remoteDim(session) |
| 275 | } | 275 | } | ||
| 276 | if config.Mode != SelectNavigationTab && session.CannotAcceptMoves { | 276 | if config.Mode != SelectNavigationTab && session.CannotAcceptMoves { | ||
| 277 | tabRow.selectable, tabRow.dim = false, true | 277 | tabRow.selectable, tabRow.dim = false, true | ||
| @@ -289,6 +289,10 @@ func remoteSelectable(session SessionView) bool { | |||||
| 289 | return session.RemoteAttachReady && session.RemoteAvailability != RemoteVersionMismatch | 289 | return session.RemoteAttachReady && session.RemoteAvailability != RemoteVersionMismatch | ||
| 290 | } | 290 | } | ||
| 291 | 291 | ||||
| 292 | + | func remoteDim(session SessionView) bool { | |||
| 293 | + | return !session.RemoteAttachReady || session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch | |||
| 294 | + | } | |||
| 295 | + | ||||
| 292 | func remoteRowDetail(session SessionView) string { | 296 | func remoteRowDetail(session SessionView) string { | ||
| 293 | if session.RemoteDetail != "" && (session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch) { | 297 | if session.RemoteDetail != "" && (session.RemoteAvailability == RemoteStale || session.RemoteAvailability == RemoteVersionMismatch) { | ||
| 294 | return session.RemoteDetail | 298 | return session.RemoteDetail | ||
M internal/usecase/picker/model_test.go +3 -3
| @@ -848,14 +848,14 @@ func TestRemoteRowsCannotAcceptMovesInEitherMoveMode(t *testing.T) { | |||
| 848 | 848 | {name: "move tab", mode: SelectMoveTabSession}, | |
| 849 | 849 | } | |
| 850 | 850 | ||
| 851 | - | for _, availability := range availability { | |
| 852 | - | t.Run(availability.name, func(t *testing.T) { | |
| 851 | + | for _, availabilityCase := range availability { | |
| 852 | + | t.Run(availabilityCase.name, func(t *testing.T) { | |
| 853 | 853 | remote := SessionView{ | |
| 854 | 854 | ID: key.ID(), | |
| 855 | 855 | Name: key.Display(), | |
| 856 | 856 | Tabs: []TabEntry{{TabID: "remote-tab", Name: "metadata"}}, | |
| 857 | 857 | RemoteKey: &key, | |
| 858 | - | RemoteAvailability: availability.state, | |
| 858 | + | RemoteAvailability: availabilityCase.state, | |
| 859 | 859 | ConnectOnly: true, | |
| 860 | 860 | RemoteAttachReady: true, | |
| 861 | 861 | CannotAcceptMoves: true, | |
| @@ -848,14 +848,14 @@ func TestRemoteRowsCannotAcceptMovesInEitherMoveMode(t *testing.T) { | |||||
| 848 | {name: "move tab", mode: SelectMoveTabSession}, | 848 | {name: "move tab", mode: SelectMoveTabSession}, | ||
| 849 | } | 849 | } | ||
| 850 | 850 | ||||
| 851 | - | for _, availability := range availability { | 851 | + | for _, availabilityCase := range availability { |
| 852 | - | t.Run(availability.name, func(t *testing.T) { | 852 | + | t.Run(availabilityCase.name, func(t *testing.T) { |
| 853 | remote := SessionView{ | 853 | remote := SessionView{ | ||
| 854 | ID: key.ID(), | 854 | ID: key.ID(), | ||
| 855 | Name: key.Display(), | 855 | Name: key.Display(), | ||
| 856 | Tabs: []TabEntry{{TabID: "remote-tab", Name: "metadata"}}, | 856 | Tabs: []TabEntry{{TabID: "remote-tab", Name: "metadata"}}, | ||
| 857 | RemoteKey: &key, | 857 | RemoteKey: &key, | ||
| 858 | - | RemoteAvailability: availability.state, | 858 | + | RemoteAvailability: availabilityCase.state, |
| 859 | ConnectOnly: true, | 859 | ConnectOnly: true, | ||
| 860 | RemoteAttachReady: true, | 860 | RemoteAttachReady: true, | ||
| 861 | CannotAcceptMoves: true, | 861 | CannotAcceptMoves: true, | ||