OUT-3763 (2/2) | Stop comment-create from leaving stale content on attachment paths#1252
OUT-3763 (2/2) | Stop comment-create from leaving stale content on attachment paths#1252arpandhakal wants to merge 1 commit into
Conversation
…chment paths The previous flow moved (renamed) each attachment to its comment-scoped path, then signed the new path. If signing failed — Supabase's signer can briefly lag a fresh write — the URL was silently dropped, no throw, and the comment was saved with content still pointing at the now-empty old path. Switch to copy-not-move, retry signing once after 200ms, and throw on persistent failure so the existing try/catch rolls back the comment. Originals are removed only after the rewritten content commits, so any failure mid-flight is recoverable. Re-throw on rollback so the activity log, notifications, and webhook don't fire for a deleted comment. Also: replace → replaceAll covers the same-file-embedded-twice edge case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes the race condition behind OUT-3763 where a comment's
Confidence Score: 4/5The core race fix is sound and the failure table in the PR description maps cleanly to the code. The only meaningful concern is that attachment processing is now fully sequential, which can add noticeable latency on comments with several files. The copy-before-write approach correctly eliminates the stale-content race. The missing throw e in the catch block was a real pre-existing bug (activity logs fired for deleted comments) and is now fixed. The sequential loop is a deliberate trade-off called out in the PR description, but it does meaningfully increase p50 latency for multi-attachment comments and could be revisited. The hard-coded 200 ms retry delay is a minor maintainability concern. src/app/api/comments/comment.service.ts — specifically the sequential attachment-processing loop (lines 380–408) if multi-attachment comment performance becomes a concern. Important Files Changed
Reviews (1): Last reviewed commit: "fix(OUT-3763): stop comment-create from ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!newUrl) { | ||
| // Supabase's signer can briefly lag a fresh write. | ||
| await new Promise((resolve) => setTimeout(resolve, 200)) | ||
| newUrl = await getSignedUrl(newFilePath) | ||
| } |
There was a problem hiding this comment.
signedUrlTtl lives in src/constants/attachments.ts — a companion constant like SIGNER_LAG_RETRY_MS there would make intent clear and allow it to be tuned alongside the TTL.
| if (!newUrl) { | |
| // Supabase's signer can briefly lag a fresh write. | |
| await new Promise((resolve) => setTimeout(resolve, 200)) | |
| newUrl = await getSignedUrl(newFilePath) | |
| } | |
| if (!newUrl) { | |
| // Supabase's signer can briefly lag a fresh write. | |
| await new Promise((resolve) => setTimeout(resolve, SIGNER_LAG_RETRY_MS)) | |
| newUrl = await getSignedUrl(newFilePath) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| for (const { originalSrc, filePath, fileName } of matches) { | ||
| const newFilePath = `${this.user.workspaceId}/${task_id}/comments/${commentId}/${fileName}` | ||
| const supabaseActions = new SupabaseActions() | ||
|
|
||
| const fileMetaData = await supabaseActions.getMetaData(filePath) | ||
| createAttachmentPayloads.push( | ||
| CreateAttachmentRequestSchema.parse({ | ||
| commentId: commentId, | ||
| commentId, | ||
| filePath: newFilePath, | ||
| fileSize: fileMetaData?.size, | ||
| fileType: fileMetaData?.contentType, | ||
| fileName: fileMetaData?.metadata?.originalFileName || getFileNameFromPath(newFilePath), | ||
| }), | ||
| ) | ||
| copyAttachmentPromises.push(supabaseActions.moveAttachment(filePath, newFilePath)) | ||
| newFilePaths.push({ originalSrc, newFilePath }) | ||
| await supabaseActions.copyAttachment(filePath, newFilePath) | ||
|
|
||
| let newUrl = await getSignedUrl(newFilePath) | ||
| if (!newUrl) { | ||
| // Supabase's signer can briefly lag a fresh write. | ||
| await new Promise((resolve) => setTimeout(resolve, 200)) | ||
| newUrl = await getSignedUrl(newFilePath) | ||
| } | ||
| if (!newUrl) { | ||
| throw new APIError( | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| `Failed to sign URL for newly copied attachment at ${newFilePath}`, | ||
| ) | ||
| } | ||
| replacements.push({ originalSrc, newUrl }) | ||
| oldFilePaths.push(filePath) | ||
| newFilePathsList.push(newFilePath) |
There was a problem hiding this comment.
The new loop runs getMetaData → copyAttachment → getSignedUrl (× up to 2) serially for every matched attachment. The previous code parallelised all copies via Promise.all(copyAttachmentPromises) and all signings via Promise.all(signedUrlPromises). For a comment with N attachments at ~150 ms per round-trip, the old path cost roughly max(copy) + max(sign) ≈ 300 ms; the new path costs N × (getMetaData + copy + sign) ≈ N × 450 ms.
The serialisation was needed to get clean early-exit semantics on the first signing failure, but the full potential parallelism still exists — all getMetaData calls are independent, all copyAttachment calls are independent, and signing can be batched once all copies are in flight. Consider structuring as three sequential Promise.all passes (fetch metadata → copy → sign with per-item retry) so failure on any single item still aborts cleanly but the N-1 happy-path attachments are processed concurrently.
1 participant
Changes
Fixes the underlying race that produced the ~26 corrupted rows OUT-3763 is patching on read. The previous flow:
moveAttachment(oldPath, newPath)— rename, file gone fromoldPathimmediately.getSignedUrl(newPath)— Supabase's signer can briefly lag the rename. On failure,getSignedUrlreturnedundefined, which the call site silently dropped (if (newUrl) push(...)).comment.update({ content: newContent })— saved content with the old (now-empty) URL still in place.In
src/app/api/comments/comment.service.ts:updateCommentIdOfAttachmentsAfterCreation:moveAttachment→copyAttachment. The original stays in place during the rewrite.APIErroron persistent failure instead of silently dropping the replacement.htmlString.replace→htmlString.replaceAllso the same file embedded twice gets both occurrences rewritten.{ content, oldFilePaths }so the caller can defer deleting the originals until after the rewritten content commits.create:oldFilePathsToRemoveoutside the try block.comment.updatesucceeds, removes each original file (per-file try/catch — a delete failure is logged, doesn't fail the request, recoverable via scrap-media cleanup).comment.deleteso the activity log, notifications, and webhook below don't fire for a comment that no longer exists in the DB.Failure modes after the change
copyAttachmentfailscreateMultipleAttachmentsfailscomment.updatefailsremoveAttachment(post-commit) failsThe previous "comment committed with stale content + file gone from old path" state is no longer reachable.
Testing Criteria
Comments.contentcontains the comment-scoped path.getSignedUrlto returnundefinedfor any path containing/comments/) → create attempt returns 500, noCommentrow remains in the DB, no activity log entry created, no webhook fired.removeAttachmentcleanup ran).Notes
Comments.contentrows still need a backfill (separate ticket).tasksShared.service.ts:498for tasks. Not changed here per scope (you confirmed tasks aren't actually hitting this path in current usage). Worth a follow-up sanity check that the new-task-with-staged-files flow doesn't trigger the same bug class.Impact & Surface Area of Change
withErrorHandleralready surfaces this as a clean HTTP error.🤖 Generated with Claude Code