Repository navigation
fix(testing): fail fast on invalid handler output - #777
Thanatos9404 wants to merge 5 commits into
Conversation
The local runner retried every invalid invocation output up to five times, five seconds apart. The service fails the execution on the first such output, with an InvalidParameterValueException error, and retries only PENDING with no pending operations, up to three attempts. - Fail the execution at once for FAILED with a Result, SUCCEEDED with an Error, a missing or unknown Status, and an output that cannot be parsed. The error has ErrorType InvalidParameterValueException and the validation message as ErrorMessage. - Keep retrying PENDING with no pending operations, capped at three consecutive attempts. Other retries keep the five-attempt budget. - Parse handler output in InvokeResponse.from_output and from_payload, which raise InvalidInvocationOutputError, so the executor can tell an output it cannot parse from a failed invocation.
|
/ai review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
InvokeResponse.from_output and from_payload get the invocation's request ID, but dropped it when they raised InvalidInvocationOutputError. The executor then recorded the invocation completion under a new UUID, so a malformed-output failure could not be matched to the invocation that produced it. InvalidInvocationOutputError now carries an optional request_id, both factories set it, and the executor records it. An error raised without one still gets a generated ID.
|
/ai review |
Codex AI reviewNo actionable findings. The prior request-ID finding is resolved. Review was limited to static inspection; runtime behavior and test results were not independently verified. Reviewed commit |
zhongkechen
left a comment
zhongkechen
left a comment
There was a problem hiding this comment.
Reviewed commit a389f5f. I reproduced two remaining gaps in invalid-output handling and an incomplete part of the request-ID fix, detailed inline. These are existing paths left incompletely fixed, rather than newly introduced regressions.
Validation: all 267 selected executor, checkpoint, invoker, exception, and invalid-output tests passed. Additional targeted reproductions covered the three store backends, undecodable response bytes, and real boto3 parsing against a local HTTP server.
| a known ``Status`` and, if present, an object ``Error``. | ||
| """ | ||
| try: | ||
| invocation_output = DurableExecutionInvocationOutput.from_dict(output) |
There was a problem hiding this comment.
[P2] Preserve an empty Error object before validating SUCCEEDED
The SDK's from_dict() treats every falsey Error as absent, so this factory turns {"Status":"SUCCEEDED","Error":{}} into an output with error=None. The executor consequently marks the execution SUCCEEDED instead of failing it with InvalidParameterValueException. I reproduced this through the runner with memory, filesystem, and SQLite stores; a nonempty Error object correctly fails. Falsey non-object values such as [], 0, and false also bypass validation. This leaves #670's explicit SUCCEEDED-with-an-Error case incompletely fixed. Please validate the raw field's presence/type and preserve an explicitly supplied empty object before normalization, with regression coverage.
There was a problem hiding this comment.
Thanks for checking this on all three stores. Fixed in f16a410.
InvokeResponse.from_output now keeps any Error that is present and not null, so {"Status": "SUCCEEDED", "Error": {}} fails with InvalidParameterValueException ("Cannot provide an Error for SUCCEEDED status."), the same as a nonempty Error. A falsey value that is not an object ([], 0, false) now raises InvalidInvocationOutputError, so the execution fails after one invocation. test_falsey_error_fails_a_succeeded_output_on_each_store in tests/e2e/invalid_invocation_output_test.py runs all four values on the memory, filesystem and SQLite stores. test_in_process_invoker_keeps_an_empty_error_object and the new cases in test_in_process_invoker_rejects_an_output_it_cannot_parse cover the parse step.
|
|
||
| # Parse response payload | ||
| # Read response payload | ||
| response_payload = response["Payload"].read().decode("utf-8") |
There was a problem hiding this comment.
[P2] Include UTF-8 decoding in invalid-output handling
For a successful Lambda invocation whose payload is b"\xff", decoding fails here before the new InvokeResponse.from_payload() path runs. The broad invocation exception handler wraps UnicodeDecodeError as DurableFunctionsTestError, so malformed output is still retried. The reproduction made five handler calls, scheduled four retries, and ended with the wrong error type and no invocation completion recorded; ordinary invalid JSON failed after one call as intended. Please classify decoding failures as InvalidInvocationOutputError too, retaining the response request ID, so undecodable output follows the same immediate-failure path.
There was a problem hiding this comment.
Good catch, fixed in f16a410.
LambdaInvoker now reads the payload as bytes, and InvokeResponse.from_payload decodes it in the same try as the JSON parse. So b"\xff" raises InvalidInvocationOutputError with the response's request ID instead of being wrapped as a failed invocation. It is covered by the not-utf-8 cases in test_lambda_invoker_rejects_an_output_it_cannot_parse and test_lambda_invoker_rejects_with_the_request_id_botocore_parses, and by from-payload-not-utf-8 in test_unparsed_output_keeps_the_request_id, which checks that the executor fails the execution on that first invocation and records it under the same ID.
| @@ -508,10 +536,6 @@ def invoke( | |||
| or f"local-{uuid4()}" | |||
There was a problem hiding this comment.
[P3] Preserve the request ID from actual boto3 responses
The request-ID fix still propagates a fabricated ID through the real HTTP path. Botocore exposes the header as HTTPHeaders["x-amzn-requestid"] and also supplies ResponseMetadata["RequestId"]; the two header spellings checked above miss both locations and fall back to local-UUID. I reproduced this with a local HTTP server sending x-amzn-RequestId and real boto3 parsing, for both invalid JSON and an unknown Status: the response metadata contained the actual ID, but InvalidInvocationOutputError.request_id contained the generated one. Prefer ResponseMetadata.RequestId with a normalized header fallback. The new mock tests use mixed-case headers, which masks this remaining part of the original request-ID finding.
There was a problem hiding this comment.
You're right, the mixed-case mocks hid this. Fixed in f16a410.
The invoker now uses ResponseMetadata["RequestId"] first, then the x-amzn-requestid or x-amzn-request-id header matched case-insensitively, and only then a generated ID. The mocked responses in invoker_test.py now have the shape boto3 returns (RequestId plus lowercased HTTPHeaders). test_lambda_invoker_keeps_the_request_id_botocore_parses and test_lambda_invoker_rejects_with_the_request_id_botocore_parses use a real client and answer the Invoke at before-send with x-amzn-RequestId: request-1, so botocore parses the response. They check the ID for a valid output, invalid JSON, an unknown Status and undecodable bytes. test_lambda_invoker_reads_the_request_id_from_the_response covers the fallback order.
Three kinds of output still got past the fail-fast path. - DurableExecutionInvocationOutput.from_dict treats any falsey Error as absent, so SUCCEEDED with an empty Error object completed the execution, and a falsey Error that is not an object ([], 0, false) was never checked. InvokeResponse.from_output now keeps any Error that is present and not null, so an empty object fails SUCCEEDED validation and a value that is not an object raises InvalidInvocationOutputError. - LambdaInvoker decoded the payload inside its try, so a payload that is not UTF-8 was reported as a failed invocation and retried. from_payload now takes the bytes and decodes them itself, so undecodable output raises InvalidInvocationOutputError with the request ID. - botocore returns the request ID as ResponseMetadata.RequestId and the header as x-amzn-requestid. The mixed-case header lookups missed both and fell back to a generated ID. The invoker now reads RequestId first, then the headers by lowercased name.
This branch was successfully deployed
2 participants
Issue #, if available: #670
Description of changes:
The local runner sent every invalid invocation output through
_retry_invocation: up to five attempts, five seconds apart, then a failure withErrorObject.from_exception(e). As #670 describes, the service fails the execution on the first invalid output and retries only PENDING with no pending operations, up to three attempts. This implements the fix proposed there.Executor._finish_invocation: a validation failure now fails the execution at once with{ErrorType: "InvalidParameterValueException", ErrorMessage: <validation message>}. That covers FAILED with a Result, SUCCEEDED with an Error, and a status the runner does not accept (such asRETRY). PENDING with no pending operations still goes through_retry_invocation, now capped atMAX_CONSECUTIVE_INVALID_PENDING_ATTEMPTS = 3, and fails with the same error shape.Status, or an output that is not a JSON object, never reached_finish_invocation:DurableExecutionInvocationOutput.from_dictraised inside the invoker, and the executor retried that as a failed invocation. Both invokers now parse through newInvokeResponse.from_outputandInvokeResponse.from_payloadfactories, which raise a newInvalidInvocationOutputError(aDurableFunctionsTestError)._invoke_handlerrecords the invocation and fails the execution with the same error shape. A stop or timeout that landed during the invocation still stands.LambdaInvokerparses the payload after itstry, so a payload that is not JSON is no longer reported as "Unexpected error during Lambda invocation"._retry_invocationtakes an optionalmax_attempts. Handler exceptions, invoke errors and invocation timeouts keepMAX_CONSECUTIVE_FAILED_ATTEMPTS = 5.docs/architecture.md: the retry budget section describes the split.The PENDING retry shares the existing
consecutive_failed_invocation_attemptscounter, so earlier consecutive failed attempts count toward its cap of 3 (two handler errors followed by an invalid PENDING fail at once). I can give it its own counter if the service counts them separately.Testing:
tests/e2e/invalid_invocation_output_test.py: six invalid outputs (FAILED with a Result, SUCCEEDED with an Error, no Status, unknown Status,RETRY, a JSON array) each fail the execution after one handler call with theInvalidParameterValueExceptionerror. Onmainall six time out while the retries run.executor_checkpoint_test.py: PENDING with no pending operations fails on the third attempt. PENDING once, then SUCCEEDED, completes and resets the counter.executor_test.py: the fourtest_should_retry_when_*tests for outputs that now fail fast are nowtest_should_fail_when_*. Two new tests cover the unparseable-output path, one with a stop during the invocation.test_should_retry_when_pending_response_has_no_operationsnow setsseq_counter; without it the test hit aTypeErrorinmax()and retried through the generic error path, not validation.invoker_test.py,exceptions_test.py: both invokers raiseInvalidInvocationOutputErrorfor outputs they cannot parse.Locally (Windows, Python 3.13, Hatch 1.16.5):
hatch run test:allhas no new failures (only the pre-existing Windows-only ones in the corelambda_service_test.pynegative timestamps and the microvm-workerworker_test.pyport tests, as onmain);hatch run dev-testing:covpasses at 96.08%;hatch run dev-testing:typecheck,hatch run types:checkandhatch fmt --checkare clean. The asserted error messages are identical on Python 3.11 to 3.14.hatch run dev-testing:js-exampleswas not run, because the harness needsfcntl; CI runs it. No conformance tests were added or changed.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.