diff --git a/cmd/codeaf/synctake.go b/cmd/codeaf/synctake.go index 6e8e45d638..b4407f49d0 100644 --- a/cmd/codeaf/synctake.go +++ b/cmd/codeaf/synctake.go @@ -4,6 +4,7 @@ import ( "context" "errors" "log" + "net/url" "os" "path/filepath" "time" @@ -372,3 +373,18 @@ func (t takeover) SafeToContinue(ctx context.Context, id, current string, automa } return !dirty, err } + +// ReleaseUnused leaves the installed files and kept work intact, just as closing +// a chat does, while returning an abandoned automatic acquisition to the directory. +func (t takeover) ReleaseUnused(ctx context.Context, id string, fence uint64) error { + return t.c.ReleaseUnused(ctx, id, fence) +} + +// RelayHost names the endpoint without exposing a path or credentials in chat copy. +func (s surfaceMachines) RelayHost() string { + parsed, err := url.Parse(s.Relay) + if err != nil { + return "" + } + return parsed.Hostname() +} diff --git a/docs/changes/unreleased/2089-relay-l3-findings.md b/docs/changes/unreleased/2089-relay-l3-findings.md new file mode 100644 index 0000000000..c49cab1e67 --- /dev/null +++ b/docs/changes/unreleased/2089-relay-l3-findings.md @@ -0,0 +1,19 @@ +--- +kind: fixed +title: Continuing opens the chat you picked, a refused continue keeps its card, and a slow relay says so +pr: 2089 +surface: [chat, remote, docs] +invalidates: + - "A bare codeaf chat that was continuing the newest chat another computer let go of answered every explicit continue on Home with a chat is already continuing here, and the older chat opened; now a confirmed different chat waits, the automatic move finishes and is let go without opening, and the chosen chat opens. The busy line now answers only a second pick during a continue the person started." + - "A continue refused because the chat's holder changed showed another device continued this chat first in the footer only; every such refusal now keeps the 1 retry / 2 leave it there card and the draft, and when the holder named on the card simply reopened the chat the card asks again instead of blaming another device." + - "Home gave up on the chat listing after 5 seconds and said other machines did not answer in time; it now waits up to 25 seconds, says waiting for ; still checking after 5, keeps its rows, and still shows a real outage at once." + - "The first message sent right after a move could be left in the box under Home's timeout sentence; a send right after this window's own move now waits up to 30 seconds for the arriving chat to be ready and sends once, and a send that cannot leave says the message is still in the box and names the relay." + - "A self-hosted codeaf relay counted the first X-Forwarded-For address for its registration and dial limits whenever a request sent one, so a relay facing the internet could be given any address; forwarding headers now count only with --trust-proxy, for registration, dial and pairing limits alike, and a relay behind a proxy must pass it or every caller shares one allowance." + - "A chat this computer had never held came over serial object batches whenever the shared store had exchanged any other chat with the relay; it now downloads its frames, and a chat this computer already holds still fetches only what changed." +--- + +Found by the two-network test of the relay (lane L3). Each continue also writes +one `chat.log` line with the time spent in each phase, so a slow move can be read +instead of guessed. Consent to continue still binds the holder and its ownership +number shown on the card; nothing about ownership rules, wire or stored formats, +or the hosted relay changed. diff --git a/docs/relay.md b/docs/relay.md index 1821d6de32..90a1560fca 100644 --- a/docs/relay.md +++ b/docs/relay.md @@ -19,12 +19,14 @@ The separate `relay` binary accepts the same flags. | `--store ` | Persistent coordination and encrypted chunks. Without it, only the blind transport and pairing mailbox run; chat sync needs storage. | | `--quiet` | Suppress arrival, departure and request logs. | | `--status=false` | Disable `GET /status`. | -| `--trust-proxy` | Use the first `X-Forwarded-For` address for pairing limits, only behind a proxy that sets it. | +| `--trust-proxy` | use the first `X-Forwarded-For` address for registration, dial and pairing limits; absent or blank falls back to the socket peer. only behind a proxy that sets it. | Set `CODEAF_SYNC_URL=https://relay.example.com` on each computer, or use `codeaf pair --via https://relay.example.com`. Pair the computers with the same identity and relay. `CODEAF_SYNC_URL=off` stops sync, including existing clients. +forwarding headers are ignored by default. behind Railway, a load balancer or nginx, pass `--trust-proxy` or every caller shares one allowance. enable it only behind a proxy that sets the header. + The relay supplies no TLS or list of allowed people. Use a TLS proxy and a private network or a proxy that checks callers: any valid signature can create an identity. Logs contain a short identity id, route pattern, status and byte counts, never a body. diff --git a/internal/cellstore/chat_warmth_test.go b/internal/cellstore/chat_warmth_test.go new file mode 100644 index 0000000000..7537814df3 --- /dev/null +++ b/internal/cellstore/chat_warmth_test.go @@ -0,0 +1,29 @@ +package cellstore + +import ( + "os" + "path/filepath" + "testing" + + "github.com/Agent-Field/codeaf/internal/cell" +) + +func TestChatWarmthBelongsToItsOwnHead(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + data := t.TempDir() + c := cell.Cell{ID: "new-chat", Root: t.TempDir()} + e := SyncEngine{Ledger: "used", Target: func(cell.Cell) Target { return Target{DataDir: data} }} + if err := os.WriteFile(filepath.Join(data, "published.used"), []byte("earlier chat's objects"), 0600); err != nil { + t.Fatal(err) + } + if e.Holds(c) { + t.Fatal("an unrelated chat's ledger made this chat warm") + } + if err := noteAdopted(c, "held-head"); err != nil { + t.Fatal(err) + } + if !e.Holds(c) { + t.Fatal("this chat's adopted history was forgotten") + } +} diff --git a/internal/cellstore/syncops.go b/internal/cellstore/syncops.go index b51b6ef5c7..94cbb62f8f 100644 --- a/internal/cellstore/syncops.go +++ b/internal/cellstore/syncops.go @@ -241,12 +241,15 @@ func (e SyncEngine) importWith(ctx context.Context, c cell.Cell, op importOp) (i return out.Imported, e.ask(ctx, c, op, &out) } -// Holds implements the sync seam: the ledger file the engine keeps beside the -// store is written by every export and import against this relay, so it has -// content exactly when the device has exchanged objects with the relay. +// Holds reports history this computer held for this chat. The publication +// ledger belongs to the shared store, so another chat's exchange proves nothing +// about this one. Local adoption and turn records already supply this evidence. func (e SyncEngine) Holds(c cell.Cell) bool { - info, err := os.Stat(filepath.Join(e.Target(c).DataDir, "published."+e.Ledger)) - return err == nil && info.Size() > 0 + if _, ok := readAdoption(c); ok { + return true + } + turn, err := lastTurn(rel(c, TurnsPath)) + return err == nil && turn != nil } // Materialize implements the sync seam. diff --git a/internal/cellsync/chat_priming_test.go b/internal/cellsync/chat_priming_test.go new file mode 100644 index 0000000000..5a50d2fe77 --- /dev/null +++ b/internal/cellsync/chat_priming_test.go @@ -0,0 +1,210 @@ +package cellsync + +import ( + "bytes" + "context" + "crypto/rand" + "os" + "path/filepath" + "sync/atomic" + "testing" + + "github.com/Agent-Field/codeaf/internal/blobstore" + "github.com/Agent-Field/codeaf/internal/cell" + "github.com/Agent-Field/codeaf/internal/cell/celltest" + "github.com/Agent-Field/codeaf/internal/cellstore" + "github.com/Agent-Field/codeaf/internal/furrow" +) + +type chatFetchCounts struct { + blobstore.Store + frames, objects, bytes atomic.Int64 +} + +func (s *chatFetchCounts) GetFrame(ctx context.Context, id string) ([]byte, error) { + s.frames.Add(1) + raw, err := s.Store.GetFrame(ctx, id) + s.bytes.Add(int64(len(raw))) + return raw, err +} +func (s *chatFetchCounts) GetMany(ctx context.Context, ids []string) ([]blobstore.Object, error) { + s.objects.Add(1) + objects, err := s.Store.GetMany(ctx, ids) + for _, o := range objects { + s.bytes.Add(int64(len(o.Bytes))) + } + return objects, err +} + +type wantCountingEngine struct { + Engine + wants atomic.Int64 +} + +func (e *wantCountingEngine) Want(ctx context.Context, c cell.Cell, h string) ([]string, error) { + e.wants.Add(1) + return e.Engine.Want(ctx, c, h) +} + +type usedStoreRig struct { + sender cellstore.Engine + pub *Publisher + receiver cellstore.SyncEngine + fetch *Fetcher + counts *chatFetchCounts + cell cell.Cell + source cell.Cell + earlier cell.Cell + earlierHead string + body []byte + head string +} + +func newUsedStoreRig(t *testing.T) *usedStoreRig { + t.Helper() + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + celltest.RequireEngine(t) + binary, err := furrow.ResolveOwned() + if err != nil { + t.Skip(err) + } + t.Cleanup(cellstore.StopSpawned) + sender := cellstore.Engine{Binary: binary, DataRoot: t.TempDir(), Transport: cellstore.Spawn{Binary: binary}} + receiver := cellstore.Engine{Binary: binary, DataRoot: t.TempDir(), Transport: cellstore.Spawn{Binary: binary}} + keys := cellstore.SyncKeys{CellKey: make([]byte, 32), Dedup: make([]byte, 32)} + send := sender.Sync(keys, "0123456789abcdef") + recv := receiver.Sync(keys, "0123456789abcdef") + r := newRig(t) + pub := &Publisher{Engine: send, Store: r.mem, Dir: r.dirA} + counts := &chatFetchCounts{Store: r.mem} + f := &Fetcher{Engine: recv, Store: counts, Dir: r.dir.For(devB), Inbox: recv.Inbox} + makeChat := func() (cell.Cell, cell.Cell) { + c, err := cell.CreateIn(t.TempDir(), cell.Options{Class: cell.Sandboxed}) + if err != nil { + t.Fatal(err) + } + local := cell.Cell{ID: c.ID, Root: filepath.Join(t.TempDir(), c.ID)} + if err := os.MkdirAll(local.Root, 0700); err != nil { + t.Fatal(err) + } + return c, local + } + publish := func(c cell.Cell, body []byte) string { + if err := os.WriteFile(filepath.Join(c.Root, "payload"), body, 0600); err != nil { + t.Fatal(err) + } + sealed, err := sender.Seal(t.Context(), c, cellstore.TurnInfo{}) + if err != nil { + t.Fatal(err) + } + if err := cellstore.Await(t.Context(), c); err != nil { + t.Fatal(err) + } + if err := pub.Publish(t.Context(), &Driving{Cell: c}, sealed.Turn.ID, PublishInfo{Class: "chat"}); err != nil { + t.Fatal(err) + } + return sealed.Turn.ID + } + earlier, localEarlier := makeChat() + first := publish(earlier, []byte("an earlier chat")) + if err := f.Fetch(t.Context(), localEarlier, first); err != nil { + t.Fatal(err) + } + source, local := makeChat() + body := make([]byte, 2<<20) + if _, err := rand.Read(body); err != nil { + t.Fatal(err) + } + head := publish(source, body) + counts.frames.Store(0) + counts.objects.Store(0) + counts.bytes.Store(0) + return &usedStoreRig{sender: sender, pub: pub, receiver: recv, fetch: f, counts: counts, cell: local, source: source, earlier: earlier, earlierHead: first, body: body, head: head} +} +func TestANewChatInAUsedStorePrimesFromItsPlan(t *testing.T) { + r := newUsedStoreRig(t) + // Skipping the plan was the old used-store path. Fetch the very same source + // through that object-only door in another already-used store for comparison. + local := cellstore.Engine{Binary: r.sender.Binary, DataRoot: t.TempDir(), Transport: cellstore.Spawn{Binary: r.sender.Binary}} + engine := local.Sync(r.receiver.Keys, r.receiver.Ledger) + before := &chatFetchCounts{Store: r.counts.Store} + plain := &Fetcher{Engine: engine, Store: before, Inbox: engine.Inbox} + if err := plain.Fetch(t.Context(), cell.Cell{ID: r.earlier.ID, Root: t.TempDir()}, r.earlierHead); err != nil { + t.Fatal(err) + } + before.frames.Store(0) + before.objects.Store(0) + before.bytes.Store(0) + plainCell := cell.Cell{ID: r.cell.ID, Root: t.TempDir()} + if err := plain.Fetch(t.Context(), plainCell, r.head); err != nil { + t.Fatal(err) + } + original, err := os.ReadFile(filepath.Join(plainCell.Root, "payload")) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(original, r.body) || before.frames.Load() != 0 || before.objects.Load() == 0 { + t.Fatal("object-only baseline did not complete the same bytes") + } + t.Logf("object-only cold baseline in used store: frame GETs=%d object POSTs=%d bytes=%d", before.frames.Load(), before.objects.Load(), before.bytes.Load()) + counted := &wantCountingEngine{Engine: r.receiver} + r.fetch.Engine = counted + if err := r.fetch.Fetch(t.Context(), r.cell, r.head); err != nil { + t.Fatal(err) + } + t.Logf("cold chat in used store: frame GETs=%d object POSTs=%d bytes=%d", r.counts.frames.Load(), r.counts.objects.Load(), r.counts.bytes.Load()) + if r.counts.frames.Load() == 0 || r.counts.objects.Load() != 0 { + t.Fatal("new chat skipped its frame plan") + } + if counted.wants.Load() < 2 { + t.Fatal("priming skipped the completeness loop") + } + got, err := os.ReadFile(filepath.Join(r.cell.Root, "payload")) + if err != nil || !bytes.Equal(got, r.body) || !bytes.Equal(got, original) { + t.Fatal("received different bytes", err) + } +} +func TestAWarmChatStillFetchesOnlyItsDelta(t *testing.T) { + r := newUsedStoreRig(t) + if err := r.fetch.Fetch(t.Context(), r.cell, r.head); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(r.source.Root, "small"), []byte("one file changed"), 0600); err != nil { + t.Fatal(err) + } + sealed, err := r.sender.Seal(t.Context(), r.source, cellstore.TurnInfo{}) + if err != nil { + t.Fatal(err) + } + if err := cellstore.Await(t.Context(), r.source); err != nil { + t.Fatal(err) + } + // Reuse the actual published head/ownership, just as the existing writer does. + view, err := r.pub.Dir.Cell(t.Context(), r.source.ID) + if err != nil { + t.Fatal(err) + } + drv := &Driving{Cell: r.source, Fence: view.Cell.Lease.Fence, Head: view.Cell.Head, Frames: view.Cell.Frames} + if err := r.pub.Publish(t.Context(), drv, sealed.Turn.ID, PublishInfo{Class: "chat"}); err != nil { + t.Fatal(err) + } + r.counts.frames.Store(0) + r.counts.objects.Store(0) + r.counts.bytes.Store(0) + if err := r.fetch.Fetch(t.Context(), r.cell, sealed.Turn.ID); err != nil { + t.Fatal(err) + } + t.Logf("warm delta: frame GETs=%d object POSTs=%d bytes=%d", r.counts.frames.Load(), r.counts.objects.Load(), r.counts.bytes.Load()) + if r.counts.frames.Load() != 0 || r.counts.objects.Load() == 0 || r.counts.bytes.Load() >= int64(len(r.body)) { + t.Fatal("warm chat redownloaded its history") + } + got, err := os.ReadFile(filepath.Join(r.cell.Root, "payload")) + if err != nil || !bytes.Equal(got, r.body) { + t.Fatal("warm history changed", err) + } + small, err := os.ReadFile(filepath.Join(r.cell.Root, "small")) + if err != nil || string(small) != "one file changed" { + t.Fatal("delta missing", err) + } +} diff --git a/internal/cellsync/engine.go b/internal/cellsync/engine.go index c4289a14f5..538555219f 100644 --- a/internal/cellsync/engine.go +++ b/internal/cellsync/engine.go @@ -33,9 +33,9 @@ type Engine interface { // reach so far, and leaves every other file, which may be waiting for a // frame that has not landed. A take calls it while frames still download. ImportPartial(ctx context.Context, c cell.Cell, head, inbox string) (int, error) - // Holds reports whether this device already holds objects of c that the - // relay holds too: its ledger for this relay is not empty. A device that - // does has most of the chat's history and wants only a delta (contract + // Holds reports whether this device has already held this chat's history. + // Activity for another chat in a shared store says nothing about this one. + // A warm chat wants only its delta (contract // §22.5), so priming it with the chat's whole frame plan would download // what it has. Holds(c cell.Cell) bool diff --git a/internal/cellsync/fetch.go b/internal/cellsync/fetch.go index d7b3a62b7c..f8ca8efd8a 100644 --- a/internal/cellsync/fetch.go +++ b/internal/cellsync/fetch.go @@ -137,8 +137,7 @@ func (f *Fetcher) prime(ctx context.Context, c cell.Cell, head, inbox string) er } inbox = owned defer os.RemoveAll(inbox) - // A device that already exchanged objects with this relay holds most of the - // chat: the plan names its whole upload history, so priming it would + // A device that already held this chat has most of its history: the plan names its whole upload history, so priming it would // download what it has to learn about a one-file change. The want loop // fetches a warm take's delta by rid. if f.Engine.Holds(c) { diff --git a/internal/handoff/handoff.go b/internal/handoff/handoff.go index 70a025f95d..ee49ba6701 100644 --- a/internal/handoff/handoff.go +++ b/internal/handoff/handoff.go @@ -107,7 +107,9 @@ func (t Taker) Take(ctx context.Context, id string) (Taken, error) { // TakeSeen continues the lease the person saw on the card. Older callers // without that reading still bind the claim to the record read before fetch. func (t Taker) TakeSeen(ctx context.Context, id string, seen *directory.LeaseExpectation) (Taken, error) { + directoryRead := Trace(t.OnPhase, "directory") view, err := t.Dir.Cell(ctx, id) + directoryRead() if err != nil { return Taken{}, fmt.Errorf("handoff: read chat %s: %w", id, err) } @@ -266,7 +268,9 @@ func (t Taker) fetchAndAcquire(ctx context.Context, stage cell.Cell, from, head if t.OnPhase != nil { t.OnPhase(Phase{Name: "first acquire", Started: time.Now()}) } + acquiring := Trace(t.OnPhase, "acquire") got, err := t.Dir.Acquire(ctx, stage.ID, how) + acquiring() if err != nil { return "", 0, nil, fmt.Errorf("handoff: take %s: %w", stage.ID, err) } @@ -400,7 +404,10 @@ func swap(stage, root string) error { // here hands it back, so the chat is takeable at once instead of after a TTL. func (t Taker) open(ctx context.Context, c cell.Cell, fence uint64) (cell.Cell, error) { for _, hook := range t.After { - if err := hook(ctx, c); err != nil { + done := Trace(t.OnPhase, "hooks") + err := hook(ctx, c) + done() + if err != nil { return cell.Cell{}, t.giveBack(ctx, c.ID, fence, fmt.Errorf("handoff: %w", err)) } } diff --git a/internal/manual/chat/continuing-a-chat-on-another-computer.md b/internal/manual/chat/continuing-a-chat-on-another-computer.md index 52c1876b1f..0c30632454 100644 --- a/internal/manual/chat/continuing-a-chat-on-another-computer.md +++ b/internal/manual/chat/continuing-a-chat-on-another-computer.md @@ -29,21 +29,17 @@ An older chat's copy is kept in a folder called `.moving` beside the original, i ## My typed message after continue here — is it kept, does it send by itself -Moving chats and pairing are available by default on Linux, macOS and native Windows on every build with a compatible engine. Pair both computers first. +before showing this question for a send, codeaf checks which computer holds the chat now. an older listing alone cannot raise it. if that check fails, nothing is sent and the whole message stays in the box, including pasted text and attachments. the chat says ` has not answered; your message is still in the box, press enter to send it`. press enter to check again. changing the draft while a check waits keeps the current words unsent; the chat says `draft changed before sending; press enter to send your current words`. -Before showing this question for a send, codeaf checks which computer holds the chat now. An older listing alone cannot raise it. If the check cannot reach your other devices and this window already knows the chat moved away, your words stay in the box and nothing is sent. Continuing a chat this computer already holds keeps its open conversation and your typed words. +a chat this window already holds, or one that has never been shared, sends without asking the service. an ordinary check for a shared copy waits up to five seconds. immediately after this window's own successful move, a message can wait up to 30 seconds for the chat to finish opening, then sends once. a window that lost the chat, or another window holding it, cannot give this window permission to send. -The first Enter waits up to five seconds for a current reading when this window -does not hold the shared chat, including in a freshly opened copy. -`other machines unreachable` means that reading -failed; it never means a reading has not happened yet. Your words stay unsent -while the reading is in progress. +if Enter raised the continue card, a successful move restores those same words, pasted text and attachments and says `your message is still in the box; press enter to send it`. it waits for another enter. choosing `leave it there` or a failed move also keeps the whole draft. -After `continue here` succeeds, those same words, pasted text and attachments are in the arrived chat's box. Press enter again when you want to send them. Choosing `leave it there` or a move that fails also keeps the whole draft. +## which draft comes back after continuing a chat -The move opens one conversation for that chat. Once you send those words, they are cleared from its saved drafts, including matching copies left by an earlier window. Taking the chat back, reopening it or restarting codeaf then shows an empty box unless you typed a new draft. +a move opens one conversation for that chat. sending clears the words from its saved drafts, including matching copies left by an earlier window. taking the chat back, reopening it or restarting codeaf then shows an empty box unless you typed a new draft. -Taking a chat from Home restores that chat's own saved draft, or leaves its box empty. Words typed for a different chat stay with that chat, even when both work in one place on disk. A draft held behind a send's continuation card stays with the chat that raised it. +taking a chat from Home restores that chat's own saved draft, or leaves its box empty. words typed for a different chat stay with that chat, even when both work in one place on disk. a draft held behind a send's continuation card stays with the chat that raised it. continuing a chat this window already holds keeps its open conversation and typed words. ## Read a finished task's worker conversation after moving its chat @@ -59,9 +55,15 @@ On `Continue this chat here?`, `leave it there` closes the question without movi If Enter in a displaced chat raises the continue card, `esc` or `2 leave it there` returns to that same chat window with its full draft still in the box. If you opened the card from a Home row, leaving the card keeps you on Home. +during an automatic continue, the first `esc` withdraws its opening and any waiting choice. the automatic chat is not opened: it finishes arriving and is let go, so it stays on this computer, free to continue from anywhere. a further `esc` navigates as usual while that finishes. if the abandoned move has not answered after two minutes, its request is cancelled before the waiting choice can begin. inspecting another chat's card and choosing `leave it there` or `esc` leaves the automatic arrival alone. + A continue card previews only that chat's unsent words, or nothing when no draft is available. Its project foot belongs to the selected chat. Words typed in another chat never appear on this card. Saved drafts belong to their chat even when a copied chat moves to a different project folder; opening another copy cannot adopt them. -Home says `other machines did not answer in time` when its ownership listing reaches a deadline. Your words stay unsent. This sentence does not establish a network failure. +## slow chat listing — waiting for a service, a timeout or an unreachable host + +at five seconds without a chat-list answer, Home says `waiting for ; still checking` and keeps its rows. the read continues for up to 25 seconds. a deadline says ` did not answer in 25s; still trying`; a failed connection says ` is unreachable; still trying`. background checks continue on their usual schedule. a successful answer clears the line. + +these are Home's listing sentences. a message left unsent by a failed send check gets its own explanation: ` has not answered; your message is still in the box, press enter to send it`. ## Why does continue here say this chat was last held here — my only computer asks to continue @@ -126,7 +128,7 @@ not committed yet: main.go, util.go · last tests: passed got it ``` -## What was not brought along — the lines on the arrival card +## What was not brought along — folders, commands and tests on the arrival card When a chat arrives on a paired computer, its arrival card can include these lines: @@ -188,23 +190,31 @@ When unsent turns were kept as a separate copy, reopening that copy checks who h Moving chats and pairing are available by default on Linux, macOS and native Windows on every build with a compatible engine. Pair both computers first. -While a chat is moving here, the card says `continuing here…`. Pressing `continue here` again before it finishes says `a chat is already continuing here; wait for it to open` on the card instead of doing nothing. Every failure is shown where you are looking — on Home's card with `1 retry` and `2 leave it there`, or inside the open chat window — never only on Home's bottom line after the card closes: +while a chat you explicitly chose is moving here, the card says `continuing here…`. another explicit continue during that move says `a chat is already continuing here; wait for it to open` on the card. an automatic startup move instead lets your explicit choice go next; see “my choice while an automatic chat is arriving”. + +every failure stays where you are looking: on Home's card with `1 retry` and `2 leave it there`, or inside the open chat window. a fresh reading decides whether a different computer continued first. if the same computer picked the chat up again, the card asks again with ` picked this chat up again; press enter to continue it here`. it does not blame another device. + +## why continue here failed — retry or leave it there + +a failed move keeps the draft and shows `1 retry` and `2 leave it there`. retry confirms the holder and turn of ownership now displayed on that card. + - `another device continued this chat first`: another device was faster. The row goes back to `running on `. Nothing changed. - `other machines unreachable`: this device cannot reach the service. Nothing can be moved until it can. - `could not continue this chat here because the engine store is busy` or `could not continue this chat here because the engine store is busy (held by codeaf chat, pid 4321)`: another `codeaf` process holds this computer's engine store writer. Close or finish that process, then choose `retry`. - `could not continue this chat here`: the move failed. The row stays where it was; choose `retry` or try again. +- A move does not fail because another chat on this computer is saving its secrets to the sync service at that moment. Its last step waits up to 30 seconds for that save to finish, then arrives. - `could not continue this chat here because other machines are unreachable; your words are still in the box`: the service went away during the move. Nothing was sent and the chat stays where it was; send again once the other machines are reachable. - `taking an open chat from another computer waits for a sync service update; all else works, so close it there and continue here` means the chat is open on another computer and the sync service is too old to take it from there safely. Everything else works, including continuing a chat the other computer has closed. Close the chat on that computer, then choose `continue here` again. - Under `codeaf chat --host`, `continue here` is not offered and a chat the remote host already holds never shows as held elsewhere. With no way to continue set up, an off row has no `continue here`: enter says why. -## Taking a chat back, and two computers racing for the same chat — who wins, and what the loser sees +## taking a chat back, and two computers continuing the same chat — which one opens Moving chats and pairing are available by default on Linux, macOS and native Windows on every build with a compatible engine. Pair both computers first. A chat has one holder at a time. To take a chat back from the computer that took it, continue it there the same way: press enter on its row on that computer and choose `continue here`. Nothing has to be returned or switched off first. A computer with the chat open when another takes it says ` continued this chat; this window now only shows it`, stops taking new turns, and keeps what it had not yet sent as a branch (` turns from : open separately / discard`). A tool call the model tries there answers with that same line. Taking the chat back leaves a `node_modules` or other install folder that computer already has alone. -The choice covers the holder and turn of ownership shown on the card. If another computer continues the chat before you confirm or while the copy is arriving, your choice cannot take it from that new holder. The slower one sees `another device continued this chat first`, its local copy is not replaced, and its row goes back to `running on `. No turn is lost on either side: whatever the earlier holder had not sent is kept for you as the branch above. This build offers discard, which archives a branch, and opening the branch as a separate chat; merging it into the original chat is unavailable. Pressing `2` (`leave it there`) on the question, or `esc`, changes nothing and leaves the chat where it is. +The choice covers the holder and turn of ownership shown on the card. If another computer continues the chat before you confirm or while the copy is arriving, your choice cannot take it from that new holder. The slower one sees `another device continued this chat first` on the card with `1 retry` and `2 leave it there`; its local copy is not replaced, and its row goes back to `running on `. if the same holder picked it up again, the refreshed card says ` picked this chat up again; press enter to continue it here` and asks for a new confirmation instead. a complete chat already held here gets one silent recovery attempt; another disagreement keeps the retry card and draft. No turn is lost on either side: whatever the earlier holder had not sent is kept for you as the branch above. This build offers discard, which archives a branch, and opening the branch as a separate chat; merging it into the original chat is unavailable. Pressing `2` (`leave it there`) on the question, or `esc`, changes nothing and leaves the chat where it is. ## This window only shows the chat — can I still send here @@ -307,6 +317,10 @@ If the last upload fails when you quit, for example because the connection dropp Device presence can arrive before its list of chats. If you press `alt+m` early, home says `chats are still arriving from your other computer; this opens when they are ready`. It keeps your request and opens the ordinary confirmation once the chat arrives; it never takes the chat without that confirmation. A recent chat left on another computer is offered when home opens, including one that computer closed cleanly. +## a new chat brings its full saved contents; a returning chat brings its changes + +a chat this computer has never held brings its saved contents in parallel, even after other chats used this store. a returning chat downloads only what changed. either path checks that all saved contents arrived before the move finishes. sharing saved file contents with other chats does not make a new chat count as one this computer already held. + ## Does first start send anything — local identity before pairing The first start may create a local identity for this computer. That file sends nothing: there is no sync traffic or network use until you pair or offer a pairing code. Reading the add-device instructions leaves it quiet. Offering a pairing code or running `codeaf pair` contacts the sync service for that pairing. `CODEAF_SYNC_URL=off` blocks sync traffic even after pairing. @@ -335,9 +349,17 @@ A clean quit lets go after the last save is durable. Opening bare `codeaf` on yo If this computer is also coordinating, a clean terminal quit releases its idle chats and returns coordination to an anchor before stopping. With the anchors reachable, the other computer can continue immediately. Sleep, a crash or a lost connection still use the fallback described on the coordination and workers page. -Once the chat arrives, you can send your next message immediately. The window keeps following changes, including renamed computers and a later move to another computer. +after the chat arrives, enter can wait up to 30 seconds for this window to finish opening the chat before sending once. if the check fails, it says ` has not answered; your message is still in the box, press enter to send it`. the window keeps following changes, including renamed computers and a later move to another computer. + +A running chat on the other computer never moves automatically. Neither does a named chat, `--host`, `--at` or `--session`, a chat with unsaved local edits, or a launch while another local window already has a chat open. if the service cannot be reached, Home names its host and keeps trying. if another computer got there first, the refreshed continue card explains the change and keeps the local chat as it was. + +## my choice while an automatic chat is arriving — continue a different chat, join it or cancel + +bare startup still chooses the most recently released eligible chat and checks local safety first. input before that check answers cancels the automatic choice. + +if its move is already running, confirming a different chat on Home or using `alt+c` chooses what opens next. Home says `continuing next; is still on its way here`, with the titles from the continue cards. the automatic move settles first and is let go with its files and kept work intact; it never opens over your choice. the next move uses exactly the computer and turn of ownership you confirmed. `esc` cancels the waiting choice and the automatic chat still settles and is let go. -A running chat on the other computer never moves automatically. Neither does a named chat, `--host`, `--at` or `--session`, a chat with unsaved local edits, or a launch while another local window already has a chat open. If the service cannot be reached or another device got there first, Home says `other machines unreachable` or `another device continued this chat first` and keeps the local chat as it was. +confirming the automatic chat itself joins its move and opens it once. explicitly opening another local chat or starting a new one also prevents a late automatic arrival from replacing it. moving the cursor, opening Home or inspecting and declining another card does not choose a different chat. an explicit move already in progress still gives the usual busy sentence for another explicit pick. ## Why an older chat could not be copied — symbolic links, home folder diff --git a/internal/manual/chat/home.md b/internal/manual/chat/home.md index 265037a99a..8309efe333 100644 --- a/internal/manual/chat/home.md +++ b/internal/manual/chat/home.md @@ -330,8 +330,7 @@ ten minutes). It asks every few seconds while the list is changing and about onc while it is not. Each row's age is counted from its last saved turn, so it keeps growing between refreshes. -If the other machines cannot be reached, the last list stays on screen and the line -`other machines unreachable` stands under it. With no connection set up at all (sync off, see "Sync your chats between machines" in running-from-the-terminal), nothing +a read still waiting at five seconds keeps the last list and says `waiting for ; still checking`. it continues for up to 25 seconds. a deadline says ` did not answer in 25s; still trying`; a failed connection says ` is unreachable; still trying`. background retries continue, and a successful answer clears the line. With no connection set up at all (sync off, see "Sync your chats between machines" in running-from-the-terminal), nothing extra is drawn and the panel is exactly the local list. `codeaf cell list --all` prints the same rows in a terminal (see "The cell verbs" in running-from-the-terminal). @@ -359,9 +358,8 @@ When continuing a chat from another paired computer: - If you had unsaved edits to that chat here, they are not overwritten. They are kept as a branch and the chat says `your unsaved edits here were kept as turns from `. -- If another machine took an `offline` chat first, the row goes back to `running on ` with one - line saying another device continued it first. Nothing was changed. -- If the other machines cannot be reached, enter says `other machines unreachable` and asks +- If another machine took an `offline` chat first, the row goes back to `running on ` with a card saying `another device continued this chat first`, `1 retry` and `2 leave it there`. if the same computer picked it up again, the card instead says ` picked this chat up again; press enter to continue it here` and asks for fresh confirmation. +- if the service cannot be reached, enter names its host and says it is still trying, and asks nothing: no chat can be taken without them. - With no way to continue set up, an offline row has no `continue here`: enter says its sentence. @@ -525,8 +523,7 @@ them`. Opening separately opens its own chat and workspace copy, with the kept t their files; the main chat stays as it is. **Discard archives** the branch: it leaves this list and is not deleted. **Merge is not offered on this build**. The row says -` turns from : open separately / discard`. If the other -machines cannot be reached, enter says `other machines unreachable` and does nothing. +` turns from : open separately / discard`. if the service cannot be reached, enter names its host and says it is still trying, and does nothing. ## Why do I see the same chat twice on Home — the same chat twice on Home, one row per chat id and collapsed branch copies @@ -2726,6 +2723,12 @@ After a clean quit lets a paired computer's chat go, bare `codeaf` on another pa If the laptop only slept or lost its connection, Home offers its latest chat and `alt+c` continues immediately. When local unsaved edits need to be kept as a branch, the continuation card asks first. Nothing moves automatically while the other computer is running that chat, for a named chat or `--host`/`--at`/`--session`, when another local window holds a chat, or when local unsaved edits need a choice. If the move fails, Home stays open and says why. +## my chosen chat opens while a startup chat is still arriving + +if bare startup is already continuing a released chat, confirming a different Home row or using `alt+c` chooses the chat that opens next. Home says `continuing next; is still on its way here`, using the same chat titles as the cards. the automatic move settles and is let go before your confirmed move begins. its files and kept work remain intact, and it never opens over your selection. `esc` cancels the waiting choice; the automatic chat still settles and is let go. + +confirming the arriving chat itself joins the existing move. opening another local chat or starting a new chat also takes precedence over the automatic arrival. moving the cursor, opening Home or inspecting and declining another card does not choose a different chat. input before the startup safety check answers still cancels that automatic choice. a move you explicitly started retains the busy sentence `a chat is already continuing here; wait for it to open` for another explicit pick. + ## Empty untitled chats from your other devices — rows before the first saved turn An empty new chat on one of your other devices stays off Home until it has a saved turn. Opening a window there does not add an `untitled` row here, even while that window remains open and running. Its startup save alone does not make it a conversation you can continue. Once that chat has its first saved turn, it appears with the other chats you can continue. diff --git a/internal/manual/chat/relay-hosted-or-your-own.md b/internal/manual/chat/relay-hosted-or-your-own.md index 717134661b..56281e7b03 100644 --- a/internal/manual/chat/relay-hosted-or-your-own.md +++ b/internal/manual/chat/relay-hosted-or-your-own.md @@ -42,7 +42,7 @@ codeaf relay --listen :8787 --store /var/lib/codeaf - `--listen` is the address to listen on. The default is `:8787`. - `--store ` is where the relay keeps the directory and the chunks. **Without `--store` the relay only passes pairing and remote-access traffic. It keeps no chats, and sync does not work through it.** The directory is made if it is not there. - `--quiet` turns the log off. `--status=false` turns off `GET /status`. -- `--trust-proxy` makes the pairing limits count the address in `X-Forwarded-For`. Use it only behind a proxy that sets that header, and never on a relay that faces the internet. +- `--trust-proxy` makes registration, dial and pairing limits count the first address in `X-Forwarded-For`, falling back to the socket address when absent or blank. without it, forwarding headers are ignored. a relay behind Railway, a load balancer or nginx must pass this flag or every caller shares the proxy's allowance. use it only behind a proxy that sets that header. Stop it with SIGTERM or ctrl+c: requests under way finish first. **The relay has no TLS of its own.** Put a proxy that ends TLS in front of it if any computer reaches it over a network you do not own. diff --git a/internal/manual/relay_defects_test.go b/internal/manual/relay_defects_test.go new file mode 100644 index 0000000000..7562df1a87 --- /dev/null +++ b/internal/manual/relay_defects_test.go @@ -0,0 +1,47 @@ +package manual + +import ( + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/packed" +) + +func TestRelayDefectAnswersSurvivePacking(t *testing.T) { + archive, err := packed.Pack("chat") + if err != nil { + t.Fatal(err) + } + if err := packed.Verify(archive, "chat"); err != nil { + t.Fatal(err) + } + compressed := newCorpus(packed.New(archive), "chat/*.md") + for _, corpus := range []*Corpus{Chat(), compressed} { + for _, probe := range []struct{ query, page, words string }{ + {"my chosen chat while a startup chat is still arriving", "home", "continuing next; is still on its way here"}, + {"slow chat listing timeout waiting for a service host", "continuing-a-chat-on-another-computer", " did not answer in 25s; still trying"}, + {"my typed message after continue here is kept does it send by itself", "continuing-a-chat-on-another-computer", "your message is still in the box; press enter to send it"}, + {"continue here another device continuing first picked this chat up again", "continuing-a-chat-on-another-computer", " picked this chat up again; press enter to continue it here"}, + {"relay trust proxy registration dial pairing X-Forwarded-For", "relay-hosted-or-your-own", "registration, dial and pairing limits"}, + } { + found := false + for _, section := range corpus.Search(probe.query, DefaultResults) { + if section.Page == probe.page && strings.Contains(section.Body, probe.words) { + found = true + } + } + if !found { + t.Errorf("%q did not reach %s saying %q", probe.query, probe.page, probe.words) + } + } + page, _ := corpus.Page("continuing-a-chat-on-another-computer") + for _, part := range strings.Split(page, "\n## ")[1:] { + heading, _, _ := strings.Cut(part, "\n") + if strings.HasPrefix(heading, "my choice while") || strings.HasPrefix(heading, "My typed message") || strings.HasPrefix(heading, "slow chat listing") || strings.HasPrefix(heading, "Continue here does nothing") || strings.HasPrefix(heading, "why continue here failed") { + if len([]rune(part)) > 2000 { + t.Errorf("section exceeds 2000 characters: %s", heading) + } + } + } + } +} diff --git a/internal/relay/proxy_limits_test.go b/internal/relay/proxy_limits_test.go new file mode 100644 index 0000000000..53b1e809ec --- /dev/null +++ b/internal/relay/proxy_limits_test.go @@ -0,0 +1,43 @@ +package relay + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func TestForwardedAddressesCannotResetUntrustedLimits(t *testing.T) { + for _, socket := range []string{"192.0.2.1:1234", "[2001:db8::1]:1234"} { + for _, door := range []struct { + path string + limit int + }{{EnginePath, RegistrationsPerMinute}, {DialPrefix + "otter-lamp-42", DialsPerMinute}} { + t.Run(socket+door.path, func(t *testing.T) { + server := &Server{Now: func() time.Time { return time.Unix(100000, 0) }} + for i := 0; i <= door.limit; i++ { + req := httptest.NewRequest(http.MethodGet, door.path, nil) + req.RemoteAddr = socket + req.Header.Set("Upgrade", Protocol) + switch i % 5 { + case 0: + case 1: + req.Header.Set("X-Forwarded-For", "") + case 2: + req.Header.Set("X-Forwarded-For", " ") + case 3: + req.Header.Set("X-Forwarded-For", fmt.Sprintf("198.51.100.%d", i)) + case 4: + req.Header.Set("X-Forwarded-For", fmt.Sprintf("198.51.100.%d, 203.0.113.1", i)) + } + out := httptest.NewRecorder() + server.ServeHTTP(out, req) + if (i == door.limit) != (out.Code == http.StatusTooManyRequests) { + t.Fatalf("request %d status=%d, header=%q", i+1, out.Code, req.Header.Get("X-Forwarded-For")) + } + } + }) + } + } +} diff --git a/internal/relay/server.go b/internal/relay/server.go index 002a727604..86f15a7892 100644 --- a/internal/relay/server.go +++ b/internal/relay/server.go @@ -28,6 +28,7 @@ import ( "sync" "time" + "github.com/Agent-Field/codeaf/internal/pairbox" "github.com/coder/websocket" ) @@ -51,6 +52,9 @@ type Ledger struct { // limit it enforces comes from the constants in relay.go, so an operator who // changes one changes it in the one place the sentences quote. type Server struct { + // Peer selects the network counted by registration and dial limits. Nil + // trusts only the socket; forwarding headers require an explicit proxy policy. + Peer func(*http.Request) string // Now is the clock, swapped by tests. Nil is [time.Now]. Now func() time.Time // Note is where a relay operator's log line goes, if they want one. It is @@ -138,7 +142,7 @@ func (s *Server) serveEngine(w http.ResponseWriter, r *http.Request) { http.Error(w, "this door speaks "+Protocol, http.StatusUpgradeRequired) return } - from := callerAddress(r) + from := s.callerAddress(r) if !s.allow(&s.regs, from, RegistrationsPerMinute) { http.Error(w, "too many registrations from here — wait a minute", http.StatusTooManyRequests) return @@ -245,7 +249,7 @@ func (s *Server) serveDial(w http.ResponseWriter, r *http.Request, name string) http.Error(w, "this door speaks "+Protocol, http.StatusUpgradeRequired) return } - if !s.allow(&s.dials, callerAddress(r), DialsPerMinute) { + if !s.allow(&s.dials, s.callerAddress(r), DialsPerMinute) { http.Error(w, "too many connections from here — wait a minute", http.StatusTooManyRequests) return } @@ -389,22 +393,13 @@ func writeUpgraded(conn net.Conn) error { return err } -// callerAddress is who a rate limit is counted against. It reads -// X-Forwarded-For only when it is there, because a relay in production sits -// behind something that terminates TLS, and counting every request against the -// load balancer would rate-limit the world as one caller. -func callerAddress(r *http.Request) string { - if forwarded := r.Header.Get("X-Forwarded-For"); forwarded != "" { - if comma := strings.IndexByte(forwarded, ','); comma >= 0 { - forwarded = forwarded[:comma] - } - return strings.TrimSpace(forwarded) - } - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - return r.RemoteAddr +// callerAddress uses the same peer policy as pairing, so a caller cannot +// mint a fresh allowance by supplying its own forwarding header. +func (s *Server) callerAddress(r *http.Request) string { + if s.Peer != nil { + return s.Peer(r) } - return host + return pairbox.SocketPeer(r) } // bucket is one caller's allowance, refilled by the minute. It is a plain diff --git a/internal/relayserve/main.go b/internal/relayserve/main.go index 94288fbfec..88fc99a4b2 100644 --- a/internal/relayserve/main.go +++ b/internal/relayserve/main.go @@ -20,15 +20,15 @@ import ( // relay --listen :8787 [--store /var/lib/codeaf] [--quiet] [--status=false] [--trust-proxy] // // It runs until SIGINT or SIGTERM and then lets in-flight requests finish. -// In front of it in production goes whatever already terminates TLS; the pipe -// reads X-Forwarded-For when it is there, so its rate limits count the caller. +// In front of it in production goes whatever already terminates TLS; pass --trust-proxy +// so registration, dial and pairing limits count callers behind that proxy. func Main(args []string) error { flags := flag.NewFlagSet("relay", flag.ContinueOnError) listen := flags.String("listen", ":8787", "address to listen on") store := flags.String("store", "", "directory that keeps the directory and blob stores; empty runs the blind pipe only") quiet := flags.Bool("quiet", false, "do not log arrivals, departures and requests") status := flags.Bool("status", true, "answer GET /status with what is connected right now") - trust := flags.Bool("trust-proxy", false, "count the address in X-Forwarded-For as the caller's network in the pairing limits; only for a relay behind a proxy that sets it") + trust := flags.Bool("trust-proxy", false, "count the first X-Forwarded-For address for registration, dial and pairing limits; required behind a proxy that sets it") minGrace := flags.Duration("min-grace", 0, "shortest grace period a rotation may ask for (default 1h); a test hook") sweepEvery := flags.Duration("sweep-every", 0, "how often replaced identities whose grace has ended are deleted (default 1h); a test hook") watchers := flags.Int("max-watchers", 0, "directory watch sockets one identity may hold (default 1000); a test hook") diff --git a/internal/relayserve/proxy_limits_test.go b/internal/relayserve/proxy_limits_test.go new file mode 100644 index 0000000000..59ab49f213 --- /dev/null +++ b/internal/relayserve/proxy_limits_test.go @@ -0,0 +1,88 @@ +package relayserve_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/pairbox" + "github.com/Agent-Field/codeaf/internal/relay" + "github.com/Agent-Field/codeaf/internal/relayserve" +) + +func TestTrustProxyAppliesToEveryRelayDoor(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + for _, store := range []bool{false, true} { + for _, trust := range []bool{false, true} { + for _, door := range []struct { + name, path, method string + limit int + }{ + {"registration", relay.EnginePath, http.MethodGet, relay.RegistrationsPerMinute}, + {"dial", relay.DialPrefix + "otter-lamp-42", http.MethodGet, relay.DialsPerMinute}, + {"mailbox", pairbox.Path, http.MethodPost, pairbox.DefaultLimits.CreatePerHour}, + {"link", directory.LinkPath + "/requests/AAAAAAAA", http.MethodGet, directory.DefaultLinkLimits.MissPerMinute}, + } { + if !store && door.name == "link" { + continue + } + t.Run(fmt.Sprintf("store=%v/trust=%v/%s", store, trust, door.name), func(t *testing.T) { + path := "" + if store { + path = t.TempDir() + } + svc := relayserve.New(relayserve.Config{Store: path, TrustProxy: trust, Now: func() time.Time { return time.Unix(100000, 0) }}) + defer svc.Close() + request := func(header string) int { + req := httptest.NewRequest(door.method, door.path, nil) + req.RemoteAddr = "192.0.2.1:1234" + req.Header.Set("X-Forwarded-For", header) + req.Header.Set("Upgrade", relay.Protocol) + if door.name == "mailbox" { + key, err := pairbox.NewKey() + if err != nil { + t.Fatal(err) + } + req.Header.Set(pairbox.KeyHeader, key.String()) + } + out := httptest.NewRecorder() + svc.Handler.ServeHTTP(out, req) + return out.Code + } + // Changing only the second entry never changes the counted network. + for i := 0; i < door.limit; i++ { + if code := request(fmt.Sprintf(" 198.51.100.1 , 203.0.113.%d", i)); code == http.StatusTooManyRequests { + t.Fatalf("early limit at %d", i) + } + } + if code := request("198.51.100.1, 203.0.113.99"); code != http.StatusTooManyRequests { + t.Fatalf("same first address: %d", code) + } + code := request("198.51.100.2, 198.51.100.1") + if (code != http.StatusTooManyRequests) != trust { + t.Fatalf("new forwarded address: trust=%v status=%d", trust, code) + } + if trust { + // Absent and blank headers fall back to the same socket allowance. + for i := 0; i < door.limit; i++ { + header := "" + if i%2 == 1 { + header = " , 203.0.113.1" + } + if code := request(header); code == http.StatusTooManyRequests { + t.Fatalf("early fallback limit at %d", i) + } + } + if code := request(" "); code != http.StatusTooManyRequests { + t.Fatalf("blank did not share fallback allowance: %d", code) + } + } + }) + } + } + } +} diff --git a/internal/relayserve/service.go b/internal/relayserve/service.go index c675938503..205c040b83 100644 --- a/internal/relayserve/service.go +++ b/internal/relayserve/service.go @@ -32,7 +32,7 @@ type Config struct { Logf func(string, ...any) Note func(name, what string) // the pipe's arrival and departure line; nil says nothing - // TrustProxy makes the pairing mailbox's per-network limits count the + // TrustProxy makes registration, dial and pairing limits count the // address in X-Forwarded-For. Only a relay behind a proxy that sets it may // say so; anywhere else a caller could name any network it liked. TrustProxy bool @@ -108,7 +108,7 @@ func New(cfg Config) *Service { if now == nil { now = time.Now } - mux := pipeMux(&relay.Server{Note: cfg.Note}, cfg.Status) + mux := pipeMux(&relay.Server{Note: cfg.Note, Peer: peerOf(cfg)}, cfg.Status) mountPairing(mux, cfg, now) mountCluster(mux, cfg.Cluster) mux.HandleFunc("/p/", servePairPage) @@ -140,9 +140,9 @@ func New(cfg Config) *Service { return svc } -// peerOf names the network of a link-pairing request: the proxy's word when -// the relay sits behind one, the socket otherwise. -func peerOf(cfg Config) directory.PeerFunc { +// peerOf keeps registrations, dials and both pairing paths on one explicit +// proxy policy. An untrusted forwarding header can never select an allowance. +func peerOf(cfg Config) func(*http.Request) string { if cfg.TrustProxy { return pairbox.ForwardedPeer } @@ -165,10 +165,7 @@ func mountPairing(mux *http.ServeMux, cfg Config, now func() time.Time) { if limits == (pairbox.Limits{}) { limits = pairbox.DefaultLimits } - peer := pairbox.SocketPeer - if cfg.TrustProxy { - peer = pairbox.ForwardedPeer - } + peer := peerOf(cfg) h := logged(cfg.Logf, pairbox.Handler(pairbox.NewMemory(limits, now), peer)) mux.Handle(pairbox.Path, h) mux.Handle(pairbox.Path+"/", h) diff --git a/internal/remote/client.go b/internal/remote/client.go index 5c6466aeb9..811c6bdd13 100644 --- a/internal/remote/client.go +++ b/internal/remote/client.go @@ -2686,3 +2686,7 @@ func (c *Client) DeleteTask(file, id string) error { _, err := c.call(nil, MethodPlacesTaskDelete, TaskDeleteArgs{File: file, ID: id}) return err } + +// WriterChanges lets an off-loop send wait for the writer's next pushed state. +// It uses existing facts and adds neither a request nor a wire field. +func (a *Agent) WriterChanges() <-chan struct{} { return a.c.facts.changes() } diff --git a/internal/remote/replica.go b/internal/remote/replica.go index 9f84df4f62..d290ae67bf 100644 --- a/internal/remote/replica.go +++ b/internal/remote/replica.go @@ -38,8 +38,9 @@ type replica struct { // rather than applied, which is the whole of the ordering law: two facts // that move in the same instant race to the writer on the engine, and a late // push applied over a newer one would make a live row go backwards. - rev uint64 - facts session.Facts + rev uint64 + facts session.Facts + changed chan struct{} } // fill takes the set a welcome arrived with, REPLACING whatever was held. @@ -60,6 +61,10 @@ func (r *replica) fill(push *FactsPush) { r.mu.Lock() defer r.mu.Unlock() r.rev, r.facts = push.Rev, push.Facts + if r.changed != nil { + close(r.changed) + r.changed = nil + } } // take applies one "facts" frame, newest wins and nothing else lands. @@ -95,6 +100,10 @@ func (r *replica) takePush(push FactsPush) bool { return false } r.rev, r.facts = push.Rev, push.Facts + if r.changed != nil { + close(r.changed) + r.changed = nil + } return true } @@ -246,3 +255,14 @@ func (r *replica) setLevel(model, level string) { // because a caller that asked four getters would be reading four independent // photographs of a set that is published whole. func (c *Client) Facts() session.Facts { return c.facts.read() } + +// changes broadcasts the next pushed fact set without consuming another reader's +// notification. Registering before reading prevents missing writer installation. +func (r *replica) changes() <-chan struct{} { + r.mu.Lock() + defer r.mu.Unlock() + if r.changed == nil { + r.changed = make(chan struct{}) + } + return r.changed +} diff --git a/internal/remote/writer_changes_test.go b/internal/remote/writer_changes_test.go new file mode 100644 index 0000000000..beab2df716 --- /dev/null +++ b/internal/remote/writer_changes_test.go @@ -0,0 +1,41 @@ +package remote + +import ( + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/session" +) + +func TestWriterInstallationWakesEveryWaitingSend(t *testing.T) { + agent := &Agent{c: &Client{}} + first, second := agent.WriterChanges(), agent.WriterChanges() + // Existing facts are the only carrier; subscribing emits no request. + if !agent.c.facts.takePush(FactsPush{Rev: 1, Facts: session.Facts{Renewal: &session.RenewalFact{Period: time.Second, Held: true}}}) { + t.Fatal("facts refused") + } + for _, changed := range []<-chan struct{}{first, second} { + select { + case <-changed: + default: + t.Fatal("writer change did not broadcast") + } + } + next := agent.WriterChanges() + select { + case <-next: + t.Fatal("new subscription was already spent") + default: + } + if _, period, held := agent.RenewalStatus(); period != time.Second || !held { + t.Fatal("pushed writer authority did not arrive") + } + if agent.c.facts.takePush(FactsPush{Rev: 1}) { + t.Fatal("old facts landed") + } + select { + case <-next: + t.Fatal("old facts woke a new send") + default: + } +} diff --git a/internal/syncsetup/continue_reopened_writer_test.go b/internal/syncsetup/continue_reopened_writer_test.go new file mode 100644 index 0000000000..b504bb0a68 --- /dev/null +++ b/internal/syncsetup/continue_reopened_writer_test.go @@ -0,0 +1,48 @@ +package syncsetup + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/directory" +) + +func TestAReturnAfterTheWriterReopensIsNotALostRace(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + h := newTwoHomes(t) + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + a := h.openA() + a.mustSay("before moving") + h.durable(h.cell.ID, h.cell) + mustNil(t, a.drive.Close(ctx)) + moved, err := h.continuerB().Take(ctx, h.cell.ID) + mustNil(t, err) + stale := &directory.LeaseExpectation{Device: h.b.Device.ID(), Fence: moved.Taken.Driving.Fence} + h.openOn(h.b, h.engB, moved.Taken.Cell, workspaceOf(moved.Taken.Cell.Root), nameB) + current, err := h.a.Dir.Cell(ctx, h.cell.ID) + mustNil(t, err) + if current.Cell.Lease.Device != stale.Device || current.Cell.Lease.Fence <= stale.Fence { + t.Fatalf("writer did not reopen: %+v", current.Cell.Lease) + } + _, err = h.continuerA().TakeSeen(ctx, h.cell.ID, stale) + t.Logf("card holder=%s ownership=%d; reopened holder=%s ownership=%d; stale confirmation=%v", stale.Device, stale.Fence, current.Cell.Lease.Device, current.Cell.Lease.Fence, err) + if !errors.Is(err, directory.ErrLeaseHeld) { + t.Fatalf("stale consent unexpectedly succeeded: %v", err) + } + // The same holder remains; the surface must ask again for this new ownership. + fresh, err := h.a.Dir.Cell(ctx, h.cell.ID) + mustNil(t, err) + if fresh.Cell.Lease != current.Cell.Lease { + t.Fatal("stale consent displaced the writer") + } + consent := &directory.LeaseExpectation{Device: fresh.Cell.Lease.Device, Fence: fresh.Cell.Lease.Fence} + got, err := h.continuerA().TakeSeen(ctx, h.cell.ID, consent) + mustNil(t, err) + if got.Taken.Driving.Fence <= consent.Fence { + t.Fatal("fresh consent did not continue") + } +} diff --git a/internal/syncsetup/continue_timing_test.go b/internal/syncsetup/continue_timing_test.go new file mode 100644 index 0000000000..a882a855fc --- /dev/null +++ b/internal/syncsetup/continue_timing_test.go @@ -0,0 +1,63 @@ +package syncsetup + +import ( + "log" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/handoff" +) + +func TestAContinueLogsWhereItsTimeWent(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + h := newTwoHomes(t) + a := h.openA() + a.mustSay("before the measured move") + h.durable(h.cell.ID, h.cell) + path := filepath.Join(t.TempDir(), "chat.log") + file, err := os.Create(path) + mustNil(t, err) + before := log.Writer() + log.SetOutput(file) + defer func() { log.SetOutput(before); _ = file.Close() }() + c := h.continuerB() + phases := map[string]bool{} + c.opt.OnPhase = func(p handoff.Phase) { + if !p.Ended.IsZero() { + phases[p.Name] = true + } + } + _, err = c.Take(t.Context(), h.cell.ID) + mustNil(t, err) + _, err = h.continuerA().TakeSeen(t.Context(), h.cell.ID, &directory.LeaseExpectation{Device: "old-device", Fence: 1}) + if err == nil { + t.Fatal("expected a failed continue") + } + raw, err := os.ReadFile(path) + mustNil(t, err) + var lines []string + for _, line := range strings.Split(string(raw), "\n") { + if strings.Contains(line, "continue "+h.cell.ID+" phases ") { + lines = append(lines, line) + } + } + if len(lines) != 2 { + t.Fatalf("wanted one line per attempt, got %v", lines) + } + for _, line := range lines { + for _, name := range []string{"capture=", "directory=", "retirement=", "fetch=", "acquire=", "restore=", "hooks=", "filing=", "total="} { + if !strings.Contains(line, name) { + t.Errorf("missing %s in %s", name, line) + } + } + } + for _, name := range []string{"local capture", "directory", "retirement", "fetch", "acquire", "restore", "hooks", "filing"} { + if !phases[name] { + t.Errorf("observer missed %s", name) + } + } +} diff --git a/internal/syncsetup/staged_warmth_test.go b/internal/syncsetup/staged_warmth_test.go new file mode 100644 index 0000000000..3d8e4399cb --- /dev/null +++ b/internal/syncsetup/staged_warmth_test.go @@ -0,0 +1,26 @@ +package syncsetup + +import ( + "os" + "path/filepath" + "testing" + + "github.com/Agent-Field/codeaf/internal/cell" + "github.com/Agent-Field/codeaf/internal/cellstore" +) + +func TestAStagedWarmChatKeepsItsHistory(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".cell"), 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, ".cell", "adopted.json"), []byte(`{"head":"local-head"}`), 0600); err != nil { + t.Fatal(err) + } + l := landing{Engine: cellstore.SyncEngine{}, rootFor: func(string) string { return root }} + if !l.Holds(cell.Cell{ID: "warm", Root: t.TempDir()}) { + t.Fatal("staging hid the chat's existing history") + } +} diff --git a/internal/syncsetup/take.go b/internal/syncsetup/take.go index b0d9bbe6ab..444879a647 100644 --- a/internal/syncsetup/take.go +++ b/internal/syncsetup/take.go @@ -4,6 +4,7 @@ import ( "context" "errors" "github.com/Agent-Field/codeaf/internal/fsreplace" + "log" "os" "path/filepath" "strings" @@ -28,6 +29,7 @@ import ( "github.com/Agent-Field/codeaf/internal/taskcopy" "github.com/Agent-Field/codeaf/internal/teams" "github.com/Agent-Field/codeaf/internal/vaultsync" + "github.com/Agent-Field/codeaf/internal/wireauth" ) // TakeOptions is what the surface tells the take side about this machine. @@ -99,7 +101,7 @@ func (s *Sync) Continuer(eng cellstore.Engine, opt TakeOptions) *Continuer { func (c *Continuer) taker(ctx context.Context, sc *Scope) handoff.Taker { s := c.sync sync := c.eng.Sync(cellstore.SyncKeys{CellKey: s.Identity.CellKey(), Dedup: s.Identity.DedupSecret()}, s.Ledger) - engine := landing{Engine: sync, workspace: c.eng.WorkspaceOf} + engine := landing{Engine: sync, workspace: c.eng.WorkspaceOf, rootFor: c.opt.RootFor} pub := &cellsync.Publisher{Engine: engine, Store: sc.Store, Dir: s.Dir} // THE TAKEOVER'S BRANCHER RUNS WITH Map NIL. The chat being taken keeps its // id on this machine; only the edits set aside live in the branch, and a @@ -151,7 +153,30 @@ func (c *Continuer) OpenBranch(ctx context.Context, id string) (Continued, error return c.take(ctx, id, nil, true) } -func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseExpectation, branchOnly bool) (Continued, error) { +func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseExpectation, branchOnly bool) (out Continued, err error) { + // Measure one attempt on its own copy: observers must not leak into another + // continue, and diagnostics must survive both early refusal and late failure. + started := time.Now() + phases := map[string]time.Duration{} + observed := *c + report := c.opt.OnPhase + observed.opt.OnPhase = func(p handoff.Phase) { + if !p.Ended.IsZero() { + phases[p.Name] += p.Ended.Sub(p.Started) + } + if report != nil { + report(p) + } + } + c = &observed + defer func() { + outcome := "done" + if err != nil { + outcome = "failed" + } + log.Printf("continue %s phases capture=%s directory=%s retirement=%s fetch=%s acquire=%s restore=%s hooks=%s filing=%s unpublished=%s dirty=%s kept_upload=%s admission=%s total=%s outcome=%s", id, + phases["local capture"], phases["directory"], phases["retirement"], phases["fetch"], phases["acquire"], phases["restore"], phases["hooks"], phases["filing"], phases["unpublished check"], phases["dirty check"], phases["kept-branch upload"], phases["staging/preflight"], time.Since(started), outcome) + }() root := c.opt.RootFor(id) defer handoff.Arrive(root)() localCapture := handoff.Trace(c.opt.OnPhase, "local capture") @@ -160,7 +185,9 @@ func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseEx return Continued{}, err } localCapture() + directoryRead := handoff.Trace(c.opt.OnPhase, "directory") view, err := c.sync.Dir.Cell(ctx, id) + directoryRead() if err != nil { return Continued{}, err } @@ -197,7 +224,9 @@ func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseEx if seen == nil { seen = &directory.LeaseExpectation{Device: lease.Device, Fence: lease.Fence} } + directoryRead = handoff.Trace(c.opt.OnPhase, "directory") from := c.sync.deviceName(lease.Device) + directoryRead() if lease.Device == c.sync.Device.ID() { from = "" } @@ -205,7 +234,10 @@ func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseEx c.retireHolder(ctx, id) retired() sc := c.sync.scope(id) - defer sc.Settle() + defer func() { + defer handoff.Trace(c.opt.OnPhase, "filing")() + sc.Settle() + }() intendedPrimary := session.Place{Dir: root, Owned: true}.Work() if project, here := projectOf(root); here { intendedPrimary = project @@ -265,6 +297,7 @@ func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseEx if err != nil { return Continued{}, err } + defer handoff.Trace(c.opt.OnPhase, "filing")() place := session.Place{Dir: taken.Cell.Root, Workspace: intendedPrimary} workspacePlan, err := session.TaskWorkspaceTransferPlan(place) if err != nil { @@ -320,7 +353,7 @@ func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseEx } } } - out := Continued{Taken: taken, Device: c.opt.DeviceName, TaskCopies: taskCopies, Resume: c.arrive(taken.Cell, from)} + out = Continued{Taken: taken, Device: c.opt.DeviceName, TaskCopies: taskCopies, Resume: c.arrive(taken.Cell, from)} if taken.Kept != "" { out.KeptTurns = c.orphanTurns(ctx, taken.Kept) } @@ -441,6 +474,16 @@ func (c *Continuer) branch(b cellsync.Brancher) func(context.Context, *cellsync. type landing struct { cellsync.Engine workspace func(cell.Cell) string + rootFor func(string) string +} + +// Holds consults the existing chat rather than its fresh staging folder, so a +// return still fetches only its delta before installing the replacement. +func (l landing) Holds(c cell.Cell) bool { + if l.rootFor != nil { + c.Root = l.rootFor(c.ID) + } + return l.Engine.Holds(c) } func (l landing) Want(ctx context.Context, c cell.Cell, head string) ([]string, error) { @@ -624,7 +667,7 @@ func (c *Continuer) pullVault(ctx context.Context, at cell.Cell) error { if err != nil { return err } - return c.sync.withVault(func(v vaultsync.Syncer) error { return v.Pull(ctx) }, files) + return c.sync.withVaultWithin(ctx, vaultBusyWait, func(v vaultsync.Syncer) error { return v.Pull(ctx) }, files) } // files is the withheld files of c as a carrier: the workspace the engine seals @@ -651,6 +694,25 @@ func (s *Sync) files(eng cellstore.Engine, c cell.Cell) (vaultsync.Files, error) // under the vault scope, so vaultsync stays unaware of counting, and what the // run moved is written when it ends. func (s *Sync) withVault(use func(vaultsync.Syncer) error, extra ...vaultsync.Carrier) error { + return s.withVaultWithin(context.Background(), 0, use, extra...) +} + +// A CONTINUE WAITS FOR THE VAULT; A BACKGROUND EXCHANGE DOES NOT. Every running +// chat's drive pushes the vault under state-sync.lock, and over a slow relay it +// holds the lock for seconds. A background exchange that finds it busy may skip, +// because the holder is already carrying the same state; a continue's pull may +// not, because it is the last step of a move the person asked for, and failing +// it handed the chat back with "could not be put in this folder" (D4 in the +// two-network test). So the continue waits up to vaultBusyWait, then fails as +// before. +const ( + vaultBusyWait = 30 * time.Second + vaultBusyRetry = 100 * time.Millisecond +) + +// withVaultWithin is withVault for a caller that waits up to within for another +// codeaf on this computer to finish its own exchange; zero never waits. +func (s *Sync) withVaultWithin(ctx context.Context, within time.Duration, use func(vaultsync.Syncer) error, extra ...vaultsync.Carrier) error { stateMu.Lock() defer stateMu.Unlock() // Refuse before opening the lock file as well as before creating keys, so @@ -663,7 +725,7 @@ func (s *Sync) withVault(use func(vaultsync.Syncer) error, extra ...vaultsync.Ca return err } defer lock.Close() - if err := filelock.Lock(lock, true, true); err != nil { + if err := lockState(ctx, lock, within); err != nil { return err } @@ -677,6 +739,29 @@ func (s *Sync) withVault(use func(vaultsync.Syncer) error, extra ...vaultsync.Ca Carry: append(s.carried(), extra...)}) } +// lockState takes the vault's lock without blocking, and while another process +// holds it retries every vaultBusyRetry until within has passed or ctx ends. The +// answer after the bound is the busy refusal itself, so a caller that gives up +// says what it always said. +func lockState(ctx context.Context, lock *os.File, within time.Duration) error { + err := filelock.Lock(lock, true, true) + if within <= 0 || !filelock.IsBusy(err) { + return err + } + deadline := time.Now().Add(within) + tick := time.NewTicker(vaultBusyRetry) + defer tick.Stop() + for filelock.IsBusy(err) && time.Now().Before(deadline) { + select { + case <-ctx.Done(): + return errors.Join(err, ctx.Err()) + case <-tick.C: + } + err = filelock.Lock(lock, true, true) + } + return err +} + // profile is the directory the product reads config.json and credentials.json // from, the same one its own stores resolve. func (s *Sync) profile() string { @@ -755,3 +840,32 @@ func (s *Sync) Driving(ctx context.Context, c cell.Cell) (*cellsync.Driving, err } return d, nil } + +// ReleaseUnused gives up only the unused acquisition returned to this window. +// The directory checks the fence, so a later writer's ownership is never released. +func (c *Continuer) ReleaseUnused(ctx context.Context, id string, fence uint64) error { + // Match the close path's bounded retry: a transient refusal must not strand + // an otherwise successful automatic acquisition without an open conversation. + for { + if err := ctx.Err(); err != nil { + return err + } + err := c.sync.Dir.Release(ctx, id, fence) + if errors.Is(err, directory.ErrFenceStale) { + return nil + } + if !errors.Is(err, directory.ErrUnreachable) && !errors.Is(err, wireauth.ErrUnavailable) { + return err + } + if _, bounded := ctx.Deadline(); !bounded { + return err + } + timer := time.NewTimer(cellsync.ReleaseRetryEvery) + select { + case <-ctx.Done(): + timer.Stop() + return ctx.Err() + case <-timer.C: + } + } +} diff --git a/internal/syncsetup/unused_take_test.go b/internal/syncsetup/unused_take_test.go new file mode 100644 index 0000000000..81fe5b9a58 --- /dev/null +++ b/internal/syncsetup/unused_take_test.go @@ -0,0 +1,50 @@ +package syncsetup + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/directory/directorytest" +) + +func TestAnUnusedTakeReleasesOnlyItsOwnHoldAndKeepsItsFiles(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + clock := directorytest.NewFakeClock() + dir := directory.NewMemory(clock.Now) + created, err := dir.For("source").Create(t.Context(), "chat", directory.CellInit{Head: "saved", Class: "chat"}) + mustNil(t, err) + receiver := dir.For("receiver") + taken, err := receiver.Acquire(t.Context(), "chat", directory.AcquireOpts{Force: true, RequireConditional: true, Expect: &directory.LeaseExpectation{Device: "source", Fence: created.Cell.Lease.Fence}}) + mustNil(t, err) + root := t.TempDir() + kept := filepath.Join(root, "kept-work") + mustNil(t, os.WriteFile(kept, []byte("all the files stay"), 0600)) + c := &Continuer{sync: &Sync{Dir: receiver}} + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + mustNil(t, c.ReleaseUnused(ctx, "chat", taken.Cell.Lease.Fence)) + released, err := receiver.Cell(t.Context(), "chat") + mustNil(t, err) + if released.Cell.Lease.Expires != 0 { + t.Fatal("unused acquisition stayed held") + } + later, err := receiver.Acquire(t.Context(), "chat", directory.AcquireOpts{RequireConditional: true, Expect: &directory.LeaseExpectation{Device: "receiver", Fence: taken.Cell.Lease.Fence}}) + mustNil(t, err) + mustNil(t, c.ReleaseUnused(ctx, "chat", taken.Cell.Lease.Fence)) + current, err := receiver.Cell(t.Context(), "chat") + mustNil(t, err) + if current.Cell.Lease != later.Cell.Lease { + t.Fatal("old cleanup released a later writer") + } + raw, err := os.ReadFile(kept) + mustNil(t, err) + if string(raw) != "all the files stay" { + t.Fatal("cleanup changed kept work") + } + mustNil(t, c.ReleaseUnused(ctx, "chat", later.Cell.Lease.Fence)) +} diff --git a/internal/syncsetup/vault_wait_test.go b/internal/syncsetup/vault_wait_test.go new file mode 100644 index 0000000000..c69cfe1df5 --- /dev/null +++ b/internal/syncsetup/vault_wait_test.go @@ -0,0 +1,95 @@ +package syncsetup + +import ( + "context" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/filelock" + "github.com/Agent-Field/codeaf/internal/handoff" +) + +// TestAContinueWaitsForAnotherWindowsVaultExchange is D4 from the two-network +// test, reproduced the way it happened: a chat running in another window holds +// state-sync.lock while it pushes the vault over a slow relay, and the continue +// reaches its vault pull meanwhile. The pull took the lock without waiting, so +// the whole move failed with "resource temporarily unavailable" and the chat was +// handed back. The continue must wait for the exchange and arrive. +func TestAContinueWaitsForAnotherWindowsVaultExchange(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + h := newTwoHomes(t) + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + a := h.openA() + a.mustSay("before moving") + h.durable(h.cell.ID, h.cell) + mustNil(t, a.drive.Close(ctx)) + + // Another window's exchange holds the lock through its own open file, which + // is exactly how a second process holds it. + held, err := os.OpenFile(filepath.Join(h.b.Home, "state-sync.lock"), os.O_CREATE|os.O_RDWR, 0o600) + mustNil(t, err) + defer held.Close() + mustNil(t, filelock.Lock(held, true, true)) + + // The lock is let go only after the move has restored its files, so the + // vault pull that follows meets it held. + var once sync.Once + released := make(chan struct{}) + b := h.continuerB() + b.opt.OnPhase = func(p handoff.Phase) { + if p.Name == "restore" { + once.Do(func() { + time.AfterFunc(time.Second, func() { + _ = filelock.Unlock(held) + close(released) + }) + }) + } + } + got, err := b.Take(ctx, h.cell.ID) + if err != nil { + t.Fatalf("the continue failed while another window held the vault: %v", err) + } + select { + case <-released: + default: + t.Fatal("the continue arrived without meeting the held vault lock, so it proved nothing") + } + if got.Taken.Cell.ID != h.cell.ID { + t.Fatalf("a different chat arrived: %s", got.Taken.Cell.ID) + } +} + +// TestAVaultWaitIsBoundedAndBackgroundExchangesNeverWait pins both sides of the +// rule: a background exchange (no wait) is refused at once as before, and a +// continue's wait ends at its bound with the same busy refusal. +func TestAVaultWaitIsBoundedAndBackgroundExchangesNeverWait(t *testing.T) { + path := filepath.Join(t.TempDir(), "state-sync.lock") + held, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + mustNil(t, err) + defer held.Close() + mustNil(t, filelock.Lock(held, true, true)) + mine, err := os.OpenFile(path, os.O_RDWR, 0o600) + mustNil(t, err) + defer mine.Close() + + started := time.Now() + if err := lockState(t.Context(), mine, 0); !filelock.IsBusy(err) { + t.Fatalf("a background exchange did not get the busy refusal at once: %v", err) + } + if waited := time.Since(started); waited > vaultBusyRetry { + t.Fatalf("a background exchange waited %s", waited) + } + started = time.Now() + if err := lockState(t.Context(), mine, 3*vaultBusyRetry); !filelock.IsBusy(err) { + t.Fatalf("a bounded wait did not end in the busy refusal: %v", err) + } + if waited := time.Since(started); waited < 3*vaultBusyRetry { + t.Fatalf("the bounded wait gave up after %s", waited) + } +} diff --git a/internal/tui3/app.go b/internal/tui3/app.go index 5f595fbec2..20dda15fe0 100644 --- a/internal/tui3/app.go +++ b/internal/tui3/app.go @@ -1487,12 +1487,23 @@ type app struct { homeTakenSaid func() tea.Cmd // homeTaking keeps a retiring session on its own continuation until arrival. homeTaking string - // homeTakeSeq numbers takeovers so a cancelled attempt's late answer is ignored. + // homeTakeSeq keeps each answer and cleanup tied to the operation that created it. homeTakeSeq uint64 - // homeTakeCancel stops the in-flight takeover context when the person cancels. + // homeTakeCancel belongs to the active mutation; automatic cleanup keeps it until settlement. homeTakeCancel context.CancelFunc + // Abandonment bounds the outstanding answer without shortening an unattended move. + homeTakeAbandonTimer *time.Timer // homeTakeDraft moves the source composer only after the arriving door opens. homeTakeDraft *homeTakenMsg + // Automatic navigation remains subordinate to a confirmed choice until its door lands. + homeTakeAutomatic bool + // The origin survives an explicit join so cancellation still settles its acquisition. + homeTakeOriginAutomatic bool + homeTakeAbandoned bool + homeTakeOpening bool + homeTakeRow chatlist.Row + homeTakePending *homeTakenMsg + homeTakeResult *homeTakenMsg // continueAskSeq gives each surface card its own token so re-raising inside // an answer closure is not wiped by closeQuestion. continueAskSeq uint64 @@ -2328,6 +2339,7 @@ type app struct { branches BranchActions machineRead machineReading machinesAsking bool + machinesAskSeq uint64 // machinesRetaken replaces a listing begun before a completed take, so // its late answer cannot put the previous holder back on screen. machinesRetaken bool @@ -2524,7 +2536,9 @@ type app struct { landing bool autoContinue bool continueGeneration uint64 - pickSession bool + // A safety answer belongs to one explicit choice, so later navigation can withdraw it. + continueChecking *continuationChecked + pickSession bool // takeOverAt is a conversation this launch met a lock on // ([Options.TakeOver]). It is a transcript path, and the launch lands on // home with that row armed instead of quietly starting a second @@ -3974,7 +3988,7 @@ func (a *app) route(msg tea.Msg) (tea.Model, tea.Cmd) { // A repeated enter keeps the pending conversation; ctrl+enter does // the same only on the start page, where it is another enter spelling. // Draft edits stay with the pending chat; navigation cancels the transition. - if a.conversationOpening { + if a.conversationOpening && !(a.homeTakeAutomatic && !a.homeTakeAbandoned) { if a.openingDraftKey(msg) { return a, nil } @@ -4668,7 +4682,7 @@ func (a *app) route(msg tea.Msg) (tea.Model, tea.Cmd) { cmd, _ := a.teamMembershipMouse(msg, msg.Mouse()) return a, cmd } - if a.conversationOpening { + if a.conversationOpening && !(a.homeTakeAutomatic && !a.homeTakeAbandoned) { previewing := a.conversationPreview != nil if previewing { mouse := msg.Mouse() @@ -5556,6 +5570,10 @@ func (a *app) route(msg tea.Msg) (tea.Model, tea.Cmd) { a.tookHomeLeftOff(msg) return a, nil + case continueConflictMsg: + return a, a.continueConflictRead(msg) + case homeTakeSettledMsg: + return a, a.automaticTakeSettled(msg) case homeTakenMsg: return a, a.tookTakeover(msg) @@ -5570,6 +5588,9 @@ func (a *app) route(msg tea.Msg) (tea.Model, tea.Cmd) { case continuationChecked: return a, a.continuationReady(msg) + case homeMachinesSlowMsg: + a.machinesStillChecking(msg) + return a, nil case homeMachinesMsg: // THE OTHER MACHINES' CHATS, COMING BACK, off the update loop for the // reason the repository's reading is (homepanel_machines.go). @@ -8902,6 +8923,7 @@ func (a *app) renew() (tea.Cmd, bool) { return a.renewRefusing(a.note) } // the new conversation's entry line, because by then that conversation is what is // on screen. func (a *app) renewRefusing(say func(string)) (tea.Cmd, bool) { + a.abandonAutomaticTake() if !a.canStart() { say(newUnavailableWord) return nil, false diff --git a/internal/tui3/continuation_listing_test.go b/internal/tui3/continuation_listing_test.go index c61669e47b..829646ba61 100644 --- a/internal/tui3/continuation_listing_test.go +++ b/internal/tui3/continuation_listing_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/chatlist" "github.com/Agent-Field/codeaf/internal/directory" ) @@ -139,7 +141,13 @@ func TestContinueTakeReplacesAnOutstandingListing(t *testing.T) { src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} taker := &fakeTaker{} a, _ := heldLab(t, src, taker) - old := a.askChats()() + // The listing travels with its five-second progress tick, so the answer is + // read now, before the take, exactly as an answer already in flight would be. + asked, ok := a.askChats()().(tea.BatchMsg) + if !ok || len(asked) == 0 { + t.Fatalf("the listing did not travel as a batch: %T", asked) + } + old := asked[0]() taker.on = func(string) { src.change(func(row *chatlist.Row) { row.Mine, row.Status = true, chatlist.Here }) } drain(t, a, a.takeRemote(src.row)) if !a.machineRead.rows[0].Mine { diff --git a/internal/tui3/continue_cleanup_test.go b/internal/tui3/continue_cleanup_test.go new file mode 100644 index 0000000000..610c491259 --- /dev/null +++ b/internal/tui3/continue_cleanup_test.go @@ -0,0 +1,77 @@ +package tui3 + +import ( + "context" + "testing" + + "github.com/Agent-Field/codeaf/internal/remote" +) + +// The arrival opens a view on the same host session as the other window. +// Closing that view must not close the shared engine or release its ownership. +func TestAnAbandonedArrivalLeavesAnotherAttachmentRunning(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + for _, alreadyHere := range []bool{false, true} { + t.Run(map[bool]string{false: "arrived", true: "already here"}[alreadyHere], func(t *testing.T) { + far := &servedAgent{fakeAgent: &fakeAgent{model: "m"}} + engineAgent := &arrivalHostAgent{servedAgent: far, closed: make(chan struct{})} + sess := remote.NewSession(&remote.Engine{Agent: engineAgent, ReleaseOnLeave: func(remote.WrappedAgent) bool { return true }}, true) + t.Cleanup(func() { _ = sess.Close() }) + attach := func() *remote.Client { + surface, engine := remote.Pipe() + go func() { + _ = remote.ServeAttach(engine, engine, remote.AttachOptions{Open: func(remote.Hello) (*remote.Session, error) { return sess, nil }}) + _ = engine.Close() + }() + client, err := remote.Dial(surface, "loopback", remote.Hello{Version: remote.Version}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = client.Close() }) + return client + } + other, unused := attach(), attach() + f := &intentTaker{} + releaseUnusedTake(f, homeTakenMsg{taken: Taken{Fence: 42, AlreadyHere: alreadyHere}}, unused.Agent()) + if sess.Ended() || len(f.released) != 0 { + t.Fatal("cleanup closed or released another window's conversation") + } + if err := other.Take(); err != nil { + t.Fatal(err) + } + if _, err := other.Agent().Submit(context.Background(), "still working"); err != nil { + t.Fatalf("other attachment stopped working: %v", err) + } + far.finish() + if alreadyHere && sess.Attached() != 2 { + t.Fatal("already-here cleanup was not skipped") + } + if !alreadyHere { + if sess.Attached() != 1 { + t.Fatal("unused view was not detached") + } + if err := other.Agent().DetachAndWait(); err != nil { + t.Fatal(err) + } + // The final stream may still be draining when detach answers. + // Its engine close is the last-window release acknowledgment. + <-engineAgent.closed + if !sess.Ended() { + t.Fatal("last-window departure did not release the conversation") + } + } + }) + } +} + +type arrivalHostAgent struct { + *servedAgent + closed chan struct{} +} + +func (a *arrivalHostAgent) Close() error { + err := a.servedAgent.Close() + close(a.closed) + return err +} diff --git a/internal/tui3/continue_conflict_test.go b/internal/tui3/continue_conflict_test.go new file mode 100644 index 0000000000..e6ff2f1d97 --- /dev/null +++ b/internal/tui3/continue_conflict_test.go @@ -0,0 +1,132 @@ +package tui3 + +import ( + "context" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/chatlist" + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/session" +) + +func TestALostRaceKeepsItsRetryCardAndDraft(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + src.row.Fence = 3 + taker := &fakeTaker{err: directory.ErrLeaseHeld} + a, _ := heldLab(t, src, taker) + a.closeHome() + a.input.setText(" keep " + pasteToken(1, 2) + " these words") + a.pastes = []pasteChip{{n: 1, text: "whole\npaste"}} + a.chips = []chip{{path: "/tmp/attachment", file: true}} + want := a.mainComposer() + drive(t, a, key("enter")) + if _, ok := a.homeAsking(); !ok { + t.Fatalf("send did not raise a card: %s notes=%v", homeText(a), noteTexts(a)) + } + taker.on = func(string) { + src.change(func(r *chatlist.Row) { r.Device, r.DeviceID, r.Fence = "desktop", "dev-desktop", 8 }) + } + confirm(t, a) + q, ok := a.homeAsking() + if !ok || q.question.Options[0].Label != continueRetry || q.question.Options[1].Label != continueStay { + t.Fatalf("missing retry card: %+v", q) + } + if !strings.Contains(homeText(a), lostRaceWord) || q.question.Subject.Name != src.row.Title { + t.Fatal(homeText(a)) + } + if !reflect.DeepEqual(a.mainComposer(), want) { + t.Fatal("draft changed") + } +} + +func TestARefreshedHoldAsksAgainInsteadOfBlamingAnotherDevice(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + row := leftOffRow("c-off", "tower", chatlist.Running, time.Minute) + row.Fence = 3 + src := &continuationListing{row: row} + taker := &conflictTaker{fakeTaker: fakeTaker{err: directory.ErrLeaseHeld}} + a := continueHome(t, taker, src, 200) + standOn(t, a, row.Cell) + src.change(func(r *chatlist.Row) { r.Fence = 4 }) + confirm(t, a) + q, ok := a.homeAsking() + if strings.Contains(homeText(a), lostRaceWord) { + t.Fatalf("blamed another device for the same holder reopening:\n%s", homeText(a)) + } + if !ok { + t.Fatalf("no refreshed card:\n%s", homeText(a)) + } + if a.home.msg != "tower picked this chat up again; press enter to continue it here" { + t.Fatal(a.home.msg) + } + if len(taker.seen) != 1 { + t.Fatal(taker.seen) + } + taker.err = nil + confirm(t, a) + if len(taker.seen) != 2 || taker.seen[1].Device != row.DeviceID || taker.seen[1].Fence != 4 { + t.Fatalf("new consent not captured: %+v card=%+v", taker.seen, q) + } +} + +// conflictTaker records the consent the person actually confirmed on each card. +type conflictTaker struct { + fakeTaker + seen []directory.LeaseExpectation +} + +func (f *conflictTaker) TakeSeen(ctx context.Context, id string, seen *directory.LeaseExpectation) (Taken, error) { + f.seen = append(f.seen, *seen) + return f.fakeTaker.Take(ctx, id) +} + +func TestALostRaceFromASilentReturnKeepsItsCardAndDraft(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: chatlist.Row{Cell: heldCell, Title: "returning chat", Status: chatlist.Off, Mine: true, DeviceID: "dev-here", Fence: 2, Head: "saved"}} + taker := &conflictTaker{fakeTaker: fakeTaker{err: directory.ErrLeaseHeld}} + a, old := heldLab(t, src, taker) + a.closeHome() + root := session.DirOf(a.file) + if err := os.MkdirAll(filepath.Join(root, ".cell"), 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, ".cell", "adopted.json"), []byte(`{"V":1,"head":"saved","turns_size":0}`), 0600); err != nil { + t.Fatal(err) + } + taker.taken = Taken{Fence: 5, Head: "saved", Transcript: a.file} + taker.on = func(string) { + src.change(func(r *chatlist.Row) { + r.Mine, r.Status, r.DeviceID, r.Device, r.Fence = false, chatlist.Running, "dev-away", "tower", 4 + }) + } + a.input.setText("keep " + pasteToken(1, 2) + " these words") + a.pastes = []pasteChip{{n: 1, text: "whole\npaste"}} + want := a.mainComposer() + drive(t, a, key("enter")) + q, ok := a.homeAsking() + if !a.at(pageHome) || !ok || q.question.Options[0].Label != continueRetry { + t.Fatalf("silent refusal has no card: %s", homeText(a)) + } + if len(old.sent) != 0 || !reflect.DeepEqual(a.mainComposer(), want) { + t.Fatal("refusal spent the draft") + } + taker.err = nil + taker.on = func(string) { src.change(func(r *chatlist.Row) { r.Mine, r.Status, r.Fence = true, chatlist.Here, 5 }) } + confirm(t, a) + if len(taker.seen) != 2 || taker.seen[1].Device != "dev-away" || taker.seen[1].Fence != 4 { + t.Fatal(taker.seen) + } + arrived, ok := a.agent.(*switchAgent) + if !ok || len(arrived.sent) != 0 || !reflect.DeepEqual(a.mainComposer(), want) { + t.Fatal("retry did not retain second-enter policy") + } +} diff --git a/internal/tui3/continue_fix_round_test.go b/internal/tui3/continue_fix_round_test.go new file mode 100644 index 0000000000..9ec2c42870 --- /dev/null +++ b/internal/tui3/continue_fix_round_test.go @@ -0,0 +1,156 @@ +package tui3 + +import ( + "context" + "errors" + "os" + "path/filepath" + "reflect" + "testing" + "testing/synctest" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/chatlist" + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/session" +) + +type cancelledIntentTaker struct{ *intentTaker } + +func (f cancelledIntentTaker) TakeSeen(ctx context.Context, id string, seen *directory.LeaseExpectation) (Taken, error) { + if id != "x" { + return f.intentTaker.TakeSeen(ctx, id, seen) + } + f.started <- id + <-ctx.Done() + return Taken{}, ctx.Err() +} + +func TestAnAbandonedAutomaticTakeBoundsTheWaitingChoice(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + a, f, rows := intentRig(t) + keepSurfaceWorkersInTest(t, a) + a.taker = cancelledIntentTaker{f} + cmd := beginAutomatic(t, a) + defer a.homeTakeCancel() + result := make(chan []tea.Msg, 1) + go func() { result <- collectBatchMsgs(cmd) }() + <-f.started + // Time before abandonment does not shorten the person's waiting choice. + time.Sleep(3 * time.Minute) + standOn(t, a, rows[1].Cell) + confirm(t, a) + time.Sleep(2*time.Minute - time.Nanosecond) + select { + case <-result: + t.Fatal("automatic take cancelled before two minutes after abandonment") + default: + } + time.Sleep(time.Nanosecond) + synctest.Wait() + select { + case messages := <-result: + for _, msg := range messages { + drive(t, a, msg) + } + default: + t.Fatal("abandoned automatic take did not cancel after two minutes") + } + if a.file != f.files["y"] || a.homeTaking != "" || len(f.released) != 0 { + t.Fatal("waiting choice did not open after the cancelled take settled") + } + }) +} + +func TestASecondEscapeNavigatesWhileAnAutomaticTakeSettles(t *testing.T) { + a, f, _ := intentRig(t) + keepSurfaceWorkersInTest(t, a) + file := a.file + cmd := beginAutomatic(t, a) + drive(t, a, key("esc")) + if !a.at(pageHome) { + t.Fatal("first Escape left Home before withdrawing automatic navigation") + } + drive(t, a, key("esc")) + if a.at(pageHome) { + t.Fatal("second Escape was swallowed by the abandoned automatic take") + } + close(f.proceed) + drain(t, a, cmd) + if a.file != file || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatal("automatic arrival opened after Escape") + } +} + +func TestInspectingAnotherCardLeavesTheAutomaticArrivalAlone(t *testing.T) { + for _, answer := range []string{"leave it there", "escape"} { + t.Run(answer, func(t *testing.T) { + a, f, rows := intentRig(t) + keepSurfaceWorkersInTest(t, a) + cmd := beginAutomatic(t, a) + standOn(t, a, rows[1].Cell) + if answer == "escape" { + drive(t, a, key("esc")) + } else { + drive(t, a, key("2"), key("enter")) + } + close(f.proceed) + drain(t, a, cmd) + if a.file != f.files["x"] || len(f.seen) != 1 || len(f.released) != 0 { + t.Fatal("inspecting and declining a card withdrew the automatic arrival") + } + }) + } +} + +func TestAlreadyHereRecoveryRetriesOnlyOnceAndKeepsTheDraft(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: chatlist.Row{Cell: heldCell, Title: "returning chat", Status: chatlist.Here, Mine: true, DeviceID: "dev-here", Fence: 3, Head: "saved"}} + f := &conflictTaker{fakeTaker: fakeTaker{err: directory.ErrLeaseHeld}} + a, _ := heldLab(t, src, f) + keepSurfaceWorkersInTest(t, a) + root := session.DirOf(a.file) + if err := os.MkdirAll(filepath.Join(root, ".cell"), 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, ".cell", "adopted.json"), []byte(`{"V":1,"head":"saved","turns_size":0}`), 0600); err != nil { + t.Fatal(err) + } + a.input.setText("keep " + pasteToken(1, 2) + " these words") + a.pastes = []pasteChip{{n: 1, text: "whole\npaste"}} + a.chips = []chip{{path: "/tmp/kept", file: true}} + want := a.mainComposer() + row := src.row + row.Fence = 2 + cmd := a.takeRemote(row) + // Deliver only a bounded number of answers, so the old retry loop fails + // the assertion rather than hanging the test's event-draining fixture. + for step := 0; cmd != nil && step < 8; step++ { + var next []tea.Cmd + for _, msg := range collectBatchMsgs(cmd) { + switch msg.(type) { + case homeTakenMsg, continueConflictMsg: + _, follow := a.Update(msg) + next = append(next, follow) + } + } + cmd = tea.Batch(next...) + } + q, ok := a.homeAsking() + if len(f.seen) != 2 || !ok || q.question.Reason != continueFails || q.question.Options[0].Label != continueRetry || !reflect.DeepEqual(a.mainComposer(), want) { + t.Fatalf("already-here disagreement: attempts=%d card=%+v draft=%+v", len(f.seen), q, a.mainComposer()) + } + for _, seen := range f.seen { + if seen.Device != "dev-here" || seen.Fence != 2 { + t.Fatal("silent recovery changed the original consent", f.seen) + } + } + // An explicit retry starts its own bounded recovery attempt. + f.err = errors.New("stop on the explicit retry") + confirm(t, a) + if len(f.seen) != 3 { + t.Fatal("failure card cannot be retried explicitly") + } +} diff --git a/internal/tui3/continue_intent_test.go b/internal/tui3/continue_intent_test.go new file mode 100644 index 0000000000..4a1528ce26 --- /dev/null +++ b/internal/tui3/continue_intent_test.go @@ -0,0 +1,435 @@ +package tui3 + +import ( + "context" + "errors" + "reflect" + "strings" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/chatlist" + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/session" +) + +type intentTaker struct { + started chan string + proceed chan struct{} + released []string + seen []directory.LeaseExpectation + files map[string]string + fail bool +} + +func (f *intentTaker) SafeToContinue(context.Context, string, string, bool) (bool, error) { + return true, nil +} +func (f *intentTaker) Take(ctx context.Context, id string) (Taken, error) { + return f.TakeSeen(ctx, id, nil) +} +func (f *intentTaker) TakeSeen(ctx context.Context, id string, seen *directory.LeaseExpectation) (Taken, error) { + f.seen = append(f.seen, *seen) + f.started <- id + if id == "x" { + <-f.proceed + if f.fail { + return Taken{}, errors.New("failed x") + } + } + return Taken{Fence: 42, Transcript: f.files[id]}, nil +} +func (f *intentTaker) ReleaseUnused(ctx context.Context, id string, fence uint64) error { + if ctx.Err() != nil { + panic("cleanup inherited a cancelled context") + } + if _, ok := ctx.Deadline(); !ok || fence != 42 { + panic("unbounded or incorrect cleanup") + } + f.released = append(f.released, id) + return nil +} +func intentRig(t *testing.T) (*app, *intentTaker, []chatlist.Row) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + lab := newHomeLab(t) + ws := lab.workspace("p") + mine := lab.session("p", "local", "local", ws, time.Now()) + f := &intentTaker{started: make(chan string, 4), proceed: make(chan struct{}), files: map[string]string{}} + for _, id := range []string{"x", "y"} { + f.files[id] = lab.session("p", id, id, ws, time.Now()) + } + a := lab.app(mine) + a.width, a.height = 200, 70 + drain(t, a, a.openHome()) + rows := []chatlist.Row{ + {Cell: "x", Title: "automatic chat", Device: "tower", DeviceID: "dev-x", Fence: 7, Status: chatlist.Idle, Released: true, ReleasedAgo: time.Minute}, + {Cell: "y", Title: "chosen chat", Device: "laptop", DeviceID: "dev-y", Fence: 9, Status: chatlist.Off}, + } + a.taker = f + a.machineRead.rows = rows + a.rebuildMachines() + return a, f, rows +} +func beginAutomatic(t *testing.T, a *app) tea.Cmd { + t.Helper() + a.autoContinue = true + check := a.continueAfterQuit(nil) + if check == nil { + t.Fatal("no startup safety check") + } + cmds := a.continuationReady(check().(continuationChecked)) + if cmds == nil { + t.Fatal("no automatic take") + } + return cmds +} +func TestAPersonsPickIsTheChatThatOpens(t *testing.T) { + for _, failed := range []bool{false, true} { + t.Run(map[bool]string{false: "success", true: "failure"}[failed], func(t *testing.T) { + a, f, rows := intentRig(t) + f.fail = failed + result := make(chan []tea.Msg, 1) + cmd := beginAutomatic(t, a) + go func() { result <- collectBatchMsgs(cmd) }() + if id := <-f.started; id != "x" { + t.Fatal(id) + } + // Confirm the captured row, even if a later listing has a different ownership. + standOn(t, a, rows[1].Cell) + confirm(t, a) + if a.home.msg != "continuing chosen chat next; automatic chat is still on its way here" { + t.Fatalf("waiting choice: %q", a.home.msg) + } + select { + case id := <-f.started: + t.Fatalf("concurrent take %s", id) + default: + } + a.machineRead.rows[1].Fence = 99 + // A committed acquisition may answer after its request was cancelled. + // Cleanup must use its own bounded context rather than inherit this one. + a.homeTakeCancel() + close(f.proceed) + for _, msg := range <-result { + _, cmd := a.Update(msg) + drain(t, a, cmd) + } + if a.file != f.files["y"] { + t.Fatalf("opened %q, want y", a.file) + } + if !reflect.DeepEqual(f.seen, []directory.LeaseExpectation{{Device: "dev-x", Fence: 7}, {Device: "dev-y", Fence: 9}}) { + t.Fatal(f.seen) + } + want := []string{"x"} + if failed { + want = nil + } + if !reflect.DeepEqual(f.released, want) { + t.Fatalf("released %v", f.released) + } + if strings.Contains(homeText(a), "failed x") { + t.Fatal("abandoned failure raised a card") + } + }) + } +} +func TestPickingTheArrivingChatJoinsItsMove(t *testing.T) { + a, f, rows := intentRig(t) + cmd := beginAutomatic(t, a) + a.offerContinue(rows[0], chatlist.OfferFor(rows[0])) + confirm(t, a) + if strings.Contains(a.home.msg, continueAlreadyRunning) { + t.Fatal(a.home.msg) + } + close(f.proceed) + drain(t, a, cmd) + if a.file != f.files["x"] || len(f.seen) != 1 || len(f.released) != 0 { + t.Fatalf("join file=%s takes=%v releases=%v", a.file, f.seen, f.released) + } +} + +func TestAnExplicitMoveKeepsTheBusyAnswer(t *testing.T) { + a, f, rows := intentRig(t) + cmd := a.takeRemote(rows[0]) + standOn(t, a, rows[1].Cell) + confirm(t, a) + q, ok := a.homeAsking() + if !ok || q.question.Reason != continueAlreadyRunning || a.home.msg != continueAlreadyRunning { + t.Fatalf("explicit move lost its busy card: %s", homeText(a)) + } + close(f.proceed) + drain(t, a, cmd) + if a.file != f.files["x"] || len(f.seen) != 1 || len(f.released) != 0 { + t.Fatalf("explicit move changed: %s %+v %v", a.file, f.seen, f.released) + } +} + +func TestABareStartWithoutInputStillContinuesTheLatestRelease(t *testing.T) { + a, f, _ := intentRig(t) + close(f.proceed) + drain(t, a, beginAutomatic(t, a)) + if a.file != f.files["x"] || len(f.seen) != 1 { + t.Fatal("unattended startup changed") + } +} +func TestALateAutomaticMoveDoesNotOpenOverAChosenChat(t *testing.T) { + a, f, _ := intentRig(t) + cmd := beginAutomatic(t, a) + line := homeLine{kind: homeSession, row: session.SessionRow{ID: "local", Transcript: a.file}} + drain(t, a, a.homeOpenLine(line)) + chosen := a.file + close(f.proceed) + drain(t, a, cmd) + if a.file != chosen || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatalf("late move file=%s released=%v", a.file, f.released) + } +} +func TestEscapeDropsAWaitingPick(t *testing.T) { + a, f, rows := intentRig(t) + cmd := beginAutomatic(t, a) + a.offerContinue(rows[1], chatlist.OfferFor(rows[1])) + confirm(t, a) + drive(t, a, key("esc")) + chosen := a.file + close(f.proceed) + drain(t, a, cmd) + if len(f.seen) != 1 || a.file != chosen || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatalf("cancel file=%s takes=%v released=%v", a.file, f.seen, f.released) + } +} + +func TestAResumeChordKeepsItsChoiceBehindAnAutomaticMove(t *testing.T) { + a, f, rows := intentRig(t) + cmd := beginAutomatic(t, a) + a.leftOff.row = &rows[1] + drive(t, a, key(resumeKey)) + if a.home.msg != "continuing chosen chat next; automatic chat is still on its way here" { + t.Fatal(a.home.msg) + } + close(f.proceed) + drain(t, a, cmd) + if a.file != f.files["y"] || len(f.seen) != 2 || f.seen[1].Fence != 9 { + t.Fatalf("chord choice lost: file=%s consent=%+v", a.file, f.seen) + } +} + +func TestALateAutomaticDoorDoesNotReplaceAPick(t *testing.T) { + for _, choice := range []string{"local", "remote", "new"} { + t.Run(choice, func(t *testing.T) { + a, f, rows := intentRig(t) + opened, proceed := make(chan struct{}), make(chan struct{}) + original := a.open + a.open = func(where, file string) (Conversation, error) { + if file == f.files["x"] { + close(opened) + <-proceed + } + return original(where, file) + } + close(f.proceed) + answers := collectBatchMsgs(beginAutomatic(t, a)) + var door tea.Cmd + for _, msg := range answers { + if got, ok := msg.(homeTakenMsg); ok { + door = a.tookTakeover(got) + } + } + <-opened + var newDoor tea.Cmd + switch choice { + case "local": + drain(t, a, a.homeOpenLine(homeLine{kind: homeSession, row: session.SessionRow{ID: "local", Transcript: a.file}})) + case "remote": + standOn(t, a, rows[1].Cell) + confirm(t, a) + case "new": + newDoor = a.homeStart("") + drive(t, a, key("z")) + } + chosen := a.file + close(proceed) + drain(t, a, door) + if newDoor != nil { + drain(t, a, newDoor) + if a.file == chosen { + t.Fatal("new chat was not opened") + } + chosen = a.file + if a.input.String() != "z" { + t.Fatal("new chat follow-up was lost behind the automatic arrival") + } + } + if choice == "remote" { + chosen = f.files["y"] + } + if a.file != chosen || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatalf("late door: choice=%s file=%s releases=%v", choice, a.file, f.released) + } + }) + } +} + +func TestMovingTheCursorDoesNotWithdrawAnAutomaticArrival(t *testing.T) { + a, f, _ := intentRig(t) + cmd := beginAutomatic(t, a) + drive(t, a, key("down")) + drain(t, a, a.openHome()) + close(f.proceed) + drain(t, a, cmd) + if a.file != f.files["x"] || len(f.released) != 0 { + t.Fatalf("navigation withdrew arrival: %s %v", a.file, f.released) + } +} + +func TestALaterLocalChoiceDropsAQueuedContinue(t *testing.T) { + a, f, rows := intentRig(t) + chosen := a.file + cmd := beginAutomatic(t, a) + result := make(chan []tea.Msg, 1) + go func() { result <- collectBatchMsgs(cmd) }() + <-f.started + standOn(t, a, rows[1].Cell) + confirm(t, a) + drain(t, a, a.homeOpenLine(homeLine{kind: homeSession, row: session.SessionRow{ID: "local", Transcript: chosen}})) + close(f.proceed) + for _, msg := range <-result { + drive(t, a, msg) + } + if a.file != chosen || len(f.seen) != 1 || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatalf("latest choice lost: %s %+v %v", a.file, f.seen, f.released) + } +} + +func TestInputBeforeTheStartupSafetyAnswerCancelsTheMove(t *testing.T) { + a, f, _ := intentRig(t) + file := a.file + a.autoContinue = true + check := a.continueAfterQuit(nil) + drive(t, a, key("down")) + if cmd := a.continuationReady(check().(continuationChecked)); cmd != nil { + t.Fatal("input did not cancel the safety answer") + } + if len(f.seen) != 0 || a.file != file { + t.Fatal("cancelled startup moved a chat") + } +} + +func TestCancellingAJoinedAutomaticTakeStillLetsItGo(t *testing.T) { + a, f, rows := intentRig(t) + file := a.file + cmd := beginAutomatic(t, a) + a.offerContinue(rows[0], chatlist.OfferFor(rows[0])) + confirm(t, a) + drive(t, a, key("esc")) + close(f.proceed) + drain(t, a, cmd) + if a.file != file || len(f.seen) != 1 || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatalf("cancelled join stayed held: %s %+v %v", a.file, f.seen, f.released) + } +} + +func TestPickingTheArrivingChatAfterCancellingAWaitingPickJoins(t *testing.T) { + a, f, rows := intentRig(t) + cmd := beginAutomatic(t, a) + standOn(t, a, rows[1].Cell) + confirm(t, a) + drive(t, a, key("esc")) + a.offerContinue(rows[0], chatlist.OfferFor(rows[0])) + confirm(t, a) + close(f.proceed) + drain(t, a, cmd) + if a.file != f.files["x"] || len(f.seen) != 1 || len(f.released) != 0 { + t.Fatalf("join did not adopt its arrival: %s %+v %v", a.file, f.seen, f.released) + } +} + +type checkedIntentTaker struct { + *intentTaker + checking chan struct{} + checked chan struct{} + checkErr error +} + +func (f *checkedIntentTaker) SafeToContinue(_ context.Context, id, _ string, _ bool) (bool, error) { + if id == "y" { + close(f.checking) + <-f.checked + return true, f.checkErr + } + return true, nil +} + +func TestALateSafetyAnswerCannotUndoANewerPick(t *testing.T) { + for _, action := range []string{"local", "escape"} { + t.Run(action, func(t *testing.T) { + a, f, rows := intentRig(t) + file := a.file + checked := &checkedIntentTaker{intentTaker: f, checking: make(chan struct{}), checked: make(chan struct{})} + a.taker = checked + arriving := make(chan []tea.Msg, 1) + cmd := beginAutomatic(t, a) + go func() { arriving <- collectBatchMsgs(cmd) }() + <-f.started + a.leftOff.row = &rows[1] + check := stepKeyNoDrain(t, a, resumeKey) + answer := make(chan []tea.Msg, 1) + go func() { answer <- collectBatchMsgs(check) }() + <-checked.checking + close(f.proceed) + for _, msg := range <-arriving { + drive(t, a, msg) + } + if a.file != file || !reflect.DeepEqual(f.released, []string{"x"}) { + t.Fatal("automatic arrival opened over a checking choice") + } + if action == "local" { + drain(t, a, a.homeOpenLine(homeLine{kind: homeSession, row: session.SessionRow{ID: "local", Transcript: file}})) + } else { + drive(t, a, key("esc")) + } + close(checked.checked) + for _, msg := range <-answer { + drive(t, a, msg) + } + if a.file != file || len(f.seen) != 1 { + t.Fatal("late safety answer opened an abandoned choice") + } + }) + } +} + +func TestAFailedSafetyCheckDoesNotCancelAnAutomaticAcquisition(t *testing.T) { + a, f, rows := intentRig(t) + file := a.file + checked := &checkedIntentTaker{intentTaker: f, checking: make(chan struct{}), checked: make(chan struct{}), checkErr: errors.New("safety read failed")} + a.taker = checked + arriving := make(chan []tea.Msg, 1) + cmd := beginAutomatic(t, a) + go func() { arriving <- collectBatchMsgs(cmd) }() + <-f.started + a.leftOff.row = &rows[1] + check := stepKeyNoDrain(t, a, resumeKey) + answer := make(chan []tea.Msg, 1) + go func() { answer <- collectBatchMsgs(check) }() + <-checked.checking + close(checked.checked) + for _, msg := range <-answer { + drive(t, a, msg) + } + if a.homeTaking != "x" { + t.Fatal("a safety error discarded the acquisition still running") + } + if _, ok := a.homeAsking(); !ok { + t.Fatal("failed safety check has no retry card") + } + close(f.proceed) + for _, msg := range <-arriving { + drive(t, a, msg) + } + if a.file != file || !reflect.DeepEqual(f.released, []string{"x"}) || len(f.seen) != 1 { + t.Fatal("failed safety check let the automatic arrival open") + } +} diff --git a/internal/tui3/conversation_open.go b/internal/tui3/conversation_open.go index 3e5672d31f..f8ef73990d 100644 --- a/internal/tui3/conversation_open.go +++ b/internal/tui3/conversation_open.go @@ -118,12 +118,22 @@ func (a *app) conversationLater(ask func() (Conversation, error), say func(strin } a.conversationRequest++ request, page, ctx := a.conversationRequest, a.page, a.ctx + var automatic *homeTakenMsg + if a.homeTakeOpening && a.homeTakeResult != nil && a.homeTakeResult.automatic { + result := *a.homeTakeResult + automatic = &result + } + taker := a.taker a.conversationOpening = true say(conversationOpeningWord) a.touch() return a.offLoop(func() func(bool) tea.Cmd { conv, err := ask() if ctx.Err() != nil { + if automatic != nil { + releaseUnusedTake(taker, *automatic, conv.Agent) + return nil + } if conv.Agent != nil { leaveAgent(conv.Agent) } @@ -132,6 +142,16 @@ func (a *app) conversationLater(ask func() (Conversation, error), say func(strin return func(here bool) tea.Cmd { preview := a.conversationPreview current := a.conversationOpening && request == a.conversationRequest + if automatic != nil && (automatic.seq != a.homeTakeSeq || a.homeTakeAbandoned || !current || !here || !a.at(page)) { + if automatic.seq == a.homeTakeSeq { + a.homeTakeAbandoned = true + } + if current { + a.conversationOpening = false + a.conversationPreview = nil + } + return a.settleAutomaticTake(*automatic, conv.Agent) + } if current { a.conversationOpening = false a.conversationPreview = nil @@ -176,6 +196,7 @@ func (a *app) conversationLater(ask func() (Conversation, error), say func(strin } func (a *app) cancelConversationOpening() { + a.abandonAutomaticTake() a.finishHomeTake(true) hadDraft := a.conversationPreview != nil && (!a.conversationPreview.view.input.empty() || len(a.conversationPreview.view.pasted) > 0) if a.conversationPreview == nil { @@ -216,6 +237,7 @@ func (a *app) startDoor(workspace string) (func() (Conversation, error), bool) { } func (a *app) renewLater(say func(string), after func() tea.Cmd) tea.Cmd { + a.abandonAutomaticTake() if a.shared { cmd, ok := a.renewRefusing(say) if ok && after != nil { diff --git a/internal/tui3/conversationrow.go b/internal/tui3/conversationrow.go index 0159d7722f..4c8b91508f 100644 --- a/internal/tui3/conversationrow.go +++ b/internal/tui3/conversationrow.go @@ -22,6 +22,7 @@ import ( // locked" shortcut here, because this place never read the flock: the open // reports it instead, which is what the resume picker has always done. func (a *app) openConversationRow(row session.SessionRow) tea.Cmd { + a.abandonAutomaticTake() a.teamViewSet("") switch { case a.holding(row.Transcript): diff --git a/internal/tui3/displaced_offline_send_test.go b/internal/tui3/displaced_offline_send_test.go index 148cf88857..41e551a53e 100644 --- a/internal/tui3/displaced_offline_send_test.go +++ b/internal/tui3/displaced_offline_send_test.go @@ -38,7 +38,7 @@ func TestDisplacedOfflineWindowCannotBorrowAnEmptyListing(t *testing.T) { if len(agent.sent) != 0 || a.input.String() != "keep these words here" { t.Fatalf("offline copy sent: %v, draft=%q", agent.sent, a.input.String()) } - if !strings.Contains(strings.Join(noteTexts(a), "\n"), chatlist.Unreachable) { + if !strings.Contains(strings.Join(noteTexts(a), "\n"), "your message is still in the box, press enter to send it") { t.Fatal("the unsent draft has no explanation") } } diff --git a/internal/tui3/home.go b/internal/tui3/home.go index 461d0e0a0b..10bf639b91 100644 --- a/internal/tui3/home.go +++ b/internal/tui3/home.go @@ -3097,6 +3097,19 @@ func (a *app) homeEnter() tea.Cmd { // cursor" is two answers to whether a project somewhere else may be opened, and // the phone tier had the older one. func (a *app) homeOpenLine(line homeLine) tea.Cmd { + if !a.homeTakeOpening { + a.continueChecking = nil + } + if !a.homeTakeOpening && a.homeTakeAutomatic && (!a.homeTakeAbandoned || a.homeTakeResult == nil) && homeSessionID(line.row) == a.homeTaking { + // A local row can still be the old copy of the arriving chat. Joining the + // move avoids opening that copy before the replacement has settled. + a.homeTakeAutomatic, a.homeTakeAbandoned, a.homeTakePending = false, false, nil + a.home.say(continuing, "") + return nil + } + if !a.homeTakeOpening { + a.abandonAutomaticTake() + } if line.row.DeletionPending { a.conversationDeleteOpen(line.row.Transcript, homeName(line.row)) return nil @@ -3433,6 +3446,7 @@ func (a *app) homeStartInProject(project string) tea.Cmd { // opens. The shared legacy connection still swaps in place and keeps its // existing transition semantics. func (a *app) homeStartWithProject(text, place string) tea.Cmd { + a.abandonAutomaticTake() a.teamViewSet("") // THE SHELL REFUSAL COMES BEFORE THE ROAD IS CHOSEN, so a line home will not // run is refused the same way whichever road would have opened it. diff --git a/internal/tui3/homeband_resume.go b/internal/tui3/homeband_resume.go index 1447e811e2..552d55f631 100644 --- a/internal/tui3/homeband_resume.go +++ b/internal/tui3/homeband_resume.go @@ -33,6 +33,7 @@ package tui3 import ( "context" + "errors" "fmt" "slices" "time" @@ -40,6 +41,7 @@ import ( tea "charm.land/bubbletea/v2" "github.com/Agent-Field/codeaf/internal/chatlist" + "github.com/Agent-Field/codeaf/internal/directory" ) const ( @@ -168,6 +170,15 @@ func (a *app) resumeKeyHandler(key string) (tea.Cmd, bool) { case resumeKey: a.leftOff.answer() a.touch() + if a.homeTaking != "" && !a.homeTakeAutomatic { + a.raiseMachineAsk(*row, a.continueBusyShown(*row)) + a.home.say(continueAlreadyRunning, "") + return nil, true + } + if a.homeTakeAutomatic && row.Cell == a.homeTaking && (!a.homeTakeAbandoned || a.homeTakeResult == nil) { + return a.takeRemote(*row), true + } + a.abandonAutomaticTake() if safe, ok := a.taker.(continuationSafety); ok { return a.continueIfSafe(*row, safe), true } @@ -203,9 +214,14 @@ type continuationSafety interface { // lease expectation so a later holder can never be displaced by this gesture. func (a *app) continueIfSafe(row chatlist.Row, safe continuationSafety) tea.Cmd { ctx, file := a.ctx, a.file + choice := a.nextContinueAskID() + a.continueChecking = &continuationChecked{row: row, generation: choice} + if a.homeTakeAutomatic && a.homeTaking != "" { + a.home.say(fmt.Sprintf("continuing %s next; %s is still on its way here", row.Title, a.homeTakeRow.Title), "") + } return func() tea.Msg { clean, err := safe.SafeToContinue(ctx, row.Cell, file, false) - return continuationChecked{row: row, clean: clean, err: err} + return continuationChecked{row: row, clean: clean, generation: choice, err: err} } } @@ -255,6 +271,12 @@ func (a *app) continueAfterQuit(err error) tea.Cmd { } func (a *app) continuationReady(msg continuationChecked) tea.Cmd { + if !msg.automatic && msg.generation != 0 { + if a.continueChecking == nil || a.continueChecking.generation != msg.generation { + return nil + } + a.continueChecking = nil + } if a.hosted() || a.machineRead.computerRemoved { return nil } @@ -276,7 +298,18 @@ func (a *app) continuationReady(msg continuationChecked) tea.Cmd { opened = a.openHome() } if msg.err != nil { - return tea.Batch(opened, a.tookTakeover(homeTakenMsg{row: msg.row, err: msg.err})) + if !a.at(pageHome) { + opened = a.openHome() + } + a.abandonAutomaticTake() + failed := homeTakenMsg{row: msg.row, err: msg.err} + if errors.Is(msg.err, directory.ErrLeaseHeld) { + return tea.Batch(opened, a.lostRace(failed)) + } + failure := a.continueFailure(failed) + a.note(failure) + a.showTakeFailure(failed, failure) + return opened } if !msg.clean { if msg.automatic { @@ -284,5 +317,5 @@ func (a *app) continuationReady(msg continuationChecked) tea.Cmd { } return a.raiseContinue(msg.row, continueYesAt) } - return tea.Batch(opened, a.takeRemote(msg.row)) + return tea.Batch(opened, a.takeRemoteState(homeTakenMsg{row: msg.row, automatic: msg.automatic})) } diff --git a/internal/tui3/homepanel_continue.go b/internal/tui3/homepanel_continue.go index 2ce4e4a490..9c013e0fab 100644 --- a/internal/tui3/homepanel_continue.go +++ b/internal/tui3/homepanel_continue.go @@ -3,12 +3,16 @@ package tui3 import ( "context" "errors" + "fmt" + "log" "path/filepath" "strings" "time" tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/cell" + "github.com/Agent-Field/codeaf/internal/cellstore" "github.com/Agent-Field/codeaf/internal/chatlist" "github.com/Agent-Field/codeaf/internal/directory" machine "github.com/Agent-Field/codeaf/internal/preflight" @@ -184,7 +188,7 @@ func (a *app) homeMachineEnter(line homeLine) tea.Cmd { a.home.say(a.machineRead.unavailableWord(), "") return nil } - if a.homeTaking != "" && (offer.Kind == chatlist.Open || offer.Kind == chatlist.ContinueHere) && a.taker != nil { + if a.homeTaking != "" && !a.homeTakeAutomatic && (offer.Kind == chatlist.Open || offer.Kind == chatlist.ContinueHere) && a.taker != nil { a.raiseMachineAsk(row, a.continueBusyShown(row)) a.home.say(continueAlreadyRunning, "") return nil @@ -214,6 +218,12 @@ func (a *app) nextContinueAskID() uint64 { // finishHomeTake clears every in-flight continuation marker on this window. // When restoreDraft is true, any unsent words captured before opening are put back. func (a *app) finishHomeTake(restoreDraft bool) { + // A different door may finish while the automatic mutation is still settling. + // Its bookkeeping cannot discard the operation whose receipt needs releasing. + if a.homeTakeOriginAutomatic && a.homeTakeAbandoned { + return + } + a.stopAutomaticTakeTimer() if a.homeTakeCancel != nil { a.homeTakeCancel() a.homeTakeCancel = nil @@ -222,12 +232,34 @@ func (a *app) finishHomeTake(restoreDraft bool) { a.restoreTakeDraft(*a.homeTakeDraft) } a.homeTaking, a.homeTakeDraft, a.homeTakenSaid = "", nil, nil + a.homeTakeAutomatic, a.homeTakeOriginAutomatic, a.homeTakeAbandoned = false, false, false + a.homeTakeResult = nil } -// cancelHomeTake stops an in-flight takeover before its answer arrives. +// cancelHomeTake withdraws the choice. An automatic acquisition still settles +// before its unused ownership is released, including after an explicit join. func (a *app) cancelHomeTake() bool { + checking := a.continueChecking != nil + if checking { + a.abandonAutomaticTake() + } if a.homeTaking == "" { - return false + if checking { + a.home.say("", "") + a.touch() + } + return checking + } + if a.homeTakeOriginAutomatic || a.homeTakeAbandoned { + if a.homeTakeAbandoned && a.homeTakePending == nil && !checking { + return false + } + a.abandonAutomaticTake() + a.homeTakeAbandoned = true + a.homeTakePending = nil + a.home.say("", "") + a.touch() + return true } a.homeTakeSeq++ a.finishHomeTake(true) @@ -239,6 +271,9 @@ func (a *app) cancelHomeTake() bool { } func (a *app) spendHomeTakeFor(line homeLine) tea.Cmd { + if a.homeTakeAbandoned { + return nil + } if a.homeTakenSaid != nil { said := a.homeTakenSaid a.homeTakenSaid = nil @@ -265,6 +300,7 @@ func (a *app) askContinue(row chatlist.Row) tea.Cmd { } func (a *app) raiseContinue(row chatlist.Row, at int) tea.Cmd { + a.continueChecking = nil if a.hosted() { return nil } @@ -278,7 +314,7 @@ func (a *app) raiseContinue(row chatlist.Row, at int) tea.Cmd { if a.taker == nil { return a.sayOffer(row, chatlist.Offer{Line: chatlist.StatusLine(row)}) } - if a.homeTaking != "" { + if a.homeTaking != "" && !a.homeTakeAutomatic { a.raiseMachineAsk(row, a.continueBusyShown(row)) a.home.say(continueAlreadyRunning, "") return nil @@ -318,7 +354,7 @@ func (a *app) continueShown(row chatlist.Row, at int) questionShown { } return nil } - if a.homeTaking != "" { + if a.homeTaking != "" && !a.homeTakeAutomatic { a.raiseMachineAsk(row, a.continueBusyShown(row)) a.home.say(continueAlreadyRunning, "") return nil @@ -359,7 +395,7 @@ func (a *app) continueBusyShown(row chatlist.Row) questionShown { } return nil } - if a.homeTaking != "" { + if a.homeTaking != "" && !a.homeTakeAutomatic { a.raiseMachineAsk(row, a.continueBusyShown(row)) a.home.say(continueAlreadyRunning, "") return nil @@ -406,7 +442,7 @@ func (a *app) continueRetryShown(msg homeTakenMsg, failure string) questionShown } return nil } - if a.homeTaking != "" { + if a.homeTaking != "" && !a.homeTakeAutomatic { a.raiseMachineAsk(row, a.continueBusyShown(row)) a.home.say(continueAlreadyRunning, "") return nil @@ -414,7 +450,9 @@ func (a *app) continueRetryShown(msg homeTakenMsg, failure string) questionShown a.machineRead.down = false retry := msg retry.err = nil + retry.automatic = false retry.taken = Taken{} + retry.recoveredHere = false return a.takeRemoteState(retry) }, } @@ -433,14 +471,17 @@ func (a *app) presentRow(row chatlist.Row) chatlist.Row { // homeTakenMsg is the takeover's answer, coming back off the update loop. type homeTakenMsg struct { - seq uint64 - row chatlist.Row - taken Taken - err error - roots map[string]string - draft *composerState - send func() tea.Cmd - fromFile string + automatic bool + seq uint64 + row chatlist.Row + taken Taken + err error + roots map[string]string + draft *composerState + send func() tea.Cmd + fromFile string + // A fresh listing can disagree with the taker once without asking again. + recoveredHere bool } // takeRemote asks for the chat off the update loop because a move waits on the network. @@ -453,6 +494,9 @@ func (a *app) takeRemoteWithRoots(row chatlist.Row, roots map[string]string) tea } func (a *app) takeRemoteState(msg homeTakenMsg) tea.Cmd { + if !msg.automatic { + a.continueChecking = nil + } if a.hosted() || a.taker == nil { a.restoreTakeDraft(msg) return nil @@ -462,6 +506,19 @@ func (a *app) takeRemoteState(msg homeTakenMsg) tea.Cmd { a.home.say(chatlist.Removed, "") return nil } + if a.homeTaking != "" && a.homeTakeAutomatic { + if a.homeTaking == msg.row.Cell && (!a.homeTakeAbandoned || a.homeTakeResult == nil) { + // Joining adopts the existing move, so later choices use the explicit busy rule. + a.stopAutomaticTakeTimer() + a.homeTakeAutomatic, a.homeTakeAbandoned, a.homeTakePending = false, false, nil + a.home.say(continuing, "") + return nil + } + a.abandonAutomaticTake() + a.homeTakePending = &msg + a.home.say(fmt.Sprintf("continuing %s next; %s is still on its way here", msg.row.Title, a.homeTakeRow.Title), "") + return nil + } if a.homeTaking != "" { a.restoreTakeDraft(msg) if a.at(pageHome) { @@ -475,6 +532,8 @@ func (a *app) takeRemoteState(msg homeTakenMsg) tea.Cmd { a.homeTakeSeq++ msg.seq = a.homeTakeSeq a.homeTaking = msg.row.Cell + a.homeTakeAutomatic, a.homeTakeOriginAutomatic, a.homeTakeAbandoned = msg.automatic, msg.automatic, false + a.homeTakeRow = msg.row a.home.say(continuing, "") baseCtx := a.ctx if baseCtx == nil { @@ -504,11 +563,106 @@ func (a *app) takeRemoteState(msg homeTakenMsg) tea.Cmd { } } +// abandonAutomaticTake withdraws navigation without interrupting a mutation that +// may already have acquired ownership. Its answer must settle before another take. +func (a *app) abandonAutomaticTake() { + a.continueChecking = nil + if !a.homeTakeOriginAutomatic { + return + } + if !a.homeTakeAbandoned && a.homeTakeResult == nil && a.homeTakeCancel != nil { + // Cancellation follows the old Escape path only after the abandoned + // request has had its bounded chance to finish and report its receipt. + a.homeTakeAbandonTimer = time.AfterFunc(abandonedTakeWithin, a.homeTakeCancel) + } + a.homeTakeAbandoned = true + if a.homeTakePending != nil { + a.homeTakePending = nil + a.home.say("", "") + } + if a.homeTakeResult != nil && a.conversationOpening { + a.conversationOpening = false + a.conversationRequest++ + } +} + +const ( + unusedTakeWithin = 10 * time.Second + abandonedTakeWithin = 2 * time.Minute +) + +func (a *app) stopAutomaticTakeTimer() { + if a.homeTakeAbandonTimer != nil { + a.homeTakeAbandonTimer.Stop() + a.homeTakeAbandonTimer = nil + } +} + +type homeTakeSettledMsg struct{ seq uint64 } + +// settleAutomaticTake leaves an unopened view through the last-window release +// path. Cleanup must outlive cancellation of the originating UI. +func (a *app) settleAutomaticTake(msg homeTakenMsg, agent Agent) tea.Cmd { + taker := a.taker + return func() tea.Msg { + releaseUnusedTake(taker, msg, agent) + return homeTakeSettledMsg{seq: msg.seq} + } +} + +func releaseUnusedTake(taker Taker, msg homeTakenMsg, agent Agent) { + if msg.taken.AlreadyHere { + return + } + if agent != nil { + if err := leaveAgentError(agent); err != nil { + log.Printf("continue %s unused view leave: %v", msg.row.Cell, err) + } + // A host owns release once a view exists. Another window may use the + // same session, even before its writer replaces the take's receipt. + if _, hosted := agent.(detachable); hosted { + return + } + } + if msg.err == nil { + if release, ok := taker.(interface { + ReleaseUnused(context.Context, string, uint64) error + }); ok { + ctx, cancel := context.WithTimeout(context.Background(), unusedTakeWithin) + defer cancel() + if err := release.ReleaseUnused(ctx, msg.row.Cell, msg.taken.Fence); err != nil { + log.Printf("continue %s unused cleanup: %v", msg.row.Cell, err) + } + } + } +} + +func (a *app) automaticTakeSettled(msg homeTakeSettledMsg) tea.Cmd { + if msg.seq != a.homeTakeSeq { + return nil + } + pending := a.homeTakePending + a.homeTakePending = nil + a.homeTakeAutomatic, a.homeTakeOriginAutomatic = false, false + a.finishHomeTake(false) + if pending != nil { + return a.takeRemoteState(*pending) + } + if a.continueChecking != nil && a.at(pageHome) { + a.home.say(continuing, "") + } + return nil +} + // tookTakeover restores unsent words on failure and opens the arrived chat on success. func (a *app) tookTakeover(msg homeTakenMsg) tea.Cmd { if msg.seq != 0 && msg.seq != a.homeTakeSeq { return nil } + a.stopAutomaticTakeTimer() + if msg.automatic && a.homeTakeAbandoned { + return a.settleAutomaticTake(msg, nil) + } if a.homeTakeCancel != nil { a.homeTakeCancel() a.homeTakeCancel = nil @@ -526,11 +680,13 @@ func (a *app) tookTakeover(msg homeTakenMsg) tea.Cmd { if msg.err != nil { failure = a.failedTake(msg) a.restoreTakeDraft(msg) - a.note(failure) + if !errors.Is(msg.err, directory.ErrLeaseHeld) { + a.note(failure) + } } switch { case errors.Is(msg.err, directory.ErrLeaseHeld): - return a.lostRace(msg.row) + return a.lostRace(msg) case errors.Is(msg.err, directory.ErrUnreachable): // Home is where the failed move lands, so it carries the whole sentence: // the reason alone does not say the chat stayed and the words were kept. @@ -554,6 +710,17 @@ func (a *app) showTakeFailure(msg homeTakenMsg, failure string) { a.home.say(failure, "") if a.at(pageHome) && a.taker != nil && !a.machineRead.computerRemoved { a.raiseMachineAsk(msg.row, a.continueRetryShown(msg, failure)) + for i, line := range a.home.lines { + remote := line.remote != nil && line.remote.Cell == msg.row.Cell + local := line.kind == homeSession && homeSessionID(line.row) == msg.row.Cell + if remote || local { + a.home.cursor = i + if local { + a.home.armed = strings.TrimSpace(line.row.Transcript) + } + break + } + } } } @@ -582,20 +749,92 @@ func (a *app) chooseTakeRoots(msg homeTakenMsg, missing []session.WorkspaceRootB return cmd } -// lostRace is another device winning the takeover. The row goes back to -// `running on ` at once, from the listing this window already holds, -// and the directory is asked again for the winner's own name. -func (a *app) lostRace(row chatlist.Row) tea.Cmd { - rows := append([]chatlist.Row(nil), a.machineRead.rows...) - for i := range rows { - if rows[i].Cell == row.Cell { - rows[i].Status = chatlist.Running +// lostRace retains the failed card and its draft while checking who actually +// holds the chat. A changed ownership number alone does not mean a new device. +func (a *app) lostRace(msg homeTakenMsg) tea.Cmd { + var opened tea.Cmd + if !a.at(pageHome) { + opened = a.openHome() + } + msg.send = nil + a.showTakeFailure(msg, continueFails) + src, seq := a.machines, a.homeTakeSeq + file := "" + if line, ok := a.homeLineOf(msg.row.Cell); ok { + file = line.row.Transcript + } + if a.file != "" && filepath.Base(session.DirOf(a.file)) == msg.row.Cell { + file = a.file + } + var cardID uint64 + if q, ok := a.homeAsking(); ok { + cardID = q.question.ID + } + read := func() tea.Msg { + ctx, cancel := context.WithTimeout(context.Background(), machinesAskTimeout) + defer cancel() + var rows []chatlist.Row + var err error + if src != nil { + rows, _, err = chatlist.RowsForSend(ctx, src) + } + here := false + for _, row := range rows { + if row.Cell == msg.row.Cell && row.Status == chatlist.Here && row.Mine && file != "" { + here, _ = cellstore.KnowsHead(cell.Cell{ID: row.Cell, Root: session.DirOf(file)}, row.Head) + } } + return continueConflictMsg{take: msg, rows: rows, err: err, seq: seq, cardID: cardID, here: here} } - a.machineRead.rows = rows - a.rebuildMachines() - a.home.say(lostRaceWord, "") - return a.askMachines() + return tea.Batch(opened, read) +} + +type continueConflictMsg struct { + take homeTakenMsg + rows []chatlist.Row + err error + seq, cardID uint64 + here bool +} + +func (a *app) continueConflictRead(msg continueConflictMsg) tea.Cmd { + if msg.seq != a.homeTakeSeq || a.homeTaking != "" { + return nil + } + if msg.cardID != 0 { + q, ok := a.homeAsking() + if !ok || q.question.ID != msg.cardID { + return nil + } + } + failed := msg.take + failure := continueFails + if msg.err == nil { + for _, row := range msg.rows { + if row.Cell != failed.row.Cell { + continue + } + if msg.here && !failed.recoveredHere { + // Keep the old consent: only the taker's complete-local-head recovery may + // bypass it. If ownership moves again, that consent must still be refused. + failed.err = nil + failed.recoveredHere = true + return a.takeRemoteState(failed) + } + sameHolder := row.DeviceID != "" && row.DeviceID == failed.row.DeviceID || row.DeviceID == "" && failed.row.DeviceID == "" && row.Device == failed.row.Device + if sameHolder && row.Fence != failed.row.Fence && row.Device != "" { + failure = fmt.Sprintf("%s picked this chat up again; press enter to continue it here", nonemptyDevice(row.Device)) + } else if !sameHolder && !row.Mine { + failure = lostRaceWord + } + failed.row = row + break + } + a.machineRead.rows = msg.rows + a.rebuildMachines() + } + a.showTakeFailure(failed, failure) + return nil } // rebuildMachines redraws home from the reading it holds. The compact inbox @@ -620,6 +859,9 @@ func (a *app) rebuildMachines() { // the one they are arriving in. A shared window opens in place, and the ride // is spent here rather than at the landing. func (a *app) openTaken(msg homeTakenMsg) tea.Cmd { + a.homeTakeResult = &msg + a.homeTakeOpening = true + defer func() { a.homeTakeOpening = false }() if a.presentRow(msg.row).Mine { msg.taken.Resume.From = "" } @@ -630,7 +872,7 @@ func (a *app) openTaken(msg homeTakenMsg) tea.Cmd { if previous.failed || (previous.fence != 0 && previous.fence != msg.taken.Fence) { msg.taken.AlreadyHere = false } - a.machineRead.sendHolds[msg.row.Cell] = sendHold{fence: msg.taken.Fence} + a.machineRead.sendHolds[msg.row.Cell] = sendHold{fence: msg.taken.Fence, awaitingWriter: !msg.taken.AlreadyHere} rows := append([]chatlist.Row(nil), a.machineRead.rows...) for i := range rows { if rows[i].Cell == msg.row.Cell { @@ -682,6 +924,9 @@ func (a *app) openTaken(msg homeTakenMsg) tea.Cmd { a.moveTakeDraftCopies(*msg.draft) } a.putComposer(*msg.draft) + if msg.send == nil { + a.note("your message is still in the box; press enter to send it") + } } else if !msg.taken.AlreadyHere { // A take from Home restores the arrived chat's saved words. The // conversation being left keeps its own composer in its own draft. diff --git a/internal/tui3/homepanel_continue_test.go b/internal/tui3/homepanel_continue_test.go index 7203b06537..4d092a1c7a 100644 --- a/internal/tui3/homepanel_continue_test.go +++ b/internal/tui3/homepanel_continue_test.go @@ -212,8 +212,18 @@ func TestTakeoverScreenLostRace(t *testing.T) { if !strings.Contains(text, "another device continued this chat first") { t.Fatalf("R25 visible refusal changed:\n%s", text) } - if !strings.Contains(text, "Port the picker running on desk") || !strings.Contains(text, lostRaceWord) { - t.Fatalf("the row did not come back as running on the winner, with one line saying so:\n%s", text) + q, card := a.homeAsking() + if !card || q.question.Options[0].Label != continueRetry || q.question.Options[1].Label != continueStay { + t.Fatalf("the refusal did not stay on its retry card:\n%s", text) + } + found := false + for _, row := range a.machineRead.rows { + if row.Cell == "c-off" && row.Device == "desk" && row.Status == chatlist.Running { + found = true + } + } + if !found { + t.Fatalf("the refreshed row lost its holder: %+v", a.machineRead.rows) } if len(taker.cells) != 1 { t.Fatalf("the takeover ran %d times", len(taker.cells)) diff --git a/internal/tui3/homepanel_heldelsewhere.go b/internal/tui3/homepanel_heldelsewhere.go index 2249633a10..d4287d4649 100644 --- a/internal/tui3/homepanel_heldelsewhere.go +++ b/internal/tui3/homepanel_heldelsewhere.go @@ -7,6 +7,7 @@ import ( "path/filepath" "slices" "strings" + "time" tea "charm.land/bubbletea/v2" @@ -45,8 +46,14 @@ func (a *app) heldElsewhere() (chatlist.Row, bool) { // tray identify what was waiting, so an answer cannot send words the person // changed or move a message into a different conversation. type ownershipReadMsg struct { - file, draft string - chips []chip + file, draft string + host string + chips []chip + pastes []pasteChip + // A local startup timeout is not a failed Home directory listing. + writerWaitFailed bool + // Displacement has already been explained by the notice that ended this wait. + writerDisplaced bool reading homeMachinesMsg resume func() tea.Cmd first *composerState @@ -55,23 +62,34 @@ type ownershipReadMsg struct { proof bool local bool // notHeld prevents a saved receipt from reviving a displaced writer offline. - notHeld bool + notHeld bool + writerStopped bool } type sendHold struct { - fence uint64 - blocked bool + // awaitingWriter belongs only to this window's successful move, never to a listing receipt. + awaitingWriter bool + // A listing or notice wakes every send waiting for this window's writer. + writerStop chan struct{} + fence uint64 + blocked bool // seen marks a block that only news of another holder set: a listing row // or the displaced notice. A fresh reading made for this send that finds // the chat held here again lifts it, because that reading is newer than // the news; a block a take decided, or a failed take, never lifts that way. seen bool - // failed marks a hold whose take did not finish, so its session is a - // copy this window may not keep using even if a later take finds the - // chat already here. + // failed prevents reusing a stopped writer or an unfinished take, even + // when a later reading finds a complete chat held by this computer. failed bool } +func (h *sendHold) stopWriterWait() { + if h.writerStop != nil { + close(h.writerStop) + h.writerStop = nil + } +} + type sendLeaseSource interface { SendLease(string) (string, uint64, error) } @@ -93,9 +111,16 @@ func (a *app) checkBeforeSendWith(resume func() tea.Cmd) tea.Cmd { src, file, draft, agent := a.machines, a.file, a.input.String(), a.agent // A read that only ran out of time does not show the connection is down, // so the next Enter asks again rather than replaying the cached failure. - offline := a.machineRead.down && a.machineRead.unavailableWord() != chatlist.ReadTimedOut - blocked := a.machineRead.sendHolds[filepath.Base(session.DirOf(file))].blocked + offline := a.machineRead.down && !readTimedOut(a.machineRead.err) + hold := a.machineRead.sendHolds[filepath.Base(session.DirOf(file))] + blocked := hold.blocked + if hold.awaitingWriter && !blocked && hold.writerStop == nil { + hold.writerStop = make(chan struct{}) + a.machineRead.sendHolds[filepath.Base(session.DirOf(file))] = hold + } + host := a.relayHost() chips := slices.Clone(a.chips) + pastes := slices.Clone(a.pastes) var first *composerState if a.conversationPreview != nil && !a.conversationOpening { a.conversationPreview.ownershipPending = true @@ -103,21 +128,42 @@ func (a *app) checkBeforeSendWith(resume func() tea.Cmd) tea.Cmd { first = &composer } return func() tea.Msg { - ctx, cancel := context.WithTimeout(context.Background(), machinesAskTimeout) + readWithin := sendOwnershipTimeout + ctx, cancel := context.WithTimeout(context.Background(), readWithin) defer cancel() var rows []chatlist.Row var version uint64 var local bool var err error + writerStopped := false + writerWaitFailed := false + writerDisplaced := false holding, notHeld := false, false if writer, ok := agent.(renewalReader); ok { _, _, holding = writer.RenewalStatus() + if hold.awaitingWriter && !blocked && !holding { + // A new writer's installation is separate from taking ownership. Waiting + // grants no authority; its live status and receipt must still agree below. + cancel() + readWithin = writerStartTimeout + ctx, cancel = context.WithTimeout(context.Background(), readWithin) + defer cancel() + holding, err = awaitSendWriter(ctx, writer, hold.writerStop) + writerDisplaced = errors.Is(err, errSendWriterDisplaced) + writerWaitFailed = err != nil && !writerDisplaced + } notHeld = !holding + if notHeld { + _, period, _ := writer.RenewalStatus() + writerStopped = period > 0 + } } if blocked { holding, notHeld = false, true } - if scoped, ok := src.(interface { + if err != nil { + // An expired startup wait keeps the original submission unsent. + } else if scoped, ok := src.(interface { RowsForChatSend(context.Context, string, bool, bool) ([]chatlist.Row, uint64, bool, error) }); ok { rows, version, local, err = scoped.RowsForChatSend(ctx, file, offline, holding) @@ -126,7 +172,7 @@ func (a *app) checkBeforeSendWith(resume func() tea.Cmd) tea.Cmd { } else { rows, version, err = chatlist.RowsForSend(ctx, src) } - msg := ownershipReadMsg{file: file, draft: draft, chips: chips, resume: resume, first: first, local: local, notHeld: notHeld, reading: homeMachinesMsg{rows: rows, version: version, err: err}} + msg := ownershipReadMsg{file: file, draft: draft, chips: chips, resume: resume, first: first, local: local, notHeld: notHeld, reading: homeMachinesMsg{rows: rows, version: version, err: err, readWithin: readWithin}, host: host, pastes: pastes, writerWaitFailed: writerWaitFailed, writerStopped: writerStopped, writerDisplaced: writerDisplaced} id := filepath.Base(session.DirOf(file)) if local, ok := src.(sendLeaseSource); ok { msg.proof = true @@ -181,7 +227,7 @@ func (a *app) afterOwnershipRead(msg ownershipReadMsg) tea.Cmd { next := a.liveComposer() a.putComposer(*msg.first) var reading tea.Cmd - if !msg.local || msg.reading.err != nil { + if !msg.writerWaitFailed && !msg.writerDisplaced && (!msg.local || msg.reading.err != nil) { reading = a.recordMachines(msg.reading) } cmd := a.resumeOwnedSend(msg) @@ -196,12 +242,12 @@ func (a *app) afterOwnershipRead(msg ownershipReadMsg) tea.Cmd { } return tea.Batch(reading, cmd) } - if a.input.String() != msg.draft || !slices.Equal(a.chips, msg.chips) { + if a.input.String() != msg.draft || !slices.Equal(a.chips, msg.chips) || !slices.Equal(a.pastes, msg.pastes) { a.note("draft changed before sending; press enter to send your current words") return nil } var reading tea.Cmd - if !msg.local || msg.reading.err != nil { + if !msg.writerWaitFailed && !msg.writerDisplaced && (!msg.local || msg.reading.err != nil) { reading = a.recordMachines(msg.reading) } return tea.Batch(reading, a.resumeOwnedSend(msg)) @@ -210,6 +256,9 @@ func (a *app) afterOwnershipRead(msg ownershipReadMsg) tea.Cmd { // resumeOwnedSend spends only the submitted gesture after its fresh ownership // reading. A free chat can resume silently only from the same saved turn. func (a *app) resumeOwnedSend(msg ownershipReadMsg) tea.Cmd { + if msg.writerDisplaced { + return nil + } id := filepath.Base(session.DirOf(msg.file)) remote := id if msg.leaseCell != "" { @@ -226,6 +275,10 @@ func (a *app) resumeOwnedSend(msg ownershipReadMsg) tea.Cmd { a.note(chatlist.Removed) return nil } + if msg.writerWaitFailed { + a.note(messageKept(msg.host)) + return nil + } if msg.reading.err != nil { // AN UNREACHABLE SERVICE DOES NOT TAKE A HELD CHAT AWAY. The window // keeps working on a chat whose hold it recorded and last saw as its @@ -235,7 +288,7 @@ func (a *app) resumeOwnedSend(msg ownershipReadMsg) tea.Cmd { if a.offlineSendHeld(id, remote, msg) { return msg.resume() } - a.note(a.machineRead.unavailableWord()) + a.note(messageKept(msg.host)) return nil } for _, row := range msg.reading.rows { @@ -247,7 +300,7 @@ func (a *app) resumeOwnedSend(msg ownershipReadMsg) tea.Cmd { if msg.proof { fence = msg.fence } - if row.Status == chatlist.Here && row.Fence == fence && (!held.blocked || held.seen && row.Mine && !held.failed) { + if row.Status == chatlist.Here && row.Fence == fence && !(msg.notHeld && (held.awaitingWriter || msg.writerStopped) && !msg.local) && (!held.blocked || held.seen && row.Mine && !held.failed) { if held.blocked { held.blocked, held.seen = false, false a.machineRead.sendHolds[id] = held @@ -264,7 +317,7 @@ func (a *app) resumeOwnedSend(msg ownershipReadMsg) tea.Cmd { a.note(chatlist.Superseded(nonemptyDevice(row.Device))) return nil } - a.machineRead.sendHolds[id] = sendHold{fence: fence, blocked: true} + a.machineRead.sendHolds[id] = sendHold{fence: fence, blocked: true, failed: msg.writerStopped} cmd := a.openHome() return tea.Batch(cmd, a.askContinue(row)) } @@ -313,6 +366,7 @@ func (a *app) rememberSendHolds(msg homeMachinesMsg) { held, known := a.machineRead.sendHolds[row.Cell] if heldElsewhere(row) && (!known || row.Fence >= held.fence) { held.blocked, held.seen = true, !known || held.seen || !held.blocked + held.stopWriterWait() a.machineRead.sendHolds[row.Cell] = held } if row.Status != chatlist.Here || held.blocked || held.failed { @@ -321,7 +375,7 @@ func (a *app) rememberSendHolds(msg homeMachinesMsg) { // The directory can show another local window's fence. Only the // publishing receipt lets this window follow its own drive's new hold. if !known || (msg.holdCell == row.Cell && msg.holdFence != 0 && msg.holdFence == row.Fence) { - a.machineRead.sendHolds[row.Cell] = sendHold{fence: row.Fence} + a.machineRead.sendHolds[row.Cell] = sendHold{fence: row.Fence, awaitingWriter: held.awaitingWriter, writerStop: held.writerStop} } } } @@ -341,6 +395,7 @@ func (a *app) blockDisplacedSend(text string) { id := filepath.Base(session.DirOf(a.file)) held, known := a.machineRead.sendHolds[id] held.blocked, held.seen = true, !known || held.seen || !held.blocked + held.stopWriterWait() a.machineRead.sendHolds[id] = held } @@ -359,6 +414,12 @@ func (a *app) failedTake(msg homeTakenMsg) string { a.machineRead.sendHolds[current] = returned } } + return a.continueFailure(msg) +} + +// continueFailure describes an error without retiring local authority. A failed +// safety reading has not started a take and must not cancel one already running. +func (a *app) continueFailure(msg homeTakenMsg) string { switch { case errors.Is(msg.err, directory.ErrLeaseHeld): return lostRaceWord @@ -384,3 +445,54 @@ func (a *app) failedTake(msg homeTakenMsg) string { return continueFails + " because the chat could not be put in this folder; your words are still in the box. Try continue here again." } } + +// awaitSendWriter waits only for an uninstalled writer. Displacement ends the +// wait even when the writer's renewal facts are indistinguishable from startup. +func awaitSendWriter(ctx context.Context, writer renewalReader, displaced <-chan struct{}) (bool, error) { + var poll *time.Ticker + defer func() { + if poll != nil { + poll.Stop() + } + }() + for { + select { + case <-displaced: + return false, errSendWriterDisplaced + default: + } + if err := ctx.Err(); err != nil { + return false, err + } + var changed <-chan struct{} + if source, ok := writer.(interface{ WriterChanges() <-chan struct{} }); ok { + changed = source.WriterChanges() + } else if source, ok := writer.(interface{ RenewalChanges() <-chan struct{} }); ok { + changed = source.RenewalChanges() + } + _, period, holding := writer.RenewalStatus() + if holding || period > 0 { + return holding, nil + } + if changed == nil { + if poll == nil { + poll = time.NewTicker(100 * time.Millisecond) + } + select { + case <-displaced: + return false, errSendWriterDisplaced + case <-ctx.Done(): + return false, ctx.Err() + case <-poll.C: + } + } else { + select { + case <-displaced: + return false, errSendWriterDisplaced + case <-ctx.Done(): + return false, ctx.Err() + case <-changed: + } + } + } +} diff --git a/internal/tui3/homepanel_machines.go b/internal/tui3/homepanel_machines.go index be00336540..1d846da95d 100644 --- a/internal/tui3/homepanel_machines.go +++ b/internal/tui3/homepanel_machines.go @@ -3,6 +3,7 @@ package tui3 import ( "context" "errors" + "fmt" "path/filepath" "sort" "time" @@ -34,6 +35,9 @@ import ( // last ask failed. A failed ask keeps the rows and explains whether the reading // reached its deadline or the connection could not be made. type machineReading struct { + host string + checking bool + readWithin time.Duration err error sendHolds map[string]sendHold computerRemoved bool @@ -57,15 +61,42 @@ type machineReading struct { // homeMachinesMsg is one listing, coming BACK from the source. type homeMachinesMsg struct { - rows []chatlist.Row - version uint64 - err error - holdCell string - holdFence uint64 + readWithin time.Duration + rows []chatlist.Row + version uint64 + err error + holdCell string + holdFence uint64 } // machinesAskTimeout bounds one ask without treating silence as a network failure. -const machinesAskTimeout = 5 * time.Second +const ( + machinesAskTimeout = 25 * time.Second + machinesSlowAfter = 5 * time.Second + sendOwnershipTimeout = 5 * time.Second + writerStartTimeout = 30 * time.Second +) + +// This signal stays inside the send path; the existing displacement notice +// already tells the person why the message cannot leave this window. +var errSendWriterDisplaced = errors.New("send writer displaced") + +type homeMachinesSlowMsg struct{ seq uint64 } + +func (a *app) relayHost() string { + if source, ok := a.machines.(interface{ RelayHost() string }); ok { + return source.RelayHost() + } + return "" +} + +func (a *app) machinesStillChecking(msg homeMachinesSlowMsg) { + if !a.machinesAsking || msg.seq != a.machinesAskSeq { + return + } + a.machineRead.checking = true + a.rebuildMachines() +} // askMachines lists the other machines, off the update loop for the reason a // repository's status is asked (homeband_repo.go): a keystroke may not wait on @@ -80,24 +111,29 @@ func (a *app) askChats() tea.Cmd { return nil } a.machinesAsking = true + a.machinesAskSeq++ + seq := a.machinesAskSeq + a.machineRead.host = a.relayHost() src, file := a.machines, a.file - return func() tea.Msg { + read := func() tea.Msg { ctx, cancel := context.WithTimeout(context.Background(), machinesAskTimeout) defer cancel() rows, version, err := chatlist.RowsAt(ctx, src) - msg := homeMachinesMsg{rows: rows, version: version, err: err} + msg := homeMachinesMsg{rows: rows, version: version, err: err, readWithin: machinesAskTimeout} if local, ok := src.(sendLeaseSource); ok && file != "" { msg.holdCell = filepath.Base(session.DirOf(file)) _, msg.holdFence, _ = local.SendLease(msg.holdCell) } return msg } + return tea.Batch(read, surfaceTick(machinesSlowAfter, func(time.Time) tea.Msg { return homeMachinesSlowMsg{seq: seq} })) } // tookMachines files the answer. An error keeps the last rows and only marks // them stale. func (a *app) tookMachines(msg homeMachinesMsg) tea.Cmd { a.machinesAsking = false + a.machineRead.checking = false if a.machinesRetaken { a.machinesRetaken = false return a.askMachines() @@ -123,6 +159,7 @@ func (a *app) recordMachines(msg homeMachinesMsg) tea.Cmd { before, wasElsewhere := a.heldElsewhere() a.fileMachines(msg) a.machineRead.err = msg.err + a.machineRead.readWithin = msg.readWithin if row, elsewhere := a.heldElsewhere(); msg.err == nil && elsewhere && (!wasElsewhere || before.DeviceID != row.DeviceID || before.Device != row.Device) { a.note(chatlist.Superseded(nonemptyDevice(row.Device))) } @@ -197,17 +234,41 @@ func (m machineReading) into(in *homeGridInput, own []homeLine) homePanelRows { lines = append(lines, remoteLines...) sort.SliceStable(lines, func(i, j int) bool { return lines[i].stamp().After(lines[j].stamp()) }) rows := homePanelCut(in, panelSessions, lines) - if m.down { + if m.down || m.checking && m.host != "" { rows.lines = append(rows.lines, homeLine{kind: homeMachineRow, cell: &homeCell{ - kind: cellWhisper, panel: panelSessions, title: m.unavailableWord()}}) + kind: cellWhisper, panel: panelSessions, title: m.waitWord()}}) } return rows } -// unavailableWord preserves the distinction between a deadline and a failed connection. -func (m machineReading) unavailableWord() string { +// readTimedOut distinguishes an expired request from a failed connection. +func readTimedOut(err error) bool { var timeout interface{ Timeout() bool } - if errors.Is(m.err, context.DeadlineExceeded) || errors.As(m.err, &timeout) && timeout.Timeout() { + return errors.Is(err, context.DeadlineExceeded) || errors.As(err, &timeout) && timeout.Timeout() +} + +func (m machineReading) waitWord() string { + if m.down { + return m.unavailableWord() + } + if m.checking && m.host != "" { + return fmt.Sprintf("waiting for %s; still checking", m.host) + } + return "" +} + +func (m machineReading) unavailableWord() string { + if m.host != "" { + if readTimedOut(m.err) { + within := m.readWithin + if within == 0 { + within = machinesAskTimeout + } + return fmt.Sprintf("%s did not answer in %s; still trying", m.host, within) + } + return fmt.Sprintf("%s is unreachable; still trying", m.host) + } + if readTimedOut(m.err) { return chatlist.ReadTimedOut } return chatlist.Unreachable @@ -344,3 +405,14 @@ func (a *app) machineRowTexts(line homeLine, at, width int, pal palette) []strin } return texts } + +// messageKept is the one line a send that could not leave says. It lives +// beside the other relay-named words, so the sentence a person reads after an +// unanswered check is worded in one place rather than inside the send path. +func messageKept(host string) string { + const kept = "your message is still in the box, press enter to send it" + if host == "" { + return kept + } + return host + " has not answered; " + kept +} diff --git a/internal/tui3/keeper.go b/internal/tui3/keeper.go index 949c1db8e8..6ac5d450a8 100644 --- a/internal/tui3/keeper.go +++ b/internal/tui3/keeper.go @@ -1083,6 +1083,9 @@ func (a *app) holding(file string) bool { // conversation in the keeper is not something to open. It is something to look // at again. func (a *app) bringForward(file string) (cmd tea.Cmd, owned bool) { + if !a.homeTakeOpening { + a.abandonAutomaticTake() + } if a.startingChat() { back := a.parkChatStart() defer func() { cmd = tea.Batch(back, cmd) }() diff --git a/internal/tui3/machinepoll.go b/internal/tui3/machinepoll.go index 90177ea6fd..33c6bd3765 100644 --- a/internal/tui3/machinepoll.go +++ b/internal/tui3/machinepoll.go @@ -126,6 +126,6 @@ func (a *app) fileMachines(msg homeMachinesMsg) { changed := msg.err == nil && !chatlist.Same(a.machineRead.rows, msg.rows) a.machinePoll.landed(now, changed, a.socketUp() && msg.err == nil) if msg.err == nil { - a.machineRead = machineReading{rows: msg.rows, at: now, version: msg.version, sendHolds: a.machineRead.sendHolds, computerRemoved: a.machineRead.computerRemoved} + a.machineRead = machineReading{rows: msg.rows, at: now, version: msg.version, sendHolds: a.machineRead.sendHolds, computerRemoved: a.machineRead.computerRemoved, host: a.machineRead.host, checking: a.machineRead.checking} } } diff --git a/internal/tui3/ownership_deadline_test.go b/internal/tui3/ownership_deadline_test.go index 1644ad1f97..f063e946e1 100644 --- a/internal/tui3/ownership_deadline_test.go +++ b/internal/tui3/ownership_deadline_test.go @@ -13,6 +13,8 @@ import ( type deadlineChatListing struct{} +func (deadlineChatListing) RelayHost() string { return "sync.example" } + func (deadlineChatListing) Rows(context.Context) ([]chatlist.Row, error) { return nil, fmt.Errorf("%w: %w", directory.ErrUnreachable, context.DeadlineExceeded) } @@ -21,7 +23,7 @@ func (deadlineChatListing) Rows(context.Context) ([]chatlist.Row, error) { func TestHomeOwnershipDeadlineDoesNotClaimTheNetworkIsUnreachable(t *testing.T) { a := machinesHome(t, deadlineChatListing{}, 200) frame := homeText(a) - if !strings.Contains(frame, "other machines did not answer in time") || strings.Contains(frame, "other machines unreachable") { + if !strings.Contains(frame, "sync.example did not answer in 25s; still trying") || strings.Contains(frame, "other machines unreachable") { t.Fatalf("ownership read deadline was misreported:\n%s", frame) } } @@ -35,7 +37,7 @@ func TestSendOwnershipDeadlineKeepsWordsAndNamesTheDeadline(t *testing.T) { a.machines = deadlineChatListing{} a.input.setText("keep these words") drive(t, a, key("enter")) - if got := plain(lastNote(t, a)); !strings.Contains(got, chatlist.ReadTimedOut) || strings.Contains(got, chatlist.Unreachable) { + if got := plain(lastNote(t, a)); !strings.Contains(got, "sync.example has not answered; your message is still in the box, press enter to send it") || strings.Contains(got, chatlist.Unreachable) { t.Fatalf("deadline = %q", got) } if len(agent.sent) != 0 || a.input.String() != "keep these words" { diff --git a/internal/tui3/relay_fix_round_test.go b/internal/tui3/relay_fix_round_test.go new file mode 100644 index 0000000000..a386c0f499 --- /dev/null +++ b/internal/tui3/relay_fix_round_test.go @@ -0,0 +1,115 @@ +package tui3 + +import ( + "context" + "path/filepath" + "reflect" + "strings" + "testing" + "testing/synctest" + + tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/chatlist" + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/session" +) + +func TestASendReadingPreservesOutstandingHomeProgress(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + for _, answer := range []error{nil, directory.ErrUnreachable, context.DeadlineExceeded} { + t.Run(map[error]string{nil: "success", directory.ErrUnreachable: "unreachable", context.DeadlineExceeded: "deadline"}[answer], func(t *testing.T) { + a := newTestApp(&fakeAgent{model: "m"}) + keepSurfaceWorkersInTest(t, a) + drain(t, a, a.openHome()) + src := &slowRelayListing{entered: make(chan context.Context, 1), answer: make(chan error, 1)} + a.machines = src + commands := a.askChats()().(tea.BatchMsg) + result := make(chan tea.Msg, 1) + go func() { result <- commands[0]() }() + <-src.entered + drive(t, a, waitOut(commands[1])) + sent := false + drain(t, a, a.afterOwnershipRead(ownershipReadMsg{file: a.file, draft: a.input.String(), reading: homeMachinesMsg{readWithin: sendOwnershipTimeout}, resume: func() tea.Cmd { sent = true; return nil }})) + if !sent || !a.machinesAsking || !strings.Contains(homeText(a), "waiting for relay.example; still checking") { + t.Errorf("send reading dropped outstanding Home progress: %s", homeText(a)) + } + src.answer <- answer + drive(t, a, <-result) + if answer == nil { + if strings.Contains(homeText(a), "still checking") { + t.Fatal("completed listing retained progress") + } + } else { + want := "relay.example is unreachable; still trying" + if answer == context.DeadlineExceeded { + want = "relay.example did not answer in 25s; still trying" + } + if !strings.Contains(homeText(a), want) { + t.Fatal("listing failure lost its host: ", homeText(a)) + } + } + }) + } +} + +type zeroRenewalFactsWriter struct{ *startingSendAgent } + +func (*zeroRenewalFactsWriter) ReasoningLevels() map[string]string { return nil } + +func TestDisplacementEndsAWriterWaitWithZeroRenewalFacts(t *testing.T) { + for _, notice := range []bool{false, true} { + t.Run(map[bool]string{false: "listing", true: "notice"}[notice], func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + a, _, rows := intentRig(t) + keepSurfaceWorkersInTest(t, a) + a.closeHome() + writer := &zeroRenewalFactsWriter{&startingSendAgent{fakeAgent: &fakeAgent{model: "m"}, ready: make(chan struct{}), observed: make(chan struct{}, 1)}} + a.agent = writer + a.machines = chatlist.Static{} + id := filepath.Base(session.DirOf(a.file)) + a.machineRead.sendHolds = map[string]sendHold{id: {fence: 4, awaitingWriter: true}} + a.input.setText("keep " + pasteToken(1, 2) + " these words") + a.pastes = []pasteChip{{n: 1, text: "whole\npaste"}} + a.chips = []chip{{path: "/tmp/kept", file: true}} + want := a.mainComposer() + // A real viewer's (zero, 0, false) is omitted by FactsOf, so + // writer startup and displacement have identical renewal facts. + if session.FactsOf(writer).Renewal != nil { + t.Fatal("fixture does not match the drive's viewer facts") + } + cmd := stepKeyNoDrain(t, a, "enter") + result := make(chan []tea.Msg, 1) + go func() { result <- collectBatchMsgs(cmd) }() + <-writer.observed + if notice { + a.note(chatlist.Superseded("tower")) + } else { + row := rows[0] + row.Cell, row.Status, row.Released, row.Device, row.Fence = id, chatlist.Running, false, "tower", 5 + drain(t, a, a.recordMachines(homeMachinesMsg{rows: []chatlist.Row{row}})) + } + synctest.Wait() + select { + case messages := <-result: + for _, msg := range messages { + drive(t, a, msg) + } + default: + t.Error("displaced writer still waits for its startup timeout") + // The fake clock finishes the old timeout without wall time. + for _, msg := range <-result { + drive(t, a, msg) + } + } + if !reflect.DeepEqual(a.mainComposer(), want) || len(writer.sent) != 0 { + t.Fatal("displacement spent or changed the submitted message") + } + notes := strings.Join(noteTexts(a), "\n") + if !strings.Contains(notes, chatlist.Superseded("tower")) || strings.Contains(notes, "has not answered") { + t.Fatal("displacement blamed the relay: ", notes) + } + }) + }) + } +} diff --git a/internal/tui3/relay_manual_test.go b/internal/tui3/relay_manual_test.go new file mode 100644 index 0000000000..11d17d7b7b --- /dev/null +++ b/internal/tui3/relay_manual_test.go @@ -0,0 +1,28 @@ +package tui3 + +import ( + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/manual" +) + +func TestRelayWaitAndChoiceWordsAreInTheChatManual(t *testing.T) { + page, ok := manual.Chat().Page("continuing-a-chat-on-another-computer") + if !ok { + t.Fatal("missing continue manual") + } + for _, words := range []string{ + "continuing next; is still on its way here", + " picked this chat up again; press enter to continue it here", + "waiting for ; still checking", + " did not answer in " + machinesAskTimeout.String() + "; still trying", + " is unreachable; still trying", + messageKept(""), + "your message is still in the box; press enter to send it", + } { + if !strings.Contains(page, words) { + t.Errorf("manual is missing %q", words) + } + } +} diff --git a/internal/tui3/relay_wait_test.go b/internal/tui3/relay_wait_test.go new file mode 100644 index 0000000000..6b7b0ddc73 --- /dev/null +++ b/internal/tui3/relay_wait_test.go @@ -0,0 +1,182 @@ +package tui3 + +import ( + "context" + "errors" + "reflect" + "strings" + "sync/atomic" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/chatlist" + "github.com/Agent-Field/codeaf/internal/directory" +) + +type slowRelayListing struct { + entered chan context.Context + answer chan error + rows []chatlist.Row +} + +func (s *slowRelayListing) RelayHost() string { return "relay.example" } +func (s *slowRelayListing) Rows(ctx context.Context) ([]chatlist.Row, error) { + s.entered <- ctx + select { + case err := <-s.answer: + return s.rows, err + case <-ctx.Done(): + return nil, ctx.Err() + } +} +func TestASlowRelayKeepsCheckingAndNamesTheRelay(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + a := newTestApp(&fakeAgent{model: "m"}) + drain(t, a, a.openHome()) + rows := []chatlist.Row{leftOffRow("kept", "tower", chatlist.Off, time.Minute)} + src := &slowRelayListing{entered: make(chan context.Context, 1), answer: make(chan error, 1), rows: rows} + a.machines = src + a.machineRead.rows = rows + batch, ok := a.askChats()().(tea.BatchMsg) + if !ok || len(batch) != 2 { + t.Fatal("listing has no progress threshold") + } + result := make(chan tea.Msg, 1) + go func() { result <- batch[0]() }() + ctx := <-src.entered + deadline, ok := ctx.Deadline() + if !ok || time.Until(deadline) < 24*time.Second || time.Until(deadline) > 25*time.Second { + t.Fatal("listing bound is not 25s") + } + drive(t, a, waitOut(batch[1])) + if !strings.Contains(homeText(a), "waiting for relay.example; still checking") || a.machineRead.down { + t.Fatal(homeText(a)) + } + select { + case <-ctx.Done(): + t.Fatal("progress abandoned the listing") + default: + } + if !reflect.DeepEqual(a.machineRead.rows, rows) { + t.Fatal("slow read dropped rows") + } + src.answer <- nil + drive(t, a, <-result) + if strings.Contains(homeText(a), "still checking") { + t.Fatal("progress survived success") + } + for _, err := range []error{directory.ErrUnreachable, context.DeadlineExceeded} { + a.machinesAsking = false + commands := a.askChats()().(tea.BatchMsg) + go func() { result <- commands[0]() }() + <-src.entered + src.answer <- err + drive(t, a, <-result) + want := "relay.example is unreachable; still trying" + if errors.Is(err, context.DeadlineExceeded) { + want = "relay.example did not answer in 25s; still trying" + } + if !strings.Contains(homeText(a), want) || !a.machineRead.down || !reflect.DeepEqual(a.machineRead.rows, rows) { + t.Fatal(homeText(a)) + } + } +} + +type startingSendAgent struct { + *fakeAgent + ready chan struct{} + observed chan struct{} + held atomic.Bool +} + +func (s *startingSendAgent) RenewalStatus() (time.Time, time.Duration, bool) { + return time.Time{}, 0, s.held.Load() +} +func (s *startingSendAgent) WriterChanges() <-chan struct{} { + select { + case s.observed <- struct{}{}: + default: + } + return s.ready +} + +type localWriterListing struct { + chatlist.Static + failure error +} + +func (s localWriterListing) RelayHost() string { return "relay.example" } +func (s localWriterListing) SendLease(string) (string, uint64, error) { return heldCell, 4, nil } +func (s localWriterListing) RowsForChatSend(_ context.Context, _ string, _ bool, holding bool) ([]chatlist.Row, uint64, bool, error) { + if !holding { + return nil, 0, false, directory.ErrUnreachable + } + return s.Static, 0, true, s.failure +} +func TestTheFirstMessageAfterAMoveWaitsForItsWriter(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + taker := &fakeTaker{} + a, _ := heldLab(t, src, taker) + taker.taken = Taken{Fence: 4, Transcript: a.file} + taker.on = func(string) { + src.change(func(row *chatlist.Row) { row.Mine, row.Status, row.Fence = true, chatlist.Here, 4 }) + } + standOn(t, a, heldCell) + confirm(t, a) + writer := &startingSendAgent{fakeAgent: &fakeAgent{model: "m"}, ready: make(chan struct{}), observed: make(chan struct{}, 1)} + a.agent = writer + a.machines = localWriterListing{Static: chatlist.Static{src.row}} + a.input.setText(" keep this exact message ") + cmd := stepKeyNoDrain(t, a, "enter") + result := make(chan []tea.Msg, 1) + go func() { result <- collectBatchMsgs(cmd) }() + select { + case <-writer.observed: + case <-result: + t.Fatal("send returned before waiting for its writer") + } + if len(writer.sent) != 0 || a.input.String() != " keep this exact message " { + t.Fatal("send was spent before writer startup") + } + writer.held.Store(true) + close(writer.ready) + for _, msg := range <-result { + drive(t, a, msg) + } + if len(writer.sent) != 1 || writer.sent[0] != "keep this exact message" { + t.Fatalf("sent %v", writer.sent) + } + if strings.Contains(strings.Join(noteTexts(a), "\n"), chatlist.ReadTimedOut) { + t.Fatal("Home listing sentence appeared in chat") + } +} +func TestAnUnsentMessageSaysItWasKept(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + a, agent := heldLab(t, src, &fakeTaker{}) + a.closeHome() + a.machines = hostedFailedListing{} + a.input.setText(" keep " + pasteToken(1, 2) + " these words") + a.pastes = []pasteChip{{n: 1, text: "whole\npaste"}} + a.chips = []chip{{path: "/tmp/kept", file: true}} + want := a.mainComposer() + drive(t, a, key("enter")) + if !reflect.DeepEqual(a.mainComposer(), want) || len(agent.sent) != 0 { + t.Fatal("unsent composer changed") + } + if got := plain(lastNote(t, a)); got != "relay.example has not answered; your message is still in the box, press enter to send it" { + t.Fatal(got) + } +} + +type hostedFailedListing struct{} + +func (hostedFailedListing) RelayHost() string { return "relay.example" } +func (hostedFailedListing) Rows(context.Context) ([]chatlist.Row, error) { + return nil, context.DeadlineExceeded +} diff --git a/internal/tui3/writer_wait_test.go b/internal/tui3/writer_wait_test.go new file mode 100644 index 0000000000..f99197e480 --- /dev/null +++ b/internal/tui3/writer_wait_test.go @@ -0,0 +1,153 @@ +package tui3 + +import ( + "context" + "errors" + "reflect" + "strings" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/Agent-Field/codeaf/internal/chatlist" +) + +type stoppedWriter struct{ *fakeAgent } + +func (stoppedWriter) RenewalStatus() (time.Time, time.Duration, bool) { + return time.Time{}, time.Second, false +} +func TestAWriterWaitCannotReviveADisplacedWriter(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + pending := &startingSendAgent{ready: make(chan struct{}), observed: make(chan struct{}, 1)} + if held, err := awaitSendWriter(ctx, pending, nil); held || !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled wait: %v %v", held, err) + } + ctx, deadline := context.WithDeadline(t.Context(), time.Unix(1, 0)) + defer deadline() + if held, err := awaitSendWriter(ctx, pending, nil); held || !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expired wait: %v %v", held, err) + } + if held, err := awaitSendWriter(t.Context(), stoppedWriter{}, nil); held || err != nil { + t.Fatalf("displaced writer: %v %v", held, err) + } +} +func TestAWriterWaitTimeoutKeepsTheWholeMessage(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + a, agent := heldLab(t, src, &fakeTaker{}) + a.closeHome() + a.input.setText(" keep " + pasteToken(1, 2) + " more") + a.pastes = []pasteChip{{n: 1, text: "whole\npaste"}} + a.chips = []chip{{path: "/tmp/kept", file: true}} + want := a.mainComposer() + msg := ownershipReadMsg{file: a.file, draft: a.input.String(), chips: a.chips, pastes: a.pastes, writerWaitFailed: true, host: "sync.example", notHeld: true, proof: true, reading: homeMachinesMsg{err: context.DeadlineExceeded}} + // The ownership result carries the unchanged submission through the same landing + // as an expired writer-start wait; it must never spend the send callback. + msg.resume = func() tea.Cmd { t.Fatal("expired writer sent"); return nil } + drain(t, a, a.afterOwnershipRead(msg)) + if len(agent.sent) != 0 || !reflect.DeepEqual(a.mainComposer(), want) { + t.Fatal("timeout changed the message") + } + if got := plain(lastNote(t, a)); got != "sync.example has not answered; your message is still in the box, press enter to send it" { + t.Fatal(got) + } +} +func TestAContinueExplainsThatTheMessageWasKept(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + taker := &fakeTaker{} + a, old := heldLab(t, src, taker) + a.closeHome() + taker.taken = Taken{Fence: 4, Transcript: a.file} + a.input.setText("message kept for the second enter") + drive(t, a, key("enter")) + taker.on = func(string) { + src.change(func(row *chatlist.Row) { row.Mine, row.Status, row.Fence = true, chatlist.Here, 4 }) + } + confirm(t, a) + notes := strings.Join(noteTexts(a), "\n") + if strings.Count(notes, "your message is still in the box; press enter to send it") != 1 || a.input.String() != "message kept for the second enter" || len(old.sent) != 0 { + t.Fatalf("missing draft explanation: %s", notes) + } +} + +type otherWindowListing struct{ localWriterListing } + +func (otherWindowListing) SendLease(string) (string, uint64, error) { return heldCell, 5, nil } +func (s otherWindowListing) RowsForChatSend(context.Context, string, bool, bool) ([]chatlist.Row, uint64, bool, error) { + return s.Static, 0, false, nil +} +func TestAnotherWindowsHoldCannotFinishAWaitingSend(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + taker := &fakeTaker{} + a, _ := heldLab(t, src, taker) + taker.taken = Taken{Fence: 4, Transcript: a.file} + taker.on = func(string) { src.change(func(r *chatlist.Row) { r.Mine, r.Status, r.Fence = true, chatlist.Here, 4 }) } + standOn(t, a, heldCell) + confirm(t, a) + agent := &fakeAgent{model: "m"} + a.agent = stoppedWriter{agent} + other := src.row + other.Fence = 5 + a.machines = otherWindowListing{localWriterListing{Static: chatlist.Static{other}}} + a.input.setText("keep this for my window") + drive(t, a, key("enter")) + if len(agent.sent) != 0 || string(a.mainComposer().box.value) != "keep this for my window" { + t.Fatal("another window's current receipt revived a stopped writer") + } + if _, ok := a.homeAsking(); !ok { + t.Fatal("unheld window has no continue card") + } + taker.taken = Taken{Fence: 5, AlreadyHere: true, Transcript: a.file} + confirm(t, a) + if a.agent == agent || len(agent.sent) != 0 { + t.Fatal("stopped writer was reused after confirmation") + } + arrived, ok := a.agent.(*switchAgent) + if !ok || len(arrived.sent) != 0 || a.input.String() != "keep this for my window" { + t.Fatal("confirmation did not reopen with the message kept") + } +} + +func TestAWriterWaitDoesNotSendAnEditedPaste(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("CODEAF_TELEMETRY", "off") + src := &continuationListing{row: leftOffRow(heldCell, "tower", chatlist.Running, time.Minute)} + taker := &fakeTaker{} + a, _ := heldLab(t, src, taker) + taker.taken = Taken{Fence: 4, Transcript: a.file} + taker.on = func(string) { src.change(func(r *chatlist.Row) { r.Mine, r.Status, r.Fence = true, chatlist.Here, 4 }) } + standOn(t, a, heldCell) + confirm(t, a) + writer := &startingSendAgent{fakeAgent: &fakeAgent{model: "m"}, ready: make(chan struct{}), observed: make(chan struct{}, 1)} + a.agent = writer + a.machines = localWriterListing{Static: chatlist.Static{src.row}} + a.input.setText("keep " + pasteToken(1, 2) + " these words") + a.pastes = []pasteChip{{n: 1, text: "original\npaste"}} + result := make(chan []tea.Msg, 1) + cmd := stepKeyNoDrain(t, a, "enter") + go func() { result <- collectBatchMsgs(cmd) }() + select { + case <-writer.observed: + case <-result: + t.Fatal("writer wait returned early") + } + a.pastes[0].text = "edited\npaste" + writer.held.Store(true) + close(writer.ready) + for _, msg := range <-result { + drive(t, a, msg) + } + if len(writer.sent) != 0 || a.pastes[0].text != "edited\npaste" { + t.Fatal("an edited paste was sent or replaced") + } + if got := plain(lastNote(t, a)); got != "draft changed before sending; press enter to send your current words" { + t.Fatal(got) + } +}