From 35188503b520b426a50e5520844b02730d372ff2 Mon Sep 17 00:00:00 2001 From: Matt Morris Date: Tue, 8 Sep 2026 15:34:18 -0500 Subject: [PATCH 1/6] feat: Retry failed play submissions - Persist submission state and original records for reliable retries - Add authenticated failed-plays page with individual and batch retry actions --- README.md | 25 +++ cmd/failed_plays.go | 153 ++++++++++++++++++ cmd/failed_plays_test.go | 102 ++++++++++++ cmd/handlers.go | 10 +- cmd/routes.go | 2 + db/db.go | 36 ++++- db/submissions.go | 115 ++++++++++++++ db/submissions_test.go | 110 +++++++++++++ models/track.go | 3 +- pages/static/main.css | 35 ++++- pages/templates/components/navBar.gohtml | 1 + pages/templates/failedPlays.gohtml | 55 +++++++ service/applemusic/applemusic.go | 4 +- service/atproto/submission.go | 144 ++++++++++++++--- service/atproto/submission_test.go | 190 +++++++++++++++++++++++ service/lastfm/lastfm.go | 7 +- service/spotify/spotify.go | 34 +--- 17 files changed, 950 insertions(+), 76 deletions(-) create mode 100644 cmd/failed_plays.go create mode 100644 cmd/failed_plays_test.go create mode 100644 db/submissions.go create mode 100644 db/submissions_test.go create mode 100644 pages/templates/failedPlays.gohtml create mode 100644 service/atproto/submission_test.go diff --git a/README.md b/README.md index 58376aa..aa2ef6a 100644 --- a/README.md +++ b/README.md @@ -166,3 +166,28 @@ Sensitive environment variables (Spotify, ATProto, Last.fm credentials) should b ``` See [module.nix](./module.nix) for additional configuration options. + +## Failed play submissions + +Open **Failed plays** in Piper's navigation to inspect saved plays that have not +been confirmed published. Each entry includes its source, play time, submission +status, attempt count, last attempt time, and last error. Retry an individual play +or the current page of up to 20 plays. Batch retries stop on the first failure. +For `invalid_grant`, log out and sign in again before retrying. + +Eligible plays and their pending submissions are saved in one transaction. +Publishing success is tracked separately from `hasStamped`, which still means +that a play met its source's eligibility rules. A retry uses the current OAuth +session and the original saved record key and payload, including the play time. +An interrupted submission can be retried after its two-minute claim expires. +Retries are user-initiated; there is no automatic retry scheduler. + +Server logs include `play_submission` events with user ID, play ID, record key, +attempt number, outcome, and a safe error summary. OAuth response bodies and +credentials are not saved in submission errors. HTTP status and actionable +errors such as `invalid_grant` are retained. + +Historical tracks saved before submission tracking was introduced have unknown +publishing outcomes. They are not automatically queued or labelled as failures. +Recovering an older gap requires comparing the saved tracks with PDS records +before backfilling, to avoid duplicating successful submissions. diff --git a/cmd/failed_plays.go b/cmd/failed_plays.go new file mode 100644 index 0000000..999de5f --- /dev/null +++ b/cmd/failed_plays.go @@ -0,0 +1,153 @@ +package main + +import ( + "bytes" + "context" + "crypto/rand" + "crypto/subtle" + "encoding/hex" + "errors" + "fmt" + "log" + "net/http" + "net/url" + "strconv" + "time" + + "github.com/teal-fm/piper/db" + atprotoauth "github.com/teal-fm/piper/oauth/atproto" + "github.com/teal-fm/piper/pages" + atprotoservice "github.com/teal-fm/piper/service/atproto" + "github.com/teal-fm/piper/session" +) + +type failedPlaysParams struct { + NavBar pages.NavBar + Plays []db.PlaySubmission + CSRF string + Notice string + Previous int + Next int + HasPrevious bool + HasNext bool +} + +func failedPlays(database *db.DB, pg *pages.Pages, auth *atprotoauth.AuthService) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + userID, ok := session.GetUserID(r.Context()) + if !ok { + http.Error(w, "Sign in to view failed plays.", http.StatusUnauthorized) + return + } + w.Header().Set("Cache-Control", "no-store") + if r.Method != http.MethodGet && r.Method != http.MethodPost { + w.Header().Set("Allow", "GET, POST") + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } + if r.Method == http.MethodPost { + r.Body = http.MaxBytesReader(w, r.Body, 8192) + if err := r.ParseForm(); err != nil { + http.Error(w, "Invalid retry request", http.StatusBadRequest) + return + } + cookie, err := r.Cookie("piper_retry_csrf") + if err != nil || len(cookie.Value) != 64 || subtle.ConstantTimeCompare([]byte(cookie.Value), []byte(r.PostForm.Get("csrf"))) != 1 { + http.Error(w, "Refresh this page before retrying.", http.StatusForbidden) + return + } + if origin := r.Header.Get("Origin"); origin != "" { + parsed, err := url.Parse(origin) + if err != nil || parsed.Host != r.Host || (parsed.Scheme != "http" && parsed.Scheme != "https") { + http.Error(w, "Invalid request origin", http.StatusForbidden) + return + } + } + ids := r.PostForm["play_id"] + if len(ids) == 0 || len(ids) > 20 { + http.Error(w, "Choose between 1 and 20 plays.", http.StatusBadRequest) + return + } + // Validate the complete request before publishing anything. + parsedIDs := make([]int64, 0, len(ids)) + seen := map[int64]bool{} + for _, raw := range ids { + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + http.Error(w, "Invalid play ID", http.StatusBadRequest) + return + } + if _, err := database.GetTrackForUser(userID, id); err != nil { + http.Error(w, "Play not found", http.StatusNotFound) + return + } + if !seen[id] { + parsedIDs = append(parsedIDs, id) + seen[id] = true + } + } + ctx, cancel := context.WithTimeout(r.Context(), 25*time.Second) + defer cancel() + published, failed, skipped := 0, 0, 0 + for _, id := range parsedIDs { + err := atprotoservice.PublishStoredPlay(ctx, database, userID, id, auth) + if errors.Is(err, db.ErrSubmissionUnavailable) { + skipped++ + continue + } + if err != nil { + failed++ + break + } + published++ + } + notice := fmt.Sprintf("Published %d. Failed %d. Unavailable %d. Not attempted %d.", published, failed, skipped, len(parsedIDs)-published-failed-skipped) + http.Redirect(w, r, "/failed-plays?notice="+url.QueryEscape(notice), http.StatusSeeOther) + return + } + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + if offset < 0 || offset > 1000000 { + offset = 0 + } + plays, err := database.ListUnpublishedPlays(userID, 21, offset) + if err != nil { + log.Printf("failed_plays user_id=%d error=%q", userID, err) + http.Error(w, "Could not load failed plays. Try again.", http.StatusInternalServerError) + return + } + if offset > 0 && len(plays) == 0 { + http.Redirect(w, r, "/failed-plays", http.StatusSeeOther) + return + } + user, err := database.GetUserByID(userID) + if err != nil { + http.Error(w, "Could not load account", http.StatusInternalServerError) + return + } + token := "" + if cookie, err := r.Cookie("piper_retry_csrf"); err == nil && len(cookie.Value) == 64 { + token = cookie.Value + } + if token == "" { + var b [32]byte + if _, err := rand.Read(b[:]); err != nil { + http.Error(w, "Could not prepare retry form", http.StatusInternalServerError) + return + } + token = hex.EncodeToString(b[:]) + http.SetCookie(w, &http.Cookie{Name: "piper_retry_csrf", Value: token, Path: "/failed-plays", HttpOnly: true, SameSite: http.SameSiteStrictMode, Secure: r.TLS != nil}) + } + params := failedPlaysParams{NavBar: pages.NewNavBar(user, true).WithBreadcrumb("Failed plays"), Plays: plays, CSRF: token, Notice: r.URL.Query().Get("notice"), HasPrevious: offset > 0, Previous: max(0, offset-20), HasNext: len(plays) > 20, Next: offset + 20} + if len(params.Plays) > 20 { + params.Plays = params.Plays[:20] + } + var body bytes.Buffer + if err := pg.Execute("failedPlays", &body, params); err != nil { + log.Printf("failed plays template: %v", err) + http.Error(w, "Could not display failed plays", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write(body.Bytes()) + } +} diff --git a/cmd/failed_plays_test.go b/cmd/failed_plays_test.go new file mode 100644 index 0000000..5bd919f --- /dev/null +++ b/cmd/failed_plays_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/teal-fm/piper/db" + "github.com/teal-fm/piper/models" + "github.com/teal-fm/piper/pages" + "github.com/teal-fm/piper/session" +) + +func TestFailedPlaysPageAndRetryAuthorization(t *testing.T) { + database, err := db.New(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatal(err) + } + defer database.Close() + if err := database.Initialize(); err != nil { + t.Fatal(err) + } + uid, err := database.CreateUser(&models.User{}) + if err != nil { + t.Fatal(err) + } + other, err := database.CreateUser(&models.User{}) + if err != nil { + t.Fatal(err) + } + save := func(userID int64, name string) int64 { + t.Helper() + id, err := database.SaveTrack(userID, db.SourceAppleMusic, &models.Track{Name: name, HasStamped: true, Timestamp: time.Now().UTC()}) + if err != nil { + t.Fatal(err) + } + return id + } + own := save(uid, "") + foreign := save(other, "Another user's private play") + handler := failedPlays(database, pages.NewPages(), nil) + ctx := session.WithUserID(context.Background(), uid) + get := httptest.NewRequest(http.MethodGet, "/failed-plays", nil).WithContext(ctx) + response := httptest.NewRecorder() + handler(response, get) + if response.Code != http.StatusOK { + t.Fatalf("GET: %d %s", response.Code, response.Body.String()) + } + body := response.Body.String() + if strings.Contains(body, "Another user's private play") || strings.Contains(body, "") || !strings.Contains(body, "<script>") { + t.Fatalf("unsafe rendered page: %s", body) + } + cookies := response.Result().Cookies() + if len(cookies) != 1 { + t.Fatal("missing CSRF cookie") + } + csrf := cookies[0] + for _, tc := range []struct { + name string + ids []int64 + token string + origin string + code int + }{ + {"missing CSRF", []int64{own}, "", "", http.StatusForbidden}, + {"foreign play", []int64{foreign}, csrf.Value, "", http.StatusNotFound}, + {"mixed ownership", []int64{own, foreign}, csrf.Value, "", http.StatusNotFound}, + {"cross origin", []int64{own}, csrf.Value, "https://evil.example", http.StatusForbidden}, + {"retry own", []int64{own}, csrf.Value, "", http.StatusSeeOther}, + } { + t.Run(tc.name, func(t *testing.T) { + form := url.Values{"csrf": {tc.token}} + for _, id := range tc.ids { + form.Add("play_id", strconv.FormatInt(id, 10)) + } + request := httptest.NewRequest(http.MethodPost, "/failed-plays", strings.NewReader(form.Encode())).WithContext(ctx) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + request.Header.Set("Origin", tc.origin) + request.AddCookie(csrf) + result := httptest.NewRecorder() + handler(result, request) + if result.Code != tc.code { + t.Fatalf("POST = %d: %s", result.Code, result.Body.String()) + } + }) + } + plays, err := database.ListUnpublishedPlays(uid, 20, 0) + if err != nil || len(plays) != 1 || plays[0].Attempts != 1 || !strings.Contains(plays[0].LastError, "Sign in") { + t.Fatalf("retry failure not visible: %+v %v", plays, err) + } + result := httptest.NewRecorder() + handler(result, httptest.NewRequest(http.MethodGet, "/failed-plays", nil)) + if result.Code != http.StatusUnauthorized { + t.Fatal("anonymous access allowed") + } +} diff --git a/cmd/handlers.go b/cmd/handlers.go index c904aa3..90d4018 100644 --- a/cmd/handlers.go +++ b/cmd/handlers.go @@ -671,7 +671,7 @@ func apiSubmitListensHandler(database *db.DB, atprotoService *atprotoauth.AuthSe // lookups are rate limited to 1/s, so doing them before responding // can outlast the proxy timeout and trap clients in a retry loop if len(savedListens) > 0 { - go hydrateAndSubmitListens(database, atprotoService, mbService, user, userID, savedListens) + go hydrateAndSubmitListens(database, atprotoService, mbService, userID, savedListens) } // Prepare response @@ -704,7 +704,7 @@ type savedListen struct { // hydrateAndSubmitListens hydrates saved listens with MusicBrainz data and // submits them to the PDS. It runs detached from the request that saved them, // on its own context, since both steps can far outlast the client connection. -func hydrateAndSubmitListens(database *db.DB, atprotoService *atprotoauth.AuthService, mbService *musicbrainz.Service, user *models.User, userID int64, listens []savedListen) { +func hydrateAndSubmitListens(database *db.DB, atprotoService *atprotoauth.AuthService, mbService *musicbrainz.Service, userID int64, listens []savedListen) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) defer cancel() @@ -723,10 +723,8 @@ func hydrateAndSubmitListens(database *db.DB, atprotoService *atprotoauth.AuthSe } } - if user.ATProtoDID != nil && atprotoService != nil { - if err := atprotoservice.SubmitPlayToPDS(ctx, *user.ATProtoDID, *user.MostRecentAtProtoSessionID, &track, atprotoService); err != nil { - log.Printf("apiSubmitListensHandler: Error submitting play to PDS for user %d: %v", userID, err) - } + if err := atprotoservice.PublishStoredPlay(ctx, database, userID, saved.trackID, atprotoService); err != nil { + log.Printf("apiSubmitListensHandler: Error submitting play to PDS for user %d: %v", userID, err) } } } diff --git a/cmd/routes.go b/cmd/routes.go index e33b601..6b1b3f1 100644 --- a/cmd/routes.go +++ b/cmd/routes.go @@ -11,6 +11,8 @@ import ( func (app *application) routes() http.Handler { mux := http.NewServeMux() + mux.HandleFunc("/failed-plays", session.WithAuth(failedPlays(app.database, app.pages, app.atprotoService), app.sessionManager)) + //Handles static file routes mux.Handle("/static/{file_name}", app.pages.Static()) diff --git a/db/db.go b/db/db.go index 6753da9..f9e346d 100644 --- a/db/db.go +++ b/db/db.go @@ -1,7 +1,9 @@ package db import ( + "crypto/rand" "database/sql" + "encoding/binary" "encoding/json" "errors" "fmt" @@ -11,6 +13,7 @@ import ( "strings" "time" + "github.com/bluesky-social/indigo/atproto/syntax" _ "github.com/mattn/go-sqlite3" "github.com/teal-fm/piper/models" ) @@ -35,6 +38,9 @@ func New(dbPath string) (*DB, error) { return nil, err } + // SQLite has one writer; one connection also keeps :memory: databases consistent. + db.SetMaxOpenConns(1) + // Test the connection if err = db.Ping(); err != nil { return nil, err @@ -192,6 +198,9 @@ func (db *DB) Initialize() error { return err } + if err := db.initializeSubmissions(); err != nil { + return err + } return db.backfillTrackSources() } @@ -492,15 +501,36 @@ func (db *DB) SaveTrack(userID int64, source TrackSource, track *models.Track) ( } var trackID int64 - - err := db.QueryRow(` + tx, err := db.Begin() + if err != nil { + return 0, err + } + defer tx.Rollback() + err = tx.QueryRow(` INSERT INTO tracks (user_id, name, recording_mbid, artist, album, release_mbid, url, timestamp, duration_ms, progress_ms, service_base_url, isrc, has_stamped, source) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING id`, userID, track.Name, track.RecordingMBID, artistString, track.Album, track.ReleaseMBID, track.URL, track.Timestamp, track.DurationMs, track.ProgressMs, track.ServiceBaseUrl, track.ISRC, track.HasStamped, source).Scan(&trackID) - return trackID, err + if err != nil { + return 0, err + } + if track.HasStamped { + var clockBytes [2]byte + if _, err := rand.Read(clockBytes[:]); err != nil { + return 0, err + } + rkey := syntax.NewTIDNow(uint(binary.BigEndian.Uint16(clockBytes[:]) & 1023)).String() + if _, err := tx.Exec(`INSERT INTO play_submissions(track_id,rkey) VALUES (?,?)`, trackID, rkey); err != nil { + return 0, err + } + } + if err := tx.Commit(); err != nil { + return 0, err + } + track.PlayID = trackID + return trackID, nil } // HasTrackListen reports whether a listen with the same name and timestamp diff --git a/db/submissions.go b/db/submissions.go new file mode 100644 index 0000000..540aeae --- /dev/null +++ b/db/submissions.go @@ -0,0 +1,115 @@ +package db + +import ( + "database/sql" + "encoding/json" + "errors" + "fmt" + "time" + + "github.com/teal-fm/piper/models" +) + +// A missing submission row means the historical publishing outcome is unknown. +// HasStamped describes eligibility, not delivery to the PDS. +func (db *DB) initializeSubmissions() error { + _, err := db.Exec(`CREATE TABLE IF NOT EXISTS play_submissions ( + track_id INTEGER PRIMARY KEY REFERENCES tracks(id), + rkey TEXT NOT NULL UNIQUE, + status TEXT NOT NULL DEFAULT 'pending', + attempts INTEGER NOT NULL DEFAULT 0, + last_attempt_at TIMESTAMP, + last_error TEXT NOT NULL DEFAULT '', + record_json TEXT, + published_at TIMESTAMP + )`) + return err +} + +type PlaySubmission struct { + TrackID int64 + Name string + Artists []models.Artist + Source string + PlayedAt time.Time + Status string + Attempts int + LastAttemptAt sql.NullTime + LastError string + RKey string + RecordJSON sql.NullString +} + +func (db *DB) ListUnpublishedPlays(userID int64, limit, offset int) ([]PlaySubmission, error) { + rows, err := db.Query(`SELECT t.id, t.name, t.artist, t.source, t.timestamp, s.status, s.attempts, + s.last_attempt_at, s.last_error FROM play_submissions s JOIN tracks t ON t.id=s.track_id + WHERE t.user_id=? AND s.status != 'published' ORDER BY t.timestamp, t.id LIMIT ? OFFSET ?`, userID, limit, offset) + if err != nil { + return nil, err + } + defer rows.Close() + var plays []PlaySubmission + for rows.Next() { + var p PlaySubmission + var artists string + if err := rows.Scan(&p.TrackID, &p.Name, &artists, &p.Source, &p.PlayedAt, &p.Status, &p.Attempts, &p.LastAttemptAt, &p.LastError); err != nil { + return nil, err + } + if err := json.Unmarshal([]byte(artists), &p.Artists); err != nil && artists != "" { + p.Artists = []models.Artist{{Name: artists}} + } + plays = append(plays, p) + } + return plays, rows.Err() +} + +func (db *DB) GetTrackForUser(userID, trackID int64) (*models.Track, error) { + return scanTrack(db.QueryRow(`SELECT id, name, recording_mbid, artist, album, release_mbid, url, + timestamp, duration_ms, progress_ms, service_base_url, isrc, has_stamped FROM tracks WHERE id=? AND user_id=?`, trackID, userID)) +} + +var ErrSubmissionUnavailable = errors.New("play is already published, currently being submitted, or unavailable") + +// ClaimSubmission uses an expiring claim so crashes do not strand plays. The +// attempt number fences late results from an older worker after lease expiry. +func (db *DB) ClaimSubmission(userID, trackID int64) (*PlaySubmission, error) { + var p PlaySubmission + err := db.QueryRow(`UPDATE play_submissions SET status='submitting', attempts=attempts+1, last_attempt_at=? + WHERE track_id=? AND track_id IN (SELECT id FROM tracks WHERE user_id=?) + AND (status IN ('pending','failed') OR (status='submitting' AND last_attempt_at < ?)) + RETURNING track_id,rkey,attempts,record_json`, time.Now().UTC(), trackID, userID, time.Now().UTC().Add(-2*time.Minute)).Scan(&p.TrackID, &p.RKey, &p.Attempts, &p.RecordJSON) + if errors.Is(err, sql.ErrNoRows) { + return nil, ErrSubmissionUnavailable + } + return &p, err +} + +func (db *DB) SaveSubmissionRecord(p *PlaySubmission, record string) error { + res, err := db.Exec(`UPDATE play_submissions SET record_json=? WHERE track_id=? AND attempts=? AND status='submitting'`, record, p.TrackID, p.Attempts) + return submissionUpdated(res, err) +} + +func (db *DB) FinishSubmission(p *PlaySubmission, message string) error { + status := "published" + var publishedAt any = time.Now().UTC() + if message != "" { + status = "failed" + publishedAt = nil + } + res, err := db.Exec(`UPDATE play_submissions SET status=?, last_error=?, published_at=? WHERE track_id=? AND attempts=? AND status='submitting'`, status, message, publishedAt, p.TrackID, p.Attempts) + return submissionUpdated(res, err) +} + +func submissionUpdated(res sql.Result, err error) error { + if err != nil { + return err + } + n, err := res.RowsAffected() + if err != nil { + return err + } + if n != 1 { + return fmt.Errorf("submission claim expired") + } + return nil +} diff --git a/db/submissions_test.go b/db/submissions_test.go new file mode 100644 index 0000000..0f853b8 --- /dev/null +++ b/db/submissions_test.go @@ -0,0 +1,110 @@ +package db + +import ( + "errors" + "testing" + "time" + + "github.com/teal-fm/piper/models" +) + +func TestSubmissionEligibilityAndLegacyMigration(t *testing.T) { + database := newTestDB(t) + uid := createTestUser(t, database) + // Historical has_stamped=true must not be mistaken for a known failure. + if _, err := database.Exec(`INSERT INTO tracks(user_id,name,artist,album,url,has_stamped) VALUES (?,'legacy','[]','','',1)`, uid); err != nil { + t.Fatal(err) + } + for _, eligible := range []bool{false, true} { + track := &models.Track{Name: "new", Timestamp: time.Now().UTC(), HasStamped: eligible} + id, err := database.SaveTrack(uid, SourceAppleMusic, track) + if err != nil { + t.Fatal(err) + } + if track.PlayID != id { + t.Fatal("saved track missing its play ID") + } + } + if err := database.Initialize(); err != nil { + t.Fatal(err) + } + plays, err := database.ListUnpublishedPlays(uid, 20, 0) + if err != nil { + t.Fatal(err) + } + if len(plays) != 1 || plays[0].Status != "pending" { + t.Fatalf("unexpected submissions: %+v", plays) + } +} + +func TestSubmissionClaimsOwnershipRetryAndExpiry(t *testing.T) { + database := newTestDB(t) + uid := createTestUser(t, database) + other := createTestUser(t, database) + id, err := database.SaveTrack(uid, SourceAppleMusic, &models.Track{Name: "play", HasStamped: true, Timestamp: time.Now().UTC()}) + if err != nil { + t.Fatal(err) + } + if _, err := database.ClaimSubmission(other, id); !errors.Is(err, ErrSubmissionUnavailable) { + t.Fatalf("other user claimed play: %v", err) + } + first, err := database.ClaimSubmission(uid, id) + if err != nil { + t.Fatal(err) + } + if _, err := database.ClaimSubmission(uid, id); !errors.Is(err, ErrSubmissionUnavailable) { + t.Fatalf("duplicate claim: %v", err) + } + if _, err := database.Exec(`UPDATE play_submissions SET last_attempt_at=? WHERE track_id=?`, time.Now().UTC().Add(-3*time.Minute), id); err != nil { + t.Fatal(err) + } + second, err := database.ClaimSubmission(uid, id) + if err != nil { + t.Fatal(err) + } + if second.RKey != first.RKey { + t.Fatal("retry changed record key") + } + if err := database.FinishSubmission(first, ""); err == nil { + t.Fatal("stale worker changed current claim") + } + if err := database.FinishSubmission(second, "HTTP 400: invalid_grant"); err != nil { + t.Fatal(err) + } + retry, err := database.ClaimSubmission(uid, id) + if err != nil { + t.Fatal(err) + } + if retry.Attempts != 3 { + t.Fatalf("attempts = %d", retry.Attempts) + } + if err := database.FinishSubmission(retry, ""); err != nil { + t.Fatal(err) + } + if _, err := database.ClaimSubmission(uid, id); !errors.Is(err, ErrSubmissionUnavailable) { + t.Fatalf("published play reclaimed: %v", err) + } + plays, err := database.ListUnpublishedPlays(uid, 20, 0) + if err != nil || len(plays) != 0 { + t.Fatalf("published play still listed: %+v %v", plays, err) + } +} + +func TestSaveTrackRollsBackWhenSubmissionCannotBeSaved(t *testing.T) { + database := newTestDB(t) + uid := createTestUser(t, database) + if _, err := database.Exec(`CREATE TRIGGER reject_submission BEFORE INSERT ON play_submissions BEGIN SELECT RAISE(ABORT, 'storage failure'); END`); err != nil { + t.Fatal(err) + } + track := &models.Track{Name: "Must not be stranded", HasStamped: true, Timestamp: time.Now().UTC()} + if _, err := database.SaveTrack(uid, SourceAppleMusic, track); err == nil { + t.Fatal("expected storage failure") + } + tracks, err := database.GetRecentTracks(uid, 20) + if err != nil { + t.Fatal(err) + } + if len(tracks) != 0 || track.PlayID != 0 { + t.Fatal("track survived without its pending submission") + } +} diff --git a/models/track.go b/models/track.go index 4f09ff8..c1715a6 100644 --- a/models/track.go +++ b/models/track.go @@ -18,7 +18,8 @@ type Track struct { ProgressMs int64 `json:"progressMs"` ServiceBaseUrl string `json:"serviceBaseUrl"` ISRC string `json:"isrc"` - HasStamped bool `json:"hasStamped"` + // HasStamped indicates a play met its source eligibility rules, not PDS success. + HasStamped bool `json:"hasStamped"` } type Artist struct { diff --git a/pages/static/main.css b/pages/static/main.css index c62ba0a..039cec7 100644 --- a/pages/static/main.css +++ b/pages/static/main.css @@ -221,9 +221,6 @@ .static { position: static; } - .sticky { - position: sticky; - } .inset-0 { inset: calc(var(--spacing) * 0); } @@ -251,12 +248,18 @@ .mt-1 { margin-top: calc(var(--spacing) * 1); } + .mt-2 { + margin-top: calc(var(--spacing) * 2); + } .mt-3 { margin-top: calc(var(--spacing) * 3); } .mt-4 { margin-top: calc(var(--spacing) * 4); } + .mt-6 { + margin-top: calc(var(--spacing) * 6); + } .mt-12 { margin-top: calc(var(--spacing) * 12); } @@ -343,6 +346,9 @@ .shrink-0 { flex-shrink: 0; } + .grow { + flex-grow: 1; + } .border-collapse { border-collapse: collapse; } @@ -367,6 +373,12 @@ .items-center { align-items: center; } + .items-start { + align-items: flex-start; + } + .justify-between { + justify-content: space-between; + } .justify-center { justify-content: center; } @@ -389,6 +401,13 @@ margin-block-end: calc(calc(var(--spacing) * 3) * calc(1 - var(--tw-space-y-reverse))); } } + .space-y-5 { + :where(& > :not(:last-child)) { + --tw-space-y-reverse: 0; + margin-block-start: calc(calc(var(--spacing) * 5) * var(--tw-space-y-reverse)); + margin-block-end: calc(calc(var(--spacing) * 5) * calc(1 - var(--tw-space-y-reverse))); + } + } .gap-x-4 { column-gap: calc(var(--spacing) * 4); } @@ -416,6 +435,10 @@ border-style: var(--tw-border-style); border-width: 1px; } + .border-t { + border-top-style: var(--tw-border-style); + border-top-width: 1px; + } .border-b { border-bottom-style: var(--tw-border-style); border-bottom-width: 1px; @@ -530,6 +553,9 @@ .py-8 { padding-block: calc(var(--spacing) * 8); } + .pt-4 { + padding-top: calc(var(--spacing) * 4); + } .pr-2 { padding-right: calc(var(--spacing) * 2); } @@ -581,6 +607,9 @@ --tw-tracking: var(--tracking-tight); letter-spacing: var(--tracking-tight); } + .break-words { + overflow-wrap: break-word; + } .break-all { word-break: break-all; } diff --git a/pages/templates/components/navBar.gohtml b/pages/templates/components/navBar.gohtml index c4d92dd..37f3fee 100644 --- a/pages/templates/components/navBar.gohtml +++ b/pages/templates/components/navBar.gohtml @@ -20,6 +20,7 @@ Now playing History {{ end }} + Failed plays API keys Failed plays +

