feat(documents): delete documents for real

migrations/0002_purge_soft_deleted_documents.down.sql

@@ -0,0 +1,10 @@
+-- 0002_purge_soft_deleted_documents.down.sql
+--
+-- Deliberately empty. 0002 deletes rows; nothing here can bring them back.
+--
+-- A down migration that silently does nothing is better than one that pretends:
+-- restoring these documents requires a backup taken before 0002 ran, not SQL.
+-- Reverting the code change (making DeleteDocument scoped again) does not
+-- resurrect them either.
+
+SELECT 1;

migrations/0002_purge_soft_deleted_documents.up.sql

@@ -0,0 +1,23 @@
+-- 0002_purge_soft_deleted_documents.up.sql
+--
+-- DeleteDocument is now a hard delete (#136). Documents deleted before that
+-- change are still in the table as tombstones: deleted_at is set, every read
+-- filters them out, and nothing can bring them back — but their `data` jsonb is
+-- intact, and for inference sinks that column holds base64 image bytes.
+--
+-- Those rows are the reason the old behaviour failed the retention question in
+-- FootprintAI/grandturks#936. Callers were told the documents were deleted.
+-- This makes that true.
+--
+-- IRREVERSIBLE. There is no down migration that can restore the data, only one
+-- that describes what was removed. Take a backup first if the rows have any
+-- value to you — by definition of the old API they should not, since nothing
+-- could read them.
+
+DELETE FROM "restcol-documents" WHERE deleted_at IS NOT NULL;
+
+-- Collections are deliberately NOT purged here. CollectionCURD.Delete is still
+-- a soft delete: a collection row carries an id, a type and a summary, not user
+-- payload, and hard-deleting one would fire the OnDelete:SET NULL constraint on
+-- restcol-collections-schema and leave orphaned schema rows behind. That is a
+-- separate change with its own consequences; see #136.

pkg/storage/documents/documents.go

@@ -159,14 +159,25 @@ func (c *DocumentCURD) Query(ctx context.Context,
 	return records, nil
 }
 
+// Delete removes a document permanently. The row and its data are gone; there
+// is no recovery.
+//
+// Unscoped is deliberate (#136). ModelDocument carries gorm.DeletedAt, so
+// without it this writes a tombstone and keeps the row — including the `data`
+// jsonb, which for inference sinks holds base64 image bytes. That gave the
+// worst of both: the caller could not recover the document (there is no
+// Undelete RPC, and every read filters deleted_at IS NULL) while the bytes
+// stayed in the database indefinitely. A delete that reports success and
+// retains the data is not a delete.
 func (c *DocumentCURD) Delete(ctx context.Context, tableName string, pid appmodelprojects.ProjectID, cid appmodelcollections.CollectionID, did appmodeldocuments.DocumentID) error {
-	err := c.With(ctx, tableName).Where("id = ? AND model_collection_id = ? AND model_project_id = ?",
-		did.String(), cid.String(), pid.String()).Delete(&appmodeldocuments.ModelDocument{}).Error
+	err := c.With(ctx, tableName).Unscoped().
+		Where("id = ? AND model_collection_id = ? AND model_project_id = ?",
+			did.String(), cid.String(), pid.String()).
+		Delete(&appmodeldocuments.ModelDocument{}).Error
 	return storage.WrapStorageError(err)
 }
 
-// CountByCollection returns the number of active (non-soft-deleted) documents
-// in the given collection.
+// CountByCollection returns the number of documents in the given collection.
 func (c *DocumentCURD) CountByCollection(ctx context.Context, tableName string, pid appmodelprojects.ProjectID, cid appmodelcollections.CollectionID) (int64, error) {
 	var count int64
 	err := c.With(ctx, tableName).
@@ -179,9 +190,12 @@ func (c *DocumentCURD) CountByCollection(ctx context.Context, tableName string,
 	return count, nil
 }
 
-// DeleteByCollection soft-deletes every document in the given collection.
+// DeleteByCollection permanently removes every document in the given
+// collection. Same reasoning as Delete: this is what DeleteCollection(force)
+// cascades through, so leaving it scoped would mean the cascade retained every
+// payload it claimed to remove.
 func (c *DocumentCURD) DeleteByCollection(ctx context.Context, tableName string, pid appmodelprojects.ProjectID, cid appmodelcollections.CollectionID) error {
-	err := c.With(ctx, tableName).
+	err := c.With(ctx, tableName).Unscoped().
 		Where("model_project_id = ? AND model_collection_id = ?", pid.String(), cid.String()).
 		Delete(&appmodeldocuments.ModelDocument{}).Error
 	return storage.WrapStorageError(err)

pkg/storage/documents/documents_test.go

@@ -216,10 +216,21 @@ func TestDocumentCURD_Delete(t *testing.T) {
 	err = docCURD.Delete(context.Background(), "", regularProject.ID, collection.ID, testDoc.ID)
 	assert.NoError(t, err)
 
-	// Verify document is soft deleted (should return empty record)
+	// Gone from the normal read path...
 	deletedDoc, err := docCURD.Get(context.Background(), "", regularProject.ID, collection.ID, testDoc.ID)
 	assert.NoError(t, err)                  // Get doesn't error, just returns empty record
-	assert.Empty(t, deletedDoc.ID.String()) // ID should be empty for soft-deleted record
+	assert.Empty(t, deletedDoc.ID.String()) // ID is empty when the row is not there
+
+	// ...and gone from the table, not merely hidden by gorm's deleted_at
+	// filter. This is the assertion that separates a hard delete from a soft
+	// one; without Unscoped here the query below would still find the row and
+	// the test would pass on a tombstone (#136).
+	var remaining int64
+	assert.NoError(t, postgrescli.GormDB().Unscoped().
+		Model(&appmodeldocuments.ModelDocument{}).
+		Where("id = ?", testDoc.ID.String()).
+		Count(&remaining).Error)
+	assert.Zero(t, remaining, "the row must be deleted, not tombstoned")
 
 	// Test delete non-existent document (should not error in GORM)
 	nonExistentID := appmodeldocuments.NewDocumentID()
@@ -283,3 +294,46 @@ func TestDocumentCURD_Delete_WithWrongScope(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, testDoc.ID, foundDoc.ID)
 }
+
+// TestDocumentCURD_DeleteByCollection_IsPermanent covers the cascade path that
+// DeleteCollection(force=true) uses. It is the one most likely to be missed:
+// deleting a whole collection is exactly when a caller expects the payloads to
+// be gone, and a scoped delete there would retain every one of them.
+func TestDocumentCURD_DeleteByCollection_IsPermanent(t *testing.T) {
+	if testing.Short() {
+		t.Skip("requires a local postgres; skipping under -short")
+	}
+	ctx := context.Background()
+	postgrescli, err := storagetestutils.NewTestPostgresCli(logger.NewLogger(false))
+	assert.NoError(t, err)
+
+	regularProject, _, err := storageprojects.TestProjectSuite(postgrescli)
+	assert.Nil(t, err)
+
+	collection, err := storagecollectionstestutils.TestCollectionSuite(postgrescli, regularProject)
+	assert.NoError(t, err)
+
+	docCURD := NewDocumentCURD(postgrescli)
+	assert.Nil(t, docCURD.AutoMigrate())
+
+	for i := 0; i < 3; i++ {
+		assert.NoError(t, docCURD.Write(ctx, "", &appmodeldocuments.ModelDocument{
+			ID:                appmodeldocuments.NewDocumentID(),
+			Data:              appmodeldocuments.NewModelDocumentData(map[string]interface{}{"n": i}),
+			ModelCollectionID: collection.ID,
+			ModelProjectID:    regularProject.ID,
+		}))
+	}
+
+	assert.NoError(t, docCURD.DeleteByCollection(ctx, "", regularProject.ID, collection.ID))
+
+	// Unscoped: counts tombstones as well as live rows, so this fails if the
+	// cascade only marked them deleted.
+	var remaining int64
+	assert.NoError(t, postgrescli.GormDB().Unscoped().
+		Model(&appmodeldocuments.ModelDocument{}).
+		Where("model_project_id = ? AND model_collection_id = ?",
+			regularProject.ID.String(), collection.ID.String()).
+		Count(&remaining).Error)
+	assert.Zero(t, remaining, "the cascade must remove the rows, not tombstone them")
+}