Repository navigation
fix(extras): stop following redirects in the MicroVM request step - #1036
Conversation
sendJob called fetch with the default redirect mode, "follow". For a 301, 302, or 303, fetch resends the POST as a GET without the job, and a 2xx for that GET counted as accepted. So the job never started and the operation waited until its heartbeat timeout. A redirect to another host also received the auth token headers. The request now uses redirect "manual", and any 3xx fails the delivery at once with MicrovmRequestRejectedError, naming the Location in the message. This matches the Python SDK, see aws/aws-durable-execution-sdk-python#776. The README documents the behavior.
ParidelPooya
left a comment
There was a problem hiding this comment.
Thanks for the fix. It closes #1001: on main a 302 turned the job into a GET and a 307 sent the token to another host, and with this PR both fail at once. The new tests fail on main too. A few changes, inline below.
Also, please add a line under [Unreleased] in CHANGELOG.md. A 307 or 308 for a missing trailing slash used to deliver the job and now fails, so callers need to set the path with its trailing slash.
A custom fetch that does not pass init.redirect on still followed a redirect, so a 302 resolved with the GET's 200 and a 307 to another origin received the auth token. The request step now fails when response.redirected is true, before the status checks, and names response.url. The fetch docstring says a custom fetch must honor init.redirect. The Location in the message, set by the server and checkpointed with the step failure, now drops its query and fragment and is cut to 200 characters. A 3xx without a Location, such as a 304, names only the status. CHANGELOG notes that a 307 or 308 for a missing trailing slash now fails.
|
Thanks, all four are in 23a4bfd. A redirect that a custom |
ParidelPooya
left a comment
There was a problem hiding this comment.
Thanks, all four changes look good. I reran the redirect probes on 23a4bfd: every 3xx fails at once with one POST, a followed redirect from a custom fetch now fails instead of resolving, a 3xx without Location names only the status, and a 15,000-character Location gives a 356-character message. Removing the response.redirected check, the query strip, or the length cap each fails a test.
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): 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.
* 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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Fixes #1001
Description
sendJobcalledfetchwith the defaultredirect: "follow". For a 301, 302 or 303, fetch resent the job as a GET without the body, and the GET's 2xx counted as accepted, so the job never started. A redirect to another host also received the auth token.This change passes
redirect: "manual"and fails any 3xx at once withMicrovmRequestRejectedError, naming theLocation, which mirrors the Python SDK's aws/aws-durable-execution-sdk-python#776. A 307 or 308 for a missing trailing slash, which used to work, now fails with that message. The README notes the behavior.Demo/Screenshots
N/A
Checklist
Testing
Unit Tests
Yes. New tests cover 301, 302, 303, 307 and 308 with and without a
Location, plus one real fetch against a local server; they fail onmain, and the extras suite, typecheck and Biome pass.Integration Tests
No, the unit tests cover it.
Examples
Not applicable.