Plays saved by Piper that have not been confirmed published. Retry them here after fixing the connection.

+{{ if .Notice }}

{{ .Notice }}

{{ end }} +{{ if .Plays }} +

If an error says invalid_grant, log out and sign in again, then return here. Retries keep the original play time.

+
+ + {{ range .Plays }}{{ end }} + +

Up to 20 plays. Stops on the first failure. Plays being submitted are skipped.

+
+
    +{{ range .Plays }} +
  1. +
    +
    +

    {{ .Name }}

    +

    {{ range $i, $artist := .Artists }}{{ if $i }}, {{ end }}{{ $artist.Name }}{{ end }}

    +

    {{ .Source }} · {{ formatTime .PlayedAt.UTC }} UTC · Play #{{ .TrackID }}

    +
    +
    + + + +
    +
    +

    {{ .Status }} · {{ .Attempts }} attempt{{ if ne .Attempts 1 }}s{{ end }}{{ if .LastAttemptAt.Valid }} · Last attempt {{ formatTime .LastAttemptAt.Time.UTC }} UTC{{ end }}

    + {{ if .LastError }}

    {{ .LastError }}

    + {{ else }}

    {{ if eq .Status "submitting" }}Submission is in progress. If it was interrupted, retry after two minutes.{{ else }}Saved and waiting for submission.{{ end }}

    {{ end }} +
    2. +{{ end }} +
