chore(recording-api): fix aws error handling - #64376
Merged
Merged
Conversation
z0br0wn
requested review from
a team,
TueHaulund,
arnohillen,
fasyy612 and
ksvat
and removed request for
a team
June 17, 2026 18:50
Contributor
|
Reviews (1): Last reviewed commit: "chore(recording-api): fix aws error hand..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens AWS SDK v3 error handling in the session replay recording API and related DynamoDB keystore/test code by replacing instanceof <AwsExceptionClass> checks with string matching on error.name, avoiding failures when multiple physical copies of the AWS SDK exist in the dependency tree.
Changes:
- Replace DynamoDB conditional write error detection with
error.name === 'ConditionalCheckFailedException'to preserve the “return existing key” path. - Replace S3 “missing object” detection with
error.name === 'NoSuchKey'to return a{ ok: false, error: 'not_found' }response instead of throwing. - Update Localstack integration test table setup/wait loops to identify DynamoDB “table not found” via
error.name === 'ResourceNotFoundException'.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| nodejs/src/session-replay/shared/keystore/dynamodb-keystore.ts | Switch conditional-write exception detection from instanceof to error.name to keep concurrent-creation behavior correct under duplicated AWS SDK deps. |
| nodejs/src/session-replay/recording-api/recording-service.ts | Switch S3 NoSuchKey handling from instanceof to error.name so missing blocks are treated as not-found reliably. |
| nodejs/src/session-replay/recording-api/recording-api.integration.test.ts | Switch DynamoDB ResourceNotFound detection from instanceof to error.name to prevent Localstack setup teardown failures under duplicated AWS SDK deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
z0br0wn
force-pushed
the
zbrown/recording-api-aws-error-name-match
branch
from
June 17, 2026 19:03
2d1c588 to
642c032
Compare
ksvat
approved these changes
Jun 17, 2026
eli-r-ph
approved these changes
Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three sites, one pattern: error instanceof → (error as Error)?.name === ''.
Why it was needed: CI is failing on #64357. A recent master dep change (the agent-platform merge, #63988) added more AWS SDK versions, leaving the tree with several @aws-sdk/core / @smithy/core copies. AWS SDK v3 ships each client's exception classes inside the package, so when the SDK is duplicated, an error thrown by one physical copy is not instanceof the class imported from another copy — the guard silently returns false. In the test that meant an expected ResourceNotFoundException was re-thrown, killing all 16 tests in beforeAll. The same latent failure mode sat in two production paths (graceful S3 "not found" becoming a thrown error; the keystore's concurrent-creation path throwing instead of returning the existing key).
How did you test this code?
see if ci passes