Grant grace period on uploads for attachments

[devksingh4]

Summary by CodeRabbit

• Bug Fixes
  • Added a 30-minute grace period before removing attachment references when files are temporarily unavailable, preventing premature cleanup of recently uploaded attachments and improving reliability.

[coderabbitai]

Warning

Rate limit exceeded

@devksingh4 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 52 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 9b1fa7b and 9c62496.

📒 Files selected for processing (2)

• src/api/routes/roomRequests.ts (2 hunks)
• src/common/constants.ts (1 hunks)

Walkthrough

Two files are modified to implement a grace period for attachment deletion. A constant UPLOAD_GRACE_PERIOD_MS (30 minutes) is added to constants, then imported and used in roomRequests.ts to conditionally delay DynamoDB attachment key removal when S3 returns NotFound errors until the grace period elapses.

Changes

Cohort / File(s)	Summary
Grace period constant src/common/constants.ts	Added new exported constant UPLOAD_GRACE_PERIOD_MS set to 1,800,000 milliseconds (30 minutes)
Conditional attachment deletion logic src/api/routes/roomRequests.ts	Extended import to include UPLOAD_GRACE_PERIOD_MS; modified two code paths (status update handling and attachment download URL retrieval) to conditionally remove DynamoDB attachment reference only after grace period has elapsed since creation; if grace period has not passed, propagate NotFoundError without deletion

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler as roomRequests Handler
    participant S3
    participant DynamoDB

    rect rgb(240, 248, 255)
    Note over Handler,DynamoDB: Before: Immediate deletion on S3 NotFound
    Client->>Handler: Request (status update / URL retrieval)
    Handler->>S3: Fetch attachment
    S3-->>Handler: NotFound error
    Handler->>DynamoDB: DeleteItem (attachment key)
    DynamoDB-->>Handler: Deleted
    Handler-->>Client: Error response
    end

    rect rgb(240, 255, 240)
    Note over Handler,DynamoDB: After: Grace period check before deletion
    Client->>Handler: Request (status update / URL retrieval)
    Handler->>S3: Fetch attachment
    S3-->>Handler: NotFound error
    Handler->>Handler: Check: createdAt + grace period?
    alt Grace period elapsed
        Handler->>DynamoDB: UpdateItem (remove attachment key)
        DynamoDB-->>Handler: Updated
    else Grace period active
        Handler->>Handler: Skip deletion
    end
    Handler-->>Client: Error response
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Both modified code paths follow a consistent pattern (time-based conditional check before deletion)
• The grace period logic is straightforward to validate
• Verify correct computation of elapsed time using createdAt timestamp
• Confirm the constant value (30 minutes) aligns with product intent

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "Grant grace period on uploads for attachments" directly and accurately describes the main change in the pull request. The changeset adds a 30-minute grace period (UPLOAD_GRACE_PERIOD_MS constant) before removing attachment metadata from DynamoDB when S3 objects are not found, which is exactly what the title conveys. The title is concise, specific, and uses clear language that would help teammates understand the purpose of these changes at a glance. It avoids vague terms and directly references the core concept being introduced.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

💰 Infracost report

Monthly estimate generated

_{This comment will be updated when code changes.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/api/routes/roomRequests.ts (2)

38-41: Address ESLint warning about file extension.

ESLint is flagging the .js extension in the import path. Consider removing it for consistency with linting rules.

Apply this diff:

 import {
   ROOM_RESERVATION_RETENTION_DAYS,
   UPLOAD_GRACE_PERIOD_MS,
-} from "common/constants.js";
+} from "common/constants";

Based on static analysis hints.

---

686-708: Consider adding logging for observability.

The grace period logic would benefit from logging to distinguish between cases where the grace period is still active versus when the DynamoDB reference is being cleaned up. This would help with debugging and monitoring upload issues.

Consider adding log statements like:

if (timeSinceCreation >= UPLOAD_GRACE_PERIOD_MS) {
  request.log.info(
    `Grace period elapsed for attachment ${unmarshalled.attachmentS3key}, removing DynamoDB reference`
  );
  // Grace period has passed, delete the attribute from DynamoDB
  await fastify.dynamoClient.send(
    // ... existing code
  );
} else {
  request.log.info(
    `Attachment not found but within grace period (${timeSinceCreation}ms / ${UPLOAD_GRACE_PERIOD_MS}ms)`
  );
}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 06aa3c0 and 9b1fa7b.

📒 Files selected for processing (2)

• src/api/routes/roomRequests.ts (2 hunks)
• src/common/constants.ts (1 hunks)

🧰 Additional context used

🪛 ESLint

src/api/routes/roomRequests.ts

[error] 41-41: Unexpected use of file extension "js" for "common/constants.js"

(import/extensions)

🪛 GitHub Actions: Required Reviews

src/common/constants.ts

[error] 1-1: CI check failed: Base Requirement not satisfied by the existing reviews.

src/api/routes/roomRequests.ts

[error] 1-1: CI check failed: Base Requirement not satisfied by the existing reviews.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Build Application
• GitHub Check: Run Unit Tests

🔇 Additional comments (2)

src/common/constants.ts (1)

6-6: LGTM!

The constant is well-named, clearly documented, and the 30-minute grace period is reasonable for handling eventual consistency or upload delays.

src/api/routes/roomRequests.ts (1)

686-708: Review comment is incorrect; no issues found.

The codebase has only one location where HeadObjectCommand is used (attachment download URL retrieval at line 679), making it the only place where S3 NotFound exceptions are caught. The status update path uses createPresignedPut, which generates presigned upload URLs without validating object existence. The grace period logic is correctly implemented in the sole location where it's needed.

Likely an incorrect or invalid review comment.