Skip to content

tui3: make the switch and plan test fakes safe for off-loop readers - #1836

Merged
ZeroPoint95 merged 5 commits into
devfrom
zeropoint95/fix-held-send-races
Oct 15, 2026
Merged

ZeroPoint95 merged 5 commits into
devfrom
zeropoint95/fix-held-send-races

Conversation

@ZeroPoint95

@ZeroPoint95 ZeroPoint95 commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes the pre-existing -race failures in tui3's held-send and plan tests. Every race was in a test fake; production code is unchanged.

What changed

Why it's the fakes, not production

  • Each race report has a test fake on one side and an off-loop reader on the other: the keeper watcher calling Attach, or the door line calling the plan reads and verbs.
  • Attach is locked in production: session/agent.go holds a.mu, and remote/observe.go holds c.mu.
  • The plan methods already build fresh results per call:
    • session/plandb_tasks.go (PlanTasks, PlanTaskPage) opens its own store handles each time and builds a new slice.
    • session/plandb_steer.go (the verbs) writes the store, not anything the surface holds.
    • remote/client.go is RPC that unmarshals a fresh value per call, so it needs no lock.

Changes (test files only, plus the change entry)

  • switchAgent: a mutex guards running/backlog/stops for Attach and its stop. Tests that flip running once a watcher may be live use setRunning.
  • planFake:
    • PlanTasks returns a copy.
    • setStatus writes a new slice instead of the one planAppWith gave the surface.
    • plan and pages are locked, and test writes go through setPage / editRows / setPlan.
  • railPlanCounter: the rows/pages counters are atomic.

Tests fixed

  • TestAFailedHeldSendStaysWaitingWithItsAttachmentsAndNotesTheFailure
  • TestAWaitingPictureSendsInItsHeldConversationAndTheWatcherAdoptsItsTurn
  • TestComingBackDuringAHeldSendNeverSendsItTwice
  • TestHeldWaitingMessagesGoOnePerLandingAfterSessionFollowUpsAndNeverTwice
  • TestStopOnTheRunsOwnPageRaisesTheCardAndTheCardStopsTheRun
  • Same class, also red on dev:
    • TestAProgramRoomReadsTheLandingTheStoreEndedAhead
    • TestTheCancelKeyOnAPlanRowEndsItThroughTheStore
    • TestPOnAPlanRowPausesThenResumes

How it was checked

  1. dev (6c65989), the 10 tests by exact name, go test -race -count=1
    • Printed FAIL for the 8 above, with 10 WARNING: DATA RACE reports.
    • The other 2 are siblings matched by the same patterns, and they pass on dev too: TestHeldWaitingMessagesUsePasteAndRemoteFileDoors and TestAHeldWaitingChatTitleRedrawsWithoutAnotherAttentionNotice.
  2. This head (merged with that dev), the same 10, -race -count=5
    • Printed --- PASS 5/5 for all 10 and ok internal/tui3, with 0 DATA RACE reports.
  3. Broad run: every test in a file that uses these fakes or that this PR touches, 496 tests, -race -count=1
    • Printed 496 PASS, 0 FAIL and 0 DATA RACE.
  4. make changelog-check
    • Printed 11 entries, all well formed.
  5. Repro:
    • GOFLAGS=-mod=readonly go test -race -count=5 -run 'HeldSend|HeldWaiting|WaitingPicture|ComingBackDuringAHeldSend|TestStopOnTheRunsOwnPage|TestAProgramRoomReadsTheLanding|TestTheCancelKeyOnAPlanRow|TestPOnAPlanRow' ./internal/tui3/

Checklist

  • A change entry: docs/changes/unreleased/1836-held-send-race-fakes.md (kind: internal).
  • The manual knows about it: not needed, since no command, key, tool, default, limit or refusal moved.
  • No new line in .github/known-red.txt: none added (the file doesn't exist on dev).
  • Only my own paths are staged: each file added by name, no git add -A.

🤖 Generated with Claude Code

ZeroPoint95 and others added 5 commits October 14, 2026 21:06
The -race failures in the held-send tests and the run-stop card test were in
the test seams, not the surface. The kept conversation's watcher calls
Attach from its own goroutine, and the door line asks PlanTasks,
PlanTaskPage and the plan verbs off the loop; the real agents take their
locks for all of these. The fakes did not:

- switchAgent: Attach and its stop guard running/backlog/stops with a
  mutex; tests that flip running while a watcher is live use setRunning.
- planFake: PlanTasks answers a copy as the store does; setStatus writes a
  new slice instead of over the one planAppWith handed the surface; pages
  are guarded, and the program-room test writes them through setPage.

Fixes the four held-send races, TestStopOnTheRunsOwnPageRaisesTheCardAndTheCardStopsTheRun,
and three more of the same class found on dev: TestAProgramRoomReadsTheLandingTheStoreEndedAhead,
TestTheCancelKeyOnAPlanRowEndsItThroughTheStore, TestPOnAPlanRowPausesThenResumes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…il counters

Review follow-ups on #1836:

- railPlanCounter's rows/pages counts are atomic; they are bumped by the
  surface's off-loop reads.
- planFake gains editRows (in place, keeping planAppWith's aliasing) and
  setPlan; every direct test write to fake.plan and fake.pages goes through
  them or setPage.
- switchAgent.mu's comment says exactly what it covers.
- The change entry's invalidates line is cut to the rules' length and drops
  surface for this test-only change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ZeroPoint95
ZeroPoint95 marked this pull request as ready for review October 15, 2026 13:06
@ZeroPoint95
ZeroPoint95 merged commit 6e73ac7 into dev Oct 15, 2026
5 checks passed
@ZeroPoint95
ZeroPoint95 deleted the zeropoint95/fix-held-send-races branch October 15, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant