fix(scheduler): try PUT before GET for schedule pause/resume (fixes #101) - #107
fix(scheduler): try PUT before GET for schedule pause/resume (fixes #101)#107ambiorix2099 wants to merge 2 commits into
Conversation
`conductor schedule pause` and `conductor schedule resume` fail against OSS Conductor with 405 "Request method 'GET' is not supported". The conductor-go SDK issues GET for both (v1.8.0 api_scheduler_resource.go:188 and :255), but upstream OSS declares them @PutMapping. The accepted verb varies by deployment, so neither verb alone is correct: 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 ("accept PUT (not just GET) for per-schedule pause/resume", 2026-07-14), so deployments older than that still need GET. This tries PUT first and falls back to GET only on a 4xx, which satisfies all three and converges on the RESTful verb as older builds age out — at which point the fallback can be deleted. The fallback is deliberately limited to 4xx. Retrying a 5xx or a transport error with a different verb would mask the real fault and report a misleading "method not supported" instead. Implemented in the CLI rather than the SDK on purpose: hardcoding PUT in conductor-go would fix OSS and break every Orkes deployment predating the change. It bypasses SchedulerClient.PauseSchedule/ResumeSchedule via the shared APIClient, which already exposes both verbs, so no SDK bump is needed to unblock the release. Adds internal.GetAPIClient for that purpose, documented as a last resort for when a typed SDK client issues the wrong request. Tests cover all three deployment shapes plus both-rejected, using stub servers that record the verbs received — asserting not just success but that the client converges on PUT and only falls back when forced. Also covers the 5xx no-fallback rule and path escaping. Verified end to end against Conductor OSS built from main: pause sets "paused": true, resume clears it, both exit 0. Previously both returned 405. Fixes #101 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mp-orkes
left a comment
mp-orkes
left a comment
There was a problem hiding this comment.
The correct fix for this is in the SDK.
Review feedback from @kowser-orkes on #107. Narrows the fallback trigger from any 4xx to exactly 405. 405 is the only status that means "wrong verb"; other 4xx have distinct causes and retrying them with GET was wrong: 404 the schedule, or the whole scheduler module, is absent. The retry produced a second 404 and reported that, masking the actionable hint added in #86. 401 auth failure. The retry simply repeated the rejection. Confirmed against OSS Conductor: wrong verb returns 405, a missing schedule and a missing endpoint both return 404 — so the previous range genuinely conflated them. `conductor schedule pause does_not_exist` now surfaces the scheduler-module hint again instead of a fallback-induced error. Also trims the doc comment to the three lines that carry information a reader cannot get from the code. Tests gain 404 and 401 cases asserting no fallback is attempted, alongside the existing 405-falls-back and 5xx-does-not cases. Re-verified end to end against Conductor OSS built from main: pause sets "paused": true, resume clears it. 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>
|
Both addressed in 67c8300 — thanks, the 405 point was a real correctness fix, not just tidying. Narrowed to 405. You were right that the 4xx range was too broad. Checked what OSS actually returns:
So the range conflated "wrong verb" with "not found". Concretely, Added tests asserting 404 and 401 do not trigger a fallback, alongside the existing 405-does and 5xx-does-not cases. Comment trimmed to three lines. Also applied the same 405 narrowing to the SDK PR (conductor-oss/go-sdk#276) so the two implementations don't diverge — see the note below on which one we keep. On placement: @mp-orkes asked for this to live in the SDK instead, and he's right — I've opened conductor-oss/go-sdk#276 with the same logic in This PR is therefore likely to be closed in favour of a |
| // setSchedulePaused pauses or resumes a schedule. OSS Conductor accepts only PUT on | ||
| // these endpoints, older Orkes deployments only GET, so try PUT and fall back to GET | ||
| // on 405. Any other status is returned as-is. | ||
| func setSchedulePaused(ctx context.Context, name, action string) error { |
There was a problem hiding this comment.
if you are making these changes conductor-oss/go-sdk#276, let's go with that instead of setSchedulePaused.
|
@mp-orkes I'd like your call rather than guess. So the fix definitely lands in the SDK, which is in this Go SDK PR #276. The open question is whether this CLI PR should be closed or kept as a fallback. |
@ambiorix2099 tell your agent, I want a human reply. |
We need a CLI PR to update the SDK and make the scheduler tests run in OSS, not skip. |
5 participants
Pull Request type
NOTE:
./gradlew spotlessApplyis not applicable — this repository is Go.go build,go vetandgo test ./...pass, and the three files touched here aregofmtclean.Changes in this PR
conductor schedule pauseandconductor schedule resumeare completely broken against OSSConductor:
The conductor-go SDK issues
GETfor both operations (v1.8.0,api_scheduler_resource.go:188and:255), but upstream OSS declares them@PutMapping.Neither verb alone is correct, because the accepted method varies by deployment:
Orkes gained
PUTin orkes-conductor1854375f0c("accept PUT (not just GET) for per-schedulepause/resume", 2026-07-14), so deployments older than that still require
GET.This tries
PUTfirst and falls back toGETonly on a 4xx. That satisfies all three rows andconverges on the RESTful verb as older Orkes builds age out, at which point the fallback can be
deleted.
The 4xx restriction is deliberate: retrying a 5xx or a transport error with a different verb would
mask the real fault and surface a misleading "method not supported" in its place.
Why in the CLI rather than the SDK. Hardcoding
a.Putin conductor-go would fix OSS and breakevery Orkes deployment predating 2026-07-14. Driving the endpoint from the CLI also means the
release is not gated on an SDK release cycle. The implementation bypasses
SchedulerClient.PauseSchedule/ResumeScheduleand uses the sharedAPIClient, which alreadyexposes both verbs. Adds
internal.GetAPIClient, documented as a last resort for when a typed SDKclient issues the wrong request.
Testing. Stub servers model all three deployment shapes plus both-rejected, recording the verbs
received — so the tests assert not merely that the call succeeds but that the client converges on
PUTand falls back only when forced. Also covered: the 5xx no-fallback rule, and path escaping forschedule names.
Verified end to end against Conductor OSS built from
conductor-oss/conductormain:pausesets"paused": true,resumeclears it, both exit 0. Both previously returned 405.Issue #
Fixes #101.
Found during release validation of the CLI against a server built from
conductor-oss/conductormain. Note the defect is invisible to current CI, which runs only against Orkes — where
GETisaccepted.
test/e2e/schedule.batstests 15 and 16 already cover pause/resume and pass on Orkes;they will exercise this path on both venues once #106 adds an OSS job.
Alternatives considered
Fix
a.Get→a.Putin conductor-go. The tidiest change (two lines) and the right long-termend state, but it would break Orkes deployments older than 2026-07-14, and it gates this release on
an SDK release. Worth doing upstream once no supported Orkes build predates PUT — the CLI fallback
can then be removed.
Send GET first, PUT on failure. Symmetric, but it keeps the legacy verb as the default forever
and never converges. PUT-first means the fallback becomes dead code we can delete.
Branch on
--server-type. Rejected: server type is OSS-vs-Enterprise, which is not the axisthat determines the verb — a recent Orkes build accepts PUT while an old one does not, and both
report the same server type. Probing the actual response is correct where a config flag would guess.
Fall back on any error, not just 4xx. Simpler, but a 500 or a dropped connection would be
retried as GET and reported as a verb problem, hiding the real failure.
🤖 Generated with Claude Code