Skip to content

feat(schedulers): expose scheduler status and controls - #27

Draft
lecodev-26 wants to merge 3 commits into
Basekick-Labs:mainfrom
lecodev-26:feat/scheduler-management-18
Draft

lecodev-26 wants to merge 3 commits into
Basekick-Labs:mainfrom
lecodev-26:feat/scheduler-management-18

Conversation

@lecodev-26

Copy link
Copy Markdown

Summary

  • Add synchronous and asynchronous client.schedulers sub-clients.
  • Expose scheduler status, continuous-query schedule reload, and immediate retention trigger.
  • Require confirm=True before issuing a retention trigger because it may permanently delete data.
  • Preserve Arc's HTTP authorization, enterprise-license, and server errors.
  • Document the API and add sync/async HTTP contract coverage.

Part of #18 (scheduler API parity). This is one independently reviewable slice; it does not close the broader tracking issue.

Compatibility

The three routes are present in Arc v26.09.3 (e83bb1e) and current main. The SDK forwards the server's administrator and enterprise-feature gates; it does not bypass them.

Verification

  • Static review: git diff --cached --check passed.
  • Manual Test: not performed.
  • IA Test: not performed.
  • Automated tests: added in tests/unit/http/test_schedulers.py; not run locally. Please rely on GitHub Actions for dynamic validation.

No tests, builds, compilation, or benchmarks were run in Termux.

@xe-nvdk xe-nvdk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked out and ran locally: 440 passed / 9 skipped, mypy clean, ruff format clean, but ruff check fails: SIM117 (nested with) in tests/unit/http/test_schedulers.py:65. CI will fail once the run is approved.

One behavioural issue: without an enterprise license, Arc answers POST /schedulers/cq/reload and /retention/trigger with 503 "... scheduler is not running - enterprise license required" (internal/api/scheduler.go handleCQReload / handleRetentionTrigger). The SDK maps that to a generic ArcServerError, which reads as a transient fault. Please map it to ArcLicenseError in the scheduler client (match on the 503 + "license required" message) and add a test for it; I'll also raise it with Arc, since 403 would be the right status.

Other notes:

  • The confirm=True guard on trigger_retention matches databases.delete(confirm=...). Good.
  • Raw dict returns: same note as #28 — fine for operator surfaces, but document it.
  • Conflicts with #28 at the same lines in client.py/async_client.py/README/CHANGELOG.

@xe-nvdk

xe-nvdk commented Nov 2, 2026

Copy link
Copy Markdown
Member

#29 just landed on main: the 0.2.0 parity work, including a new client.diagnostics and changes to client.py, async_client.py, README and CHANGELOG. Please rebase onto main and resolve the conflicts. Things to know:

  • tests/conftest.py now has an autouse fixture that isolates the installation-id file, so nothing is needed on your side for it.
  • Typed ArcErrors should pass through unchanged, the way the query and write clients now do. Wrapping is only for non-Arc exceptions, which your _json helper already does.
  • The CHANGELOG Added section has grown; put your entry next to the other operator surfaces.
  • After rebasing, please run uv run pytest tests/unit -q, uv run ruff check ., uv run ruff format --check . and uv run mypy src/arc_client --ignore-missing-imports locally. The earlier review found one lint failure in each PR, which CI would have caught. If you can't run them, say so and I'll run them on the rebased branch.

Copy link
Copy Markdown
Author

