diff --git a/docs/changes/unreleased/1836-held-send-race-fakes.md b/docs/changes/unreleased/1836-held-send-race-fakes.md new file mode 100644 index 0000000000..3f6d5cd590 --- /dev/null +++ b/docs/changes/unreleased/1836-held-send-race-fakes.md @@ -0,0 +1,16 @@ +--- +kind: internal +title: the held-send and plan tests pass under -race; their fakes now lock what off-loop readers touch +pr: 1836 +invalidates: + - >- + The tui3 held-send and run-stop-card tests were believed to fail under -race + because the keeper goroutine or the room touched state the update loop owns; + the races were in the switchAgent and planFake test fakes, which now lock + what the off-loop readers touch, and production code is unchanged. +--- + +The real agents already lock Attach, and the real plan reads build fresh values +on every call. Three more tests with the same fake races, also red under -race +on dev, are fixed by the same change: TestAProgramRoomReadsTheLandingTheStoreEndedAhead, +TestTheCancelKeyOnAPlanRowEndsItThroughTheStore and TestPOnAPlanRowPausesThenResumes. diff --git a/internal/tui3/c295_rail_task_page_test.go b/internal/tui3/c295_rail_task_page_test.go index df79637015..548f9a897e 100644 --- a/internal/tui3/c295_rail_task_page_test.go +++ b/internal/tui3/c295_rail_task_page_test.go @@ -3,6 +3,7 @@ package tui3 import ( "strings" "sync" + "sync/atomic" "testing" tea "charm.land/bubbletea/v2" @@ -11,19 +12,20 @@ import ( ) // railPlanCounter proves that opening the store is a gesture, never paint. +// The counts are bumped by reads the surface makes off the loop. type railPlanCounter struct { *planFake - rows int - pages int + rows atomic.Int64 + pages atomic.Int64 } func (f *railPlanCounter) PlanTasks() []session.PlanTaskRow { - f.rows++ + f.rows.Add(1) return f.planFake.PlanTasks() } func (f *railPlanCounter) PlanTaskPage(id string) (session.PlanTaskPage, bool) { - f.pages++ + f.pages.Add(1) return f.planFake.PlanTaskPage(id) } @@ -55,12 +57,12 @@ func railTaskPageApp(t *testing.T, held bool) (*app, *railPlanCounter) { func TestRailClickReadsTheStoreAtTheGestureAndOpensTheTaskPage(t *testing.T) { a, fake := railTaskPageApp(t, true) - if fake.pages != 0 { - t.Fatalf("building and drawing the conversation read %d task pages", fake.pages) + if fake.pages.Load() != 0 { + t.Fatalf("building and drawing the conversation read %d task pages", fake.pages.Load()) } clickRail(t, a, 0) - if fake.pages != 1 { - t.Fatalf("rail click read task pages %d times, want once at the gesture", fake.pages) + if fake.pages.Load() != 1 { + t.Fatalf("rail click read task pages %d times, want once at the gesture", fake.pages.Load()) } if plan := a.roomPlan(); plan == nil || plan.id != "2" { t.Fatalf("rail click did not open the task room over store task 2: room=%v", a.roomOpen()) @@ -74,16 +76,16 @@ func TestRailEnterReadsTheStoreAtTheGestureAndOpensTheTaskPage(t *testing.T) { a, fake := railTaskPageApp(t, true) a.railWhere, a.railHold = railSpot{id: 2}, true drive(t, a, tea.KeyPressMsg{Code: tea.KeyEnter}) - if plan := a.roomPlan(); fake.pages != 1 || plan == nil || plan.id != "2" { - t.Fatalf("rail enter reads=%d room=%v", fake.pages, a.roomOpen()) + if plan := a.roomPlan(); fake.pages.Load() != 1 || plan == nil || plan.id != "2" { + t.Fatalf("rail enter reads=%d room=%v", fake.pages.Load(), a.roomOpen()) } } func TestRailRowWithoutAStoredPageStillOpensItsRoom(t *testing.T) { a, fake := railTaskPageApp(t, false) clickRail(t, a, 0) - if fake.pages != 1 { - t.Fatalf("room fallback did %d page reads, want one gesture read", fake.pages) + if fake.pages.Load() != 1 { + t.Fatalf("room fallback did %d page reads, want one gesture read", fake.pages.Load()) } if a.roomPlan() != nil || !a.roomOpen() || a.room.id != 2 { t.Fatalf("absent page opened room=%v id=%d", a.roomOpen(), roomID(a)) @@ -93,16 +95,18 @@ func TestRailRowWithoutAStoredPageStillOpensItsRoom(t *testing.T) { func TestRailTaskPageSurvivesSettledFramesAndFramesDoNotReadTheAgent(t *testing.T) { a, fake := railTaskPageApp(t, true) clickRail(t, a, 0) - for i := range fake.plan { - fake.plan[i].Status = "done" - } + fake.editRows(func(rows []session.PlanTaskRow) { + for i := range rows { + rows[i].Status = "done" + } + }) for i := range a.taskSheet.mine.plan { a.taskSheet.mine.plan[i].Status = "done" } settled := fake.planFake.pages["2"] settled.Row.Status = "done" - fake.planFake.pages["2"] = settled - reads, pages := fake.rows, fake.pages + fake.planFake.setPage("2", settled) + reads, pages := fake.rows.Load(), fake.pages.Load() for range 10 { drive(t, a, frameMsg{}) frame, _, _ := a.frame() @@ -112,8 +116,8 @@ func TestRailTaskPageSurvivesSettledFramesAndFramesDoNotReadTheAgent(t *testing. } // A ROOM ON A RUNNING TASK FOLLOWS IT ON ITS OWN BEAT, never on the paint // clock: ten frames cost no read of the page and no read of the rows. - if fake.rows != reads || fake.pages != pages { - t.Fatalf("frames called the agent: row reads %d→%d, page reads %d→%d", reads, fake.rows, pages, fake.pages) + if fake.rows.Load() != reads || fake.pages.Load() != pages { + t.Fatalf("frames called the agent: row reads %d→%d, page reads %d→%d", reads, fake.rows.Load(), pages, fake.pages.Load()) } if a.roomPlan() == nil { t.Fatal("ten settled frames closed the task room") @@ -140,8 +144,8 @@ func TestAChildRemainsOpenableFromARailTaskRoom(t *testing.T) { t.Fatalf("the run's room does not draw its part:\n%s", got) } openPlanRoomNow(t, a, "3") - if plan := a.roomPlan(); fake.pages != 2 || plan == nil || plan.id != "3" { - t.Fatalf("child open reads=%d room=%v", fake.pages, a.roomOpen()) + if plan := a.roomPlan(); fake.pages.Load() != 2 || plan == nil || plan.id != "3" { + t.Fatalf("child open reads=%d room=%v", fake.pages.Load(), a.roomOpen()) } if got := planRoomText(t, a); !strings.Contains(got, "the child page") { t.Fatalf("the child's room was not drawn:\n%s", got) diff --git a/internal/tui3/parkkeep_test.go b/internal/tui3/parkkeep_test.go index 3a4b1b45ee..837a820449 100644 --- a/internal/tui3/parkkeep_test.go +++ b/internal/tui3/parkkeep_test.go @@ -58,7 +58,7 @@ func parkedKeeperLab(t *testing.T, parks []parked) (*app, *switchAgent, *fakeAge // Attach is the authority behind follow-up priority. func landHeld(t *testing.T, a *app, agent *switchAgent, key string, held *kept, running bool) { t.Helper() - agent.running = running + agent.setRunning(running) held.watch.landed.Store(true) held.watch.armed.Store(true) drive(t, a, runCmd(a.behindStir(behindStirMsg{key: key}))...) @@ -185,7 +185,7 @@ func TestHeldWaitingMessagesGoOnePerLandingAfterSessionFollowUpsAndNeverTwice(t func TestComingBackDuringAHeldSendNeverSendsItTwice(t *testing.T) { a, agent, _, key, held := parkedKeeperLab(t, []parked{{text: "only once"}}) - agent.running = false + agent.setRunning(false) cmd := a.sendBehindParked(key, held) if cmd == nil || !held.side.parkSending || !held.side.parks[0].sending { t.Fatal("the held send did not mark its queue head before crossing") diff --git a/internal/tui3/planroom_test.go b/internal/tui3/planroom_test.go index 6d60b674fe..4512a10029 100644 --- a/internal/tui3/planroom_test.go +++ b/internal/tui3/planroom_test.go @@ -351,7 +351,7 @@ func TestTheRunsTaskRoomFollowsAStepTheStoreRecordsLater(t *testing.T) { t.Fatalf("the room opened with calls %q", got) } page.Steps = append(page.Steps, session.PlanStep{Kind: "step", Step: 2, Command: "go test ./..."}) - fake.pages[row.ID] = page + fake.setPage(row.ID, page) drive(t, a, planRoomTickMsg{gen: a.room.gen}) if got := roomCommands(a); len(got) != 2 || got[1] != "go test ./..." { t.Fatalf("the beat did not bring the new step in once: %q", got) @@ -372,7 +372,7 @@ func TestTheLiveStepLeavesTheRoomWhenTheTaskEnds(t *testing.T) { row.Status, row.Live = "done", plandb.LiveStep{} page.Row, page.Live = row, row.Live page.Steps = []session.PlanStep{{Kind: "step", Step: 2, Command: "go test ./...", Observation: "ok"}} - fake.pages[row.ID] = page + fake.setPage(row.ID, page) drive(t, a, planRoomTickMsg{gen: a.room.gen}) if !a.room.done { t.Fatal("the room of a task that ended is not done") diff --git a/internal/tui3/planrows_offframe_test.go b/internal/tui3/planrows_offframe_test.go index ffaeb30184..1014cff940 100644 --- a/internal/tui3/planrows_offframe_test.go +++ b/internal/tui3/planrows_offframe_test.go @@ -96,7 +96,7 @@ func TestWhileTheRowsAreOnTheirWayTheFrameDrawsWhatItHeld(t *testing.T) { a.agent = slow a.taskSheet.regroup(a) - fake.plan = append(fake.plan, session.PlanTaskRow{ID: "p1", Parent: "1", Title: "a part the run added", Status: "running"}) + fake.setPlan(append(fake.plan, session.PlanTaskRow{ID: "p1", Parent: "1", Title: "a part the run added", Status: "running"})) a.railStamp++ cmd := a.refreshPlanRows() if cmd == nil { diff --git a/internal/tui3/programbadge_test.go b/internal/tui3/programbadge_test.go index 1a00b7a217..b6ba26365f 100644 --- a/internal/tui3/programbadge_test.go +++ b/internal/tui3/programbadge_test.go @@ -188,14 +188,14 @@ func TestDrawingAProgramsBadgeReadsNothingFromTheAgent(t *testing.T) { // AN OLDER ENGINE'S ROW, naming no program: the badge is read off the held // plan row. drive(t, a, streamEventMsg{gen: a.gen, ev: update(7, row.Title, session.TaskRunning, session.TaskNotice{StartedAt: programRunBegan})}) - rows, pages := counted.rows, counted.pages + rows, pages := counted.rows.Load(), counted.pages.Load() if drawn := plain(strings.Join(a.railRows(a.viewHeight()), "\n")); !strings.Contains(drawn, "[sd]") { t.Fatalf("a node named only by its held plan row wears no badge:\n%s", drawn) } a.stripRow(90) a.taskSheetOwnRows() - if counted.rows != rows || counted.pages != pages { - t.Fatalf("drawing the badge read the agent: rows %d→%d, pages %d→%d", rows, counted.rows, pages, counted.pages) + if counted.rows.Load() != rows || counted.pages.Load() != pages { + t.Fatalf("drawing the badge read the agent: rows %d→%d, pages %d→%d", rows, counted.rows.Load(), pages, counted.pages.Load()) } } diff --git a/internal/tui3/programroom_test.go b/internal/tui3/programroom_test.go index 6eb6939193..d43add6eac 100644 --- a/internal/tui3/programroom_test.go +++ b/internal/tui3/programroom_test.go @@ -151,7 +151,7 @@ func TestAProgramRoomNamesItsTaskOnceAndHidesItsBriefBehindADropdown(t *testing. a, agent := programRoomApp(t, 120, 40) page := agent.planFake.pages["7"] page.Description = strings.Repeat("the store interface changes and every caller moves with it. ", 12) + "THE LAST WORDS" - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) openProgramRoomNow(t, a) width, _ := a.size() head := func() []string { @@ -198,7 +198,7 @@ func TestAProgramsOpenBriefIsAWholeDocumentTheRoomScrolls(t *testing.T) { items = append(items, "- item "+strconv.Itoa(i)+" keeps __init__.py as it is") } page.Description = "THE WORK\n\nFIRST ACTION: read the spec.\n" + strings.Join(items, "\n") + "\n\nWHAT TO PRODUCE\n\nTHE LAST WORDS" - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) openProgramRoomNow(t, a) a.roomScroll(-3) before := a.room.offset @@ -266,7 +266,7 @@ func TestAProgramsStepOpensToItsWholeStepAndShutsAgain(t *testing.T) { program.Actions = append(program.Actions, delegate.Shown{At: at, Step: "explore", Text: "ran go test ./...", Outcome: "fails · exit 1", Detail: "bash: go test ./...\n\n--- FAIL: TestTheWholeOutput"}) page.Program = &program - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) openProgramRoomNow(t, a) var target row for _, r := range a.roomRows(a.bodyWidth()) { @@ -300,16 +300,16 @@ func TestAProgramRoomFollowsWhileRunningAndStopsAfterItSettles(t *testing.T) { if !a.programRoomFollows() { t.Fatal("a room on a running program is not on the paint clock") } - reads := agent.railPlanCounter.pages + reads := agent.railPlanCounter.pages.Load() for range 5 { drive(t, a, frameMsg{}) } - if agent.railPlanCounter.pages != reads { - t.Fatalf("frames inside one beat read the page %d times", agent.railPlanCounter.pages-reads) + if agent.railPlanCounter.pages.Load() != reads { + t.Fatalf("frames inside one beat read the page %d times", agent.railPlanCounter.pages.Load()-reads) } for beat := 1; beat <= 3; beat++ { planBeat(t, a) - if got := agent.railPlanCounter.pages - reads; got != beat { + if got := agent.railPlanCounter.pages.Load() - reads; got != int64(beat) { t.Fatalf("after %d beats the page was read %d times, want once a beat", beat, got) } } @@ -317,13 +317,13 @@ func TestAProgramRoomFollowsWhileRunningAndStopsAfterItSettles(t *testing.T) { page := agent.planFake.pages["7"] page.Row.Status, page.Row.Ended = "done", a.now() page.Notes = []session.PlanTaskNote{{Body: "the work landed on branch senior-dev/auth"}} - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) drive(t, a, streamEventMsg{gen: a.gen, ev: update(7, page.Row.Title, session.TaskDone, session.TaskNotice{StartedAt: programRunBegan, EndedAt: a.now()})}) - before := agent.railPlanCounter.pages + before := agent.railPlanCounter.pages.Load() drive(t, a, frameMsg{}) - if agent.railPlanCounter.pages != before+1 { - t.Fatalf("the landing was read %d times, want once", agent.railPlanCounter.pages-before) + if agent.railPlanCounter.pages.Load() != before+1 { + t.Fatalf("the landing was read %d times, want once", agent.railPlanCounter.pages.Load()-before) } if !strings.Contains(roomText(a), "the work landed on branch senior-dev/auth") { t.Fatalf("the room did not end on the page the store ended on:\n%s", roomText(a)) @@ -334,12 +334,12 @@ func TestAProgramRoomFollowsWhileRunningAndStopsAfterItSettles(t *testing.T) { if a.programRoomFollows() { t.Fatal("a settled program's room keeps the paint clock turning") } - settled := agent.railPlanCounter.pages + settled := agent.railPlanCounter.pages.Load() for range 3 { planBeat(t, a) } - if agent.railPlanCounter.pages != settled { - t.Fatalf("a settled program's room was read %d more times", agent.railPlanCounter.pages-settled) + if agent.railPlanCounter.pages.Load() != settled { + t.Fatalf("a settled program's room was read %d more times", agent.railPlanCounter.pages.Load()-settled) } } @@ -363,7 +363,7 @@ func TestAProgramRoomReadsTheLandingTheStoreEndedAhead(t *testing.T) { openProgramRoomNow(t, a) page := agent.planFake.pages["7"] page.Row.Status, page.Row.Ended = "done", a.now() - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) planBeat(t, a) if a.room.done || !a.programRoomFollows() { t.Fatalf("a store that ended ahead of the row took the room off the clock: done=%v", a.room.done) @@ -385,10 +385,10 @@ func TestAProgramRoomReadsTheLandingTheStoreEndedAhead(t *testing.T) { // The read that was out answers with the page before the landing. stale := page stale.Notes = nil - agent.planFake.pages["7"] = stale + agent.planFake.setPage("7", stale) drain(t, a, out) } - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) for range 2 { drive(t, a, frameMsg{}) } @@ -485,7 +485,7 @@ func TestAProgramRunReadsOneFigureOnTheRoomTheRailAndTheCard(t *testing.T) { // The store was seeded sixteen seconds before the run's hand-off. page := agent.planFake.pages["7"] page.Row.Started = programRunBegan.Add(-16 * time.Second) - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) rail := strings.Split(plain(a.railTelemetry(a.tasks[7], 40)), railSep)[0] openProgramRoomNow(t, a) facts, _ := a.programFactsWord(120) @@ -498,7 +498,7 @@ func TestAProgramRunReadsOneFigureOnTheRoomTheRailAndTheCard(t *testing.T) { now := ended.Add(3 * time.Second) a.clock = func() time.Time { return now } page.Row.Status, page.Row.Ended = "done", ended.Add(2*time.Second) - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) drive(t, a, streamEventMsg{gen: a.gen, ev: update(7, page.Row.Title, session.TaskDone, session.TaskNotice{StartedAt: programRunBegan, EndedAt: ended, Elapsed: ended.Sub(programRunBegan)})}) drive(t, a, frameMsg{}) @@ -530,7 +530,7 @@ func TestAProgramRoomsClockStopsAtTheProgramsExit(t *testing.T) { exit := programRunBegan.Add(20 * time.Minute) page := agent.planFake.pages["7"] page.Row.Ended = exit - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) now := exit.Add(65 * time.Second) a.clock = func() time.Time { return now } drain(t, a, a.programRoomRead()) @@ -590,7 +590,7 @@ func TestAStepThatChangedAFileWearsItsLinesInTheDiffsColours(t *testing.T) { delegate.Shown{At: at, Step: "implement", Text: "edited internal/auth/middleware.go", Lines: true, Added: 123, Removed: 21}, delegate.Shown{At: at.Add(time.Second), Step: "implement", Text: "read internal/auth/store.go"}) page.Program = &program - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) openProgramRoomNow(t, a) var edited, read string for _, r := range a.roomRows(a.bodyWidth()) { @@ -640,7 +640,7 @@ func TestAProgramRoomsSeamNamesTheModelsItsRunWasLaunchedOn(t *testing.T) { program.Models = []string{"deepseek/deepseek-v4-pro", "moonshotai/kimi-k2.6", "z-ai/glm-5.1"} program.Effort = "high" page.Program = &program - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) a.closeRoom() openProgramRoomNow(t, a) frame, _, _ := a.frame() @@ -718,7 +718,7 @@ func TestProgramRoomStartingRefusalKeepsThePersonsWords(t *testing.T) { a, agent := programRoomApp(t, 120, 28) page := agent.planFake.pages["7"] page.Program.Listens = true - agent.planFake.pages["7"] = page + agent.planFake.setPage("7", page) openProgramRoomNow(t, a) // The held node is what owns the room's ending, so queued remains live. a.programOf().page.Row.Status = state diff --git a/internal/tui3/stoprun_page_test.go b/internal/tui3/stoprun_page_test.go index 6c8581d99e..8e52ca5b07 100644 --- a/internal/tui3/stoprun_page_test.go +++ b/internal/tui3/stoprun_page_test.go @@ -68,7 +68,7 @@ func TestStopOnAPartsRoomIsTheCardAndThenTheStoresOwnCancel(t *testing.T) { counted.setStatus("3", "running") part := counted.planFake.pages["3"] part.Row.Status = "running" - counted.planFake.pages["3"] = part + counted.planFake.setPage("3", part) openPlanRoomNow(t, a, "3") if plan := a.roomPlan(); plan == nil || plan.id != "3" { t.Fatal("the part's room did not open") diff --git a/internal/tui3/switch_test.go b/internal/tui3/switch_test.go index 6be67891ec..03f7ab34b0 100644 --- a/internal/tui3/switch_test.go +++ b/internal/tui3/switch_test.go @@ -20,6 +20,12 @@ import ( // no other test on this surface needs. type switchAgent struct { *fakeAgent + // mu guards running, backlog and stops against Attach and its stop, which a + // kept conversation's watcher calls from its own goroutine (keeper.go), the + // way the real agents take a.mu. A test that changes running once a watcher + // may be live does it through setRunning. Reads and writes made from the + // test goroutine before any watcher starts, or after it stops, need no lock. + mu sync.Mutex // backlog is what a turn already in flight has said. Attach hands it over in // order and then stays open, which is what the real hub does under one hold // of its lock. @@ -36,6 +42,8 @@ type switchAgent struct { } func (s *switchAgent) Attach() (<-chan session.Event, bool, func()) { + s.mu.Lock() + defer s.mu.Unlock() if !s.running { return nil, false, func() {} } @@ -43,7 +51,18 @@ func (s *switchAgent) Attach() (<-chan session.Event, bool, func()) { for _, ev := range s.backlog { out <- ev } - return out, true, func() { s.stops++ } + return out, true, func() { + s.mu.Lock() + s.stops++ + s.mu.Unlock() + } +} + +// setRunning changes what Attach answers once a watcher may be asking. +func (s *switchAgent) setRunning(running bool) { + s.mu.Lock() + s.running = running + s.mu.Unlock() } func (s *switchAgent) PendingConsent() []uint64 { return s.pending } diff --git a/internal/tui3/taskconversation_test.go b/internal/tui3/taskconversation_test.go index 20674a0ed0..e757ca57c6 100644 --- a/internal/tui3/taskconversation_test.go +++ b/internal/tui3/taskconversation_test.go @@ -287,7 +287,8 @@ func TestTheCallInFlightLeavesWhenItReturns(t *testing.T) { back := programPage(row, programTurns()) back.Program.Turns[2].Ended = taskFixtureNow back.Program.Actions = append(back.Program.Actions, delegate.Shown{At: taskFixtureNow, Step: "implement", Text: "ran go test ./internal/auth/...", Outcome: "passes"}) - fake.pages[row.ID], fake.pages[planTaskIDWord(row.ID)] = back, back + fake.setPage(row.ID, back) + fake.setPage(planTaskIDWord(row.ID), back) planBeat(t, a) lines := programPageLines(a) page := strings.Join(lines, "\n") @@ -541,13 +542,13 @@ func TestDrawingAProgramsPageReadsNothing(t *testing.T) { a.agent = counted a.width, a.height = 80, 30 openPlanPage(t, a) - rows, pages := counted.rows, counted.pages + rows, pages := counted.rows.Load(), counted.pages.Load() for i := 0; i < 3; i++ { programPageLines(a) a.programOf().calls = !a.programOf().calls } - if counted.rows != rows || counted.pages != pages { - t.Fatalf("drawing the page read the agent: rows %d→%d, pages %d→%d", rows, counted.rows, pages, counted.pages) + if counted.rows.Load() != rows || counted.pages.Load() != pages { + t.Fatalf("drawing the page read the agent: rows %d→%d, pages %d→%d", rows, counted.rows.Load(), pages, counted.pages.Load()) } } @@ -566,17 +567,17 @@ func TestTheRailRowOfAProgramsRunSaysItsStageAndSpend(t *testing.T) { if node == nil { t.Fatal("the run's row never reached the rail") } - reads := counted.rows + reads := counted.rows.Load() under := plain(strings.Join(a.railUnder(node, 40), "\n")) if !strings.Contains(under, "implement") || !strings.Contains(under, "$1.24") { t.Fatalf("the rail row of a program's run reads %q, want its stage and its spend", under) } - if counted.rows != reads || counted.pages != 0 { - t.Fatalf("drawing the rail row read the agent: rows %d→%d, pages %d", reads, counted.rows, counted.pages) + if counted.rows.Load() != reads || counted.pages.Load() != 0 { + t.Fatalf("drawing the rail row read the agent: rows %d→%d, pages %d", reads, counted.rows.Load(), counted.pages.Load()) } // A RUN NO PROGRAM WAS HANDED IS UNCHANGED. row.Program, row.Stage = "", "" - fake.plan[0] = row + fake.editRows(func(rows []session.PlanTaskRow) { rows[0] = row }) a.planRows = fake.plan plainUnder := plain(strings.Join(a.railUnder(node, 40), "\n")) if strings.Contains(plainUnder, "implement") || strings.Contains(plainUnder, "$1.24") { diff --git a/internal/tui3/taskpage_keys_test.go b/internal/tui3/taskpage_keys_test.go index 3d48a8dde9..8e8be0c349 100644 --- a/internal/tui3/taskpage_keys_test.go +++ b/internal/tui3/taskpage_keys_test.go @@ -23,7 +23,7 @@ func endPlanTask(counted *railPlanCounter, id, status string) { counted.setStatus(id, status) if page, ok := counted.planFake.pages[id]; ok { page.Row.Status = status - counted.planFake.pages[id] = page + counted.planFake.setPage(id, page) } } @@ -103,7 +103,7 @@ func TestKeysTypedWhileAPartsRoomOpensAreTheBoxAndNothingElse(t *testing.T) { counted.setStatus("3", "running") part := counted.planFake.pages["3"] part.Row.Status = "running" - counted.planFake.pages["3"] = part + counted.planFake.setPage("3", part) held := &heldRailPlan{railPlanCounter: counted, started: make(chan struct{}), release: make(chan struct{})} a.agent = held diff --git a/internal/tui3/taskplan_test.go b/internal/tui3/taskplan_test.go index 4cfb3aacfd..06f3d7bd77 100644 --- a/internal/tui3/taskplan_test.go +++ b/internal/tui3/taskplan_test.go @@ -10,6 +10,7 @@ import ( "context" "errors" "strings" + "sync" "testing" "time" @@ -49,8 +50,12 @@ func TestPlanUnstartedRunningRootShowsMachineHold(t *testing.T) { // a different one. type planFake struct { *taskFake - plan []session.PlanTaskRow - pages map[string]session.PlanTaskPage + // planMu guards plan and pages between the surface's off-loop reads and the + // verbs, which also run off the loop (offloop.go). Tests change them through + // setPage, editRows and setPlan, since a read may be out at any time. + planMu sync.Mutex + plan []session.PlanTaskRow + pages map[string]session.PlanTaskPage // THE SIX VERBS, each recording the call it was asked for so a test can read // back the id and the words a key produced. `refuse` is the sentence every one @@ -73,7 +78,13 @@ type planCall struct { n int } -func (f *planFake) PlanTasks() []session.PlanTaskRow { return f.plan } +// PlanTasks answers a copy, as the real store does: the surface holds what it +// read, and a verb writing the next status must not reach into that. +func (f *planFake) PlanTasks() []session.PlanTaskRow { + f.planMu.Lock() + defer f.planMu.Unlock() + return append([]session.PlanTaskRow(nil), f.plan...) +} // PlanRunSummary and RefreshRunSummary keep this general plan fixture on the // local-store door while answering that it has no stored summary. Summary tests @@ -87,10 +98,35 @@ func (f *planFake) RefreshRunSummary(context.Context, string, time.Time) (sessio } func (f *planFake) PlanTaskPage(id string) (session.PlanTaskPage, bool) { + f.planMu.Lock() + defer f.planMu.Unlock() page, ok := f.pages[id] return page, ok } +// setPage replaces one store page under the lock the off-loop read takes. +func (f *planFake) setPage(id string, page session.PlanTaskPage) { + f.planMu.Lock() + f.pages[id] = page + f.planMu.Unlock() +} + +// editRows changes the store's rows in place under the same lock. In place on +// purpose: [planAppWith] hands the surface these rows, so an edit here moves +// what the surface holds, the way a run moving its store does. +func (f *planFake) editRows(edit func(rows []session.PlanTaskRow)) { + f.planMu.Lock() + edit(f.plan) + f.planMu.Unlock() +} + +// setPlan replaces the store's rows; the surface learns of them on its next read. +func (f *planFake) setPlan(rows []session.PlanTaskRow) { + f.planMu.Lock() + f.plan = rows + f.planMu.Unlock() +} + func (f *planFake) PlanNote(id, text string) error { f.noted = append(f.noted, planCall{id: id, text: text}) if f.refuse == nil { @@ -103,6 +139,8 @@ func (f *planFake) PlanNote(id, text string) error { // note verb does: the next read carries it with its author and its moment, which // is the receipt the page draws and the store cannot draw itself. func (f *planFake) addNote(id, text string) { + f.planMu.Lock() + defer f.planMu.Unlock() page, ok := f.pages[id] if !ok { return @@ -148,12 +186,20 @@ func (f *planFake) PlanPriority(id string, n int) error { // setStatus writes a verb's effect back over the store row, which is what makes a // second `p` resume what the first paused: the toggle reads the status fresh // ([app.taskPlanPaused]) and the pane's next reading wears the new word. +// +// It writes a new slice rather than over the old one: [planAppWith] hands the +// surface the fake's own rows, and a verb runs off the loop while the loop may +// be drawing them. func (f *planFake) setStatus(id, status string) { - for i := range f.plan { - if f.plan[i].ID == id { - f.plan[i].Status = status + f.planMu.Lock() + defer f.planMu.Unlock() + rows := append([]session.PlanTaskRow(nil), f.plan...) + for i := range rows { + if rows[i].ID == id { + rows[i].Status = status } } + f.plan = rows } // planAppWith is [taskApp] over an agent that answers a plan: a pinned clock, a @@ -552,7 +598,7 @@ func planPageWithSteps(row session.PlanTaskRow, n int) session.PlanTaskPage { func appendPlanStep(fake *planFake, id string, n int) { page := fake.pages[id] page.Steps = append(page.Steps, session.PlanStep{Step: n, Command: planStepCommand(n)}) - fake.pages[id] = page + fake.setPage(id, page) } // planDrawnKin is the connector cell the place draws each plan row with, in draw @@ -695,7 +741,7 @@ func TestTheRunsPlanIsReadAgainOnItsOwnBeat(t *testing.T) { if got := len(a.taskSheet.mine.plan); got != 1 { t.Fatalf("the first reading holds %d plan rows, want the run's one", got) } - fake.plan = append(fake.plan, session.PlanTaskRow{ID: "p1", Parent: "1", Title: "a part the run added", Status: "running"}) + fake.setPlan(append(fake.plan, session.PlanTaskRow{ID: "p1", Parent: "1", Title: "a part the run added", Status: "running"})) now = now.Add(elsewhereEvery - time.Millisecond) readPlanRows(t, a) if got := len(a.taskSheet.mine.plan); got != 1 { diff --git a/internal/tui3/worktab_test.go b/internal/tui3/worktab_test.go index 8cd78d5367..b43fe5ebf8 100644 --- a/internal/tui3/worktab_test.go +++ b/internal/tui3/worktab_test.go @@ -30,7 +30,7 @@ func TestWorkTabAppearsAfterConversationOnlyForALiveRun(t *testing.T) { if len(tabs) != 2 || !tabs[0].here || !tabs[1].work || tabs[1].word != "Root" { t.Fatalf("live run tabs = %+v, want conversation then titled work tab", tabs) } - fake.plan[0].Status, fake.plan[1].Status = "done", "done" + fake.editRows(func(rows []session.PlanTaskRow) { rows[0].Status, rows[1].Status = "done", "done" }) a.workTabStable() a.workTabStable() if tabs := a.tabList(); len(tabs) != 1 || tabs[0].work { @@ -86,7 +86,7 @@ func TestTheRunsRoomDrawsNoAuthorAsAStoreID(t *testing.T) { {Author: "2ytmh2", Body: "the worker's own note"}, {Person: true, Body: "keep the middleware order"}, } - fake.pages["t-root"] = page + fake.setPage("t-root", page) a.height = 40 openWorkTabNow(t, a) text := planRoomText(t, a) @@ -106,7 +106,7 @@ func TestWorkTabEscReturnsToConversationAndLandingCardRemains(t *testing.T) { t.Fatal("esc left the run's room open") } drive(t, a, streamEventMsg{gen: a.gen, ev: update(7, "Root", session.TaskDone, session.TaskNotice{Report: "the importer landed"})}) - fake.plan[0].Status, fake.plan[1].Status = "done", "done" + fake.editRows(func(rows []session.PlanTaskRow) { rows[0].Status, rows[1].Status = "done", "done" }) a.workTabStable() a.workTabStable() if got := taskText(a); !strings.Contains(got, "Root") {