diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6ebf11b..a520a82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,6 +67,10 @@ jobs: - name: Vet run: go vet ./... + # Windows-only process and lock behaviour the unix legs cannot cover. + - name: Test (windows-specific) + run: go test ./internal/filelock/ ./internal/server/ -run "TestWindows|TestLock" -v + lint: name: Lint runs-on: ubuntu-latest diff --git a/AGENTS.md b/AGENTS.md index 95d3814..c8d73bb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,7 @@ Keep it that way. | `internal/tokens` | Local persistence of per-session auth tokens | | `internal/workspace` | Per-cwd draft, queue, history, and last-session id | | `internal/settings` | Persisted front-end preferences (theme, bell, notify, plain, verbosity, thinking, resume, reduce-motion) at `~/.bodek/config.json`; flag > file > default resolution with a theme-only `BODEK_THEME` env override (`BODEK_CONFIG` relocates the file); odek's config is untouched | +| `internal/filelock` | Cross-process advisory lock (`.lock`, flock / LockFileEx) around the stores' read-merge-write; best-effort, gives up after 2s | | `internal/watchdog` | Orphan guard: re-execs a tiny watchdog that terminates `odek serve` when the bodek parent dies for any reason (per-platform impls) | | `internal/tui` | The Bubble Tea model, update loop, panels, and view | | `internal/update` | Self-upgrade: fetch and swap in the latest GitHub release binary | @@ -236,8 +237,11 @@ feat(tui): compact tool steps with Ctrl+E details toggle stays. Approvals render above a live composer: bare `a`/`d`/`t` decide only while the composer draft is empty (a non-empty draft, paste, or any modifier routes to the composer), and `Alt+A`/`Alt+D`/`Alt+T` - decide. On Alt-less terminals (macOS Option-as-UTF-8), `^U` clears the - whole draft first, restoring the bare-key path — never leave such a + decide. On Alt-less terminals (macOS Option-as-UTF-8), emptying the draft + (Backspace, or `^W` word-wise — `^U` pages the transcript while a card + is up, and Shift+Delete is a no-op there) restores the bare-key path; + with the find bar or a drawer editor open, a bare letter is swallowed + with a hint and Esc closes the field first — never leave such a terminal with no reachable approval binding. Text, paste, cursor keys, and Enter retain composer behavior. Friction captures `apprTyped` only after explicit `Alt+A`; Escape returns to the draft without deciding. Keep the editor rune/row bounded and reset it on head changes. Clarify questions capture the diff --git a/cmd/bodek/bughunt_regress_test.go b/cmd/bodek/bughunt_regress_test.go index 5e832ee..63c38bc 100644 --- a/cmd/bodek/bughunt_regress_test.go +++ b/cmd/bodek/bughunt_regress_test.go @@ -3,6 +3,7 @@ package main import ( "errors" "flag" + "fmt" "io" "os" "path/filepath" @@ -88,3 +89,28 @@ func TestRegressHelpPrintsUsageOnce(t *testing.T) { t.Fatalf("usage printed %d times", n) } } + +// Two instances saving different preferences at once keep both. +func TestRegressConcurrentSettingsSavesKeepBoth(t *testing.T) { + t.Setenv("BODEK_CONFIG", filepath.Join(t.TempDir(), "config.json")) + for i := 0; i < 20; i++ { + a, err := parseConfig(nil, io.Discard) + if err != nil { + t.Fatal(err) + } + b, _ := parseConfig(nil, io.Discard) + a.persist.Theme = fmt.Sprintf("theme-%d", i) + b.persist.Verbosity = fmt.Sprintf("v-%d", i) + done := make(chan struct{}) + go func() { _ = a.save(); close(done) }() + _ = b.save() + <-done + got, err := settings.Load() + if err != nil { + t.Fatal(err) + } + if got.Theme != a.persist.Theme || got.Verbosity != b.persist.Verbosity { + t.Fatalf("round %d lost a save: %+v", i, got) + } + } +} diff --git a/cmd/bodek/main.go b/cmd/bodek/main.go index 9de243b..6e99fbe 100644 --- a/cmd/bodek/main.go +++ b/cmd/bodek/main.go @@ -22,6 +22,7 @@ import ( "github.com/muesli/termenv" "github.com/BackendStack21/bodek/internal/client" + "github.com/BackendStack21/bodek/internal/filelock" "github.com/BackendStack21/bodek/internal/server" "github.com/BackendStack21/bodek/internal/settings" "github.com/BackendStack21/bodek/internal/tui" @@ -74,6 +75,8 @@ func (c *config) save() error { if c.persistDisabled { return fmt.Errorf("settings file could not be parsed; changes are not saved this run") } + // Another instance must not save between this load and the rename. + defer filelock.Lock(settings.Path())() cur, err := settings.Load() if err != nil { // A missing file loads clean; an error means the file became diff --git a/internal/client/bughunt_round3_test.go b/internal/client/bughunt_round3_test.go index d223b89..04457d1 100644 --- a/internal/client/bughunt_round3_test.go +++ b/internal/client/bughunt_round3_test.go @@ -53,7 +53,7 @@ func TestRegressPromptFrameBounds(t *testing.T) { } select { case n := <-got: - if n > maxFrameBytes { + if n > MaxFrameBytes { t.Fatalf("frame %d bytes exceeds the cap", n) } case <-time.After(5 * time.Second): @@ -90,3 +90,13 @@ func TestRegressCloseDoesNotWaitOutStuckWrite(t *testing.T) { } <-sent } + +func TestRegressCheckPromptSize(t *testing.T) { + if err := CheckPromptSize("hi", PromptOpts{}); err != nil { + t.Fatalf("small prompt refused: %v", err) + } + big := []Attachment{{Name: "a", Content: strings.Repeat("\n", 5<<20)}} // \n encodes to 2 bytes + if err := CheckPromptSize("hi", PromptOpts{Attachments: big}); !errors.Is(err, ErrFrameTooLarge) { + t.Fatalf("err = %v, want ErrFrameTooLarge", err) + } +} diff --git a/internal/client/client.go b/internal/client/client.go index fbe8b72..de626d8 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -536,7 +536,19 @@ type PromptOpts struct { // SendPrompt submits a task. Session continuity is automatic on a single // connection; SessionID+AuthToken resume a saved conversation. func (c *Client) SendPrompt(content string, opts PromptOpts) error { - return c.send(prompt{ + return c.send(promptFrame(content, opts)) +} + +// CheckPromptSize reports ErrFrameTooLarge for a prompt whose frame odek +// would refuse, so callers can keep the draft and attachments instead of +// committing a turn that cannot be sent. +func CheckPromptSize(content string, opts PromptOpts) error { + _, err := encodeFrame(promptFrame(content, opts)) + return err +} + +func promptFrame(content string, opts PromptOpts) prompt { + return prompt{ Type: "prompt", Content: content, Thinking: opts.Thinking, @@ -544,7 +556,7 @@ func (c *Client) SendPrompt(content string, opts PromptOpts) error { SessionID: opts.SessionID, AuthToken: opts.AuthToken, Attachments: opts.Attachments, - }) + } } // approval is the client→server response to an approval_request. @@ -627,32 +639,41 @@ func (c *Client) SendSkillPromptResponse(action, skillName string) error { // prompt on a slow link. var writeTimeout = 60 * time.Second -// maxFrameBytes is odek serve's inbound WebSocket message cap (8 MiB). A +// MaxFrameBytes is odek serve's inbound WebSocket message cap (8 MiB). A // larger frame makes odek drop the whole connection — the prompt is lost // behind a bare "connection lost" — so send refuses it with a reason. -const maxFrameBytes = 8 << 20 +const MaxFrameBytes = 8 << 20 // ErrFrameTooLarge reports a frame over odek's inbound message cap. var ErrFrameTooLarge = errors.New("message exceeds odek serve's 8 MiB frame limit") // send marshals v and writes it as one frame, serialized against other writers. func (c *Client) send(v any) error { + frame, err := encodeFrame(v) + if err != nil { + return err + } + c.wmu.Lock() + defer c.wmu.Unlock() + _ = c.conn.SetWriteDeadline(time.Now().Add(writeTimeout)) + return ws.Message.Send(c.conn, string(frame)) // string → text frame, as JSON.Send +} + +// encodeFrame marshals one wire frame, refusing one over MaxFrameBytes. +func encodeFrame(v any) ([]byte, error) { // No HTML escaping: < > & would each grow to six bytes, so attached // source files could blow past the frame cap that their size passed. var buf bytes.Buffer enc := json.NewEncoder(&buf) enc.SetEscapeHTML(false) if err := enc.Encode(v); err != nil { - return err + return nil, err } frame := bytes.TrimSuffix(buf.Bytes(), []byte("\n")) - if len(frame) > maxFrameBytes { - return fmt.Errorf("%w (%.1f MiB encoded) — attach fewer or smaller files", ErrFrameTooLarge, float64(len(frame))/(1<<20)) + if len(frame) > MaxFrameBytes { + return nil, fmt.Errorf("%w (%.1f MiB encoded) — attach fewer or smaller files", ErrFrameTooLarge, float64(len(frame))/(1<<20)) } - c.wmu.Lock() - defer c.wmu.Unlock() - _ = c.conn.SetWriteDeadline(time.Now().Add(writeTimeout)) - return ws.Message.Send(c.conn, string(frame)) // string → text frame, as JSON.Send + return frame, nil } // closeGrace bounds Close's close-frame write. A send stuck on a half-open diff --git a/internal/filelock/filelock.go b/internal/filelock/filelock.go new file mode 100644 index 0000000..32fd87b --- /dev/null +++ b/internal/filelock/filelock.go @@ -0,0 +1,60 @@ +// Package filelock serializes read-merge-write cycles on bodek's shared +// stores (~/.bodek tokens, workspace, settings) across processes. Each store +// already merges the on-disk state before replacing it, but two instances +// could still interleave between the read and the rename and drop the +// other's update. An advisory lock on a sidecar ".lock" closes that +// window; the OS releases it if the holder dies. +package filelock + +import ( + "os" + "path/filepath" + "time" +) + +// wait bounds how long Lock blocks on a peer. Persistence is best-effort +// and must never hang the TUI, so after it the write proceeds unlocked. +var wait = 2 * time.Second + +// tryLockFn is tryLock, swappable so tests can fake an unsupported mount. +var tryLockFn = tryLock + +// retryEvery is the polling interval while a peer holds the lock. +const retryEvery = 10 * time.Millisecond + +// Lock takes the exclusive lock for path and returns its release. It never +// fails: when the lock file cannot be opened or a peer holds it past wait, +// the returned release is a no-op and the caller proceeds as before. +func Lock(path string) (unlock func()) { + if path == "" { + return func() {} + } + // Two links to one store must share its lock; the very first write + // must not run unlocked for want of the directory. + if target, err := filepath.EvalSymlinks(path); err == nil { + path = target + } + _ = os.MkdirAll(filepath.Dir(path), 0o700) + f, err := os.OpenFile(path+".lock", os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return func() {} + } + deadline := time.Now().Add(wait) + for { + acquired, busy := tryLockFn(f) + if acquired { + return func() { + unlockFile(f) + _ = f.Close() + } + } + // Only a peer holding the lock is worth waiting for: a filesystem + // without locking (NFS nolocks, some SMB/FUSE) fails every try, and + // spinning there would stall each save by the full wait. + if !busy || time.Now().After(deadline) { + _ = f.Close() + return func() {} + } + time.Sleep(retryEvery) + } +} diff --git a/internal/filelock/filelock_test.go b/internal/filelock/filelock_test.go new file mode 100644 index 0000000..bd5fae9 --- /dev/null +++ b/internal/filelock/filelock_test.go @@ -0,0 +1,105 @@ +package filelock + +import ( + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +// Lock excludes a second holder until released (flock locks are per open +// file description, so two Locks in one process still contend). +func TestLockExcludesUntilReleased(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.json") + unlock := Lock(path) + got := make(chan struct{}) + go func() { + u := Lock(path) + close(got) + u() + }() + select { + case <-got: + t.Fatal("second Lock acquired while the first was held") + case <-time.After(200 * time.Millisecond): + } + unlock() + select { + case <-got: + case <-time.After(wait + time.Second): + t.Fatal("second Lock never acquired after release") + } +} + +// A peer holding the lock past wait degrades to an unlocked write instead +// of hanging persistence. +func TestLockGivesUpAfterWait(t *testing.T) { + old := wait + wait = 50 * time.Millisecond + t.Cleanup(func() { wait = old }) + path := filepath.Join(t.TempDir(), "store.json") + unlock := Lock(path) + defer unlock() + start := time.Now() + Lock(path)() + if time.Since(start) > 2*time.Second { + t.Fatal("Lock blocked far past its wait") + } +} + +// An unopenable lock path and an empty path are no-ops, never panics. +func TestLockBestEffort(t *testing.T) { + Lock("")() + file := filepath.Join(t.TempDir(), "file") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + Lock(filepath.Join(file, "store.json"))() // parent is a file: cannot open +} + +// Serialized increments from many goroutines never lose an update. +func TestLockSerializesReadModifyWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "counter") + if err := os.WriteFile(path, nil, 0o600); err != nil { + t.Fatal(err) + } + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func() { + defer wg.Done() + defer Lock(path)() + b, _ := os.ReadFile(path) + _ = os.WriteFile(path, append(b, 'x'), 0o600) + }() + } + wg.Wait() + b, _ := os.ReadFile(path) + if n := strings.Count(string(b), "x"); n != 20 { + t.Fatalf("lost updates: %d of 20", n) + } +} + +// A filesystem without locking fails every try: Lock must give up at once +// rather than stall each save by the full wait. +func TestLockUnsupportedReturnsImmediately(t *testing.T) { + old := tryLockFn + tryLockFn = func(*os.File) (bool, bool) { return false, false } + t.Cleanup(func() { tryLockFn = old }) + start := time.Now() + Lock(filepath.Join(t.TempDir(), "store.json"))() + if d := time.Since(start); d > wait/2 { + t.Fatalf("Lock spun %v on an unsupported filesystem", d) + } +} + +// The first write creates the store directory, so it is locked too. +func TestLockCreatesMissingDir(t *testing.T) { + path := filepath.Join(t.TempDir(), "new", "store.json") + defer Lock(path)() + if _, err := os.Stat(path + ".lock"); err != nil { + t.Fatalf("lock file not created: %v", err) + } +} diff --git a/internal/filelock/filelock_unix.go b/internal/filelock/filelock_unix.go new file mode 100644 index 0000000..0961893 --- /dev/null +++ b/internal/filelock/filelock_unix.go @@ -0,0 +1,23 @@ +//go:build unix + +package filelock + +import ( + "errors" + "os" + "syscall" +) + +// tryLock reports whether the lock was taken, and if not whether a peer +// holds it (worth retrying) rather than locking being unsupported. +func tryLock(f *os.File) (acquired, busy bool) { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return true, false + } + return false, errors.Is(err, syscall.EWOULDBLOCK) || errors.Is(err, syscall.EAGAIN) || errors.Is(err, syscall.EINTR) +} + +func unlockFile(f *os.File) { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) +} diff --git a/internal/filelock/filelock_windows.go b/internal/filelock/filelock_windows.go new file mode 100644 index 0000000..a5f26ab --- /dev/null +++ b/internal/filelock/filelock_windows.go @@ -0,0 +1,27 @@ +//go:build windows + +package filelock + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// tryLock reports whether the lock was taken, and if not whether a peer +// holds it (worth retrying) rather than locking being unsupported. +func tryLock(f *os.File) (acquired, busy bool) { + var ol windows.Overlapped + err := windows.LockFileEx(windows.Handle(f.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, &ol) + if err == nil { + return true, false + } + return false, errors.Is(err, windows.ERROR_LOCK_VIOLATION) || errors.Is(err, windows.ERROR_IO_PENDING) +} + +func unlockFile(f *os.File) { + var ol windows.Overlapped + _ = windows.UnlockFileEx(windows.Handle(f.Fd()), 0, 1, 0, &ol) +} diff --git a/internal/server/pgroup_unix.go b/internal/server/pgroup_unix.go index b989c3d..d3c9b6b 100644 --- a/internal/server/pgroup_unix.go +++ b/internal/server/pgroup_unix.go @@ -31,3 +31,6 @@ func (c *Conn) signalServer(sig syscall.Signal) { _ = c.proc.Process.Signal(sig) } } + +// guardChild is a no-op here: the re-exec watchdog guards against orphaning. +func (c *Conn) guardChild() {} diff --git a/internal/server/pgroup_windows.go b/internal/server/pgroup_windows.go index 95dfbfb..53a2b46 100644 --- a/internal/server/pgroup_windows.go +++ b/internal/server/pgroup_windows.go @@ -6,18 +6,66 @@ import ( "os" "os/exec" "syscall" + "unsafe" + + "golang.org/x/sys/windows" ) -// watchdogArg is unused on windows: there is no process-group signalling, -// so no orphan guard is spawned. +// watchdogArg is unused on windows: the kill-on-close Job Object from +// guardChild replaces the re-exec orphan guard. const watchdogArg = "__bodek-watchdog" -// setPgroup is a no-op on windows. -func setPgroup(*exec.Cmd) {} +// setPgroup starts the child in its own console process group so a +// CTRL_BREAK can target it alone (the group id is its pid). The group also +// stops console Ctrl+C from reaching odek; guardChild makes sure a bodek +// that dies on one still takes odek with it. +func setPgroup(cmd *exec.Cmd) { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.CreationFlags |= windows.CREATE_NEW_PROCESS_GROUP +} + +// guardChild assigns the child to a Job Object that kills it on close. +// bodek holds the only handle, so if it dies for any reason the OS closes +// the handle and terminates odek (and the children it starts after this +// point). Stop closes it once the child is gone, sweeping leftovers. +// Best effort: on any failure the child just runs unguarded, as before. +func (c *Conn) guardChild() { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return + } + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil { + _ = windows.CloseHandle(job) + return + } + h, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(c.proc.Process.Pid)) + if err != nil { + _ = windows.CloseHandle(job) + return + } + err = windows.AssignProcessToJobObject(job, h) + _ = windows.CloseHandle(h) + if err != nil { + _ = windows.CloseHandle(job) + return + } + c.watchMu.Lock() + c.watch = func() { _ = windows.CloseHandle(job) } + c.watchMu.Unlock() +} -// signalServer falls back to bare-process signalling on windows (no -// process groups); Kill maps to Process.Kill, anything else to Interrupt -// as before. +// signalServer maps SIGKILL to Process.Kill. Anything else becomes a +// CTRL_BREAK to the child's group, which a Go odek receives as +// os.Interrupt — Process.Signal(os.Interrupt) is unsupported on windows +// and always fails, so every Stop waited out the grace and force-killed. func (c *Conn) signalServer(sig syscall.Signal) { if c.proc == nil || c.proc.Process == nil { return @@ -26,5 +74,8 @@ func (c *Conn) signalServer(sig syscall.Signal) { _ = c.proc.Process.Kill() return } + if windows.GenerateConsoleCtrlEvent(windows.CTRL_BREAK_EVENT, uint32(c.proc.Process.Pid)) == nil { + return + } _ = c.proc.Process.Signal(os.Interrupt) } diff --git a/internal/server/pgroup_windows_test.go b/internal/server/pgroup_windows_test.go new file mode 100644 index 0000000..27a2c48 --- /dev/null +++ b/internal/server/pgroup_windows_test.go @@ -0,0 +1,107 @@ +//go:build windows + +package server + +import ( + "bufio" + "fmt" + "os" + "os/exec" + "os/signal" + "strings" + "syscall" + "testing" + "time" + + "golang.org/x/sys/windows" +) + +// TestWindowsHelperChild is the stand-in odek: re-exec'd by the tests +// below, it reports readiness and exits 0 on os.Interrupt. +func TestWindowsHelperChild(t *testing.T) { + if os.Getenv("BODEK_WIN_CHILD") == "" { + t.Skip("helper process only") + } + ch := make(chan os.Signal, 1) + signal.Notify(ch, os.Interrupt) + fmt.Println("ready") + select { + case <-ch: + os.Exit(0) + case <-time.After(30 * time.Second): + os.Exit(3) + } +} + +func startHelper(t *testing.T) *exec.Cmd { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=^TestWindowsHelperChild$") + cmd.Env = append(os.Environ(), "BODEK_WIN_CHILD=1") + setPgroup(cmd) + out, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = cmd.Process.Kill() }) + line, _ := bufio.NewReader(out).ReadString('\n') + if !strings.Contains(line, "ready") { + t.Fatalf("helper not ready: %q", line) + } + return cmd +} + +func waitExit(t *testing.T, cmd *exec.Cmd) error { + t.Helper() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + return err + case <-time.After(10 * time.Second): + t.Fatal("child did not exit") + return nil + } +} + +// The child needs its own console group, or CTRL_BREAK cannot target it. +func TestWindowsSetPgroupNewConsoleGroup(t *testing.T) { + cmd := exec.Command("cmd") + setPgroup(cmd) + if cmd.SysProcAttr == nil || cmd.SysProcAttr.CreationFlags&windows.CREATE_NEW_PROCESS_GROUP == 0 { + t.Fatal("child not started in a new process group") + } +} + +// signalServer's graceful path reaches a Go child as os.Interrupt, so +// Stop no longer waits out the grace and force-kills on windows. +func TestWindowsGracefulSignalReachesChild(t *testing.T) { + cmd := startHelper(t) + if err := windows.GenerateConsoleCtrlEvent(windows.CTRL_BREAK_EVENT, uint32(cmd.Process.Pid)); err != nil { + t.Skipf("no console to deliver ctrl events: %v", err) + } + if err := waitExit(t, cmd); err != nil { + t.Fatalf("child did not exit cleanly on CTRL_BREAK: %v", err) + } + cmd = startHelper(t) + (&Conn{proc: cmd}).signalServer(syscall.SIGINT) + if err := waitExit(t, cmd); err != nil { + t.Fatalf("signalServer did not stop the child gracefully: %v", err) + } +} + +// Closing the guard's job handle (what the OS does when bodek dies) +// terminates the child: no orphaned odek. +func TestWindowsJobKillsChildOnClose(t *testing.T) { + cmd := startHelper(t) + c := &Conn{proc: cmd} + c.guardChild() + if c.watch == nil { + t.Fatal("job object guard not installed") + } + c.watch() + _ = waitExit(t, cmd) // fails the test if the child outlives its job + +} diff --git a/internal/server/server.go b/internal/server/server.go index 6a243d2..12f7c08 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -233,6 +233,9 @@ func (c *Conn) spawn(opts Options, addr string) error { return fmt.Errorf("start odek serve: %w", err) } c.proc = cmd + // Before the reaper: on windows Wait releases the process handle, and + // a recycled pid must never land in the kill-on-close job. + c.guardChild() c.startReaper() c.startWatchdog() return nil diff --git a/internal/settings/settings.go b/internal/settings/settings.go index 3ae97d7..13f3c26 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -78,6 +78,11 @@ func Save(s Settings) error { if err != nil { return err } + // Write through a symlinked config.json (dotfile managers): the rename + // would otherwise replace the link with a regular file. + if target, err := filepath.EvalSymlinks(path); err == nil { + path = target + } // Staged tmp + rename: a crash mid plain WriteFile left a truncated // config.json, which Load reports as malformed — every setting reset. tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*.tmp") diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index e536233..fd3f0fa 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" ) @@ -189,3 +190,26 @@ func TestBODEKConfigOverride(t *testing.T) { } func ptr(b bool) *bool { return &b } + +// A symlinked config.json (dotfile managers) stays a symlink across saves. +func TestRegressSymlinkedConfigSurvivesSave(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "real.json") + link := filepath.Join(dir, "config.json") + if err := os.WriteFile(real, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, link); err != nil { + t.Skip("symlinks unsupported") + } + t.Setenv("BODEK_CONFIG", link) + if err := Save(Settings{Theme: "classic"}); err != nil { + t.Fatal(err) + } + if fi, err := os.Lstat(link); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Fatal("save replaced the symlink with a regular file") + } + if b, _ := os.ReadFile(real); !strings.Contains(string(b), "classic") { + t.Fatal("settings not written through the link") + } +} diff --git a/internal/tokens/crossprocess_test.go b/internal/tokens/crossprocess_test.go new file mode 100644 index 0000000..84be760 --- /dev/null +++ b/internal/tokens/crossprocess_test.go @@ -0,0 +1,53 @@ +package tokens + +import ( + "fmt" + "os" + "path/filepath" + "sync" + "testing" +) + +// Two instances persisting concurrently must not drop each other's tokens: +// the in-process mutex cannot order writes across processes. +func TestRegressConcurrentInstancesKeepEveryToken(t *testing.T) { + path := filepath.Join(t.TempDir(), "tokens.json") + a, b := openAt(path), openAt(path) + var wg sync.WaitGroup + for _, s := range []struct { + st *Store + prefix string + }{{a, "a"}, {b, "b"}} { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 40; i++ { + s.st.Set(fmt.Sprintf("%s%d", s.prefix, i), "tok") + } + }() + } + wg.Wait() + if got := len(openAt(path).m); got != 80 { + t.Fatalf("store kept %d of 80 tokens", got) + } +} + +// A symlinked store (dotfile managers) stays a symlink across saves. +func TestRegressSymlinkedStoreSurvivesSave(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "real.json") + link := filepath.Join(dir, "tokens.json") + if err := os.WriteFile(real, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, link); err != nil { + t.Skip("symlinks unsupported") + } + openAt(link).Set("s1", "tok") + if fi, err := os.Lstat(link); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Fatal("save replaced the symlink with a regular file") + } + if openAt(real).Get("s1") != "tok" { + t.Fatal("token not written through the link") + } +} diff --git a/internal/tokens/tokens.go b/internal/tokens/tokens.go index c4cf401..01cda7b 100644 --- a/internal/tokens/tokens.go +++ b/internal/tokens/tokens.go @@ -16,6 +16,8 @@ import ( "os" "path/filepath" "sync" + + "github.com/BackendStack21/bodek/internal/filelock" ) // Store is a concurrency-safe session-id → token map with best-effort @@ -92,6 +94,7 @@ func (s *Store) Set(id, token string) { if s.m[id] == token { return // no change; skip the disk write } + defer filelock.Lock(s.path)() // a peer must not interleave between merge and rename s.mergeLocked() s.m[id] = token s.markPendingLocked(id, token) @@ -108,6 +111,7 @@ func (s *Store) Delete(id string) { if _, ok := s.m[id]; !ok { return } + defer filelock.Lock(s.path)() // a peer must not interleave between merge and rename s.mergeLocked() delete(s.m, id) s.markPendingLocked(id, "") @@ -181,6 +185,11 @@ func persist(path string, m map[string]string) error { if err != nil { return fmt.Errorf("encode store: %w", err) } + // Write through a symlinked store (dotfile managers): the rename + // would otherwise replace the link with a regular file. + if target, err := filepath.EvalSymlinks(path); err == nil { + path = target + } // A per-call staged name: callers persist after releasing the store // mutex, so a shared path+'.tmp' let two interleaved writes tear the // file (A.Write → B.Write truncates → A.Rename) and silently wipe diff --git a/internal/tui/attach.go b/internal/tui/attach.go index 785bbcb..128b16c 100644 --- a/internal/tui/attach.go +++ b/internal/tui/attach.go @@ -135,7 +135,7 @@ func (m *Model) unattachFile(name string) tea.Cmd { m.attachments = nil m.attachPaths = nil m.persistLocal() - return m.transientNoteCmd("dropped " + plural(n, "staged file", "staged files")) + return tea.Batch(m.transientNoteCmd("dropped "+plural(n, "staged file", "staged files")), m.retryRefusedQueue()) } for i := range m.attachments { if m.attachments[i].Name == name { @@ -144,12 +144,21 @@ func (m *Model) unattachFile(name string) tea.Cmd { m.attachPaths = append(m.attachPaths[:i], m.attachPaths[i+1:]...) } m.persistLocal() - return m.transientNoteCmd("dropped " + name) + return tea.Batch(m.transientNoteCmd("dropped "+name), m.retryRefusedQueue()) } } return m.transientNoteCmd("not staged: " + name) } +// retryRefusedQueue re-drains a queue whose head was refused as too large, +// now that the staged files shrank — no unrelated event needs to arrive. +func (m *Model) retryRefusedQueue() tea.Cmd { + if m.qRefused == "" { + return nil + } + return m.sendQueued() +} + // expandTilde resolves a leading ~ to the user's home directory. func expandTilde(p string) string { if p == "~" || strings.HasPrefix(p, "~/") { diff --git a/internal/tui/bughunt_round3_test.go b/internal/tui/bughunt_round3_test.go index 44f6d0a..7dad4ed 100644 --- a/internal/tui/bughunt_round3_test.go +++ b/internal/tui/bughunt_round3_test.go @@ -344,3 +344,33 @@ func TestRegressQueueTailRowHasNoControls(t *testing.T) { t.Fatalf("tail click acted on a row: len=%d qsel=%d", len(m.queue), m.qsel) } } + +// A prompt odek would refuse (frame over 8 MiB) is never committed: the +// draft, the staged files and the queue all survive for a trim. +func TestRegressOversizedPromptKeepsDraftAndQueue(t *testing.T) { + m := newTestModel() + big := client.Attachment{Name: "big.txt", Content: strings.Repeat("x", 9<<20)} + m.attachments = []client.Attachment{big} + m.ta.SetValue("review this") + m.submit() + if m.ta.Value() != "review this" || len(m.attachments) != 1 || len(m.msgs) != 0 || m.busy { + t.Fatalf("oversized submit committed: draft=%q att=%d msgs=%d busy=%v", + m.ta.Value(), len(m.attachments), len(m.msgs), m.busy) + } + m.queue = []string{"queued"} + if m.sendQueued() == nil { + t.Fatal("refused drain gave no note") + } + if len(m.queue) != 1 || len(m.msgs) != 0 { + t.Fatalf("oversized drain popped the queue: queue=%d msgs=%d", len(m.queue), len(m.msgs)) + } + // Idle events (pongs) re-run the drain: no re-encode, no note spam. + if m.sendQueued() != nil { + t.Fatal("unchanged refused head re-noted") + } + // Dropping the files drains the head at once. + m.unattachFile("big.txt") + if len(m.queue) != 0 || !m.busy { + t.Fatalf("unattach did not drain the refused head: queue=%d busy=%v", len(m.queue), m.busy) + } +} diff --git a/internal/tui/input.go b/internal/tui/input.go index ea7ee3f..a35d443 100644 --- a/internal/tui/input.go +++ b/internal/tui/input.go @@ -1,6 +1,7 @@ package tui import ( + "errors" "fmt" "os" "reflect" @@ -504,15 +505,68 @@ func (m *Model) submit() tea.Cmd { } return m.transientNoteCmd("queued — it sends when the turn ends") } + if note := m.promptTooLarge(text); note != nil { + return note // the draft and the staged files stay for a trim + } m.ta.Reset() m.syncComposer() m.closeAC() return m.sendPrompt(text) } +// promptTooLarge preflights the prompt frame against odek's inbound cap +// and returns the explaining note when odek would refuse it. Sending +// anyway would drop the connection, the staged files and the queue drain. +func (m *Model) promptTooLarge(text string) tea.Cmd { + // Fast path: JSON grows a byte by at most 6x (\u00XX), so a prompt + // that small cannot reach the cap — skip encoding it on every drain. + raw := len(text) + for _, a := range m.attachments { + raw += len(a.Name) + len(a.Content) + } + if raw*6+4096 < client.MaxFrameBytes { + return nil + } + opts := m.promptOpts() + err := client.CheckPromptSize(text, opts) + if !errors.Is(err, client.ErrFrameTooLarge) { + return nil + } + hint := " · /unattach drops files" + opts.Attachments = nil + if errors.Is(client.CheckPromptSize(text, opts), client.ErrFrameTooLarge) { + hint = " · the text alone is too long — shorten it" + } + return m.transientNoteCmd("not sent: " + err.Error() + hint) +} + +// queueSendKey identifies what the queue head would send: its text plus +// the staged files' count and size. +func (m *Model) queueSendKey() string { + n := 0 + for _, a := range m.attachments { + n += len(a.Name) + len(a.Content) + } + return fmt.Sprintf("%d/%d\x00%s", len(m.attachments), n, m.queue[0]) +} + +// promptOpts are the wire options the next prompt carries. +func (m *Model) promptOpts() client.PromptOpts { + return client.PromptOpts{ + Thinking: thinkingWire(m.thinking), + Model: m.pendModel, + SessionID: m.sessionID, + AuthToken: m.authToken, + Attachments: m.attachments, + } +} + // sendPrompt appends the user/assistant pair to the transcript, records the // prompt in the history ring, and dispatches it to the server. func (m *Model) sendPrompt(text string) tea.Cmd { + if note := m.promptTooLarge(text); note != nil { + return note // nothing committed: no transcript card, no busy state + } m.lastEvent = time.Now() // a fresh turn must not inherit the idle gap as a stale age m.lastPrompt = text m.recordHistory(text) @@ -555,14 +609,7 @@ func (m *Model) sendPrompt(text string) tea.Cmd { // only sticks when already at the bottom). m.vp.GotoBottom() - thinking := thinkingWire(m.thinking) - opts := client.PromptOpts{ - Thinking: thinking, - Model: m.pendModel, - SessionID: m.sessionID, - AuthToken: m.authToken, - Attachments: m.attachments, - } + opts := m.promptOpts() // Attachments are per-prompt by contract; the next send starts clean. m.attachments = nil m.attachPaths = nil @@ -593,6 +640,17 @@ func (m *Model) sendQueued() tea.Cmd { if m.busy || m.disconn || m.resumeInFlight || len(m.queue) == 0 { return nil } + // The drain runs after every idle event (pongs included): re-encode and + // re-note a refused head only once it or the staged files change. + key := m.queueSendKey() + if key == m.qRefused { + return nil + } + if note := m.promptTooLarge(m.queue[0]); note != nil { + m.qRefused = key + return note // stays queued until it or its staged files shrink + } + m.qRefused = "" text := m.queue[0] m.queue = m.queue[1:] m.qsel = clampSel(m.qsel, len(m.queue)) diff --git a/internal/tui/model.go b/internal/tui/model.go index 7c43a29..8485e3e 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -265,6 +265,7 @@ type Model struct { qfocus bool // the queue strip owns the keyboard (ctrl+q) qsel int // selected strip row while qfocus qarm int // armed-for-delete row while qfocus (-1 = none) + qRefused string // queueSendKey of a head refused as too large (drain stays quiet until it changes) lastPrompt string // most recent prompt sent — /retry re-sends it ws *workspace.Store // nil in tests unless injected pendingResume string // cwd last-session id; consumed by resumeLast diff --git a/internal/tui/queue.go b/internal/tui/queue.go index 986b3f1..c0f4dd7 100644 --- a/internal/tui/queue.go +++ b/internal/tui/queue.go @@ -330,6 +330,9 @@ func (m *Model) queueSendSelected() tea.Cmd { } i := m.panelSel text := m.queue[i] + if note := m.promptTooLarge(text); note != nil { + return note + } m.queue = append(m.queue[:i], m.queue[i+1:]...) m.qsel = clampSel(m.qsel, len(m.queue)) m.panelSel = clampSel(m.panelSel, len(m.queue)) diff --git a/internal/workspace/crossprocess_test.go b/internal/workspace/crossprocess_test.go new file mode 100644 index 0000000..a0b965e --- /dev/null +++ b/internal/workspace/crossprocess_test.go @@ -0,0 +1,52 @@ +package workspace + +import ( + "fmt" + "os" + "path/filepath" + "sync" + "testing" +) + +// Two instances saving different directories concurrently keep both. +func TestRegressConcurrentInstancesKeepEveryWorkspace(t *testing.T) { + t.Setenv("BODEK_WORKSPACE", filepath.Join(t.TempDir(), "workspace.json")) + a, b := Open(), Open() + var wg sync.WaitGroup + for _, s := range []struct { + st *Store + prefix string + }{{a, "/a"}, {b, "/b"}} { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 30; i++ { + _ = s.st.Save(fmt.Sprintf("%s%d", s.prefix, i), State{Draft: "d"}) + } + }() + } + wg.Wait() + if got := len(Open().all); got != 60 { + t.Fatalf("workspace kept %d of 60 directories", got) + } +} + +// A symlinked workspace file stays a symlink across saves. +func TestRegressSymlinkedWorkspaceSurvivesSave(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "real.json") + link := filepath.Join(dir, "workspace.json") + if err := os.WriteFile(real, []byte(`{"workspaces":{}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, link); err != nil { + t.Skip("symlinks unsupported") + } + t.Setenv("BODEK_WORKSPACE", link) + if err := Open().Save("/x", State{Draft: "d"}); err != nil { + t.Fatal(err) + } + if fi, err := os.Lstat(link); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Fatal("save replaced the symlink with a regular file") + } +} diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 8cc435d..495e272 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -12,6 +12,8 @@ import ( "os" "path/filepath" "sync" + + "github.com/BackendStack21/bodek/internal/filelock" ) // State is the bodek-owned snapshot for one working directory. @@ -140,6 +142,7 @@ func (s *Store) Save(cwd string, st State) error { // unlocking lets two interleaved saves land out of order on disk. s.mu.Lock() defer s.mu.Unlock() + defer filelock.Lock(s.path)() // a peer must not interleave between merge and rename s.reloadLocked(cwd) s.all[cwd] = cloneState(st) return persist(s.path, cloneAll(s.all)) @@ -152,6 +155,7 @@ func (s *Store) Patch(cwd string, fn func(*State)) { } s.mu.Lock() defer s.mu.Unlock() + defer filelock.Lock(s.path)() // a peer must not interleave between merge and rename s.reloadLocked(cwd) st := cloneState(s.all[cwd]) fn(&st) @@ -189,6 +193,11 @@ func persist(path string, all map[string]State) error { if err != nil { return fmt.Errorf("encode workspace: %w", err) } + // Write through a symlinked store (dotfile managers): the rename + // would otherwise replace the link with a regular file. + if target, err := filepath.EvalSymlinks(path); err == nil { + path = target + } // A per-call staged name: callers persist after releasing the store // mutex, so a shared path+'.tmp' let two interleaved writes tear the // file (A.Write → B.Write truncates → A.Rename) — the same tear