diff --git a/cmd/codeaf/appdevices.go b/cmd/codeaf/appdevices.go index e2c7ee630c..c406114b31 100644 --- a/cmd/codeaf/appdevices.go +++ b/cmd/codeaf/appdevices.go @@ -91,7 +91,7 @@ func (d *appDevices) snapshot() (appwire.Devices, error) { for deviceID, rec := range known.Devices { row := appwire.Device{ID: deviceID, Name: chatlist.DeviceName(known.Devices, deviceID, open), Platform: rec.Platform, Self: deviceID == self.ID(), Removed: rec.Revoked} if view.Mode == appwire.DevicesOn && !row.Self && !row.Removed { - row.RemoveLine = pair.ChatsRevokedLine(row.Name) + row.RemoveLine = pair.ChatsRemoveLine(row.Name) } if rec.LastSeen > 0 { row.LastSeen = time.UnixMilli(rec.LastSeen).UTC() diff --git a/cmd/codeaf/appdevices_test.go b/cmd/codeaf/appdevices_test.go index 62dd0cb132..8aea29a0e8 100644 --- a/cmd/codeaf/appdevices_test.go +++ b/cmd/codeaf/appdevices_test.go @@ -338,8 +338,9 @@ func TestAppDevicesV1V2V3PairedHomes(t *testing.T) { t.Fatalf("paired list: %+v", view) } // V3 amendment: the engine's target-specific explanation is available - // before removal, and only on a row this computer may remove. - if view.Devices[0].RemoveLine != "" || view.Devices[1].RemoveLine != pair.ChatsRevokedLine("laptop") { + // before removal, and only on a row this computer may remove. It says what + // removing will do; the receipt says it was done. + if view.Devices[0].RemoveLine != "" || view.Devices[1].RemoveLine != pair.ChatsRemoveLine("laptop") || view.Devices[1].RemoveLine == pair.ChatsRevokedLine("laptop") { t.Fatalf("removal confirmation copy: %+v", view.Devices) } for _, params := range []map[string]any{{"name": ""}, {"name": strings.Repeat("x", devname.Most+1)}, {"name": "a\nb"}, {"name": "other", "id": sb.Device.ID()}} { @@ -352,7 +353,7 @@ func TestAppDevicesV1V2V3PairedHomes(t *testing.T) { t.Fatalf("rename receipt: %+v", renamed) } appDeviceEvent(t, rec, func(v appwire.Devices) bool { - return len(v.Devices) == 2 && v.Devices[0].Name == "studio" && v.Devices[0].RemoveLine == "" && v.Devices[1].RemoveLine == pair.ChatsRevokedLine("laptop") + return len(v.Devices) == 2 && v.Devices[0].Name == "studio" && v.Devices[0].RemoveLine == "" && v.Devices[1].RemoveLine == pair.ChatsRemoveLine("laptop") }) other := appDeviceList(t, wb) if len(other.Devices) != 2 || other.Devices[0].ID != sb.Device.ID() || other.Devices[1].Name != "studio" { diff --git a/docs/changes/unreleased/app-hello-first-remove-confirmation.md b/docs/changes/unreleased/app-hello-first-remove-confirmation.md new file mode 100644 index 0000000000..01cde36359 --- /dev/null +++ b/docs/changes/unreleased/app-hello-first-remove-confirmation.md @@ -0,0 +1,18 @@ +--- +kind: fixed +title: The app's first line is always its hello, and removal is confirmed in the future tense +pr: 1983 +surface: [engine] +invalidates: + - "An app engine could write sync.changed before its hello when startup was slow (the idle sweep reads sync status every 30 seconds). Events emitted before the hello now wait and follow it, in order." + - "devices.list's remove_line used the after-removal sentence (\"… was removed\") before anything was removed. It now says what removing will do; the devices.remove receipt keeps the after sentence." +--- + +The desktop refuses a connection whose first line is not the hello, so a slow +start could leave it unable to reach the engine at all. The appwire server now +holds any event emitted before Serve has said hello and writes it straight after, +so the hello always comes first and sequence numbers stay in write order. + +A computer's `remove_line` is shown before the person confirms. It now reads +" will no longer sync your chats", followed by the same warning about what +removal cannot undo; the receipt after `devices.remove` is unchanged. diff --git a/internal/appwire/server.go b/internal/appwire/server.go index 0ff7068e3f..6a6638ae34 100644 --- a/internal/appwire/server.go +++ b/internal/appwire/server.go @@ -39,6 +39,12 @@ type Server struct { // the queue, so no frame is ever sent on a closed channel. mu sync.RWMutex closed bool + // greeted is whether the hello has been queued. Until it has, every other + // frame waits in early: the app refuses a connection whose first line is + // not the hello, and the backend can emit before Serve has built it (a + // slow start's idle sweep, a crew opening its members). + greeted bool + early []frame wrote chan struct{} err error @@ -116,17 +122,41 @@ func (s *Server) Emit(event string, data any) { func (s *Server) send(f frame) { s.mu.RLock() - defer s.mu.RUnlock() - if s.closed { + if s.greeted || s.closed { + defer s.mu.RUnlock() + if !s.closed { + s.frames <- f + } return } - s.frames <- f + s.mu.RUnlock() + s.mu.Lock() + defer s.mu.Unlock() + switch { + case s.closed: + case s.greeted: + s.frames <- f + case f.event == EventHello: + s.greeted = true + s.frames <- f + for _, held := range s.early { + s.frames <- held + } + s.early = nil + default: + s.early = append(s.early, f) + } } // finish closes the queue and waits for every queued line to be written. func (s *Server) finish() { s.mu.Lock() if !s.closed { + // A server that never said hello still writes what was emitted to it. + for _, held := range s.early { + s.frames <- held + } + s.early = nil s.closed = true close(s.frames) } diff --git a/internal/appwire/server_test.go b/internal/appwire/server_test.go index 2d93bf906a..f74630355e 100644 --- a/internal/appwire/server_test.go +++ b/internal/appwire/server_test.go @@ -349,6 +349,42 @@ func TestSequenceNumbersAreConsecutiveAcrossGoroutines(t *testing.T) { } } +// The app refuses a connection whose first line is not the hello, so what the +// backend emits before Serve has said it follows the hello, in order. +func TestAnEventEmittedBeforeTheHelloFollowsIt(t *testing.T) { + inR, inW := io.Pipe() + outR, outW := io.Pipe() + server := NewServer(outW) + server.Emit(EventSyncChanged, SyncChanged{Sync: Sync{Mode: SyncOff}}) + server.Emit(EventLog, Log{Level: "info", Message: "early"}) + served := make(chan error, 1) + go func() { + served <- server.Serve(context.Background(), inR, Hello{Version: "test"}, &fakeEngine{}) + outW.Close() + }() + scanner := bufio.NewScanner(outR) + scanner.Buffer(make([]byte, 1<<20), 1<<20) + var got []eventLine + for len(got) < 3 && scanner.Scan() { + var line eventLine + if err := json.Unmarshal(scanner.Bytes(), &line); err != nil { + t.Fatal(err) + } + got = append(got, line) + } + inW.Close() + <-served + want := []string{EventHello, EventSyncChanged, EventLog} + if len(got) != len(want) { + t.Fatalf("read %d lines, want %d", len(got), len(want)) + } + for i, line := range got { + if line.Event != want[i] || line.Seq != uint64(i+1) { + t.Fatalf("line %d = %s seq %d, want %s seq %d", i, line.Event, line.Seq, want[i], i+1) + } + } +} + // recorder is an Emitter that keeps what it was handed. type recorder struct { mu sync.Mutex diff --git a/internal/pair/list.go b/internal/pair/list.go index bea93ea09b..65d9a1a5a7 100644 --- a/internal/pair/list.go +++ b/internal/pair/list.go @@ -144,6 +144,13 @@ func chatsMarks(one ChatsDevice) string { // chats has been removed. It says what removing cannot do, because the person // who has just removed a stolen computer is the one who most needs to know. func ChatsRevokedLine(name string) string { - return name + " was removed — it can no longer sync your chats.\n" + - "It cannot undo what that device already holds: your chats and keys. If it was lost or stolen, treat your chats as exposed and run `codeaf identity rotate` to lock it out for good." + return name + " was removed — it can no longer sync your chats.\n" + revokeLimit } + +// ChatsRemoveLine is what a person reads before removing that computer: what +// removing will do, and the same limit, said while they can still choose. +func ChatsRemoveLine(name string) string { + return name + " will no longer sync your chats.\n" + revokeLimit +} + +const revokeLimit = "It cannot undo what that device already holds: your chats and keys. If it was lost or stolen, treat your chats as exposed and run `codeaf identity rotate` to lock it out for good."