Skip to content

feat: add deleteFile GraphQL mutation with sync S3 delete + async fallback#1063

Closed
pyramation wants to merge 2 commits intomainfrom
feat/storage-delete-support
Closed

feat: add deleteFile GraphQL mutation with sync S3 delete + async fallback#1063
pyramation wants to merge 2 commits intomainfrom
feat/storage-delete-support

Conversation

@pyramation
Copy link
Copy Markdown
Contributor

Summary

Adds a deleteFile GraphQL mutation to the presigned-url plugin. The mutation deletes a file record from the database (RLS enforced) and attempts to clean up the S3 object synchronously. If the sync S3 delete fails, the AFTER DELETE trigger on the files table (added in constructive-db PR #1033) enqueues an async delete_s3_object job as fallback.

How it works:

  1. Resolve the file row and storage module config across all scopes (app-level + entity-scoped)
  2. DELETE the file row — RLS enforced, only owner/admin can delete. The AFTER DELETE trigger fires and enqueues the async GC job.
  3. Refcount check — if other files reference the same S3 key (content-addressed dedup), skip S3 delete
  4. Sync S3 delete — best-effort inline DeleteObject. If it fails, the async job handles it.

Changes:

  • graphile-presigned-url-plugin/src/plugin.tsDeleteFileInput, DeleteFilePayload types + deleteFile mutation plan + processDelete function
  • graphile-presigned-url-plugin/src/s3-signer.tsdeleteS3Object() helper
  • uploads/s3-utils/src/utils.tsdeleteObject() utility (low-level S3 DeleteObjectCommand)

Companion PR: constructive-db #1033 — AFTER DELETE GC trigger on files table

Tracking: #789

Review & Testing Checklist for Human

  • Verify the deleteFile mutation appears in the GraphQL schema when the presigned-url plugin is loaded
  • Test that deleting a file you own returns { success: true, deletedFromS3: true } and the S3 object is removed
  • Test that deleting a file when content-addressed dedup is active (multiple files share the same key) returns { success: true, deletedFromS3: false } — S3 object preserved
  • Test that an unauthorized user gets DELETE_DENIED error (RLS blocks the DELETE)
  • Verify the async GC job fires when S3 is unavailable (DB trigger fallback)

Notes

  • The deleteObject in s3-utils catches NoSuchKey/404 and returns false (idempotent). The deleteS3Object in s3-signer does not — it lets unexpected errors propagate up to processDelete which catches them and returns { deletedFromS3: false }.
  • The sync S3 delete is a performance optimization. The AFTER DELETE trigger is the reliable path. When sync succeeds, the async job worker will find the S3 object already gone — this is a no-op since DeleteObject is idempotent.
  • No new dependencies added. Uses existing @aws-sdk/client-s3 DeleteObjectCommand.

Link to Devin session: https://app.devin.ai/sessions/ffa3ed8652fc412f976accbdc229c88d
Requested by: @pyramation

pyramation added 2 commits May 6, 2026 19:56
- Add deleteObject to s3-utils (DeleteObjectCommand)
- Add deleteS3Object to s3-signer (used by presigned-url plugin)
- Add deleteFile GraphQL mutation to presigned-url plugin:
  - Resolves file across all storage modules (app + entity-scoped)
  - DELETE file row (RLS enforced)
  - Check refcount for content-hash dedup safety
  - Try sync S3 DeleteObject, fall back to async delete_s3_object job
  - Returns { success, deletedFromS3, key }
The AFTER DELETE trigger on the files table (constructive-db PR #1033)
already enqueues the async delete_s3_object job via SECURITY DEFINER.
The manual job enqueue from the Graphile plugin would fail anyway since
the authenticated role cannot access app_jobs.

The sync S3 delete in the plugin is best-effort; the DB trigger is the
reliable fallback.
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@devin-ai-integration
Copy link
Copy Markdown
Contributor

Closing — this global deleteFile mutation has been superseded by the per-table storage middleware in PR #1064 (now merged into main).

Delete functionality is now handled as middleware wrapping the PostGraphile-generated delete* mutations on @storageFiles-tagged tables (e.g., deleteAppFile, deleteDataRoomFile). No scope resolution or probing needed — PostGraphile already knows which table each type comes from.

@devin-ai-integration
Copy link
Copy Markdown
Contributor

Superseded by PR #1064 (per-table storage middleware, now merged).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant