fix(scheduler): PUT with GET fallback for per-schedule pause/resume - #276
Open
ambiorix2099 wants to merge 3 commits into
Open
fix(scheduler): PUT with GET fallback for per-schedule pause/resume#276ambiorix2099 wants to merge 3 commits into
ambiorix2099 wants to merge 3 commits into
Conversation
PauseSchedule and ResumeSchedule issue a GET, but upstream OSS Conductor declares both endpoints @PutMapping, so every call against an OSS server fails with 405 "Request method 'GET' is not supported". A straight GET -> PUT swap is not safe, because the accepted verb varies by deployment: OSS Conductor PUT only (GET -> 405) Orkes Conductor >= 2026-07-14 PUT or GET Orkes Conductor < 2026-07-14 GET only (PUT -> 405) Orkes gained PUT in orkes-conductor 1854375f0c (2026-07-14); anything older still needs GET. Both methods now go through putThenGet, which issues PUT and falls back to GET only when the server refuses with a 4xx. That keeps existing callers working against every deployment while converging on the RESTful verb, so the fallback can be deleted once no supported server predates PUT. The fallback is limited to 4xx on purpose: retrying a 5xx or a transport error with a different verb would mask the real fault and report it as a method problem. Tests model all three deployment generations plus neither-verb-accepted with stub servers that record the verbs received, asserting that PUT is preferred and GET is used only when PUT is refused. Also covers the 5xx no-fallback rule and that resume targets its own endpoint. Reported downstream as conductor-oss/conductor-cli#101. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to review feedback on the downstream CLI change (conductor-oss/conductor-cli#107), applied here so the two implementations do not diverge. 405 is the only status that means "wrong verb". The previous 4xx range also caught statuses with unrelated causes, and retrying those with GET was wrong: 404 the schedule, or the whole scheduler module, is absent — the retry produced a second 404 and reported that instead of the real cause 401 auth failure — the retry simply repeated the rejection Verified against OSS Conductor: a wrong verb returns 405, while a missing schedule and a missing endpoint both return 404, so the range genuinely conflated them. Tests gain 404 and 401 cases asserting no fallback is attempted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
mp-orkes
reviewed
Aug 5, 2026
| return a.putThenGet(ctx, path) | ||
| } | ||
|
|
||
| // putThenGet issues a PUT and falls back to a GET if the server rejects it with a |
Contributor
There was a problem hiding this comment.
This documentation is wrong
putThenGet issues a PUT and falls back to a GET if the server rejects it with a 4xx - it's just 405.
mp-orkes
reviewed
Aug 5, 2026
| } | ||
|
|
||
| // putThenGet issues a PUT and falls back to a GET if the server rejects it with a | ||
| // 4xx. It exists because the verb accepted for per-schedule pause/resume differs by |
Contributor
There was a problem hiding this comment.
Please trim the documentation down.
mp-orkes
approved these changes
Aug 5, 2026
mp-orkes
left a comment
mp-orkes
left a comment
Contributor
There was a problem hiding this comment.
Overall LGTM but LLMs are too verbose. Please read the documentation and rephrase it. Make it simpler and remove what's not relevant. E.g.: Why even mention @PutMapping in Go Code. All we need to know is it's a PUT.
Review feedback from @mp-orkes on #276. The doc comment still said the fallback triggers on "4xx" after the code was narrowed to 405 — the earlier commit updated the later paragraphs and missed the opening sentence, so the documentation contradicted the implementation. Fixed. Trimmed the surrounding commentary: dropped the deployment table, the @PutMapping reference (irrelevant in Go — that it is a PUT is the only part that matters), and the in-function comment that restated the doc comment. Same for the test file, where the per-case notes repeated what the case names already said. Net -27 lines of comment with no change in behaviour; tests unchanged and passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Open
6 tasks
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
2 participants
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.
Problem
PauseScheduleandResumeSchedulesendGET, but OSS Conductor only acceptsPUTon bothendpoints. Every call against an OSS server fails:
Swapping
GETforPUTis not enough — the accepted verb depends on the deployment:Orkes only added
PUTon 2026-07-14 (orkes-conductor1854375f0c), so hardcodingPUTwould fixOSS and break older Orkes.
Change
Both methods now go through
putThenGet: sendPUT, fall back toGETonly on a 405. That coversall three rows and keeps existing callers unchanged.
Fallback is limited to 405 on purpose. Any other status means something other than the verb is
wrong, and retrying it would report a method problem instead of the real cause.
Tests
Stub servers cover all three deployments plus a server that accepts neither verb, and record which
verb they received — so the tests check that the client prefers
PUT, not just that the callsucceeds. Also covered: 404, 401 and 500 do not trigger the fallback, and
resumehits its ownendpoint.
How to test
Built the Conductor CLI unmodified against this branch, ran it against OSS Conductor from
main:Both returned 405 before. No CLI change was needed.
Reported as conductor-oss/conductor-cli#101. Not caught by that repo's CI, which only runs against
Orkes.