fix: address code review findings
- Start Web UI before the blocking initial sync so the dashboard is reachable during the (rate-limited, potentially multi-minute) first sync; fold the immediate sync into startPeriodicSync's overlap guard so it can never race a concurrent tick over the shared DB / MB client. - Make MarkNotificationSent idempotent: INSERT OR IGNORE for same-second PK collisions, and explicitly swallow FK violations when a release was pruned by a concurrent re-sync. Prevents a single vanished/duplicate release from aborting the digest mark-sent loop and re-sending. - Do not abort NotifyOnce's mark-sent loop on a single failure; log and continue so every release in the batch is marked. - NULL-safe reads: COALESCE(type,''), COALESCE(release_date,'') in the external_releases and unnotified readers to match the cache reader. - Update/extend tests for the new idempotency and startup contracts.
This commit is contained in:
@@ -52,7 +52,7 @@ func GetExternalRelease(db *DB, rgid string) (*ExternalRelease, error) {
|
||||
var cachedAt sql.NullTime
|
||||
var secondaryTypes sql.NullString
|
||||
err := db.Conn().QueryRow(
|
||||
"SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE rgid = ?",
|
||||
"SELECT rgid, artist_id, title, COALESCE(type,''), COALESCE(release_date,''), is_ignored, cached_at, secondary_types FROM external_releases WHERE rgid = ?",
|
||||
rgid,
|
||||
).Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt, &secondaryTypes)
|
||||
if err != nil {
|
||||
@@ -83,7 +83,7 @@ func SaveExternalRelease(db *DB, release *ExternalRelease) error {
|
||||
// GetExternalReleasesByArtist returns all external_release rows for a given artist_id.
|
||||
func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, error) {
|
||||
rows, err := db.Conn().Query(
|
||||
"SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE artist_id = ?",
|
||||
"SELECT rgid, artist_id, title, COALESCE(type,''), COALESCE(release_date,''), is_ignored, cached_at, secondary_types FROM external_releases WHERE artist_id = ?",
|
||||
artistID,
|
||||
)
|
||||
if err != nil {
|
||||
@@ -116,7 +116,7 @@ func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, er
|
||||
// GetIgnoredReleases returns all external_release rows where is_ignored = 1.
|
||||
func GetIgnoredReleases(db *DB) ([]ExternalRelease, error) {
|
||||
rows, err := db.Conn().Query(
|
||||
"SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE is_ignored = 1",
|
||||
"SELECT rgid, artist_id, title, COALESCE(type,''), COALESCE(release_date,''), is_ignored, cached_at, secondary_types FROM external_releases WHERE is_ignored = 1",
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("query ignored releases: %w", err)
|
||||
|
||||
@@ -1,16 +1,37 @@
|
||||
package database
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
|
||||
sqlite3 "github.com/mattn/go-sqlite3"
|
||||
)
|
||||
|
||||
// MarkNotificationSent records that a notification has been sent for the given RGID.
|
||||
//
|
||||
// Uses INSERT OR IGNORE so a pre-existing marker for the same RGID (a
|
||||
// same-second re-notify colliding on the (rgid, sent_at) primary key) is a
|
||||
// no-op rather than an error: the marker's presence, not its exact timestamp,
|
||||
// is what matters for idempotency.
|
||||
//
|
||||
// A concurrent re-sync that prunes the external_releases row before this insert
|
||||
// would violate the FK constraint. OR IGNORE does NOT downgrade FK violations
|
||||
// in this SQLite build, so the FK error is caught explicitly and treated as a
|
||||
// benign no-op ("the release is already gone"). This ensures a single vanished
|
||||
// release cannot abort a whole digest's mark-sent loop and trigger duplicate
|
||||
// notifications on the next run.
|
||||
func MarkNotificationSent(db *DB, rgid string) error {
|
||||
_, err := db.Conn().Exec(
|
||||
"INSERT INTO notifications_sent (rgid) VALUES (?)",
|
||||
"INSERT OR IGNORE INTO notifications_sent (rgid) VALUES (?)",
|
||||
rgid,
|
||||
)
|
||||
if err != nil {
|
||||
var sqliteErr sqlite3.Error
|
||||
if errors.As(err, &sqliteErr) && sqliteErr.Code == sqlite3.ErrConstraint &&
|
||||
sqliteErr.ExtendedCode == sqlite3.ErrConstraintForeignKey {
|
||||
// Release row was pruned concurrently; nothing to mark.
|
||||
return nil
|
||||
}
|
||||
return fmt.Errorf("mark notification sent: %w", err)
|
||||
}
|
||||
return nil
|
||||
@@ -33,7 +54,7 @@ func IsNotificationSent(db *DB, rgid string) (bool, error) {
|
||||
// artists are excluded so the digest honors the monitoring contract.
|
||||
func GetUnnotifiedReleases(db *DB) ([]ExternalRelease, error) {
|
||||
rows, err := db.Conn().Query(`
|
||||
SELECT e.rgid, e.artist_id, e.title, e.type, e.release_date, e.is_ignored
|
||||
SELECT e.rgid, e.artist_id, e.title, COALESCE(e.type,''), COALESCE(e.release_date,''), e.is_ignored
|
||||
FROM external_releases e
|
||||
JOIN artist_settings s ON e.artist_id = s.id
|
||||
LEFT JOIN notifications_sent n ON e.rgid = n.rgid
|
||||
|
||||
@@ -41,9 +41,38 @@ func TestMarkNotificationSent_New(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestMarkNotificationSent_DuplicateSecond verifies that inserting the same RGID twice
|
||||
// within the same second fails due to the composite primary key (rgid, sent_at).
|
||||
// In practice, notifications are sent at most once per day, so this is acceptable.
|
||||
// TestMarkNotificationSent_MissingReleaseIsNoOp verifies that marking a release
|
||||
// whose external_releases row does not exist (e.g. pruned by a concurrent
|
||||
// re-sync) does not error: the FK violation is swallowed by INSERT OR IGNORE so
|
||||
// a single vanished release cannot abort a digest's mark-sent loop.
|
||||
func TestMarkNotificationSent_MissingReleaseIsNoOp(t *testing.T) {
|
||||
db, err := New(":memory:")
|
||||
if err != nil {
|
||||
t.Fatalf("New() error: %v", err)
|
||||
}
|
||||
defer db.Close()
|
||||
|
||||
// No artist/release inserted: rgid-gone has no external_releases row.
|
||||
if err := MarkNotificationSent(db, "rgid-gone"); err != nil {
|
||||
t.Fatalf("MarkNotificationSent() for missing release should be a no-op, got error: %v", err)
|
||||
}
|
||||
|
||||
// Nothing should have been recorded (FK violation ignored, row skipped).
|
||||
var count int
|
||||
if err := db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rgid-gone").Scan(&count); err != nil {
|
||||
t.Fatalf("count query error: %v", err)
|
||||
}
|
||||
if count != 0 {
|
||||
t.Errorf("expected 0 notification rows for missing release, got %d", count)
|
||||
}
|
||||
}
|
||||
|
||||
// TestMarkNotificationSent_DuplicateSecond verifies that marking the same RGID
|
||||
// twice within the same second is an idempotent no-op (INSERT OR IGNORE) rather
|
||||
// than an error: a same-second collision on the composite primary key
|
||||
// (rgid, sent_at) must not abort a digest's mark-sent loop, since that would
|
||||
// leave later releases unmarked and cause duplicate notifications on the next
|
||||
// run. The marker's presence, not its exact timestamp, is what matters.
|
||||
func TestMarkNotificationSent_DuplicateSecond(t *testing.T) {
|
||||
db, err := New(":memory:")
|
||||
if err != nil {
|
||||
@@ -60,10 +89,9 @@ func TestMarkNotificationSent_DuplicateSecond(t *testing.T) {
|
||||
if err := MarkNotificationSent(db, "rgid-1"); err != nil {
|
||||
t.Fatalf("first MarkNotificationSent() error: %v", err)
|
||||
}
|
||||
// Second insert in the same second should fail with a UNIQUE constraint error.
|
||||
err = MarkNotificationSent(db, "rgid-1")
|
||||
if err == nil {
|
||||
t.Fatal("expected UNIQUE constraint error on duplicate insert, got nil")
|
||||
// Second mark in the same second should be a silent no-op, not an error.
|
||||
if err := MarkNotificationSent(db, "rgid-1"); err != nil {
|
||||
t.Fatalf("duplicate MarkNotificationSent() should be a no-op, got error: %v", err)
|
||||
}
|
||||
|
||||
// Should still have exactly one row.
|
||||
|
||||
Reference in New Issue
Block a user