Skip to content

fix(acp): log session persistence failures instead of discarding them #493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
euxaristia wants to merge 1 commit into from
+48 −4
Closed

fix(acp): log session persistence failures instead of discarding them #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions internal/acp/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"os"
"strings"
"sync"

Expand Down Expand Up @@ -83,7 +85,7 @@
}

// Serve runs the connection read loop until the stream closes or ctx is done.
func (a *Agent) Serve(ctx context.Context) error { return a.conn.Serve(ctx) }

Check failure on line 88 in internal/acp/agent.go

View workflow job for this annotation

GitHub Actions / Security & code health 

unreachable func: Agent.Serve

// ---- initialize ----

Expand Down Expand Up @@ -388,15 +390,21 @@

func (a *Agent) persistTurn(sess *acpSession, user, assistant string) {
if a.deps.Store != nil {
_, _ = a.deps.Store.AppendEvent(sess.id, sessions.AppendEventInput{
if _, err := a.deps.Store.AppendEvent(sess.id, sessions.AppendEventInput{
Type: sessions.EventMessage,
Payload: map[string]any{"role": "user", "content": user},
})
}); err != nil {
// best-effort; log at least so we notice history loss
// (real fix would surface to transcript / user)
fmt.Fprintf(os.Stderr, "warning: failed to persist user turn: %v\n", err)
}
if assistant != "" {
_, _ = a.deps.Store.AppendEvent(sess.id, sessions.AppendEventInput{
if _, err := a.deps.Store.AppendEvent(sess.id, sessions.AppendEventInput{
Type: sessions.EventMessage,
Payload: map[string]any{"role": "assistant", "content": assistant},
})
}); err != nil {
fmt.Fprintf(os.Stderr, "warning: failed to persist assistant turn: %v\n", err)
}
}
}
sess.appendHistory(turnRecord{user: user, assistant: assistant})
Expand Down
36 changes: 36 additions & 0 deletions internal/acp/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"io"
"os"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -165,6 +166,41 @@ func TestACPUnknownSessionPromptErrors(t *testing.T) {
}
}

// TestPersistTurnLogsAppendEventFailures proves that a persistence failure
// (previously silently discarded via `_, _ = ...AppendEvent(...)`) now gets
// logged instead of vanishing without a trace. An invalid session id makes
// AppendEvent fail deterministically without needing to break the store.
func TestPersistTurnLogsAppendEventFailures(t *testing.T) {
store := sessions.NewStore(sessions.StoreOptions{RootDir: t.TempDir()})
a := &Agent{deps: Deps{Store: store}}
sess := &acpSession{id: "not a valid session id!!"}

origStderr := os.Stderr
read, write, err := os.Pipe()
if err != nil {
t.Fatalf("os.Pipe: %v", err)
}
os.Stderr = write

a.persistTurn(sess, "hello", "world")

if err := write.Close(); err != nil {
t.Fatalf("close pipe writer: %v", err)
}
os.Stderr = origStderr

captured, err := io.ReadAll(read)
if err != nil {
t.Fatalf("read captured stderr: %v", err)
}
output := string(captured)
Comment on lines +178 to +196

@coderabbitai coderabbitai Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

os.Stderr isn't restored if any t.Fatalf fires between redirect and restore.

os.Stderr = write happens at Line 183, but restoration at Line 190 only runs after the write.Close() error check at Line 187-189. t.Fatalf calls runtime.Goexit, not os.Exit, so if write.Close() (or the earlier os.Pipe() at Line 179-182) fails, os.Stderr is left pointed at a closed/dangling pipe for the rest of the test binary — silently breaking output capture or crashing subsequent tests in the package. Restore via defer immediately after the swap, as is standard practice for this pattern.

🔧 Proposed fix 
 	origStderr := os.Stderr
 	read, write, err := os.Pipe()
 	if err != nil {
 		t.Fatalf("os.Pipe: %v", err)
 	}
 	os.Stderr = write
+	defer func() { os.Stderr = origStderr }()
 
 	a.persistTurn(sess, "hello", "world")
 
 	if err := write.Close(); err != nil {
 		t.Fatalf("close pipe writer: %v", err)
 	}
-	os.Stderr = origStderr
 
 	captured, err := io.ReadAll(read)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
origStderr := os.Stderr
read, write, err := os.Pipe()
if err != nil {
t.Fatalf("os.Pipe: %v", err)
}
os.Stderr = write
a.persistTurn(sess, "hello", "world")
if err := write.Close(); err != nil {
t.Fatalf("close pipe writer: %v", err)
}
os.Stderr = origStderr
captured, err := io.ReadAll(read)
if err != nil {
t.Fatalf("read captured stderr: %v", err)
}
output := string(captured)
origStderr := os.Stderr
read, write, err := os.Pipe()
if err != nil {
t.Fatalf("os.Pipe: %v", err)
}
os.Stderr = write
defer func() { os.Stderr = origStderr }()
a.persistTurn(sess, "hello", "world")
if err := write.Close(); err != nil {
t.Fatalf("close pipe writer: %v", err)
}
captured, err := io.ReadAll(read)
if err != nil {
t.Fatalf("read captured stderr: %v", err)
}
output := string(captured)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/agent_test.go` around lines 178 - 196, The stderr capture in
persistTurn testing leaves os.Stderr redirected if a t.Fatalf happens before
manual restoration, so update the test around a.persistTurn in
internal/acp/agent_test.go to restore os.Stderr with a deferred cleanup
immediately after swapping it to the pipe writer. Keep the existing
os.Pipe/read-capture flow, but ensure the original stderr is always restored
regardless of failures in os.Pipe, write.Close, or io.ReadAll, so the rest of
the test process is not left with a dangling stderr.

for _, want := range []string{"failed to persist user turn", "failed to persist assistant turn"} {
if !strings.Contains(output, want) {
t.Fatalf("expected stderr to contain %q, got %q", want, output)
}
}
}

func TestACPSetModeUpdatesSession(t *testing.T) {
h := newHarness(t, testDeps(t))
defer h.stop()
Expand Down
Loading