+ +{{ else }} +
+

No failed or pending plays

+

New publishing failures will appear here with an error and a retry option.

+
+{{ end }} +

Older plays saved before submission tracking was added have an unknown publishing status and are not listed here. Recovering them requires checking which records are missing from your PDS.

+ +{{ end }} diff --git a/service/applemusic/applemusic.go b/service/applemusic/applemusic.go index 69a3fa5..2b0ab2d 100644 --- a/service/applemusic/applemusic.go +++ b/service/applemusic/applemusic.go @@ -610,8 +610,8 @@ func (s *Service) ProcessUser(ctx context.Context, user *models.User) error { } // Submit to PDS - if user.ATProtoDID != nil && user.MostRecentAtProtoSessionID != nil && s.atprotoService != nil { - if err := atprotoservice.SubmitPlayToPDS(ctx, *user.ATProtoDID, *user.MostRecentAtProtoSessionID, track, s.atprotoService); err != nil { + if track.HasStamped { + if err := atprotoservice.PublishStoredPlay(ctx, s.DB, user.ID, track.PlayID, s.atprotoService); err != nil { s.logger.Printf("failed submit to PDS for user %d: %v", user.ID, err) } } diff --git a/service/atproto/submission.go b/service/atproto/submission.go index 8a119f0..3e335f8 100644 --- a/service/atproto/submission.go +++ b/service/atproto/submission.go @@ -2,51 +2,151 @@ package atproto import ( "context" + "encoding/json" + "errors" "fmt" "log" + "reflect" + "regexp" + "strings" "time" comatproto "github.com/bluesky-social/indigo/api/atproto" + "github.com/bluesky-social/indigo/atproto/client" lexutil "github.com/bluesky-social/indigo/lex/util" "github.com/spf13/viper" "github.com/teal-fm/piper/api/teal" + "github.com/teal-fm/piper/db" "github.com/teal-fm/piper/models" atprotoauth "github.com/teal-fm/piper/oauth/atproto" ) -// SubmitPlayToPDS submits a track play to the ATProto PDS as a feed.play record -func SubmitPlayToPDS(ctx context.Context, did string, mostRecentAtProtoSessionID string, track *models.Track, atprotoService *atprotoauth.AuthService) error { - if did == "" { - return fmt.Errorf("DID cannot be empty") - } +// PublishStoredPlay submits a saved, eligible play using the user's current session. +// The durable claim is shared by ingestion and manual retries. +func PublishStoredPlay(ctx context.Context, database *db.DB, userID, trackID int64, auth *atprotoauth.AuthService) error { + return publishStoredPlay(ctx, database, userID, trackID, func(ctx context.Context, user *models.User, key string, record *teal.FeedPlay) error { + if auth == nil || user.ATProtoDID == nil || user.MostRecentAtProtoSessionID == nil || *user.MostRecentAtProtoSessionID == "" { + return errors.New("Sign in to Piper again to reconnect publishing.") + } + client, err := auth.GetATProtoClient(*user.ATProtoDID, *user.MostRecentAtProtoSessionID, ctx) + if err != nil { + return fmt.Errorf("opening OAuth session: %s", submissionError(err)) + } + if client == nil { + return errors.New("OAuth session returned no client. Sign in to Piper again.") + } + return createPlayRecord(ctx, client, *user.ATProtoDID, key, record) + }) +} - // Get ATProto client - client, err := atprotoService.GetATProtoClient(did, mostRecentAtProtoSessionID, ctx) - if err != nil || client == nil { - return fmt.Errorf("failed to get ATProto client: %w", err) - } +type playPublisher func(context.Context, *models.User, string, *teal.FeedPlay) error - // Convert track to feed.play record - playRecord, err := TrackToPlayRecord(track) +func publishStoredPlay(ctx context.Context, database *db.DB, userID, trackID int64, publish playPublisher) error { + p, err := database.ClaimSubmission(userID, trackID) if err != nil { - return fmt.Errorf("failed to convert track to play record: %w", err) + return err } - - // Create the record - input := comatproto.RepoCreateRecord_Input{ - Collection: "fm.teal.feed.play", - Repo: client.AccountDID.String(), - Record: &lexutil.LexiconTypeDecoder{Val: playRecord}, + ctx, cancel := context.WithTimeout(ctx, 45*time.Second) + defer cancel() + err = func() error { + user, err := database.GetUserByID(userID) + if err != nil { + return errors.New("Could not load the publishing account.") + } + if user == nil { + return errors.New("Publishing account was not found.") + } + var record *teal.FeedPlay + if p.RecordJSON.Valid { + if err := json.Unmarshal([]byte(p.RecordJSON.String), &record); err != nil { + return errors.New("Could not read saved play record.") + } + } else { + track, err := database.GetTrackForUser(userID, trackID) + if err != nil { + return errors.New("Could not load saved play.") + } + record, err = TrackToPlayRecord(track) + if err != nil { + return err + } + encoded, err := json.Marshal(record) + if err != nil { + return errors.New("Could not encode saved play.") + } + if err := database.SaveSubmissionRecord(p, string(encoded)); err != nil { + return errors.New("Could not save publishing record.") + } + } + return publish(ctx, user, p.RKey, record) + }() + message := "" + if err != nil { + message = err.Error() + } + if saveErr := database.FinishSubmission(p, message); saveErr != nil { + log.Printf("play_submission user_id=%d play_id=%d attempt=%d outcome=persistence_failed error=%q", userID, trackID, p.Attempts, saveErr) + return fmt.Errorf("saving publishing outcome: %w", saveErr) + } + outcome := "published" + if err != nil { + outcome = "failed" } + log.Printf("play_submission user_id=%d play_id=%d rkey=%s attempt=%d outcome=%s error=%q", userID, trackID, p.RKey, p.Attempts, outcome, message) + return err +} +// createRecord retains create-only OAuth permissions. If the write succeeded +// but its response was lost, verify the existing record at the same key. +func createPlayRecord(ctx context.Context, client lexutil.LexClient, did, key string, record *teal.FeedPlay) error { + input := comatproto.RepoCreateRecord_Input{Collection: "fm.teal.feed.play", Repo: did, Rkey: &key, Record: &lexutil.LexiconTypeDecoder{Val: record}} if _, err := comatproto.RepoCreateRecord(ctx, client, &input); err != nil { - return fmt.Errorf("failed to create play record for DID %s: %w", did, err) + existing, readErr := comatproto.RepoGetRecord(ctx, client, "", "fm.teal.feed.play", did, key) + if readErr == nil && existing != nil && existing.Value != nil { + // Compare decoded JSON so field order does not affect equality. + a, _ := json.Marshal(existing.Value) + b, _ := json.Marshal(record) + var av, bv any + if json.Unmarshal(a, &av) == nil && json.Unmarshal(b, &bv) == nil && reflect.DeepEqual(av, bv) { + return nil + } + } + return fmt.Errorf("publishing play: %s", submissionError(err)) } - - log.Printf("Successfully submitted play to PDS for DID %s: %s - %s", did, track.Artist[0].Name, track.Name) return nil } +var httpStatusPattern = regexp.MustCompile(`HTTP[ )]*(\d{3})`) +var errorNamePattern = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_]{0,63}$`) + +// Never persist arbitrary response bodies, URLs, or token-bearing OAuth errors. +// Keep the actionable code, HTTP status, and operation instead. +func submissionError(err error) string { + if errors.Is(err, context.DeadlineExceeded) { + return "Request timed out. Retry this play." + } + if errors.Is(err, context.Canceled) { + return "Request was interrupted. Retry this play." + } + detail := "Request failed. Retry this play." + var apiErr *client.APIError + if errors.As(err, &apiErr) { + detail = fmt.Sprintf("HTTP %d", apiErr.StatusCode) + if errorNamePattern.MatchString(apiErr.Name) { + detail += ": " + apiErr.Name + } + } else if match := httpStatusPattern.FindStringSubmatch(err.Error()); len(match) > 1 { + detail = "HTTP " + match[1] + } + if strings.Contains(err.Error(), "invalid_grant") { + detail += ": invalid_grant. Sign in to Piper again, then retry." + } + if strings.Contains(err.Error(), "failed to refresh OAuth tokens") || strings.Contains(err.Error(), "token refresh failed") { + detail = "Failed to refresh OAuth tokens. " + detail + } + return detail +} + // TrackToPlayRecord converts a models.Track to teal.FeedPlay func TrackToPlayRecord(track *models.Track) (*teal.FeedPlay, error) { if track.Name == "" { diff --git a/service/atproto/submission_test.go b/service/atproto/submission_test.go new file mode 100644 index 0000000..188c00b --- /dev/null +++ b/service/atproto/submission_test.go @@ -0,0 +1,190 @@ +package atproto + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/bluesky-social/indigo/atproto/client" + "github.com/bluesky-social/indigo/xrpc" + "github.com/teal-fm/piper/api/teal" + "github.com/teal-fm/piper/db" + "github.com/teal-fm/piper/models" +) + +func TestFailedSubmissionSurvivesRestartAndRetriesOriginalRecord(t *testing.T) { + path := filepath.Join(t.TempDir(), "piper.db") + database, err := db.New(path) + if err != nil { + t.Fatal(err) + } + if err := database.Initialize(); err != nil { + t.Fatal(err) + } + uid, err := database.CreateUser(&models.User{}) + if err != nil { + t.Fatal(err) + } + playedAt := time.Date(2026, 9, 3, 18, 3, 0, 0, time.UTC) + track := &models.Track{Name: "Original title", Timestamp: playedAt, HasStamped: true} + id, err := database.SaveTrack(uid, db.SourceAppleMusic, track) + if err != nil { + t.Fatal(err) + } + var originalKey string + err = publishStoredPlay(context.Background(), database, uid, id, func(ctx context.Context, u *models.User, key string, r *teal.FeedPlay) error { + originalKey = key + return errors.New("publishing play: HTTP 400: invalid_grant. Sign in to Piper again, then retry.") + }) + if err == nil { + t.Fatal("expected publishing failure") + } + plays, err := database.ListUnpublishedPlays(uid, 20, 0) + if err != nil || len(plays) != 1 || plays[0].Attempts != 1 || !strings.Contains(plays[0].LastError, "invalid_grant") { + t.Fatalf("failure not saved: %+v %v", plays, err) + } + database.Close() + database, err = db.New(path) + if err != nil { + t.Fatal(err) + } + defer database.Close() + if err := database.Initialize(); err != nil { + t.Fatal(err) + } + // Later hydration cannot change the record sent by a retry. + if _, err := database.Exec(`UPDATE tracks SET name='Changed title' WHERE id=?`, id); err != nil { + t.Fatal(err) + } + err = publishStoredPlay(context.Background(), database, uid, id, func(ctx context.Context, u *models.User, key string, r *teal.FeedPlay) error { + if key != originalKey || r.TrackName != "Original title" || r.PlayedTime == nil || *r.PlayedTime != playedAt.Format(time.RFC3339) { + t.Fatalf("retry changed original record: %s %+v", key, r) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + plays, err = database.ListUnpublishedPlays(uid, 20, 0) + if err != nil || len(plays) != 0 { + t.Fatalf("successful retry still listed: %+v %v", plays, err) + } +} + +func TestCreatePlayRecordVerifiesAmbiguousWrite(t *testing.T) { + for _, matches := range []bool{true, false} { + t.Run(fmt.Sprint(matches), func(t *testing.T) { + record := &teal.FeedPlay{LexiconTypeID: "fm.teal.feed.play", TrackName: "Track"} + key := "3jzfcijpj2z2a" + creates := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + var input struct { + RKey string `json:"rkey"` + } + if err := json.NewDecoder(r.Body).Decode(&input); err != nil { + t.Error(err) + } + if input.RKey != key { + t.Errorf("unstable key %q", input.RKey) + } + creates++ + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"error":"InternalServerError"}`) + return + } + if r.URL.Query().Get("rkey") != key { + t.Errorf("wrong verification key") + } + existing := *record + if !matches { + existing.TrackName = "Different play" + } + json.NewEncoder(w).Encode(map[string]any{"uri": "at://did:plc:test/fm.teal.feed.play/" + key, "value": existing}) + })) + defer server.Close() + err := createPlayRecord(context.Background(), &xrpc.Client{Host: server.URL, Client: server.Client()}, "did:plc:test", key, record) + if matches && err != nil { + t.Fatal(err) + } + if !matches && err == nil { + t.Fatal("different remote record counted as successful") + } + if creates != 1 { + t.Fatalf("unexpected create count %d", creates) + } + }) + } +} + +func TestSubmissionErrorsKeepActionableDetailsWithoutSecrets(t *testing.T) { + for _, tc := range []struct { + err error + want string + }{ + {errors.New("failed to refresh OAuth tokens: token refresh failed (HTTP 400): invalid_grant secret-token"), "HTTP 400: invalid_grant"}, + {&client.APIError{StatusCode: 429, Name: "RateLimitExceeded", Message: "secret-token"}, "HTTP 429: RateLimitExceeded"}, + {errors.New("https://secret-token@host/?refresh_token=secret-token"), "Request failed"}, + {context.DeadlineExceeded, "timed out"}, + } { + got := submissionError(tc.err) + if !strings.Contains(got, tc.want) || strings.Contains(got, "secret-token") { + t.Errorf("unsafe or unhelpful error %q", got) + } + } +} + +func TestOverlappingRetriesPublishOnce(t *testing.T) { + database, err := db.New(filepath.Join(t.TempDir(), "piper.db")) + if err != nil { + t.Fatal(err) + } + defer database.Close() + if err := database.Initialize(); err != nil { + t.Fatal(err) + } + uid, err := database.CreateUser(&models.User{}) + if err != nil { + t.Fatal(err) + } + id, err := database.SaveTrack(uid, db.SourceAppleMusic, &models.Track{Name: "One play", HasStamped: true, Timestamp: time.Now().UTC()}) + if err != nil { + t.Fatal(err) + } + started := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- publishStoredPlay(context.Background(), database, uid, id, func(ctx context.Context, u *models.User, key string, r *teal.FeedPlay) error { + close(started) + <-release + return nil + }) + }() + select { + case <-started: + case err := <-done: + t.Fatalf("first retry exited before publishing: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("first retry did not start") + } + err = publishStoredPlay(context.Background(), database, uid, id, func(ctx context.Context, u *models.User, key string, r *teal.FeedPlay) error { + t.Error("overlapping retry attempted a second publish") + return nil + }) + close(release) + if !errors.Is(err, db.ErrSubmissionUnavailable) { + t.Errorf("overlapping retry = %v", err) + } + if err := <-done; err != nil { + t.Fatal(err) + } +} diff --git a/service/lastfm/lastfm.go b/service/lastfm/lastfm.go index cf7058f..8f014eb 100644 --- a/service/lastfm/lastfm.go +++ b/service/lastfm/lastfm.go @@ -487,7 +487,7 @@ func (l *Service) processTracks(ctx context.Context, username string, tracks []T return err } l.logger.Printf("Submitting track") - err = l.SubmitTrackToPDS(*user.ATProtoDID, *user.MostRecentAtProtoSessionID, hydratedTrack, ctx) + err = atprotoservice.PublishStoredPlay(ctx, l.db, user.ID, hydratedTrack.PlayID, l.atprotoService) if err != nil { l.logger.Printf("error submitting track for user %s: %s - %s: %v", username, track.Artist.Text, track.Name, err) } @@ -510,11 +510,6 @@ func (l *Service) processTracks(ctx context.Context, username string, tracks []T return nil } -func (l *Service) SubmitTrackToPDS(did string, mostRecentAtProtoSessionID string, track *models.Track, ctx context.Context) error { - // Use shared atproto service for submission - return atprotoservice.SubmitPlayToPDS(ctx, did, mostRecentAtProtoSessionID, track, l.atprotoService) -} - // convertLastFMTrackToModelsTrack converts a Last.fm Track to models.Track format func (l *Service) convertLastFMTrackToModelsTrack(track Track) *models.Track { // Create artist array diff --git a/service/spotify/spotify.go b/service/spotify/spotify.go index 7bda7ed..4c4e5e2 100644 --- a/service/spotify/spotify.go +++ b/service/spotify/spotify.go @@ -91,17 +91,6 @@ func NewSpotifyService(database *db.DB, atprotoAuthService *atprotoauth.AuthServ } } -func (s *Service) SubmitTrackToPDS(did string, mostRecentAtProtoSessionID string, track *models.Track, ctx context.Context) error { - //Had a empty feed.play get submitted not sure why. Tracking here - if track.Name == "" { - s.logger.Println("Track name is empty. Skipping submission. Please record the logs before and send to the teal.fm Discord") - return nil - } - - // Use shared atproto service for submission - return atprotoservice.SubmitPlayToPDS(ctx, did, mostRecentAtProtoSessionID, track, s.atprotoAuthService) -} - func (s *Service) SetAccessToken(token string, refreshToken string, userId int64) (int64, error) { userID, err := s.identifyAndStoreUser(token, refreshToken, userId) if err != nil { @@ -828,29 +817,8 @@ func (s *Service) stampTrack(ctx context.Context, userID int64, track *models.Tr return } - // Submit play record to ATProto PDS - - // Fetch the user - dbUser, err := s.DB.GetUserByID(userID) - if err != nil { - s.logger.Printf("User %d: Error fetching user for PDS: %v", userID, err) - return - } - if dbUser == nil { - s.logger.Printf("User %d: User not found in DB. Skipping PDS submission.", userID) - return - } - if dbUser.ATProtoDID == nil || *dbUser.ATProtoDID == "" { - // No DID configured, skip PDS submission silently - return - } - - // Perform submission to PDS - s.logger.Printf("User %d: Submitting track '%s' to PDS (DID: %s)", userID, trackToSubmit.Name, *dbUser.ATProtoDID) - if err := s.SubmitTrackToPDS(*dbUser.ATProtoDID, *dbUser.MostRecentAtProtoSessionID, trackToSubmit, ctx); err != nil { + if err := atprotoservice.PublishStoredPlay(ctx, s.DB, userID, trackToSubmit.PlayID, s.atprotoAuthService); err != nil { s.logger.Printf("User %d: Error submitting to PDS: %v", userID, err) - } else { - s.logger.Printf("User %d: Successfully submitted track '%s' to PDS", userID, trackToSubmit.Name) } } -- knotmirror 0.0.1 From 963c720196faf1d1f6a12a87084f62307fd388dd Mon Sep 17 00:00:00 2001 From: Matt Morris Date: Tue, 8 Sep 2026 15:45:28 -0500 Subject: [PATCH 2/6] fix: Show sign-in errors inline and widen page layout --- cmd/handlers.go | 16 +++++--- models/constants.go | 2 +- oauth/atproto/atproto.go | 24 ++++++++---- oauth/atproto/login_test.go | 31 ++++++++++++++++ oauth/oauth_manager.go | 4 ++ pages/login.go | 21 +++++++++++ pages/navbar_test.go | 8 ++-- pages/static/main.css | 47 +++++++++++++++++------- pages/templates/components/navBar.gohtml | 8 ++-- pages/templates/home.gohtml | 5 +++ pages/templates/layouts/base.gohtml | 2 +- pages/templates_test.go | 20 ++++++++++ 12 files changed, 152 insertions(+), 36 deletions(-) create mode 100644 oauth/atproto/login_test.go create mode 100644 pages/login.go diff --git a/cmd/handlers.go b/cmd/handlers.go index 90d4018..ed15f4a 100644 --- a/cmd/handlers.go +++ b/cmd/handlers.go @@ -25,9 +25,11 @@ import ( ) type HomeParams struct { - NavBar pages.NavBar - BuildTime time.Time - Agent string + LoginError string + LoginHandle string + NavBar pages.NavBar + BuildTime time.Time + Agent string } func home(database *db.DB, pg *pages.Pages, lastfmService *lastfm.Service, atprotoService *atprotoauth.AuthService, buildTime time.Time) http.HandlerFunc { @@ -49,9 +51,11 @@ func home(database *db.DB, pg *pages.Pages, lastfmService *lastfm.Service, atpro } params := HomeParams{ - NavBar: pages.NewNavBar(user, isLoggedIn), - BuildTime: buildTime, - Agent: models.SubmissionAgent, + NavBar: pages.NewNavBar(user, isLoggedIn), + BuildTime: buildTime, + Agent: models.SubmissionAgent, + LoginError: pages.LoginErrorMessage(r.URL.Query().Get("login_error")), + LoginHandle: r.URL.Query().Get("handle"), } err := pg.Execute("home", w, params) if err != nil { diff --git a/models/constants.go b/models/constants.go index 74f6fda..930a29e 100644 --- a/models/constants.go +++ b/models/constants.go @@ -1,3 +1,3 @@ package models -const SubmissionAgent = "piper/v0.0.11" +const SubmissionAgent = "piper/v0.0.12" diff --git a/oauth/atproto/atproto.go b/oauth/atproto/atproto.go index 4deb991..2cd49c7 100644 --- a/oauth/atproto/atproto.go +++ b/oauth/atproto/atproto.go @@ -93,7 +93,7 @@ func (a *AuthService) HandleLogin(w http.ResponseWriter, r *http.Request) { handle := r.URL.Query().Get("handle") if handle == "" { a.logger.Printf("ATProto Login Error: handle is required") - http.Error(w, "handle query parameter is required", http.StatusBadRequest) + redirectLoginError(w, r, "missing_handle", handle) return } ctx := r.Context() @@ -102,30 +102,34 @@ func (a *AuthService) HandleLogin(w http.ResponseWriter, r *http.Request) { atid, err := syntax.ParseAtIdentifier(handle) if err != nil { - http.Error(w, fmt.Sprintf("Error parsing AT Identifier (%s): %v", handle, err), http.StatusInternalServerError) + a.logger.Printf("Error parsing AT Identifier %q: %v", handle, err) + redirectLoginError(w, r, "invalid_handle", handle) return } ident, err := a.clientApp.Dir.Lookup(ctx, *atid) if err != nil { - http.Error(w, fmt.Sprintf("Error resolving DID for AT Identifier (%s): %v", handle, err), http.StatusInternalServerError) + a.logger.Printf("Error resolving AT Identifier %q: %v", handle, err) + redirectLoginError(w, r, "lookup_failed", handle) return } accountDid := ident.DID.String() if len(a.allowedDids) > 0 && !slices.Contains(a.allowedDids, accountDid) { a.logger.Printf("ATProto Login Error: DID %s for handle %s is not in the allowed list", accountDid, handle) - http.Error(w, "Unauthorized", http.StatusForbidden) + redirectLoginError(w, r, "not_allowed", handle) return } redirectURL, err := a.clientApp.StartAuthFlow(ctx, accountDid) if err != nil { - http.Error(w, fmt.Sprintf("Error initiating login: %v", err), http.StatusInternalServerError) + a.logger.Printf("Error initiating login: %v", err) + redirectLoginError(w, r, "login_failed", handle) return } authUrl, err := url.Parse(redirectURL) if err != nil { - http.Error(w, fmt.Sprintf("Error initiating login: %v", err), http.StatusInternalServerError) + a.logger.Printf("Error initiating login: %v", err) + redirectLoginError(w, r, "login_failed", handle) return } @@ -133,6 +137,12 @@ func (a *AuthService) HandleLogin(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, authUrl.String(), http.StatusFound) } +// Redirect with a fixed error code so OAuth details never enter the page URL. +func redirectLoginError(w http.ResponseWriter, r *http.Request, code, handle string) { + query := url.Values{"login_error": {code}, "handle": {handle}} + http.Redirect(w, r, "/?"+query.Encode(), http.StatusSeeOther) +} + func (a *AuthService) HandleLogout(w http.ResponseWriter, r *http.Request) { cookie, err := r.Cookie("session") @@ -209,7 +219,6 @@ func (a *AuthService) HandleCallback(w http.ResponseWriter, r *http.Request) (in sessData, err := a.clientApp.ProcessCallback(ctx, r.URL.Query()) if err != nil { errMsg := fmt.Errorf("processing OAuth callback: %w", err) - http.Error(w, errMsg.Error(), http.StatusBadRequest) return 0, errMsg } @@ -222,7 +231,6 @@ func (a *AuthService) HandleCallback(w http.ResponseWriter, r *http.Request) (in user, err := a.DB.FindOrCreateUserByDID(sessData.AccountDID.String()) if err != nil { a.logger.Printf("ATProto Callback Error: Failed to find or create user for DID %s: %v", sessData.AccountDID.String(), err) - http.Error(w, "Failed to process user information.", http.StatusInternalServerError) return 0, fmt.Errorf("failed to find or create user") } diff --git a/oauth/atproto/login_test.go b/oauth/atproto/login_test.go new file mode 100644 index 0000000..974ac79 --- /dev/null +++ b/oauth/atproto/login_test.go @@ -0,0 +1,31 @@ +package atproto + +import ( + "io" + "log" + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +func TestInvalidLoginReturnsToForm(t *testing.T) { + service := &AuthService{logger: log.New(io.Discard, "", 0)} + for _, tc := range []struct{ handle, code string }{{"asdf./asdf", "invalid_handle"}, {"", "missing_handle"}} { + t.Run(tc.code, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/login/atproto?"+url.Values{"handle": {tc.handle}}.Encode(), nil) + response := httptest.NewRecorder() + service.HandleLogin(response, req) + if response.Code != http.StatusSeeOther { + t.Fatalf("status = %d", response.Code) + } + location, err := url.Parse(response.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + if location.Path != "/" || location.Query().Get("login_error") != tc.code || location.Query().Get("handle") != tc.handle { + t.Fatalf("unexpected redirect: %s", location) + } + }) + } +} diff --git a/oauth/oauth_manager.go b/oauth/oauth_manager.go index 9707972..c2a82b7 100644 --- a/oauth/oauth_manager.go +++ b/oauth/oauth_manager.go @@ -88,6 +88,10 @@ func (m *ServiceManager) HandleCallback(serviceName string) http.HandlerFunc { if err != nil { m.logger.Printf("Error handling callback for service '%s': %v", serviceName, err) + if serviceName == "atproto" { + http.Redirect(w, r, "/?login_error=callback_failed", http.StatusSeeOther) + return + } http.Error(w, fmt.Sprintf("Error handling callback for service '%s'", serviceName), http.StatusInternalServerError) return } diff --git a/pages/login.go b/pages/login.go new file mode 100644 index 0000000..901a98e --- /dev/null +++ b/pages/login.go @@ -0,0 +1,21 @@ +package pages + +// LoginErrorMessage accepts only known error codes, never arbitrary URL text. +func LoginErrorMessage(code string) string { + switch code { + case "missing_handle": + return "Enter your ATProto handle to sign in." + case "invalid_handle": + return "Enter a valid ATProto handle, such as alice.bsky.social. Do not include a URL or slash." + case "lookup_failed": + return "We could not find that account. Check your handle and try again." + case "not_allowed": + return "This account is not allowed to sign in to this Piper instance." + case "login_failed": + return "Could not start sign-in with your provider. Please try again." + case "callback_failed": + return "Sign-in could not be completed. Please try again." + default: + return "" + } +} diff --git a/pages/navbar_test.go b/pages/navbar_test.go index 6fda5cb..e5dcfd4 100644 --- a/pages/navbar_test.go +++ b/pages/navbar_test.go @@ -13,9 +13,11 @@ func ptr(s string) *string { return &s } // homeParams mirrors cmd.HomeParams, which this package can't import. type homeParams struct { - NavBar NavBar - BuildTime time.Time - Agent string + LoginError string + LoginHandle string + NavBar NavBar + BuildTime time.Time + Agent string } func TestNewNavBar(t *testing.T) { diff --git a/pages/static/main.css b/pages/static/main.css index 039cec7..32d8a83 100644 --- a/pages/static/main.css +++ b/pages/static/main.css @@ -8,8 +8,11 @@ --font-serif: ui-serif, Georgia, Cambria, "Times New Roman", Times, serif; --font-mono: "JetBrains Mono", ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace; + --color-red-300: oklch(80.8% 0.114 19.571); --color-red-400: oklch(70.4% 0.191 22.216); + --color-red-500: oklch(63.7% 0.237 25.331); --color-red-600: oklch(57.7% 0.245 27.325); + --color-red-700: oklch(50.5% 0.213 27.518); --color-neutral-100: oklch(97% 0 0); --color-neutral-200: oklch(92.2% 0 0); --color-neutral-300: oklch(87% 0 0); @@ -23,6 +26,7 @@ --spacing: 0.25rem; --container-sm: 24rem; --container-3xl: 48rem; + --container-6xl: 72rem; --text-xs: 0.75rem; --text-xs--line-height: calc(1 / 0.75); --text-sm: 0.875rem; @@ -272,6 +276,9 @@ .mb-3 { margin-bottom: calc(var(--spacing) * 3); } + .mb-4 { + margin-bottom: calc(var(--spacing) * 4); + } .mb-5 { margin-bottom: calc(var(--spacing) * 5); } @@ -331,8 +338,8 @@ .w-full { width: 100%; } - .max-w-3xl { - max-width: var(--container-3xl); + .max-w-6xl { + max-width: var(--container-6xl); } .max-w-\[12rem\] { max-width: 12rem; @@ -470,6 +477,12 @@ border-color: color-mix(in oklab, var(--color-neutral-400) 30%, transparent); } } + .border-red-500\/30 { + border-color: color-mix(in srgb, oklch(63.7% 0.237 25.331) 30%, transparent); + @supports (color: color-mix(in lab, red, red)) { + border-color: color-mix(in oklab, var(--color-red-500) 30%, transparent); + } + } .bg-accent { background-color: var(--color-accent); } @@ -637,6 +650,9 @@ .text-red-600 { color: var(--color-red-600); } + .text-red-700 { + color: var(--color-red-700); + } .text-white { color: var(--color-white); } @@ -740,28 +756,28 @@ outline-style: none; } } - .sm\:ml-auto { - @media (width >= 40rem) { - margin-left: auto; - } - } .sm\:grid-cols-3 { @media (width >= 40rem) { grid-template-columns: repeat(3, minmax(0, 1fr)); } } - .sm\:flex-row { - @media (width >= 40rem) { + .lg\:ml-auto { + @media (width >= 64rem) { + margin-left: auto; + } + } + .lg\:flex-row { + @media (width >= 64rem) { flex-direction: row; } } - .sm\:items-center { - @media (width >= 40rem) { + .lg\:items-center { + @media (width >= 64rem) { align-items: center; } } - .sm\:gap-4 { - @media (width >= 40rem) { + .lg\:gap-4 { + @media (width >= 64rem) { gap: calc(var(--spacing) * 4); } } @@ -833,6 +849,11 @@ color: var(--color-neutral-600); } } + .dark\:text-red-300 { + @media (prefers-color-scheme: dark) { + color: var(--color-red-300); + } + } .dark\:text-red-400 { @media (prefers-color-scheme: dark) { color: var(--color-red-400); diff --git a/pages/templates/components/navBar.gohtml b/pages/templates/components/navBar.gohtml index 37f3fee..55fc992 100644 --- a/pages/templates/components/navBar.gohtml +++ b/pages/templates/components/navBar.gohtml @@ -1,7 +1,7 @@ {{ define "components/navBar" }} -
-