Conversation
Service emits "Invalid checkpoint token" (lowercase). The SDK compared with a Title Case prefix, so stale tokens were treated as non-retryable execution failures. Compare case-insensitively and cover both casings in unit tests. Fixes aws#721
|
Thank you for the contribution! Overall approach looks good to me - just some small requested changes. Also, I ran the CI tests for this PR and it looks like some checks are failing. Can you please have a look and address that as well? Example failure. |
hln33
added
the
changes-requested
| ], | ||
| ) | ||
| def test_checkpoint_error_classification_invalid_token_invocation(message: str): | ||
| """Service emits lowercase 'checkpoint token'; match case-insensitively as invocation.""" |
There was a problem hiding this comment.
I would prefer the original description for this test.
I feel that it more clearly conveys what the purpose of the test is. The original also follows the format that most of the test descriptions in this file use.
Maybe something like this is better:
| """Service emits lowercase 'checkpoint token'; match case-insensitively as invocation.""" | |
| """Test 4xx InvalidParameterValueException with "Invalid checkpoint token" error message is an invocation error.""" |
There was a problem hiding this comment.
Restored the original-style test description and dropped the parametrize so we only cover the backend casing.
| INVALID_CHECKPOINT_TOKEN_PREFIX | ||
| ) | ||
| and (error.get("Message") or "") | ||
| .casefold() |
There was a problem hiding this comment.
let's perform an exact string match instead of doing a lowercase/casefold match. The main purpose is to match the backend exactly. If the error message does not exactly match we wouldn't want this condition to be true.
Can you please make this comparison case-sensitive and amend the other changes in this PR to reflect that?
There was a problem hiding this comment.
Switched to an exact case-sensitive .startswith("Invalid checkpoint token") match (no casefold) so we only treat the backend’s exact message as invocation.
Match the backend error prefix case-sensitively and restore the single-unit test description preferred in review.
|
Updated per review: switched to an exact case-sensitive match on |
|
LGTM! Integration/Conformance tests are failing due to missing credentials, but that is expected for external PRs as we only grant Thanks once again for your contribution! |
2 participants
Issue #, if available:: #721
Description of changes:
The service emits
Invalid checkpoint token(lowercase). The SDK matched a Title Case prefix, so a stale checkpoint token was classified as a non-retryable execution failure instead of a retryable invocation failure.Testing:
hatch run dev-core:test: 1670 passedhatch fmt --checkin the core package: passedBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.