Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/database/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ type Media struct {
DBID int64
MediaTitleDBID int64
SystemDBID int64
IsMissing int
}

type TagType struct {
Expand Down Expand Up @@ -353,6 +354,8 @@ type MediaDBI interface {
GetLastIndexedSystem() (string, error)
SetIndexingSystems(systemIDs []string) error
GetIndexingSystems() ([]string, error)
MarkSystemsMediaMissing(systemIDs []string) error
MarkAllMediaMissing() error
TruncateSystems(systemIDs []string) error

SearchMediaPathExact(systems []systemdefs.System, query string) ([]SearchResult, error)
Expand Down
9 changes: 6 additions & 3 deletions pkg/database/mediadb/batch_inserter.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
)

// BatchInserter manages batched multi-row inserts for a specific table
type BatchInserter struct {

Check failure on line 33 in pkg/database/mediadb/batch_inserter.go

View workflow job for this annotation

GitHub Actions / Lint (macos-latest)

fieldalignment: struct with 152 pointer bytes could be 112 (govet)

Check failure on line 33 in pkg/database/mediadb/batch_inserter.go

View workflow job for this annotation

GitHub Actions / Lint (ubuntu-latest)

fieldalignment: struct with 152 pointer bytes could be 112 (govet)

Check failure on line 33 in pkg/database/mediadb/batch_inserter.go

View workflow job for this annotation

GitHub Actions / Lint (windows-latest)

fieldalignment: struct with 152 pointer bytes could be 112 (govet)
ctx context.Context
tx *sql.Tx
tableName string
Expand All @@ -41,6 +41,7 @@
columnCount int
currentCount int
orIgnore bool
onConflict string
}

// NewBatchInserter creates a batch inserter for the given table
Expand All @@ -51,7 +52,7 @@
columns []string,
batchSize int,
) (*BatchInserter, error) {
return NewBatchInserterWithOptions(ctx, tx, tableName, columns, batchSize, false)
return NewBatchInserterWithOptions(ctx, tx, tableName, columns, batchSize, false, "")
}

// NewBatchInserterWithOptions creates a batch inserter with OR IGNORE option
Expand All @@ -62,6 +63,7 @@
columns []string,
batchSize int,
orIgnore bool,
onConflict string,
) (*BatchInserter, error) {
if tx == nil {
return nil, errors.New("transaction is nil")
Expand All @@ -86,6 +88,7 @@
buffer: make([]any, 0, batchSize*len(columns)),
currentCount: 0,
orIgnore: orIgnore,
onConflict: onConflict,
}, nil
}

Expand Down Expand Up @@ -306,7 +309,7 @@
placeholder := "(" + strings.Repeat("?, ", b.columnCount-1) + "?)"
placeholders := strings.Repeat(placeholder+",\n ", rowCount-1) + placeholder

return fmt.Sprintf("%s INTO %s (%s) VALUES\n %s", insertKeyword, b.tableName, colNames, placeholders)
return fmt.Sprintf("%s INTO %s (%s) VALUES\n %s \n%s", insertKeyword, b.tableName, colNames, placeholders, b.onConflict)
}

// generateSingleRowInsertSQL creates a single-row INSERT statement
Expand All @@ -318,5 +321,5 @@

colNames := strings.Join(b.columns, ", ")
placeholders := strings.Repeat("?, ", b.columnCount-1) + "?"
return fmt.Sprintf("%s INTO %s (%s) VALUES (%s)", insertKeyword, b.tableName, colNames, placeholders)
return fmt.Sprintf("%s INTO %s (%s) VALUES (%s) %s", insertKeyword, b.tableName, colNames, placeholders, b.onConflict)
}
2 changes: 1 addition & 1 deletion pkg/database/mediadb/batch_inserter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ func TestBatchInserter_OrIgnoreDuplicates(t *testing.T) {

// Create batch inserter with OR IGNORE
bi, err := NewBatchInserterWithOptions(ctx, tx, "test_table",
[]string{"DBID", "SystemID", "Name"}, 10, true)
[]string{"DBID", "SystemID", "Name"}, 10, true, "")
require.NoError(t, err)

// Add rows including duplicates
Expand Down
38 changes: 31 additions & 7 deletions pkg/database/mediadb/mediadb.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,28 @@ func (db *MediaDB) TruncateSystems(systemIDs []string) error {
return nil
}

