feat: PortalTaskArchive polls and caches instrument capabilities (§A.4) - #868
Merged
Merged
Conversation
PortalTaskArchive.get_instrument_capabilities() now actually returns live data instead of inheriting TaskArchive's None default -- the last piece of the plan's §A. A new _poll_instrument_capabilities(), called from the existing _poll() alongside the tasks/projects check, re-downloads GET /api/instruments/ only when pyobs-portal#144's last_instrument_update marker moves, using the same marker-gated pattern _poll()/_update() already use for tasks/projects (pyobs-portal#84). Kept deliberately independent of the tasks/projects poll: a failure fetching or parsing instrument capabilities (portal unreachable, or an extra="forbid" rejection on a payload shape pyobs-core doesn't recognize) is caught and throttled locally, keeps serving the last-good InstrumentCapabilities rather than clearing it, and never blocks or forces a retry of the tasks/projects side -- resolves both open review notes from pyobs-core#864-867 (the extra="forbid" conflict, and pagination -- reuses the existing http_request_paginated helper already used for tasks/ projects, so no truncation risk). specs/plans/2026-09-01-instrument-capability-duration-estimates.md's §A is now fully landed end-to-end in pyobs-core. Real capability data only actually reaches the scheduler once pyobs-portal#144 (marker + schema.py wiring, merged) and this are both deployed together with a PortalTaskArchive-backed scheduler module. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
thusser
commented
Sep 3, 2026
thusser
left a comment
Member
Author
There was a problem hiding this comment.
Review: approve — clean, faithful §A.4 implementation
This genuinely resolves both review notes raised on #864/#867 (the extra="forbid" hard-fail risk and the >100-instrument pagination truncation), with regression tests for each. No blockers.
Verified
- Both #864/#867 review notes are resolved in code + tests. The unparseable-payload regression test (
[{"some_unrecognized_future_field": 1}]) proves anextra="forbid"rejection degrades inside the poll's own try/except — last-good cache kept, marker not advanced — rather than propagating. Pagination useshttp_request_paginated(..., strict=True), the same all-pages + no-silent-truncation helper as tasks/projects. - Failure-handling details are correct.
except Exceptiondoesn't swallowCancelledError(BaseException); the throttleclear("instrument_capabilities")fires on any successful marker check (even when no download is needed), so a recovered streak resets; the marker advances only after a successful parse, so a mid-way failure retries next poll;_instrument_capabilitiesis never cleared on failure — the last-good object keeps being served. - Consistent with the existing pattern.
_poll_instrument_capabilitiesmirrors_poll's marker-gating and_check_for_changes'LogThrottlekey/clear idiom; naming (_instrument_capabilities/_instrument_capabilities_marker) mirrors_tasks/_last_marker; theget_instrument_capabilities()override matches the base'sInstrumentCapabilities | Nonesignature (pyrefly confirms). - Contract matches the merged portal side. The URL
/api/instruments/last_instrument_update/and theTime(res["last_instrument_update"])epoch-safe parse match pyobs-portal#144 as merged (confirmedMERGED— the status claim in the PR body is accurate). - Tooling/tests: 59/59 portal-archive tests pass (51 existing + 8 new, all well-targeted: first-poll download, unchanged-marker skip, moved-marker re-download, three keep-last-good failure modes, and the
_poll()wiring test); ruff/black/pyrefly clean (0 errors). Full suite: 1888 passed here with only the 7 known environmental failures (sunpy config dir +/opt/pyobsread-only in this sandbox), consistent with the claimed 1895 in a healthy environment.
Notes (minor, non-blocking)
- Strictly-greater marker comparison misses backward marker moves. If the admin deletes the row holding the current
max(updated_at), the marker decreases and this poll never notices — the cached capabilities keep the removed device until some later edit pushes the marker forward. This exactly mirrors the pre-existing tasks-marker semantics (and the impact is estimate-only data that self-heals on the next edit), so it's fine — but!=(with the existingNonefirst-poll check) would also catch deletions at zero cost if you want instruments to be deletion-aware where tasks aren't. - Poll ordering coupling (asymmetric).
_poll_instrument_capabilities()runs after the tasks block, so a tasks-side exception skips the instrument attempt for that cycle. In practice both fail together (portal down), and the docstring's promise — instrument failures never block/retry the tasks side — holds. Just noting the reverse direction isn't also isolated. - Persistent unparseable payload = full re-download every 5s. When the marker has advanced but parsing keeps failing (e.g. the portal deployed a new serializer field before this side catches up), each
_pollcycle re-downloads the entire instrument list and fails again — same cadence and no backoff beyondhttp_request_with_retries' internal retries and the log throttle. Matches the tasks-side pattern; acceptable, but a short failure backoff would be a nice future hardening.
Summary
This completes §A end-to-end (models → plumbing → leaf scripts → portal fetch), the degradation semantics are exactly what the plan's "optional/degrade-to-None" convention calls for, and the two open review notes from the earlier PRs in this series are closed with genuine regression tests. Merge as-is.
…ward pyobs-core#868's review: a strict > comparison misses a marker moving backward (e.g. deleting the row that held the current max(updated_at)), silently leaving a removed device cached indefinitely until some later edit happens to push the marker forward again. != catches both directions at no extra cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
thusser
added a commit
that referenced
this pull request
Sep 3, 2026
…emented §A landed across pyobs-core#864/#865/#867/#868, released as pyobs-core v2.4.0. §B landed in pyobs-portal#144/#145 (dependency bumped, real path now exercised end-to-end, 0 skipped). All test-plan items checked off except the manual script-builder browser click-through, left open -- automated coverage exercises the same code path but nobody's watched it in a browser yet. specs/plans/index.md's entry marked implemented; fleet-open-items.md's plan entries removed per its own maintenance rule (implemented items don't stay, they get removed outright), #858/#859's dangling "plan below" reference reworded since the plan itself is no longer listed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
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
PortalTaskArchive.get_instrument_capabilities()now actually returns live data instead of inheritingTaskArchive'sNonedefault — the last piece ofspecs/plans/2026-09-01-instrument-capability-duration-estimates.md's §A._poll_instrument_capabilities(), called from the existing_poll()alongside the tasks/projects check: re-downloadsGET /api/instruments/only when pyobs-portal#144'slast_instrument_updatemarker moves, using the same marker-gated pattern_poll()/_update()already use for tasks/projects (pyobs-portal#84).InstrumentCapabilitiesrather than clearing it, and never blocks or forces a retry of the tasks/projects side.Resolves both open review notes from #864/#867
extra="forbid"conflict: a portal payload the pyobs-core models don't recognize now raises inside_poll_instrument_capabilities()'s own try/except, degrading like any other fetch failure rather than propagating.http_request_paginatedhelper already used for tasks/projects, so no truncation risk on a fleet with >100 instruments.Status of the plan
§A is now fully landed end-to-end in pyobs-core (#864, #865, #867, this PR). Real capability data only actually reaches the scheduler once pyobs-portal#144 (merged) and this PR are both deployed together with a
PortalTaskArchive-backed scheduler module — and pyobs-portal's ownpyobs-coredependency needs bumping to a release containing #864-867 first (see #144's review).Test plan
get_instrument_capabilities()default,_poll()actually calls the new methodpytest tests/robotic/storage/portal/— 59/59 passing (51 existing + 8 new)pytest tests/(excludingintegration/xmpp) — 1895 passed, no regressionsruff check/black --check/pyrefly check— clean🤖 Generated with Claude Code
https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE