Fix correctness bugs found by code review

- matrix: drop cryptohelper LoginAs so device.json creds aren't invalidated on every restart by Init's re-login - matrix: thread ctx through PostThreadedReply; bot dispatcher derives per-command ctx from app lifecycle so SIGTERM cancels in-flight work - matrix: isTokenValid verifies /whoami user_id matches configured one - matrix: loadDevice distinguishes missing file from corrupt parse; refuse to silently overwrite a corrupt device.json - matrix: Start checks Syncer type assertion and returns error - arr: parseLookup skips empty-title items and extracts existing id - bot: skip Add when Result.Exists; reply "already in library"
2026-05-24 20:50:51 -07:00
parent 8c295d183b
commit 035089c159
5 changed files with 137 additions and 52 deletions
--- a/internal/arr/arr.go
+++ b/internal/arr/arr.go
@@ -19,11 +19,15 @@ import (

 // Result is a single candidate returned by an *arr lookup. Title and Year
 // are extracted for display; raw is the untouched JSON object used to build
-// the Add payload.
+// the Add payload. Exists is true when the *arr server already has this item
+// in its library (lookup responses set "id" to a non-zero value in that case),
+// so the bot can skip the Add and report 'already in library' instead of
+// blindly re-POSTing.
 type Result struct {
-	Title string
-	Year  int
-	raw   json.RawMessage
+	Title  string
+	Year   int
+	Exists bool
+	raw    json.RawMessage
 }

 // Raw exposes the underlying lookup JSON; useful for tests and debugging.
@@ -105,10 +109,29 @@ func parseLookup(body []byte, titleField string) ([]Result, error) {
 			continue
 		}
 		var title string
-		_ = json.Unmarshal(meta[titleField], &title)
+		if raw, ok := meta[titleField]; ok && len(raw) > 0 {
+			_ = json.Unmarshal(raw, &title)
+		}
+		if title == "" {
+			// Skip items we can't display. The Add path would still POST the
+			// raw JSON, but the user reply would read "Added to Radarr:" with
+			// no title — worse than dropping the candidate.
+			continue
+		}
 		var year int
-		_ = json.Unmarshal(meta["year"], &year)
-		out = append(out, Result{Title: title, Year: year, raw: item})
+		if raw, ok := meta["year"]; ok && len(raw) > 0 {
+			_ = json.Unmarshal(raw, &year)
+		}
+		var existingID int
+		if raw, ok := meta["id"]; ok && len(raw) > 0 {
+			_ = json.Unmarshal(raw, &existingID)
+		}
+		out = append(out, Result{
+			Title:  title,
+			Year:   year,
+			Exists: existingID > 0,
+			raw:    item,
+		})
 	}
 	return out, nil
 }
--- a/internal/bot/bot.go
+++ b/internal/bot/bot.go
@@ -19,9 +19,10 @@ import (
 	"maunium.net/go/mautrix/id"
 )

-// Replier sends a threaded reply into roomID under rootEventID.
+// Replier sends a threaded reply into roomID under rootEventID. The ctx
+// governs cancellation/deadline so shutdown propagates through replies.
 type Replier interface {
-	PostThreadedReply(roomID id.RoomID, rootEventID id.EventID, plain, htmlBody string) error
+	PostThreadedReply(ctx context.Context, roomID id.RoomID, rootEventID id.EventID, plain, htmlBody string) error
 }

 // Services holds the configured *arr clients. Any field may be nil; commands
@@ -34,6 +35,7 @@ type Services struct {

 // Dispatcher routes parsed commands to services and posts replies.
 type Dispatcher struct {
+	baseCtx  context.Context
 	prefix   string
 	services Services
 	replier  Replier
@@ -41,12 +43,17 @@ type Dispatcher struct {
 }

 // New builds a Dispatcher. prefix is the leading character(s) on a command
-// (e.g. "!"); a zero value defaults to "!".
-func New(prefix string, services Services, replier Replier) *Dispatcher {
+// (e.g. "!"); a zero value defaults to "!". baseCtx becomes the parent of
+// every per-command context so app shutdown cancels in-flight handlers.
+func New(baseCtx context.Context, prefix string, services Services, replier Replier) *Dispatcher {
 	if prefix == "" {
 		prefix = "!"
 	}
+	if baseCtx == nil {
+		baseCtx = context.Background()
+	}
 	return &Dispatcher{
+		baseCtx:  baseCtx,
 		prefix:   prefix,
 		services: services,
 		replier:  replier,
@@ -64,11 +71,11 @@ func (d *Dispatcher) Handle(roomID id.RoomID, eventID id.EventID, sender id.User

 	slog.Info("bot: command received", "room", roomID, "sender", sender, "cmd", cmd, "query", query)

-	ctx, cancel := context.WithTimeout(context.Background(), d.timeout)
+	ctx, cancel := context.WithTimeout(d.baseCtx, d.timeout)
 	defer cancel()

 	plain, htmlBody := d.dispatch(ctx, cmd, query)
-	if err := d.replier.PostThreadedReply(roomID, eventID, plain, htmlBody); err != nil {
+	if err := d.replier.PostThreadedReply(ctx, roomID, eventID, plain, htmlBody); err != nil {
 		slog.Error("bot: reply failed", "room", roomID, "err", err)
 	}
 }
@@ -130,6 +137,10 @@ func (d *Dispatcher) run(ctx context.Context, cmd, service string, client arr.Cl
 	}

 	top := results[0]
+	if top.Exists {
+		msg := fmt.Sprintf("%s already has %s in the library.", service, formatTitle(top))
+		return msg, html.EscapeString(msg)
+	}
 	if err := client.Add(ctx, top); err != nil {
 		slog.Error("bot: add failed", "service", service, "title", top.Title, "err", err)
 		msg := fmt.Sprintf("%s: failed to add %s: %v", service, formatTitle(top), err)
--- a/internal/bot/bot_test.go
+++ b/internal/bot/bot_test.go
@@ -44,7 +44,7 @@ type stubReplier struct {
 	calls    int
 }

-func (r *stubReplier) PostThreadedReply(_ id.RoomID, _ id.EventID, plain, htmlBody string) error {
+func (r *stubReplier) PostThreadedReply(_ context.Context, _ id.RoomID, _ id.EventID, plain, htmlBody string) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	r.calls++
@@ -53,6 +53,10 @@ func (r *stubReplier) PostThreadedReply(_ id.RoomID, _ id.EventID, plain, htmlBo
 	return nil
 }

+func newDispatcher(prefix string, services Services, replier Replier) *Dispatcher {
+	return New(context.Background(), prefix, services, replier)
+}
+
 const (
 	room   = id.RoomID("!room:example.org")
 	event  = id.EventID("$evt")
@@ -87,7 +91,7 @@ func TestDispatchMovieTopHit(t *testing.T) {
 		{Title: "Dune", Year: 1984},
 	}}
 	rep := &stubReplier{}
-	d := New("!", Services{Radarr: radarr}, rep)
+	d := newDispatcher("!", Services{Radarr: radarr}, rep)

 	d.Handle(room, event, sender, "!movie dune")

@@ -105,7 +109,7 @@ func TestDispatchMovieTopHit(t *testing.T) {
 func TestDispatchNoResults(t *testing.T) {
 	sonarr := &stubClient{}
 	rep := &stubReplier{}
-	d := New("!", Services{Sonarr: sonarr}, rep)
+	d := newDispatcher("!", Services{Sonarr: sonarr}, rep)

 	d.Handle(room, event, sender, "!tv obscure show")

@@ -119,7 +123,7 @@ func TestDispatchNoResults(t *testing.T) {

 func TestDispatchUnconfigured(t *testing.T) {
 	rep := &stubReplier{}
-	d := New("!", Services{}, rep)
+	d := newDispatcher("!", Services{}, rep)

 	d.Handle(room, event, sender, "!music radiohead")

@@ -131,7 +135,7 @@ func TestDispatchUnconfigured(t *testing.T) {
 func TestDispatchEmptyQuery(t *testing.T) {
 	radarr := &stubClient{}
 	rep := &stubReplier{}
-	d := New("!", Services{Radarr: radarr}, rep)
+	d := newDispatcher("!", Services{Radarr: radarr}, rep)

 	d.Handle(room, event, sender, "!movie")

@@ -149,7 +153,7 @@ func TestDispatchAddError(t *testing.T) {
 		addErr:  errors.New("boom"),
 	}
 	rep := &stubReplier{}
-	d := New("!", Services{Radarr: radarr}, rep)
+	d := newDispatcher("!", Services{Radarr: radarr}, rep)

 	d.Handle(room, event, sender, "!movie dune")

@@ -161,7 +165,7 @@ func TestDispatchAddError(t *testing.T) {
 func TestDispatchSearchError(t *testing.T) {
 	radarr := &stubClient{srchErr: errors.New("network down")}
 	rep := &stubReplier{}
-	d := New("!", Services{Radarr: radarr}, rep)
+	d := newDispatcher("!", Services{Radarr: radarr}, rep)

 	d.Handle(room, event, sender, "!movie dune")

@@ -172,7 +176,7 @@ func TestDispatchSearchError(t *testing.T) {

 func TestDispatchHelp(t *testing.T) {
 	rep := &stubReplier{}
-	d := New("!", Services{}, rep)
+	d := newDispatcher("!", Services{}, rep)

 	d.Handle(room, event, sender, "!help")

@@ -185,7 +189,7 @@ func TestDispatchHelp(t *testing.T) {

 func TestDispatchUnknown(t *testing.T) {
 	rep := &stubReplier{}
-	d := New("!", Services{}, rep)
+	d := newDispatcher("!", Services{}, rep)

 	d.Handle(room, event, sender, "!frobnicate stuff")

@@ -196,7 +200,7 @@ func TestDispatchUnknown(t *testing.T) {

 func TestNonCommandIgnored(t *testing.T) {
 	rep := &stubReplier{}
-	d := New("!", Services{}, rep)
+	d := newDispatcher("!", Services{}, rep)

 	d.Handle(room, event, sender, "just chatting")

@@ -205,10 +209,25 @@ func TestNonCommandIgnored(t *testing.T) {
 	}
 }

+func TestDispatchAlreadyInLibrary(t *testing.T) {
+	radarr := &stubClient{results: []arr.Result{{Title: "Dune", Year: 2021, Exists: true}}}
+	rep := &stubReplier{}
+	d := newDispatcher("!", Services{Radarr: radarr}, rep)
+
+	d.Handle(room, event, sender, "!movie dune")
+
+	if len(radarr.added) != 0 {
+		t.Fatalf("should not re-add existing item, got %d adds", len(radarr.added))
+	}
+	if !strings.Contains(rep.plain, "already has") || !strings.Contains(rep.plain, "Dune (2021)") {
+		t.Fatalf("unexpected reply: %q", rep.plain)
+	}
+}
+
 func TestCustomPrefix(t *testing.T) {
 	radarr := &stubClient{results: []arr.Result{{Title: "Dune", Year: 2021}}}
 	rep := &stubReplier{}
-	d := New(".bh ", Services{Radarr: radarr}, rep)
+	d := newDispatcher(".bh ", Services{Radarr: radarr}, rep)

 	d.Handle(room, event, sender, ".bh movie dune")

--- a/internal/matrix/client.go
+++ b/internal/matrix/client.go
@@ -3,6 +3,7 @@ package matrix
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -50,13 +51,16 @@ func New(cfg config.MatrixConfig) (*Client, error) {

 	device, err := loadDevice(devicePath)
 	if err != nil {
-		slog.Info("no existing device found, will login fresh")
+		// File exists but is unparseable — refuse to overwrite. A silent fresh
+		// login would orphan the prior device's megolm sessions and break
+		// decryption in every encrypted room the bot is already in.
+		return nil, fmt.Errorf("load device file %s: %w (refusing to overwrite — fix or delete the file)", devicePath, err)
 	}

 	var mx *mautrix.Client

 	if device != nil {
-		if isTokenValid(cfg.Homeserver, device.AccessToken) {
+		if isTokenValid(cfg.Homeserver, device.AccessToken, cfg.UserID) {
 			slog.Info("existing device credentials valid", "device_id", device.DeviceID)
 			mx, err = mautrix.NewClient(cfg.Homeserver, id.UserID(device.UserID), device.AccessToken)
 			if err != nil {
@@ -92,12 +96,11 @@ func New(cfg config.MatrixConfig) (*Client, error) {
 		mx.UserID = resp.UserID
 		mx.DeviceID = resp.DeviceID

-		device = &deviceInfo{
+		if err := saveDevice(devicePath, &deviceInfo{
 			AccessToken: resp.AccessToken,
 			DeviceID:    string(resp.DeviceID),
 			UserID:      string(resp.UserID),
-		}
-		if err := saveDevice(devicePath, device); err != nil {
+		}); err != nil {
 			slog.Warn("failed to save device info", "err", err)
 		}

@@ -117,16 +120,11 @@ func New(cfg config.MatrixConfig) (*Client, error) {
 			"room", evt.RoomID, "event_id", evt.ID, "sender", evt.Sender, "err", err)
 	}

-	ch.LoginAs = &mautrix.ReqLogin{
-		Type: mautrix.AuthTypePassword,
-		Identifier: mautrix.UserIdentifier{
-			Type: mautrix.IdentifierTypeUser,
-			User: cfg.UserID,
-		},
-		Password:                 cfg.Password,
-		InitialDeviceDisplayName: cfg.DisplayName,
-	}
-
+	// Intentionally NOT setting ch.LoginAs: cryptohelper.Init will Login() every
+	// time LoginAs is non-nil (see mautrix cryptohelper.go), which would mint a
+	// new access_token on every startup and silently invalidate the device.json
+	// we just wrote. We already have valid credentials in mx by this point —
+	// Init will reuse them.
 	if err := ch.Init(context.Background()); err != nil {
 		return nil, fmt.Errorf("crypto helper init: %w", err)
 	}
@@ -189,8 +187,11 @@ func (c *Client) SetMessageHandler(fn MessageHandler) {
 }

 // Start begins the Matrix sync loop in the background.
-func (c *Client) Start(ctx context.Context) {
-	syncer := c.mx.Syncer.(*mautrix.DefaultSyncer)
+func (c *Client) Start(ctx context.Context) error {
+	syncer, ok := c.mx.Syncer.(*mautrix.DefaultSyncer)
+	if !ok {
+		return fmt.Errorf("matrix syncer is not *mautrix.DefaultSyncer (got %T)", c.mx.Syncer)
+	}

 	syncer.OnEventType(event.EventMessage, func(ctx context.Context, evt *event.Event) {
 		if evt.Sender == c.userID {
@@ -243,6 +244,7 @@ func (c *Client) Start(ctx context.Context) {
 			}
 		}
 	}()
+	return nil
 }

 // Stop halts the sync loop and closes the crypto store.
@@ -259,10 +261,8 @@ func (c *Client) Stop() {

 // PostThreadedReply sends a plain/HTML message into the thread rooted at rootEventID.
 // IsFallingBack + m.in_reply_to gives non-thread-aware clients a sensible quoted reply.
-func (c *Client) PostThreadedReply(roomID id.RoomID, rootEventID id.EventID, plain, htmlBody string) error {
-	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
-	defer cancel()
-
+// The caller's ctx governs cancellation and deadline.
+func (c *Client) PostThreadedReply(ctx context.Context, roomID id.RoomID, rootEventID id.EventID, plain, htmlBody string) error {
 	content := &event.MessageEventContent{
 		MsgType:       event.MsgText,
 		Body:          plain,
@@ -279,14 +279,24 @@ func (c *Client) PostThreadedReply(roomID id.RoomID, rootEventID id.EventID, pla
 	return err
 }

+// loadDevice returns (nil, nil) when no device file exists, the parsed device
+// when it does, or (nil, err) when the file is present but unreadable/corrupt.
+// Distinguishing the corrupt case lets the caller refuse to silently overwrite
+// it with a brand-new device registration.
 func loadDevice(path string) (*deviceInfo, error) {
 	data, err := os.ReadFile(path)
 	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return nil, nil
+		}
 		return nil, err
 	}
 	var info deviceInfo
 	if err := json.Unmarshal(data, &info); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parse: %w", err)
+	}
+	if info.AccessToken == "" || info.DeviceID == "" || info.UserID == "" {
+		return nil, fmt.Errorf("missing required fields (access_token/device_id/user_id)")
 	}
 	return &info, nil
 }
@@ -299,7 +309,11 @@ func saveDevice(path string, info *deviceInfo) error {
 	return os.WriteFile(path, data, 0o600)
 }

-func isTokenValid(homeserver, accessToken string) bool {
+// isTokenValid hits /whoami and confirms both that the token is accepted AND
+// that it identifies the user we expect. A bare status-code check would accept
+// a token belonging to a different account if the operator swapped user_id
+// without clearing data_dir.
+func isTokenValid(homeserver, accessToken, expectedUserID string) bool {
 	req, err := http.NewRequest("GET", homeserver+"/_matrix/client/v3/account/whoami", nil)
 	if err != nil {
 		return false
@@ -312,5 +326,19 @@ func isTokenValid(homeserver, accessToken string) bool {
 		return false
 	}
 	defer resp.Body.Close()
-	return resp.StatusCode == http.StatusOK
+	if resp.StatusCode != http.StatusOK {
+		return false
+	}
+	var body struct {
+		UserID string `json:"user_id"`
+	}
+	if err := json.NewDecoder(resp.Body).Decode(&body); err != nil {
+		return false
+	}
+	if body.UserID != expectedUserID {
+		slog.Warn("saved token belongs to a different user, will re-login",
+			"saved", body.UserID, "configured", expectedUserID)
+		return false
+	}
+	return true
 }
--- a/main.go
+++ b/main.go
@@ -43,13 +43,17 @@ func main() {
 		os.Exit(1)
 	}

-	dispatcher := bot.New(cfg.Matrix.CommandPrefix, services, mx)
-	mx.SetMessageHandler(dispatcher.Handle)
-
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()

-	mx.Start(ctx)
+	dispatcher := bot.New(ctx, cfg.Matrix.CommandPrefix, services, mx)
+	mx.SetMessageHandler(dispatcher.Handle)
+
+	if err := mx.Start(ctx); err != nil {
+		slog.Error("matrix start failed", "err", err)
+		mx.Stop()
+		os.Exit(1)
+	}
 	slog.Info("bellhop started", "allowed_rooms", len(cfg.Matrix.AllowedRooms))

 	sig := make(chan os.Signal, 1)