Never cache a table for a document that could not be read

src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs

@@ -57,6 +57,23 @@ public static async Task<PreparationResult> 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(

src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs

@@ -120,6 +120,7 @@ public async Task<IReadOnlyList<TabularTableInfo>> 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();
     }
 
+    /// <summary>
+    /// Discards tables left behind by an import that produced no content, so the document is read
+    /// again on this request.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    private void RemoveFailedImportPlaceholderTables()
+    {
+        if (_tables.Count == 0)
+        {
+            return;
+        }
+
+        List<LoadedTable> 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);
+        }
+    }
+
+    /// <summary>
+    /// Determines whether a table has the exact shape a failed import leaves behind: the single
+    /// placeholder <c>value</c> column and no rows.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    /// <param name="tableName">The table to inspect.</param>
+    /// <returns><see langword="true"/> when the table is a failed import's leftover.</returns>
+    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;
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <param name="worksheet">The parsed worksheet.</param>
+    /// <returns><see langword="true"/> when the worksheet has a header or rows.</returns>
+    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)

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");