From 27298130c004f9e2cd955000cecfd3198c6a1241 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 16:50:32 +0200 Subject: [PATCH 1/7] fix(store): serialize writers across instances and keep symlinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token, workspace and settings stores merge the file before replacing it, but two bodek instances could still interleave between the read and the rename and drop each other's update (a test lost half of 80 tokens). A new internal/filelock takes an advisory lock on .lock around the cycle — best-effort, released by the OS if the holder dies, and skipped after 2s so persistence never hangs the TUI. Saves also wrote the staged file over a symlinked store, replacing a dotfile-managed link with a regular file; they now write through it. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + cmd/bodek/bughunt_regress_test.go | 26 ++++++++ cmd/bodek/main.go | 3 + internal/filelock/filelock.go | 46 ++++++++++++++ internal/filelock/filelock_test.go | 79 +++++++++++++++++++++++++ internal/filelock/filelock_unix.go | 16 +++++ internal/filelock/filelock_windows.go | 20 +++++++ internal/settings/settings.go | 5 ++ internal/settings/settings_test.go | 24 ++++++++ internal/tokens/crossprocess_test.go | 53 +++++++++++++++++ internal/tokens/tokens.go | 9 +++ internal/workspace/crossprocess_test.go | 52 ++++++++++++++++ internal/workspace/workspace.go | 9 +++ 13 files changed, 343 insertions(+) create mode 100644 internal/filelock/filelock.go create mode 100644 internal/filelock/filelock_test.go create mode 100644 internal/filelock/filelock_unix.go create mode 100644 internal/filelock/filelock_windows.go create mode 100644 internal/tokens/crossprocess_test.go create mode 100644 internal/workspace/crossprocess_test.go diff --git a/AGENTS.md b/AGENTS.md index 95d3814..f2bb7e3 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 | 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/filelock/filelock.go b/internal/filelock/filelock.go new file mode 100644 index 0000000..6542d08 --- /dev/null +++ b/internal/filelock/filelock.go @@ -0,0 +1,46 @@ +// 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" + "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 + +// 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() {} + } + f, err := os.OpenFile(path+".lock", os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return func() {} + } + deadline := time.Now().Add(wait) + for { + if tryLock(f) { + return func() { + unlockFile(f) + _ = f.Close() + } + } + if 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..6104594 --- /dev/null +++ b/internal/filelock/filelock_test.go @@ -0,0 +1,79 @@ +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("")() + Lock(filepath.Join(t.TempDir(), "missing", "dir", "store.json"))() +} + +// 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) + } +} diff --git a/internal/filelock/filelock_unix.go b/internal/filelock/filelock_unix.go new file mode 100644 index 0000000..0e32f1c --- /dev/null +++ b/internal/filelock/filelock_unix.go @@ -0,0 +1,16 @@ +//go:build unix + +package filelock + +import ( + "os" + "syscall" +) + +func tryLock(f *os.File) bool { + return syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) == nil +} + +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..e08df25 --- /dev/null +++ b/internal/filelock/filelock_windows.go @@ -0,0 +1,20 @@ +//go:build windows + +package filelock + +import ( + "os" + + "golang.org/x/sys/windows" +) + +func tryLock(f *os.File) bool { + var ol windows.Overlapped + return windows.LockFileEx(windows.Handle(f.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, &ol) == nil +} + +func unlockFile(f *os.File) { + var ol windows.Overlapped + _ = windows.UnlockFileEx(windows.Handle(f.Fd()), 0, 1, 0, &ol) +} 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/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 From 80220a56d1d2e7236a84d105445b4e74bd6c9573 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 16:52:08 +0200 Subject: [PATCH 2/7] fix(tui): keep draft, files and queue when a prompt exceeds odek's cap An oversized prompt was committed (transcript card, busy state, staged files cleared) before the client refused the frame, and its errMsg never drained the queue. Submit, queue sends and drains now preflight the frame size with client.CheckPromptSize and leave everything in place behind an explaining note. Co-Authored-By: Claude Opus 5.5 --- internal/client/bughunt_round3_test.go | 12 ++++++- internal/client/client.go | 43 ++++++++++++++++------ internal/tui/bughunt_round3_test.go | 19 ++++++++++ internal/tui/input.go | 50 +++++++++++++++++++++----- internal/tui/queue.go | 3 ++ 5 files changed, 107 insertions(+), 20 deletions(-) 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/tui/bughunt_round3_test.go b/internal/tui/bughunt_round3_test.go index 44f6d0a..7043e93 100644 --- a/internal/tui/bughunt_round3_test.go +++ b/internal/tui/bughunt_round3_test.go @@ -344,3 +344,22 @@ 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"} + m.sendQueued() + 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)) + } +} diff --git a/internal/tui/input.go b/internal/tui/input.go index ea7ee3f..ffe3029 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,52 @@ 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 + } + err := client.CheckPromptSize(text, m.promptOpts()) + if errors.Is(err, client.ErrFrameTooLarge) { + return m.transientNoteCmd("not sent: " + err.Error() + " · /unattach drops files") + } + return nil +} + +// 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 +593,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 +624,9 @@ func (m *Model) sendQueued() tea.Cmd { if m.busy || m.disconn || m.resumeInFlight || len(m.queue) == 0 { return nil } + if note := m.promptTooLarge(m.queue[0]); note != nil { + return note // stays queued until its staged files are trimmed + } text := m.queue[0] m.queue = m.queue[1:] m.qsel = clampSel(m.qsel, len(m.queue)) 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)) From d5d975f5b938e270b6b6f7a64ac664045c82d9f6 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 16:52:33 +0200 Subject: [PATCH 3/7] docs: describe the approval bare-key fallback as implemented AGENTS.md promised ^U clears the draft over an approval card; the code, README and tests have ^U page the transcript there. Document the real path (Backspace / ^W) and the find/drawer-editor swallow rule. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f2bb7e3..c8d73bb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -237,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 From 9e4ba1e23b838c83f1c0b63ff8ba2c4b2f194e71 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 16:55:24 +0200 Subject: [PATCH 4/7] fix(server): stop odek gracefully on windows behind a job-object guard Process.Signal(os.Interrupt) is unsupported on windows, so every quit waited out the 12s grace and force-killed odek mid-cleanup. The child now starts in its own console process group and gets CTRL_BREAK, which Go delivers as os.Interrupt. The new group also hides console Ctrl+C from odek, and windows has no re-exec watchdog, so the child is placed in a kill-on-close Job Object whose only handle bodek holds: a bodek that dies for any reason takes odek with it. CI's windows leg now runs these process tests and the filelock suite. Co-Authored-By: Claude Opus 5.5 --- internal/server/pgroup_unix.go | 3 + internal/server/pgroup_windows.go | 65 +++++++++++++-- internal/server/pgroup_windows_test.go | 107 +++++++++++++++++++++++++ internal/server/server.go | 1 + 4 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 internal/server/pgroup_windows_test.go 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..636ee6d 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -235,6 +235,7 @@ func (c *Conn) spawn(opts Options, addr string) error { c.proc = cmd c.startReaper() c.startWatchdog() + c.guardChild() return nil } From b6980900b365ff84b88b157beb82ab350c9fcda9 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 17:02:27 +0200 Subject: [PATCH 5/7] fix(filelock): wait only on contention and lock the first write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tryLock treated every error as a busy peer, so a mount without locking (NFS nolocks, some SMB/FUSE) stalled each save — workspace persists run on the update loop — by the full 2s. Only EWOULDBLOCK / lock violation retries now. Lock also creates the store directory (the first write ran unlocked) and keys on the resolved path, so two links to one store share its lock. Co-Authored-By: Claude Opus 5.5 --- internal/filelock/filelock.go | 18 +++++++++++++++-- internal/filelock/filelock_test.go | 28 ++++++++++++++++++++++++++- internal/filelock/filelock_unix.go | 11 +++++++++-- internal/filelock/filelock_windows.go | 13 ++++++++++--- 4 files changed, 62 insertions(+), 8 deletions(-) diff --git a/internal/filelock/filelock.go b/internal/filelock/filelock.go index 6542d08..32fd87b 100644 --- a/internal/filelock/filelock.go +++ b/internal/filelock/filelock.go @@ -8,6 +8,7 @@ package filelock import ( "os" + "path/filepath" "time" ) @@ -15,6 +16,9 @@ import ( // 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 @@ -25,19 +29,29 @@ 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 { - if tryLock(f) { + acquired, busy := tryLockFn(f) + if acquired { return func() { unlockFile(f) _ = f.Close() } } - if time.Now().After(deadline) { + // 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() {} } diff --git a/internal/filelock/filelock_test.go b/internal/filelock/filelock_test.go index 6104594..bd5fae9 100644 --- a/internal/filelock/filelock_test.go +++ b/internal/filelock/filelock_test.go @@ -52,7 +52,11 @@ func TestLockGivesUpAfterWait(t *testing.T) { // An unopenable lock path and an empty path are no-ops, never panics. func TestLockBestEffort(t *testing.T) { Lock("")() - Lock(filepath.Join(t.TempDir(), "missing", "dir", "store.json"))() + 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. @@ -77,3 +81,25 @@ func TestLockSerializesReadModifyWrite(t *testing.T) { 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 index 0e32f1c..0961893 100644 --- a/internal/filelock/filelock_unix.go +++ b/internal/filelock/filelock_unix.go @@ -3,12 +3,19 @@ package filelock import ( + "errors" "os" "syscall" ) -func tryLock(f *os.File) bool { - return syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) == nil +// 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) { diff --git a/internal/filelock/filelock_windows.go b/internal/filelock/filelock_windows.go index e08df25..a5f26ab 100644 --- a/internal/filelock/filelock_windows.go +++ b/internal/filelock/filelock_windows.go @@ -3,15 +3,22 @@ package filelock import ( + "errors" "os" "golang.org/x/sys/windows" ) -func tryLock(f *os.File) bool { +// 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 - return windows.LockFileEx(windows.Handle(f.Fd()), - windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, &ol) == nil + 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) { From f8e082c517eb9a6a6042ab8a65573734fcb5bc76 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 17:02:27 +0200 Subject: [PATCH 6/7] fix(tui): note a refused queue head once and drain it after unattach The drain runs after every idle event, so an oversized queue head was re-encoded and re-noted on each heartbeat. The refusal is remembered until the head or the staged files change, /unattach re-drains at once, and the note says when the text alone is too long. Co-Authored-By: Claude Opus 5.5 --- internal/tui/attach.go | 13 +++++++++-- internal/tui/bughunt_round3_test.go | 13 ++++++++++- internal/tui/input.go | 34 ++++++++++++++++++++++++----- internal/tui/model.go | 1 + 4 files changed, 53 insertions(+), 8 deletions(-) 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 7043e93..7dad4ed 100644 --- a/internal/tui/bughunt_round3_test.go +++ b/internal/tui/bughunt_round3_test.go @@ -358,8 +358,19 @@ func TestRegressOversizedPromptKeepsDraftAndQueue(t *testing.T) { m.ta.Value(), len(m.attachments), len(m.msgs), m.busy) } m.queue = []string{"queued"} - m.sendQueued() + 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 ffe3029..a35d443 100644 --- a/internal/tui/input.go +++ b/internal/tui/input.go @@ -527,11 +527,27 @@ func (m *Model) promptTooLarge(text string) tea.Cmd { if raw*6+4096 < client.MaxFrameBytes { return nil } - err := client.CheckPromptSize(text, m.promptOpts()) - if errors.Is(err, client.ErrFrameTooLarge) { - return m.transientNoteCmd("not sent: " + err.Error() + " · /unattach drops files") + opts := m.promptOpts() + err := client.CheckPromptSize(text, opts) + if !errors.Is(err, client.ErrFrameTooLarge) { + return nil } - 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. @@ -624,9 +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 { - return note // stays queued until its staged files are trimmed + 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 From c1c34107a4d4c2fe22e56f6e48cbd353aacc4541 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 17 Oct 2026 17:02:27 +0200 Subject: [PATCH 7/7] fix(server): guard the windows child before the reaper; run its tests guardChild opened the child by pid after the reaper's Wait could have released the handle, so a child that exited at once risked a recycled pid landing in the kill-on-close job. It now runs first. The CI step that runs the windows process and filelock tests was also missing from the previous commit. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 4 ++++ internal/server/server.go | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) 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/internal/server/server.go b/internal/server/server.go index 636ee6d..12f7c08 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -233,9 +233,11 @@ 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() - c.guardChild() return nil }