diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceDocumentEventHandler.cs b/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceDocumentEventHandler.cs index bd1ba1e7..fd3206ab 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceDocumentEventHandler.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceDocumentEventHandler.cs @@ -1,7 +1,6 @@ using CrestApps.Core.AI.Documents.Models; using CrestApps.Core.AI.Documents.Tabular; using CrestApps.Core.AI.Models; -using Microsoft.Data.Sqlite; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -46,22 +45,18 @@ public async Task RemovedAsync(AIChatDocumentRemoveContext context, Cancellation /// private void TryDropDocumentTable(string referenceType, string referenceId, string documentId) { - if (string.IsNullOrEmpty(_basePath) || string.IsNullOrEmpty(referenceType) || string.IsNullOrEmpty(referenceId)) - { - return; - } + var databasePath = TabularWorkspaceDatabase.GetDatabasePath(_basePath, referenceType, referenceId); - var databasePath = Path.Combine(_basePath, "documents", referenceType, referenceId, "data", "tabular.db"); - - if (!File.Exists(databasePath)) + if (databasePath is null || !File.Exists(databasePath)) { return; } try { - using var connection = new SqliteConnection($"Data Source={databasePath}"); - connection.Open(); + // The workspace keeps its connection read-only between write windows, so the connection is + // opened through the shared helper that turns query_only back off before anything writes. + using var connection = TabularWorkspaceDatabase.Open(databasePath); // A single document can produce multiple tables (one per worksheet), so drop them all. var tableNames = new List(); @@ -115,18 +110,19 @@ SELECT COUNT(*) FROM "_workspace_meta" } catch (Exception ex) { - if (_logger.IsEnabled(LogLevel.Debug)) - { - _logger.LogDebug(ex, "Failed to drop tables for document '{DocumentId}' from workspace database.", documentId); - } + // The document was removed from the conversation but its data is still in the workspace + // database, so the model can keep querying it. That is worth surfacing rather than hiding + // behind a debug-level log. + _logger.LogWarning(ex, "Failed to drop tables for document '{DocumentId}' from workspace database.", documentId); } } private void TryDeleteDatabaseFiles(string databasePath) { - TryDeleteFile(databasePath); - TryDeleteFile(databasePath + "-wal"); - TryDeleteFile(databasePath + "-shm"); + foreach (var path in TabularWorkspaceDatabase.GetDatabaseFilePaths(databasePath)) + { + TryDeleteFile(path); + } } private void TryDeleteFile(string path) diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceHistoryClearedHandler.cs b/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceHistoryClearedHandler.cs index 9d236d58..b4799171 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceHistoryClearedHandler.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Handlers/TabularWorkspaceHistoryClearedHandler.cs @@ -1,4 +1,5 @@ using CrestApps.Core.AI.Chat; +using CrestApps.Core.AI.Documents.Tabular; using CrestApps.Core.AI.Models; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -44,10 +45,17 @@ public Task HistoryClearedAsync( return Task.CompletedTask; } - var databasePath = Path.Combine(_basePath, "documents", AIReferenceTypes.Document.ChatInteraction, interaction.ItemId, "data", "tabular.db"); - TryDeleteFile(databasePath); - TryDeleteFile(databasePath + "-wal"); - TryDeleteFile(databasePath + "-shm"); + var databasePath = TabularWorkspaceDatabase.GetDatabasePath(_basePath, AIReferenceTypes.Document.ChatInteraction, interaction.ItemId); + + if (databasePath is null) + { + return Task.CompletedTask; + } + + foreach (var path in TabularWorkspaceDatabase.GetDatabaseFilePaths(databasePath)) + { + TryDeleteFile(path); + } return Task.CompletedTask; } diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Services/DefaultConversationDocumentCleanupService.cs b/src/Primitives/CrestApps.Core.AI.Documents/Services/DefaultConversationDocumentCleanupService.cs index 164fabdb..1a657a1a 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Services/DefaultConversationDocumentCleanupService.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Services/DefaultConversationDocumentCleanupService.cs @@ -1,4 +1,3 @@ -using System.Text.RegularExpressions; using CrestApps.Core.AI.Documents.Generation; using CrestApps.Core.AI.Documents.Tabular; using CrestApps.Core.AI.Models; @@ -17,8 +16,6 @@ namespace CrestApps.Core.AI.Documents.Services; /// public sealed class DefaultConversationDocumentCleanupService : IConversationDocumentCleanupService { - private static readonly Regex _safePathSegmentExpression = new("^[a-zA-Z0-9._-]+$", RegexOptions.Compiled); - private readonly IAIDocumentStore _documentStore; private readonly IAIDocumentChunkStore _chunkStore; private readonly IDocumentFileStore _fileStore; @@ -64,11 +61,6 @@ public async Task CleanupAsync(string referenceId, string referenceType, Cancell var documents = await _documentStore.GetDocumentsAsync(referenceId, referenceType); - if (documents.Count == 0) - { - return; - } - foreach (var document in documents) { cancellationToken.ThrowIfCancellationRequested(); @@ -76,9 +68,12 @@ public async Task CleanupAsync(string referenceId, string referenceType, Cancell await DeleteDocumentAsync(document, cancellationToken); } + // Runs even when the conversation has no documents left. The workspace database is a separate + // file that outlives the document rows, so a conversation whose spreadsheets were each removed + // one by one still has a database to delete, and returning early would orphan it forever. TryDeleteTabularDatabase(referenceType, referenceId); - if (_logger.IsEnabled(LogLevel.Debug)) + if (documents.Count > 0 && _logger.IsEnabled(LogLevel.Debug)) { _logger.LogDebug( "Removed {DocumentCount} document(s) for conversation '{ReferenceId}' of type '{ReferenceType}'.", @@ -142,15 +137,17 @@ private async Task DeleteDocumentAsync(AIDocument document, CancellationToken ca private void TryDeleteTabularDatabase(string referenceType, string referenceId) { - var databasePath = BuildTabularDatabaseStoragePath(referenceType, referenceId); + var databasePath = TabularWorkspaceDatabase.GetStorageRelativePath(referenceType, referenceId); + if (databasePath is null) { return; } - TryDeleteFile(databasePath); - TryDeleteFile(databasePath + "-wal"); - TryDeleteFile(databasePath + "-shm"); + foreach (var path in TabularWorkspaceDatabase.GetDatabaseFilePaths(databasePath)) + { + TryDeleteFile(path); + } } private void TryDeleteFile(string path) @@ -167,23 +164,4 @@ private void TryDeleteFile(string path) } } } - - private static string BuildTabularDatabaseStoragePath(string referenceType, string referenceId) - { - if (!IsSafePathSegment(referenceType) || !IsSafePathSegment(referenceId)) - { - return null; - } - - return Path.Combine("documents", referenceType, referenceId, "data", "tabular.db") - .Replace(Path.DirectorySeparatorChar, '/') - .Replace(Path.AltDirectorySeparatorChar, '/'); - } - - private static bool IsSafePathSegment(string value) - { - return !string.IsNullOrWhiteSpace(value) - && value is not "." and not ".." - && _safePathSegmentExpression.IsMatch(value); - } } diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolContext.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolContext.cs index 1b951406..b6934040 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolContext.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolContext.cs @@ -251,12 +251,7 @@ private static string ResolveDatabasePath(IServiceProvider services, string refe var fileStoreOptions = services.GetRequiredService>().Value; - if (string.IsNullOrEmpty(fileStoreOptions.BasePath)) - { - return null; - } - - return Path.Combine(fileStoreOptions.BasePath, "documents", referenceType, referenceId, "data", "tabular.db"); + return TabularWorkspaceDatabase.GetDatabasePath(fileStoreOptions.BasePath, referenceType, referenceId); } private static AIChatSession ResolveSession() diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs index 8f7a31b0..fc3e479b 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs @@ -120,6 +120,7 @@ public async Task> EnsureReadyAsync( try { + RemoveTablesForDetachedDocuments(documents); await SynchronizeTablesAsync(documents, artifactLoader, workspaceImporter, cancellationToken); } finally @@ -559,16 +560,7 @@ private void EnsureLoaded() /// The connection to toggle. /// When , writes are allowed; otherwise they are blocked. private static void SetWritable(SqliteConnection connection, bool writable) - { - if (connection is null) - { - return; - } - - using var command = connection.CreateCommand(); - command.CommandText = writable ? "PRAGMA query_only = OFF" : "PRAGMA query_only = ON"; - command.ExecuteNonQuery(); - } + => TabularWorkspaceDatabase.SetWritable(connection, writable); private async Task SynchronizeTablesAsync( IReadOnlyList documents, @@ -655,6 +647,80 @@ private async Task SynchronizeTablesAsync( } } + /// + /// Drops every table whose document is no longer attached to the conversation. + /// + /// + /// The workspace database outlives a single request, so a document removed from the conversation + /// leaves its tables behind and the model can keep querying data the user believes is gone. The + /// removal handler drops them eagerly; this pass is what makes the workspace self-correcting when + /// that never ran, failed, or the document disappeared by another route. The caller supplies the + /// complete document set for the scope, so anything else in the database is detached by + /// definition. Must run inside a write window. + /// + /// The documents currently attached to the conversation. + private void RemoveTablesForDetachedDocuments(IReadOnlyList documents) + { + if (_tables.Count == 0) + { + return; + } + + var attached = new HashSet(StringComparer.Ordinal); + + foreach (var document in documents) + { + attached.Add(document.DocumentId); + } + + List detached = null; + + foreach (var table in _tables.Values) + { + if (!attached.Contains(table.DocumentId)) + { + (detached ??= []).Add(table); + } + } + + if (detached is null) + { + return; + } + + foreach (var table in detached) + { + DropTable(table.TableName); + DeleteMetadataEntry(table.TableName); + _tables.Remove(table.TableName); + + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug( + "Dropped tabular table '{TableName}' because document '{DocumentId}' is no longer attached to the conversation.", + table.TableName, + table.DocumentId); + } + } + } + + private void DropTable(string tableName) + { + using var command = _connection.CreateCommand(); + command.CommandText = $"DROP TABLE IF EXISTS \"{tableName.Replace("\"", "\"\"", StringComparison.Ordinal)}\""; + command.ExecuteNonQuery(); + } + + private void DeleteMetadataEntry(string tableName) + { + using var command = _connection.CreateCommand(); + command.CommandText = $""" + DELETE FROM "{MetadataTableName}" WHERE table_name = $tableName + """; + command.Parameters.AddWithValue("$tableName", tableName); + command.ExecuteNonQuery(); + } + private bool IsDocumentLoaded(string documentId) { foreach (var table in _tables.Values) @@ -709,13 +775,7 @@ private static string AllocateTableName( private SqliteConnection OpenConnection() { - string connectionString; - - if (string.IsNullOrEmpty(_databasePath)) - { - connectionString = "Data Source=:memory:"; - } - else + if (!string.IsNullOrEmpty(_databasePath)) { var directory = Path.GetDirectoryName(_databasePath); @@ -723,12 +783,11 @@ private SqliteConnection OpenConnection() { Directory.CreateDirectory(directory); } - - connectionString = $"Data Source={_databasePath}"; } - var connection = new SqliteConnection(connectionString); - connection.Open(); + // Opens the connection with writes enabled so the journal-mode and metadata statements below + // succeed; the write window is closed again once the database is ready. + var connection = TabularWorkspaceDatabase.Open(_databasePath); EnableDoubleQuotedStringLiterals(connection); if (_logger.IsEnabled(LogLevel.Debug)) diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspaceDatabase.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspaceDatabase.cs new file mode 100644 index 00000000..71ef5582 --- /dev/null +++ b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspaceDatabase.cs @@ -0,0 +1,146 @@ +using System.Text.RegularExpressions; +using Microsoft.Data.Sqlite; + +namespace CrestApps.Core.AI.Documents.Tabular; + +/// +/// Centralizes how the file-backed tabular workspace database is located and opened so every caller +/// agrees on the path layout, the connection string, and the connection's initial write state. +/// +internal static partial class TabularWorkspaceDatabase +{ + private const string DatabaseFileName = "tabular.db"; + private const string DocumentsFolderName = "documents"; + private const string DataFolderName = "data"; + + /// + /// Builds the absolute path of the workspace database for a conversation scope. + /// + /// The document file store base path. + /// The reference type owning the workspace. + /// The reference identifier owning the workspace. + /// The database path, or when the scope is incomplete or unsafe. + public static string GetDatabasePath(string basePath, string referenceType, string referenceId) + { + if (string.IsNullOrEmpty(basePath) || !IsSafeScope(referenceType, referenceId)) + { + return null; + } + + return Path.Combine(basePath, DocumentsFolderName, referenceType, referenceId, DataFolderName, DatabaseFileName); + } + + /// + /// Builds the workspace database path relative to the document file store, using forward slashes. + /// + /// The reference type owning the workspace. + /// The reference identifier owning the workspace. + /// The store-relative path, or when the scope is incomplete or unsafe. + public static string GetStorageRelativePath(string referenceType, string referenceId) + { + if (!IsSafeScope(referenceType, referenceId)) + { + return null; + } + + return string.Join('/', DocumentsFolderName, referenceType, referenceId, DataFolderName, DatabaseFileName); + } + + /// + /// Determines whether a scope maps to exactly one directory of its own. + /// + /// + /// Each conversation scope owns a separate database file, so isolation between chat sessions and + /// chat interactions rests entirely on these two segments. Restricting them to a single path + /// segment keeps a scope from ever resolving into another scope's directory, whatever the + /// identifiers turn out to contain. + /// + /// The reference type owning the workspace. + /// The reference identifier owning the workspace. + /// when both segments are safe; otherwise . + private static bool IsSafeScope(string referenceType, string referenceId) + { + return IsSafePathSegment(referenceType) && IsSafePathSegment(referenceId); + } + + private static bool IsSafePathSegment(string value) + { + return !string.IsNullOrWhiteSpace(value) + && value is not "." and not ".." + && SafePathSegmentExpression().IsMatch(value); + } + + [GeneratedRegex("^[a-zA-Z0-9._-]+$")] + private static partial Regex SafePathSegmentExpression(); + + /// + /// Returns the database file and its write-ahead-log sidecars, which must be removed together. + /// + /// The database path. + /// The database path followed by its -wal and -shm sidecars. + public static string[] GetDatabaseFilePaths(string databasePath) + { + return [databasePath, databasePath + "-wal", databasePath + "-shm"]; + } + + /// + /// Builds the connection string for a workspace database. + /// + /// + /// Connection pooling is disabled for file-backed workspaces. A pooled connection keeps the + /// underlying SQLite handle open after , which both leaks + /// connection-level state such as the query_only pragma to the next caller that opens the + /// same file and holds a file lock that prevents the database from being deleted. The workspace + /// opens a single long-lived connection per scope, so pooling buys nothing here. + /// + /// The database path, or for an in-memory workspace. + /// The connection string. + public static string BuildConnectionString(string databasePath) + { + if (string.IsNullOrEmpty(databasePath)) + { + return "Data Source=:memory:"; + } + + return new SqliteConnectionStringBuilder + { + DataSource = databasePath, + Pooling = false, + }.ToString(); + } + + /// + /// Opens a connection to a workspace database and puts it into a known-writable state. + /// + /// The database path, or for an in-memory workspace. + /// The open connection. + public static SqliteConnection Open(string databasePath) + { + var connection = new SqliteConnection(BuildConnectionString(databasePath)); + connection.Open(); + + // Never inherit the write state from whatever opened this database last. The workspace runs + // most of its life with query_only turned on, so a caller that assumed the SQLite default + // would otherwise fail with "attempt to write a readonly database". + SetWritable(connection, true); + + return connection; + } + + /// + /// Toggles SQLite's connection-level query_only flag. + /// + /// The connection to toggle. + /// When , writes are allowed; otherwise they are blocked. + public static void SetWritable(SqliteConnection connection, bool writable) + { + if (connection is null) + { + return; + } + + using var command = connection.CreateCommand(); + command.CommandText = writable ? "PRAGMA query_only = OFF" : "PRAGMA query_only = ON"; + command.ExecuteNonQuery(); + } +} diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Handlers/TabularWorkspaceHistoryClearedHandlerTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Handlers/TabularWorkspaceHistoryClearedHandlerTests.cs new file mode 100644 index 00000000..3e0cd701 --- /dev/null +++ b/tests/CrestApps.Core.Tests/Core/Documents/Handlers/TabularWorkspaceHistoryClearedHandlerTests.cs @@ -0,0 +1,124 @@ +using CrestApps.Core.AI.Documents; +using CrestApps.Core.AI.Documents.Handlers; +using CrestApps.Core.AI.Models; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; + +namespace CrestApps.Core.Tests.Core.Documents.Handlers; + +/// +/// Clearing a chat interaction's history must take its workspace database with it, so the next tool +/// call rebuilds from the original uploaded files instead of serving mutations from the cleared +/// conversation — and must never reach a database that belongs to a different interaction. +/// +public sealed class TabularWorkspaceHistoryClearedHandlerTests : IDisposable +{ + private readonly string _basePath; + + public TabularWorkspaceHistoryClearedHandlerTests() + { + _basePath = Path.Combine(Path.GetTempPath(), "tabular-history-cleared-tests", Path.GetRandomFileName()); + Directory.CreateDirectory(_basePath); + } + + public void Dispose() + { + try + { + Directory.Delete(_basePath, recursive: true); + } + catch (IOException) + { + } + } + + [Fact] + public async Task HistoryClearedAsync_DeletesTheDatabaseAndItsWriteAheadLogSidecars() + { + var paths = CreateDatabaseFiles("interaction-1"); + + await CreateHandler().HistoryClearedAsync( + new ChatInteraction { ItemId = "interaction-1" }, + [], + TestContext.Current.CancellationToken); + + Assert.All(paths, path => Assert.False(File.Exists(path), $"Expected '{path}' to be deleted.")); + } + + [Fact] + public async Task HistoryClearedAsync_LeavesOtherInteractionsDatabasesAlone() + { + var cleared = CreateDatabaseFiles("interaction-1"); + var untouched = CreateDatabaseFiles("interaction-2"); + + await CreateHandler().HistoryClearedAsync( + new ChatInteraction { ItemId = "interaction-1" }, + [], + TestContext.Current.CancellationToken); + + Assert.All(cleared, path => Assert.False(File.Exists(path))); + Assert.All(untouched, path => Assert.True(File.Exists(path), $"Expected '{path}' to survive.")); + } + + [Fact] + public async Task HistoryClearedAsync_DoesNotReachTheChatSessionDatabaseOfTheSameIdentifier() + { + var interactionFiles = CreateDatabaseFiles("shared-id"); + var sessionFiles = CreateDatabaseFiles("shared-id", "chat-session"); + + await CreateHandler().HistoryClearedAsync( + new ChatInteraction { ItemId = "shared-id" }, + [], + TestContext.Current.CancellationToken); + + Assert.All(interactionFiles, path => Assert.False(File.Exists(path))); + Assert.All(sessionFiles, path => Assert.True(File.Exists(path), $"Expected '{path}' to survive.")); + } + + [Theory] + [InlineData("..")] + [InlineData("interaction-1/../interaction-2")] + public async Task HistoryClearedAsync_WhenTheInteractionIdIsNotASinglePathSegment_DeletesNothing(string itemId) + { + var files = CreateDatabaseFiles("interaction-2"); + + await CreateHandler().HistoryClearedAsync( + new ChatInteraction { ItemId = itemId }, + [], + TestContext.Current.CancellationToken); + + Assert.All(files, path => Assert.True(File.Exists(path), $"Expected '{path}' to survive.")); + } + + [Fact] + public async Task HistoryClearedAsync_WhenTheInteractionIsMissing_DoesNothing() + { + var files = CreateDatabaseFiles("interaction-1"); + + await CreateHandler().HistoryClearedAsync(null, [], TestContext.Current.CancellationToken); + + Assert.All(files, path => Assert.True(File.Exists(path))); + } + + private string[] CreateDatabaseFiles(string referenceId, string referenceType = "chat-interaction") + { + var databasePath = Path.Combine(_basePath, "documents", referenceType, referenceId, "data", "tabular.db"); + Directory.CreateDirectory(Path.GetDirectoryName(databasePath)); + + string[] paths = [databasePath, databasePath + "-wal", databasePath + "-shm"]; + + foreach (var path in paths) + { + File.WriteAllText(path, "content"); + } + + return paths; + } + + private TabularWorkspaceHistoryClearedHandler CreateHandler() + { + return new TabularWorkspaceHistoryClearedHandler( + Options.Create(new DocumentFileSystemFileStoreOptions { BasePath = _basePath }), + NullLogger.Instance); + } +} diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Services/ConversationDocumentCleanupServiceTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Services/ConversationDocumentCleanupServiceTests.cs index f12c18a2..91b282e1 100644 --- a/tests/CrestApps.Core.Tests/Core/Documents/Services/ConversationDocumentCleanupServiceTests.cs +++ b/tests/CrestApps.Core.Tests/Core/Documents/Services/ConversationDocumentCleanupServiceTests.cs @@ -46,35 +46,106 @@ public async Task CleanupAsync_RemovesDocumentsFilesArtifactsAndChunks() fileStore.Verify(store => store.DeleteFileAsync(document.StoredFilePath), Times.Once); documentStore.Verify(store => store.DeleteAsync(document, It.IsAny()), Times.Once); } + + // The workspace database holds a copy of every spreadsheet, so deleting the conversation has + // to take the database with it. + foreach (var path in TabularDatabasePaths("chat-session", "session-1")) + { + fileStore.Verify(store => store.DeleteFileAsync(path), Times.Once); + } } [Fact] - public async Task CleanupAsync_WhenNoDocuments_DoesNothing() + public async Task CleanupAsync_WhenNoDocumentsRemain_StillDeletesTheTabularDatabase() { - var documentStore = new Mock(MockBehavior.Strict); - var chunkStore = new Mock(MockBehavior.Strict); - var fileStore = new Mock(MockBehavior.Strict); - var artifactStore = new Mock(MockBehavior.Strict); + var documentStore = new Mock(); + var fileStore = new Mock(); + // The conversation's spreadsheets were each removed individually, so no document rows remain. + // The database file is separate storage and survives that, so cleanup still has to delete it. documentStore .Setup(store => store.GetDocumentsAsync("session-1", "chat-session")) .ReturnsAsync([]); - var service = new DefaultConversationDocumentCleanupService( + var service = CreateService(documentStore, fileStore); + + await service.CleanupAsync("session-1", "chat-session", TestContext.Current.CancellationToken); + + foreach (var path in TabularDatabasePaths("chat-session", "session-1")) + { + fileStore.Verify(store => store.DeleteFileAsync(path), Times.Once); + } + } + + [Theory] + [InlineData("chat-session")] + [InlineData("chat-interaction")] + public async Task CleanupAsync_DeletesOnlyTheTabularDatabaseOfTheScopeBeingCleaned(string referenceType) + { + var documentStore = new Mock(); + var fileStore = new Mock(); + + documentStore + .Setup(store => store.GetDocumentsAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync([]); + + var service = CreateService(documentStore, fileStore); + + await service.CleanupAsync("scope-1", referenceType, TestContext.Current.CancellationToken); + + foreach (var path in TabularDatabasePaths(referenceType, "scope-1")) + { + fileStore.Verify(store => store.DeleteFileAsync(path), Times.Once); + } + + // Nothing outside the cleaned scope may be touched: not a sibling conversation, and not the + // same identifier under the other reference type. + var otherType = referenceType == "chat-session" ? "chat-interaction" : "chat-session"; + + foreach (var path in TabularDatabasePaths(referenceType, "scope-2").Concat(TabularDatabasePaths(otherType, "scope-1"))) + { + fileStore.Verify(store => store.DeleteFileAsync(path), Times.Never); + } + } + + [Theory] + [InlineData("..")] + [InlineData("scope-1/../scope-2")] + [InlineData("scope 1")] + public async Task CleanupAsync_WhenTheScopeIsNotASinglePathSegment_DeletesNoDatabase(string referenceId) + { + var documentStore = new Mock(); + var fileStore = new Mock(); + + documentStore + .Setup(store => store.GetDocumentsAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync([]); + + var service = CreateService(documentStore, fileStore); + + await service.CleanupAsync(referenceId, "chat-session", TestContext.Current.CancellationToken); + + fileStore.Verify(store => store.DeleteFileAsync(It.IsAny()), Times.Never); + } + + private static DefaultConversationDocumentCleanupService CreateService( + Mock documentStore, + Mock fileStore) + { + return new DefaultConversationDocumentCleanupService( documentStore.Object, - chunkStore.Object, + Mock.Of(), fileStore.Object, - artifactStore.Object, + Mock.Of(), Options.Create(new DocumentFileSystemFileStoreOptions { BasePath = Path.Combine(Path.GetTempPath(), "cleanup-tests") }), NullLogger.Instance); + } - await service.CleanupAsync("session-1", "chat-session", TestContext.Current.CancellationToken); + private static string[] TabularDatabasePaths(string referenceType, string referenceId) + { + var databasePath = $"documents/{referenceType}/{referenceId}/data/tabular.db"; - documentStore.Verify(store => store.GetDocumentsAsync("session-1", "chat-session"), Times.Once); - documentStore.VerifyNoOtherCalls(); - chunkStore.VerifyNoOtherCalls(); - fileStore.VerifyNoOtherCalls(); - artifactStore.VerifyNoOtherCalls(); + return [databasePath, databasePath + "-wal", databasePath + "-shm"]; } [Theory] diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceDocumentEventHandlerTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceDocumentEventHandlerTests.cs index f10ed8dd..f8992332 100644 --- a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceDocumentEventHandlerTests.cs +++ b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceDocumentEventHandlerTests.cs @@ -4,6 +4,7 @@ using CrestApps.Core.AI.Documents.Models; using CrestApps.Core.AI.Documents.Tabular; using CrestApps.Core.AI.Models; +using Microsoft.Data.Sqlite; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Moq; @@ -84,14 +85,230 @@ await handler.RemovedAsync(new AIChatDocumentRemoveContext Times.Once); } - private static TabularWorkspaceDocumentEventHandler CreateHandler(Mock artifactStore) + [Fact] + public async Task RemovedAsync_WhenTheDocumentOwnedTheLastTables_DeletesTheDatabaseFile() + { + var basePath = CreateTemporaryBasePath(); + + try + { + var databasePath = CreateWorkspaceDatabase(basePath, "session-1", "doc-1", "sales", "sales_q2"); + + var artifactStore = new Mock(); + var handler = CreateHandler(artifactStore, basePath); + + await handler.RemovedAsync(new AIChatDocumentRemoveContext + { + ReferenceId = "session-1", + ReferenceType = AIReferenceTypes.Document.ChatSession, + DocumentInfo = new ChatDocumentInfo + { + DocumentId = "doc-1", + FileName = "data.csv", + }, + }, TestContext.Current.CancellationToken); + + // The handler's own connection must release the file handle when it closes. A pooled + // connection keeps the SQLite handle open after Dispose and the delete silently fails. + Assert.False(File.Exists(databasePath)); + } + finally + { + TryDeleteDirectory(basePath); + } + } + + [Theory] + [InlineData("..")] + [InlineData("../session-2")] + [InlineData("session-1/../session-2")] + public async Task RemovedAsync_WhenTheScopeIsNotASinglePathSegment_LeavesEveryDatabaseAlone(string referenceId) + { + var basePath = CreateTemporaryBasePath(); + + try + { + // Each conversation scope owns its own database file, so a reference id that walks out of + // its directory would let one scope drop another scope's tables. + var databasePath = CreateWorkspaceDatabase(basePath, "session-2", "doc-1", "sales"); + + var artifactStore = new Mock(); + var handler = CreateHandler(artifactStore, basePath); + + await handler.RemovedAsync(new AIChatDocumentRemoveContext + { + ReferenceId = referenceId, + ReferenceType = AIReferenceTypes.Document.ChatSession, + DocumentInfo = new ChatDocumentInfo + { + DocumentId = "doc-1", + FileName = "data.csv", + }, + }, TestContext.Current.CancellationToken); + + Assert.True(File.Exists(databasePath)); + Assert.Equal(["sales"], GetUserTableNames(databasePath)); + } + finally + { + TryDeleteDirectory(basePath); + } + } + + [Fact] + public async Task RemovedAsync_WhenAnEarlierConnectionLeftTheDatabaseReadOnly_StillDropsTheDocumentTables() + { + var basePath = CreateTemporaryBasePath(); + + try + { + var databasePath = CreateWorkspaceDatabase(basePath, "session-1", "doc-1", "sales"); + AddWorkspaceTable(databasePath, "doc-2", "budget"); + + // The workspace keeps its own connection in query_only mode between write windows. A + // pooled connection carries that pragma over to the next caller that opens the same file, + // so every write then fails with "attempt to write a readonly database". + LeaveDatabaseReadOnlyInThePool(databasePath); + + var artifactStore = new Mock(); + var handler = CreateHandler(artifactStore, basePath); + + await handler.RemovedAsync(new AIChatDocumentRemoveContext + { + ReferenceId = "session-1", + ReferenceType = AIReferenceTypes.Document.ChatSession, + DocumentInfo = new ChatDocumentInfo + { + DocumentId = "doc-1", + FileName = "data.csv", + }, + }, TestContext.Current.CancellationToken); + + Assert.True(File.Exists(databasePath)); + Assert.Equal(["budget"], GetUserTableNames(databasePath)); + } + finally + { + TryDeleteDirectory(basePath); + } + } + + private static string CreateTemporaryBasePath() + { + var basePath = Path.Combine(Path.GetTempPath(), "tabular-doc-event-tests", Path.GetRandomFileName()); + Directory.CreateDirectory(basePath); + + return basePath; + } + + private static string CreateWorkspaceDatabase(string basePath, string referenceId, string documentId, params string[] tableNames) + { + var databasePath = Path.Combine(basePath, "documents", AIReferenceTypes.Document.ChatSession, referenceId, "data", "tabular.db"); + Directory.CreateDirectory(Path.GetDirectoryName(databasePath)); + + using (var connection = OpenUnpooled(databasePath)) + { + using var command = connection.CreateCommand(); + command.CommandText = """ + CREATE TABLE IF NOT EXISTS "_workspace_meta" ( + "table_name" TEXT PRIMARY KEY, + "document_id" TEXT NOT NULL, + "worksheet_name" TEXT, + "file_name" TEXT NOT NULL, + "source_names_json" TEXT NOT NULL + ) + """; + command.ExecuteNonQuery(); + } + + foreach (var tableName in tableNames) + { + AddWorkspaceTable(databasePath, documentId, tableName); + } + + return databasePath; + } + + private static void AddWorkspaceTable(string databasePath, string documentId, string tableName) + { + using var connection = OpenUnpooled(databasePath); + + using (var createCommand = connection.CreateCommand()) + { + createCommand.CommandText = $"CREATE TABLE \"{tableName}\" (\"value\" TEXT)"; + createCommand.ExecuteNonQuery(); + } + + using var insertCommand = connection.CreateCommand(); + insertCommand.CommandText = """ + INSERT INTO "_workspace_meta" ("table_name", "document_id", "worksheet_name", "file_name", "source_names_json") + VALUES ($tableName, $documentId, NULL, 'data.csv', '{}') + """; + insertCommand.Parameters.AddWithValue("$tableName", tableName); + insertCommand.Parameters.AddWithValue("$documentId", documentId); + insertCommand.ExecuteNonQuery(); + } + + private static void LeaveDatabaseReadOnlyInThePool(string databasePath) + { + // Uses the pooled connection string on purpose: disposing this connection returns it to the + // pool still carrying query_only = ON, which is what production hit. + using var connection = new SqliteConnection($"Data Source={databasePath}"); + connection.Open(); + + using var command = connection.CreateCommand(); + command.CommandText = "PRAGMA query_only = ON"; + command.ExecuteNonQuery(); + } + + private static List GetUserTableNames(string databasePath) + { + var names = new List(); + + using var connection = OpenUnpooled(databasePath); + + using var command = connection.CreateCommand(); + command.CommandText = "SELECT name FROM sqlite_master WHERE type = 'table' AND name <> '_workspace_meta' ORDER BY name"; + + using var reader = command.ExecuteReader(); + + while (reader.Read()) + { + names.Add(reader.GetString(0)); + } + + return names; + } + + private static SqliteConnection OpenUnpooled(string databasePath) + { + var connection = new SqliteConnection($"Data Source={databasePath};Pooling=False"); + connection.Open(); + + return connection; + } + + private static void TryDeleteDirectory(string path) + { + try + { + Directory.Delete(path, recursive: true); + } + catch (IOException) + { + } + } + + private static TabularWorkspaceDocumentEventHandler CreateHandler( + Mock artifactStore, + string basePath = null) { var options = new ChatDocumentsOptions(); options.Add(new ExtractorExtension(".csv", embeddable: false, isTabular: true)); var fileStoreOptions = new DocumentFileSystemFileStoreOptions { - BasePath = Path.Combine(Path.GetTempPath(), "tabular-doc-event-tests"), + BasePath = basePath ?? Path.Combine(Path.GetTempPath(), "tabular-doc-event-tests"), }; return new TabularWorkspaceDocumentEventHandler( diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs new file mode 100644 index 00000000..bdaf7fb9 --- /dev/null +++ b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs @@ -0,0 +1,286 @@ +using CrestApps.Core.AI.Documents.Tabular; +using Microsoft.Data.Sqlite; + +namespace CrestApps.Core.Tests.Core.Documents.Tabular; + +/// +/// Covers what happens to a file-backed workspace database across requests: tables must not outlive +/// the document they came from, and one conversation's database must never be reachable from another. +/// +public sealed class TabularWorkspaceLifecycleTests : IDisposable +{ + private const string SalesCsv = "region,amount\nNorth,100\nSouth,200"; + private const string BudgetCsv = "team,total\nOps,10\nEng,20"; + + private readonly string _root; + + public TabularWorkspaceLifecycleTests() + { + _root = Path.Combine(Path.GetTempPath(), "tabular-lifecycle-tests", Path.GetRandomFileName()); + Directory.CreateDirectory(_root); + } + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + } + } + + [Fact] + public async Task EnsureReadyAsync_WhenADocumentIsNoLongerAttached_DropsItsTablesOnTheNextRequest() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv"), Document("doc-2", "budget.csv")], + LoaderFor(("doc-1", SalesCsv), ("doc-2", BudgetCsv)), + cancellationToken); + } + + Assert.Equal(["budget", "sales"], GetUserTableNames(databasePath)); + + // The next request resolves only doc-1, so doc-2 is gone from the conversation. + using (var workspace = CreateWorkspace(databasePath)) + { + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("sales", table.TableName); + } + + // The table is really gone from the database, not merely hidden from the model. + Assert.Equal(["sales"], GetUserTableNames(databasePath)); + Assert.Equal(["sales"], GetMetadataTableNames(databasePath)); + } + + [Fact] + public async Task EnsureReadyAsync_WhenEveryDocumentIsDetached_DropsEveryTable() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + } + + using (var workspace = CreateWorkspace(databasePath)) + { + var tables = await workspace.EnsureReadyAsync([], LoaderFor(), cancellationToken); + + Assert.Empty(tables); + } + + Assert.Empty(GetUserTableNames(databasePath)); + Assert.Empty(GetMetadataTableNames(databasePath)); + } + + [Fact] + public async Task EnsureReadyAsync_WhenADetachedDocumentHadSeveralWorksheets_DropsAllOfItsTables() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + var multiSheet = new TabularDocumentArtifact + { + Worksheets = + [ + new TabularWorksheet { Name = "Q1", Header = ["region", "amount"], Rows = [["North", "100"]] }, + new TabularWorksheet { Name = "Q2", Header = ["region", "amount"], Rows = [["South", "200"]] }, + ], + }; + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.xlsx"), Document("doc-2", "budget.csv")], + (document, _) => Task.FromResult( + document.DocumentId == "doc-1" + ? multiSheet + : TabularDocumentArtifact.FromDelimitedContent(BudgetCsv, document.FileName)), + null, + cancellationToken); + } + + Assert.Equal(["budget", "sales_Q1", "sales_Q2"], GetUserTableNames(databasePath)); + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync( + [Document("doc-2", "budget.csv")], + LoaderFor(("doc-2", BudgetCsv)), + cancellationToken); + } + + Assert.Equal(["budget"], GetUserTableNames(databasePath)); + } + + [Fact] + public async Task EnsureReadyAsync_DoesNotReachAnotherConversationsDatabase() + { + var cancellationToken = TestContext.Current.CancellationToken; + var first = DatabasePath("session-1"); + var second = DatabasePath("session-2"); + + using (var workspace = CreateWorkspace(first)) + { + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + } + + using (var workspace = CreateWorkspace(second)) + { + await workspace.EnsureReadyAsync( + [Document("doc-2", "budget.csv")], + LoaderFor(("doc-2", BudgetCsv)), + cancellationToken); + } + + // Two conversations, two files, neither aware of the other's tables. + Assert.Equal(["sales"], GetUserTableNames(first)); + Assert.Equal(["budget"], GetUserTableNames(second)); + + // Detaching everything from the first conversation leaves the second untouched. + using (var workspace = CreateWorkspace(first)) + { + await workspace.EnsureReadyAsync([], LoaderFor(), cancellationToken); + } + + Assert.Empty(GetUserTableNames(first)); + Assert.Equal(["budget"], GetUserTableNames(second)); + } + + [Fact] + public async Task EnsureReadyAsync_ASecondConversationNeverSeesTheFirstConversationsDocument() + { + var cancellationToken = TestContext.Current.CancellationToken; + var first = DatabasePath("session-1"); + var second = DatabasePath("session-2"); + + using (var workspace = CreateWorkspace(first)) + { + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + } + + using var other = CreateWorkspace(second); + var tables = await other.EnsureReadyAsync( + [Document("doc-2", "budget.csv")], + LoaderFor(("doc-2", BudgetCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("budget", table.TableName); + + // The other conversation's table is not queryable from here. + var exception = await Assert.ThrowsAsync( + () => other.QueryAsync("SELECT * FROM sales", 10, cancellationToken)); + + Assert.Contains("no such table", exception.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task EnsureReadyAsync_ReattachingADroppedDocument_RebuildsItsTable() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + } + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync([], LoaderFor(), cancellationToken); + } + + using (var workspace = CreateWorkspace(databasePath)) + { + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("sales", table.TableName); + Assert.Equal(2, table.RowCount); + } + } + + private string DatabasePath(string referenceId) + { + var path = Path.Combine(_root, "documents", "chat-session", referenceId, "data", "tabular.db"); + Directory.CreateDirectory(Path.GetDirectoryName(path)); + + return path; + } + + private static TabularWorkspace CreateWorkspace(string databasePath) + { + return new TabularWorkspace(new TabularWorkspaceOptions(), databasePath); + } + + private static TabularDocumentRef Document(string documentId, string fileName) + { + return new TabularDocumentRef(documentId, fileName); + } + + private static Func> LoaderFor(params (string DocumentId, string Content)[] contents) + { + var map = contents.ToDictionary(c => c.DocumentId, c => c.Content, StringComparer.Ordinal); + + return (documentId, _) => Task.FromResult(map.TryGetValue(documentId, out var content) ? content : string.Empty); + } + + private static List GetUserTableNames(string databasePath) + { + return ReadStrings(databasePath, "SELECT name FROM sqlite_master WHERE type = 'table' AND name <> '_workspace_meta' ORDER BY name"); + } + + private static List GetMetadataTableNames(string databasePath) + { + return ReadStrings(databasePath, "SELECT table_name FROM \"_workspace_meta\" ORDER BY table_name"); + } + + private static List ReadStrings(string databasePath, string sql) + { + var values = new List(); + + using var connection = new SqliteConnection($"Data Source={databasePath};Pooling=False"); + connection.Open(); + + using var command = connection.CreateCommand(); + command.CommandText = sql; + + using var reader = command.ExecuteReader(); + + while (reader.Read()) + { + values.Add(reader.GetString(0)); + } + + return values; + } +}