diff --git a/cmd/codeaf/c10_sync_unnamed_test.go b/cmd/codeaf/c10_sync_unnamed_test.go new file mode 100644 index 0000000000..b9c12dcb3d --- /dev/null +++ b/cmd/codeaf/c10_sync_unnamed_test.go @@ -0,0 +1,64 @@ +package main + +import ( + "fmt" + "os" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/enginehost" + "github.com/Agent-Field/codeaf/internal/remote" + "github.com/Agent-Field/codeaf/internal/syncsetup" +) + +// `codeaf chat --host` runs `codeaf engine` under a non-interactive ssh shell, +// which rarely carries the CODEAF_SYNC_URL the person exported in their own +// terminal. That launch named no relay, so it must join the host the terminal +// started — idle or busy — rather than retiring it or being refused by it; +// otherwise the two launches take the workspace from each other in turn. +func TestC10ALaunchThatNamesNoRelayJoinsTheHostATerminalStarted(t *testing.T) { + for _, busy := range []bool{false, true} { + t.Run(fmt.Sprintf("busy=%t", busy), func(t *testing.T) { + workspace, _ := environmentTestHome(t) + t.Setenv(syncsetup.URLVar, "http://127.0.0.1:41001") + child := environmentChild(t, workspace, os.Getenv(config.APIKeyEnv)) + if busy { + environmentWindow(t, workspace, "") + } + waitEnvironmentHost(t, workspace, func(self remote.HostSelf) bool { return self.Busy == busy }) + t.Setenv(syncsetup.URLVar, "") + if enginehost.SyncDiffers(workspace) || enginehost.EnvironmentDiffers(workspace) { + t.Fatal("a launch that named no relay reads the host's own variable as a different setting") + } + note, err := clearStaleEngineHost(workspace) + if err != nil || note != "" { + t.Fatalf("a launch that named no relay displaced the host: note=%q err=%v", note, err) + } + self, err := enginehost.Ask(workspace, remote.WhoIs{}) + if err != nil || self.PID != child.Process.Pid { + t.Fatalf("the host started by the terminal was replaced: %+v %v", self, err) + } + }) + } +} + +// Naming no relay is not a way to miss a real change: when the address a +// pairing saved moves, both sides read it from the same home, and an idle +// host still restarts to pick it up. +func TestC10AChangedSavedRelayStillRetiresAnIdleHost(t *testing.T) { + workspace, _ := environmentTestHome(t) + t.Setenv(syncsetup.URLVar, "") + if err := syncsetup.SaveRelayURL(os.Getenv("CODEAF_HOME"), "http://127.0.0.1:41001"); err != nil { + t.Fatal(err) + } + environmentChild(t, workspace, os.Getenv(config.APIKeyEnv)) + waitEnvironmentHost(t, workspace, func(self remote.HostSelf) bool { return !self.Busy }) + if err := syncsetup.SaveRelayURL(os.Getenv("CODEAF_HOME"), "http://127.0.0.1:41002"); err != nil { + t.Fatal(err) + } + note, err := clearStaleEngineHost(workspace) + if err != nil || !strings.Contains(note, "restarted") || !nothingHolds(workspace) { + t.Fatalf("an idle host kept a relay the pairing replaced: note=%q err=%v", note, err) + } +} diff --git a/docs/changes/unreleased/2067-host-relay-engine-pingpong.md b/docs/changes/unreleased/2067-host-relay-engine-pingpong.md new file mode 100644 index 0000000000..669414b149 --- /dev/null +++ b/docs/changes/unreleased/2067-host-relay-engine-pingpong.md @@ -0,0 +1,27 @@ +--- +kind: fixed +title: codeaf chat --host no longer takes the engine away from a window that set CODEAF_SYNC_URL, and back +pr: 2067 +surface: [engine, remote] +invalidates: + - >- + A window attaching to a running session host compared its whole resolved + relay with the host's, so a launch with CODEAF_SYNC_URL unset (the ssh + child of --host, or any shell that never exported it) counted as a + different sync setting and retired an idle host or was refused by a busy + one. Now only a relay the launch names itself is compared whole; a launch + with the variable unset is compared only on the pairing's saved address + (else the hosted default), which the host records as sync-unnamed. + - >- + The host's provider `environment` fingerprint included the resolved relay. + It no longer does; the relay is judged only by SyncDiffers, and + EnvironmentMAC (the automations clock) still includes it. +--- + +On a machine where a person ran codeaf with `CODEAF_SYNC_URL` pointing at a +self-hosted relay, `codeaf chat --host` from another machine ran `codeaf engine` +under an ssh shell without that variable. The engine read the silence as a +different relay and retired the host or refused to attach, and the local +window's next launch did the same in reverse. A terminal that explicitly names +a different address or `off` is still refused or restarts the engine exactly as +before. diff --git a/internal/enginehost/environment.go b/internal/enginehost/environment.go index dfe452b688..4d1a453045 100644 --- a/internal/enginehost/environment.go +++ b/internal/enginehost/environment.go @@ -23,8 +23,19 @@ const environmentName = "environment" // must be refused even when a provider environment change can join busy work. const syncEnvironmentName = "sync-environment" -func syncEnvironment() string { - relay := syncsetup.Resolve(home.Dir()) +// syncUnnamedName is the relay the host would have resolved with no +// CODEAF_SYNC_URL of its own. A launch that names no relay is compared against +// THIS: `codeaf chat --host` runs `codeaf engine` under a non-interactive ssh +// shell that rarely carries the variable, and comparing its silence with the +// variable of the terminal that started the host retired that host, and the +// terminal's next launch retired the ssh one's, back and forth. +const syncUnnamedName = "sync-unnamed" + +func syncEnvironment() string { return relayWord(syncsetup.Resolve(home.Dir())) } + +func syncUnnamed() string { return relayWord(syncsetup.ResolveUnnamed(home.Dir())) } + +func relayWord(relay syncsetup.Relay) string { if relay.Off { return "off" } @@ -96,11 +107,18 @@ func fingerprintMAC(salt []byte, value string) []byte { // writeEnvironment writes a fresh salt and digest atomically, so a launcher // never compares a partly written answer. No environment value is saved, even // when the environment contains a provider key with an unfamiliar shape. +// +// THE PROVIDER FINGERPRINT LEAVES THE RELAY OUT. The relay has its own two +// answers and its own rule ([SyncDiffers]), and folding it in here too would +// let a launch that named no relay restart an idle host through the back door. func writeEnvironment(dir, workspace string) error { if err := writeFingerprint(dir, syncEnvironmentName, syncEnvironment()); err != nil { return err } - return writeFingerprint(dir, environmentName, resolvedEnvironment(workspace)) + if err := writeFingerprint(dir, syncUnnamedName, syncUnnamed()); err != nil { + return err + } + return writeFingerprint(dir, environmentName, canonicalEnvironment(workspace)) } func writeFingerprint(dir, name, value string) error { @@ -127,15 +145,27 @@ func writeFingerprint(dir, name, value string) error { // or malformed files mean the host has not said, which must never be a reason // to restart it. Comparison stays local and takes constant time for the digest. func EnvironmentDiffers(workspace string) bool { - return fingerprintDiffers(workspace, environmentName, resolvedEnvironment(workspace), false) + return fingerprintDiffers(workspace, environmentName, canonicalEnvironment(workspace), false) || SyncDiffers(workspace) } // SyncDiffers compares the relay the host resolved at startup with this // terminal's choice, including a pairing's saved address and the off switch. +// +// ONLY A RELAY THIS PROCESS NAMES IS A CHOICE. With CODEAF_SYNC_URL set, the +// whole resolved answer is compared, and an address or off that differs from +// the host's must not be joined. With it unset, this process said nothing +// about the relay, so only what both sides read from the same home — a +// pairing's saved address, else the hosted default — is compared; the +// variable the host's own terminal carried is that terminal's word and stands. func SyncDiffers(workspace string) bool { - // A host from before this fingerprint cannot confirm an explicit address - // or off switch. That choice must not fall through to busy-host reuse. - return fingerprintDiffers(workspace, syncEnvironmentName, syncEnvironment(), env.Get(syncsetup.URLVar) != "") + if env.Get(syncsetup.URLVar) != "" { + // A host from before this fingerprint cannot confirm an explicit address + // or off switch. That choice must not fall through to busy-host reuse. + return fingerprintDiffers(workspace, syncEnvironmentName, syncEnvironment(), true) + } + // A host from before the unnamed fingerprint cannot say, and a process that + // named nothing has no choice for it to be ignoring: join it. + return fingerprintDiffers(workspace, syncUnnamedName, syncUnnamed(), false) } func fingerprintDiffers(workspace, name, value string, unknown bool) bool { diff --git a/internal/enginehost/host.go b/internal/enginehost/host.go index f99b0139c2..78a4382df1 100644 --- a/internal/enginehost/host.go +++ b/internal/enginehost/host.go @@ -209,7 +209,9 @@ func Run(workspace string, opts Options) error { // THE ENVIRONMENT IS RECORDED BEFORE THE SOCKET ANSWERS. Only the lock // owner writes it, so a replacement cannot publish the previous host's // fingerprint during its birth. A failed write leaves the answer unknown. - _ = os.Remove(filepath.Join(dir, environmentName)) + for _, name := range []string{environmentName, syncEnvironmentName, syncUnnamedName} { + _ = os.Remove(filepath.Join(dir, name)) + } _ = writeEnvironment(dir, workspace) // Whatever socket file is lying there belongs to a host that is gone: the diff --git a/internal/manual/chat/running-from-the-terminal.md b/internal/manual/chat/running-from-the-terminal.md index 26825e9607..020422d464 100644 --- a/internal/manual/chat/running-from-the-terminal.md +++ b/internal/manual/chat/running-from-the-terminal.md @@ -462,7 +462,9 @@ other machines cannot be reached it starts with `other machines unreachable`. ## A different sync setting on an existing engine -The engine keeps its sync setting from startup. Changing `CODEAF_SYNC_URL` to another address or `off` retires an idle engine before opening a chat and says `the engine restarted to pick up this terminal's sync setting`. If another window or work still uses it, the new window refuses to attach: `the engine for this folder started with a different environment than this terminal and is still in use — close its other windows or let its work finish, or run codeaf engine --stop --workspace `. The command names the exact folder. Let that work finish or stop the engine explicitly before reopening with the new setting. +The engine keeps its sync setting from startup. Changing `CODEAF_SYNC_URL` to another address or `off` retires an idle engine before opening a chat and says `the engine restarted to pick up this terminal's sync setting`. If another window or work still uses it, the new window refuses to attach: `the background engine for this folder started with a different environment than this terminal and is still in use — after work finishes, close every window for this workspace and reopen. To stop now (interrupts active turns and tasks), run codeaf engine --stop --workspace `. The command names the exact folder. Let that work finish or stop the engine explicitly before reopening with the new setting. + +Only a terminal that sets `CODEAF_SYNC_URL` itself is asking for a different relay. A window with the variable unset — `codeaf chat --host` reaching this computer over ssh, whose shell usually does not carry what you exported, or a terminal you never set it in — joins the running engine and its relay as it is. It restarts the engine only when the relay a pairing saved on this computer has changed since the engine started. ## Sync your chats between machines — CODEAF_SYNC_URL and CODEAF_SYNC_INTERVAL_MS diff --git a/internal/syncsetup/relayurl.go b/internal/syncsetup/relayurl.go index 5dc8f11c50..521b8ba290 100644 --- a/internal/syncsetup/relayurl.go +++ b/internal/syncsetup/relayurl.go @@ -59,6 +59,22 @@ func Resolve(home string) Relay { return Relay{} } +// ResolveUnnamed is the relay this computer syncs through when the process +// asking names none of its own: the address a pairing saved, else the hosted +// default. CODEAF_SYNC_URL is a word said in ONE shell, and a process started +// somewhere that never said it — `codeaf engine` under ssh for --host, most of +// all — has not chosen a different relay by staying quiet. The session host +// compares such a process against this answer and never against the variable +// the terminal that started it happened to carry (internal/enginehost). +func ResolveUnnamed(home string) Relay { + for _, from := range sources[1:] { + if relay, ok := from(home); ok { + return relay + } + } + return Relay{} +} + // Named is url when it is an address someone chose, and empty when it is the // hosted default, which needs no naming to another computer. func Named(url string) string {