Thanks for the detailed review. I’ve rebased this branch onto the current main (including #29) and addressed the findings:

  • Flattened the nested context managers flagged by SIM117.
  • Added shared HTTP error mapping so a scheduler-control HTTP 503 whose message contains license required raises ArcLicenseError rather than a generic ArcServerError; generic scheduler 503 responses remain ArcServerError.
  • Added sync/async regression coverage for the license-specific 503 response.
  • Documented that scheduler methods return raw, server-defined JSON.
  • Updated the Arc compatibility wording to “26.09.3 and later” and resolved the overlapping README/CHANGELOG/client changes from feat(sdk): 0.2.0 parity with Arc main + opt-out installation telemetry #29.

git diff --check and conflict-marker/line-length checks pass. I have not run tests or builds in Termux; uv/ruff are not installed in that environment. Please run the requested unit, lint, format and mypy checks in CI or on your side.

@xe-nvdk xe-nvdk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick rebase. I merged this onto current main and ran it against an Arc main build (92a1f9d1) on three single nodes: licensed, licensed with continuous_query.enabled=false and retention.enabled=false, and unlicensed. Admin gating (403 for a read token), authentication (401 without a token), status(), reload_continuous_queries() and trigger_retention(confirm=True) all behave as documented. The trigger really ran a policy: 3 old rows deleted, 1 newer row kept.

I'm reversing the 503 request from our earlier review. We asked you to map Arc's 503 "license required" to ArcLicenseError. The live run shows Arc returns that exact message on a node that is licensed but has the scheduler disabled in config:

node status() reason reload_continuous_queries() with this PR
licensed, CQ disabled CQ feature not enabled (continuous_query.enabled=false) ArcLicenseError: Enterprise license required ❌
unlicensed Enterprise license required ArcLicenseError ✅

So the mapping turns Arc's misleading message into a typed exception that blames the license. I filed the Arc side as Basekick-Labs/arc#1433. Please:

  1. Revert the base.py change and the CHANGELOG wording (403/503 back to 403), and let the 503 surface as ArcServerError (status 503). In the two control methods' docstrings, say that a 503 means the scheduler is not running and that status() gives the reason. When Arc fixes #1433 with a 403 for a missing license, the existing 403 license detection will raise ArcLicenseError with no SDK change. Turn test_scheduler_license_required_503_maps_to_license_error into a test asserting ArcServerError with status_code == 503.
  2. trigger_retention blocks until every active policy has run (RetentionScheduler.TriggerNow is synchronous), so a real run can outlast the default 30 s timeout. Give it the same treatment as delete.delete: post with timeout=max(self._config.timeout, DELETE_TIMEOUT_FLOOR) (from arc_client.management.delete). Also document that in a cluster it must go to the primary writer: other nodes answer 500 "node role is not primary writer", and a cycle already running also answers 500 (both are in arc#1433).
  3. CI (I approved the workflow run, so you'll see it):
    • ruff check: SIM117 at tests/unit/http/test_schedulers.py:117.
    • ruff format: src/arc_client/http/base.py and tests/unit/http/test_schedulers.py. (base.py goes away with point 1.)
    • One failing test: test_retention_trigger_requires_explicit_confirmation[True] combines async with AsyncArcClient(...) as client, pytest.raises(...), and pytest.raises is not an async context manager. Nest the with pytest.raises inside the async with; ruff accepts that form for mixed async/sync context managers.

The design is right: raw dicts, confirm=True on the destructive call, lazy sub-client, sync/async parity. Whichever of #27/#28 merges second will need a small rebase, since both add lines at the same spots in client.py, async_client.py and management/__init__.py.

@lecodev-26
lecodev-26 force-pushed the feat/scheduler-management-18 branch from a4dbca4 to 5461967 Compare November 2, 2026 21:24

Copy link
Copy Markdown
Author

Thanks for the live validation and for correcting the earlier 503 guidance. I’ve updated the PR and pushed the fixes on top of current upstream main (358f9e9, including #32):

  • Reverted the HTTP 503-to-ArcLicenseError mapping in http/base.py; 503 now remains ArcServerError with status_code == 503. The CHANGELOG again documents ArcLicenseError as HTTP 403 only.
  • Updated both scheduler-control docstrings: HTTP 503 means the scheduler is not running, and status() gives the reason; missing licensed features are described as HTTP 403.
  • Added the same timeout floor used by delete.delete to sync and async trigger_retention: max(self._config.timeout, DELETE_TIMEOUT_FLOOR).
  • Documented that retention must be triggered on the primary writer in a cluster, and that a non-primary node or an already-running cycle can return HTTP 500.
  • Updated the regression test to assert that the misleading “enterprise license required” HTTP 503 remains an ArcServerError; corrected the async confirmation test's mixed context managers and the sync regression test's context layout.

Static git diff --check and the changed-file line-length review pass. I have not run tests, builds, compilation, benchmarks, or equivalent dynamic processes in Termux; ruff and uv are not installed there. Please run the unit, Ruff, format, and mypy checks in CI/on your side and let me know if anything else needs adjustment.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants