From d96a29ef400d6bee0000f35d9922505cb83a56dc Mon Sep 17 00:00:00 2001 From: xiaocui-big Date: Tue, 15 Sep 2026 23:16:05 +0800 Subject: [PATCH 1/4] fix(folders): clean up blob storage on recursive folder delete (#177) Recursive folder delete removed DB rows but left every underlying object in bucket storage. The code had an explicit TODO acknowledging this. Changes: - Add ObjectStore and logger options to folder.Service (WithStore, WithLogger) - Collect storage keys before deleting files, then delete blobs after the transaction commits (best-effort, matching file.Delete's shape) - Log failed blob deletions at WARN level so operators can see orphans - Update memd wiring to pass store and logger to folder service - Add integration tests verifying blob cleanup and backward compatibility - Document object retention behavior in DEPLOYMENT.md - Add CHANGELOG entry The crash window (process killed between commit and blob delete) is documented as a known gap. A reaper would need to record keys whose delete was never attempted, since storage.Store has no listing. --- CHANGELOG.md | 10 + docs/DEPLOYMENT.md | 23 ++ server/cmd/memd/main.go | 5 +- .../folder/delete_integration_test.go | 253 ++++++++++++++++++ server/internal/folder/folder.go | 104 ++++++- 5 files changed, 388 insertions(+), 7 deletions(-) create mode 100644 server/internal/folder/delete_integration_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 25c73ed..d8a2fba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,16 @@ The project publishes 0.x prerelease versions; a stable release line is not yet repository coordinate the installer itself uses, so the next rename cannot leave a stale identifier behind unnoticed. +### Fixed + +- Recursive folder delete now removes the corresponding objects from bucket + storage after the database transaction commits (`#177`). Previously, the DB + rows were deleted but the blobs remained orphaned in the bucket. The cleanup + is best-effort: a failed object delete does not roll back the folder delete, + and failures are logged at WARN level so the operator can see which keys + remain. The folder service now accepts an optional `ObjectStore` and logger + via `folder.WithStore` and `folder.WithLogger` options. + ### Security - Normalize the client-declared MIME type of a stored file before deciding how diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 25d0b57..698a94e 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -261,6 +261,29 @@ Redis AOF protects normal restarts but is not in the portable backup. A restore therefore starts with an empty queue/replay window. Requeue or reindex any file whose processing did not reach a terminal state before the backup. +### Object storage retention + +Object keys are per-file by construction: each key embeds the row's own file ID +(`users///`), so deleting one row's key cannot +remove another row's bytes. No reference counting is needed. + +When a file or folder is deleted, the database row is removed first, then the +corresponding object is deleted from bucket storage on a best-effort basis. A +failed object delete does not roll back the database change; the orphaned key +is logged at `WARN` level so the operator can see which keys remain. + +**Crash window**: if the process is killed after the database transaction +commits but before the blob delete lands, the object remains in the bucket +permanently. There is currently no reaper or garbage-collection pass to sweep +these residues. The server has no listing capability against the bucket (the +`storage.Store` interface exposes only `Put`/`Get`/`Delete`), so a reaper would +need to record keys whose delete was never attempted. This is a known gap; +operators should monitor bucket growth against expected database row counts. + +To manually reconcile, compare the bucket contents against the `files` table's +`storage_key` column. Objects present in the bucket but absent from the database +are safe to delete — they cannot be referenced by any live row. + ### Restore drill Restore only into an empty installation. The script verifies every checksum diff --git a/server/cmd/memd/main.go b/server/cmd/memd/main.go index 3e08444..af4d519 100644 --- a/server/cmd/memd/main.go +++ b/server/cmd/memd/main.go @@ -119,7 +119,10 @@ func run() error { "providers", cfg.ManagedEmbeddingProviders, ) } - folderSvc := folder.New(database.Pool) + folderSvc := folder.New(database.Pool, + folder.WithStore(store), + folder.WithLogger(logger), + ) fileSvc := file.New(database.Pool, store, folderSvc) memorySvc := memory.New(database.Pool) durableContextSvc := durablecontext.New(database.Pool, memorySvc) diff --git a/server/internal/folder/delete_integration_test.go b/server/internal/folder/delete_integration_test.go new file mode 100644 index 0000000..6630b6e --- /dev/null +++ b/server/internal/folder/delete_integration_test.go @@ -0,0 +1,253 @@ +package folder + +import ( + "bytes" + "context" + "io" + "os" + "strings" + "sync" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" + + memdb "github.com/PeterGuy326/mem/server/internal/db" +) + +// trackingObjectStore records which keys are deleted so tests can assert that +// recursive folder delete cleans up blobs. +type trackingObjectStore struct { + mu sync.Mutex + objects map[string]bool + deleted []string +} + +func newTrackingObjectStore() *trackingObjectStore { + return &trackingObjectStore{objects: make(map[string]bool)} +} + +func (s *trackingObjectStore) Put(_ context.Context, key string, _ io.Reader, _ int64, _ string) error { + s.mu.Lock() + defer s.mu.Unlock() + s.objects[key] = true + return nil +} + +func (s *trackingObjectStore) Get(_ context.Context, key string) (io.ReadCloser, error) { + s.mu.Lock() + defer s.mu.Unlock() + if !s.objects[key] { + return nil, &objectNotFoundError{key: key} + } + return io.NopCloser(bytes.NewReader(nil)), nil +} + +func (s *trackingObjectStore) Delete(_ context.Context, key string) error { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.objects, key) + s.deleted = append(s.deleted, key) + return nil +} + +func (s *trackingObjectStore) has(key string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.objects[key] +} + +func (s *trackingObjectStore) deleteCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.deleted) +} + +type objectNotFoundError struct{ key string } + +func (e *objectNotFoundError) Error() string { return "object not found: " + e.key } + +// TestRecursiveDeleteCleansUpBlobs verifies that recursive folder delete +// removes objects from the store after the DB rows are deleted. +// +// MEM_TEST_DB=postgres://mem:mem@localhost:5432/mem_test?sslmode=disable \ +// go test ./internal/folder -run TestRecursiveDeleteCleansUpBlobs +func TestRecursiveDeleteCleansUpBlobs(t *testing.T) { + dsn := os.Getenv("MEM_TEST_DB") + if dsn == "" { + t.Skip("MEM_TEST_DB not set; skipping DB integration test") + } + config, err := pgxpool.ParseConfig(dsn) + if err != nil { + t.Fatalf("parse MEM_TEST_DB: %v", err) + } + if !strings.HasSuffix(config.ConnConfig.Database, "_test") { + t.Fatalf( + "refusing to modify non-test database %q; MEM_TEST_DB must end in _test", + config.ConnConfig.Database, + ) + } + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + database, err := memdb.Open(ctx, dsn) + if err != nil { + t.Fatalf("open test database: %v", err) + } + t.Cleanup(database.Close) + if err := database.Migrate(ctx); err != nil { + t.Fatalf("migrate test database: %v", err) + } + + userID := createFolderDeleteTenant(t, ctx, database.Pool) + store := newTrackingObjectStore() + service := New(database.Pool, WithStore(store)) + + // Create a folder hierarchy with files. + if _, err := service.Create(ctx, userID, "/Project/Sub"); err != nil { + t.Fatalf("create folders: %v", err) + } + + // Insert files with storage keys that the store tracks. + fileKeys := []string{ + "users/" + userID.String() + "/" + uuid.NewString() + "/a.txt", + "users/" + userID.String() + "/" + uuid.NewString() + "/b.txt", + "users/" + userID.String() + "/" + uuid.NewString() + "/c.txt", + } + for i, key := range fileKeys { + if err := store.Put(ctx, key, nil, 0, "text/plain"); err != nil { + t.Fatalf("put object: %v", err) + } + paths := []string{"/Project", "/Project", "/Project/Sub"} + names := []string{"a.txt", "b.txt", "c.txt"} + if _, err := database.Pool.Exec(ctx, ` + INSERT INTO files (id, user_id, name, path, size, sha256, mime, storage_key, index_status) + VALUES ($1, $2, $3, $4, 0, $5, 'text/plain', $6, 'ready') + `, uuid.New(), userID, names[i], paths[i], strings.Repeat("x", 64), key); err != nil { + t.Fatalf("insert file: %v", err) + } + } + + // Verify objects exist before delete. + for _, key := range fileKeys { + if !store.has(key) { + t.Fatalf("object %s should exist before delete", key) + } + } + + // Recursive delete. + if err := service.Delete(ctx, userID, "/Project", true); err != nil { + t.Fatalf("recursive delete: %v", err) + } + + // Verify all objects were deleted from the store. + for _, key := range fileKeys { + if store.has(key) { + t.Errorf("object %s should have been deleted from store", key) + } + } + if store.deleteCount() != len(fileKeys) { + t.Errorf("delete calls = %d, want %d", store.deleteCount(), len(fileKeys)) + } + + // Verify DB rows are gone. + var fileCount int + if err := database.Pool.QueryRow(ctx, ` + SELECT count(*) FROM files WHERE user_id = $1 + `, userID).Scan(&fileCount); err != nil { + t.Fatalf("count files: %v", err) + } + if fileCount != 0 { + t.Errorf("files remaining = %d, want 0", fileCount) + } + + var folderCount int + if err := database.Pool.QueryRow(ctx, ` + SELECT count(*) FROM folders WHERE user_id = $1 + `, userID).Scan(&folderCount); err != nil { + t.Fatalf("count folders: %v", err) + } + if folderCount != 0 { + t.Errorf("folders remaining = %d, want 0", folderCount) + } +} + +// TestRecursiveDeleteWithoutStore verifies that recursive delete works without +// a store configured (backward compatibility — DB rows are deleted but blobs +// are not cleaned up). +func TestRecursiveDeleteWithoutStore(t *testing.T) { + dsn := os.Getenv("MEM_TEST_DB") + if dsn == "" { + t.Skip("MEM_TEST_DB not set; skipping DB integration test") + } + config, err := pgxpool.ParseConfig(dsn) + if err != nil { + t.Fatalf("parse MEM_TEST_DB: %v", err) + } + if !strings.HasSuffix(config.ConnConfig.Database, "_test") { + t.Fatalf( + "refusing to modify non-test database %q; MEM_TEST_DB must end in _test", + config.ConnConfig.Database, + ) + } + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + database, err := memdb.Open(ctx, dsn) + if err != nil { + t.Fatalf("open test database: %v", err) + } + t.Cleanup(database.Close) + if err := database.Migrate(ctx); err != nil { + t.Fatalf("migrate test database: %v", err) + } + + userID := createFolderDeleteTenant(t, ctx, database.Pool) + service := New(database.Pool) // no store + + if _, err := service.Create(ctx, userID, "/Orphan"); err != nil { + t.Fatalf("create folder: %v", err) + } + key := "users/" + userID.String() + "/" + uuid.NewString() + "/orphan.txt" + if _, err := database.Pool.Exec(ctx, ` + INSERT INTO files (id, user_id, name, path, size, sha256, mime, storage_key, index_status) + VALUES ($1, $2, 'orphan.txt', '/Orphan', 0, $3, 'text/plain', $4, 'ready') + `, uuid.New(), userID, strings.Repeat("y", 64), key); err != nil { + t.Fatalf("insert file: %v", err) + } + + // Delete should succeed even without a store. + if err := service.Delete(ctx, userID, "/Orphan", true); err != nil { + t.Fatalf("recursive delete without store: %v", err) + } + + // DB row should be gone. + var fileCount int + if err := database.Pool.QueryRow(ctx, ` + SELECT count(*) FROM files WHERE user_id = $1 + `, userID).Scan(&fileCount); err != nil { + t.Fatalf("count files: %v", err) + } + if fileCount != 0 { + t.Errorf("files remaining = %d, want 0", fileCount) + } +} + +func createFolderDeleteTenant(t *testing.T, ctx context.Context, pool *pgxpool.Pool) uuid.UUID { + t.Helper() + var userID uuid.UUID + if err := pool.QueryRow(ctx, ` + INSERT INTO users (email, password_hash) + VALUES ($1, 'folder-delete-test') + RETURNING id + `, "folder-delete-"+uuid.NewString()+"@example.com").Scan(&userID); err != nil { + t.Fatalf("create user: %v", err) + } + t.Cleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _, _ = pool.Exec(cleanupCtx, `DELETE FROM users WHERE id = $1`, userID) + }) + return userID +} diff --git a/server/internal/folder/folder.go b/server/internal/folder/folder.go index 0dd0469..f5571fb 100644 --- a/server/internal/folder/folder.go +++ b/server/internal/folder/folder.go @@ -13,6 +13,7 @@ import ( "context" "errors" "fmt" + "log/slog" "strings" "time" @@ -58,13 +59,40 @@ type Node struct { Children []*Node `json:"children,omitempty"` } +// ObjectStore is the subset of storage.Store that folder needs for blob cleanup. +type ObjectStore interface { + Delete(context.Context, string) error +} + // Service is the folder service. type Service struct { - pool *pgxpool.Pool + pool *pgxpool.Pool + store ObjectStore + logger *slog.Logger +} + +// Option configures a folder Service. +type Option func(*Service) + +// WithStore sets the object store used for blob cleanup on recursive delete. +// If not set, recursive delete removes DB rows but leaves orphan blobs. +func WithStore(store ObjectStore) Option { + return func(s *Service) { s.store = store } +} + +// WithLogger sets the logger for reporting best-effort blob cleanup failures. +func WithLogger(logger *slog.Logger) Option { + return func(s *Service) { s.logger = logger } } // New constructs a folder Service. -func New(pool *pgxpool.Pool) *Service { return &Service{pool: pool} } +func New(pool *pgxpool.Pool, opts ...Option) *Service { + s := &Service{pool: pool} + for _, opt := range opts { + opt(s) + } + return s +} // Sentinel errors. var ( @@ -586,9 +614,10 @@ func rewritePrefixTx(ctx context.Context, tx pgx.Tx, userID, srcID uuid.UUID, ol // // - recursive=false (default): folder must be empty (no subfolders, no files) // or ErrNotEmpty is returned. -// - recursive=true: subfolders + files are deleted from the DB. S3 cleanup -// is TODO — for now we only purge the DB rows; orphan blobs will be -// reaped by a future garbage-collection pass. +// - recursive=true: subfolders + files are deleted from the DB, and their +// blobs are removed from object storage on a best-effort basis after the +// transaction commits. A failed blob removal does not roll back the DB +// delete; orphaned keys are logged so the operator can see them. func (s *Service) Delete(ctx context.Context, userID uuid.UUID, path string, recursive bool) error { norm, err := pathx.Normalize(path) if err != nil { @@ -597,7 +626,8 @@ func (s *Service) Delete(ctx context.Context, userID uuid.UUID, path string, rec if norm == pathx.Root { return ErrRootOp } - return s.withPathMutationTx(ctx, userID, func(tx pgx.Tx) error { + var orphanKeys []string + err = s.withPathMutationTx(ctx, userID, func(tx pgx.Tx) error { src, err := selectFolderByPathTx(ctx, tx, userID, norm) if err != nil { return err @@ -628,6 +658,14 @@ func (s *Service) Delete(ctx context.Context, userID uuid.UUID, path string, rec // explicit forget operation first. return ErrContainsMemories } + // Collect storage keys before deleting rows so we can clean up + // blobs after the transaction commits. Keys are per-row by + // construction (see 0013_file_content_identity.sql), so deleting + // each key cannot destroy another row's bytes. + orphanKeys, err = collectStorageKeysTx(ctx, tx, userID, src.ID, src.Path) + if err != nil { + return fmt.Errorf("collect storage keys for recursive delete: %w", err) + } // Hard delete: remove all descendant files first (FKs cascade // from folders → files would only NULL out folder_id, so we have // to delete files explicitly). @@ -648,6 +686,60 @@ func (s *Service) Delete(ctx context.Context, userID uuid.UUID, path string, rec } return nil }) + if err != nil { + return err + } + // Blob cleanup happens after the transaction commits: a failed object + // delete must not roll back the user's folder delete. This matches the + // best-effort shape of file.Delete. + if len(orphanKeys) > 0 && s.store != nil { + s.cleanupBlobs(ctx, orphanKeys) + } + return nil +} + +// collectStorageKeysTx returns the storage_key values for all files that will +// be deleted by a recursive folder delete. This must be called before the +// DELETE FROM files so the keys are available for post-commit blob cleanup. +func collectStorageKeysTx(ctx context.Context, tx pgx.Tx, userID uuid.UUID, folderID uuid.UUID, folderPath string) ([]string, error) { + rows, err := tx.Query(ctx, + `SELECT storage_key FROM files + WHERE user_id = $1 + AND (folder_id = $2 OR path = $3 + OR left(path, length($3) + 1) = $3 || '/') + AND storage_key <> ''`, + userID, folderID, folderPath) + if err != nil { + return nil, err + } + defer rows.Close() + var keys []string + for rows.Next() { + var key string + if err := rows.Scan(&key); err != nil { + return nil, err + } + keys = append(keys, key) + } + return keys, rows.Err() +} + +// cleanupBlobs removes objects from the store on a best-effort basis. Failures +// are logged so the operator can see which keys remain; they do not propagate +// to the caller because the DB rows are already gone. +func (s *Service) cleanupBlobs(ctx context.Context, keys []string) { + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) + defer cancel() + for _, key := range keys { + if err := s.store.Delete(cleanupCtx, key); err != nil { + if s.logger != nil { + s.logger.Warn("folder delete: blob cleanup failed", + "storage_key", key, + "error", err, + ) + } + } + } } func isEmptyTx(ctx context.Context, tx pgx.Tx, userID, folderID uuid.UUID, path string) (bool, error) { From 001affd0298f6ce2761f79979e6c29f284292b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8B=92=E5=B8=83=E6=9C=97-=E8=A9=B9=E5=A7=86=E6=96=AF?= <318569545+waterbro-8@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:06:25 +0000 Subject: [PATCH 2/4] fix(folders): refuse recursive delete when a memory cites the tree Recursive blob cleanup would otherwise destroy objects still cited by active/archived memories whose path is outside the folder, because source_file_id is ON DELETE SET NULL. --- CHANGELOG.md | 5 +- docs/DEPLOYMENT.md | 9 +- .../folder/delete_integration_test.go | 109 +++++++++++++++++- server/internal/folder/folder.go | 38 ++++-- 4 files changed, 149 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8a2fba..c143e7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,7 +59,10 @@ The project publishes 0.x prerelease versions; a stable release line is not yet is best-effort: a failed object delete does not roll back the folder delete, and failures are logged at WARN level so the operator can see which keys remain. The folder service now accepts an optional `ObjectStore` and logger - via `folder.WithStore` and `folder.WithLogger` options. + via `folder.WithStore` and `folder.WithLogger` options. Recursive delete + also refuses when an active or archived memory outside the folder still + cites a file in the tree via `source_file_id`, so blob cleanup cannot + destroy a live citation through `ON DELETE SET NULL`. ### Security diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 698a94e..8e99432 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -270,7 +270,14 @@ remove another row's bytes. No reference counting is needed. When a file or folder is deleted, the database row is removed first, then the corresponding object is deleted from bucket storage on a best-effort basis. A failed object delete does not roll back the database change; the orphaned key -is logged at `WARN` level so the operator can see which keys remain. +is logged at `WARN` level so the operator can see which keys remain. If the +shared 30-second cleanup budget is exhausted mid-batch, later keys log that +the budget ran out rather than a per-object store error. + +Recursive folder delete refuses with the existing `forget` sentinel when an +active or archived memory — including one whose `path` is outside the folder +— still cites a file in the tree through `source_file_id`. That keeps blob +cleanup from destroying a live citation via `ON DELETE SET NULL`. **Crash window**: if the process is killed after the database transaction commits but before the blob delete lands, the object remains in the bucket diff --git a/server/internal/folder/delete_integration_test.go b/server/internal/folder/delete_integration_test.go index 6630b6e..78cf60c 100644 --- a/server/internal/folder/delete_integration_test.go +++ b/server/internal/folder/delete_integration_test.go @@ -3,6 +3,7 @@ package folder import ( "bytes" "context" + "errors" "io" "os" "strings" @@ -19,9 +20,9 @@ import ( // trackingObjectStore records which keys are deleted so tests can assert that // recursive folder delete cleans up blobs. type trackingObjectStore struct { - mu sync.Mutex - objects map[string]bool - deleted []string + mu sync.Mutex + objects map[string]bool + deleted []string } func newTrackingObjectStore() *trackingObjectStore { @@ -234,6 +235,108 @@ func TestRecursiveDeleteWithoutStore(t *testing.T) { } } +// TestRecursiveDeleteBlocksWhenMemoryCitesFileElsewhere is the #210 review +// regression: a memory living at /Work/task that cites a file under /Photos +// must block recursive delete of /Photos. Otherwise ON DELETE SET NULL plus +// blob cleanup would destroy the cited object while the memory stays active. +func TestRecursiveDeleteBlocksWhenMemoryCitesFileElsewhere(t *testing.T) { + dsn := os.Getenv("MEM_TEST_DB") + if dsn == "" { + t.Skip("MEM_TEST_DB not set; skipping DB integration test") + } + config, err := pgxpool.ParseConfig(dsn) + if err != nil { + t.Fatalf("parse MEM_TEST_DB: %v", err) + } + if !strings.HasSuffix(config.ConnConfig.Database, "_test") { + t.Fatalf( + "refusing to modify non-test database %q; MEM_TEST_DB must end in _test", + config.ConnConfig.Database, + ) + } + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + database, err := memdb.Open(ctx, dsn) + if err != nil { + t.Fatalf("open test database: %v", err) + } + t.Cleanup(database.Close) + if err := database.Migrate(ctx); err != nil { + t.Fatalf("migrate test database: %v", err) + } + + userID := createFolderDeleteTenant(t, ctx, database.Pool) + var workspaceID uuid.UUID + if err := database.Pool.QueryRow(ctx, ` + INSERT INTO workspaces (name, resource_owner_user_id) + VALUES ('folder-delete-cite', $1) + RETURNING id + `, userID).Scan(&workspaceID); err != nil { + t.Fatalf("create workspace: %v", err) + } + + store := newTrackingObjectStore() + service := New(database.Pool, WithStore(store)) + if _, err := service.Create(ctx, userID, "/Photos"); err != nil { + t.Fatalf("create /Photos: %v", err) + } + if _, err := service.Create(ctx, userID, "/Work"); err != nil { + t.Fatalf("create /Work: %v", err) + } + photos, err := service.Get(ctx, userID, "/Photos") + if err != nil { + t.Fatalf("get /Photos: %v", err) + } + + fileID := uuid.New() + key := "users/" + userID.String() + "/" + fileID.String() + "/cited.txt" + if err := store.Put(ctx, key, nil, 0, "text/plain"); err != nil { + t.Fatalf("put object: %v", err) + } + sha := strings.Repeat("ab", 32) + if _, err := database.Pool.Exec(ctx, ` + INSERT INTO files (id, user_id, folder_id, name, path, size, sha256, mime, storage_key, index_status) + VALUES ($1, $2, $3, 'cited.txt', '/Photos', 0, $4, 'text/plain', $5, 'ready') + `, fileID, userID, photos.ID, sha, key); err != nil { + t.Fatalf("insert cited file: %v", err) + } + if _, err := database.Pool.Exec(ctx, ` + INSERT INTO memories ( + workspace_id, kind, content, path, source_type, + source_file_id, source_file_sha256, + idempotency_key, request_sha256, content_sha256, + lifecycle_status + ) VALUES ( + $1, 'note', 'cites a photo', '/Work/task', 'agent', + $2, $3, + $4, $3, $3, + 'active' + ) + `, workspaceID, fileID, sha, "folder-delete-cite-"+uuid.NewString()); err != nil { + t.Fatalf("insert citing memory: %v", err) + } + + if err := service.Delete(ctx, userID, "/Photos", true); !errors.Is(err, ErrContainsMemories) { + t.Fatalf("recursive delete with cross-path citation = %v, want ErrContainsMemories", err) + } + if !store.has(key) { + t.Fatal("cited blob was deleted despite the blocking memory") + } + var fileCount int + if err := database.Pool.QueryRow(ctx, ` + SELECT count(*) FROM files WHERE id = $1 + `, fileID).Scan(&fileCount); err != nil { + t.Fatalf("count cited file: %v", err) + } + if fileCount != 1 { + t.Fatalf("cited file remaining = %d, want 1", fileCount) + } + if _, err := service.Get(ctx, userID, "/Photos"); err != nil { + t.Fatalf("/Photos changed despite blocked recursive delete: %v", err) + } +} + func createFolderDeleteTenant(t *testing.T, ctx context.Context, pool *pgxpool.Pool) uuid.UUID { t.Helper() var userID uuid.UUID diff --git a/server/internal/folder/folder.go b/server/internal/folder/folder.go index f5571fb..1c77a89 100644 --- a/server/internal/folder/folder.go +++ b/server/internal/folder/folder.go @@ -648,7 +648,7 @@ func (s *Service) Delete(ctx context.Context, userID uuid.UUID, path string, rec if containsTaskState { return ErrContainsTaskState } - containsMemories, err := containsMemoriesTx(ctx, tx, userID, src.Path, true) + containsMemories, err := containsMemoriesTx(ctx, tx, userID, src.ID, src.Path, true) if err != nil { return fmt.Errorf("check recursive delete memories: %w", err) } @@ -733,7 +733,11 @@ func (s *Service) cleanupBlobs(ctx context.Context, keys []string) { for _, key := range keys { if err := s.store.Delete(cleanupCtx, key); err != nil { if s.logger != nil { - s.logger.Warn("folder delete: blob cleanup failed", + msg := "folder delete: blob cleanup failed" + if cleanupCtx.Err() != nil { + msg = "folder delete: blob cleanup stopped; 30s shared budget exhausted" + } + s.logger.Warn(msg, "storage_key", key, "error", err, ) @@ -767,7 +771,7 @@ func isEmptyTx(ctx context.Context, tx pgx.Tx, userID, folderID uuid.UUID, path if containsTaskState { return false, nil } - containsMemories, err := containsMemoriesTx(ctx, tx, userID, path, false) + containsMemories, err := containsMemoriesTx(ctx, tx, userID, folderID, path, false) if err != nil { return false, fmt.Errorf("check folder memories: %w", err) } @@ -775,8 +779,15 @@ func isEmptyTx(ctx context.Context, tx pgx.Tx, userID, folderID uuid.UUID, path } // containsMemoriesTx reports whether a resource owner's workspace contains an -// active or archived memory at path. When recursive is true, descendants are -// included with a literal segment-boundary comparison. +// active or archived memory that would be harmed by deleting this folder. +// When recursive is true, descendants are included with a literal +// segment-boundary comparison, and memories that live elsewhere but reference +// a file in this tree via source_file_id also block the delete. +// +// That second check matters once recursive delete removes blobs: ON DELETE +// SET NULL would otherwise silently drop the citation while this function +// physically destroys the object, leaving an active memory pointing at +// bytes that no longer exist. // // Forgotten/tombstoned rows intentionally do not block folder deletion: an // explicit memory lifecycle transition has already happened for those rows. @@ -784,6 +795,7 @@ func containsMemoriesTx( ctx context.Context, tx pgx.Tx, userID uuid.UUID, + folderID uuid.UUID, path string, recursive bool, ) (bool, error) { @@ -796,9 +808,21 @@ func containsMemoriesTx( JOIN workspaces AS w ON w.id = m.workspace_id WHERE w.resource_owner_user_id = $1 AND m.lifecycle_status IN ('active', 'archived') - AND (m.path = $2 OR left(m.path, length($2) + 1) = $2 || '/') + AND ( + m.path = $2 + OR left(m.path, length($2) + 1) = $2 || '/' + OR EXISTS ( + SELECT 1 + FROM files AS f + WHERE f.id = m.source_file_id + AND f.user_id = $1 + AND (f.folder_id = $3 + OR f.path = $2 + OR left(f.path, length($2) + 1) = $2 || '/') + ) + ) )`, - userID, path).Scan(&exists) + userID, path, folderID).Scan(&exists) return exists, err } err := tx.QueryRow(ctx, From 7a7dc009abd3d4efa47c3ac8372cd24f5172dd9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8B=92=E5=B8=83=E6=9C=97-=E8=A9=B9=E5=A7=86=E6=96=AF?= <318569545+waterbro-8@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:33:39 +0000 Subject: [PATCH 3/4] test(folders): create a workspace and match current memory columns Recursive-delete tests must lock a resource-owner workspace, insert memories with idempotency_key_sha256, and keep source_file_id on the session-local memories shadow table. --- .../folder/delete_integration_test.go | 38 +++++++++++-------- server/internal/folder/folder_test.go | 1 + 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/server/internal/folder/delete_integration_test.go b/server/internal/folder/delete_integration_test.go index 78cf60c..8a15f84 100644 --- a/server/internal/folder/delete_integration_test.go +++ b/server/internal/folder/delete_integration_test.go @@ -101,7 +101,7 @@ func TestRecursiveDeleteCleansUpBlobs(t *testing.T) { t.Fatalf("migrate test database: %v", err) } - userID := createFolderDeleteTenant(t, ctx, database.Pool) + userID, _ := createFolderDeleteTenant(t, ctx, database.Pool) store := newTrackingObjectStore() service := New(database.Pool, WithStore(store)) @@ -204,7 +204,7 @@ func TestRecursiveDeleteWithoutStore(t *testing.T) { t.Fatalf("migrate test database: %v", err) } - userID := createFolderDeleteTenant(t, ctx, database.Pool) + userID, _ := createFolderDeleteTenant(t, ctx, database.Pool) service := New(database.Pool) // no store if _, err := service.Create(ctx, userID, "/Orphan"); err != nil { @@ -266,15 +266,7 @@ func TestRecursiveDeleteBlocksWhenMemoryCitesFileElsewhere(t *testing.T) { t.Fatalf("migrate test database: %v", err) } - userID := createFolderDeleteTenant(t, ctx, database.Pool) - var workspaceID uuid.UUID - if err := database.Pool.QueryRow(ctx, ` - INSERT INTO workspaces (name, resource_owner_user_id) - VALUES ('folder-delete-cite', $1) - RETURNING id - `, userID).Scan(&workspaceID); err != nil { - t.Fatalf("create workspace: %v", err) - } + userID, workspaceID := createFolderDeleteTenant(t, ctx, database.Pool) store := newTrackingObjectStore() service := New(database.Pool, WithStore(store)) @@ -305,15 +297,15 @@ func TestRecursiveDeleteBlocksWhenMemoryCitesFileElsewhere(t *testing.T) { INSERT INTO memories ( workspace_id, kind, content, path, source_type, source_file_id, source_file_sha256, - idempotency_key, request_sha256, content_sha256, + idempotency_key_sha256, request_sha256, content_sha256, lifecycle_status ) VALUES ( $1, 'note', 'cites a photo', '/Work/task', 'agent', $2, $3, - $4, $3, $3, + $3, $3, $3, 'active' ) - `, workspaceID, fileID, sha, "folder-delete-cite-"+uuid.NewString()); err != nil { + `, workspaceID, fileID, sha); err != nil { t.Fatalf("insert citing memory: %v", err) } @@ -337,7 +329,7 @@ func TestRecursiveDeleteBlocksWhenMemoryCitesFileElsewhere(t *testing.T) { } } -func createFolderDeleteTenant(t *testing.T, ctx context.Context, pool *pgxpool.Pool) uuid.UUID { +func createFolderDeleteTenant(t *testing.T, ctx context.Context, pool *pgxpool.Pool) (uuid.UUID, uuid.UUID) { t.Helper() var userID uuid.UUID if err := pool.QueryRow(ctx, ` @@ -352,5 +344,19 @@ func createFolderDeleteTenant(t *testing.T, ctx context.Context, pool *pgxpool.P defer cancel() _, _ = pool.Exec(cleanupCtx, `DELETE FROM users WHERE id = $1`, userID) }) - return userID + var workspaceID uuid.UUID + if err := pool.QueryRow(ctx, ` + INSERT INTO workspaces (name, resource_owner_user_id) + VALUES ('folder-delete', $1) + RETURNING id + `, userID).Scan(&workspaceID); err != nil { + t.Fatalf("create workspace: %v", err) + } + if _, err := pool.Exec(ctx, ` + INSERT INTO workspace_memberships (workspace_id, user_id, role) + VALUES ($1, $2, 'owner') + `, workspaceID, userID); err != nil { + t.Fatalf("create workspace membership: %v", err) + } + return userID, workspaceID } diff --git a/server/internal/folder/folder_test.go b/server/internal/folder/folder_test.go index dcb9fc9..27ad925 100644 --- a/server/internal/folder/folder_test.go +++ b/server/internal/folder/folder_test.go @@ -211,6 +211,7 @@ func TestMemoryPathLifecycleIntegration(t *testing.T) { workspace_id uuid NOT NULL, path text NOT NULL, lifecycle_status text NOT NULL, + source_file_id uuid, updated_at timestamptz NOT NULL DEFAULT now() ) ON COMMIT PRESERVE ROWS `); err != nil { From 91166d3f3755436e66376ffc84b3301d50607175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8B=92=E5=B8=83=E6=9C=97-=E8=A9=B9=E5=A7=86=E6=96=AF?= <318569545+waterbro-8@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:08:23 +0000 Subject: [PATCH 4/4] test(folders): do not reuse one SQL parameter for uuid-adjacent text columns PostgreSQL 42P08: $3 was bound to source_file_sha256 (text) and idempotency_key_sha256 (char(64)) at once. --- server/internal/folder/delete_integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/internal/folder/delete_integration_test.go b/server/internal/folder/delete_integration_test.go index 8a15f84..7ab8d6c 100644 --- a/server/internal/folder/delete_integration_test.go +++ b/server/internal/folder/delete_integration_test.go @@ -302,10 +302,10 @@ func TestRecursiveDeleteBlocksWhenMemoryCitesFileElsewhere(t *testing.T) { ) VALUES ( $1, 'note', 'cites a photo', '/Work/task', 'agent', $2, $3, - $3, $3, $3, + $4, $5, $6, 'active' ) - `, workspaceID, fileID, sha); err != nil { + `, workspaceID, fileID, sha, sha, sha, sha); err != nil { t.Fatalf("insert citing memory: %v", err) }