Fail closed on ambiguous IPC mutation outcomes instead of blind retry
Vikunja #269 (P0): Client.call() retried any connection-loss uniformly, including the case where the request frame was already sent and the reply never arrived — the server may have already committed the write before dying, so a blind retry could double-apply it. This violates the ecosystem rule against retrying an unknown mutation outcome. - Split errConnLost into errWriteLost (request never sent — always safe to retry) and errReadLost (request sent, reply lost — ambiguous). - errReadLost is only auto-retried for read-only methods (replaying a read can't double-apply). A mutation method instead returns ErrAmbiguousOutcome so the caller can decide, rather than the boundary silently guessing. - Added commit-then-disconnect regression tests: a mutation (WriteFact) surfaces ErrAmbiguousOutcome and does not retry; a read (Presence) retries transparently past the same disconnect timing.
This commit is contained in:
kami
+66
-22
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user