fix: make idempotency reservation atomic and fail closed

CHANGELOG.md

@@ -47,6 +47,18 @@ Targeted for the 0.2.6 release.
   a multi-byte character is no longer split into invalid UTF-8.
 - A failed read of attachment data is reported instead of discarded. Previously a
   short read was written to disk as though it were the complete attachment.
+- Idempotency keys are reserved before a send is attempted rather than recorded after
+  it succeeds, closing a window in which two concurrent invocations both saw the key
+  as unused and both sent. Reservations are now one marker file per key created with
+  `O_EXCL`, which is atomic across processes; a failed send releases the key so it can
+  be retried. Previously a corrupt store also silently disabled the protection
+  entirely, because a JSON parse error returned an empty store.
+
+### Removed
+
+- The `idempotency.json` store is replaced by an `idempotency/` directory. No
+  migration is performed: keys expire after 24 hours, so the old file simply goes
+  unused and can be deleted.
 
 ### Changed
 - `mail list --unread` now returns up to `--limit` unread messages. Previously the

internal/cli/mail.go

@@ -318,13 +318,16 @@ func (c *MailSendCmd) Run(ctx *Context) error {
 		return fmt.Errorf("not configured - run 'pm-cli config init' first")
 	}
 
-	// Check idempotency key
+	// Reserve the idempotency key before sending, not after. Recording it
+	// afterwards left a window in which two concurrent invocations both saw
+	// the key as unused and both sent. The reservation is released below if
+	// the send fails, so the same key can be retried.
 	if c.IdempotencyKey != "" {
-		used, err := config.CheckIdempotencyKey(c.IdempotencyKey)
+		reserved, err := config.ReserveIdempotencyKey(c.IdempotencyKey)
 		if err != nil {
 			return fmt.Errorf("idempotency check failed: %w", err)
 		}
-		if used {
+		if !reserved {
 			if ctx.Formatter.JSON {
 				return ctx.Formatter.PrintJSON(map[string]interface{}{
 					"success":         true,
@@ -414,15 +417,11 @@ func (c *MailSendCmd) Run(ctx *Context) error {
 	ctx.Formatter.Verbosef("Sending email to %s...", strings.Join(to, ", "))
 
 	if err := smtpClient.Send(msg); err != nil {
-		return err
-	}
-
-	// Record idempotency key after successful send
-	if c.IdempotencyKey != "" {
-		if err := config.RecordIdempotencyKey(c.IdempotencyKey); err != nil {
-			// Log but don't fail - email was already sent
-			ctx.Formatter.Verbosef("Warning: failed to record idempotency key: %v", err)
+		// Free the key so the caller can retry with the same one.
+		if relErr := config.ReleaseIdempotencyKey(c.IdempotencyKey); relErr != nil {
+			ctx.Formatter.Verbosef("Warning: failed to release idempotency key: %v", relErr)
 		}
+		return err
 	}
 
 	if ctx.Formatter.JSON {
@@ -859,13 +858,16 @@ func (c *MailReplyCmd) Run(ctx *Context) error {
 		return fmt.Errorf("not configured - run 'pm-cli config init' first")
 	}
 
-	// Check idempotency key
+	// Reserve the idempotency key before sending, not after. Recording it
+	// afterwards left a window in which two concurrent invocations both saw
+	// the key as unused and both sent. The reservation is released below if
+	// the send fails, so the same key can be retried.
 	if c.IdempotencyKey != "" {
-		used, err := config.CheckIdempotencyKey(c.IdempotencyKey)
+		reserved, err := config.ReserveIdempotencyKey(c.IdempotencyKey)
 		if err != nil {
 			return fmt.Errorf("idempotency check failed: %w", err)
 		}
-		if used {
+		if !reserved {
 			if ctx.Formatter.JSON {
 				return ctx.Formatter.PrintJSON(map[string]interface{}{
 					"success":         true,
@@ -986,14 +988,11 @@ func (c *MailReplyCmd) Run(ctx *Context) error {
 	ctx.Formatter.Verbosef("Sending reply to %s...", strings.Join(recipients, ", "))
 
 	if err := smtpClient.Send(replyMsg); err != nil {
-		return err
-	}
-
-	// Record idempotency key after successful send
-	if c.IdempotencyKey != "" {
-		if err := config.RecordIdempotencyKey(c.IdempotencyKey); err != nil {
-			ctx.Formatter.Verbosef("Warning: failed to record idempotency key: %v", err)
+		// Free the key so the caller can retry with the same one.
+		if relErr := config.ReleaseIdempotencyKey(c.IdempotencyKey); relErr != nil {
+			ctx.Formatter.Verbosef("Warning: failed to release idempotency key: %v", relErr)
 		}
+		return err
 	}
 
 	if ctx.Formatter.JSON {
@@ -1021,13 +1020,16 @@ func (c *MailForwardCmd) Run(ctx *Context) error {
 		return fmt.Errorf("not configured - run 'pm-cli config init' first")
 	}
 
-	// Check idempotency key
+	// Reserve the idempotency key before sending, not after. Recording it
+	// afterwards left a window in which two concurrent invocations both saw
+	// the key as unused and both sent. The reservation is released below if
+	// the send fails, so the same key can be retried.
 	if c.IdempotencyKey != "" {
-		used, err := config.CheckIdempotencyKey(c.IdempotencyKey)
+		reserved, err := config.ReserveIdempotencyKey(c.IdempotencyKey)
 		if err != nil {
 			return fmt.Errorf("idempotency check failed: %w", err)
 		}
-		if used {
+		if !reserved {
 			if ctx.Formatter.JSON {
 				return ctx.Formatter.PrintJSON(map[string]interface{}{
 					"success":         true,
@@ -1118,14 +1120,11 @@ func (c *MailForwardCmd) Run(ctx *Context) error {
 	ctx.Formatter.Verbosef("Forwarding email to %s...", strings.Join(c.To, ", "))
 
 	if err := smtpClient.Send(fwdMsg); err != nil {
-		return err
-	}
-
-	// Record idempotency key after successful send
-	if c.IdempotencyKey != "" {
-		if err := config.RecordIdempotencyKey(c.IdempotencyKey); err != nil {
-			ctx.Formatter.Verbosef("Warning: failed to record idempotency key: %v", err)
+		// Free the key so the caller can retry with the same one.
+		if relErr := config.ReleaseIdempotencyKey(c.IdempotencyKey); relErr != nil {
+			ctx.Formatter.Verbosef("Warning: failed to release idempotency key: %v", relErr)
 		}
+		return err
 	}
 
 	if ctx.Formatter.JSON {

internal/config/config.go

@@ -1,7 +1,8 @@
 package config
 
 import (
-	"encoding/json"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"os"
@@ -214,101 +215,128 @@ func Exists() bool {
 }
 
 // Idempotency support
+//
+// A key is reserved before the send is attempted, not recorded after it
+// succeeds. Recording afterwards left a window in which two concurrent
+// invocations both saw the key as unused and both sent, which defeats the
+// only purpose of the mechanism.
+//
+// Each key is one marker file whose name is a hash of the key, created with
+// O_EXCL. That makes the reservation atomic across processes without a lock
+// file, and keeps a user-supplied key (which may contain path separators) from
+// influencing the path. Expiry is read from the file's mtime, so there is no
+// stored content that could fail to parse.
 
 const idempotencyTTL = 24 * time.Hour
 
-type idempotencyStore struct {
-	Keys map[string]int64 `json:"keys"` // key -> unix timestamp
-}
-
-func idempotencyPath() (string, error) {
+func idempotencyDir() (string, error) {
 	dir, err := ConfigDir()
 	if err != nil {
 		return "", err
 	}
-	return filepath.Join(dir, "idempotency.json"), nil
+	return filepath.Join(dir, "idempotency"), nil
 }
 
-func loadIdempotencyStore() (*idempotencyStore, error) {
-	path, err := idempotencyPath()
+func idempotencyMarkerPath(key string) (string, error) {
+	dir, err := idempotencyDir()
 	if err != nil {
-		return nil, err
+		return "", err
 	}
+	sum := sha256.Sum256([]byte(key))
+	return filepath.Join(dir, hex.EncodeToString(sum[:])), nil
+}
 
-	store := &idempotencyStore{Keys: make(map[string]int64)}
+// ReserveIdempotencyKey atomically claims key. It returns true when the caller
+// owns the reservation and should proceed, and false when the key is already
+// held within the TTL, meaning this is a duplicate.
+//
+// An empty key disables the mechanism and always reserves successfully.
+//
+// Errors are returned rather than swallowed: if the reservation state cannot
+// be determined, the caller must not treat that as permission to send.
+func ReserveIdempotencyKey(key string) (bool, error) {
+	if key == "" {
+		return true, nil
+	}
 
-	data, err := os.ReadFile(path)
+	path, err := idempotencyMarkerPath(key)
 	if err != nil {
-		if os.IsNotExist(err) {
-			return store, nil
-		}
-		return nil, err
+		return false, err
 	}
-
-	if err := json.Unmarshal(data, store); err != nil {
-		return store, nil // Return empty store on parse error
+	if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil {
+		return false, fmt.Errorf("failed to create idempotency directory: %w", err)
 	}
 
-	return store, nil
-}
-
-func (s *idempotencyStore) save() error {
-	path, err := idempotencyPath()
-	if err != nil {
-		return err
-	}
+	purgeExpiredMarkers()
 
-	// Clean expired keys
-	now := time.Now().Unix()
-	for key, ts := range s.Keys {
-		if now-ts > int64(idempotencyTTL.Seconds()) {
-			delete(s.Keys, key)
+	for attempt := 0; attempt < 2; attempt++ {
+		f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600)
+		if err == nil {
+			return true, f.Close()
+		}
+		if !os.IsExist(err) {
+			return false, fmt.Errorf("failed to reserve idempotency key: %w", err)
 		}
-	}
 
-	data, err := json.Marshal(s)
-	if err != nil {
-		return err
+		// The marker exists. Honor it unless it has aged out.
+		info, statErr := os.Stat(path)
+		if statErr != nil {
+			if os.IsNotExist(statErr) {
+				continue // raced with a purge; try to claim it again
+			}
+			return false, fmt.Errorf("failed to read idempotency key: %w", statErr)
+		}
+		if time.Since(info.ModTime()) <= idempotencyTTL {
+			return false, nil
+		}
+		if rmErr := os.Remove(path); rmErr != nil && !os.IsNotExist(rmErr) {
+			return false, fmt.Errorf("failed to expire idempotency key: %w", rmErr)
+		}
 	}
 
-	return os.WriteFile(path, data, 0600)
+	// Another process claimed it between our removal and retry. Treat that as
+	// a duplicate rather than sending twice.
+	return false, nil
 }
 
-// CheckIdempotencyKey returns true if the key was already used (within TTL)
-func CheckIdempotencyKey(key string) (bool, error) {
+// ReleaseIdempotencyKey drops a reservation, so a send that failed can be
+// retried with the same key. It is not an error to release a key that is not
+// held.
+func ReleaseIdempotencyKey(key string) error {
 	if key == "" {
-		return false, nil
+		return nil
 	}
-
-	store, err := loadIdempotencyStore()
+	path, err := idempotencyMarkerPath(key)
 	if err != nil {
-		return false, err
-	}
-
-	ts, exists := store.Keys[key]
-	if !exists {
-		return false, nil
+		return err
 	}
-
-	// Check if expired
-	if time.Now().Unix()-ts > int64(idempotencyTTL.Seconds()) {
-		return false, nil
+	if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
+		return err
 	}
-
-	return true, nil
+	return nil
 }
 
-// RecordIdempotencyKey marks a key as used
-func RecordIdempotencyKey(key string) error {
-	if key == "" {
-		return nil
+// purgeExpiredMarkers removes aged-out reservations opportunistically. Failures
+// are ignored: this is housekeeping, and an unremoved marker only expires late.
+func purgeExpiredMarkers() {
+	dir, err := idempotencyDir()
+	if err != nil {
+		return
 	}
-
-	store, err := loadIdempotencyStore()
+	entries, err := os.ReadDir(dir)
 	if err != nil {
-		return err
+		return
+	}
+	for _, e := range entries {
+		if e.IsDir() {
+			continue
+		}
+		info, err := e.Info()
+		if err != nil {
+			continue
+		}
+		if time.Since(info.ModTime()) > idempotencyTTL {
+			_ = os.Remove(filepath.Join(dir, e.Name()))
+		}
 	}
-
-	store.Keys[key] = time.Now().Unix()
-	return store.save()
 }