diff --git a/internal/ipc/client.go b/internal/ipc/client.go index 1c61b30..63005fe 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -26,17 +26,51 @@ type Client struct { mu sync.Mutex } -// errConnLost marks a dropped connection — the write failed, or the read hit -// EOF because the peer went away (a core restart closes the accepted conn, and -// the failure can surface on either phase depending on socket-buffer timing). -// call() redials and retries once on this. Retry is safe for the case that -// actually happens — core restarted, so the request was never processed. The -// only unsafe window is a write method whose request the server committed and -// then died before replying; a retry would re-apply it. That's rare, and the -// store is append-only (a duplicate is a superseding row, not corruption), so -// self-healing across restarts is the right trade. ponytail: retry-once, not a -// full idempotency-key protocol — add request IDs if double-apply ever bites. -var errConnLost = errors.New("ipc: connection lost") +// errWriteLost marks a conn drop while sending the request frame: the request +// never reached the server (or the server never saw a complete frame), so +// retrying is always safe regardless of method — nothing was applied to +// retry twice. +var errWriteLost = errors.New("ipc: connection lost before request sent") + +// errReadLost marks a conn drop while waiting for the reply: the request was +// sent and may have already been applied server-side before the connection +// died (core restart mid-request, crash after commit but before reply, etc). +// Retrying here can double-apply a mutation, which ECOSYSTEM-SPEC.md's +// "never retry an unknown outcome" invariant forbids. call() only auto-retries +// this for read-only methods (idempotent by construction); a mutation method +// returns ErrAmbiguousOutcome instead so the caller can decide — the outcome +// genuinely is unknown, not safely retryable and not safely reported as failed. +var errReadLost = errors.New("ipc: connection lost awaiting reply") + +// ErrAmbiguousOutcome is returned when a mutation's request may or may not +// have been applied server-side (the connection dropped after the request was +// sent, before the reply arrived). Callers must not blindly retry — the retry +// itself could double-apply. Surface this to the user/operator rather than +// silently treating it as either success or failure. +var ErrAmbiguousOutcome = errors.New("ipc: mutation outcome unknown (connection lost awaiting reply)") + +// readOnlyMethods are safe to retry on an ambiguous (post-send) connection +// loss: replaying a read cannot double-apply anything. Every method not +// listed here is treated as a mutation for retry purposes — being +// conservative (refusing to retry) is the safe default for a method added +// here by omission. +var readOnlyMethods = map[Method]bool{ + MethodLatestFact: true, + MethodLatestFactBySource: true, + MethodSince: true, + MethodPresence: true, + MethodListReminders: true, + MethodRecentOutcomes: true, + MethodRecentFacts: true, + MethodCalendarEvents: true, + MethodRecentNudges: true, + MethodQueryNotes: true, + MethodRecentNotes: true, + MethodLookupTool: true, + MethodListTools: true, + MethodListProposedRoutines: true, + MethodTickTrace: true, +} // Dial connects to a core socket at path and returns a Client. The module // owns its Client lifecycle; Close on shutdown. @@ -109,11 +143,20 @@ func (c *Client) call(ctx context.Context, m Method, params, result any) error { var resp Response err := c.roundtrip(m, raw, &resp) - if errors.Is(err, errConnLost) { - // Core likely restarted (new socket inode) and our cached conn is dead. - // The write never landed, so redial (roundtrip re-dials on a nil conn) - // and retry exactly once. + switch { + case errors.Is(err, errWriteLost): + // The request never left; a duplicate send can't double-apply. + // Redial (roundtrip re-dials on a nil conn) and retry exactly once. err = c.roundtrip(m, raw, &resp) + case errors.Is(err, errReadLost): + if readOnlyMethods[m] { + // A duplicate read can't double-apply either — safe to replay. + err = c.roundtrip(m, raw, &resp) + } else { + // The mutation may have already committed server-side. Do not + // retry: report the ambiguity instead of guessing. + return fmt.Errorf("%w: %v", ErrAmbiguousOutcome, err) + } } if err != nil { return err @@ -130,25 +173,26 @@ func (c *Client) call(ctx context.Context, m Method, params, result any) error { } // roundtrip sends one request and reads its reply on c.conn, lazily (re)dialing -// if the conn is nil (fresh Client or a prior drop). A write failure is wrapped -// in errConnLost (safe to retry); a read failure is returned bare (ambiguous). -// Either way a failed conn is dropped so the next call re-dials clean. Caller -// holds c.mu. +// if the conn is nil (fresh Client or a prior drop). A write-phase (or dial) +// failure is wrapped in errWriteLost (always safe to retry); a read-phase +// failure is wrapped in errReadLost (ambiguous — call() only retries it for +// read-only methods). Either way a failed conn is dropped so the next call +// re-dials clean. Caller holds c.mu. func (c *Client) roundtrip(m Method, raw json.RawMessage, resp *Response) error { if c.conn == nil { conn, err := net.Dial("unix", c.path) if err != nil { - return fmt.Errorf("%w: dial %s: %v", errConnLost, c.path, err) + return fmt.Errorf("%w: dial %s: %v", errWriteLost, c.path, err) } c.conn = conn } if err := writeFrame(c.conn, Request{Method: m, Params: raw}); err != nil { c.drop() - return fmt.Errorf("%w: %v", errConnLost, err) + return fmt.Errorf("%w: %v", errWriteLost, err) } if err := readFrame(c.conn, resp); err != nil { c.drop() - return fmt.Errorf("%w: %v", errConnLost, err) + return fmt.Errorf("%w: %v", errReadLost, err) } return nil } diff --git a/internal/ipc/ipc_test.go b/internal/ipc/ipc_test.go index f4faf85..be8ef53 100644 --- a/internal/ipc/ipc_test.go +++ b/internal/ipc/ipc_test.go @@ -542,6 +542,99 @@ func dialRaw(path string) (net.Conn, error) { return net.Dial("unix", path) } +// crashAfterReceiveServer accepts exactly one connection, reads exactly one +// request frame (so, from the client's point of view, the request definitely +// reached the server — a real write could have already committed at this +// point), then closes the connection without ever writing a reply. This is +// the "commit-then-disconnect" scenario the audit finding is about: the +// client cannot tell success from failure from the dropped connection alone. +func crashAfterReceiveServer(t *testing.T, path string) { + t.Helper() + l, err := net.Listen("unix", path) + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = l.Close() }) + go func() { + conn, err := l.Accept() + if err != nil { + return + } + var req Request + _ = readFrame(conn, &req) + _ = conn.Close() // crash: request received, no reply ever sent + }() +} + +// TestClient_MutationNotRetriedOnAmbiguousDisconnect — the audit's core +// finding (Vikunja #269): a write whose reply never arrived (server received +// the frame, then died before replying) must not be silently retried, since +// the original request may have already committed. The client must surface +// ErrAmbiguousOutcome instead of guessing either way. +func TestClient_MutationNotRetriedOnAmbiguousDisconnect(t *testing.T) { + sock := tmpSocket(t) + crashAfterReceiveServer(t, sock) + + cli, err := Dial(sock) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer cli.Close() + + _, err = cli.WriteFact(context.Background(), WriteFactReq{Key: "k", Value: "v", Source: "test"}) + if !errors.Is(err, ErrAmbiguousOutcome) { + t.Fatalf("expected ErrAmbiguousOutcome on commit-then-disconnect, got %v", err) + } +} + +// TestClient_ReadRetriedOnAmbiguousDisconnect — the same disconnect timing on +// a read-only method is safe to retry (replaying a read can't double-apply): +// the first connection receives the request and dies without replying, the +// second connection (the client's automatic retry redial) gets a real reply. +// No sleeps: each accepted connection is handled deterministically by index. +func TestClient_ReadRetriedOnAmbiguousDisconnect(t *testing.T) { + sock := tmpSocket(t) + + l, err := net.Listen("unix", sock) + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = l.Close() }) + + var n int + go func() { + for { + conn, err := l.Accept() + if err != nil { + return + } + n++ + attempt := n + go func(conn net.Conn, attempt int) { + defer conn.Close() + var req Request + if err := readFrame(conn, &req); err != nil { + return + } + if attempt == 1 { + return // crash: request received, no reply ever sent + } + _ = writeFrame(conn, Response{Result: mustJSON(Presence{})}) + }(conn, attempt) + } + }() + + cli, err := Dial(sock) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer cli.Close() + + if _, err := cli.Presence(context.Background()); err != nil { + t.Fatalf("read should have retried past the ambiguous disconnect, got: %v", err) + } +} + func mustJSON(v any) []byte { b, err := json.Marshal(v) if err != nil {