Fix Critical: IndexSystem lock races and TrackRepository.Update silent-create

- Replace object lock with SemaphoreSlim(1,1) on both DirectoryIndexDirectory
  and VaultIndexDirectory; SaveIndexAsync now executes inside the semaphore
  so mutate+persist is atomic
- ReloadIndexAsync acquires the semaphore before LoadIndexAsync so disk load
  and in-memory swap are atomic with respect to concurrent writes
- HasIndexEntry and GetEntryMetadata converted to async Task with WaitAsync;
  MediaVault.GetEntryAsync call sites updated accordingly
- TrackRepository.Update throws InvalidOperationException when Id not found
  instead of silently calling Create; service layer catches and wraps as fail result
This commit is contained in:
Daniel Harvey
2026-05-17 11:24:50 -04:00
parent 56d15027e4
commit 4a0c17c318
6 changed files with 57 additions and 31 deletions
@@ -57,7 +57,7 @@ public abstract class IndexDirectory : AbstractIndexContainer
public int GetIndexSize() => Index.GetEntriesSize(); public int GetIndexSize() => Index.GetEntriesSize();
public bool HasIndexEntry(string entryId) => Index.HasEntry(entryId); public virtual Task<bool> HasIndexEntry(string entryId) => Task.FromResult(Index.HasEntry(entryId));
} }
/// <summary> /// <summary>
@@ -66,17 +66,26 @@ public abstract class IndexDirectory : AbstractIndexContainer
public class DirectoryIndexDirectory : IndexDirectory public class DirectoryIndexDirectory : IndexDirectory
{ {
private readonly IDirectoryIndex _directoryIndex; private readonly IDirectoryIndex _directoryIndex;
private readonly SemaphoreSlim _indexLock = new(1, 1);
public DirectoryIndexDirectory(string rootPath, IDirectoryIndex index, IIndexDataFactory? indexDataFactory = null) public DirectoryIndexDirectory(string rootPath, IDirectoryIndex index, IIndexDataFactory? indexDataFactory = null)
: base(rootPath, IndexType.Directory, index, indexDataFactory) : base(rootPath, IndexType.Directory, index, indexDataFactory)
{ {
_directoryIndex = index; _directoryIndex = index;
} }
protected async Task AddToIndexAsync(string entryId) protected async Task AddToIndexAsync(string entryId)
{ {
_directoryIndex.PutEntry(entryId); await _indexLock.WaitAsync();
await SaveIndexAsync(_directoryIndex); try
{
_directoryIndex.PutEntry(entryId);
await SaveIndexAsync(_directoryIndex);
}
finally
{
_indexLock.Release();
}
} }
} }
@@ -86,7 +95,7 @@ public class DirectoryIndexDirectory : IndexDirectory
public class VaultIndexDirectory : IndexDirectory public class VaultIndexDirectory : IndexDirectory
{ {
private IVaultIndex _vaultIndex; private IVaultIndex _vaultIndex;
private readonly object _indexLock = new(); private readonly SemaphoreSlim _indexLock = new(1, 1);
private readonly IndexFactoryService _factoryService = new(); private readonly IndexFactoryService _factoryService = new();
public VaultIndexDirectory(string rootPath, IVaultIndex index, IIndexDataFactory? indexDataFactory = null) public VaultIndexDirectory(string rootPath, IVaultIndex index, IIndexDataFactory? indexDataFactory = null)
@@ -97,11 +106,16 @@ public class VaultIndexDirectory : IndexDirectory
protected async Task AddToIndexAsync(string entryId, MetaData metaData) protected async Task AddToIndexAsync(string entryId, MetaData metaData)
{ {
lock (_indexLock) await _indexLock.WaitAsync();
try
{ {
_vaultIndex.PutEntry(entryId, metaData); _vaultIndex.PutEntry(entryId, metaData);
await SaveIndexAsync(_vaultIndex);
}
finally
{
_indexLock.Release();
} }
await SaveIndexAsync(_vaultIndex);
} }
/// <summary> /// <summary>
@@ -109,16 +123,14 @@ public class VaultIndexDirectory : IndexDirectory
/// </summary> /// </summary>
public async Task ReloadIndexAsync() public async Task ReloadIndexAsync()
{ {
await _indexLock.WaitAsync();
try try
{ {
var newIndex = await _factoryService.LoadIndexAsync(IndexType.Vault, RootPath); var newIndex = await _factoryService.LoadIndexAsync(IndexType.Vault, RootPath);
if (newIndex is IVaultIndex vaultIndex) if (newIndex is IVaultIndex vaultIndex)
{ {
lock (_indexLock) _vaultIndex = vaultIndex;
{ Index = vaultIndex;
_vaultIndex = vaultIndex;
Index = vaultIndex;
}
Console.WriteLine($"VaultIndexDirectory: Reloaded index for {RootPath}, {vaultIndex.GetEntriesSize()} entries"); Console.WriteLine($"VaultIndexDirectory: Reloaded index for {RootPath}, {vaultIndex.GetEntriesSize()} entries");
} }
} }
@@ -126,27 +138,41 @@ public class VaultIndexDirectory : IndexDirectory
{ {
Console.WriteLine($"VaultIndexDirectory: Failed to reload index for {RootPath}: {ex.Message}"); Console.WriteLine($"VaultIndexDirectory: Failed to reload index for {RootPath}: {ex.Message}");
} }
finally
{
_indexLock.Release();
}
} }
/// <summary> /// <summary>
/// Thread-safe check for index entry /// Thread-safe check for index entry
/// </summary> /// </summary>
public new bool HasIndexEntry(string entryId) public override async Task<bool> HasIndexEntry(string entryId)
{ {
lock (_indexLock) await _indexLock.WaitAsync();
try
{ {
return _vaultIndex.HasEntry(entryId); return _vaultIndex.HasEntry(entryId);
} }
finally
{
_indexLock.Release();
}
} }
/// <summary> /// <summary>
/// Thread-safe get entry metadata /// Thread-safe get entry metadata
/// </summary> /// </summary>
public MetaData? GetEntryMetadata(string entryId) public async Task<MetaData?> GetEntryMetadata(string entryId)
{ {
lock (_indexLock) await _indexLock.WaitAsync();
try
{ {
return _vaultIndex.GetEntry(entryId); return _vaultIndex.GetEntry(entryId);
} }
finally
{
_indexLock.Release();
}
} }
} }
@@ -64,11 +64,11 @@ public abstract class MediaVault : VaultIndexDirectory
var vaultType = MediaVaultTypeMap.GetVaultType<T>(); var vaultType = MediaVaultTypeMap.GetVaultType<T>();
// Use thread-safe method from VaultIndexDirectory // Use thread-safe method from VaultIndexDirectory
if (!HasIndexEntry(entryId)) if (!await HasIndexEntry(entryId))
return null; return null;
// Use thread-safe metadata retrieval // Use thread-safe metadata retrieval
var metaData = GetEntryMetadata(entryId); var metaData = await GetEntryMetadata(entryId);
if (metaData == null) if (metaData == null)
return null; return null;
+2 -2
View File
@@ -92,7 +92,7 @@ public class FileDatabaseTests
// Assert // Assert
var vault = _fileDatabase.GetVault(TestData.TestKeys.ImageVaultKey); var vault = _fileDatabase.GetVault(TestData.TestKeys.ImageVaultKey);
Assert.That(vault, Is.Not.Null, "Vault should not be null"); Assert.That(vault, Is.Not.Null, "Vault should not be null");
Assert.That(vault!.HasIndexEntry(TestData.TestKeys.TestImageEntry), Is.True, Assert.That(await vault!.HasIndexEntry(TestData.TestKeys.TestImageEntry), Is.True,
"Added image should be in the index"); "Added image should be in the index");
} }
@@ -182,7 +182,7 @@ public class FileDatabaseTests
Assert.That(reloadedDatabase.GetIndexSize(), Is.EqualTo(1), "Index count should be 1"); Assert.That(reloadedDatabase.GetIndexSize(), Is.EqualTo(1), "Index count should be 1");
// Verify vault exists // Verify vault exists
Assert.That(reloadedDatabase.HasIndexEntry(TestData.TestKeys.ImageVaultKey), Is.True, Assert.That(await reloadedDatabase.HasIndexEntry(TestData.TestKeys.ImageVaultKey), Is.True,
"Vault should be present in index"); "Vault should be present in index");
Assert.That(reloadedDatabase.HasVault(TestData.TestKeys.ImageVaultKey), Is.True, Assert.That(reloadedDatabase.HasVault(TestData.TestKeys.ImageVaultKey), Is.True,
"Vault should be present in vault collection"); "Vault should be present in vault collection");
+4 -4
View File
@@ -492,13 +492,13 @@ public class IndexSystemTests
await database!.CreateVaultAsync(testVaultKey, MediaVaultType.Image); await database!.CreateVaultAsync(testVaultKey, MediaVaultType.Image);
// Assert // Assert
Assert.That(database.HasIndexEntry(testVaultKey), Is.True, "Should contain added entry"); Assert.That(await database.HasIndexEntry(testVaultKey), Is.True, "Should contain added entry");
Assert.That(database.GetIndexSize(), Is.EqualTo(1), "Should have one entry"); Assert.That(database.GetIndexSize(), Is.EqualTo(1), "Should have one entry");
Assert.That(File.Exists(IndexPath), Is.True, "Should persist to file"); Assert.That(File.Exists(IndexPath), Is.True, "Should persist to file");
// Verify persistence by reloading database // Verify persistence by reloading database
var reloadedDatabase = await FileDatabase.FromAsync(TestDirectory); var reloadedDatabase = await FileDatabase.FromAsync(TestDirectory);
Assert.That(reloadedDatabase!.HasIndexEntry(testVaultKey), Is.True, "Should persist entry across restarts"); Assert.That(await reloadedDatabase!.HasIndexEntry(testVaultKey), Is.True, "Should persist entry across restarts");
} }
[Test] [Test]
@@ -516,13 +516,13 @@ public class IndexSystemTests
await vault!.AddEntryAsync(testKey, testImage); await vault!.AddEntryAsync(testKey, testImage);
// Assert // Assert
Assert.That(vault.HasIndexEntry(testKey), Is.True, "Should contain added entry"); Assert.That(await vault.HasIndexEntry(testKey), Is.True, "Should contain added entry");
Assert.That(vault.GetIndexSize(), Is.EqualTo(1), "Should have one entry"); Assert.That(vault.GetIndexSize(), Is.EqualTo(1), "Should have one entry");
Assert.That(File.Exists(IndexPath), Is.True, "Should persist to file"); Assert.That(File.Exists(IndexPath), Is.True, "Should persist to file");
// Verify persistence by reloading vault // Verify persistence by reloading vault
var reloadedVault = await ImageVault.FromAsync(TestDirectory); var reloadedVault = await ImageVault.FromAsync(TestDirectory);
Assert.That(reloadedVault!.HasIndexEntry(testKey), Is.True, "Should persist entry across restarts"); Assert.That(await reloadedVault!.HasIndexEntry(testKey), Is.True, "Should persist entry across restarts");
} }
} }
} }
+5 -5
View File
@@ -120,7 +120,7 @@ public class MediaVaultTests
await _imageVault.AddEntryAsync(entryKey, imageBinary); await _imageVault.AddEntryAsync(entryKey, imageBinary);
// Assert // Assert
Assert.That(_imageVault.HasIndexEntry(entryKey), Is.True, "Should add to index"); Assert.That(await _imageVault.HasIndexEntry(entryKey), Is.True, "Should add to index");
var expectedFilePath = Path.Combine(TestDirectory, "test-image.png"); var expectedFilePath = Path.Combine(TestDirectory, "test-image.png");
AssertMediaFileExists(expectedFilePath, imageBinary.Buffer); AssertMediaFileExists(expectedFilePath, imageBinary.Buffer);
@@ -148,7 +148,7 @@ public class MediaVaultTests
foreach (var (key, binary) in entries) foreach (var (key, binary) in entries)
{ {
Assert.That(_imageVault.HasIndexEntry(key), Is.True, $"Should contain {key} in index"); Assert.That(await _imageVault.HasIndexEntry(key), Is.True, $"Should contain {key} in index");
var expectedFilePath = Path.Combine(TestDirectory, $"{key}.png"); var expectedFilePath = Path.Combine(TestDirectory, $"{key}.png");
AssertMediaFileExists(expectedFilePath, binary.Buffer); AssertMediaFileExists(expectedFilePath, binary.Buffer);
} }
@@ -264,7 +264,7 @@ public class MediaVaultTests
await _audioVault.AddEntryAsync(entryKey, audioBinary); await _audioVault.AddEntryAsync(entryKey, audioBinary);
// Assert // Assert
Assert.That(_audioVault.HasIndexEntry(entryKey), Is.True, "Should add to index"); Assert.That(await _audioVault.HasIndexEntry(entryKey), Is.True, "Should add to index");
var expectedFilePath = Path.Combine(TestDirectory, "test-audio.mp3"); var expectedFilePath = Path.Combine(TestDirectory, "test-audio.mp3");
AssertMediaFileExists(expectedFilePath, audioBinary.Buffer); AssertMediaFileExists(expectedFilePath, audioBinary.Buffer);
@@ -337,7 +337,7 @@ public class MediaVaultTests
return (string)method!.Invoke(_vault, new object[] { mediaKey })!; return (string)method!.Invoke(_vault, new object[] { mediaKey })!;
} }
public bool HasIndexEntry(string entryId) => _vault.HasIndexEntry(entryId); public Task<bool> HasIndexEntry(string entryId) => _vault.HasIndexEntry(entryId);
public Task AddEntryAsync(string entryId, FileBinary media) => public Task AddEntryAsync(string entryId, FileBinary media) =>
_vault.AddEntryAsync(entryId, media); _vault.AddEntryAsync(entryId, media);
} }
@@ -423,7 +423,7 @@ public class MediaVaultTests
await vault!.AddEntryAsync(entryKey, imageBinary); await vault!.AddEntryAsync(entryKey, imageBinary);
// Assert // Assert
Assert.That(vault.HasIndexEntry(entryKey), Is.True, "Should add entry to index"); Assert.That(await vault.HasIndexEntry(entryKey), Is.True, "Should add entry to index");
var expectedFilePath = Path.Combine(TestDirectory, "test-media.png"); var expectedFilePath = Path.Combine(TestDirectory, "test-media.png");
AssertMediaFileExists(expectedFilePath, imageBinary.Buffer); AssertMediaFileExists(expectedFilePath, imageBinary.Buffer);
@@ -50,7 +50,7 @@ public class TrackRepository
if (trackEntity == null) if (trackEntity == null)
{ {
return await Create(track); throw new InvalidOperationException($"Track not found: {track.Id}");
} }
trackEntity.Album = track.Album; trackEntity.Album = track.Album;