From 83107add9d9618f54589afdc707985be63718240 Mon Sep 17 00:00:00 2001 From: Mike Alhayek Date: Mon, 14 Sep 2026 12:24:01 -0700 Subject: [PATCH] Never cache a table for a document that could not be read A document whose content could not be loaded was still turned into a table. FromDelimitedContent returns an empty artifact for empty content, and CreateTable answers an empty header with a placeholder "value" column and no rows. That table was then written to _workspace_meta, which made IsDocumentLoaded true for the rest of the conversation: the workspace served an empty table from then on and never read the file again. The model saw files with a placeholder column and no data and told the user to upload them again, which was the only way out. An import that yields no content now registers nothing, so the next request tries again, and an importer that returns no tables falls through to the artifact loader instead of marking the document done. TabularToolRunner says the files could not be read rather than handing back an empty workspace, which reads to the model as files that contain no data. Databases already holding one of these tables repair themselves: a table with the single placeholder column and no rows is dropped on the next request and its document imported again. The row check is what makes that safe to act on. A table emptied through the manipulation tool keeps the real columns of its source file, so a deliberate edit can never match, and re-importing something that does match cannot lose data because it holds none. Co-Authored-By: Claude Opus 5 --- .../Tabular/TabularToolRunner.cs | 17 +++ .../Tabular/TabularWorkspace.cs | 124 ++++++++++++++- .../Tabular/TabularWorkspaceLifecycleTests.cs | 144 ++++++++++++++++++ 3 files changed, 283 insertions(+), 2 deletions(-) diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs index 7d559e55..d496142a 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs @@ -57,6 +57,23 @@ public static async Task PrepareAsync(IServiceProvider servic context.ImportToWorkspaceAsync, cancellationToken); + if (tables.Count == 0) + { + // The conversation has tabular files but none of them could be read. Say so instead of + // handing back an empty workspace, which reads to the model as files that contain no data. + workspace.Dispose(); + + logger?.LogWarning( + "None of the {DocumentCount} tabular document(s) attached to this conversation could be loaded into the workspace.", + context.Documents.Count); + + return new PreparationResult( + null, + null, + null, + "The tabular files attached to this conversation could not be read. Their stored content is unavailable, so no data can be queried. Ask the user to upload the files again."); + } + if (logger?.IsEnabled(LogLevel.Debug) == true) { logger.LogDebug( diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs index fc3e479b..8d404206 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 { + RemoveFailedImportPlaceholderTables(); RemoveTablesForDetachedDocuments(documents); await SynchronizeTablesAsync(documents, artifactLoader, workspaceImporter, cancellationToken); } @@ -593,7 +594,10 @@ private async Task SynchronizeTablesAsync( var importResults = await workspaceImporter(document, _connection, allocator, cancellationToken); - if (importResults != null) + // An importer that produced nothing falls through to the artifact loader rather than + // marking the document imported, so a failed streaming import still gets a second + // chance and never leaves the document represented by no table at all. + if (importResults is { Count: > 0 }) { foreach (var importResult in importResults) { @@ -616,7 +620,23 @@ private async Task SynchronizeTablesAsync( } var artifact = await artifactLoader(document, cancellationToken); - var worksheets = artifact?.GetWorksheets() ?? [new TabularWorksheet()]; + var worksheets = (artifact?.GetWorksheets() ?? []).Where(HasContent).ToList(); + + // A document whose content could not be loaded (a missing artifact, missing chunks, or an + // unreadable file) must not be turned into a table. Creating one would write a placeholder + // column with no rows, register it in the metadata, and make IsDocumentLoaded true forever + // after, so the workspace would serve an empty table for the rest of the conversation and + // never retry the import. Leaving it unregistered means the next request tries again. + if (worksheets.Count == 0) + { + _logger.LogWarning( + "No tabular content could be loaded for document '{FileName}' ('{DocumentId}'), so no table was created. The import will be retried on the next request.", + document.FileName, + document.DocumentId); + + continue; + } + var singleWorksheetDocument = worksheets.Count == 1; foreach (var worksheet in worksheets) @@ -721,6 +741,106 @@ private void DeleteMetadataEntry(string tableName) command.ExecuteNonQuery(); } + /// + /// Discards tables left behind by an import that produced no content, so the document is read + /// again on this request. + /// + /// + /// Such a table was created before a failed import stopped being registered, and it is indelible + /// on its own: the metadata entry makes the document count as loaded, so the workspace serves an + /// empty table for the rest of the conversation and never reads the file again. This repairs a + /// database already in that state without waiting for the user to upload the files a second time. + /// Must run inside a write window. + /// + private void RemoveFailedImportPlaceholderTables() + { + if (_tables.Count == 0) + { + return; + } + + List placeholders = null; + + foreach (var table in _tables.Values) + { + if (IsFailedImportPlaceholder(table.TableName)) + { + (placeholders ??= []).Add(table); + } + } + + if (placeholders is null) + { + return; + } + + foreach (var table in placeholders) + { + DropTable(table.TableName); + DeleteMetadataEntry(table.TableName); + _tables.Remove(table.TableName); + + _logger.LogWarning( + "Discarded the empty table '{TableName}' left behind by a failed import of document '{DocumentId}'. The document will be imported again.", + table.TableName, + table.DocumentId); + } + } + + /// + /// Determines whether a table has the exact shape a failed import leaves behind: the single + /// placeholder value column and no rows. + /// + /// + /// The row check is what makes this safe to act on. A table the user emptied through a + /// manipulation tool keeps the real columns of its source file, so it can never match, and + /// re-importing something that matches cannot lose data because it holds none. + /// + /// The table to inspect. + /// when the table is a failed import's leftover. + private bool IsFailedImportPlaceholder(string tableName) + { + using (var schemaCommand = _connection.CreateCommand()) + { + schemaCommand.CommandText = $"PRAGMA table_info({QuoteIdentifier(tableName)})"; + + using var reader = schemaCommand.ExecuteReader(); + + // Exactly one column, named "value". + if (!reader.Read() || !string.Equals(reader.GetString(1), "value", StringComparison.Ordinal) || reader.Read()) + { + return false; + } + } + + using var rowCommand = _connection.CreateCommand(); + rowCommand.CommandText = $"SELECT EXISTS(SELECT 1 FROM {QuoteIdentifier(tableName)})"; + + return Convert.ToInt64(rowCommand.ExecuteScalar()) == 0; + } + + /// + /// Determines whether a worksheet carries anything worth creating a table for. A worksheet with + /// no header and no rows means the import produced nothing, not that the spreadsheet is empty. + /// + /// The parsed worksheet. + /// when the worksheet has a header or rows. + private static bool HasContent(TabularWorksheet worksheet) + { + if (worksheet is null) + { + return false; + } + + if (worksheet.Rows is { Count: > 0 }) + { + return true; + } + + return worksheet.Header is { Count: > 0 } + && worksheet.Header.Any(name => !string.IsNullOrWhiteSpace(name)); + } + private bool IsDocumentLoaded(string documentId) { foreach (var table in _tables.Values) diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs index bdaf7fb9..af9793cd 100644 --- a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs +++ b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs @@ -229,6 +229,150 @@ await workspace.EnsureReadyAsync( } } + [Fact] + public async Task EnsureReadyAsync_WhenADocumentsContentCannotBeLoaded_CreatesNoTable() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using var workspace = CreateWorkspace(databasePath); + + // A placeholder table here would read to the model as a file that contains no data. + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "projections.xlsx")], + LoaderFor(), + cancellationToken); + + Assert.Empty(tables); + Assert.Empty(GetUserTableNames(databasePath)); + Assert.Empty(GetMetadataTableNames(databasePath)); + } + + [Fact] + public async Task EnsureReadyAsync_WhenContentBecomesAvailableLater_RetriesTheFailedImport() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync([Document("doc-1", "sales.csv")], LoaderFor(), cancellationToken); + } + + // Registering the failed import would make IsDocumentLoaded true forever after, so the + // workspace would keep serving an empty table and never read the file again. + 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); + } + } + + [Fact] + public async Task EnsureReadyAsync_WhenOneDocumentFails_KeepsTheDocumentsThatLoaded() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using var workspace = CreateWorkspace(databasePath); + + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv"), Document("doc-2", "budget.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("sales", table.TableName); + Assert.Equal(2, table.RowCount); + } + + [Fact] + public async Task EnsureReadyAsync_WhenTheDatabaseAlreadyHoldsAFailedImportsEmptyTable_ImportsTheDocumentAgain() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + // The shape an earlier build wrote when a document's content could not be loaded. The metadata + // entry made the document count as loaded, so the empty table was served forever after. + WritePlaceholderTable(databasePath, "sales", "doc-1", "sales.csv"); + + 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); + Assert.Equal(["region", "amount"], table.Columns.Select(c => c.Name)); + } + + [Fact] + public async Task EnsureReadyAsync_DoesNotReimportATableTheUserEmptied() + { + 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); + + // Deleting every row through the manipulation tool is a deliberate edit, not a failed import. + await workspace.ExecuteAsync("DELETE FROM sales", cancellationToken); + + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal(0, table.RowCount); + Assert.Equal(["region", "amount"], table.Columns.Select(c => c.Name)); + } + + private static void WritePlaceholderTable(string databasePath, string tableName, string documentId, string fileName) + { + using var connection = new SqliteConnection($"Data Source={databasePath};Pooling=False"); + connection.Open(); + + using (var schemaCommand = connection.CreateCommand()) + { + schemaCommand.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 + ); + CREATE TABLE "{tableName}" ("value" TEXT); + """; + schemaCommand.ExecuteNonQuery(); + } + + using var command = connection.CreateCommand(); + command.CommandText = """ + INSERT INTO "_workspace_meta" ("table_name", "document_id", "worksheet_name", "file_name", "source_names_json") + VALUES ($tableName, $documentId, NULL, $fileName, $sourceNames) + """; + command.Parameters.AddWithValue("$tableName", tableName); + command.Parameters.AddWithValue("$documentId", documentId); + command.Parameters.AddWithValue("$fileName", fileName); + command.Parameters.AddWithValue("$sourceNames", "{\"value\":null}"); + command.ExecuteNonQuery(); + } + private string DatabasePath(string referenceId) { var path = Path.Combine(_root, "documents", "chat-session", referenceId, "data", "tabular.db");