[frahlg]

Found by the review of #1841 on 25 October 2026 and left out of that PR.

Problem

appPlans.Rev (go/cmd/ftw/app_link.go) gives the plan a number that moves when the planner replans, so the app can tell a new plan from the same one sent again. Every app session on the box shares one appPlans. With two sessions, for example a phone and a watch, one replan could move the number more than once.

Cause

Rev read the planner's latest plan before it took revMu. A slow session could read the old plan, then wait for the lock while another session read the new plan and counted it. When the slow session got the lock, its old plan's timestamp differed from the one just stored, so it counted again and stored the old timestamp. The next read then counted the new plan a third time. One replan moved the number from 1 to 4. #1841 makes each message re-read until its plan is stable, so the cost is the jump and extra pushes, not a mixed plan.

Fix

Rev reads the plan under revMu. Reads are then counted in the order they happen, and the planner's latest plan only moves forward, so each replan moves the number once. A plan copy that only adds the shadow result keeps its GeneratedAtMs and still moves nothing.

The lock order is revMu, then the planner's read lock. Nothing that holds the planner's lock calls Rev.

Evidence

TestPlanRevMovesOncePerReplanAcrossSessions holds revMu, lets a session start Rev against the old plan, installs a new plan, and counts it as another session's Rev would under the lock. On master, 10 of 10 runs:

app_link_test.go:497: the slow session got rev 3 for the replan, want 2
app_link_test.go:500: the next read got rev 4 with no replan since, want 2

On this branch it passes. Its 50 ms wait cannot make it fail here: the session reads the plan only once it holds the lock. On master, a session slower than 50 ms to reach the lock would make it pass by mistake.

• go test -race -count=2 -run 'TestPlanRev|Plan|AppLink' ./cmd/ftw/ passes.
• go vet ./... passes.
• go test ./internal/... ./cmd/ftw/... passes.

Review

An independent review (Sonnet) found no bug. It checked every write to the planner's latest plan: the shadow copy keeps GeneratedAtMs, the solve and InstallPlan publish fresh plans, and RestoreDiagnostic runs before the app link starts. It traced the lock order planMu, revMu, then the planner's read lock, and found no path the other way: plan-change hooks run after the planner unlocks, and PlanFeed pushes on its own goroutine. With the fix reverted, the test failed 5 of 5 runs with the message above.

Its notes: the changeset said a replan moves the number "by one", but two replans between two reads move it once in total; the second commit says "at most once". It also noted that the 50 ms wait only matters on master, and that a stress test with many sessions and plans would guard a future regression more strongly. I kept the single deterministic test.

Patch changeset.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-25T13:19:45.419937Z	edf7b32	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.