Repository navigation
test(extras): fix the #811 deploy test race - #818
Merged
Merged
Conversation
Two session tests of #811 check the resume mode across a deploy. The handler counts its invocations. The first invocation uses the launch's config. Every later invocation uses the deployed config. Each test runs two jobs and asserts len(invocations) > 1. 1. The run fixture completes each callback as soon as the job creates it. 2. A checkpoint response carries every operation that changed since the last response. 3. So the first callback's result can reach the first invocation. 4. So both jobs can be delivered in the first invocation, with the launch's config. 5. A suspension while the second job waits then satisfies len(invocations) > 1. No job is delivered after the deploy. 6. If both callbacks complete early, the execution ends in one invocation. The assertion then fails. The Codex review of #811 reported this race. In 65 local runs of the old test body, the second job was delivered in invocation 2. The test body did not ensure that order. The two tests now run a durable wait between the jobs. A durable wait always ends the invocation. The local runner skips the wait's time. So the second job is delivered in a later invocation, after the deploy, in every run. Each test also records the invocation that sent each request. It asserts that the first request was sent in invocation 1, and the second request in a later invocation. The tests fail against the session code before #811. The failures are at the GetMicrovm count: 1 instead of 3, and 1 instead of 0.
The README said that the JavaScript operation follows redirects. aws/aws-durable-execution-sdk-js#1036 changed that. The JavaScript request step now passes redirect: "manual". It fails every 3xx at once. The operation then raises MicrovmDeliveryError. So the README now states that the JavaScript operation also fails every 3xx at once, with MicrovmDeliveryError.
The send_job docstring said that the JavaScript operation follows redirects. aws/aws-durable-execution-sdk-js#1036 changed that. The JavaScript request step now fails every 3xx at once. So the docstring contradicted the corrected README sentence. The docstring now states that neither operation follows a redirect. It then states why: a followed 301, 302, or 303 would send a GET without the job, and a followed 307 or 308 to another host would receive the auth token.
The handler two_jobs_in_two_invocations runs two jobs with a durable wait between them. The execution runs 4 invocations, not 2. So the name is now two_jobs_with_a_wait_between. The SDK's wait returns without a suspension if the START checkpoint response marks the wait SUCCEEDED. The local runner always answers a START with STARTED. So the docstring now states that a durable wait ends the invocation in the local runner. A request can also time out without an answer. So the comment in _InvocationLog.append now says that the POST gets an answer or times out, and that the fake endpoint answers at once. The sent_in assertions require at least 2 invocations. So the two deploy tests no longer assert len(invocations) > 1.
ParidelPooya
marked this pull request as ready for review
October 23, 2026 20:32
ParidelPooya
temporarily deployed
to
ai-pr-review-runtime
October 23, 2026 20:32 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
ParidelPooya
had a problem deploying
to
ai-pr-review-runtime
October 23, 2026 20:33 — with
GitHub Actions
Error
GitHub Actions
Error
ParidelPooya
temporarily deployed
to
ai-pr-review-runtime
October 23, 2026 20:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Codex AI reviewNo actionable findings in the PR changes. Residual test risk: this was a static, read-only review; tests were not executed. Reviewed commit |
zhongkechen
pushed a commit
that referenced
this pull request
Oct 23, 2026
* test(extras): end the invocation between two jobs Two session tests of #811 check the resume mode across a deploy. The handler counts its invocations. The first invocation uses the launch's config. Every later invocation uses the deployed config. Each test runs two jobs and asserts len(invocations) > 1. 1. The run fixture completes each callback as soon as the job creates it. 2. A checkpoint response carries every operation that changed since the last response. 3. So the first callback's result can reach the first invocation. 4. So both jobs can be delivered in the first invocation, with the launch's config. 5. A suspension while the second job waits then satisfies len(invocations) > 1. No job is delivered after the deploy. 6. If both callbacks complete early, the execution ends in one invocation. The assertion then fails. The Codex review of #811 reported this race. In 65 local runs of the old test body, the second job was delivered in invocation 2. The test body did not ensure that order. The two tests now run a durable wait between the jobs. A durable wait always ends the invocation. The local runner skips the wait's time. So the second job is delivered in a later invocation, after the deploy, in every run. Each test also records the invocation that sent each request. It asserts that the first request was sent in invocation 1, and the second request in a later invocation. The tests fail against the session code before #811. The failures are at the GetMicrovm count: 1 instead of 3, and 1 instead of 0. * docs(extras): state the JS redirect behavior The README said that the JavaScript operation follows redirects. aws/aws-durable-execution-sdk-js#1036 changed that. The JavaScript request step now passes redirect: "manual". It fails every 3xx at once. The operation then raises MicrovmDeliveryError. So the README now states that the JavaScript operation also fails every 3xx at once, with MicrovmDeliveryError. * docs(extras): fix the send_job redirect docstring The send_job docstring said that the JavaScript operation follows redirects. aws/aws-durable-execution-sdk-js#1036 changed that. The JavaScript request step now fails every 3xx at once. So the docstring contradicted the corrected README sentence. The docstring now states that neither operation follows a redirect. It then states why: a followed 301, 302, or 303 would send a GET without the job, and a followed 307 or 308 to another host would receive the auth token. * test(extras): name the wait between the two jobs The handler two_jobs_in_two_invocations runs two jobs with a durable wait between them. The execution runs 4 invocations, not 2. So the name is now two_jobs_with_a_wait_between. The SDK's wait returns without a suspension if the START checkpoint response marks the wait SUCCEEDED. The local runner always answers a START with STARTED. So the docstring now states that a durable wait ends the invocation in the local runner. A request can also time out without an answer. So the comment in _InvocationLog.append now says that the POST gets an answer or times out, and that the fake endpoint answers at once. The sent_in assertions require at least 2 invocations. So the two deploy tests no longer assert len(invocations) > 1.
This branch was previously deployed
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.
Summary
This PR makes two session deploy tests from #811 deterministic. It also corrects a README sentence and a docstring about JavaScript redirect behavior.
Why
arf_v1_ajwnsebjasbacyyspt6wbmvvj4, P2) found a race in two tests.runfixture completes each callback as soon as the job creates it.len(invocations) > 1can then pass without any delivery after the deploy.MicrovmDeliveryError. Thesend_jobdocstring inrequest.pymade the same claim.Changes
tests/microvm/session_test.py:test_still_resumes_before_each_job_when_a_deploy_turns_suspending_offandtest_makes_no_state_check_when_a_deploy_turns_suspending_onnow run a durable wait between the two jobs. In the local runner, a durable wait ends the invocation.two_jobs_with_a_wait_between. The execution runs 4 invocations, so the earlier nametwo_jobs_in_two_invocationswas inaccurate.README.md: the JavaScript redirect sentence now states the parity.src/.../microvm/request.py: thesend_jobdocstring now states that neither operation follows a redirect. It no longer says that the JavaScriptfetchfollows one.Testing
session.pyandoperation.pyfrom before fix(extras): record the session resume mode #811, at the GetMicrovm count: 1 instead of 3, and 1 instead of 0.hatch run dev-extras:cov: 279 passed, 100 % coverage.hatch run dev-extras:typecheckandhatch fmt --checkpass. The commit lint passes for all four commits.