Conversation
|
Warning Review limit reached
Next review available in: 49 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe metadata correction workflow records normalized CMR writeback errors on failed audit rows, exposes them through audit queries, and preserves the original failure. Local tooling simulates ingest failures and validates the end-to-end path. CMR requests now have timeouts and validation controls, and the service receives reserved concurrency configuration. Metadata correction writeback failures
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SmokeTest
participant MockCMRServer
participant metadataCorrectionService
participant RDF4J
SmokeTest->>MockCMRServer: configure ingest failure
SmokeTest->>metadataCorrectionService: invoke correction event
metadataCorrectionService->>MockCMRServer: write corrected metadata with timeout and validation headers
MockCMRServer-->>metadataCorrectionService: configured error response
metadataCorrectionService->>RDF4J: persist failed audit row
SmokeTest->>RDF4J: fetch audit rows
RDF4J-->>SmokeTest: failed status and error message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
=======================================
Coverage 99.73% 99.73%
=======================================
Files 229 229
Lines 6029 6059 +30
Branches 1773 1776 +3
=======================================
+ Hits 6013 6043 +30
Misses 14 14
Partials 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
serverless/src/shared/cmrPutRequest.js (1)
106-132: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the timeout active through response-body consumption.
fetch()resolves once headers arrive, so Line 106 can return aResponsewhile its body still stalls. Line 129 then clears the timer beforewriteCorrectedMetadataToCmrparses either a success or error body, allowing the Lambda to run until its 30-second timeout and preventing failed-audit handling. Apply the deadline through response parsing, not only until headers are received.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serverless/src/shared/cmrPutRequest.js` around lines 106 - 132, Keep the timeout established by the CMR PUT request active through response-body consumption and parsing, rather than clearing it immediately after fetch resolves. Update the request flow around the fetch return and its caller’s success/error body parsing so the deadline covers both headers and the complete response body; clear the timer only after parsing finishes or the request otherwise completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@serverless/src/shared/cmrPutRequest.js`:
- Around line 106-132: Keep the timeout established by the CMR PUT request
active through response-body consumption and parsing, rather than clearing it
immediately after fetch resolves. Update the request flow around the fetch
return and its caller’s success/error body parsing so the deadline covers both
headers and the complete response body; clear the timer only after parsing
finishes or the request otherwise completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b88137ec-a6e6-4f3b-9ca4-fc9ff658e2e5
📒 Files selected for processing (8)
bin/deploy-bamboo.shcdk/app/lib/CmrEventProcessingStack.tscdk/app/lib/helper/MetadataCorrectionSetup.tscdk/bin/main.tsserverless/src/shared/__tests__/cmrPutRequest.test.jsserverless/src/shared/__tests__/writeCorrectedMetadataToCmr.test.jsserverless/src/shared/cmrPutRequest.jsserverless/src/shared/writeCorrectedMetadataToCmr.js
| * Creates the metadata correction SNS/SQS/Lambda plumbing and exports its endpoints. | ||
| */ | ||
| export class MetadataCorrectionSetup extends Construct { | ||
| private static readonly METADATA_CORRECTION_SERVICE_RESERVED_CONCURRENCY = 5 |
There was a problem hiding this comment.
Isn't this just going to override the bamboo var? I don't feel strongly either way of if we need it at deployment vs hardcode but
| writebackErrorMessage: getWritebackErrorMessage(error) | ||
| }) | ||
|
|
||
| logger.info('[metadata-correction] Persisted failed metadata correction audit log', { |
There was a problem hiding this comment.
| logger.info('[metadata-correction] Persisted failed metadata correction audit log', { | |
| logger.debug('[metadata-correction] Persisted failed metadata correction audit log', { |
There was a problem hiding this comment.
I could say either way I'm just trying to be conscious right now the api has alot of logging in place as a baseline
| Authorization: 'Bearer writer-token' | ||
| Authorization: 'Bearer writer-token', | ||
| 'cmr-validate-keywords': 'false', | ||
| 'Cmr-Validate-Umm-C': 'false' |
There was a problem hiding this comment.
I think this is right one thing with the cmr-validate-umm-c var one to keep in mind is in the CMR docs it says CMR was going to make that the default but, that was > 5 years ago. I haven't dug very deeply into what that code does yet
| await rejectionExpectation | ||
| }) | ||
|
|
||
| test('should skip abort wiring when AbortController is unavailable', async () => { |
There was a problem hiding this comment.
Seems good for coverage but, isn't this just a core node feature would we expect this to happen?
3 participants
Overview
What is the feature?
Captures CMR ingest/writeback failures in the metadata correction audit log so failed writeback attempts are visible alongside successful correction history.
Today we persist
pendingbefore writeback andappliedafter success, but if CMR rejects the corrected metadata the audit log does not clearly show that failure or the ingest error returned by CMR.What is the Solution?
This branch updates the metadata correction audit flow to persist failed writeback attempts with the CMR error payload.
Key changes:
writebackErrorMessagepersistence to metadata correction audit rows.writebackErrorMessagewhen present.runCollectionMetadataCorrectionso that when CMR writeback fails after corrections were applied:pendingaudit row behavior stays the samestatus=failederror.cmrResponseBodyover the generic error message when building the stored writeback error text, so audit rows capture the actual CMR ingest response body when available.What areas of the application does this impact?
Testing
Checklist
Summary by CodeRabbit
New Features
writebackErrorMessagefor failed writebacks.Bug Fixes
New/Updated Tests
Chores