func (db *MediaDB) MarkSystemsMediaMissing(systemIDs []string) error {
if db.sql == nil {
return ErrNullSQL
}
err := sqlMarkSystemsMediaMissing(db.ctx, db.sql, systemIDs)
if err != nil {
return err
}
return nil
}

func (db *MediaDB) MarkAllMediaMissing() error {
if db.sql == nil {
return ErrNullSQL
}
err := sqlMarkAllMediaMissing(db.ctx, db.sql)
if err != nil {
return err
}
return nil
}

func (db *MediaDB) Allocate() error {
if db.sql == nil {
return ErrNullSQL
Expand Down Expand Up @@ -595,39 +617,41 @@ func (db *MediaDB) BeginTransaction(batchEnabled bool) error {
// - This corrupt DBID is then used as FK in child tables → FK constraint violations
// - Better to fail fast with UNIQUE constraint error than continue with bad state
if db.batchInsertSystem, err = NewBatchInserterWithOptions(db.ctx, tx, "Systems",
[]string{"DBID", "SystemID", "Name"}, db.batchSize, false); err != nil {
[]string{"DBID", "SystemID", "Name"}, db.batchSize, false, ""); err != nil {
db.rollbackAndLogError()
return fmt.Errorf("failed to create batch inserter for systems: %w", err)
}

if db.batchInsertMediaTitle, err = NewBatchInserterWithOptions(db.ctx, tx, "MediaTitles",
[]string{"DBID", "SystemDBID", "Slug", "Name", "SlugLength", "SlugWordCount", "SecondarySlug"},
db.batchSize, false); err != nil {
db.batchSize, false, ""); err != nil {
db.rollbackAndLogError()
return fmt.Errorf("failed to create batch inserter for media titles: %w", err)
}

if db.batchInsertMedia, err = NewBatchInserterWithOptions(db.ctx, tx, "Media",
[]string{"DBID", "MediaTitleDBID", "SystemDBID", "Path"}, db.batchSize, false); err != nil {
[]string{"DBID", "MediaTitleDBID", "SystemDBID", "Path", "IsMissing"}, db.batchSize, false,
` ON CONFLICT (SystemDBID, Path) DO UPDATE SET IsMissing = 0 ON CONFLICT (DBID) DO UPDATE SET IsMissing = 0 `,
); err != nil {
db.rollbackAndLogError()
return fmt.Errorf("failed to create batch inserter for media: %w", err)
}

if db.batchInsertTag, err = NewBatchInserterWithOptions(db.ctx, tx, "Tags",
[]string{"DBID", "TypeDBID", "Tag"}, db.batchSize, false); err != nil {
[]string{"DBID", "TypeDBID", "Tag"}, db.batchSize, false, ""); err != nil {
db.rollbackAndLogError()
return fmt.Errorf("failed to create batch inserter for tags: %w", err)
}

if db.batchInsertTagType, err = NewBatchInserterWithOptions(db.ctx, tx, "TagTypes",
[]string{"DBID", "Type"}, db.batchSize, false); err != nil {
[]string{"DBID", "Type"}, db.batchSize, false, ""); err != nil {
db.rollbackAndLogError()
return fmt.Errorf("failed to create batch inserter for tag types: %w", err)
}

// MediaTags uses INSERT OR IGNORE - it's a link table with no dependent foreign keys
if db.batchInsertMediaTag, err = NewBatchInserterWithOptions(db.ctx, tx, "MediaTags",
[]string{"MediaDBID", "TagDBID"}, db.batchSize, true); err != nil {
[]string{"MediaDBID", "TagDBID"}, db.batchSize, true, ""); err != nil {
db.rollbackAndLogError()
return fmt.Errorf("failed to create batch inserter for media tags: %w", err)
}
Expand Down Expand Up @@ -1621,7 +1645,7 @@ func (db *MediaDB) InsertMedia(row database.Media) (database.Media, error) {

// Use batch inserter if available
if db.batchInsertMedia != nil {
err = db.batchInsertMedia.Add(row.DBID, row.MediaTitleDBID, row.SystemDBID, row.Path)
err = db.batchInsertMedia.Add(row.DBID, row.MediaTitleDBID, row.SystemDBID, row.Path, row.IsMissing)
if err != nil {
return row, fmt.Errorf("failed to add media to batch: %w", err)
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/database/mediadb/migrations/20260304011734_media_ismissing.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- +goose Up

-- Add New IsMissing Field for rescan matching
ALTER TABLE Media
ADD IsMissing integer NOT NULL DEFAULT 0;

-- +goose Down

-- Remove New IsMissing Field for rescan matching
ALTER TABLE Media
DROP COLUMN IsMissing;
30 changes: 30 additions & 0 deletions pkg/database/mediadb/sql_maintenance.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,36 @@
return nil
}

func sqlMarkSystemsMediaMissing(ctx context.Context, db *sql.DB, systemIDs []string) error {
if len(systemIDs) == 0 {
return nil
}

// Create placeholders for IN clause
placeholders := prepareVariadic("?", ",", len(systemIDs))

// Convert systemIDs to interface slice for query parameters
args := make([]any, len(systemIDs))
for i, id := range systemIDs {
args[i] = id
}
deleteStmt := fmt.Sprintf("UPDATE Media SET IsMissing = 1 FROM Systems WHERE Systems.DBID = Media.SystemDBID AND SystemID IN (%s)", placeholders)

Check failure on line 153 in pkg/database/mediadb/sql_maintenance.go

View workflow job for this annotation

GitHub Actions / Lint (macos-latest)

G201: SQL string formatting (gosec)

Check failure on line 153 in pkg/database/mediadb/sql_maintenance.go

View workflow job for this annotation

GitHub Actions / Lint (ubuntu-latest)

G201: SQL string formatting (gosec)

Check failure on line 153 in pkg/database/mediadb/sql_maintenance.go

View workflow job for this annotation

GitHub Actions / Lint (windows-latest)

G201: SQL string formatting (gosec)
_, err := db.ExecContext(ctx, deleteStmt, args...)
if err != nil {
return fmt.Errorf("failed to mark systems media missing: %w", err)
}

return nil
}

func sqlMarkAllMediaMissing(ctx context.Context, db *sql.DB) error {
_, err := db.ExecContext(ctx, "UPDATE Media SET IsMissing = 1")
if err != nil {
return fmt.Errorf("failed to mark all media missing: %w", err)
}
return nil
}

func sqlVacuum(ctx context.Context, db *sql.DB) error {
sqlStmt := `
vacuum;
Expand Down
14 changes: 11 additions & 3 deletions pkg/database/mediadb/sql_media.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,15 @@ import (
"github.com/rs/zerolog/log"
)

const insertMediaSQL = `INSERT INTO Media (DBID, MediaTitleDBID, SystemDBID, Path) VALUES (?, ?, ?, ?)`
const insertMediaSQL = `INSERT INTO Media
(DBID, MediaTitleDBID, SystemDBID, Path, IsMissing)
VALUES (?, ?, ?, ?, ?)
ON CONFLICT (SystemDBID, Path)
DO UPDATE SET
IsMissing = 0
ON CONFLICT (DBID)
DO UPDATE SET
IsMissing = 0;`
Comment on lines +33 to +41
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: During a selective rescan, the ON CONFLICT clause for Media updates IsMissing but fails to update MediaTitleDBID, leaving a stale reference and creating orphaned records.
Severity: HIGH

Suggested Fix

Modify the ON CONFLICT (SystemDBID, Path) clause in the InsertMedia function. The DO UPDATE statement should also update the MediaTitleDBID to the new value, for example: ... DO UPDATE SET IsMissing = 0, MediaTitleDBID = excluded.MediaTitleDBID.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: pkg/database/mediadb/sql_media.go#L33-L41

Potential issue: During a selective rescan, the logic in `AddMediaPath` creates a new
`MediaTitle` record for a file, but the subsequent `InsertMedia` call uses an `ON
CONFLICT` clause that only updates the `IsMissing` flag on the existing `Media` row. It
fails to update the `MediaTitleDBID` to point to the newly created title record. This
results in the `Media` row retaining a stale reference to an old `MediaTitle`, leading
to incorrect metadata associations and orphaned `MediaTitle` entries in the database.


func sqlFindMedia(ctx context.Context, db sqlQueryable, media database.Media) (database.Media, error) {
var row database.Media
Expand Down Expand Up @@ -83,7 +91,7 @@ func sqlInsertMediaWithPreparedStmt(ctx context.Context, stmt *sql.Stmt, row dat
dbID = row.DBID
}

res, err := stmt.ExecContext(ctx, dbID, row.MediaTitleDBID, row.SystemDBID, row.Path)
res, err := stmt.ExecContext(ctx, dbID, row.MediaTitleDBID, row.SystemDBID, row.Path, row.IsMissing)
if err != nil {
return row, fmt.Errorf("failed to execute prepared insert media statement: %w", err)
}
Expand Down Expand Up @@ -113,7 +121,7 @@ func sqlInsertMedia(ctx context.Context, db *sql.DB, row database.Media) (databa
}
}()

res, err := stmt.ExecContext(ctx, dbID, row.MediaTitleDBID, row.SystemDBID, row.Path)
res, err := stmt.ExecContext(ctx, dbID, row.MediaTitleDBID, row.SystemDBID, row.Path, row.IsMissing)
if err != nil {
return row, fmt.Errorf("failed to execute insert media statement: %w", err)
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/database/mediascanner/indexing_pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@
Path: pf.Path,
MediaTitleDBID: int64(titleIndex),
SystemDBID: int64(systemIndex),
IsMissing: 0,
})
if err != nil {
ss.MediaIndex-- // Rollback index increment on failure
Expand All @@ -169,6 +170,16 @@
ss.MediaIDs[mediaKey] = mediaIndex
} else {
mediaIndex = foundMediaIndex
// Update anyway to mark found
db.InsertMedia(database.Media{

Check failure on line 174 in pkg/database/mediascanner/indexing_pipeline.go

View workflow job for this annotation

GitHub Actions / Lint (macos-latest)

Error return value of `db.InsertMedia` is not checked (errcheck)

Check failure on line 174 in pkg/database/mediascanner/indexing_pipeline.go

View workflow job for this annotation

GitHub Actions / Lint (ubuntu-latest)

Error return value of `db.InsertMedia` is not checked (errcheck)

Check failure on line 174 in pkg/database/mediascanner/indexing_pipeline.go

View workflow job for this annotation

GitHub Actions / Lint (windows-latest)

Error return value of `db.InsertMedia` is not checked (errcheck)
DBID: 0, // Use 0 for NULL binding to force CONFLICT constraint update
Path: pf.Path,
MediaTitleDBID: int64(titleIndex),
SystemDBID: int64(systemIndex),
IsMissing: 0,
})
// Don't process tags if found
return titleIndex, mediaIndex, nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above — since MediaIDs is empty, this else branch never actually runs right now. Every path takes the !ok branch and does a fresh insert (which upserts via ON CONFLICT). So the early return skipping tags is dead code until the maps are populated. Worth keeping in mind when addressing the maps question.

}

// Extract extension tag only if filename tags are enabled
Expand Down
58 changes: 36 additions & 22 deletions pkg/database/mediascanner/mediascanner.go
Original file line number Diff line number Diff line change
Expand Up @@ -661,45 +661,58 @@
}
}

// Full truncate - foreign keys not needed since we're deleting everything
log.Info().Msgf("performing full database truncation (indexing %d systems)", len(currentSystemIDs))
err = db.Truncate()
/*

Check failure on line 664 in pkg/database/mediascanner/mediascanner.go

View workflow job for this annotation

GitHub Actions / Lint (macos-latest)

commentedOutCode: may want to remove commented-out code (gocritic)

Check failure on line 664 in pkg/database/mediascanner/mediascanner.go

View workflow job for this annotation

GitHub Actions / Lint (ubuntu-latest)

commentedOutCode: may want to remove commented-out code (gocritic)

Check failure on line 664 in pkg/database/mediascanner/mediascanner.go

View workflow job for this annotation

GitHub Actions / Lint (windows-latest)

commentedOutCode: may want to remove commented-out code (gocritic)
// Full truncate - foreign keys not needed since we're deleting everything
log.Info().Msgf("performing full database truncation (indexing %d systems)", len(currentSystemIDs))
err = db.Truncate()
if err != nil {
return 0, fmt.Errorf("failed to truncate database: %w", err)
}
log.Info().Msg("database truncation completed")*/
err := db.MarkAllMediaMissing()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan for rows that stay IsMissing=1 after a scan finishes? Right now search queries don't filter on it, so removed files would still show up in results. Is the idea to add a cleanup step later, filter at query time, or leave them for a future orphan management UI?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BossRighteous we agreed that there really wouldn't be that many and it can be worried about later right?

if err != nil {
return 0, fmt.Errorf("failed to truncate database: %w", err)
return 0, fmt.Errorf("failed to mark all media is missing %v: %w", currentSystemIDs, err)
}
log.Info().Msg("database truncation completed")
log.Info().Msg("all media marked missing for rescan")
} else {
// Selective indexing
// DELETE mode disables FKs for performance, but TruncateSystems() relies on CASCADE
// to properly delete Media/MediaTitles/MediaTags when a System is deleted
log.Info().Msgf(
/*log.Info().Msgf(
"performing selective truncation for systems: %v",
currentSystemIDs,
)
err = db.TruncateSystems(currentSystemIDs)
if err != nil {
return 0, fmt.Errorf("failed to truncate systems %v: %w", currentSystemIDs, err)
}
log.Info().Msg("selective truncation completed")
log.Info().Msg("selective truncation completed")*/

// For selective indexing, populate scan state with max IDs, global data, and
// maps for systems NOT being reindexed to avoid conflicts with existing data
log.Info().Msgf(
"Populating scan state for selective indexing (excluding systems: %v)", currentSystemIDs,
)
if err = PopulateScanStateForSelectiveIndexing(ctx, db, &scanState, currentSystemIDs); err != nil {
// Check if this is a cancellation error
if errors.Is(err, context.Canceled) {
return handleCancellation(
ctx, db, "Media indexing cancelled during selective scan state population",
)
}
log.Error().Err(err).Msg("failed to populate scan state for selective indexing")
return 0, fmt.Errorf("failed to populate scan state for selective indexing: %w", err)
err := db.MarkSystemsMediaMissing(currentSystemIDs)
if err != nil {
return 0, fmt.Errorf("failed to mark systems media is missing %v: %w", currentSystemIDs, err)
}
log.Info().Msg("successfully populated scan state for selective indexing")
log.Info().Msg("systems media marked missing for rescan")

}

// For rescan indexing, populate scan state with max IDs, global data, and
// maps for systems NOT being reindexed to avoid conflicts with existing data
log.Info().Msgf(
"Populating scan state for selective indexing (excluding systems: %v)", currentSystemIDs,
)
if err = PopulateScanStateForSelectiveIndexing(ctx, db, &scanState, currentSystemIDs); err != nil {
// Check if this is a cancellation error
if errors.Is(err, context.Canceled) {
return handleCancellation(
ctx, db, "Media indexing cancelled during selective scan state population",
)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since data isn't truncated anymore, PopulateScanStateForSelectiveIndexing leaves TitleIDs/MediaIDs maps empty — so AddMediaPath will re-insert titles with new DBIDs each rescan. MediaTitles doesn't have a unique constraint so they'd duplicate. Is that a problem or is this handled somewhere I'm missing?

log.Error().Err(err).Msg("failed to populate scan state for selective indexing")
return 0, fmt.Errorf("failed to populate scan state for selective indexing: %w", err)
}
log.Info().Msg("successfully populated scan state for selective indexing")

if setErr := db.SetIndexingStatus(mediadb.IndexingStatusRunning); setErr != nil {
log.Error().Err(setErr).Msg("failed to set indexing status to running on fresh start")
}
Expand All @@ -709,6 +722,7 @@

// Selective indexing already seeds tags via PopulateScanStateForSelectiveIndexing;
// only seed here for full truncate where TagTypesIndex is still 0.

if scanState.TagTypesIndex == 0 {
log.Info().Msg("seeding known tags for fresh indexing")
// SeedCanonicalTags runs in its own non-batch transaction for safety.
Expand Down
13 changes: 13 additions & 0 deletions pkg/testing/helpers/db_mocks.go
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,19 @@ func (m *MockMediaDBI) GetIndexingSystems() ([]string, error) {
return nil, nil
}

func (m *MockMediaDBI) MarkSystemsMediaMissing(systemIDs []string) error {
args := m.Called(systemIDs)
if err := args.Error(0); err != nil {
return fmt.Errorf("mock operation failed: %w", err)
}
return nil
}

func (m *MockMediaDBI) MarkAllMediaMissing() error {
m.Called()
return nil
}

func (m *MockMediaDBI) TruncateSystems(systemIDs []string) error {
args := m.Called(systemIDs)
if err := args.Error(0); err != nil {
Expand Down
Loading